mirror of
https://github.com/HeyPuter/puter.git
synced 2026-08-29 01:16:43 +00:00
fix(permissions): decide a flat delete from the primary, not a lagging replica
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
3eb682790d
commit
aa983446fb
@@ -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();
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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<LinkedUserUserPermRow[]> {
|
||||
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<LinkedUserUserPermRow>(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,
|
||||
|
||||
Reference in New Issue
Block a user