Skip to content

πŸ—οΈ PUT-1708 + PUT-1709 + PUT-1743: TeamController, teams_enabled, and the isolation suite - #3714

Merged
jfcastro92 merged 1 commit into
juancastro/put-1705-24-teamservice-provision-an-org-accountfrom
juancastro/put-1708-26-teamcontroller-and-audit-writes
Sep 3, 2026
Merged

πŸ—οΈ PUT-1708 + PUT-1709 + PUT-1743: TeamController, teams_enabled, and the isolation suite#3714
jfcastro92 merged 1 commit into
juancastro/put-1705-24-teamservice-provision-an-org-accountfrom
juancastro/put-1708-26-teamcontroller-and-audit-writes

Conversation

@jfcastro92

@jfcastro92 jfcastro92 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Eighth in the stack: #3704 β†’ #3705 β†’ #3708 β†’ #3709 β†’ #3710 β†’ #3712 β†’ #3713 β†’ this. Review bottom-up.

Covers PUT-1708 (controller + audit writes), PUT-1709 (teams_enabled) and PUT-1743 (the isolation regression suite). Grouped because 2.7 cannot pass until the surface exists, and 2.8 gates exactly what 2.6 adds.

The routes

Twelve, under /teams, all on the api subdomain.

Method Path Who
POST / GET /teams any signed-in user / the caller's own workspaces
GET /teams/:uid member
PUT / DELETE /teams/:uid master
GET / POST /teams/:uid/members member / master
POST …/members/:username/activation master
POST …/members/:username/disable and /enable master
GET /teams/:uid/audit master
GET /teams/:uid/audit/me member

Every route sets requireUserActor: true. That option is what makes server.ts install requireAuthGate, requireVerifiedAccount and requireNonAccessTokenGate β€” needsAuth is derived from the route options, so a route that sets none gets no auth middleware at all even though req.actor is populated globally. Reads need it as much as writes: the suspension check lives in requireAuthGate, and without it a just-disabled member would still read the roster and a scoped access token would reach the audit log. gates.ts is explicit that account management stays closed to every access token, which is requireUserActorGate's job rather than requireNonAccessTokenGate's β€” the latter admits full-access PATs and never inspects actor.app.

Authority is checked before anything observable. Two orderings matter:

  • The body is parsed after requireOwner, so an unauthorized caller cannot learn whether their request was well-formed by comparing 400 against 403.
  • :username is resolved after requireOwner, so the member routes are not a global username-existence oracle.

A non-member gets 404 rather than 403 so the endpoint does not confirm a workspace exists, and the workspace owner is refused as the target of every member route via org_owned.

Layering: the controller touches no database

Audit writes live in TeamService, at the point of each action β€” provisionAccount, disableMember, enableMember, deleteWorkspace β€” rather than in the route handlers. A caller reaching the service directly (a driver, an extension, a cron job) therefore cannot skip them, which matters for a table whose whole purpose is accountability under a single administrator. The SQL lives in TeamStore: appendAudit, listAudit, listAuditForUser, countPayers.

Audit reads map internal user ids to usernames before responding, the same way toClientTeam strips team.id, and stay readable by the owner after the workspace is soft-deleted β€” otherwise the delete_team entry would be written and immediately unreachable, since the normal lookup filters deleted_at IS NULL.

Provisioning reuses signup's validation

USERNAME_REGEX, USERNAME_MAX_LENGTH, RESERVED_USERNAMES and validator.isEmail, now exported from AuthController rather than reimplemented. A workspace must not be able to mint accounts signup itself would refuse β€” the username becomes the /username home-directory segment β€” nor claim unregistered reserved names, nor send activation mail to unvalidated addresses.

Handle problems surface as 400 (unusable) or 409 (taken), including the unique-index race. A bare Error here would reach server.ts as a 500 and a deduped critical alarm, so an uppercase handle would page on-call.

Disable takes effect on the next request

