diff --git a/src/backend/services/permission/PermissionService.test.ts b/src/backend/services/permission/PermissionService.test.ts index 263e20327..30fe40a2e 100644 --- a/src/backend/services/permission/PermissionService.test.ts +++ b/src/backend/services/permission/PermissionService.test.ts @@ -17,7 +17,7 @@ * along with this program. If not, see . */ -import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; import { v4 as uuidv4 } from 'uuid'; import type { Actor } from '../../core/actor.js'; import { runWithContext } from '../../core/context.js'; @@ -729,4 +729,276 @@ describe('PermissionService (integration)', () => { ); }); }); + + describe('derived-actor cache invalidation (app-under-user)', () => { + // An app-under-user actor's reading embeds its user's reading, and + // its cache keys fold in the user's generation counter — so a + // user-level grant/revoke must take effect for the user's app + // actors on their very next check, not after the scan-cache TTL. + const grantManage = async ( + issuer: { id: number }, + permission: string, + ) => { + await server.stores.permission.setFlatUserPerm( + issuer.id, + `manage:${permission}`, + { + permission: `manage:${permission}`, + deleted: false, + issuer_user_id: issuer.id, + } as never, + ); + }; + + const makeApp = async (ownerUserId: number) => + (server.stores.app.create as unknown as ( + fields: Record, + opts: { ownerUserId: number }, + ) => Promise<{ uid: string; id: number }>)( + { + name: `dac-${uuidv4()}`, + title: 'Derived-actor cache app', + index_url: `https://dac-${uuidv4()}.test/`, + }, + { ownerUserId }, + ); + + it('a user-level revoke is visible immediately to the user\'s app actors', async () => { + const { user: issuer, actor: issuerActor } = + await makeUserActor(); + const { user: target, actor: targetActor } = + await makeUserActor(); + const app = await makeApp(target.id); + const permission = `service:app-revoke-now-${uuidv4()}:ii:read`; + await grantManage(issuer, permission); + + await runWithContext({ actor: issuerActor }, () => + permService.grantUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + // The user lets the app act with this permission, so the app + // actor resolves it through the user's own reading. + await runWithContext({ actor: targetActor }, () => + permService.grantUserAppPermission( + targetActor, + app.uid, + permission, + ), + ); + + const appActor = { + user: targetActor.user, + app: { id: app.id, uid: app.uid }, + } as unknown as Actor; + + // Prime the app actor's cache with a "granted" reading. + expect(await permService.check(appActor, permission)).toBe(true); + + await runWithContext({ actor: issuerActor }, () => + permService.revokeUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + + // Without the user generation folded into the app actor's + // cache keys this would still read `true` for up to the TTL. + expect(await permService.check(appActor, permission)).toBe(false); + }); + + it('a user-level grant busts an app actor\'s cached denial immediately', async () => { + const { user: issuer, actor: issuerActor } = + await makeUserActor(); + const { user: target, actor: targetActor } = + await makeUserActor(); + const app = await makeApp(target.id); + const permission = `service:app-grant-now-${uuidv4()}:ii:read`; + await grantManage(issuer, permission); + + // App is allowed to act with the permission, but the user does + // not hold it yet — primes a "denied" reading for the app actor. + await runWithContext({ actor: targetActor }, () => + permService.grantUserAppPermission( + targetActor, + app.uid, + permission, + ), + ); + const appActor = { + user: targetActor.user, + app: { id: app.id, uid: app.uid }, + } as unknown as Actor; + expect(await permService.check(appActor, permission)).toBe(false); + + await runWithContext({ actor: issuerActor }, () => + permService.grantUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + + expect(await permService.check(appActor, permission)).toBe(true); + }); + }); + + describe('revoke durability (flat/linked consistency)', () => { + const grantManage = async ( + issuer: { id: number }, + permission: string, + ) => { + await server.stores.permission.setFlatUserPerm( + issuer.id, + `manage:${permission}`, + { + permission: `manage:${permission}`, + deleted: false, + issuer_user_id: issuer.id, + } as never, + ); + }; + + it('revokeUserUserPermission deletes the linked SQL row before resolving', async () => { + const { user: issuer, actor: issuerActor } = + await makeUserActor(); + const { user: target } = await makeUserActor(); + const permission = `service:rvk-sync-${uuidv4()}:ii:read`; + await grantManage(issuer, permission); + await runWithContext({ actor: issuerActor }, () => + permService.grantUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + + await runWithContext({ actor: issuerActor }, () => + permService.revokeUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + + // The linked row must be gone the moment the revoke resolves — + // a fire-and-forget delete could lose the race against the + // post-bump rescan, which would re-warm the flat view from the + // surviving SQL row and resurrect the grant. + const rows = await server.stores.permission.readLinkedUserUserPerms( + target.id, + [permission], + ); + expect(rows).toHaveLength(0); + }); + + it('revokeUserUserPermission surfaces a failed SQL delete instead of swallowing it', async () => { + const { user: issuer, actor: issuerActor } = + await makeUserActor(); + const { user: target } = await makeUserActor(); + const permission = `service:rvk-fail-${uuidv4()}:ii:read`; + await grantManage(issuer, permission); + await runWithContext({ actor: issuerActor }, () => + permService.grantUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + + const spy = vi + .spyOn(server.stores.permission, 'deleteUserUserPermByHolder') + .mockRejectedValue(new Error('simulated db failure')); + try { + await expect( + runWithContext({ actor: issuerActor }, () => + permService.revokeUserUserPermission( + issuerActor, + target.username, + permission, + ), + ), + ).rejects.toThrow('simulated db failure'); + } finally { + spy.mockRestore(); + } + + // Retry once the store works again — the revoke completes. + await runWithContext({ actor: issuerActor }, () => + permService.revokeUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + }); + + it('scan-path warms of the flat view carry an expiry (grants are permanent)', async () => { + const { user: issuer, actor: issuerActor } = + await makeUserActor(); + const { user: target, actor: targetActor } = + await makeUserActor(); + const permission = `service:warm-ttl-${uuidv4()}:ii:read`; + await grantManage(issuer, permission); + // The linked (SQL) path is a delegation chain: it only grants + // if the issuer holds the permission themselves. Give the + // issuer a terminal flat grant so the fallback below resolves. + await server.stores.permission.setFlatUserPerm( + issuer.id, + permission, + { + permission, + deleted: false, + issuer_user_id: issuer.id, + } as never, + ); + await runWithContext({ actor: issuerActor }, () => + permService.grantUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + // The grant's linked-row upsert is fire-and-forget — wait for + // it so the linked fallback below has something to find. + await vi.waitFor(async () => { + const rows = + await server.stores.permission.readLinkedUserUserPerms( + target.id, + [permission], + ); + expect(rows.length).toBeGreaterThan(0); + }); + // Drop the flat entry so the next check takes the linked SQL + // fallback and re-warms the flat view. + await server.stores.permission.delFlatUserPerm( + target.id, + permission, + ); + + const spy = vi.spyOn(server.stores.permission, 'setFlatUserPerm'); + try { + expect(await permService.check(targetActor, permission)).toBe( + true, + ); + // The warm is fire-and-forget; wait for it to land. + await vi.waitFor(() => { + const warmCall = spy.mock.calls.find( + (c) => c[1] === permission, + ); + expect(warmCall).toBeDefined(); + // Derived warms must self-expire so one that races a + // concurrent revoke cannot persist indefinitely. + expect(warmCall![3]?.expireAt).toBeGreaterThan( + Math.floor(Date.now() / 1000), + ); + }); + } finally { + spy.mockRestore(); + } + }); + }); }); diff --git a/src/backend/services/permission/PermissionService.ts b/src/backend/services/permission/PermissionService.ts index 0d2c7bc54..b21483338 100644 --- a/src/backend/services/permission/PermissionService.ts +++ b/src/backend/services/permission/PermissionService.ts @@ -23,6 +23,7 @@ import { Context } from '../../core/context'; import { HttpError } from '../../core/http/HttpError.js'; import { PuterService } from '../types'; import { + FLAT_PERM_WARM_TTL_SECONDS, MANAGE_PERM_PREFIX, PERMISSION_SCAN_CACHE_TTL_SECONDS, } from './consts'; @@ -215,8 +216,7 @@ export class PermissionService extends PuterService { // -- Cache pass: one pipelined MGET -- const aUid = actorUid(actor); - const generation = - await this.stores.permission.getCacheGeneration(aUid); + const generation = await this.#cacheGenerationTag(actor); const cached = await this.stores.permission.getMultiCheckCache( aUid, dedup, @@ -296,9 +296,10 @@ export class PermissionService extends PuterService { // -- Redis scan cache -- // The per-actor cache generation is folded into the key so a // grant/revoke bump orphans this actor's cached readings at once. + // For derived actors the tag combines every relevant counter, so a + // user-level bump also orphans that user's app/token actors. const aUid = actorUid(actor); - const generation = - await this.stores.permission.getCacheGeneration(aUid); + const generation = await this.#cacheGenerationTag(actor); const cacheKey = this.stores.permission.buildScanCacheKey( aUid, options, @@ -763,24 +764,43 @@ export class PermissionService extends PuterService { const linkedReading = await linkedPromise; const flatOptions = PermissionUtil.readingToOptions(linkedReading); - // Warm flat KV cache for future hits (fire-and-forget, don't block result) + // Warm flat KV cache for future hits (fire-and-forget, don't block + // result). Warms expire: they are derived from the SQL traversal + // above, and a warm whose KV write lands after a concurrent + // revoke's flat delete would otherwise re-materialize the revoked + // grant permanently. The expiry bounds that to the warm TTL — the + // next scan re-derives from SQL, which the revoke deletes + // synchronously. (Grant-path flat writes are authoritative and + // carry no expiry.) + const warmExpireAt = + Math.floor(Date.now() / 1000) + FLAT_PERM_WARM_TTL_SECONDS; for (const opt of flatOptions) { if (!opt.permission) continue; const data = Array.isArray(opt.data) ? opt.data : [opt.data]; const issuerUserId = (data[0] as { issuer_user_id?: number }) ?.issuer_user_id; this.stores.permission - .setFlatUserPerm(actor.user.id, opt.permission, { - permission: opt.permission, - issuer_user_id: issuerUserId, - data, - }) + .setFlatUserPerm( + actor.user.id, + opt.permission, + { + permission: opt.permission, + issuer_user_id: issuerUserId, + data, + }, + { expireAt: warmExpireAt }, + ) .catch(() => { /* swallow — this is a cache warm */ }); } - return flatReading; + // Return the traversal result itself — not the (empty) flat + // reading — so the fallback grants on the check that took it + // rather than only after the warm lands. Returning the empty flat + // reading here would cache a false "denied" for the TTL every + // time a warm expires. + return linkedReading; } async #flatValidateUserPerms( @@ -951,9 +971,17 @@ export class PermissionService extends PuterService { const issuerId = actor.user.id; await this.stores.permission.delFlatUserPerm(user.id, permission); - this.stores.permission - .deleteUserUserPermByHolder(user.id, permission) - .catch(() => {}); + // Awaited (unlike the grant-path upsert): the generation bump below + // guarantees the holder's very next check re-derives from SQL, so a + // fire-and-forget delete here could lose the race and let that scan + // resurrect the revoked grant into the flat view. If this fails the + // caller gets the error — the permission is then still effectively + // granted (flat falls back to the surviving SQL row), which is the + // consistent, retryable outcome. + await this.stores.permission.deleteUserUserPermByHolder( + user.id, + permission, + ); this.stores.permission .auditUserUserPerm({ holder_user_id: user.id, @@ -1375,6 +1403,53 @@ export class PermissionService extends PuterService { // cached scan/check readings at once. This is cluster-safe (a single // INCR, no pattern scan) and makes a grant/revoke take effect on the // holder's very next check rather than after the cache TTL. + // + // Derived actors fold their user's counter into their cache keys (see + // #cacheGenerationKeys), so a `user:` bump also takes immediate + // effect for that user's app-under-user and access-token actors. + // Readings that embed a *different* user's reading (group/dev-app/ + // user-user issuer chains) are not generation-linked to that issuer; + // those remain bounded by the scan-cache TTL. + + /** + * Generation keys whose counters this actor's cached readings depend + * on. A derived actor (app-under-user, access-token) acts through its + * user — its readings embed that user's reading via the recursive + * issuer scan — so the user's counter is folded into its cache keys. + * A plain user actor depends only on its own counter. + */ + #cacheGenerationKeys(actor: Actor): string[] { + const keys = [actorUid(actor)]; + if (actor.accessToken) { + keys.push(...this.#cacheGenerationKeys(actor.accessToken.issuer)); + if (actor.accessToken.authorized) { + keys.push( + ...this.#cacheGenerationKeys(actor.accessToken.authorized), + ); + } + } else if (actor.app && actor.user?.uuid) { + keys.push(`user:${actor.user.uuid}`); + } + return Array.from(new Set(keys)); + } + + /** + * Cache-generation tag for an actor: the single counter value for a + * plain user actor (key format unchanged: `g`), or the dependent + * counters joined with '.' for derived actors (e.g. `g.`). + * Joined rather than summed so distinct counter states can never + * collide on the same tag. + */ + async #cacheGenerationTag(actor: Actor): Promise { + const keys = this.#cacheGenerationKeys(actor); + if (keys.length === 1) { + return this.stores.permission.getCacheGeneration(keys[0]); + } + const gens = await Promise.all( + keys.map((k) => this.stores.permission.getCacheGeneration(k)), + ); + return gens.join('.'); + } /** Bump a plain user holder (`user:`). */ async #bumpUserCacheGeneration(userUuid: string): Promise { diff --git a/src/backend/services/permission/consts.ts b/src/backend/services/permission/consts.ts index 3957853c8..57655e213 100644 --- a/src/backend/services/permission/consts.ts +++ b/src/backend/services/permission/consts.ts @@ -41,6 +41,18 @@ export const PERMISSION_SCAN_CACHE_TTL_SECONDS = 20; */ export const PERMISSION_CACHE_GENERATION_TTL_SECONDS = 24 * 60 * 60; +/** + * TTL (seconds) for flat user-permission entries written by the scan-path + * cache warm (`validateUserPerms`), as opposed to entries written by an + * explicit grant, which are authoritative and permanent. A warm is derived + * from a SQL traversal, so a warm that races a concurrent revoke (its KV + * write landing after the revoke's flat delete) can re-materialize a + * just-revoked grant. The expiry bounds that failure to this window — + * after it lapses the next scan re-derives from SQL, which the revoke + * deletes synchronously — instead of letting it persist indefinitely. + */ +export const FLAT_PERM_WARM_TTL_SECONDS = 60; + /** * TTL (seconds) for the per-node in-process cache of the generation * counter. Permission checks are very hot, so reading the counter from diff --git a/src/backend/stores/permission/PermissionStore.ts b/src/backend/stores/permission/PermissionStore.ts index 97e72bf6f..61ae0fd63 100644 --- a/src/backend/stores/permission/PermissionStore.ts +++ b/src/backend/stores/permission/PermissionStore.ts @@ -127,18 +127,26 @@ export class PermissionStore extends PuterStore { ); } - /** Write a single flat user-to-user permission entry to KV. */ + /** + * Write a single flat user-to-user permission entry to KV. + * + * `opts.expireAt` (epoch seconds) marks the entry as a derived cache + * warm rather than an authoritative grant: warms self-expire so a warm + * that raced a concurrent revoke cannot re-materialize the grant + * indefinitely. Grant-path writes omit it and are permanent. + */ async setFlatUserPerm( holderUserId: number, permission: string, value: FlatPermValue, + opts: { expireAt?: number } = {}, ): Promise { const key = PermissionUtil.join( PERM_KEY_PREFIX, String(holderUserId), permission, ); - await this.stores.kv.set({ key, value }); + await this.stores.kv.set({ key, value, expireAt: opts.expireAt }); } /** Delete a single flat user-to-user permission entry from KV. */ @@ -563,6 +571,11 @@ export class PermissionStore extends PuterStore { // no CROSSSLOT pattern scan) instantly orphans every cached reading for // that actor: subsequent lookups compute a new key and miss. // + // Derived actors (app-under-user, access-token) fold the generations of + // every actor they act through into their cache keys — see + // PermissionService's cacheGenerationTag — so a bump of `user:` + // also orphans that user's app and token actors' readings. + // // The authoritative counter lives in shared Redis. Permission checks are // extremely hot, so each node keeps a tiny in-process cache (the kv.js // singleton, ~2s TTL) in front of the Redis read — this collapses the @@ -632,7 +645,7 @@ export class PermissionStore extends PuterStore { buildScanCacheKey( actorUid: string, permissionOptions: string[], - generation = 0, + generation: number | string = 0, ): string { return PermissionUtil.join( 'permission-scan', @@ -680,7 +693,7 @@ export class PermissionStore extends PuterStore { #checkCacheKey( actorUid: string, permission: string, - generation = 0, + generation: number | string = 0, ): string { return PermissionUtil.join( 'permission-check', @@ -694,7 +707,7 @@ export class PermissionStore extends PuterStore { async getMultiCheckCache( actorUid: string, permissions: string[], - generation = 0, + generation: number | string = 0, ): Promise> { const out = new Map(); if (permissions.length === 0) return out; @@ -720,7 +733,7 @@ export class PermissionStore extends PuterStore { async setMultiCheckCache( actorUid: string, entries: Array<{ permission: string; granted: boolean }>, - generation = 0, + generation: number | string = 0, ttlSeconds: number = PERMISSION_SCAN_CACHE_TTL_SECONDS, ): Promise { if (entries.length === 0) return;