diff --git a/src/backend/services/share/ShareService.ts b/src/backend/services/share/ShareService.ts index 130862e61..3f4a8eca7 100644 --- a/src/backend/services/share/ShareService.ts +++ b/src/backend/services/share/ShareService.ts @@ -2468,9 +2468,9 @@ export class ShareService extends PuterService { * asking the user to rebuild it. * * `updateMetadata` merges rather than replaces, and refreshes the cached - * row, so the switch bites on the very next share. Deliberately does not - * gate team-delivered shares; blocking the sender, or leaving the team, - * does. + * row, so the switch bites on the very next share. Shares made to a team + * are out of scope, as they are for `blockSender`: leaving the team is what + * ends those. */ async setBlockAllSenders( actor: Actor, @@ -2486,9 +2486,13 @@ export class ShareService extends PuterService { /** * Refuse further shares from `username`. Existing shares stand: access * someone already has is theirs until it is withdrawn, and a control - * labelled "block" silently revoking it would be a surprise. Team-delivered - * items from this sender stop being listed, announced or pushed while the - * block stands; the grants are untouched, so unblocking restores the view. + * labelled "block" silently revoking it would be a surprise. + * + * Shares made to a team both accounts belong to are deliberately out of + * scope: the grant is the team's, and one colleague does not get to + * withhold the team's files from another. Leaving the team ends those. The + * notification is still suppressed, so a block always stops the + * interruption even where it does not stop the access. */ async blockSender( actor: Actor, diff --git a/src/backend/services/share/TeamShare.test.ts b/src/backend/services/share/TeamShare.test.ts index 69e63ddf2..93f700c58 100644 --- a/src/backend/services/share/TeamShare.test.ts +++ b/src/backend/services/share/TeamShare.test.ts @@ -611,9 +611,9 @@ describe('sharing with a team', () => { }); // -- the recipient block list --------------------------------------- - // A block suspends delivery only — listing, count, fan-out, telling. The - // grant and the authority graph stay whole, or a reversible block turns - // into permanent revocations downstream. + // A team share is the team's, so a per-sender block does not withhold it + // from a colleague; only the notification is suppressed. Leaving the team + // is what ends the access. const block = (blocker: FixtureUser, blocked: FixtureUser) => fx.env.server.stores.userBlock.create(blocker.userId, blocked.userId); @@ -623,36 +623,30 @@ describe('sharing with a team', () => { blocked.userId, ); - it('does not deliver a team share to a member who blocked the sender', async () => { + it('still delivers a team share to a member who blocked the sender', async () => { const [blockingSeat, otherSeat] = fx.a.seats; await block(blockingSeat, fx.a.owner); try { - const before = await shares().listSharedWithMe( - await actorFor(blockingSeat.userId), - { limit: 100, includeTotal: true }, - ); const file = await makeFile(fx.a.owner.userId); await shareWithTeam(fx.a.owner.userId, file.path, { team: fx.a.uid, }); - // Neither the listing entry nor the count moves for the blocker. - const after = await shares().listSharedWithMe( + // The block is about one person's contact, not the team's files. + const mine = await shares().listSharedWithMe( await actorFor(blockingSeat.userId), { limit: 100, includeTotal: true }, ); - expect(after.items.map((i) => i.entryUid)).not.toContain(file.uid); - expect(after.total).toBe(before.total); + expect(mine.items.map((i) => i.entryUid)).toContain(file.uid); - // The grant itself stands — nothing is revoked by a block. - const perms = + // And the grant is there to back it, not just the listing row. + expect( await fx.env.server.stores.permission.readUserGroupPerms( blockingSeat.userId, [`fs:${file.uid}:read`], - ); - expect(perms).toHaveLength(1); + ), + ).toHaveLength(1); - // The rest of the team is unaffected. const others = (await inbox(otherSeat.userId)).items; expect(others.map((i) => i.entryUid)).toContain(file.uid); } finally { @@ -660,31 +654,26 @@ describe('sharing with a team', () => { } }); - it('suspends delivery on block and restores it on unblock', async () => { + it('counts a team share for a blocking member, so page and total agree', async () => { const seat = fx.a.seats[0]; - const file = await makeFile(fx.a.owner.userId); - await shareWithTeam(fx.a.owner.userId, file.path, { team: fx.a.uid }); - expect((await inbox(seat.userId)).items.map((i) => i.entryUid)).toContain( - file.uid, - ); - await block(seat, fx.a.owner); try { - expect( - (await inbox(seat.userId)).items.map((i) => i.entryUid), - ).not.toContain(file.uid); + const file = await makeFile(fx.a.owner.userId); + await shareWithTeam(fx.a.owner.userId, file.path, { + team: fx.a.uid, + }); + const res = await shares().listSharedWithMe( + await actorFor(seat.userId), + { limit: 100, includeTotal: true }, + ); + expect(res.total).toBeGreaterThanOrEqual(res.items.length); } finally { await unblock(seat, fx.a.owner); } - - // Nothing was revoked, so lifting the block needs no re-share. - expect((await inbox(seat.userId)).items.map((i) => i.entryUid)).toContain( - file.uid, - ); }); - it('keeps a blocked member out of the live-event fan-out', async () => { - const [blockingSeat, otherSeat] = fx.a.seats; + it('keeps a blocking member in the live-event fan-out', async () => { + const [blockingSeat] = fx.a.seats; const file = await makeFile(fx.a.owner.userId); await shareWithTeam(fx.a.owner.userId, file.path, { team: fx.a.uid }); const entry = await fx.env.server.stores.fsEntry.getEntryByUuid( @@ -693,66 +682,19 @@ describe('sharing with a team', () => { await block(blockingSeat, fx.a.owner); try { - // Pushing changes to them would tell them what the block hides. + // They can open the file, so they have to be told it changed. const rows = await fx.env.server.stores.share.listGroupReachingMembers([ entry.id, ]); - const reached = rows.map((r) => Number(r.holder_user_id)); - expect(reached).not.toContain(blockingSeat.userId); - expect(reached).toContain(otherSeat.userId); + expect(rows.map((r) => Number(r.holder_user_id))).toContain( + blockingSeat.userId, + ); } finally { await unblock(blockingSeat, fx.a.owner); } }); - it('only suspends the blocked pair, not the member\'s other shares', async () => { - const seat = fx.a.seats[0]; - const peerFile = await makeFile(fx.a.seats[1].userId); - await shareWithTeam(fx.a.seats[1].userId, peerFile.path, { - team: fx.a.uid, - }); - - await block(seat, fx.a.owner); - try { - const items = (await inbox(seat.userId)).items; - expect(items.map((i) => i.entryUid)).toContain(peerFile.uid); - } finally { - await unblock(seat, fx.a.owner); - } - }); - - it('leaves a blocked member their authority, so they can still withdraw', async () => { - const seat = fx.a.seats[0]; - const file = await makeFile(fx.a.owner.userId); - await shareWithTeam( - fx.a.owner.userId, - file.path, - { team: fx.a.uid }, - 'manage', - ); - await shares().share(await actorFor(seat.userId), { - path: file.path, - recipient: { username: fx.outsider.username }, - mode: 'read', - } as never); - - await block(seat, fx.a.owner); - try { - // Authority must survive the block, or what they granted becomes - // theirs to keep but not theirs to take back. - await shares().unshare(await actorFor(seat.userId), { - path: file.path, - recipient: { username: fx.outsider.username }, - } as never); - expect( - (await inbox(fx.outsider.userId)).items.map((i) => i.entryUid), - ).not.toContain(file.uid); - } finally { - await unblock(seat, fx.a.owner); - } - }); - // -- unshare sweeps by rows, not by a member page -------------------- it('sweeps a member re-share on team unshare', async () => { diff --git a/src/backend/stores/share/ShareStore.js b/src/backend/stores/share/ShareStore.js index 5939d7d07..04f1ec188 100644 --- a/src/backend/stores/share/ShareStore.js +++ b/src/backend/stores/share/ShareStore.js @@ -20,7 +20,6 @@ import { v4 as uuidv4 } from 'uuid'; import { HttpError } from '../../core/http/HttpError.js'; import { encodeCursor, decodeCursor } from '../../util/pagination'; -import { notBlockedSql } from '../userBlock/UserBlockStore'; import { PuterStore } from '../types'; /** Default page size for the keyset listings. */ @@ -89,16 +88,15 @@ export class ShareStore extends PuterStore { const afterId = this.#afterId(cursor); const groups = [...new Set(groupIds)].filter(Boolean); - // Same keyset page: `ORDER BY id` holds whatever the holder is. The - // group arm skips issuers this holder blocked. + // Same keyset page: `ORDER BY id` holds whatever the holder is. A + // per-sender block deliberately does not apply here — a team share is + // the team's, not one colleague's to withhold from another. const holderClause = groups.length - ? `(\`holder_user_id\` = ? OR (\`holder_group_id\` IN (${groups + ? `(\`holder_user_id\` = ? OR \`holder_group_id\` IN (${groups .map(() => '?') - .join(', ')}) AND ${this.#issuerNotBlockedSql()}))` + .join(', ')}))` : '`holder_user_id` = ?'; - const holderParams = groups.length - ? [holderUserId, ...groups, holderUserId] - : [holderUserId]; + const holderParams = [holderUserId, ...groups]; // One extra row tells us whether another page exists. const rows = await this.clients.db.read( @@ -391,7 +389,6 @@ export class ShareStore extends PuterStore { async listGroupReachingMembers(fsentryIds) { if (fsentryIds.length === 0) return []; const placeholders = fsentryIds.map(() => '?').join(', '); - // A member who blocked the issuer is not pushed that issuer's shares. const rows = await this.clients.db.read( 'SELECT `share`.*, `ug`.`user_id` AS `member_user_id` FROM `share` ' + 'JOIN `jct_user_group` `ug` ON `ug`.`group_id` = `share`.`holder_group_id` ' + @@ -399,7 +396,6 @@ export class ShareStore extends PuterStore { `WHERE \`share\`.\`fsentry_id\` IN (${placeholders}) ` + 'AND `share`.`holder_group_id` IS NOT NULL ' + 'AND `g`.`deleted_at` IS NULL ' + - `AND ${notBlockedSql('`ug`.`user_id`', '`share`.`issuer_user_id`')} ` + 'ORDER BY `share`.`id`', fsentryIds, ); @@ -464,17 +460,16 @@ export class ShareStore extends PuterStore { */ async countByHolder(holderUserId, { groupIds = [] } = {}) { const groups = [...new Set(groupIds)].filter(Boolean); - // Group arm filtered as `listByHolder` is, or the total overcounts. + // Same union as `listByHolder`, blocks included, or the total and the + // page disagree. const holderClause = groups.length - ? `(\`holder_user_id\` = ? OR (\`holder_group_id\` IN (${groups + ? `(\`holder_user_id\` = ? OR \`holder_group_id\` IN (${groups .map(() => '?') - .join(', ')}) AND ${this.#issuerNotBlockedSql()}))` + .join(', ')}))` : '`holder_user_id` = ?'; const rows = await this.clients.db.read( `SELECT COUNT(*) AS \`count\` FROM \`share\` WHERE ${holderClause}`, - groups.length - ? [holderUserId, ...groups, holderUserId] - : [holderUserId], + [holderUserId, ...groups], ); return Number(rows[0]?.count ?? 0); } @@ -1074,11 +1069,6 @@ export class ShareStore extends PuterStore { ); } - /** Group rows only exist for teams, so no kind guard is needed here. */ - #issuerNotBlockedSql() { - return notBlockedSql('?', '`share`.`issuer_user_id`'); - } - /** @param {number} [limit] */ #pageSize(limit) { return Math.min( diff --git a/src/backend/stores/userBlock/UserBlockStore.ts b/src/backend/stores/userBlock/UserBlockStore.ts index 7167c682f..5a296990c 100644 --- a/src/backend/stores/userBlock/UserBlockStore.ts +++ b/src/backend/stores/userBlock/UserBlockStore.ts @@ -19,19 +19,6 @@ import { PuterStore } from '../types'; -/** - * SQL fragment: true when `blockerExpr` has no block against `blockedExpr`. The - * one spelling of the rule, so every filtered surface stays in step. Exprs are - * SQL (a column or a `?`), never user input. - */ -export const notBlockedSql = ( - blockerExpr: string, - blockedExpr: string, -): string => - 'NOT EXISTS (SELECT 1 FROM `user_block` `ub` ' + - `WHERE \`ub\`.\`blocker_user_id\` = ${blockerExpr} ` + - `AND \`ub\`.\`blocked_user_id\` = ${blockedExpr})`; - /** One row of `user_block`. `created_at` is unix seconds. */ export interface UserBlockRow { id: number; diff --git a/src/docs/src/FS/share.md b/src/docs/src/FS/share.md index a8a20739e..59f6d769f 100644 --- a/src/docs/src/FS/share.md +++ b/src/docs/src/FS/share.md @@ -37,7 +37,8 @@ Who to share with. A string containing `@` is treated as an email address, and a Where the deployment has [Teams](/Teams/), pass `{ team: uid }` to share with every member of a team the caller belongs to — including anyone added to it later. There is no string form for a team: a bare string is always read as an email or username. -A team share is never refused for one member's sake, so it does not produce `recipient_not_accepting_shares` — but a member who has blocked the sharer is not reached by it. Nothing you share with the team is listed, announced, or pushed to them while their block stands; the grant itself is untouched, so lifting the block restores their view without a re-share. The rest of the team is unaffected, and nothing tells the sharer. The blanket "block everyone" switch does not extend to teams the recipient belongs to — blocking the sender, or leaving the team, is what stops those. +A team share is never refused for one member's sake, so it does not produce `recipient_not_accepting_shares`, and **recipient blocks do not apply to it**: the grant is the team's, not one colleague's to withhold from another. A member who has blocked you still reaches anything you share with a team you both belong to — they are simply not notified about it. Leaving the team is what ends that access. + Pass `{ anyone: true }` to share with **anyone with the link** — see below. Only the object form is read as that; the word `anyone` typed as a string is a username like any other. #### `mode` (String) (optional) diff --git a/src/docs/src/Teams.md b/src/docs/src/Teams.md index 20d3c3e21..24f7531e7 100644 --- a/src/docs/src/Teams.md +++ b/src/docs/src/Teams.md @@ -6,115 +6,98 @@ platforms: [websites, apps]