diff --git a/src/backend/services/permission/PermissionService.test.ts b/src/backend/services/permission/PermissionService.test.ts index 233997148..e68f0e4c2 100644 --- a/src/backend/services/permission/PermissionService.test.ts +++ b/src/backend/services/permission/PermissionService.test.ts @@ -321,6 +321,44 @@ describe('PermissionService (integration)', () => { ).rejects.toMatchObject({ statusCode: 404 }); }); + it('lets a holder give up a permission it cannot manage', async () => { + const { user: issuer, actor: issuerActor } = await makeUserActor(); + const { user: target, actor: targetActor } = await makeUserActor(); + const permission = `zztest:self-revoke-${uuidv4()}:ii:read`; + await server.stores.permission.setFlatUserPerm( + issuer.id, + `manage:${permission}`, + { + permission: `manage:${permission}`, + deleted: false, + issuer_user_id: issuer.id, + } as never, + ); + await runWithContext({ actor: issuerActor }, () => + permService.grantUserUserPermission( + issuerActor, + target.username, + permission, + ), + ); + expect(await permService.check(targetActor, permission)).toBe(true); + + // The holder has no manage authority here — renouncing access is + // allowed anyway, since it can only narrow their own reach. + await runWithContext({ actor: targetActor }, () => + permService.revokeUserUserPermission( + targetActor, + target.username, + permission, + {}, + { issuerUserId: issuer.id }, + ), + ); + expect( + await permService.check(targetActor, permission), + ).toBeFalsy(); + }); + it('revokeUserUserPermission throws 403 when the issuer lacks manage', async () => { const { actor: issuer } = await makeUserActor(); const { user: target } = await makeUserActor(); @@ -996,6 +1034,48 @@ describe('PermissionService (integration)', () => { expect(await permService.check(targetActor, permission)).toBeFalsy(); }); + it('keeps the flat entry while another issuer still grants the permission', async () => { + const { user: issuerA, actor: actorA } = await makeUserActor(); + const { user: issuerB, actor: actorB } = await makeUserActor(); + const { user: target, actor: targetActor } = await makeUserActor(); + const permission = `zztest:two-issuers-${uuidv4()}:ii:read`; + await grantManage(issuerA, permission); + await grantManage(issuerB, permission); + + for (const actor of [actorA, actorB]) { + await runWithContext({ actor }, () => + permService.grantUserUserPermission( + actor, + target.username, + permission, + ), + ); + } + + await runWithContext({ actor: actorA }, () => + permService.revokeUserUserPermission( + actorA, + target.username, + permission, + ), + ); + + // B's grant stands, so the shared flat key must survive with it — + // the key isn't issuer-scoped and B may not resolve via the chain. + expect(await permService.check(targetActor, permission)).toBe(true); + + await runWithContext({ actor: actorB }, () => + permService.revokeUserUserPermission( + actorB, + target.username, + permission, + ), + ); + 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(); diff --git a/src/backend/services/permission/PermissionService.ts b/src/backend/services/permission/PermissionService.ts index 00ea79a95..7b7bfc7c5 100644 --- a/src/backend/services/permission/PermissionService.ts +++ b/src/backend/services/permission/PermissionService.ts @@ -882,6 +882,10 @@ export class PermissionService extends PuterService { } /** + * Remove the grant `actor` issued, or the one named by `opts.issuerUserId` + * when the caller has established authority over another issuer's grant (a + * resource owner clearing a delegate's re-grant). + * * 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. @@ -891,6 +895,7 @@ export class PermissionService extends PuterService { username: string, permission: string, meta: GrantMeta = {}, + opts: { issuerUserId?: number } = {}, ): Promise { permission = await this.rewritePermission(permission); const user = await this.stores.user.getByUsername(username); @@ -899,18 +904,24 @@ export class PermissionService extends PuterService { legacyCode: 'subject_does_not_exist', }); - if (!(await this.canManagePermission(actor, permission))) { - throw new HttpError(403, `permission_denied: ${permission}`, { - legacyCode: 'permission_denied', - }); - } if (!actor.user?.id) throw new HttpError(403, 'actor must be a user', { legacyCode: 'forbidden', }); const issuerId = actor.user.id; - await this.stores.permission.delFlatUserPerm(user.id, permission); + // Giving up access you hold needs no authority over the permission — + // it can only ever narrow what you can reach. + const isSelfRevoke = user.id === issuerId; + if ( + !isSelfRevoke && + !(await this.canManagePermission(actor, permission)) + ) { + throw new HttpError(403, `permission_denied: ${permission}`, { + legacyCode: 'permission_denied', + }); + } + // 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 @@ -921,8 +932,21 @@ export class PermissionService extends PuterService { const revoked = await this.stores.permission.deleteUserUserPermByHolder( user.id, permission, + opts.issuerUserId ?? issuerId, ); + // The flat key isn't issuer-scoped, so it may only go once no issuer + // 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], + ); + if (remaining.length === 0) { + await this.stores.permission.delFlatUserPerm(user.id, permission); + } + // Only record a revoke that happened. if (revoked) { this.stores.permission diff --git a/src/backend/stores/permission/PermissionStore.test.ts b/src/backend/stores/permission/PermissionStore.test.ts index 1d8bdde9e..bf0a93c5e 100644 --- a/src/backend/stores/permission/PermissionStore.test.ts +++ b/src/backend/stores/permission/PermissionStore.test.ts @@ -848,7 +848,11 @@ describe('PermissionStore', () => { expect(rows).toHaveLength(1); expect(rows[0].issuer_user_id).toBe(issuer.id); - await store.deleteUserUserPermByHolder(holder.id, 'fs:u:read'); + await store.deleteUserUserPermByHolder( + holder.id, + 'fs:u:read', + issuer.id, + ); expect( await store.readLinkedUserUserPerms(holder.id, ['fs:u:read']), ).toEqual([]); @@ -927,6 +931,40 @@ describe('PermissionStore', () => { ).toEqual([]); }); + it('revokes only the named issuer grant, not another issuer identical one', async () => { + const issuerA = await makeUser(); + const issuerB = await makeUser(); + const holder = await makeUser(); + await store.upsertUserUserPerm( + holder.id, + issuerA.id, + 'fs:shared:read', + {}, + ); + await store.upsertUserUserPerm( + holder.id, + issuerB.id, + 'fs:shared:read', + {}, + ); + + expect( + await store.deleteUserUserPermByHolder( + holder.id, + 'fs:shared:read', + issuerA.id, + ), + ).toBe(true); + + // Two people can grant the same access independently; one of them + // withdrawing must not take the other's grant with it. + const rows = await store.readLinkedUserUserPerms(holder.id, [ + 'fs:shared:read', + ]); + expect(rows).toHaveLength(1); + expect(rows[0].issuer_user_id).toBe(issuerB.id); + }); + it('reports whether the delete matched a row', async () => { const issuer = await makeUser(); const holder = await makeUser(); @@ -938,15 +976,24 @@ describe('PermissionStore', () => { ); expect( - await store.deleteUserUserPermByHolder(holder.id, 'fs:u:read'), + await store.deleteUserUserPermByHolder( + holder.id, + 'fs:u:read', + issuer.id, + ), ).toBe(true); expect( - await store.deleteUserUserPermByHolder(holder.id, 'fs:u:read'), + await store.deleteUserUserPermByHolder( + holder.id, + 'fs:u:read', + issuer.id, + ), ).toBe(false); expect( await store.deleteUserUserPermByHolder( holder.id, 'fs:never-granted:read', + issuer.id, ), ).toBe(false); }); diff --git a/src/backend/stores/permission/PermissionStore.ts b/src/backend/stores/permission/PermissionStore.ts index 63581ea2e..f542722d0 100644 --- a/src/backend/stores/permission/PermissionStore.ts +++ b/src/backend/stores/permission/PermissionStore.ts @@ -260,14 +260,22 @@ export class PermissionStore extends PuterStore { }); } - /** Returns whether a row was actually deleted. */ + /** + * Remove one issuer's grant. Scoped to the issuer because grants are keyed + * on (holder, issuer, permission) — two people can grant the same access + * independently, and one withdrawing must not take the other's with it. + * + * Returns whether a row was actually deleted. + */ async deleteUserUserPermByHolder( holderUserId: number, permission: string, + issuerUserId: number, ): Promise { const result = await this.clients.db.write( - 'DELETE FROM `user_to_user_permissions` WHERE `holder_user_id` = ? AND `permission` = ?', - [holderUserId, permission], + 'DELETE FROM `user_to_user_permissions` WHERE `holder_user_id` = ? ' + + 'AND `permission` = ? AND `issuer_user_id` = ?', + [holderUserId, permission, issuerUserId], ); if (!result.anyRowsAffected) return false; await this.publishCacheKeys({