disableMember drops sessions through SessionStore.removeByUuid rather than a raw DELETE. The store invalidates every composite cache key with its double-delete pattern; without that, a disabled member keeps authenticating from the session cache for its TTL β€” which is exactly the "next authenticated request, not after a cache TTL" property PUT-1707 relies on.

Going through the store also revokes rather than deletes, so last_ip and last_user_agent survive. PUT-1746's member-facing audit view reads those to make administrative use of a reset visible to the account it was performed on; a hard delete would remove that evidence.

The feature flag

teams_enabled gates route registration, through an optional isEnabled() that #registerControllerRoutes honours. With it off the paths do not exist rather than existing and refusing, and no team code is reachable. It does not gate DDL β€” the tables ship either way and the feature is inert, which is what lets phases 1–3 be live in production before a workspace can be created. It is also the backout from phase 4 on: turning it off removes the feature without touching data.

The isolation suite

TeamIsolation.http.test.ts asserts the negative the whole feature rests on: a workspace manages accounts and cannot read them. Negatives do not defend themselves β€” nobody writes that grant deliberately, but a new implicator or a widened actor would create it and no other test would fail.

Case Expected
Workspace owner β†’ a member's file refused
Workspace owner β†’ a member's home directory refused
Workspace owner via full-access token β†’ a member's file refused
/teams/:uid/files, …/members/:username/files, …/kv no such route
Disabled member β†’ their own file refused
Workspace owner β†’ that same file, after the disable still refused
Workspace owner β†’ its own files allowed
Member β†’ their own files allowed
Workspace owner β†’ a file the member explicitly granted allowed

The full-access token case is called out separately because #scanAccessToken resolves such a token by re-scanning the issuer as a plain user actor β€” the path a future implicator would be inherited through.

The suite asserts outcomes, never the absence of an implicator: a test checking that none is registered would pass while the access existed by another route. The last row is the control that keeps the refusals from being vacuous β€” it refuses before the grant and succeeds after, inside one test, so it cannot pass by the endpoint being broken in either direction.


Verification

$ npx vitest run --config src/backend/vitest.config.ts src/backend/{controllers,services,stores}/team/
 Test Files  4 passed (4)
      Tests  70 passed (70)

$ PUTER_TEST_DB_ENGINE=postgres … src/backend/{services,stores}/team/
      Tests  52 passed (52)

$ PUTER_TEST_DB_ENGINE=postgres … src/backend/controllers/team/
      Tests  18 passed (18)

$ npm run test:backend
 Test Files  254 passed | 24 skipped (278)
      Tests  6778 passed | 26 skipped (6804)

$ npm run typecheck
Type check passed β€” no new errors (33 known, baselined).

Both HTTP suites run against a real server on both engines. Worth knowing for anyone adding to them: a failed beforeAll is reported by vitest as skipped, not failed, so a broken fixture makes a security suite silently stop running while CI stays green. Two dialect-specific fixture bugs were caught that way β€” is_dir is a real boolean on postgres, and pragma_table_info is sqlite-only.

Known gaps

Gap Note
The audit lists are capped at 200 with no cursor Older entries drop off the admin view silently. listMembers in the same store is keyset-paginated; the audit should match
The audit row is appended after the state change, outside a transaction If appendAudit throws, the account is already suspended and signed out with no record of it
Billing events, invalidateActorSubscription, the outer.gui.* push phase 3 and the GUI phase

Closes PUT-1708, PUT-1709 and PUT-1743.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Coverage Report

