fix: enhance user context validation in auth module (#1726)

elibosley · web-flow · commit cd5eff11bcb4 · 2025-09-26T12:50:31.000-04:00
Fixes #1723 - Improved error handling in the auth module to ensure user context is present and valid. - Added checks for user roles and identifiers, throwing appropriate exceptions for missing or invalid data. - Introduced a new integration test suite for AuthZGuard, validating role-based access control for various actions in the application. - Tests cover scenarios for viewer and admin roles, ensuring correct permissions are enforced.  ## Summary by CodeRabbit * **Bug Fixes** * Hardened authorization: properly rejects requests with missing users or invalid roles and ensures a valid subject is derived for permission checks, improving reliability and security of access control responses. * **Tests** * Added comprehensive integration tests for authorization, covering admin/viewer role behaviors, API key permissions, and various resource actions to verify expected allow/deny outcomes across scenarios.
diff --git a/api/src/unraid-api/auth/auth.module.ts b/api/src/unraid-api/auth/auth.module.ts
@@ -8,6 +8,7 @@ import { AuthService } from '@app/unraid-api/auth/auth.service.js';
 import { CasbinModule } from '@app/unraid-api/auth/casbin/casbin.module.js';
 import { CasbinService } from '@app/unraid-api/auth/casbin/casbin.service.js';
 import { BASE_POLICY, CASBIN_MODEL } from '@app/unraid-api/auth/casbin/index.js';
+import { resolveSubjectFromUser } from '@app/unraid-api/auth/casbin/resolve-subject.util.js';
 import { CookieService, SESSION_COOKIE_CONFIG } from '@app/unraid-api/auth/cookie.service.js';
 import { UserCookieStrategy } from '@app/unraid-api/auth/cookie.strategy.js';
 import { ServerHeaderStrategy } from '@app/unraid-api/auth/header.strategy.js';
@@ -41,13 +42,7 @@ import { getRequest } from '@app/utils.js';
 
                 try {
                     const request = getRequest(ctx);
-                    const roles = request?.user?.roles || [];
-
-                    if (!Array.isArray(roles)) {
-                        throw new UnauthorizedException('User roles must be an array');
-                    }
-
-                    return roles.join(',');
+                    return resolveSubjectFromUser(request?.user);
                 } catch (error) {
                     logger.error('Failed to extract user context', error);
                     throw new UnauthorizedException('Failed to authenticate user');
diff --git a/api/src/unraid-api/auth/casbin/authz.guard.integration.spec.ts b/api/src/unraid-api/auth/casbin/authz.guard.integration.spec.ts
@@ -0,0 +1,133 @@
+import { ExecutionContext, Type } from '@nestjs/common';
+import { Reflector } from '@nestjs/core';
+import { ExecutionContextHost } from '@nestjs/core/helpers/execution-context-host.js';
+
+import type { Enforcer } from 'casbin';
+import { AuthAction, Resource, Role } from '@unraid/shared/graphql.model.js';
+import { AuthZGuard, BatchApproval } from 'nest-authz';
+import { beforeAll, describe, expect, it } from 'vitest';
+
+import { CasbinService } from '@app/unraid-api/auth/casbin/casbin.service.js';
+import { CASBIN_MODEL } from '@app/unraid-api/auth/casbin/model.js';
+import { BASE_POLICY } from '@app/unraid-api/auth/casbin/policy.js';
+import { resolveSubjectFromUser } from '@app/unraid-api/auth/casbin/resolve-subject.util.js';
+import { DockerMutationsResolver } from '@app/unraid-api/graph/resolvers/docker/docker.mutations.resolver.js';
+import { DockerResolver } from '@app/unraid-api/graph/resolvers/docker/docker.resolver.js';
+import { VmMutationsResolver } from '@app/unraid-api/graph/resolvers/vms/vms.mutations.resolver.js';
+import { MeResolver } from '@app/unraid-api/graph/user/user.resolver.js';
+import { getRequest } from '@app/utils.js';
+
+type Handler = (...args: any[]) => unknown;
+
+type TestUser = {
+    id?: string;
+    roles?: Role[];
+};
+
+type TestRequest = {
+    user?: TestUser;
+};
+
+function createExecutionContext(
+    handler: Handler,
+    classRef: Type<unknown> | null,
+    roles: Role[],
+    userId = 'api-key-viewer'
+): ExecutionContext {
+    const request: TestRequest = {
+        user: {
+            id: userId,
+            roles: [...roles],
+        },
+    };
+
+    const graphqlContextHost = new ExecutionContextHost(
+        [undefined, undefined, { req: request }, undefined],
+        classRef,
+        handler
+    );
+
+    graphqlContextHost.setType('graphql');
+
+    return graphqlContextHost as unknown as ExecutionContext;
+}
+
+describe('AuthZGuard + Casbin policies', () => {
+    let guard: AuthZGuard;
+    let enforcer: Enforcer;
+
+    beforeAll(async () => {
+        const casbinService = new CasbinService();
+        enforcer = await casbinService.initializeEnforcer(CASBIN_MODEL, BASE_POLICY);
+
+        await enforcer.addGroupingPolicy('api-key-viewer', Role.VIEWER);
+        await enforcer.addGroupingPolicy('api-key-admin', Role.ADMIN);
+
+        guard = new AuthZGuard(new Reflector(), enforcer, {
+            enablePossession: false,
+            batchApproval: BatchApproval.ALL,
+            userFromContext: (ctx: ExecutionContext) => {
+                const request = getRequest(ctx) as TestRequest | undefined;
+
+                return resolveSubjectFromUser(request?.user);
+            },
+        });
+    });
+
+    it('denies viewer role from stopping docker containers', async () => {
+        const context = createExecutionContext(
+            DockerMutationsResolver.prototype.stop,
+            DockerMutationsResolver,
+            [Role.VIEWER],
+            'api-key-viewer'
+        );
+
+        await expect(guard.canActivate(context)).resolves.toBe(false);
+    });
+
+    it('allows admin role to stop docker containers', async () => {
+        const context = createExecutionContext(
+            DockerMutationsResolver.prototype.stop,
+            DockerMutationsResolver,
+            [Role.ADMIN],
+            'api-key-admin'
+        );
+
+        await expect(guard.canActivate(context)).resolves.toBe(true);
+    });
+
+    it('denies viewer role from stopping virtual machines', async () => {
+        const context = createExecutionContext(
+            VmMutationsResolver.prototype.stop,
+            VmMutationsResolver,
+            [Role.VIEWER],
+            'api-key-viewer'
+        );
+
+        await expect(guard.canActivate(context)).resolves.toBe(false);
+    });
+
+    it('allows viewer role to read docker data', async () => {
+        const context = createExecutionContext(
+            DockerResolver.prototype.containers,
+            DockerResolver,
+            [Role.VIEWER],
+            'api-key-viewer'
+        );
+
+        await expect(guard.canActivate(context)).resolves.toBe(true);
+    });
+
+    it('allows API key with explicit permission to access ME resource', async () => {
+        await enforcer.addPolicy('api-key-custom', Resource.ME, AuthAction.READ_ANY);
+
+        const context = createExecutionContext(
+            MeResolver.prototype.me,
+            MeResolver,
+            [],
+            'api-key-custom'
+        );
+
+        await expect(guard.canActivate(context)).resolves.toBe(true);
+    });
+});
diff --git a/api/src/unraid-api/auth/casbin/resolve-subject.util.spec.ts b/api/src/unraid-api/auth/casbin/resolve-subject.util.spec.ts
@@ -0,0 +1,43 @@
+import { UnauthorizedException } from '@nestjs/common';
+
+import { describe, expect, it } from 'vitest';
+
+import { resolveSubjectFromUser } from '@app/unraid-api/auth/casbin/resolve-subject.util.js';
+
+describe('resolveSubjectFromUser', () => {
+    it('returns trimmed user id when available', () => {
+        const subject = resolveSubjectFromUser({ id: '  user-123  ', roles: ['viewer'] });
+
+        expect(subject).toBe('user-123');
+    });
+
+    it('falls back to a single non-empty role', () => {
+        const subject = resolveSubjectFromUser({ roles: ['  viewer  '] });
+
+        expect(subject).toBe('viewer');
+    });
+
+    it('throws when role list is empty', () => {
+        expect(() => resolveSubjectFromUser({ roles: [] })).toThrow(UnauthorizedException);
+    });
+
+    it('throws when multiple roles are present', () => {
+        expect(() => resolveSubjectFromUser({ roles: ['viewer', 'admin'] })).toThrow(
+            UnauthorizedException
+        );
+    });
+
+    it('throws when roles is not an array', () => {
+        expect(() => resolveSubjectFromUser({ roles: 'viewer' as unknown })).toThrow(
+            UnauthorizedException
+        );
+    });
+
+    it('throws when role subject is blank', () => {
+        expect(() => resolveSubjectFromUser({ roles: ['  '] })).toThrow(UnauthorizedException);
+    });
+
+    it('throws when user is missing', () => {
+        expect(() => resolveSubjectFromUser(undefined)).toThrow(UnauthorizedException);
+    });
+});
diff --git a/api/src/unraid-api/auth/casbin/resolve-subject.util.ts b/api/src/unraid-api/auth/casbin/resolve-subject.util.ts
@@ -0,0 +1,46 @@
+import { UnauthorizedException } from '@nestjs/common';
+
+type CasbinUser = {
+    id?: unknown;
+    roles?: unknown;
+};
+
+/**
+ * Determine the Casbin subject for a request user.
+ *
+ * Prefers a non-empty `user.id`, otherwise falls back to a single non-empty role.
+ * Throws when the subject cannot be resolved.
+ */
+export function resolveSubjectFromUser(user: CasbinUser | undefined): string {
+    if (!user) {
+        throw new UnauthorizedException('Request user context missing');
+    }
+
+    const roles = user.roles ?? [];
+
+    if (!Array.isArray(roles)) {
+        throw new UnauthorizedException('User roles must be an array');
+    }
+
+    const userId = typeof user.id === 'string' ? user.id.trim() : '';
+
+    if (userId.length > 0) {
+        return userId;
+    }
+
+    if (roles.length === 1) {
+        const [role] = roles;
+
+        if (typeof role === 'string') {
+            const trimmedRole = role.trim();
+
+            if (trimmedRole.length > 0) {
+                return trimmedRole;
+            }
+        }
+
+        throw new UnauthorizedException('Role subject must be a non-empty string');
+    }
+
+    throw new UnauthorizedException('Unable to determine subject from user context');
+}
