fix(security): make organisation-name-as-canonical explicit - #159
Merged
Merged
Conversation
added 4 commits
September 10, 2026 12:44
…scoping Decision 228b3cc8 piece 1. custom:organization always carries the organisation NAME (assignUserRole writes it, extractOrgFromEvent reads it back verbatim), never the generated orgId UUID. The non-admin filter in listOrganizations compared item.orgId against the NAME-valued claim, which is always false for a real row -- non-admins got an empty list. Admin bypass is preserved (deliberate cross-tenant visibility, same as listUsers/getUser); only the underlying comparison is fixed. The existing org-scoping fixture masked this because it set orgId equal to the claim value by coincidence. Updated the fixture to a realistic row shape (UUID orgId, distinct name) so the assertion actually exercises name-to-name matching.
Decision 228b3cc8 pieces 2 and 3. Piece 2 (uniqueness): createOrganization replaced its Scan-then-Put name check (non-atomic, race-prone) with a conditional Put of a NAME#<name> reservation row in the SAME OrganisationTable (ConditionExpression: attribute_not_exists(orgId)) -- the same write-once idiom already used across this codebase (eval-comparison- resolver.ts, eval-run-resolver.ts, execspec-resolver.ts, etc.). DynamoDB evaluates the condition atomically server-side, so two concurrent creates for the same name can no longer both succeed; the loser's ConditionalCheckFailedException is translated to the existing 'already exists' error. No table/GSI change needed. Piece 3 (reuse prevention): deleteOrganization's own comment warned that a freed name could be recreated and inherit the previous tenant's name-stamped rows. The reservation row is now tombstoned (itemType flipped to name_tombstone, retained permanently) instead of deleted, and createOrganization rejects a tombstoned name with a clear message. Chose tombstone-the-owned-reservation-row over 'refuse delete while any name-stamped row survives' because the latter would require scanning tables this resolver has no handle on or consistency guarantee over, while the reservation row is a single race-free source of truth this resolver already owns.
Decision 228b3cc8 piece 4. Adds org-name-canonical-guard.test.ts:
(a) fails if any updateOrganization/renameOrganization mutation is
declared in schema.graphql's Mutation block, or any equivalent
dispatch case / name-mutating UpdateCommand shape appears in
organization-resolver.ts. Dispatch-switch and UpdateCommand checks
are TypeScript-AST-based (SwitchStatement/CallExpression node
walking), not regex, per repo convention; the GraphQL SDL check
tokenizes the Mutation block's field names since SDL has no
compiler AST.
(b) asserts the seeded admin's custom:organization literal
(seed-admin-user/index.py) equals a name literal seeded by
seed-organizations/index.py, so seeding cannot silently drift.
(c) requires a comment in auth-event.ts documenting that
custom:organization is canonically NAME-based -- added that
documentation to the top of the file.
All three currently pass against the codebase as fixed by pieces 1-3.
Decision 228b3cc8 piece 5. adminCreateUser previously set only email, email_verified, given_name and family_name -- the organisation claim was applied SOLELY by a separate, OPTIONAL assignUserRole call the frontend made only when a role was also selected. An organisation-only, no-role creation left the user with no custom:organization claim at all, which then fails closed (by design) across every org-scoped resolver with no clear signal of why. organization is now a REQUIRED field on AdminCreateUserInput (schema and resolver) and is written directly into custom:organization on AdminCreateUserCommand. Chose to require it, not default it: Team.tsx's Add User dialog already collects an organisation from the name-valued organizations list, so a silent default (e.g. 'Default') would let a caller who forgot to pick one create a user in the wrong tenant with no warning -- worse than a clear upfront error. adminCreateUser stays an admin-only write; PR 146's non-user-writable custom:organization attribute is unaffected. Frontend: Team.tsx's Add User dialog now requires an organization (button disabled until selected) and passes it straight to adminCreateUser instead of relying on the follow-up assignUserRole.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The organisation name is canonical for the
custom:organizationclaim and all tenancy comparisons. This is what the platform already did - the projects family, datastore rows and their index, user-management, and the governance ledger are all name-versus-name - but it was convention rather than mechanism, and one comparison disagreed.orgIdwas considered and rejected. The investigation that decided it found no rename path exists (organization-resolverdispatches only create and delete; noupdateOrganizationsymbol anywhere), so the mutability hazard that would make names unsafe is unreachable, while migrating to rekeying 25 GSIs, rewriting an integrations table keyed onORG#<name>, and re-issuing every Cognito claim atomically with the row rewrite - for no benefit while nothing renames.Changes
1. The one mixed comparison, fixed.
listOrganizationscompared a row'sorgIdagainst the name-valued claim - always false, so non-admins got nothing. Now name-to-name. The existing fixture coincidentally setorgIdequal to the claim, which is why no test caught it; replaced with a distinct-name fixture that actually exercises the path.2. Name uniqueness is now atomic. The scan-then-write race is replaced by a conditional put of a
NAME#<name>reservation row (attribute_not_exists), reusing the write-once idiom already used elsewhere in the repo. No new table or GSI. Concurrent creates: exactly one wins.3. Freed names are tombstoned.
deleteOrganization's own comment warned that reuse "risks cross-tenant access if the org name is ever reused". Deletion now flips the reservation row to a permanent tombstone and creation rejects it with a distinct message, so a recreated organisation cannot inherit the previous tenant's name-stamped rows. Chosen over blocking deletion while name-stamped rows survive, which would require scanning tables this resolver does not own.4. The convention is pinned mechanically. An AST guard fails if a rename operation appears in the schema
Mutationblock, the dispatch switch, or as anUpdateCommandsetting the name - the whole model rests on names never changing, and that was convention only. Plus a test asserting the seeded admin's claim equals the seeded organisation name, and the canonical rule documented in the shared auth helper with an explicit warning against reintroducing an 'orgId' comparison.5. Users can no longer exist without a tenancy claim.
adminCreateUsersetscustom:organizationin theAdminCreateUserCommand(admin API - PR 146's non-user-writable rule intact). Previously it was applied only by a separate optionalassignuserRole, so an admin who skipped that step created an account that failed closed across ~43 gates.Please note
organizationis now required on the create-user input - a breaking change for any consumer supplying it. Chosen over a default because the dialog already collects the value, and a silent default would misassign a tenant quietlylistUsers/getUserTesting
Red-first throughout. Non-admin now receives their own organisation where they previously got none; duplicate-name race proven closed at the condition expression; tombstoned name rejected on recreate; immutability guard proven to bite on a planted rename and to ignore the legitimate tombstone update; guard verified AST-based, no false positive from the token in a comment or string literal.
tsc o, Lint o, targeted 89, all 14 gate-enumeration guards 348), full backend jest 7847, frontend 2068,
vite build0.Outstanding
No backfill exists for existing users created before this change who carry no claim.