From e8dc3f159eb2c80dba33bf9742ed7bb36912b0f3 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Sun, 25 Oct 2026 19:20:16 -0400 Subject: [PATCH 1/3] chore(ai-review): record authorization race fix forecast --- ...x-casbin-permission-race-4f61c40b0160.json | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 .limetech/ai-review-markers/fix-casbin-permission-race-4f61c40b0160.json diff --git a/.limetech/ai-review-markers/fix-casbin-permission-race-4f61c40b0160.json b/.limetech/ai-review-markers/fix-casbin-permission-race-4f61c40b0160.json new file mode 100644 index 0000000000..50c24560e4 --- /dev/null +++ b/.limetech/ai-review-markers/fix-casbin-permission-race-4f61c40b0160.json @@ -0,0 +1,20 @@ +{ + "disposition": "NOT_REQUIRED", + "expected_structural_delta": "No new owners, caches, dependencies or public contracts; two production files and one integration suite.", + "minimum_design": "Use existing Casbin synchronized enforcer and apply only changed custom permissions.", + "outcome": "Concurrent custom API key authorization remains correct during permission synchronization.", + "reuse_decisions": [ + { + "decision": "reuse", + "name": "Casbin newSyncedEnforcer", + "rationale": "Installed dependency already serializes policy readers and writers" + }, + { + "decision": "extend", + "name": "AuthZService permission methods", + "rationale": "Compute deltas instead of clearing unchanged grants" + } + ], + "schema": "limetech.ai-review-marker.v2", + "stage": "forecast" +} From 1ec467ee05e1787d294b86167b47ceba12f10776 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Sun, 25 Oct 2026 19:22:40 -0400 Subject: [PATCH 2/3] fix(auth): synchronize custom permission policy updates Reproduced from PR #2123 post-merge QA at 3371fd43: concurrent custom-permission checks raised a Casbin TypeError when authentication removed rows during enforcement. Use the existing synchronized enforcer and preserve unchanged grants. Cover HTTP concurrency, denial, revocation, and an explicitly held in-flight read. --- api/src/unraid-api/auth/auth.service.ts | 22 +- .../unraid-api/auth/casbin/casbin.service.ts | 5 +- .../permission-sync.integration.spec.ts | 233 ++++++++++++++++++ 3 files changed, 254 insertions(+), 6 deletions(-) create mode 100644 api/src/unraid-api/auth/casbin/permission-sync.integration.spec.ts diff --git a/api/src/unraid-api/auth/auth.service.ts b/api/src/unraid-api/auth/auth.service.ts index 7a13880542..987d3c973c 100644 --- a/api/src/unraid-api/auth/auth.service.ts +++ b/api/src/unraid-api/auth/auth.service.ts @@ -149,9 +149,6 @@ export class AuthService { public async syncApiKeyPermissions(apiKeyId: string, permissions: Array): Promise { try { - // Clear existing permissions first - await this.authzService.deletePermissionsForUser(apiKeyId); - // Create array of permission-action pairs for processing // Filter out any permissions with empty or undefined resources const permissionActions = permissions @@ -184,8 +181,25 @@ export class AuthService { }) ); + const existing = await this.authzService.getPermissionsForUser(apiKeyId); + const matches = (policy: string[], permission: { resource: string; action: string }) => + policy[1] === permission.resource && policy[2] === permission.action; + + // Keep unchanged grants available to other requests while this key authenticates. + await Promise.all( + existing + .filter( + (policy) => !permissionActions.some((permission) => matches(policy, permission)) + ) + .map(([, resource, action]) => + this.authzService.deletePermissionForUser(apiKeyId, resource, action) + ) + ); + const { errors, errorOccurred: errorOccured } = await batchProcess( - permissionActions, + permissionActions.filter( + (permission) => !existing.some((policy) => matches(policy, permission)) + ), ({ resource, action }) => this.authzService.addPermissionForUser(apiKeyId, resource, action) ); diff --git a/api/src/unraid-api/auth/casbin/casbin.service.ts b/api/src/unraid-api/auth/casbin/casbin.service.ts index 632d0ff8f7..b601f474f3 100644 --- a/api/src/unraid-api/auth/casbin/casbin.service.ts +++ b/api/src/unraid-api/auth/casbin/casbin.service.ts @@ -1,6 +1,6 @@ import { Injectable, InternalServerErrorException, Logger, OnModuleInit } from '@nestjs/common'; -import { Model as CasbinModel, Enforcer, newEnforcer, StringAdapter } from 'casbin'; +import { Model as CasbinModel, Enforcer, newSyncedEnforcer, StringAdapter } from 'casbin'; import { LOG_LEVEL } from '@app/environment.js'; @@ -19,7 +19,8 @@ export class CasbinService { casbinModel.loadModelFromText(model); const casbinPolicy = new StringAdapter(policy); try { - const enforcer = await newEnforcer(casbinModel, casbinPolicy); + // Authentication refreshes policies while other GraphQL fields are authorizing. + const enforcer = await newSyncedEnforcer(casbinModel, casbinPolicy); if (LOG_LEVEL === 'TRACE') { enforcer.enableLog(true); } diff --git a/api/src/unraid-api/auth/casbin/permission-sync.integration.spec.ts b/api/src/unraid-api/auth/casbin/permission-sync.integration.spec.ts new file mode 100644 index 0000000000..b594382021 --- /dev/null +++ b/api/src/unraid-api/auth/casbin/permission-sync.integration.spec.ts @@ -0,0 +1,233 @@ +import { ApolloDriver, ApolloDriverConfig } from '@nestjs/apollo'; +import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common'; +import { APP_GUARD } from '@nestjs/core'; +import { GraphQLModule, Query, Resolver } from '@nestjs/graphql'; +import { FastifyAdapter, NestFastifyApplication } from '@nestjs/platform-fastify'; +import { Test } from '@nestjs/testing'; + +import type { Enforcer } from 'casbin'; +import { AuthAction, Resource, Role } from '@unraid/shared/graphql.model.js'; +import { PrefixedID } from '@unraid/shared/prefixed-id-scalar.js'; +import { UsePermissions } from '@unraid/shared/use-permissions.directive.js'; +import { AUTHZ_ENFORCER, AuthZModule, AuthZService } from 'nest-authz'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { ApiKeyService } from '@app/unraid-api/auth/api-key.service.js'; +import { AuthService } from '@app/unraid-api/auth/auth.service.js'; +import { AuthorizationGuard } from '@app/unraid-api/auth/authorization.guard.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 { CookieService } from '@app/unraid-api/auth/cookie.service.js'; +import { LocalSessionService } from '@app/unraid-api/auth/local-session.service.js'; +import { ApiKey } from '@app/unraid-api/graph/resolvers/api-key/api-key.model.js'; +import { getRequest } from '@app/utils.js'; + +@Injectable() +class PermissionSyncGuard implements CanActivate { + constructor(private readonly auth: AuthService) {} + + async canActivate(context: ExecutionContext) { + const request = getRequest(context); + request.user = await this.auth.validateApiKeyCasbin(String(request.headers['x-api-key'])); + return true; + } +} + +@Resolver() +class PermissionProbeResolver { + @Query(() => Boolean, { nullable: true }) + @UsePermissions({ resource: Resource.NOTIFICATIONS, action: AuthAction.READ_ANY }) + notificationsProbe() { + return true; + } + + @Query(() => Boolean, { nullable: true }) + @UsePermissions({ resource: Resource.DOCKER, action: AuthAction.READ_ANY }) + dockerProbe() { + return true; + } + + @Query(() => Boolean, { nullable: true }) + @UsePermissions({ resource: Resource.VMS, action: AuthAction.READ_ANY }) + vmsProbe() { + return true; + } + + @Query(() => Boolean, { nullable: true }) + @UsePermissions({ resource: Resource.DOCKER, action: AuthAction.DELETE_ANY }) + deleteProbe() { + return true; + } +} + +describe('Concurrent custom API key authorization', () => { + let app: NestFastifyApplication; + let enforcer: Enforcer; + let auth: AuthService; + let key: ApiKey; + + beforeEach(async () => { + key = { + id: 'custom-permission-key', + key: 'synthetic-key', + name: 'Concurrent test', + roles: [Role.GUEST], + createdAt: new Date().toISOString(), + permissions: [Resource.NOTIFICATIONS, Resource.DOCKER, Resource.VMS].map((resource) => ({ + resource, + actions: [AuthAction.READ_ANY], + })), + }; + enforcer = await new CasbinService().initializeEnforcer(CASBIN_MODEL, BASE_POLICY); + const module = await Test.createTestingModule({ + imports: [ + AuthZModule.register({ + enablePossession: false, + enforcerProvider: { provide: AUTHZ_ENFORCER, useValue: enforcer }, + userFromContext: (context) => getRequest(context)?.user?.id ?? '', + }), + GraphQLModule.forRoot({ + driver: ApolloDriver, + autoSchemaFile: true, + context: ({ req }) => ({ req }), + }), + ], + providers: [ + AuthService, + PermissionProbeResolver, + PrefixedID, + { provide: ApiKeyService, useValue: { findByKey: vi.fn(async () => key) } }, + { provide: CookieService, useValue: {} }, + { provide: LocalSessionService, useValue: {} }, + { provide: APP_GUARD, useClass: PermissionSyncGuard }, + { provide: APP_GUARD, useClass: AuthorizationGuard }, + ], + }).compile(); + auth = module.get(AuthService); + app = module.createNestApplication(new FastifyAdapter()); + await app.init(); + await app.getHttpAdapter().getInstance().ready(); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + await app?.close(); + }); + + async function execute(query: string) { + const response = await app.inject({ + method: 'POST', + url: '/graphql', + headers: { 'x-api-key': key.key }, + payload: { query }, + }); + expect(response.statusCode).toBe(200); + return response.json<{ + data: Record; + errors?: { extensions: { code: string } }[]; + }>(); + } + + it('preserves grants and denials across parallel fields and repeated HTTP requests', async () => { + for (let round = 0; round < 4; round++) { + const results = await Promise.all( + Array.from({ length: 12 }, () => + execute('{ notificationsProbe dockerProbe vmsProbe deleteProbe }') + ) + ); + for (const result of results) { + expect(result.data).toEqual({ + notificationsProbe: true, + dockerProbe: true, + vmsProbe: true, + deleteProbe: null, + }); + expect(result.errors).toHaveLength(1); + expect(result.errors?.[0].extensions.code).toBe('FORBIDDEN'); + } + } + }); + + it('does not remove an existing grant while refreshing an unchanged key', async () => { + await auth.validateApiKeyCasbin(key.key); + const authz = app.get(AuthZService); + const remove = vi.spyOn(authz, 'deletePermissionsForUser'); + const removeOne = vi.spyOn(authz, 'deletePermissionForUser'); + const add = vi.spyOn(authz, 'addPermissionForUser'); + await Promise.all(Array.from({ length: 12 }, () => auth.validateApiKeyCasbin(key.key))); + expect(remove).not.toHaveBeenCalled(); + expect(removeOne).not.toHaveBeenCalled(); + expect(add).not.toHaveBeenCalled(); + }); + + it('applies permission removal and new grants on the next authenticated request', async () => { + await auth.validateApiKeyCasbin(key.key); + key.permissions = [{ resource: Resource.DOCKER, actions: [AuthAction.DELETE_ANY] }]; + const result = await execute('{ notificationsProbe dockerProbe vmsProbe deleteProbe }'); + expect(result.data).toEqual({ + notificationsProbe: null, + dockerProbe: null, + vmsProbe: null, + deleteProbe: true, + }); + expect(result.errors).toHaveLength(3); + expect(result.errors?.every((error) => error.extensions.code === 'FORBIDDEN')).toBe(true); + }); + + it('finishes an in-flight check before removing policy rows, then denies the revoked grant', async () => { + await auth.validateApiKeyCasbin(key.key); + let entered!: () => void; + let release!: () => void; + const checking = new Promise((resolve) => { + entered = resolve; + }); + const proceed = new Promise((resolve) => { + release = resolve; + }); + const roles = enforcer.getRoleManager(); + const hasLink = roles.hasLink.bind(roles); + vi.spyOn(roles, 'hasLink').mockImplementationOnce(async (...args) => { + entered(); + await proceed; + return hasLink(...args); + }); + const decision = enforcer.enforce(key.id, Resource.VMS, AuthAction.READ_ANY); + // Observe rejection immediately so the baseline failure cannot leak an unhandled rejection. + const observed = decision.then( + (value) => ({ value }), + (error) => ({ error }) + ); + await checking; + const removal = enforcer.deletePermissionsForUser(key.id); + await new Promise((resolve) => setImmediate(resolve)); + release(); + expect(await observed).toEqual({ value: true }); + await removal; + await expect(enforcer.enforce(key.id, Resource.VMS, AuthAction.READ_ANY)).resolves.toBe(false); + }); + + it('retains wildcard expansion and removes every custom grant for an empty permission set', async () => { + // @ts-expect-error Legacy persisted wildcard actions predate the current GraphQL enum. + await auth.syncApiKeyPermissions(key.id, [{ resource: Resource.DOCKER, actions: ['*'] }]); + await expect(enforcer.enforce(key.id, Resource.DOCKER, AuthAction.READ_ANY)).resolves.toBe(true); + await expect(enforcer.enforce(key.id, Resource.DOCKER, AuthAction.DELETE_ANY)).resolves.toBe( + true + ); + await auth.syncApiKeyPermissions(key.id, []); + await expect(enforcer.enforce(key.id, Resource.DOCKER, AuthAction.READ_ANY)).resolves.toBe( + false + ); + await expect(enforcer.enforce(key.id, Resource.DOCKER, AuthAction.DELETE_ANY)).resolves.toBe( + false + ); + }); + + it('releases synchronization after a failed check so later requests still authorize', async () => { + await auth.validateApiKeyCasbin(key.key); + vi.spyOn(enforcer.getRoleManager(), 'hasLink').mockRejectedValueOnce( + new Error('role lookup failure') + ); + await expect(enforcer.enforce(key.id, Resource.VMS, AuthAction.READ_ANY)).rejects.toThrow(); + await expect(enforcer.enforce(key.id, Resource.VMS, AuthAction.READ_ANY)).resolves.toBe(true); + }); +}); From c014b2fef24069e24a91772c7bdac06176838607 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Sun, 25 Oct 2026 19:23:05 -0400 Subject: [PATCH 3/3] chore(ai-review): record authorization fix review receipt --- ...x-casbin-permission-race-4f61c40b0160.json | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/.limetech/ai-review-markers/fix-casbin-permission-race-4f61c40b0160.json b/.limetech/ai-review-markers/fix-casbin-permission-race-4f61c40b0160.json index 50c24560e4..9439e6ebf6 100644 --- a/.limetech/ai-review-markers/fix-casbin-permission-race-4f61c40b0160.json +++ b/.limetech/ai-review-markers/fix-casbin-permission-race-4f61c40b0160.json @@ -1,20 +1,8 @@ { "disposition": "NOT_REQUIRED", - "expected_structural_delta": "No new owners, caches, dependencies or public contracts; two production files and one integration suite.", - "minimum_design": "Use existing Casbin synchronized enforcer and apply only changed custom permissions.", - "outcome": "Concurrent custom API key authorization remains correct during permission synchronization.", - "reuse_decisions": [ - { - "decision": "reuse", - "name": "Casbin newSyncedEnforcer", - "rationale": "Installed dependency already serializes policy readers and writers" - }, - { - "decision": "extend", - "name": "AuthZService permission methods", - "rationale": "Compute deltas instead of clearing unchanged grants" - } - ], + "forecast_commit": "e8dc3f159eb2c80dba33bf9742ed7bb36912b0f3", + "reviewed_sha": "1ec467ee05e1787d294b86167b47ceba12f10776", "schema": "limetech.ai-review-marker.v2", - "stage": "forecast" + "stage": "final", + "unresolved_proportionality_findings": [] }