From c35cb427ebdad5ba44ddd2cccf08ba0ee1e15a4e Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Fri, 9 Oct 2026 16:15:48 -0400 Subject: [PATCH] Share attribution and cascade: finish PUT-1856 and PUT-1857 Replayed onto main after #4118 squash-merged. Same tree as the reviewed head (8d8bd871f); only the redundant merges from the old base are gone. --- .../services/share/ShareService.test.ts | 113 ++++++++++++++++-- src/backend/services/share/ShareService.ts | 99 ++++++++++++--- .../services/share/TeamShareNotify.test.ts | 3 +- src/backend/stores/share/ShareStore.js | 98 ++++++++++++--- src/backend/stores/share/ShareStore.test.js | 79 +++++++++--- 5 files changed, 337 insertions(+), 55 deletions(-) diff --git a/src/backend/services/share/ShareService.test.ts b/src/backend/services/share/ShareService.test.ts index 549ad87a0..f67c13bd6 100644 --- a/src/backend/services/share/ShareService.test.ts +++ b/src/backend/services/share/ShareService.test.ts @@ -1178,9 +1178,9 @@ describe('ShareService', () => { const listed = await listSharedByMe(owner.actor, { includeTotal: true, }); - expect( - listed.items.map((r) => r.holder?.username), - ).toContain(third.user.username); + expect(listed.items.map((r) => r.holder?.username)).toContain( + third.user.username, + ); expect(listed.total).toBeGreaterThanOrEqual(2); }); @@ -1882,7 +1882,35 @@ describe('ShareService', () => { // One row records one issuance, so attribution follows the most // recent one — in both directions. - it('re-attributes a grant to whoever issued it last', async () => { + it('does not let an app claim a share the user made in person', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const app = await makeApp(owner.user.id); + const file = await makeFile(owner.user); + await grantAppReach(owner, app, file); + + // The user's own share, made without any app involved. + await share(owner.actor, { + uid: file.uuid, + recipient: { username: recipient.user.username }, + mode: 'write', + }); + const [mine] = (await listSharedByMe(owner.actor, { appUid: null })) + .items; + + // The app re-shares the same pair; the row is still the user's. + await share(asApp(owner, app), { + uid: file.uuid, + recipient: { username: recipient.user.username }, + mode: 'read', + }); + expect((await listSharedByMe(asApp(owner, app))).items).toEqual([]); + await expect( + revokeByUid(asApp(owner, app), mine.uid), + ).rejects.toMatchObject({ statusCode: 404 }); + }); + + it('lets a grant lose its app, and never take one back', async () => { const owner = await makeUser(); const recipient = await makeUser(); const app = await makeApp(owner.user.id); @@ -1913,18 +1941,21 @@ describe('ShareService', () => { revokeByUid(asApp(owner, app), uid), ).rejects.toMatchObject({ statusCode: 404 }); - // And back: re-shared through the app, the same row is the app's - // again. + // Not back: re-crediting would let the app withdraw it. await share(asApp(owner, app), { uid: file.uuid, recipient: { username: recipient.user.username }, mode: 'read', }); + expect((await listSharedByMe(asApp(owner, app))).items).toEqual([]); expect( - (await listSharedByMe(asApp(owner, app))).items.map( + (await listSharedByMe(owner.actor, { appUid: null })).items.map( (i) => i.uid, ), ).toEqual([uid]); + await expect( + revokeByUid(asApp(owner, app), uid), + ).rejects.toMatchObject({ statusCode: 404 }); }); // Every uid the caller may not act on answers alike, or the endpoint @@ -2233,6 +2264,73 @@ describe('ShareService', () => { expect(await canRead(third.actor, file.path)).toBe(false); }); + it("leaves a re-share resting on the delegate's own grant below", async () => { + const owner = await makeUser(); + const delegate = await makeUser(); + const third = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + + // Two separate grants: the folder, and the file inside it. + await share(owner.actor, { + uid: dir.uuid, + recipient: { email: delegate.email }, + mode: 'manage', + }); + await share(owner.actor, { + uid: file.uuid, + recipient: { email: delegate.email }, + mode: 'manage', + }); + await share(delegate.actor, { + uid: file.uuid, + recipient: { email: third.email }, + mode: 'read', + }); + expect(await canRead(third.actor, file.path)).toBe(true); + + // The folder grant goes; the one on the file does not. + await unshare(owner.actor, { + uid: dir.uuid, + recipient: { username: delegate.user.username }, + }); + + expect(await canRead(delegate.actor, dir.path)).toBe(false); + expect(await canRead(delegate.actor, file.path)).toBe(true); + expect(await canRead(third.actor, file.path)).toBe(true); + }); + + it("leaves the delegate's invite below a revoked folder standing", async () => { + const owner = await makeUser(); + const delegate = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + + for (const uid of [dir.uuid, file.uuid]) { + await share(owner.actor, { + uid, + recipient: { email: delegate.email }, + mode: 'manage', + }); + } + // An invite, not a grant: nobody has claimed it yet. + await share(delegate.actor, { + uid: file.uuid, + recipient: { email: `nobody-${Math.random()}@test.local` }, + mode: 'read', + }); + const before = await server.stores.share.listPendingOnFsentry(file.id); + expect(before.length).toBeGreaterThan(0); + + await unshare(owner.actor, { + uid: dir.uuid, + recipient: { username: delegate.user.username }, + }); + + // The file grant stands, so the invite made under it does too. + expect( + await server.stores.share.listPendingOnFsentry(file.id), + ).toHaveLength(before.length); + }); + it('keeps the index row when the actor could not revoke anything', async () => { const owner = await makeUser(); const delegate = await makeUser(); @@ -5639,6 +5737,5 @@ describe('ShareService', () => { expect(listed.some((row) => row.pending)).toBe(false); expect(JSON.stringify(listed)).not.toContain(email); }); - }); }); diff --git a/src/backend/services/share/ShareService.ts b/src/backend/services/share/ShareService.ts index f8e7c8780..c2dc0202d 100644 --- a/src/backend/services/share/ShareService.ts +++ b/src/backend/services/share/ShareService.ts @@ -1449,6 +1449,61 @@ export class ShareService extends PuterService { ); } + /** Nodes under `entry` that `userId` manages by a grant of their own. */ + async #nodesManagedDirectly( + entry: FSEntry, + userId: number, + unwinding: Set, + ): Promise> { + // Every row, not just the applied ones: a node whose only share is a + // pending invite or a team grant is exactly what the group and invite + // sweeps ask about. + const rows = await this.stores.share.listAllByFsentrySubtree(entry.id); + const nodes = await this.stores.fsEntry.getEntriesByIds( + rows.map((row: { fsentry_id: number }) => Number(row.fsentry_id)), + ); + const candidates = [...nodes.values()].filter( + (node) => node.id !== entry.id, + ); + if (candidates.length === 0) return new Set(); + + // One read for the whole subtree: the query is holder-scoped, so + // asking per node is the same rows over and over. + const permOf = (node: FSEntry) => + entryPermissionForMode(node.uuid, MANAGE_PERM_PREFIX); + const wanted = candidates.map(permOf); + const [linked, viaGroup] = await Promise.all([ + this.stores.permission.readLinkedUserUserPermsFromPrimary( + userId, + wanted, + ), + this.stores.permission.readUserGroupPerms(userId, wanted), + ]); + // Not from anyone being unwound: two delegates can hold each other up. + const held = new Set([ + ...linked + .filter((row) => !unwinding.has(Number(row.issuer_user_id))) + .map((row) => String(row.permission)), + ...viaGroup.map((row) => String(row.permission)), + ]); + + // `manage` inherits downwards, so a grant on a folder between `entry` + // and the node answers for the node too. + const managedNodes = candidates.filter((node) => + held.has(permOf(node)), + ); + const managed = new Set(); + for (const node of candidates) { + const covered = managedNodes.some( + (held) => + held.id === node.id || + node.path.startsWith(`${held.path}/`), + ); + if (covered) managed.add(node.id); + } + return managed; + } + /** * Withdraw everything `issuerId` granted on this node, and everything those * recipients granted in turn. @@ -1465,19 +1520,6 @@ export class ShareService extends PuterService { if (seen.has(issuerId)) return 0; seen.add(issuerId); - // Their unclaimed invites go the same way as their re-shares: an - // invite rests on the same authority, and nothing else retires it — - // claiming re-checks, but only when the recipient shows up, and until - // then the row keeps the entry in the revoked issuer's listing. - await this.stores.share.deletePendingByIssuerSubtree( - issuerId, - entry.id, - ); - - // What they re-shared to teams goes too: a group grant left behind is - // dormant, and springs back if the issuer ever requalifies. - let revoked = await this.#revokeGroupSharesBy(actor, entry, issuerId); - // The whole subtree, not just this node: `manage` inherits downwards, // so a grant on a descendant can rest on authority held here. const rows = ( @@ -1486,6 +1528,28 @@ export class ShareService extends PuterService { (row: { issuer_user_id: number }) => Number(row.issuer_user_id) === issuerId, ); + + // Nodes they manage in their own right, exempt from all three sweeps. + const exempt = await this.#nodesManagedDirectly(entry, issuerId, seen); + + // Their unclaimed invites go the same way as their re-shares: an + // invite rests on the same authority, and nothing else retires it — + // claiming re-checks, but only when the recipient shows up, and until + // then the row keeps the entry in the revoked issuer's listing. + await this.stores.share.deletePendingByIssuerSubtree( + issuerId, + entry.id, + { exemptFsentryIds: [...exempt] }, + ); + + // What they re-shared to teams goes too: a group grant left behind is + // dormant, and springs back if the issuer ever requalifies. + let revoked = await this.#revokeGroupSharesBy( + actor, + entry, + issuerId, + exempt, + ); if (rows.length === 0) return revoked; const [nodes, holders] = await Promise.all([ @@ -1507,6 +1571,9 @@ export class ShareService extends PuterService { const downstream = holders.get(holderId); if (!node || !downstream?.username) continue; + // Their own authority here, not the one being withdrawn above. + if (node.id !== entry.id && exempt.has(node.id)) continue; + const { revoked: didRevoke, authorized } = await this.#revokeFor( actor, node, @@ -1547,12 +1614,14 @@ export class ShareService extends PuterService { actor: Actor, entry: FSEntry, issuerId: number, + exempt: Set = new Set(), ): Promise { const rows = ( await this.stores.share.listGroupSharesBySubtree(entry.id) ).filter( - (row: { issuer_user_id: number }) => - Number(row.issuer_user_id) === issuerId, + (row: { issuer_user_id: number; fsentry_id: number }) => + Number(row.issuer_user_id) === issuerId && + !exempt.has(Number(row.fsentry_id)), ); if (rows.length === 0) return 0; diff --git a/src/backend/services/share/TeamShareNotify.test.ts b/src/backend/services/share/TeamShareNotify.test.ts index 84b8e62eb..8503c3ceb 100644 --- a/src/backend/services/share/TeamShareNotify.test.ts +++ b/src/backend/services/share/TeamShareNotify.test.ts @@ -208,7 +208,8 @@ describe('announcing a team share', () => { expect( sent.some((mail) => mail.to.includes(fx.b.owner.username)), ).toBe(false); - }); + // Longer than the wait above, which the 5s default cuts short. + }, 30_000); it('does not announce to a member who blocked the sharer', async () => { // A member nobody has notified yet, so a new row is detectable. Reusing diff --git a/src/backend/stores/share/ShareStore.js b/src/backend/stores/share/ShareStore.js index f62ada12e..7345763f5 100644 --- a/src/backend/stores/share/ShareStore.js +++ b/src/backend/stores/share/ShareStore.js @@ -600,17 +600,19 @@ export class ShareStore extends PuterStore { } const existing = await this.clients.db.read( - 'SELECT `uid` FROM `share` WHERE `recipient_email` = ? AND ' + + 'SELECT `uid`, `data` FROM `share` WHERE `recipient_email` = ? AND ' + '`fsentry_id` = ? AND `issuer_user_id` = ? AND ' + '`holder_user_id` IS NULL LIMIT 1', [recipientEmail, fsentryId, issuerUserId], ); - // Same key an active share records the app under, so one reader covers - // an invite and the grant it becomes. Attribution follows the most - // recent issuance: re-inviting refreshes `data`, exactly as - // `upsertActive` does on conflict. + // The key an active share uses, so one reader covers both. + const attribution = existing[0] + ? this.#keptAttribution(existing[0].data, issuerAppUid) + : issuerAppUid + ? { issuedByApp: issuerAppUid } + : {}; const data = JSON.stringify({ - ...(issuerAppUid ? { issuedByApp: issuerAppUid } : {}), + ...attribution, ...(displayEmail && displayEmail !== recipientEmail ? { invitedAddress: displayEmail } : {}), @@ -689,11 +691,21 @@ export class ShareStore extends PuterStore { ); } - // A share issued through an app is attributed to the user, because the - // grant is theirs. `data` records which app asked for it, so the owner - // can tell an app-issued share from one they made themselves. + // The user's grant; `data` records which app asked for it. + // No app asked, so there is nothing to keep and nothing to read for. + const prior = issuerAppUid + ? await this.getActive({ + holderUserId, + fsentryId, + issuerUserId, + }) + : null; const data = JSON.stringify( - issuerAppUid ? { issuedByApp: issuerAppUid } : {}, + prior + ? this.#keptAttribution(prior.data, issuerAppUid) + : issuerAppUid + ? { issuedByApp: issuerAppUid } + : {}, ); await this.clients.db.write( 'INSERT INTO `share` (`uid`, `issuer_user_id`, `recipient_email`, ' + @@ -745,8 +757,20 @@ export class ShareStore extends PuterStore { 'upsertActiveGroup: issuerUserId, holderGroupId, fsentryId and mode are required', ); } + // No app asked, so there is nothing to keep and nothing to read for. + const prior = issuerAppUid + ? await this.getActiveGroup({ + holderGroupId, + fsentryId, + issuerUserId, + }) + : null; const data = JSON.stringify( - issuerAppUid ? { issuedByApp: issuerAppUid } : {}, + prior + ? this.#keptAttribution(prior.data, issuerAppUid) + : issuerAppUid + ? { issuedByApp: issuerAppUid } + : {}, ); await this.clients.db.write( 'INSERT INTO `share` (`uid`, `issuer_user_id`, `recipient_email`, ' + @@ -928,13 +952,22 @@ export class ShareStore extends PuterStore { ); } - async deletePendingByIssuerSubtree(issuerUserId, fsentryId) { + /** + * @param {number} issuerUserId @param {number} fsentryId + * @param {{ exemptFsentryIds?: number[] }} [opts] + */ + async deletePendingByIssuerSubtree( + issuerUserId, + fsentryId, + { exemptFsentryIds = [] } = {}, + ) { // Read-then-delete rather than a CTE inside the DELETE, which the // dialects disagree on. The gap between the two only ever leaves an // invite standing, and the claim path re-checks authority anyway. + const exempt = new Set(exemptFsentryIds.map(Number)); const rows = await this.clients.db.read( this.#subtreeCte() + - 'SELECT `share`.`uid` FROM `share` ' + + 'SELECT `share`.`uid`, `share`.`fsentry_id` FROM `share` ' + 'JOIN `subtree` ON `share`.`fsentry_id` = `subtree`.`id` ' + // Group and link rows also have no holder user; deleting one // here would drop the index row and leave its grant standing. @@ -944,11 +977,15 @@ export class ShareStore extends PuterStore { '`share`.`issuer_user_id` = ?', [fsentryId, issuerUserId], ); - if (rows.length === 0) return 0; - const placeholders = rows.map(() => '?').join(', '); + // Not the ones on a node they manage in their own right. + const retired = rows.filter( + (row) => !exempt.has(Number(row.fsentry_id)), + ); + if (retired.length === 0) return 0; + const placeholders = retired.map(() => '?').join(', '); const result = await this.clients.db.write( `DELETE FROM \`share\` WHERE \`uid\` IN (${placeholders})`, - rows.map((row) => row.uid), + retired.map((row) => row.uid), ); return result?.affectedRows ?? result?.changes ?? 0; } @@ -1049,8 +1086,14 @@ export class ShareStore extends PuterStore { 'upsertAnyone: issuerUserId, fsentryId and mode are required', ); } + // As the other upserts: re-issuing may lose the app, never switch it. + const prior = issuerAppUid ? await this.getAnyone(fsentryId) : null; const data = JSON.stringify( - issuerAppUid ? { issuedByApp: issuerAppUid } : {}, + prior + ? this.#keptAttribution(prior.data, issuerAppUid) + : issuerAppUid + ? { issuedByApp: issuerAppUid } + : {}, ); await this.clients.db.write( 'INSERT INTO `share` (`uid`, `issuer_user_id`, `recipient_email`, ' + @@ -1123,6 +1166,27 @@ export class ShareStore extends PuterStore { return typeof count === 'number' ? count : amount; } + /** + * Who a re-issued share stays credited to: only the same app re-issuing + * keeps it, so a share may lose attribution but never gain or switch it. + */ + #keptAttribution(existingData, issuerAppUid) { + let prior = existingData ?? {}; + if (typeof prior === 'string') { + // `data` is not always JSON on sqlite; `#normalizeRow` says so too. + try { + prior = JSON.parse(prior || '{}'); + } catch { + prior = {}; + } + } + // `issuerAppUid` is the older spelling, read in two other places. + const priorApp = prior?.issuedByApp ?? prior?.issuerAppUid; + return issuerAppUid && priorApp === issuerAppUid + ? { issuedByApp: issuerAppUid } + : {}; + } + /** @param {number} userId @param {string} scope */ #dailyQuotaKey(userId, scope = 'quota') { const day = new Date().toISOString().slice(0, 10); diff --git a/src/backend/stores/share/ShareStore.test.js b/src/backend/stores/share/ShareStore.test.js index a74b17255..d11877b8b 100644 --- a/src/backend/stores/share/ShareStore.test.js +++ b/src/backend/stores/share/ShareStore.test.js @@ -3,18 +3,19 @@ * * This file is part of Puter. * - * Puter is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published - * by the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. + * Puter is free software: you can redistribute it and/or modify it under the + * terms of the GNU Affero General Public License as published by the Free + * Software Foundation, either version 3 of the License, or (at your option) any + * later version. * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. * * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . + * along with this program. If not, see + * [https://www.gnu.org/licenses/](https://www.gnu.org/licenses/). */ import { v4 as uuidv4 } from 'uuid'; @@ -268,7 +269,13 @@ describe('ShareStore', () => { const dirUuid = uuidv4(); await server.clients.db.write( 'INSERT INTO `fsentries` (`uuid`, `name`, `path`, `user_id`, `is_dir`, `modified`) VALUES (?, ?, ?, ?, 1, ?)', - [dirUuid, `d-${dirUuid.slice(0, 8)}`, `/x/${dirUuid}`, issuer.id, now], + [ + dirUuid, + `d-${dirUuid.slice(0, 8)}`, + `/x/${dirUuid}`, + issuer.id, + now, + ], ); const dirRows = await server.clients.db.read( 'SELECT `id` FROM `fsentries` WHERE `uuid` = ?', @@ -279,7 +286,14 @@ describe('ShareStore', () => { const childUuid = uuidv4(); await server.clients.db.write( 'INSERT INTO `fsentries` (`uuid`, `name`, `path`, `user_id`, `is_dir`, `modified`, `parent_id`, `parent_uid`) VALUES (?, ?, NULL, ?, 0, ?, ?, ?)', - [childUuid, `f-${childUuid.slice(0, 8)}`, issuer.id, now, dirId, dirUuid], + [ + childUuid, + `f-${childUuid.slice(0, 8)}`, + issuer.id, + now, + dirId, + dirUuid, + ], ); const childRows = await server.clients.db.read( 'SELECT `id` FROM `fsentries` WHERE `uuid` = ?', @@ -295,9 +309,9 @@ describe('ShareStore', () => { }); const rows = await store.listByFsentrySubtree(dirId); - expect( - rows.some((r) => Number(r.fsentry_id) === childId), - ).toBe(true); + expect(rows.some((r) => Number(r.fsentry_id) === childId)).toBe( + true, + ); }); it('records an active share and lists it for the holder', async () => { @@ -318,6 +332,43 @@ describe('ShareStore', () => { expect(page.items.map((r) => r.uid)).toContain(created.uid); }); + it('lets a link share lose its app, and never take another one', async () => { + const entry = await makeEntry(issuer); + const appOf = (row) => { + const data = + typeof row.data === 'string' + ? JSON.parse(row.data || '{}') + : (row.data ?? {}); + return data.issuedByApp ?? null; + }; + + await store.upsertAnyone({ + issuerUserId: issuer.id, + fsentryId: entry.id, + mode: 'read', + issuerAppUid: 'app-one', + }); + expect(appOf(await store.getAnyone(entry.id))).toBe('app-one'); + + // The same app again keeps it. + await store.upsertAnyone({ + issuerUserId: issuer.id, + fsentryId: entry.id, + mode: 'write', + issuerAppUid: 'app-one', + }); + expect(appOf(await store.getAnyone(entry.id))).toBe('app-one'); + + // A different one does not take it. + await store.upsertAnyone({ + issuerUserId: issuer.id, + fsentryId: entry.id, + mode: 'read', + issuerAppUid: 'app-two', + }); + expect(appOf(await store.getAnyone(entry.id))).toBeNull(); + }); + it('moves an existing share to a new mode instead of duplicating it', async () => { const entry = await makeEntry(issuer); const first = await store.upsertActive({