From aa983446fbf10793aeabcf56d0a7b636b070df49 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Fri, 14 Aug 2026 14:33:49 -0400 Subject: [PATCH] fix(permissions): decide a flat delete from the primary, not a lagging replica MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit revokeUserUserPermission deletes the SQL grant, then only drops the flat KV entry once no issuer still grants the permission. That remaining-check read through the row cache the delete had just invalidated, straight to a replica — under any lag the deleted row reappeared, the flat delete was skipped, and the stale rows were re-cached for another five minutes. Grant-path flat entries carry no TTL, so the holder kept working access with zero SQL rows behind it, invisible to every listing. The check now reads the primary and re-warms the cache with what it actually saw. Co-Authored-By: Claude Fable 5 --- .../permission/PermissionService.test.ts | 48 +++++++++++++++++++ .../services/permission/PermissionService.ts | 15 ++++-- .../stores/permission/PermissionStore.ts | 31 ++++++++++++ 3 files changed, 90 insertions(+), 4 deletions(-) diff --git a/src/backend/services/permission/PermissionService.test.ts b/src/backend/services/permission/PermissionService.test.ts index 79171b9ca..f63ef448f 100644 --- a/src/backend/services/permission/PermissionService.test.ts +++ b/src/backend/services/permission/PermissionService.test.ts @@ -952,6 +952,54 @@ describe('PermissionService (integration)', () => { ); }); + it('drops the flat entry even when a lagging replica still shows the deleted row', async () => { + const { user: issuer, actor: issuerActor } = await makeUserActor(); + const { user: target } = await makeUserActor(); + const permission = `zztest:rvk-lag-${uuidv4()}:ii:read`; + await grantManage(issuer, permission); + await runWithContext({ actor: issuerActor }, () => + permService.grantUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + + // Simulate replica lag at the store boundary (sqlite has no + // replica to lag): the plain read path keeps returning the row + // the revoke just deleted from the primary. The remaining-check + // must go through the primary-read variant instead — trusting + // the replica view (or re-warming the row cache from it) skips + // the flat delete and leaves a no-TTL flat grant standing with + // no SQL rows behind it. + const staleRow = { + holder_user_id: target.id, + issuer_user_id: issuer.id, + permission, + extra: {}, + }; + const spy = vi + .spyOn(server.stores.permission, 'readLinkedUserUserPerms') + .mockResolvedValue([staleRow as never]); + try { + await runWithContext({ actor: issuerActor }, () => + permService.revokeUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + } finally { + spy.mockRestore(); + } + + const flat = await server.stores.permission.getFlatUserPerms( + target.id, + [permission], + ); + expect(flat.filter((v) => !v.deleted)).toHaveLength(0); + }); + it('grantUserUserPermission persists the linked SQL row before resolving', async () => { const { user: issuer, actor: issuerActor } = await makeUserActor(); const { user: target } = await makeUserActor(); diff --git a/src/backend/services/permission/PermissionService.ts b/src/backend/services/permission/PermissionService.ts index 109e71181..2e8999dc9 100644 --- a/src/backend/services/permission/PermissionService.ts +++ b/src/backend/services/permission/PermissionService.ts @@ -939,10 +939,17 @@ export class PermissionService extends PuterService { // grants this any more. Dropping it while another grant stands would // cut access outright for a `manage:`-only issuer, whose grant the // linked chain can't resolve. - const remaining = await this.stores.permission.readLinkedUserUserPerms( - user.id, - [permission], - ); + // + // Must read the primary: the delete above just landed there, and a + // replica (or the row cache the plain read would re-warm from it) can + // still show the deleted row. Skipping the flat delete on that stale + // view leaves a no-TTL flat grant standing with no SQL rows behind + // it — permanent, invisible access. + const remaining = + await this.stores.permission.readLinkedUserUserPermsFromPrimary( + user.id, + [permission], + ); if (remaining.length === 0) { await this.stores.permission.delFlatUserPerm(user.id, permission); } diff --git a/src/backend/stores/permission/PermissionStore.ts b/src/backend/stores/permission/PermissionStore.ts index 099e43c70..2c99be15e 100644 --- a/src/backend/stores/permission/PermissionStore.ts +++ b/src/backend/stores/permission/PermissionStore.ts @@ -234,6 +234,37 @@ export class PermissionStore extends PuterStore { return all.filter((row) => wanted.has(row.permission)); } + /** + * Read-after-write variant of {@link readLinkedUserUserPerms}: skips the row + * cache and queries the primary, so a check that immediately follows a + * write on this holder cannot be misled by replica lag or by a stale cached + * row set. Re-warms the cache with what the primary returned — the plain + * read path would otherwise re-cache the replica's stale view. + */ + async readLinkedUserUserPermsFromPrimary( + holderUserId: number, + permissions: string[], + ): Promise { + if (permissions.length === 0) return []; + const rows = await this.clients.db.pread( + 'SELECT * FROM `user_to_user_permissions` WHERE `holder_user_id` = ?', + [holderUserId], + ); + const decoded = rows.map((row) => + this.#decodeExtra(row), + ); + this.clients.redis + .set( + this.#u2uCacheKey(holderUserId), + JSON.stringify(decoded), + 'EX', + U2U_CACHE_TTL_SECONDS, + ) + .catch(() => {}); + const wanted = new Set(permissions); + return decoded.filter((row) => wanted.has(row.permission)); + } + async upsertUserUserPerm( holderUserId: number, issuerUserId: number,