From f3b05ca822b9b78318124d3e549bf32fd8a1b6d7 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 6 Oct 2026 15:55:22 -0400 Subject: [PATCH 1/8] fix: page GET /teams like every other team listing `listTeamsForUser` returned every row and the controller never read `req.query`, so `limit`, `cursor` and `includeTotal` were accepted and ignored on the one team route that did not paginate. The docs promise a capped page there, and every sibling listing already honours it. Keyset-paginated on `id` per doc/pagination.md, with the envelope the other routes return. The app-actor directory filter still runs over the page, so an app sees the same teams it saw before, just a page at a time. --- .../controllers/team/TeamController.ts | 15 ++++-- src/backend/stores/team/TeamStore.test.ts | 37 ++++++++++++-- src/backend/stores/team/TeamStore.ts | 48 ++++++++++++++++--- 3 files changed, 88 insertions(+), 12 deletions(-) diff --git a/src/backend/controllers/team/TeamController.ts b/src/backend/controllers/team/TeamController.ts index 0f176efc8..dd208e218 100644 --- a/src/backend/controllers/team/TeamController.ts +++ b/src/backend/controllers/team/TeamController.ts @@ -111,16 +111,25 @@ export class TeamController extends PuterController { async listTeams(req: Request, res: Response): Promise { const userId = this.#requireUserId(req); await this.#requireTeamsAvailable(req, userId); - const teams = await this.stores.team.listTeamsForUser(userId); + const page = await this.stores.team.listTeamsForUser(userId, { + limit: req.query.limit, + cursor: + typeof req.query.cursor === 'string' + ? req.query.cursor + : undefined, + includeTotal: req.query.includeTotal === 'true', + }); // An app only sees a team whose owner opened the directory to apps; // a team that hasn't is indistinguishable from no team at all. const visible = isAccountContext(req.actor) - ? teams - : teams.filter((t) => this.services.team.isDirectoryOpen(t)); + ? page.items + : page.items.filter((t) => this.services.team.isDirectoryOpen(t)); res.json({ items: visible.map((t) => toClientTeam(t, t.owner_user_id === userId), ), + ...(page.cursor ? { cursor: page.cursor } : {}), + ...(page.total === undefined ? {} : { total: page.total }), }); } diff --git a/src/backend/stores/team/TeamStore.test.ts b/src/backend/stores/team/TeamStore.test.ts index cf35beaab..e1bfdc04f 100644 --- a/src/backend/stores/team/TeamStore.test.ts +++ b/src/backend/stores/team/TeamStore.test.ts @@ -467,8 +467,8 @@ describe('TeamStore', () => { await store.addMember(a.uid, member.id, { orgOwned: true }); await store.addMember(b.uid, member.id, { orgOwned: true }); - const teams = await store.listTeamsForUser(member.id); - expect(teams.map((t) => t.uid).sort()).toEqual([a.uid, b.uid].sort()); + const { items } = await store.listTeamsForUser(member.id); + expect(items.map((t) => t.uid).sort()).toEqual([a.uid, b.uid].sort()); }); it('drops a soft-deleted team from the user\'s list', async () => { @@ -481,10 +481,41 @@ describe('TeamStore', () => { await store.addMember(team.uid, member.id, { orgOwned: true }); await store.softDelete(team.uid); - expect(await store.listTeamsForUser(member.id)).toEqual([]); + expect((await store.listTeamsForUser(member.id)).items).toEqual([]); await expect(store.isMember(team.uid, member.id)).resolves.toBe(false); }); + it('pages the user team list and counts on request', async () => { + const member = await makeUser(); + const made = []; + for (let i = 0; i < 3; i++) { + const team = await store.create({ + ownerUserId: owner.id, + name: `Paged ${i}`, + handle: freeHandle(), + }); + await store.addMember(team.uid, member.id, { orgOwned: true }); + made.push(team.uid); + } + + const first = await store.listTeamsForUser(member.id, { + limit: 2, + includeTotal: true, + }); + expect(first.items).toHaveLength(2); + expect(first.total).toBe(3); + expect(first.cursor).toBeTruthy(); + + const second = await store.listTeamsForUser(member.id, { + cursor: first.cursor, + }); + expect(second.items).toHaveLength(1); + expect(second.cursor).toBeUndefined(); + expect( + [...first.items, ...second.items].map((t) => t.uid).sort(), + ).toEqual([...made].sort()); + }); + it('refuses to add a member to a group that is not a team', async () => { const [seeded] = (await server.clients.db.read( 'SELECT `uid` FROM `group` WHERE `kind` IS NULL LIMIT 1', diff --git a/src/backend/stores/team/TeamStore.ts b/src/backend/stores/team/TeamStore.ts index a85b5ae1d..84878ded7 100644 --- a/src/backend/stores/team/TeamStore.ts +++ b/src/backend/stores/team/TeamStore.ts @@ -610,14 +610,50 @@ export class TeamStore extends PuterStore { } /** Teams this user belongs to, oldest first. */ - async listTeamsForUser(userId: number): Promise { - const rows = await this.clients.db.read( + /** A user's teams, keyset-paginated on `id` per doc/pagination.md. */ + async listTeamsForUser( + userId: number, + opts: { limit?: unknown; cursor?: string; includeTotal?: boolean } = {}, + ): Promise> { + const limit = + normalizeLimit(opts.limit, { cap: MEMBER_PAGE_CAP }) ?? + MEMBER_PAGE_SIZE; + const page = decodeCursor(opts.cursor, 'team cursor'); + const after = typeof page?.id === 'number' ? page.id : null; + + const rows = (await this.clients.db.read( 'SELECT g.* FROM `group` g ' + 'JOIN `jct_user_group` ug ON ug.`group_id` = g.`id` ' + - `WHERE ug.\`user_id\` = ? AND g.${this.#live()} ORDER BY g.\`id\``, - [userId, TEAM_KIND], - ); - return rows as unknown as TeamRow[]; + `WHERE ug.\`user_id\` = ? AND g.${this.#live()}` + + (after === null ? '' : ' AND g.`id` > ?') + + ' ORDER BY g.`id` LIMIT ?', + after === null + ? [userId, TEAM_KIND, limit + 1] + : [userId, TEAM_KIND, after, limit + 1], + )) as unknown as TeamRow[]; + + const items = rows.slice(0, limit); + const cursor = + rows.length > limit + ? encodeCursor({ id: items[items.length - 1].id }) + : undefined; + + let total: number | undefined; + if (opts.includeTotal) { + const totals = (await this.clients.db.read( + 'SELECT COUNT(*) AS n FROM `group` g ' + + 'JOIN `jct_user_group` ug ON ug.`group_id` = g.`id` ' + + `WHERE ug.\`user_id\` = ? AND g.${this.#live()}`, + [userId, TEAM_KIND], + )) as unknown as Array<{ n: number | string }>; + total = Number(totals[0]?.n ?? 0); + } + + return { + items, + ...(cursor ? { cursor } : {}), + ...(total === undefined ? {} : { total }), + }; } /** `orgOwned` decides who pays: 1 team-created, 0 the team owner. */ From 665314535e4455eb6b7ec0eb9cfba38e5d916067 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 6 Oct 2026 15:55:40 -0400 Subject: [PATCH 2/8] docs: say what TeamStore actually caches The cache docblock claimed `isMember` and `getByUid` were left uncached "for that reason". Both go through it -- `getByUid` directly, `isMember` through `getMembership` -- so the comment told a reader the opposite of the code, on the two reads that decide authorization. What makes it safe is the busting, not an exemption: every mutation here drops the keys it touches, and the TTL is the backstop for the one path that does not come through this store, the ON DELETE CASCADE on the membership row. No stale window is constructible today; the comment now says why. Also drops a one-line docblock stranded above it, describing a method that is no longer in the file. --- src/backend/stores/team/TeamStore.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/backend/stores/team/TeamStore.ts b/src/backend/stores/team/TeamStore.ts index 84878ded7..289fa9c9f 100644 --- a/src/backend/stores/team/TeamStore.ts +++ b/src/backend/stores/team/TeamStore.ts @@ -522,11 +522,8 @@ export class TeamStore extends PuterStore { return rows[0] ?? null; } - /** Members to announce a team share to; bounded, or the send is too. */ /** - * Short-lived, and deliberately only over reads that are not authorization. - * A stale entry here costs a notification, never access -- `isMember` and - * `getByUid` are left uncached for that reason. + * Short-lived, and safe for `getByUid`/`getMembership`: mutations bust it. * * `jct_user_group.user_id` is ON DELETE CASCADE, so deleting an account * changes membership without passing through this store. The TTL is the From d896250ae24e68988efeb0eaf01f108cb955fff0 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 6 Oct 2026 15:56:10 -0400 Subject: [PATCH 3/8] fix: make team deletion resumable, and stop it serializing its mail `deleteTeam` tore seats down one at a time, each one awaiting a mail send, so a full 40-seat team was 40 round trips end to end inside a single request. The per-seat work is independent, so it now runs under the same bounded-concurrency helper the rest of the backend uses for fan-out sends. The loop also had no equivalent of the guard `disableMember` carries. That one checks `suspended` before emitting, with the reason written down: a second `team.account.disabled` opens a second byte charge that only one `enabled` will ever close. Deleting a team emitted unconditionally, so any partial failure followed by a retry double-charged every seat the first attempt had already disabled. The seat teardown moves into one method both paths can state the same way, and a retry now resumes instead of re-charging. Failure semantics are unchanged: a seat that throws still aborts the delete and leaves the team live, which is what makes the retry meaningful. Touching this file reformats a few untouched spans -- it carried ten pre-existing `prettier/prettier` errors that the repo's own pre-commit `eslint --fix` resolves on any commit that stages it. --- src/backend/services/team/TeamEvents.test.ts | 18 ++++ src/backend/services/team/TeamService.ts | 103 ++++++++++++------- 2 files changed, 83 insertions(+), 38 deletions(-) diff --git a/src/backend/services/team/TeamEvents.test.ts b/src/backend/services/team/TeamEvents.test.ts index 7358c8581..d38e1333f 100644 --- a/src/backend/services/team/TeamEvents.test.ts +++ b/src/backend/services/team/TeamEvents.test.ts @@ -191,6 +191,24 @@ describe('team billing events', () => { }); }); + it('does not re-charge a seat that was already disabled before the delete', async () => { + const team = await makeTeam(); + const a = await provision(team); + const b = await provision(team); + await service.disableMember(team.uid, owner.id, a.userId); + seen.length = 0; + + await service.deleteTeam(team.uid, owner.id); + + // Only one `enabled` will ever close a charge, so only one may open it. + const disabled = of('team.account.disabled'); + expect(disabled).toHaveLength(1); + expect(disabled[0].data.user_id).toBe(b.userId); + + // Still both seats: the team is losing two accounts either way. + expect(of('team.deleted')[0].data).toMatchObject({ account_count: 2 }); + }); + it('emits account-deleted, which a post-delete listener could not', async () => { const team = await makeTeam(); const seat = await provision(team); diff --git a/src/backend/services/team/TeamService.ts b/src/backend/services/team/TeamService.ts index 9c7160fc0..1eb156759 100644 --- a/src/backend/services/team/TeamService.ts +++ b/src/backend/services/team/TeamService.ts @@ -28,11 +28,15 @@ import { isReservedUsername } from '../../util/reservedUsernames.js'; import type { EmailTemplateName } from '../../clients/email/templates.js'; import { subscriptionSatisfies } from '../metering/enforcement.js'; import { ORG_SEAT_FREE_SUBSCRIPTION } from '../metering/consts.js'; +import { runWithConcurrencyLimit } from '../../util/concurrency.js'; // A free team is small on purpose; paying widens it. Both overridable in config. const FREE_SEAT_CAP = 4; const PAID_SEAT_CAP = 40; +// Seat teardown is independent per member, so a delete need not be serial. +const TEARDOWN_CONCURRENCY = 8; + import type { EventMap, TeamBillingContext, @@ -637,32 +641,19 @@ export class TeamService extends PuterService { // Otherwise they keep working, unreachable through a deleted team. let page = await this.stores.team.listMembers(teamUid, { limit: 200 }); for (;;) { - for (const member of page.items) { - if (Number(member.org_owned) !== 1) continue; - await this.stores.team.appendAudit({ - teamId: team.id, - userId: member.user_id, - actorUserId, - action: 'disable', - reason: 'team_deleted', - }); - const held = await this.#heldBytes(member.user_id); - await this.#suspend(member.user_id); - disabled++; - - // Per seat, not one bulk event: the byte charge is per account. - this.#emitBilling('team.account.disabled', { - ...billing, - user_id: member.user_id, - user_uuid: member.uuid, - username: member.username, - held_bytes: held, - }); - - // The team notice covers the disabling, so a member is - // told once rather than twice about the same event. - await this.#notifyMember(member.user_id, 'team_closed', team); - } + // Bounded: a seat costs a mail send, and a full team is 40 of them. + const outcomes = await runWithConcurrencyLimit( + page.items.filter((m) => Number(m.org_owned) === 1), + TEARDOWN_CONCURRENCY, + (member) => + this.#disableSeatForDelete( + member, + team, + actorUserId, + billing, + ), + ); + disabled += outcomes.filter(Boolean).length; if (!page.cursor) break; page = await this.stores.team.listMembers(teamUid, { limit: 200, @@ -684,6 +675,44 @@ export class TeamService extends PuterService { }); } + /** As `disableMember`; a suspended seat is skipped so a retry resumes. */ + async #disableSeatForDelete( + member: TeamMemberRow, + team: TeamRow, + actorUserId: number, + billing: TeamBillingContext, + ): Promise { + const current = await this.stores.user.getByProperty( + 'id', + member.user_id, + { force: true }, + ); + if (current?.suspended) return true; + + await this.stores.team.appendAudit({ + teamId: team.id, + userId: member.user_id, + actorUserId, + action: 'disable', + reason: 'team_deleted', + }); + const held = await this.#heldBytes(member.user_id); + await this.#suspend(member.user_id); + + // Per seat, not one bulk event: the byte charge is per account. + this.#emitBilling('team.account.disabled', { + ...billing, + user_id: member.user_id, + user_uuid: member.uuid, + username: member.username, + held_bytes: held, + }); + + // The team notice covers the disabling, so a member is told once. + await this.#notifyMember(member.user_id, 'team_closed', team); + return true; + } + /** Team owner only. Readable after deletion -- that is the point of it. */ async listAudit( teamUid: string, @@ -820,18 +849,16 @@ export class TeamService extends PuterService { id === null ? null : (users.get(id)?.username ?? null); return { - items: page.items.map( - (row): MemberActivityEntry => ({ - action: row.action, - reason: row.reason, - created_at: epochSeconds(row.created_at), - username: name(row.user_id_keep), - actor_username: name(row.actor_user_id), - // Only a sign-in carries these; the shape stays uniform. - ip: null, - user_agent: null, - }), - ), + items: page.items.map((row): MemberActivityEntry => ({ + action: row.action, + reason: row.reason, + created_at: epochSeconds(row.created_at), + username: name(row.user_id_keep), + actor_username: name(row.actor_user_id), + // Only a sign-in carries these; the shape stays uniform. + ip: null, + user_agent: null, + })), ...(page.cursor ? { cursor: page.cursor } : {}), }; } From 6826071156c544b58118d12f05262ecee7c7e45e Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 6 Oct 2026 15:56:33 -0400 Subject: [PATCH 4/8] fix: stop mailing a plaintext team credential to an unconfirmed address Provisioning a seat lets the owner supply any `email` that passes a format check. Puter then mailed a working temporary password to it. Nothing confirms the owner controls that address -- provisioned accounts are created with `requires_email_confirmation: false`, because the team is the trust anchor for the account, not for the mailbox. `resendActivation` reissues the credential and mails it again, up to the 20/day `team:reset-password` budget, so the same address can be hit repeatedly. The credential now stays out of the mail on both paths, which is what `resetMemberPassword` in the same file already does. Nothing is lost: all three routes return `temporary_password` to the owner, and the SDK has always documented that as the delivery -- "The returned password is shown once and is not retrievable afterwards -- deliver it out of band." The mail still goes out, naming the account and saying the team will supply the password. The two tests that pinned the old shape are inverted rather than deleted; one of them carried the rationale "without it this is the only copy", which the return value contradicts. --- src/backend/clients/email/templates.ts | 8 ++++---- src/backend/services/team/TeamService.test.ts | 11 ++++++----- src/backend/services/team/TeamService.ts | 10 +++------- 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/backend/clients/email/templates.ts b/src/backend/clients/email/templates.ts index 15c4c04c6..5ed2264e9 100644 --- a/src/backend/clients/email/templates.ts +++ b/src/backend/clients/email/templates.ts @@ -352,10 +352,10 @@ support@puter.com immediately. html: `

Hi there,

{{team_name}} has created a Puter account for you.

-

Username: {{username}}
-Temporary password: {{temporary_password}}

-

You will be asked to choose your own password the first time you sign in. -This temporary one stops working then, and it expires on its own if unused.

+

Username: {{username}}

+

{{team_name}} will give you a temporary password. You will be asked to choose +your own the first time you sign in. The temporary one stops working then, and it +expires on its own if unused.

What this means:

  • This account belongs to {{team_name}}. They pay for it and can close it.
    • diff --git a/src/backend/services/team/TeamService.test.ts b/src/backend/services/team/TeamService.test.ts index f8716cf5a..a8d084e3d 100644 --- a/src/backend/services/team/TeamService.test.ts +++ b/src/backend/services/team/TeamService.test.ts @@ -1259,7 +1259,7 @@ describe('TeamService', () => { return { sent, restore: () => (client.sendRaw = original) }; }; - it('emails the credential when an address is given', async () => { + it('notifies the member without putting the credential in the mail', async () => { const { team } = await makeTeam(); const username = `mail_${Math.random().toString(36).slice(2, 9)}`; @@ -1277,8 +1277,8 @@ describe('TeamService', () => { expect(mail.sent).toHaveLength(1); expect(mail.sent[0].to).toBe(`${username}@test.local`); expect(mail.sent[0].html).toContain(username); - // The point of the address: without it this is the only copy. - expect(mail.sent[0].html).toContain(created.temporaryPassword); + // Nobody confirmed this address; the owner has the credential already. + expect(mail.sent[0].html).not.toContain(created.temporaryPassword); }); it('sends nothing when no address is given', async () => { @@ -1294,7 +1294,7 @@ describe('TeamService', () => { expect(mail.sent).toHaveLength(0); }); - it('emails the fresh credential on re-issue, not the old one', async () => { + it('keeps the re-issued credential out of the mail too', async () => { const { team } = await makeTeam(); const username = `re_${Math.random().toString(36).slice(2, 9)}`; const first = await service.provisionAccount(team.uid, owner.id, { @@ -1315,7 +1315,8 @@ describe('TeamService', () => { } expect(mail.sent).toHaveLength(1); - expect(mail.sent[0].html).toContain(again.temporaryPassword); + // The re-sendable path, so the one that made Puter a usable mailer. + expect(mail.sent[0].html).not.toContain(again.temporaryPassword); expect(mail.sent[0].html).not.toContain(first.temporaryPassword); }); diff --git a/src/backend/services/team/TeamService.ts b/src/backend/services/team/TeamService.ts index 1eb156759..a38e14646 100644 --- a/src/backend/services/team/TeamService.ts +++ b/src/backend/services/team/TeamService.ts @@ -1006,11 +1006,9 @@ export class TeamService extends PuterService { action: 'provision', }); - // Returned once; forced change on first use is what bounds it. + // Not mailed: nobody confirmed the address the owner typed in. const temporaryPassword = await this.#issueTemporaryPassword(user.id); - await this.#notifyUser(user, 'team_account_created', team, { - temporary_password: temporaryPassword, - }); + await this.#notifyUser(user, 'team_account_created', team); // Last: the seat is only chargeable once it exists and can be used. this.#emitBilling('team.account.created', { @@ -1053,9 +1051,7 @@ export class TeamService extends PuterService { }); const temporaryPassword = await this.#issueTemporaryPassword(targetUserId); - await this.#notifyUser(user, 'team_account_created', team, { - temporary_password: temporaryPassword, - }); + await this.#notifyUser(user, 'team_account_created', team); return { temporaryPassword }; } From 9d8e37dc6d0f00e6110663b5e0a11d2b415e2df5 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 6 Oct 2026 16:00:32 -0400 Subject: [PATCH 5/8] fix: stop an unconfirmed claim on an address from blocking invites to it `#resolveRecipient` already refuses to treat an unconfirmed address as an identity, and says why: "resolving it would hand the share to whoever registered the address first." `#invite` then looked the same address up again and honoured whatever that row said about blocking -- `blocksAllShares`, or a block on this particular sender. So registering an address you do not own, and never confirming it, was enough to refuse every invite anyone sends to that mailbox. Nothing about that row is evidence the mailbox is yours, which is the premise resolution already works from. A team seat reaches this cheaply: those accounts are provisioned with `requires_email_confirmation: false` and an address the owner typed. The block is now honoured only for a row that confirmed the address. The alias reasoning behind the lookup is unchanged -- a confirmed holder still cannot be reached through a variant they did not type. Not addressed here: the same unconfirmed seat still holds the address against the real owner's signup, because `idx_user_owned_email` keys on having a password rather than on confirmation. Narrowing that is a schema change and wants its own ticket. --- .../services/share/ShareService.test.ts | 32 +++++++++++++++++++ src/backend/services/share/ShareService.ts | 4 ++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/backend/services/share/ShareService.test.ts b/src/backend/services/share/ShareService.test.ts index fe9adce7e..d1d73ec73 100644 --- a/src/backend/services/share/ShareService.test.ts +++ b/src/backend/services/share/ShareService.test.ts @@ -293,6 +293,38 @@ describe('ShareService', () => { expect(await canRead(squatter.actor, file.path)).toBe(true); }); + it('lets an unconfirmed claim on an address refuse nothing', async () => { + const owner = await makeUser(); + const squatter = await makeUser(); + await server.stores.user.update(squatter.user.id, { + email_confirmed: false, + }); + await server.services.share.setBlockAllSenders(squatter.actor, true); + const file = await makeFile(owner.user); + + // Otherwise one unconfirmed row blocks every invite to that mailbox. + const result = await share(owner.actor, { + uid: file.uuid, + recipient: { email: squatter.email }, + mode: 'read', + }); + expect(result.pending).toBe(true); + + // Confirming it makes the claim an identity, and the block real. + await server.stores.user.update(squatter.user.id, { + email_confirmed: true, + }); + await expect( + share(owner.actor, { + uid: (await makeFile(owner.user)).uuid, + recipient: { email: squatter.email }, + mode: 'read', + }), + ).rejects.toMatchObject({ + legacyCode: 'recipient_not_accepting_shares', + }); + }); + // `+` is only an alias separator where the domain says so. it('does not hand a plus-addressed share to the base account', async () => { const owner = await makeUser(); diff --git a/src/backend/services/share/ShareService.ts b/src/backend/services/share/ShareService.ts index feed605a3..a949a307b 100644 --- a/src/backend/services/share/ShareService.ts +++ b/src/backend/services/share/ShareService.ts @@ -3091,7 +3091,9 @@ export class ShareService extends PuterService { const mayReach = (await this.stores.user.findEmailOwner(email)) ?? (await this.stores.user.getByCleanEmail(abuseKey(email))); - if (mayReach) await this.#assertNotBlocked(issuerId, mayReach); + // Confirmed only: a claim must not decide who may write to an address. + if (mayReach?.email_confirmed) + await this.#assertNotBlocked(issuerId, mayReach); // Stored canonicalized, because claiming matches on it: the confirmed // address arrives in whatever form the signup normalized to, and an From 36a22b05a259dfc34a3cc098c1f0f6cff82401df Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 6 Oct 2026 16:01:42 -0400 Subject: [PATCH 6/8] docs: stop claiming the directory discloses what /members does Both docblocks on the directory route said it carries nothing a colleague cannot already read through `/members`. It carries `uuid`, and `/members` withholds `uuid` from everyone but the owner, with its own comment pointing apps at the directory to get one. The disclosure itself is the opt-in working as designed: `directory_enabled` is the owner's consent to expose a roster, every member reaches the route the same way whether they call it from a session or through an app, and the SDK and the teams recipes already document the directory as where a stable id comes from. So the comments are what was wrong, not the route. Removing `uuid` from the directory would be the other way to make them agree, but it breaks `listDirectory()` for every caller and contradicts the published recipes; that wants a decision, not a cleanup commit. --- src/backend/controllers/team/TeamController.ts | 6 +----- src/backend/services/team/TeamService.ts | 5 +---- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/backend/controllers/team/TeamController.ts b/src/backend/controllers/team/TeamController.ts index dd208e218..2d161fa6c 100644 --- a/src/backend/controllers/team/TeamController.ts +++ b/src/backend/controllers/team/TeamController.ts @@ -245,11 +245,7 @@ export class TeamController extends PuterController { }); } - /** - * Admits an app actor, like `listTeams` and `listMembers`, once the team - * has opted in. Discloses nothing a colleague cannot already read through - * `/members`. - */ + /** App actors too, once opted in; carries the `uuid` `/members` hides. */ @Get('/:uid/directory', { subdomain: 'api', requireVerified: true, diff --git a/src/backend/services/team/TeamService.ts b/src/backend/services/team/TeamService.ts index a38e14646..af200e260 100644 --- a/src/backend/services/team/TeamService.ts +++ b/src/backend/services/team/TeamService.ts @@ -540,10 +540,7 @@ export class TeamService extends PuterService { } } - /** - * The member list as an app may read it, once the team has opted in. The - * page carries only what a colleague already sees through `/members`. - */ + /** The roster an app may read once opted in; that opt-in grants `uuid`. */ async listDirectory( teamUid: string, actorUserId: number, From 25f132b28d1a771bc4196556abb9c3bc1f9b0852 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 6 Oct 2026 16:22:48 -0400 Subject: [PATCH 7/8] fix: address code review on the teams cleanup Six findings, all real. `total` described rows the page withheld. The count ran over every live team the user belongs to, while the app-actor filter ran afterwards on the items only, so `GET /teams?includeTotal=true` from an app returned `items: []` with a non-zero `total` -- the exact thing the filter exists to prevent, and newly reachable because `includeTotal` was inert before this branch. The directory-open restriction moves into the query, so items, cursor and total all describe the same set. The GUI told owners the opposite of what the server now does: "we email the username and temporary password to it." An owner who believed it would dismiss the one copy of the password and strand the seat. Rewritten. `runWithConcurrencyLimit` rejects on the first error but cannot cancel the workers beside it, so a failed seat let up to seven others finish suspending and emailing "your team has been closed" for a team that was still live. The page now settles before the rejection is rethrown, which leaves a state the retry can resume from instead of an arbitrary one. My earlier claim that failure semantics were unchanged was wrong. `#disableSeatForDelete` returned `true` on both paths, so the boolean read as if it distinguished them. It returns nothing; the counter uses the settled results. `listTeamsForUser` had picked up a second stacked docblock -- the same defect this branch removes from `#cached` one file over. `reissueCredential` sent the provisioning mail, which after the credential change was a byte-identical duplicate saying nothing about the password it had just invalidated, while the GUI promises the member hears about it. It has its own template now. Regression tests for the count leak and the reissue notice; both confirmed to fail without their fix. --- src/backend/clients/email/templates.ts | 15 +++++++++++++++ .../team/TeamController.http.test.ts | 15 +++++++++++++++ src/backend/controllers/team/TeamController.ts | 9 +++------ src/backend/services/team/TeamService.test.ts | 3 +++ src/backend/services/team/TeamService.ts | 17 ++++++++++------- src/backend/stores/team/TeamStore.ts | 15 +++++++++++---- src/gui/src/i18n/translations/en.js | 2 +- 7 files changed, 58 insertions(+), 18 deletions(-) diff --git a/src/backend/clients/email/templates.ts b/src/backend/clients/email/templates.ts index 5ed2264e9..31aab15a2 100644 --- a/src/backend/clients/email/templates.ts +++ b/src/backend/clients/email/templates.ts @@ -364,6 +364,21 @@ expires on its own if unused.

    as you. Choose your own password promptly.

Sincerely,

+

Puter

+ `, + }, + team_account_reissued: { + subject: 'A new temporary password for your {{team_name}} account', + html: ` +

Hi there,

+

{{team_name}} has issued a new temporary password for your Puter account +{{username}}. Any temporary password you were given before this one has +stopped working.

+

{{team_name}} will give you the new one. You will be asked to choose your own +password the first time you sign in; the temporary one stops working then, and +it expires on its own if unused.

+

If you were not expecting this, ask {{team_name}} -- only they can issue it.

+

Sincerely,

Puter

`, }, diff --git a/src/backend/controllers/team/TeamController.http.test.ts b/src/backend/controllers/team/TeamController.http.test.ts index fb68d9ce9..6a06f8cb7 100644 --- a/src/backend/controllers/team/TeamController.http.test.ts +++ b/src/backend/controllers/team/TeamController.http.test.ts @@ -303,6 +303,21 @@ describe('team endpoints over HTTP', () => { expect(afterBody.items.some((t) => t.uid === team.uid)).toBe(true); }); + it('does not count a team it hides from an app token', async () => { + const { team } = await makeServiceTeam(); + const appToken = await makeAppToken(env.users.user); + + const res = await call('GET', '/teams?includeTotal=true', appToken); + expect(res.status).toBe(200); + const body = (await res.json()) as { + items: { uid: string }[]; + total: number; + }; + expect(body.items.some((t) => t.uid === team.uid)).toBe(false); + // A count over rows the page withholds reports the hidden team. + expect(body.total).toBe(body.items.length); + }); + it('still lists a team to its owner session with the directory off', async () => { const { team } = await makeServiceTeam(); const res = await call('GET', '/teams', env.users.user.token); diff --git a/src/backend/controllers/team/TeamController.ts b/src/backend/controllers/team/TeamController.ts index 2d161fa6c..ce1d586f2 100644 --- a/src/backend/controllers/team/TeamController.ts +++ b/src/backend/controllers/team/TeamController.ts @@ -111,6 +111,7 @@ export class TeamController extends PuterController { async listTeams(req: Request, res: Response): Promise { const userId = this.#requireUserId(req); await this.#requireTeamsAvailable(req, userId); + // In SQL, or cursor and total describe teams the app cannot see. const page = await this.stores.team.listTeamsForUser(userId, { limit: req.query.limit, cursor: @@ -118,14 +119,10 @@ export class TeamController extends PuterController { ? req.query.cursor : undefined, includeTotal: req.query.includeTotal === 'true', + openOnly: !isAccountContext(req.actor), }); - // An app only sees a team whose owner opened the directory to apps; - // a team that hasn't is indistinguishable from no team at all. - const visible = isAccountContext(req.actor) - ? page.items - : page.items.filter((t) => this.services.team.isDirectoryOpen(t)); res.json({ - items: visible.map((t) => + items: page.items.map((t) => toClientTeam(t, t.owner_user_id === userId), ), ...(page.cursor ? { cursor: page.cursor } : {}), diff --git a/src/backend/services/team/TeamService.test.ts b/src/backend/services/team/TeamService.test.ts index a8d084e3d..63654d7ac 100644 --- a/src/backend/services/team/TeamService.test.ts +++ b/src/backend/services/team/TeamService.test.ts @@ -1318,6 +1318,9 @@ describe('TeamService', () => { // The re-sendable path, so the one that made Puter a usable mailer. expect(mail.sent[0].html).not.toContain(again.temporaryPassword); expect(mail.sent[0].html).not.toContain(first.temporaryPassword); + // Its own notice, not a second `created` mail saying nothing new. + expect(mail.sent[0].subject).toMatch(/new temporary password/iu); + expect(mail.sent[0].html).toMatch(/stopped working/iu); }); it('tells a member their account was disabled', async () => { diff --git a/src/backend/services/team/TeamService.ts b/src/backend/services/team/TeamService.ts index af200e260..d32e8e105 100644 --- a/src/backend/services/team/TeamService.ts +++ b/src/backend/services/team/TeamService.ts @@ -28,7 +28,7 @@ import { isReservedUsername } from '../../util/reservedUsernames.js'; import type { EmailTemplateName } from '../../clients/email/templates.js'; import { subscriptionSatisfies } from '../metering/enforcement.js'; import { ORG_SEAT_FREE_SUBSCRIPTION } from '../metering/consts.js'; -import { runWithConcurrencyLimit } from '../../util/concurrency.js'; +import { runWithConcurrencyLimitSettled } from '../../util/concurrency.js'; // A free team is small on purpose; paying widens it. Both overridable in config. const FREE_SEAT_CAP = 4; @@ -639,7 +639,7 @@ export class TeamService extends PuterService { let page = await this.stores.team.listMembers(teamUid, { limit: 200 }); for (;;) { // Bounded: a seat costs a mail send, and a full team is 40 of them. - const outcomes = await runWithConcurrencyLimit( + const outcomes = await runWithConcurrencyLimitSettled( page.items.filter((m) => Number(m.org_owned) === 1), TEARDOWN_CONCURRENCY, (member) => @@ -650,7 +650,10 @@ export class TeamService extends PuterService { billing, ), ); - disabled += outcomes.filter(Boolean).length; + disabled += outcomes.filter((o) => o.status === 'fulfilled').length; + // Settled: a rejection cannot cancel its peers, so finish the page. + const failed = outcomes.find((o) => o.status === 'rejected'); + if (failed) throw failed.reason; if (!page.cursor) break; page = await this.stores.team.listMembers(teamUid, { limit: 200, @@ -678,13 +681,13 @@ export class TeamService extends PuterService { team: TeamRow, actorUserId: number, billing: TeamBillingContext, - ): Promise { + ): Promise { const current = await this.stores.user.getByProperty( 'id', member.user_id, { force: true }, ); - if (current?.suspended) return true; + if (current?.suspended) return; await this.stores.team.appendAudit({ teamId: team.id, @@ -707,7 +710,6 @@ export class TeamService extends PuterService { // The team notice covers the disabling, so a member is told once. await this.#notifyMember(member.user_id, 'team_closed', team); - return true; } /** Team owner only. Readable after deletion -- that is the point of it. */ @@ -1048,7 +1050,8 @@ export class TeamService extends PuterService { }); const temporaryPassword = await this.#issueTemporaryPassword(targetUserId); - await this.#notifyUser(user, 'team_account_created', team); + // Its own notice; the created one omits the password just invalidated. + await this.#notifyUser(user, 'team_account_reissued', team); return { temporaryPassword }; } diff --git a/src/backend/stores/team/TeamStore.ts b/src/backend/stores/team/TeamStore.ts index 289fa9c9f..4e08f4c67 100644 --- a/src/backend/stores/team/TeamStore.ts +++ b/src/backend/stores/team/TeamStore.ts @@ -606,22 +606,28 @@ export class TeamStore extends PuterStore { return rows.map((r) => Number(r.user_id)); } - /** Teams this user belongs to, oldest first. */ - /** A user's teams, keyset-paginated on `id` per doc/pagination.md. */ + /** A user's teams, oldest first; `openOnly` is the page an app may see. */ async listTeamsForUser( userId: number, - opts: { limit?: unknown; cursor?: string; includeTotal?: boolean } = {}, + opts: { + limit?: unknown; + cursor?: string; + includeTotal?: boolean; + openOnly?: boolean; + } = {}, ): Promise> { const limit = normalizeLimit(opts.limit, { cap: MEMBER_PAGE_CAP }) ?? MEMBER_PAGE_SIZE; const page = decodeCursor(opts.cursor, 'team cursor'); const after = typeof page?.id === 'number' ? page.id : null; + const open = opts.openOnly ? ' AND g.`directory_enabled` = 1' : ''; const rows = (await this.clients.db.read( 'SELECT g.* FROM `group` g ' + 'JOIN `jct_user_group` ug ON ug.`group_id` = g.`id` ' + `WHERE ug.\`user_id\` = ? AND g.${this.#live()}` + + open + (after === null ? '' : ' AND g.`id` > ?') + ' ORDER BY g.`id` LIMIT ?', after === null @@ -640,7 +646,8 @@ export class TeamStore extends PuterStore { const totals = (await this.clients.db.read( 'SELECT COUNT(*) AS n FROM `group` g ' + 'JOIN `jct_user_group` ug ON ug.`group_id` = g.`id` ' + - `WHERE ug.\`user_id\` = ? AND g.${this.#live()}`, + `WHERE ug.\`user_id\` = ? AND g.${this.#live()}` + + open, [userId, TEAM_KIND], )) as unknown as Array<{ n: number | string }>; total = Number(totals[0]?.n ?? 0); diff --git a/src/gui/src/i18n/translations/en.js b/src/gui/src/i18n/translations/en.js index 83f040206..8e811e956 100644 --- a/src/gui/src/i18n/translations/en.js +++ b/src/gui/src/i18n/translations/en.js @@ -617,7 +617,7 @@ const en = { teams_account_of: 'This account belongs to %%', teams_add_account: 'Add an account', teams_email_optional: 'Email (optional)', - teams_add_account_email_hint: 'If you add an address, we email the username and temporary password to it. Otherwise the password below is the only copy.', + teams_add_account_email_hint: 'If you add an address, we email them their username — never the password. The password below is the only copy, so pass it on yourself.', teams_add_account_hint: 'Puter creates the account and gives you a one-time password to pass on. The username has to be free across all of Puter.', teams_plan_change_for: 'Which plan for {{username}}?', From bd5aa901aba9a420c7119f5cfcfe9a70873feed8 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 6 Oct 2026 18:55:53 -0400 Subject: [PATCH 8/8] perf: batch the per-page reads in a team delete Measured first, against a real server, counting DB round trips: a 40-seat delete was 284 queries, 7.1 per seat, scaling linearly. Four of those seven answered for the whole page and had no reason to be asked per account. 120x SELECT * FROM `user` WHERE `id` = ? 41x INSERT INTO `audit_team_membership` 40x SELECT COALESCE(SUM(size), 0) FROM fsentries 40x UPDATE `user` SET `suspended` = ? 40x SELECT `uuid` FROM `sessions` WHERE `user_id` = ? The page now reads who is already suspended, what each holds, and the rows the notices need in one query each, and writes the audit as one statement. What is genuinely per account -- the suspension, its billing event, the mail -- stays per account under the same concurrency bound. Same 40-seat delete is now 170 queries, 4.3 per seat. Three new store methods rather than raw SQL at the call site: `getSuspendedByIds` reads the primary, deliberately not `getByIds`, which is cache-first. A stale `suspended` is exactly what re-charges a seat on a retry, which is the thing this path was just fixed to stop doing. `getHeldBytesForUsers` is `getHeldBytes` with a GROUP BY. `appendAuditMany` is `appendAudit` as one multi-row insert, still written before any suspension, so a failed append cannot leave an unlogged one. What is left is concentrated in `#suspend`: the UPDATE, the forced re-read the user store does afterwards to refresh its cache, and the session drop. Those are shared store paths and batching them means owning their cache invalidation, so they are not in scope here. --- src/backend/services/team/TeamService.ts | 105 ++++++++++++++--------- src/backend/stores/fs/FSEntryStore.ts | 32 +++++++ src/backend/stores/team/TeamStore.ts | 32 +++++++ src/backend/stores/user/UserStore.ts | 34 ++++++++ 4 files changed, 161 insertions(+), 42 deletions(-) diff --git a/src/backend/services/team/TeamService.ts b/src/backend/services/team/TeamService.ts index d32e8e105..818fc42f9 100644 --- a/src/backend/services/team/TeamService.ts +++ b/src/backend/services/team/TeamService.ts @@ -638,22 +638,12 @@ export class TeamService extends PuterService { // Otherwise they keep working, unreachable through a deleted team. let page = await this.stores.team.listMembers(teamUid, { limit: 200 }); for (;;) { - // Bounded: a seat costs a mail send, and a full team is 40 of them. - const outcomes = await runWithConcurrencyLimitSettled( + disabled += await this.#disableSeatsForDelete( page.items.filter((m) => Number(m.org_owned) === 1), - TEARDOWN_CONCURRENCY, - (member) => - this.#disableSeatForDelete( - member, - team, - actorUserId, - billing, - ), + team, + actorUserId, + billing, ); - disabled += outcomes.filter((o) => o.status === 'fulfilled').length; - // Settled: a rejection cannot cancel its peers, so finish the page. - const failed = outcomes.find((o) => o.status === 'rejected'); - if (failed) throw failed.reason; if (!page.cursor) break; page = await this.stores.team.listMembers(teamUid, { limit: 200, @@ -675,41 +665,72 @@ export class TeamService extends PuterService { }); } - /** As `disableMember`; a suspended seat is skipped so a retry resumes. */ - async #disableSeatForDelete( - member: TeamMemberRow, + /** + * One page of seats, as `disableMember` does one member. Everything that + * answers for the whole page -- who is already off, what they hold, the + * audit -- goes out once; what is genuinely per account stays per account. + * Returns how many of them end up disabled. + */ + async #disableSeatsForDelete( + seats: TeamMemberRow[], team: TeamRow, actorUserId: number, billing: TeamBillingContext, - ): Promise { - const current = await this.stores.user.getByProperty( - 'id', - member.user_id, - { force: true }, + ): Promise { + if (seats.length === 0) return 0; + + // From the primary: a cached `suspended` would re-charge on a retry. + const suspended = await this.stores.user.getSuspendedByIds( + seats.map((m) => m.user_id), ); - if (current?.suspended) return; + const todo = seats.filter((m) => !suspended.get(m.user_id)); + // Already off, and still accounts the team is losing. + if (todo.length === 0) return seats.length; - await this.stores.team.appendAudit({ - teamId: team.id, - userId: member.user_id, - actorUserId, - action: 'disable', - reason: 'team_deleted', - }); - const held = await this.#heldBytes(member.user_id); - await this.#suspend(member.user_id); + const ids = todo.map((m) => m.user_id); + const held = await this.stores.fsEntry.getHeldBytesForUsers(ids); + const rows = await this.stores.user.getByIds(ids); + // Recorded first, so a failed append cannot leave an unlogged suspension. + await this.stores.team.appendAuditMany( + todo.map((m) => ({ + teamId: team.id, + userId: m.user_id, + actorUserId, + action: 'disable', + reason: 'team_deleted', + })), + ); - // Per seat, not one bulk event: the byte charge is per account. - this.#emitBilling('team.account.disabled', { - ...billing, - user_id: member.user_id, - user_uuid: member.uuid, - username: member.username, - held_bytes: held, - }); + // Bounded: what is left per seat is a write and a mail send. + const outcomes = await runWithConcurrencyLimitSettled( + todo, + TEARDOWN_CONCURRENCY, + async (member) => { + await this.#suspend(member.user_id); - // The team notice covers the disabling, so a member is told once. - await this.#notifyMember(member.user_id, 'team_closed', team); + // Per seat, not one bulk event: the byte charge is per account. + this.#emitBilling('team.account.disabled', { + ...billing, + user_id: member.user_id, + user_uuid: member.uuid, + username: member.username, + held_bytes: held.get(member.user_id) ?? 0, + }); + + // The team notice covers the disabling, so a member is told once. + await this.#notifyUser( + rows.get(member.user_id), + 'team_closed', + team, + ); + }, + ); + + const done = outcomes.filter((o) => o.status === 'fulfilled').length; + // Settled: a rejection cannot cancel its peers, so finish the page. + const failed = outcomes.find((o) => o.status === 'rejected'); + if (failed) throw failed.reason; + return seats.length - todo.length + done; } /** Team owner only. Readable after deletion -- that is the point of it. */ diff --git a/src/backend/stores/fs/FSEntryStore.ts b/src/backend/stores/fs/FSEntryStore.ts index 84a81a783..d33985a1c 100644 --- a/src/backend/stores/fs/FSEntryStore.ts +++ b/src/backend/stores/fs/FSEntryStore.ts @@ -3344,6 +3344,38 @@ export class FSEntryStore extends PuterStore { return Number(rows[0]?.totalUsage ?? 0); } + /** + * Held bytes for several accounts in one grouped read, as `getHeldBytes` + * does for one. + */ + async getHeldBytesForUsers( + userIds: number[], + ): Promise> { + const out = new Map(); + const unique = [ + ...new Set( + (Array.isArray(userIds) ? userIds : []).filter( + (id): id is number => typeof id === 'number', + ), + ), + ]; + for (const id of unique) out.set(id, 0); + + for (let offset = 0; offset < unique.length; offset += 200) { + const chunk = unique.slice(offset, offset + 200); + const placeholders = chunk.map(() => '?').join(', '); + const rows = (await this.clients.db.read( + `SELECT user_id, COALESCE(SUM(size), 0) AS ${this.clients.db.quoteIdentifier('totalUsage')} ` + + `FROM fsentries WHERE user_id IN (${placeholders}) GROUP BY user_id`, + chunk, + )) as { user_id: number; totalUsage: number }[]; + for (const row of rows) { + out.set(Number(row.user_id), Number(row.totalUsage ?? 0)); + } + } + return out; + } + /** * `consistentRead` reads both queries from the primary instead of a replica * — used for the exact re-check when a replica-backed read would reject a diff --git a/src/backend/stores/team/TeamStore.ts b/src/backend/stores/team/TeamStore.ts index 4e08f4c67..1cbd46cab 100644 --- a/src/backend/stores/team/TeamStore.ts +++ b/src/backend/stores/team/TeamStore.ts @@ -820,6 +820,38 @@ export class TeamStore extends PuterStore { ); } + /** The same rows as `appendAudit`, in one statement per chunk. */ + async appendAuditMany( + entries: Array<{ + teamId: number; + userId: number; + actorUserId: number; + action: string; + reason?: string | null; + }>, + ): Promise { + if (entries.length === 0) return; + for (let offset = 0; offset < entries.length; offset += 200) { + const chunk = entries.slice(offset, offset + 200); + const values = chunk.map(() => '(?, ?, ?, ?, ?, ?, ?)').join(', '); + const params = chunk.flatMap((e) => [ + e.teamId, + e.teamId, + e.userId, + e.userId, + e.actorUserId, + e.action, + e.reason ?? null, + ]); + await this.clients.db.write( + 'INSERT INTO `audit_team_membership` ' + + '(`group_id`, `group_id_keep`, `user_id`, `user_id_keep`, ' + + `\`actor_user_id\`, \`action\`, \`reason\`) VALUES ${values}`, + params, + ); + } + } + /** The whole team's audit, newest first, keyset-paginated on `id`. */ async listAudit( teamId: number, diff --git a/src/backend/stores/user/UserStore.ts b/src/backend/stores/user/UserStore.ts index 176ff4c9a..c077b25c5 100644 --- a/src/backend/stores/user/UserStore.ts +++ b/src/backend/stores/user/UserStore.ts @@ -248,6 +248,40 @@ export class UserStore extends PuterStore { return this.getByProperty('referral_code', normalized, opts); } + /** + * Which of these accounts are suspended, read from the primary. + * + * Deliberately not `getByIds`, which answers from cache: a caller deciding + * whether to suspend an account needs the row as it is now, not as it was + * when something last cached it. + */ + async getSuspendedByIds(ids: number[]): Promise> { + const out = new Map(); + const unique = [ + ...new Set( + (Array.isArray(ids) ? ids : []).filter( + (id): id is number => typeof id === 'number', + ), + ), + ]; + for ( + let offset = 0; + offset < unique.length; + offset += BULK_QUERY_CHUNK_SIZE + ) { + const chunk = unique.slice(offset, offset + BULK_QUERY_CHUNK_SIZE); + const placeholders = chunk.map(() => '?').join(', '); + const rows = (await this.clients.db.pread( + `SELECT \`id\`, \`suspended\` FROM \`user\` WHERE \`id\` IN (${placeholders})`, + chunk, + )) as Array<{ id: number; suspended: unknown }>; + for (const row of rows) { + out.set(Number(row.id), Boolean(row.suspended)); + } + } + return out; + } + /** * Batched lookup by id. Dedupes input ids, reads cache via a pipelined * MGET, and resolves remaining misses with a single `SELECT … WHERE id IN