diff --git a/src/backend/services/share/ShareService.test.ts b/src/backend/services/share/ShareService.test.ts index bb137538e..f4187a828 100644 --- a/src/backend/services/share/ShareService.test.ts +++ b/src/backend/services/share/ShareService.test.ts @@ -1251,6 +1251,97 @@ describe('ShareService', () => { }); }); + // The other derived actor: same reach bound, a different arm of the check. + describe('a token is bounded by what it was minted for', () => { + /** Mint a token and resolve it the way an authenticated request does. */ + const asToken = async ( + owner: { actor: Actor }, + permissions: Array<[string]>, + ) => { + const token = await runWithContext({ actor: owner.actor }, () => + server.services.auth.createAccessToken( + owner.actor, + permissions, + { label: 'share-test' }, + ), + ); + const actor = + await server.services.auth.authenticateFromToken(token); + if (!actor) throw new Error('token did not resolve to an actor'); + return actor; + }; + + it('shares a file the token carries', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const file = await makeFile(owner.user); + const actor = await asToken(owner, [[`fs:${file.uuid}:read`]]); + + const result = await share(actor, { + uid: file.uuid, + recipient: { username: recipient.user.username }, + mode: 'read', + }); + expect(result.mode).toBe('read'); + expect(await canRead(recipient.actor, file.path)).toBe(true); + }); + + it('refuses a file of its issuer’s that the token does not carry', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const carried = await makeFile(owner.user); + const other = await makeFile(owner.user); + const actor = await asToken(owner, [[`fs:${carried.uuid}:read`]]); + + await expect( + share(actor, { + uid: other.uuid, + recipient: { username: recipient.user.username }, + mode: 'read', + }), + ).rejects.toMatchObject({ statusCode: 404 }); + expect(await canRead(recipient.actor, other.path)).toBe(false); + }); + + it('cannot hand out more than it holds', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const file = await makeFile(owner.user); + const actor = await asToken(owner, [[`fs:${file.uuid}:read`]]); + + await expect( + share(actor, { + uid: file.uuid, + recipient: { username: recipient.user.username }, + mode: 'write', + }), + ).rejects.toMatchObject({ statusCode: 403 }); + expect(await canRead(recipient.actor, file.path)).toBe(false); + }); + + it('cannot withdraw a share on a file it does not carry', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const carried = await makeFile(owner.user); + const other = await makeFile(owner.user); + await share(owner.actor, { + uid: other.uuid, + recipient: { username: recipient.user.username }, + mode: 'read', + }); + + const actor = await asToken(owner, [[`fs:${carried.uuid}:read`]]); + await expect( + unshare(actor, { + uid: other.uuid, + recipient: { username: recipient.user.username }, + }), + ).rejects.toMatchObject({ statusCode: 404 }); + // The share it could not reach is still standing. + expect(await canRead(recipient.actor, other.path)).toBe(true); + }); + }); + it('retires grants when the entry is deleted', async () => { const owner = await makeUser(); const recipient = await makeUser(); @@ -1489,6 +1580,65 @@ describe('ShareService', () => { expect(after.items.map((i) => i.entryUid)).not.toContain(file.uuid); }); + // One entry's answer must not vouch for another's in the batched read. + it('drops a withdrawn listing even when another share survives', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const withdrawn = await makeFile(owner.user); + const kept = await makeFile(owner.user); + + for (const file of [withdrawn, kept]) { + await share(owner.actor, { + uid: file.uuid, + recipient: { email: recipient.email }, + mode: 'read', + }); + } + + await server.services.permission.revokeUserUserPermission( + owner.actor, + recipient.user.username!, + `fs:${withdrawn.uuid}:read`, + ); + expect(await canRead(recipient.actor, withdrawn.path)).toBe(false); + expect(await canRead(recipient.actor, kept.path)).toBe(true); + + const after = await server.services.share.listSharedWithMe( + recipient.actor, + ); + const listed = after.items.map((i) => i.entryUid); + expect(listed).toContain(kept.uuid); + expect(listed).not.toContain(withdrawn.uuid); + }); + + // The owner's view of the same withdrawal. + it('stops naming a holder whose grant was withdrawn outside the index', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const file = await makeFile(owner.user); + + await share(owner.actor, { + uid: file.uuid, + recipient: { email: recipient.email }, + mode: 'read', + }); + await server.services.permission.revokeUserUserPermission( + owner.actor, + recipient.user.username!, + `fs:${file.uuid}:read`, + ); + expect(await canRead(recipient.actor, file.path)).toBe(false); + + const shares = await runWithContext({ actor: owner.actor }, () => + server.services.share.listSharesOf(owner.actor, { + uid: file.uuid, + }), + ); + expect(shares.map((s) => s.holder.username)).not.toContain( + recipient.user.username, + ); + }); + it('lets a recipient leave a share that was never indexed', async () => { const owner = await makeUser(); const recipient = await makeUser(); diff --git a/src/backend/services/share/ShareService.ts b/src/backend/services/share/ShareService.ts index db4c1bb10..1df340c9d 100644 --- a/src/backend/services/share/ShareService.ts +++ b/src/backend/services/share/ShareService.ts @@ -47,6 +47,17 @@ export interface ShareTarget { uid?: string; } +/** A `share` row, as much of it as this service reads back. */ +interface ShareIndexRow { + uid: string; + mode: string; + holder_user_id: number; + issuer_user_id: number; + fsentry_id: number; + created_at?: unknown; + data?: unknown; +} + export interface ShareInput extends ShareTarget { recipient: ShareRecipient; mode: AclMode; @@ -618,10 +629,10 @@ export class ShareService extends PuterService { const uuid = uuidFromEntryPermission(row.permission); if (uuid) live.add(uuid); } - for (let i = 0; i < wanted.length; i++) { - const value = flat[i]; - if (!value || value.deleted) continue; - const uuid = uuidFromEntryPermission(wanted[i]); + // Not positional against `wanted`: misses are dropped, keys deduped. + for (const value of flat) { + if (!value?.permission || value.deleted) continue; + const uuid = uuidFromEntryPermission(value.permission); if (uuid) live.add(uuid); } // An owner listing something shared *to* them can't happen, but a @@ -633,6 +644,37 @@ export class ShareService extends PuterService { return live; } + /** The `:` pairs whose grant is still standing. */ + async #reachingHolders( + rows: ShareIndexRow[], + nodeById: Map, + ): Promise> { + const nodesByHolder = new Map>(); + for (const row of rows) { + const holderId = Number(row.holder_user_id); + const node = nodeById.get(Number(row.fsentry_id)); + if (!node || !Number.isFinite(holderId)) continue; + const nodes = nodesByHolder.get(holderId) ?? new Map(); + nodes.set(node.id as number, node); + nodesByHolder.set(holderId, nodes); + } + + const live = new Set(); + await Promise.all( + [...nodesByHolder].map(async ([holderId, nodes]) => { + const uuids = await this.#liveGrants(holderId, [ + ...nodes.values(), + ]); + for (const node of nodes.values()) { + if (uuids.has(node.uuid)) { + live.add(`${holderId}:${node.id}`); + } + } + }), + ); + return live; + } + /** * Withdraw a recipient's access. An owner may clear any issuer's share of * their node; anyone else may only clear the ones they issued. @@ -986,13 +1028,15 @@ export class ShareService extends PuterService { maskEntryPath(node), ]), ); - const inherited: Array<{ row: Record; via: string }> = - (await this.stores.share.listByFsentries([...viaById.keys()])).map( - (row: { fsentry_id: number }) => ({ - row, - via: viaById.get(Number(row.fsentry_id)) as string, - }), - ); + const nodeById = new Map( + [entry, ...ancestorNodes.values()].map((node) => [node.id, node]), + ); + const inherited: Array<{ row: ShareIndexRow; via: string }> = ( + await this.stores.share.listByFsentries([...viaById.keys()]) + ).map((row: ShareIndexRow) => ({ + row, + via: viaById.get(Number(row.fsentry_id)) as string, + })); const rows = await this.stores.share.listByFsentry(entry.id); const pendingRows = await this.stores.share.listPendingOnFsentry( @@ -1012,8 +1056,19 @@ export class ShareService extends PuterService { const users = await this.stores.user.getByIds(userIds); const maskedPath = maskEntryPath(entry); - const inheritedShares: ResolvedShare[] = inherited.map( - ({ row, via }) => ({ + // As in `#liveGrants`: an index row outlives the grant it records. + const stillReaches = await this.#reachingHolders( + [...rows, ...inherited.map((i) => i.row)], + nodeById, + ); + const isLive = (row: ShareIndexRow): boolean => + stillReaches.has( + `${Number(row.holder_user_id)}:${Number(row.fsentry_id)}`, + ); + + const inheritedShares: ResolvedShare[] = inherited + .filter(({ row }) => isLive(row)) + .map(({ row, via }) => ({ uid: String(row.uid), mode: String(row.mode), path: maskedPath, @@ -1032,10 +1087,9 @@ export class ShareService extends PuterService { inheritedFrom: via, modified: entry.modified, size: entry.size, - }), - ); + })); - const own: ResolvedShare[] = rows.map( + const own: ResolvedShare[] = rows.filter(isLive).map( (row: { uid: string; mode: string;