From 06de72a355b1ea838712faae08908cb068176055 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Thu, 5 Feb 2026 16:44:50 -0600 Subject: [PATCH] fix(#338): Implement proper system admin role separate from workspace ownership - Replace workspace ownership check with explicit SYSTEM_ADMIN_IDS env var - System admin access is now explicit and configurable via environment - Workspace owners no longer automatically get system admin privileges - Add 15 unit tests verifying security separation - Add SYSTEM_ADMIN_IDS documentation to .env.example Refs #338 Co-Authored-By: Claude Opus 4.5 --- apps/api/.env.example | 6 + apps/api/src/auth/guards/admin.guard.spec.ts | 170 +++++++++++++++++++ apps/api/src/auth/guards/admin.guard.ts | 50 ++++-- 3 files changed, 214 insertions(+), 12 deletions(-) create mode 100644 apps/api/src/auth/guards/admin.guard.spec.ts diff --git a/apps/api/.env.example b/apps/api/.env.example index 8fef7fd..fe6c8dd 100644 --- a/apps/api/.env.example +++ b/apps/api/.env.example @@ -1,6 +1,12 @@ # Database DATABASE_URL=postgresql://user:password@localhost:5432/database +# System Administration +# Comma-separated list of user IDs that have system administrator privileges +# These users can perform system-level operations across all workspaces +# Note: Workspace ownership does NOT grant system admin access +# SYSTEM_ADMIN_IDS=uuid1,uuid2,uuid3 + # Federation Instance Identity # Display name for this Mosaic instance INSTANCE_NAME=Mosaic Instance diff --git a/apps/api/src/auth/guards/admin.guard.spec.ts b/apps/api/src/auth/guards/admin.guard.spec.ts new file mode 100644 index 0000000..7b06eb7 --- /dev/null +++ b/apps/api/src/auth/guards/admin.guard.spec.ts @@ -0,0 +1,170 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { ExecutionContext, ForbiddenException } from "@nestjs/common"; +import { AdminGuard } from "./admin.guard"; + +describe("AdminGuard", () => { + const originalEnv = process.env.SYSTEM_ADMIN_IDS; + + afterEach(() => { + // Restore original environment + if (originalEnv !== undefined) { + process.env.SYSTEM_ADMIN_IDS = originalEnv; + } else { + delete process.env.SYSTEM_ADMIN_IDS; + } + vi.clearAllMocks(); + }); + + const createMockExecutionContext = (user: { id: string } | undefined): ExecutionContext => { + const mockRequest = { + user, + }; + + return { + switchToHttp: () => ({ + getRequest: () => mockRequest, + }), + } as ExecutionContext; + }; + + describe("constructor", () => { + it("should parse system admin IDs from environment variable", () => { + process.env.SYSTEM_ADMIN_IDS = "admin-1,admin-2,admin-3"; + const guard = new AdminGuard(); + + expect(guard.isSystemAdmin("admin-1")).toBe(true); + expect(guard.isSystemAdmin("admin-2")).toBe(true); + expect(guard.isSystemAdmin("admin-3")).toBe(true); + }); + + it("should handle whitespace in admin IDs", () => { + process.env.SYSTEM_ADMIN_IDS = " admin-1 , admin-2 , admin-3 "; + const guard = new AdminGuard(); + + expect(guard.isSystemAdmin("admin-1")).toBe(true); + expect(guard.isSystemAdmin("admin-2")).toBe(true); + expect(guard.isSystemAdmin("admin-3")).toBe(true); + }); + + it("should handle empty environment variable", () => { + process.env.SYSTEM_ADMIN_IDS = ""; + const guard = new AdminGuard(); + + expect(guard.isSystemAdmin("any-user")).toBe(false); + }); + + it("should handle missing environment variable", () => { + delete process.env.SYSTEM_ADMIN_IDS; + const guard = new AdminGuard(); + + expect(guard.isSystemAdmin("any-user")).toBe(false); + }); + + it("should handle single admin ID", () => { + process.env.SYSTEM_ADMIN_IDS = "single-admin"; + const guard = new AdminGuard(); + + expect(guard.isSystemAdmin("single-admin")).toBe(true); + }); + }); + + describe("isSystemAdmin", () => { + let guard: AdminGuard; + + beforeEach(() => { + process.env.SYSTEM_ADMIN_IDS = "admin-uuid-1,admin-uuid-2"; + guard = new AdminGuard(); + }); + + it("should return true for configured system admin", () => { + expect(guard.isSystemAdmin("admin-uuid-1")).toBe(true); + expect(guard.isSystemAdmin("admin-uuid-2")).toBe(true); + }); + + it("should return false for non-admin user", () => { + expect(guard.isSystemAdmin("regular-user-id")).toBe(false); + }); + + it("should return false for empty string", () => { + expect(guard.isSystemAdmin("")).toBe(false); + }); + }); + + describe("canActivate", () => { + let guard: AdminGuard; + + beforeEach(() => { + process.env.SYSTEM_ADMIN_IDS = "admin-uuid-1,admin-uuid-2"; + guard = new AdminGuard(); + }); + + it("should return true for system admin user", () => { + const context = createMockExecutionContext({ id: "admin-uuid-1" }); + + const result = guard.canActivate(context); + + expect(result).toBe(true); + }); + + it("should throw ForbiddenException for non-admin user", () => { + const context = createMockExecutionContext({ id: "regular-user-id" }); + + expect(() => guard.canActivate(context)).toThrow(ForbiddenException); + expect(() => guard.canActivate(context)).toThrow( + "This operation requires system administrator privileges" + ); + }); + + it("should throw ForbiddenException when user is not authenticated", () => { + const context = createMockExecutionContext(undefined); + + expect(() => guard.canActivate(context)).toThrow(ForbiddenException); + expect(() => guard.canActivate(context)).toThrow("User not authenticated"); + }); + + it("should NOT grant admin access based on workspace ownership", () => { + // This test verifies that workspace ownership alone does not grant admin access + // The user must be explicitly listed in SYSTEM_ADMIN_IDS + const workspaceOwnerButNotSystemAdmin = { id: "workspace-owner-id" }; + const context = createMockExecutionContext(workspaceOwnerButNotSystemAdmin); + + expect(() => guard.canActivate(context)).toThrow(ForbiddenException); + expect(() => guard.canActivate(context)).toThrow( + "This operation requires system administrator privileges" + ); + }); + + it("should deny access when no system admins are configured", () => { + process.env.SYSTEM_ADMIN_IDS = ""; + const guardWithNoAdmins = new AdminGuard(); + + const context = createMockExecutionContext({ id: "any-user-id" }); + + expect(() => guardWithNoAdmins.canActivate(context)).toThrow(ForbiddenException); + }); + }); + + describe("security: workspace ownership vs system admin", () => { + it("should require explicit system admin configuration, not implicit workspace ownership", () => { + // Setup: user is NOT in SYSTEM_ADMIN_IDS + process.env.SYSTEM_ADMIN_IDS = "different-admin-id"; + const guard = new AdminGuard(); + + // Even if this user owns workspaces, they should NOT have system admin access + // because they are not in SYSTEM_ADMIN_IDS + const context = createMockExecutionContext({ id: "workspace-owner-user-id" }); + + expect(() => guard.canActivate(context)).toThrow(ForbiddenException); + }); + + it("should grant access only to users explicitly listed as system admins", () => { + const adminUserId = "explicitly-configured-admin"; + process.env.SYSTEM_ADMIN_IDS = adminUserId; + const guard = new AdminGuard(); + + const context = createMockExecutionContext({ id: adminUserId }); + + expect(guard.canActivate(context)).toBe(true); + }); + }); +}); diff --git a/apps/api/src/auth/guards/admin.guard.ts b/apps/api/src/auth/guards/admin.guard.ts index e3c721c..9793e9a 100644 --- a/apps/api/src/auth/guards/admin.guard.ts +++ b/apps/api/src/auth/guards/admin.guard.ts @@ -2,8 +2,14 @@ * Admin Guard * * Restricts access to system-level admin operations. - * Currently checks if user owns at least one workspace (indicating admin status). - * Future: Replace with proper role-based access control (RBAC). + * System administrators are configured via the SYSTEM_ADMIN_IDS environment variable. + * + * Configuration: + * SYSTEM_ADMIN_IDS=uuid1,uuid2,uuid3 (comma-separated list of user IDs) + * + * Note: Workspace ownership does NOT grant system admin access. These are separate concepts: + * - Workspace owner: Can manage their workspace and its members + * - System admin: Can perform system-level operations across all workspaces */ import { @@ -13,16 +19,42 @@ import { ForbiddenException, Logger, } from "@nestjs/common"; -import { PrismaService } from "../../prisma/prisma.service"; import type { AuthenticatedRequest } from "../../common/types/user.types"; @Injectable() export class AdminGuard implements CanActivate { private readonly logger = new Logger(AdminGuard.name); + private readonly systemAdminIds: Set; - constructor(private readonly prisma: PrismaService) {} + constructor() { + // Load system admin IDs from environment variable + const adminIdsEnv = process.env.SYSTEM_ADMIN_IDS ?? ""; + this.systemAdminIds = new Set( + adminIdsEnv + .split(",") + .map((id) => id.trim()) + .filter((id) => id.length > 0) + ); - async canActivate(context: ExecutionContext): Promise { + if (this.systemAdminIds.size === 0) { + this.logger.warn( + "No system administrators configured. Set SYSTEM_ADMIN_IDS environment variable." + ); + } else { + this.logger.log( + `System administrators configured: ${String(this.systemAdminIds.size)} user(s)` + ); + } + } + + /** + * Check if a user ID is a system administrator + */ + isSystemAdmin(userId: string): boolean { + return this.systemAdminIds.has(userId); + } + + canActivate(context: ExecutionContext): boolean { const request = context.switchToHttp().getRequest(); const user = request.user; @@ -30,13 +62,7 @@ export class AdminGuard implements CanActivate { throw new ForbiddenException("User not authenticated"); } - // Check if user owns any workspace (admin indicator) - // TODO: Replace with proper RBAC system admin role check - const ownedWorkspaces = await this.prisma.workspace.count({ - where: { ownerId: user.id }, - }); - - if (ownedWorkspaces === 0) { + if (!this.isSystemAdmin(user.id)) { this.logger.warn(`Non-admin user ${user.id} attempted admin operation`); throw new ForbiddenException("This operation requires system administrator privileges"); }