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 <noreply@anthropic.com>
This commit is contained in:
Jason Woltje
2026-02-05 16:44:50 -06:00
parent 32c81e96cf
commit 06de72a355
3 changed files with 214 additions and 12 deletions

View File

@@ -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);
});
});
});