diff --git a/src/backend/services/permission/PermissionService.test.ts b/src/backend/services/permission/PermissionService.test.ts index 762f71736..233997148 100644 --- a/src/backend/services/permission/PermissionService.test.ts +++ b/src/backend/services/permission/PermissionService.test.ts @@ -943,6 +943,116 @@ describe('PermissionService (integration)', () => { ); }); + it('grantUserUserPermission persists the linked SQL row before resolving', async () => { + const { user: issuer, actor: issuerActor } = await makeUserActor(); + const { user: target } = await makeUserActor(); + const permission = `zztest:grant-sync-${uuidv4()}:ii:read`; + await grantManage(issuer, permission); + + await runWithContext({ actor: issuerActor }, () => + permService.grantUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + + // A fire-and-forget upsert leaves the flat view claiming a grant + // that nothing durable backs — losing it if the entry is ever + // dropped, with no SQL row to re-derive from. + const rows = await server.stores.permission.readLinkedUserUserPerms( + target.id, + [permission], + ); + expect(rows).toHaveLength(1); + expect(rows[0].issuer_user_id).toBe(issuer.id); + }); + + it('grantUserUserPermission surfaces a failed SQL upsert instead of swallowing it', async () => { + const { user: issuer, actor: issuerActor } = await makeUserActor(); + const { user: target, actor: targetActor } = await makeUserActor(); + const permission = `zztest:grant-fail-${uuidv4()}:ii:read`; + await grantManage(issuer, permission); + + const spy = vi + .spyOn(server.stores.permission, 'upsertUserUserPerm') + .mockRejectedValue(new Error('simulated db failure')); + try { + await expect( + runWithContext({ actor: issuerActor }, () => + permService.grantUserUserPermission( + issuerActor, + target.username, + permission, + ), + ), + ).rejects.toThrow('simulated db failure'); + } finally { + spy.mockRestore(); + } + + // Fails closed: the durable write went first, so a failure there + // leaves no flat entry granting access either. + expect(await permService.check(targetActor, permission)).toBeFalsy(); + }); + + it('reports whether a grant was actually removed', async () => { + const { user: issuer, actor: issuerActor } = await makeUserActor(); + const { user: target } = await makeUserActor(); + const permission = `zztest:rvk-reports-${uuidv4()}:ii:read`; + await grantManage(issuer, permission); + await runWithContext({ actor: issuerActor }, () => + permService.grantUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + + const first = await runWithContext({ actor: issuerActor }, () => + permService.revokeUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + expect(first).toBe(true); + + // Nothing left to revoke — still not an error, but it must not + // claim to have removed something. + const second = await runWithContext({ actor: issuerActor }, () => + permService.revokeUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + expect(second).toBe(false); + }); + + it('writes no audit row for a revoke that matched nothing', async () => { + const { user: issuer, actor: issuerActor } = await makeUserActor(); + const { user: target } = await makeUserActor(); + const permission = `zztest:rvk-noaudit-${uuidv4()}:ii:read`; + await grantManage(issuer, permission); + + // Never granted, so there is no row to remove. + const revoked = await runWithContext({ actor: issuerActor }, () => + permService.revokeUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + expect(revoked).toBe(false); + + const rows = await server.clients.db.read( + 'SELECT `action` FROM `audit_user_to_user_permissions` WHERE `holder_user_id` = ? AND `permission` = ?', + [target.id, permission], + ); + expect(rows).toHaveLength(0); + }); + 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(); diff --git a/src/backend/services/permission/PermissionService.ts b/src/backend/services/permission/PermissionService.ts index 1868b37a8..00ea79a95 100644 --- a/src/backend/services/permission/PermissionService.ts +++ b/src/backend/services/permission/PermissionService.ts @@ -845,7 +845,15 @@ export class PermissionService extends PuterService { }); const issuerId = actor.user.id; - // Flat upsert (awaited so callers see immediate effect) + // Durable row before the flat view, both awaited. A `manage:`-only + // delegate's grant resolves via flat and not via the linked chain, so + // writing SQL first makes a partial failure fail closed. + await this.stores.permission.upsertUserUserPerm( + user.id, + issuerId, + permission, + extra, + ); await this.stores.permission.setFlatUserPerm(user.id, permission, { ...extra, issuer_user_id: issuerId, @@ -853,10 +861,7 @@ export class PermissionService extends PuterService { deleted: false, }); - // Linked upsert + audit fire-and-forget. - this.stores.permission - .upsertUserUserPerm(user.id, issuerId, permission, extra) - .catch(() => {}); + // Off the critical path, but a silent drop makes the log untrustworthy. this.stores.permission .auditUserUserPerm({ holder_user_id: user.id, @@ -865,18 +870,28 @@ export class PermissionService extends PuterService { action: 'grant', reason: meta.reason ?? 'granted via PermissionService', }) - .catch(() => {}); + .catch((err) => { + console.warn( + '[PermissionService] failed to audit user-user grant:', + err, + ); + }); // Bust any cached "denied" reading so the grant is live immediately. if (user.uuid) await this.#bumpUserCacheGeneration(user.uuid); } + /** + * Returns whether a grant was actually removed. Matching nothing isn't an + * error (an owner has no grant row to delete), but callers must be able to + * tell rather than reporting a removal that didn't happen. + */ async revokeUserUserPermission( actor: Actor, username: string, permission: string, meta: GrantMeta = {}, - ): Promise { + ): Promise { permission = await this.rewritePermission(permission); const user = await this.stores.user.getByUsername(username); if (!user) @@ -903,22 +918,33 @@ export class PermissionService extends PuterService { // 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( + const revoked = await this.stores.permission.deleteUserUserPermByHolder( user.id, permission, ); - this.stores.permission - .auditUserUserPerm({ - holder_user_id: user.id, - issuer_user_id: issuerId, - permission, - action: 'revoke', - reason: meta.reason ?? 'revoked via PermissionService', - }) - .catch(() => {}); - // The holder loses access on their next check, not after the TTL. + // Only record a revoke that happened. + if (revoked) { + this.stores.permission + .auditUserUserPerm({ + holder_user_id: user.id, + issuer_user_id: issuerId, + permission, + action: 'revoke', + reason: meta.reason ?? 'revoked via PermissionService', + }) + .catch((err) => { + console.warn( + '[PermissionService] failed to audit user-user revoke:', + err, + ); + }); + } + + // Unconditional: the flat delete above can't report what it removed, so + // skipping the bump on a no-op risks leaving a cached allow standing. if (user.uuid) await this.#bumpUserCacheGeneration(user.uuid); + return revoked; } /** diff --git a/src/backend/stores/permission/PermissionStore.test.ts b/src/backend/stores/permission/PermissionStore.test.ts index a9fe1d158..1d8bdde9e 100644 --- a/src/backend/stores/permission/PermissionStore.test.ts +++ b/src/backend/stores/permission/PermissionStore.test.ts @@ -651,6 +651,129 @@ describe('PermissionStore', () => { expect(await store.getScanCache(nextKey)).toBeNull(); }); + it('announces a bump so peer regions can bump their own counter', async () => { + const actorUid = `actor-${uuidv4()}`; + const seen: unknown[] = []; + server.clients.event.on( + 'outer.permission.generationBumped', + (_key, data) => { + seen.push(data); + }, + ); + + await store.bumpCacheGeneration(actorUid); + expect(seen).toContainEqual({ actorUid }); + }); + + it('applies a remote bump without re-announcing it', async () => { + const actorUid = `actor-${uuidv4()}`; + const before = await store.getCacheGeneration(actorUid); + let announced = 0; + server.clients.event.on( + 'outer.permission.generationBumped', + (_key, _data, meta) => { + if (!(meta as { from_outside?: boolean })?.from_outside) { + announced++; + } + }, + ); + + // What BroadcastService does with an inbound webhook event. + await server.clients.event.emitAndWait( + 'outer.permission.generationBumped', + { actorUid }, + { from_outside: true }, + ); + + expect(await store.getCacheGeneration(actorUid)).toBeGreaterThan( + before, + ); + // Re-announcing would ping-pong between regions forever. + expect(announced).toBe(0); + }); + + it('ignores a locally-emitted bump event', async () => { + const actorUid = `actor-${uuidv4()}`; + await server.clients.event.emitAndWait( + 'outer.permission.generationBumped', + { actorUid }, + {}, + ); + expect(await store.getCacheGeneration(actorUid)).toBe(0); + }); + + it('announces a flat delete so peer regions drop their own copy', async () => { + const holder = await makeUser(); + await store.setFlatUserPerm(holder.id, 'fs:u:read', { + permission: 'fs:u:read', + deleted: false, + } as never); + + const seen: unknown[] = []; + server.clients.event.on( + 'outer.permission.flatInvalidated', + (_key, data) => { + seen.push(data); + }, + ); + + await store.delFlatUserPerm(holder.id, 'fs:u:read'); + expect(seen).toContainEqual({ + holderUserId: holder.id, + permission: 'fs:u:read', + }); + }); + + it('applies a remote flat delete without re-announcing it', async () => { + const holder = await makeUser(); + await store.setFlatUserPerm(holder.id, 'fs:u:read', { + permission: 'fs:u:read', + deleted: false, + } as never); + expect( + await store.getFlatUserPerms(holder.id, ['fs:u:read']), + ).toHaveLength(1); + + let announced = 0; + server.clients.event.on( + 'outer.permission.flatInvalidated', + (_key, _data, meta) => { + if (!(meta as { from_outside?: boolean })?.from_outside) { + announced++; + } + }, + ); + + await server.clients.event.emitAndWait( + 'outer.permission.flatInvalidated', + { holderUserId: holder.id, permission: 'fs:u:read' }, + { from_outside: true }, + ); + + expect( + await store.getFlatUserPerms(holder.id, ['fs:u:read']), + ).toEqual([]); + expect(announced).toBe(0); + }); + + it('ignores a locally-emitted flat invalidation', async () => { + const holder = await makeUser(); + await store.setFlatUserPerm(holder.id, 'fs:u:read', { + permission: 'fs:u:read', + deleted: false, + } as never); + + await server.clients.event.emitAndWait( + 'outer.permission.flatInvalidated', + { holderUserId: holder.id, permission: 'fs:u:read' }, + {}, + ); + + expect( + await store.getFlatUserPerms(holder.id, ['fs:u:read']), + ).toHaveLength(1); + }); + it('drops a scan cache entry on explicit invalidation', async () => { const key = store.buildScanCacheKey(`actor-${uuidv4()}`, ['p'], 0); await store.setScanCache(key, { allowed: false }); @@ -737,5 +860,95 @@ describe('PermissionStore', () => { [], ); }); + + it('deletes every grant at or beneath a permission prefix', async () => { + const issuer = await makeUser(); + const holder = await makeUser(); + const other = await makeUser(); + const uid = uuidv4(); + + for (const [h, perm] of [ + [holder, `fs:${uid}:read`], + [other, `fs:${uid}:write`], + [holder, `fs:${uid}`], + ] as const) { + await store.upsertUserUserPerm(h.id, issuer.id, perm, {}); + await store.setFlatUserPerm(h.id, perm, { + permission: perm, + deleted: false, + } as never); + } + // A different entry must survive. + const keeper = `fs:${uuidv4()}:read`; + await store.upsertUserUserPerm(holder.id, issuer.id, keeper, {}); + + const removed = await store.deleteUserUserPermsByPermissionPrefix( + `fs:${uid}`, + ); + + expect(removed).toHaveLength(3); + expect( + await store.readLinkedUserUserPerms(holder.id, [ + `fs:${uid}:read`, + `fs:${uid}`, + ]), + ).toEqual([]); + expect( + await store.getFlatUserPerms(holder.id, [`fs:${uid}:read`]), + ).toEqual([]); + expect( + await store.readLinkedUserUserPerms(holder.id, [keeper]), + ).toHaveLength(1); + }); + + it('does not let a wildcard in the prefix widen the match', async () => { + const issuer = await makeUser(); + const holder = await makeUser(); + const victim = `fs:${uuidv4()}:read`; + await store.upsertUserUserPerm(holder.id, issuer.id, victim, {}); + + // `_` and `%` are LIKE wildcards; unescaped they would match this. + expect( + await store.deleteUserUserPermsByPermissionPrefix('fs:%'), + ).toEqual([]); + expect( + await store.deleteUserUserPermsByPermissionPrefix('fs:_'), + ).toEqual([]); + expect( + await store.readLinkedUserUserPerms(holder.id, [victim]), + ).toHaveLength(1); + }); + + it('returns an empty list when the prefix matches nothing', async () => { + expect( + await store.deleteUserUserPermsByPermissionPrefix( + `fs:${uuidv4()}`, + ), + ).toEqual([]); + }); + + it('reports whether the delete matched a row', async () => { + const issuer = await makeUser(); + const holder = await makeUser(); + await store.upsertUserUserPerm( + holder.id, + issuer.id, + 'fs:u:read', + {}, + ); + + expect( + await store.deleteUserUserPermByHolder(holder.id, 'fs:u:read'), + ).toBe(true); + expect( + await store.deleteUserUserPermByHolder(holder.id, 'fs:u:read'), + ).toBe(false); + expect( + await store.deleteUserUserPermByHolder( + holder.id, + 'fs:never-granted:read', + ), + ).toBe(false); + }); }); }); diff --git a/src/backend/stores/permission/PermissionStore.ts b/src/backend/stores/permission/PermissionStore.ts index 2f7326204..63581ea2e 100644 --- a/src/backend/stores/permission/PermissionStore.ts +++ b/src/backend/stores/permission/PermissionStore.ts @@ -98,6 +98,48 @@ export interface AuditEntry { export class PermissionStore extends PuterStore { declare protected stores: LayerInstances; + override onServerStart(): void { + this.#subscribeRemoteGenerationBumps(); + this.#subscribeRemoteFlatInvalidations(); + } + + /** + * Apply a peer region's flat-permission delete. Independent of whether the + * KV table replicates: a redundant delete is a no-op, a needed one is the + * only thing that makes the revoke real there. + */ + #subscribeRemoteFlatInvalidations(): void { + this.clients.event.on( + 'outer.permission.flatInvalidated', + (_key, data, meta) => { + if (!(meta as { from_outside?: boolean })?.from_outside) return; + const { holderUserId, permission } = (data ?? {}) as { + holderUserId?: unknown; + permission?: unknown; + }; + if (typeof holderUserId !== 'number') return; + if (typeof permission !== 'string' || permission === '') return; + void this.#applyFlatUserPermDelete(holderUserId, permission); + }, + ); + } + + /** + * Apply a peer region's cache-generation bump. Our own emit reaches local + * listeners too, and that half already ran before it went out. + */ + #subscribeRemoteGenerationBumps(): void { + this.clients.event.on( + 'outer.permission.generationBumped', + (_key, data, meta) => { + if (!(meta as { from_outside?: boolean })?.from_outside) return; + const actorUid = (data as { actorUid?: unknown })?.actorUid; + if (typeof actorUid !== 'string' || actorUid === '') return; + void this.#applyCacheGenerationBump(actorUid); + }, + ); + } + // -- Flat view (KV under system namespace) ------------------------ /** @@ -154,6 +196,23 @@ export class PermissionStore extends PuterStore { async delFlatUserPerm( holderUserId: number, permission: string, + ): Promise { + await this.#applyFlatUserPermDelete(holderUserId, permission); + try { + this.clients.event.emit( + 'outer.permission.flatInvalidated', + { holderUserId, permission }, + {}, + ); + } catch { + // Peer regions keep the entry until their KV replicates the delete. + } + } + + /** Local half of a flat delete. Never emits, so a remote one can't loop. */ + async #applyFlatUserPermDelete( + holderUserId: number, + permission: string, ): Promise { const key = PermissionUtil.join( PERM_KEY_PREFIX, @@ -201,17 +260,73 @@ export class PermissionStore extends PuterStore { }); } + /** Returns whether a row was actually deleted. */ async deleteUserUserPermByHolder( holderUserId: number, permission: string, - ): Promise { - await this.clients.db.write( + ): Promise { + const result = await this.clients.db.write( 'DELETE FROM `user_to_user_permissions` WHERE `holder_user_id` = ? AND `permission` = ?', [holderUserId, permission], ); + if (!result.anyRowsAffected) return false; await this.publishCacheKeys({ keys: [this.#u2uCacheKey(holderUserId)], }); + return true; + } + + /** + * Delete every user-to-user grant at or beneath `permission`, clearing the + * flat KV view too, and return the rows removed so the caller can audit + * them and bust caches. + * + * The subject lives in the permission text rather than a column, so no + * foreign key can cascade it — this is how a deleted fsentry's grants get + * withdrawn. `permission` has no index, so this is a table scan: fine on + * deletion, never on a hot path. + */ + async deleteUserUserPermsByPermissionPrefix(permission: string): Promise< + Array<{ + holder_user_id: number; + issuer_user_id: number; + permission: string; + }> + > { + // See deleteAppGrantsByPermissionPrefix for why `!` is the escape. + const escaped = permission.replace(/([!%_])/g, '!$1'); + const prefix = `${escaped}:%`; + + const rows = (await this.clients.db.read( + 'SELECT `holder_user_id`, `issuer_user_id`, `permission` FROM `user_to_user_permissions` ' + + "WHERE `permission` = ? OR `permission` LIKE ? ESCAPE '!'", + [permission, prefix], + )) as Array<{ + holder_user_id: number; + issuer_user_id: number; + permission: string; + }>; + if (rows.length === 0) return []; + + await this.clients.db.write( + 'DELETE FROM `user_to_user_permissions` ' + + "WHERE `permission` = ? OR `permission` LIKE ? ESCAPE '!'", + [permission, prefix], + ); + + // Via delFlatUserPerm so peer regions hear about it too. + await Promise.all( + rows.map((row) => + this.delFlatUserPerm(row.holder_user_id, row.permission), + ), + ); + + const keys = [ + ...new Set(rows.map((r) => this.#u2uCacheKey(r.holder_user_id))), + ]; + if (keys.length > 0) await this.publishCacheKeys({ keys }); + + return rows; } async auditUserUserPerm( @@ -689,6 +804,20 @@ export class PermissionStore extends PuterStore { } async bumpCacheGeneration(actorUid: string): Promise { + await this.#applyCacheGenerationBump(actorUid); + try { + this.clients.event.emit( + 'outer.permission.generationBumped', + { actorUid }, + {}, + ); + } catch { + // Peer regions fall back to their scan-cache TTL. + } + } + + /** Local half of a bump. Never emits, so a remote bump can't ping-pong. */ + async #applyCacheGenerationBump(actorUid: string): Promise { const key = this.#cacheGenerationKey(actorUid); try { const next = await this.clients.redis.incr(key);