Status Category Percentage Covered / Total
πŸ”΅ Lines 93.83%
⬇️ -0.14%
27659 / 29477
πŸ”΅ Statements 91.92%
⬇️ -0.16%
29991 / 32624
πŸ”΅ Functions 90.18%
⬇️ -0.21%
4990 / 5533
πŸ”΅ Branches 80.66%
⬇️ -0.25%
19916 / 24690
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/backend/server.ts 84.47%
⬆️ +0.10%
68.8%
⬆️ +0.38%
90.9%
🟰 ±0%
85.68%
⬆️ +0.11%
107-108, 122-127, 197-210, 237-245, 352, 360-363, 373-374, 414, 519, 525, 562-564, 598-599, 644, 654, 657, 697-702, 705-711, 714-720, 723-729, 756, 761, 765-767, 782, 802, 810-837, 855, 882-894, 897-904, 947, 978-983
src/backend/types.ts 100%
🟰 ±0%
100%
🟰 ±0%
100%
🟰 ±0%
100%
🟰 ±0%
src/backend/controllers/index.ts 100%
🟰 ±0%
100%
🟰 ±0%
100%
🟰 ±0%
100%
🟰 ±0%
src/backend/controllers/team/TeamController.ts 72.37% 43.33% 63.26% 73.97% 6, 24-26, 32, 33, 34, 38-39, 44, 45, 47, 163-167, 182-193, 201, 240-245, 257-269, 282-289, 305-307, 320-322, 327-329, 336-338
src/backend/services/team/TeamService.ts 81.7% 67.36% 92.85% 84.1% 113-115, 118-120, 129-162, 190-193, 201, 207, 236-239, 286-288, 342, 402-405, 443-445, 465, 480
src/backend/stores/team/TeamStore.ts 95.91% 86.95% 95.83% 96.73% 240, 266, 373-378
Generated in workflow #1445 for commit 9bc2cd7 by the Vitest Coverage Report Action

@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from cef8d34 to 4f9dafd Compare September 1, 2026 21:14
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 4f9dafd to d1fe0a7 Compare September 1, 2026 21:36
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from d1fe0a7 to 4cfb67c Compare September 1, 2026 22:04
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch 2 times, most recently from f8a5120 to 9d6c97e Compare September 1, 2026 22:38
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 9d6c97e to 66a4984 Compare September 1, 2026 22:51
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 66a4984 to 457e996 Compare September 2, 2026 16:05
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 457e996 to 6645730 Compare September 2, 2026 16:51
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 6645730 to 75103bb Compare September 2, 2026 19:32
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch 2 times, most recently from 40da279 to 6c19992 Compare September 2, 2026 20:05
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 6c19992 to e548b8d Compare September 2, 2026 21:38
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from e548b8d to 1577f8b Compare September 3, 2026 13:46
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 1577f8b to e01c3eb Compare September 3, 2026 14:30
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from e01c3eb to 3b20199 Compare September 3, 2026 15:42
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 3b20199 to 5dc7aac Compare September 3, 2026 18:56
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 5dc7aac to ad8f7a5 Compare September 3, 2026 19:07
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from ad8f7a5 to 41c88f4 Compare September 3, 2026 19:17
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 41c88f4 to 5f6f322 Compare September 3, 2026 19:20
@jfcastro92

Copy link
Copy Markdown
Collaborator Author

Local validation

Engine: mysql 8 Β· full teams stack Β· verified session actors

The flag genuinely unregisters the routes. With teams_enabled absent from
config.json (so defaulting to false), against a running server:

/teams                 -> 404
/teams/abc             -> 404
/teams/abc/members     -> 404
/whoami                -> 401     <- control: this route exists and wants auth

{"error":"Not Found","code":"not_found"}. The contrast with /whoami is the
point β€” 404 rather than 403 means the paths do not exist, not that they exist and
refuse. Adding "teams_enabled": true and restarting, with nothing else changed:

/teams                 -> 401 {"code":"token_missing"}
/teams/abc/members     -> 401

404 β†’ 401 purely from config, which also confirms the value reaches the
controller from the real config.json rather than from a test fixture.

Isolation: an outsider cannot distinguish a real workspace from a nonexistent
one.
tmoutsider is authenticated and verified, but not a member:

GET  /teams/<real-uid>          -> 404 {"error":"Workspace not found","code":"team_not_found"}
GET  /teams/00000000-...-0000   -> 404 {"error":"Workspace not found","code":"team_not_found"}
GET  /teams/<real-uid>/members  -> 404 {"error":"Workspace not found","code":"team_not_found"}
GET  /teams/00000000-...-0000/members -> 404 {"error":"Workspace not found","code":"team_not_found"}

