Skip to content

Commit 05ba1bf

Browse files
elibosleyelibosley
authored
fix(auth): prevent custom permission checks racing policy refresh (#2125)
Custom API keys could fail mixed GraphQL queries with `Cannot read properties of undefined (reading '0')` inside Casbin. Authentication cleared and rebuilt the shared permission rows while another authorization check was iterating them. This was discovered during post-merge QA of #2123 at 3371fd4. This uses Casbin’s existing synchronized enforcer to serialize checks with policy mutations. Permission synchronization now removes obsolete grants and adds missing grants, preserving unchanged grants during repeated authentication. Permission changes still apply on the next authentication; no permission cache, schema change or migration is introduced. Reviewer consideration: authorization checks now share Casbin’s dependency-owned lock. This prevents concurrent policy mutation; unchanged keys avoid policy writes. An in-flight check finishes before revocation, and later checks observe the revoked permission. Validation: all 366 authentication tests, API type-check, API lint and authorization coverage lint passed locally. Six integration regressions cover 48 concurrent HTTP queries with granted/denied fields, unchanged refreshes, changed permissions, wildcard/empty permissions, a deterministically paused authorization check overlapping policy removal, and lock recovery after a failed check. The deterministic overlap reproduced the exact exception before the fix. Exact-head CI build/test and CodeQL passed; the advisory dependency audit still fails. The packaged plugin from run 40244541250 was installed on a managed Unraid 7.4.0-rc.1 QA VM, and all 32 packaged API dist files matched the downloaded CI payload. Appliance QA passed 300 real HTTP requests, including 288 requests at concurrency 24 across two distinct GUEST keys. Cases covered stable grants and cross-key denial, actual unavailable Docker/libvirt backends with healthy notification data retained, denied API-key mutation without side effects, revocation and restoration on subsequent requests, empty custom grants with an explicit GUEST role, ADMIN/VIEWER compatibility, and persisted grants/revocations after service restart. Zero Casbin type errors were logged. Observed p95 request latency was 49 ms (diagnostic, not a comparative performance claim). All five temporary keys were deleted and verified absent through the native CLI. Notification configuration remained byte-for-byte unchanged. Provider rollback completed, and a fresh check confirmed the reservation absent and the capacity available and powered off. PR remains unmerged. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Improved API key permission updates to avoid temporarily clearing all access grants during synchronization. * Permission changes are applied more consistently during concurrent requests, while outdated grants are removed and unchanged grants are retained. * Updated permissions, including wildcard grants, are reflected in subsequent authorization checks. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 3371fd4 commit 05ba1bf

4 files changed

Lines changed: 262 additions & 6 deletions

File tree

‎.limetech/ai-review-markers/fix-casbin-permission-race-4f61c40b0160.json‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
{
2+
"disposition": "NOT_REQUIRED",
3+
"forecast_commit": "e8dc3f159eb2c80dba33bf9742ed7bb36912b0f3",
4+
"reviewed_sha": "1ec467ee05e1787d294b86167b47ceba12f10776",
5+
"schema": "limetech.ai-review-marker.v2",
6+
"stage": "final",
7+
"unresolved_proportionality_findings": []
8+
}

‎api/src/unraid-api/auth/auth.service.ts‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -149,9 +149,6 @@ export class AuthService {
149149

150150
public async syncApiKeyPermissions(apiKeyId: string, permissions: Array<Permission>): Promise<void> {
151151
try {
152-
// Clear existing permissions first
153-
await this.authzService.deletePermissionsForUser(apiKeyId);
154-
155152
// Create array of permission-action pairs for processing
156153
// Filter out any permissions with empty or undefined resources
157154
const permissionActions = permissions
@@ -184,8 +181,25 @@ export class AuthService {
184181
})
185182
);
186183

184+
const existing = await this.authzService.getPermissionsForUser(apiKeyId);
185+
const matches = (policy: string[], permission: { resource: string; action: string }) =>
186+
policy[1] === permission.resource && policy[2] === permission.action;
187+
188+
// Keep unchanged grants available to other requests while this key authenticates.
189+
await Promise.all(
190+
existing
191+
.filter(
192+
(policy) => !permissionActions.some((permission) => matches(policy, permission))
193+
)
194+
.map(([, resource, action]) =>
195+
this.authzService.deletePermissionForUser(apiKeyId, resource, action)
196+
)
197+
);
198+
187199
const { errors, errorOccurred: errorOccured } = await batchProcess(
188-
permissionActions,
200+
permissionActions.filter(
201+
(permission) => !existing.some((policy) => matches(policy, permission))
202+
),
189203
({ resource, action }) =>
190204
this.authzService.addPermissionForUser(apiKeyId, resource, action)
191205
);

‎api/src/unraid-api/auth/casbin/casbin.service.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Injectable, InternalServerErrorException, Logger, OnModuleInit } from '@nestjs/common';
22

3-
import { Model as CasbinModel, Enforcer, newEnforcer, StringAdapter } from 'casbin';
3+
import { Model as CasbinModel, Enforcer, newSyncedEnforcer, StringAdapter } from 'casbin';
44

55
import { LOG_LEVEL } from '@app/environment.js';
66

@@ -19,7 +19,8 @@ export class CasbinService {
1919
casbinModel.loadModelFromText(model);
2020
const casbinPolicy = new StringAdapter(policy);
2121
try {
22-
const enforcer = await newEnforcer(casbinModel, casbinPolicy);
22+
// Authentication refreshes policies while other GraphQL fields are authorizing.
23+
const enforcer = await newSyncedEnforcer(casbinModel, casbinPolicy);
2324
if (LOG_LEVEL === 'TRACE') {
2425
enforcer.enableLog(true);
2526
}

‎api/src/unraid-api/auth/casbin/permission-sync.integration.spec.ts‎

Lines changed: 233 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,233 @@
1+
import { ApolloDriver, ApolloDriverConfig } from '@nestjs/apollo';
2+
import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common';
3+
import { APP_GUARD } from '@nestjs/core';
4+
import { GraphQLModule, Query, Resolver } from '@nestjs/graphql';
5+
import { FastifyAdapter, NestFastifyApplication } from '@nestjs/platform-fastify';
6+
import { Test } from '@nestjs/testing';
7+
8+
import type { Enforcer } from 'casbin';
9+
import { AuthAction, Resource, Role } from '@unraid/shared/graphql.model.js';
10+
import { PrefixedID } from '@unraid/shared/prefixed-id-scalar.js';
11+
import { UsePermissions } from '@unraid/shared/use-permissions.directive.js';
12+
import { AUTHZ_ENFORCER, AuthZModule, AuthZService } from 'nest-authz';
13+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
14+
15+
import { ApiKeyService } from '@app/unraid-api/auth/api-key.service.js';
16+
import { AuthService } from '@app/unraid-api/auth/auth.service.js';
17+
import { AuthorizationGuard } from '@app/unraid-api/auth/authorization.guard.js';
18+
import { CasbinService } from '@app/unraid-api/auth/casbin/casbin.service.js';
19+
import { BASE_POLICY, CASBIN_MODEL } from '@app/unraid-api/auth/casbin/index.js';
20+
import { CookieService } from '@app/unraid-api/auth/cookie.service.js';
21+
import { LocalSessionService } from '@app/unraid-api/auth/local-session.service.js';
22+
import { ApiKey } from '@app/unraid-api/graph/resolvers/api-key/api-key.model.js';
23+
import { getRequest } from '@app/utils.js';
24+
25+
@Injectable()
26+
class PermissionSyncGuard implements CanActivate {
27+
constructor(private readonly auth: AuthService) {}
28+
29+
async canActivate(context: ExecutionContext) {
30+
const request = getRequest(context);
31+
request.user = await this.auth.validateApiKeyCasbin(String(request.headers['x-api-key']));
32+
return true;
33+
}
34+
}
35+
36+
@Resolver()
37+
class PermissionProbeResolver {
38+
@Query(() => Boolean, { nullable: true })
39+
@UsePermissions({ resource: Resource.NOTIFICATIONS, action: AuthAction.READ_ANY })
40+
notificationsProbe() {
41+
return true;
42+
}
43+
44+
@Query(() => Boolean, { nullable: true })
45+
@UsePermissions({ resource: Resource.DOCKER, action: AuthAction.READ_ANY })
46+
dockerProbe() {
47+
return true;
48+
}
49+
50+
@Query(() => Boolean, { nullable: true })
51+
@UsePermissions({ resource: Resource.VMS, action: AuthAction.READ_ANY })
52+
vmsProbe() {
53+
return true;
54+
}
55+
56+
@Query(() => Boolean, { nullable: true })
57+
@UsePermissions({ resource: Resource.DOCKER, action: AuthAction.DELETE_ANY })
58+
deleteProbe() {
59+
return true;
60+
}
61+
}
62+
63+
describe('Concurrent custom API key authorization', () => {
64+
let app: NestFastifyApplication;
65+
let enforcer: Enforcer;
66+
let auth: AuthService;
67+
let key: ApiKey;
68+
69+
beforeEach(async () => {
70+
key = {
71+
id: 'custom-permission-key',
72+
key: 'synthetic-key',
73+
name: 'Concurrent test',
74+
roles: [Role.GUEST],
75+
createdAt: new Date().toISOString(),
76+
permissions: [Resource.NOTIFICATIONS, Resource.DOCKER, Resource.VMS].map((resource) => ({
77+
resource,
78+
actions: [AuthAction.READ_ANY],
79+
})),
80+
};
81+
enforcer = await new CasbinService().initializeEnforcer(CASBIN_MODEL, BASE_POLICY);
82+
const module = await Test.createTestingModule({
83+
imports: [
84+
AuthZModule.register({
85+
enablePossession: false,
86+
enforcerProvider: { provide: AUTHZ_ENFORCER, useValue: enforcer },
87+
userFromContext: (context) => getRequest(context)?.user?.id ?? '',
88+
}),
89+
GraphQLModule.forRoot<ApolloDriverConfig>({
90+
driver: ApolloDriver,
91+
autoSchemaFile: true,
92+
context: ({ req }) => ({ req }),
93+
}),
94+
],
95+
providers: [
96+
AuthService,
97+
PermissionProbeResolver,
98+
PrefixedID,
99+
{ provide: ApiKeyService, useValue: { findByKey: vi.fn(async () => key) } },
100+
{ provide: CookieService, useValue: {} },
101+
{ provide: LocalSessionService, useValue: {} },
102+
{ provide: APP_GUARD, useClass: PermissionSyncGuard },
103+
{ provide: APP_GUARD, useClass: AuthorizationGuard },
104+
],
105+
}).compile();
106+
auth = module.get(AuthService);
107+
app = module.createNestApplication<NestFastifyApplication>(new FastifyAdapter());
108+
await app.init();
109+
await app.getHttpAdapter().getInstance().ready();
110+
});
111+
112+
afterEach(async () => {
113+
vi.restoreAllMocks();
114+
await app?.close();
115+
});
116+
117+
async function execute(query: string) {
118+
const response = await app.inject({
119+
method: 'POST',
120+
url: '/graphql',
121+
headers: { 'x-api-key': key.key },
122+
payload: { query },
123+
});
124+
expect(response.statusCode).toBe(200);
125+
return response.json<{
126+
data: Record<string, boolean | null>;
127+
errors?: { extensions: { code: string } }[];
128+
}>();
129+
}
130+
131+
it('preserves grants and denials across parallel fields and repeated HTTP requests', async () => {
132+
for (let round = 0; round < 4; round++) {
133+
const results = await Promise.all(
134+
Array.from({ length: 12 }, () =>
135+
execute('{ notificationsProbe dockerProbe vmsProbe deleteProbe }')
136+
)
137+
);
138+
for (const result of results) {
139+
expect(result.data).toEqual({
140+
notificationsProbe: true,
141+
dockerProbe: true,
142+
vmsProbe: true,
143+
deleteProbe: null,
144+
});
145+
expect(result.errors).toHaveLength(1);
146+
expect(result.errors?.[0].extensions.code).toBe('FORBIDDEN');
147+
}
148+
}
149+
});
150+
151+
it('does not remove an existing grant while refreshing an unchanged key', async () => {
152+
await auth.validateApiKeyCasbin(key.key);
153+
const authz = app.get(AuthZService);
154+
const remove = vi.spyOn(authz, 'deletePermissionsForUser');
155+
const removeOne = vi.spyOn(authz, 'deletePermissionForUser');
156+
const add = vi.spyOn(authz, 'addPermissionForUser');
157+
await Promise.all(Array.from({ length: 12 }, () => auth.validateApiKeyCasbin(key.key)));
158+
expect(remove).not.toHaveBeenCalled();
159+
expect(removeOne).not.toHaveBeenCalled();
160+
expect(add).not.toHaveBeenCalled();
161+
});
162+
163+
it('applies permission removal and new grants on the next authenticated request', async () => {
164+
await auth.validateApiKeyCasbin(key.key);
165+
key.permissions = [{ resource: Resource.DOCKER, actions: [AuthAction.DELETE_ANY] }];
166+
const result = await execute('{ notificationsProbe dockerProbe vmsProbe deleteProbe }');
167+
expect(result.data).toEqual({
168+
notificationsProbe: null,
169+
dockerProbe: null,
170+
vmsProbe: null,
171+
deleteProbe: true,
172+
});
173+
expect(result.errors).toHaveLength(3);
174+
expect(result.errors?.every((error) => error.extensions.code === 'FORBIDDEN')).toBe(true);
175+
});
176+
177+
it('finishes an in-flight check before removing policy rows, then denies the revoked grant', async () => {
178+
await auth.validateApiKeyCasbin(key.key);
179+
let entered!: () => void;
180+
let release!: () => void;
181+
const checking = new Promise<void>((resolve) => {
182+
entered = resolve;
183+
});
184+
const proceed = new Promise<void>((resolve) => {
185+
release = resolve;
186+
});
187+
const roles = enforcer.getRoleManager();
188+
const hasLink = roles.hasLink.bind(roles);
189+
vi.spyOn(roles, 'hasLink').mockImplementationOnce(async (...args) => {
190+
entered();
191+
await proceed;
192+
return hasLink(...args);
193+
});
194+
const decision = enforcer.enforce(key.id, Resource.VMS, AuthAction.READ_ANY);
195+
// Observe rejection immediately so the baseline failure cannot leak an unhandled rejection.
196+
const observed = decision.then(
197+
(value) => ({ value }),
198+
(error) => ({ error })
199+
);
200+
await checking;
201+
const removal = enforcer.deletePermissionsForUser(key.id);
202+
await new Promise<void>((resolve) => setImmediate(resolve));
203+
release();
204+
expect(await observed).toEqual({ value: true });
205+
await removal;
206+
await expect(enforcer.enforce(key.id, Resource.VMS, AuthAction.READ_ANY)).resolves.toBe(false);
207+
});
208+
209+
it('retains wildcard expansion and removes every custom grant for an empty permission set', async () => {
210+
// @ts-expect-error Legacy persisted wildcard actions predate the current GraphQL enum.
211+
await auth.syncApiKeyPermissions(key.id, [{ resource: Resource.DOCKER, actions: ['*'] }]);
212+
await expect(enforcer.enforce(key.id, Resource.DOCKER, AuthAction.READ_ANY)).resolves.toBe(true);
213+
await expect(enforcer.enforce(key.id, Resource.DOCKER, AuthAction.DELETE_ANY)).resolves.toBe(
214+
true
215+
);
216+
await auth.syncApiKeyPermissions(key.id, []);
217+
await expect(enforcer.enforce(key.id, Resource.DOCKER, AuthAction.READ_ANY)).resolves.toBe(
218+
false
219+
);
220+
await expect(enforcer.enforce(key.id, Resource.DOCKER, AuthAction.DELETE_ANY)).resolves.toBe(
221+
false
222+
);
223+
});
224+
225+
it('releases synchronization after a failed check so later requests still authorize', async () => {
226+
await auth.validateApiKeyCasbin(key.key);
227+
vi.spyOn(enforcer.getRoleManager(), 'hasLink').mockRejectedValueOnce(
228+
new Error('role lookup failure')
229+
);
230+
await expect(enforcer.enforce(key.id, Resource.VMS, AuthAction.READ_ANY)).rejects.toThrow();
231+
await expect(enforcer.enforce(key.id, Resource.VMS, AuthAction.READ_ANY)).resolves.toBe(true);
232+
});
233+
});

0 commit comments

Comments
 (0)