Byte-identical, so the endpoint is not an existence oracle. Mutations behave the
same way rather than falling through to a 403:

PUT    /teams/<real-uid>  {"name":"Hijacked"} -> 404 team_not_found
DELETE /teams/<real-uid>                      -> 404 team_not_found

Audit endpoint returns the real trail, newest first, with actor attribution:

{"action":"enable",    "username":"acmeseat1","actor_username":"tmowner","created_at":"...23:29:40.000Z"}
{"action":"disable",   "username":"acmeseat1","actor_username":"tmowner","created_at":"...23:29:39.000Z"}
{"action":"provision", "username":"acmeseat3","actor_username":"tmowner","created_at":"...23:27:29.000Z"}
{"action":"provision", "username":"acmeseat2","actor_username":"tmowner","created_at":"...23:27:29.000Z"}
{"action":"provision", "username":"acmeseat1","actor_username":"tmowner","created_at":"...23:27:28.000Z"}

Owner CRUD

POST /teams {"name":"Acme Corp","handle":"acme"} -> 200, uid 13341565-...
GET  /teams                                       -> 200 {"items":[ ...1 item, is_owner:true ]}
GET  /teams/<uid>                                 -> 200
PUT  /teams/<uid> {"name":"Acme Renamed"}         -> 200, name updated
GET  /teams/<uid>/members                         -> 200 [{"username":"tmowner","org_owned":false}]

⚠ One follow-up tracked in PUT-1745, not here: the credential-reissue route is not
currently written to the audit table, so the actions above are the complete set.
Detail is on that ticket.

@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 5f6f322 to 33f593f Compare September 3, 2026 19:45
…uite

Covers PUT-1708, PUT-1709 and PUT-1743.

Twelve routes, every one setting requireUserActor -- that option is what
installs requireAuthGate, requireVerifiedAccount and requireNonAccessTokenGate,
because server.ts derives `needsAuth` from the route options. Reads need it as
much as writes: without an auth option a route gets no suspension check and
admits access tokens, so a just-disabled member could still read the roster and
a scoped third-party token could read the audit log.

Authority is checked before anything observable. Validating the body first made
POST /members answer 400 before 403, and resolving :username first turned the
member routes into a global username-existence oracle.

Provisioning applies the same username and email rules as signup rather than
its own -- USERNAME_REGEX, USERNAME_MAX_LENGTH, RESERVED_USERNAMES and
validator.isEmail, now exported from AuthController. Without them a workspace
could mint accounts signup would refuse, claim unregistered reserved names, and
mail arbitrary unvalidated addresses.

Handle problems are 400 or 409 rather than a bare Error, which the server turns
into a 500 and a deduped critical alarm -- an uppercase handle should not page
on-call.

Disable drops sessions through SessionStore.removeByUuid rather than a raw
DELETE. The store invalidates every composite cache key; without that a
disabled member kept authenticating from cache for the session TTL, which is
exactly the "takes effect on the next request, not after a cache TTL" property
disable is supposed to have. Revoking also preserves last_ip/last_user_agent,
which the member-facing audit view reads.

Audit writes live in TeamService at the point of each action rather than in the
route, so a caller reaching the service directly cannot skip them, and the SQL
lives in TeamStore. Audit reads map internal user ids to usernames, and remain
readable by the owner after the workspace is soft-deleted -- otherwise the
delete_team entry was written and immediately unreachable.

teams_enabled gates route registration through an optional isEnabled() the
server honours, so with it off the paths do not exist rather than existing and
refusing. It does not gate DDL.

TeamIsolation.http.test.ts asserts the negative the feature rests on: the
workspace manages accounts and cannot read them, including through a
full-access token and after the member is disabled. It asserts outcomes rather
than the absence of an implicator.
@jfcastro92
jfcastro92 force-pushed the juancastro/put-1708-26-teamcontroller-and-audit-writes branch from 33f593f to 9bc2cd7 Compare September 3, 2026 20:02
@jfcastro92
jfcastro92 merged commit d855315 into main Sep 3, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants