Multi-organization membership - #376
Open
jhodapp wants to merge 24 commits into
Open
Conversation
Living plan for letting a SuperAdmin (or a scoped org admin) attach an existing user to another organization with an explicit org role, notify them by email, and remove them from a single org without destroying the account. Also gitignores docs/implementation-plans/handoffs/, which holds the per-phase implementer handoffs (working docs, not version-controlled).
Adds the entity_api and domain foundation for a user belonging to more than one organization: attaching an existing user to another organization, removing them from one organization without deleting their account, and looking a user up by email under a visibility scope. Scoped lookup. lookup_by_email_scoped returns 0 or 1 results and never distinguishes "no such email" from "a real user you may not see": an empty vector is the only not-found signal, and the visibility check (shares_administered_organization) runs on every path, including unknown emails and super-admin requesters. shares_administered_organization always issues exactly two queries, so query count (and therefore response time) carries no enumeration signal. The UserLookupResult DTO exposes only id, name and email. Role scoping. scope_roles_to_organization is a security control, not a cosmetic filter. users::Model serializes its roles vector to API clients, so find_by_organization now drops every role belonging to another organization (keeping global roles) before returning members. Without it, one organization's admin would learn which other organizations each member belongs to. Guards. user_role::create rejects SuperAdmin before any query (422 rather than the 500 the entity-level before_save produces), remove_from_organization refuses to remove an organization's last admin, and the global user delete now refuses a user who belongs to more than one organization. Single-org delete behavior is unchanged. Three new error variants flow entity_api -> domain -> web, all mapping to 409.
Phase 1 landed in b6325c6. Notes the two things phase 2 must not drop: the visibility check that attach_to_organization deliberately omits, and dropping organization_count from the multi-org conflict response body. Also defers the last-admin guard on the global user delete, which needs a per-administered-org check rather than the single-org one already shipped.
Exposes the Phase 1 multi-org membership primitives over HTTP: - GET /users?email= returns a narrow DTO array of 0 or 1 elements, scoped by domain::user_role::lookup_by_email_scoped. - POST /organizations/:organization_id/users/:user_id/role attaches an existing user to an organization with a role. - DELETE on the same path removes a membership without deleting the account. The two organization routes are gated by a new OrganizationAdminAccess extractor, which evaluates the SuperAdmin/Admin predicate in memory against the roles AuthenticatedUser already hydrated, so it costs no extra connection. attach_to_organization performs no requester check, so the handler runs one via the new domain::user_role::can_administer_user. A caller who may not see the target gets 404, not 403: a 403 for "this user exists but is not yours to see" versus a 404 for "no such user" would tell the caller which user ids are real, the same enumeration oracle the lookup endpoint is built to avoid. attach_to_organization already returns NotFound for a genuinely missing user, so both paths converge on 404. The 409 for UserBelongsToMultipleOrganizations no longer reports the organization count. The caller cannot act on it and has no right to know how many other organizations a member belongs to; the message already says what to do instead.
Phase 2 landed in 452fcf5. Notes for the frontend phase that success responses are HTTP 200 with the real code in the ApiResponse envelope while errors carry real HTTP status codes, and records the false-passing-mock failure mode phase 2 surfaced.
…cess Moves the last three organization-user routes (create, resend-invite, delete) off the `protect::organizations::users` middleware and onto the `OrganizationAdminAccess` extractor, closing the split introduced when the role routes were added. Part of issue #218. The `UserIsNotSelf` predicate that guarded self-deletion lived in the middleware, so the guard now sits inline at the top of `delete`, matching the one already in `remove_role`. `OrganizationAdminAccess` is declared ahead of `OrganizationUserAccess` so a non-admin still gets 403 rather than a 404 for a user id they may not probe. `UserIsAdmin` and `UserIsNotSelf` stay in `protect::mod`; `UserIsAdmin` still has callers, and `UserIsNotSelf` is kept for the routes still to be migrated. Behavior is pinned by nine regression tests in `web/src/controller/organization/user_controller_tests.rs`, written and passing against the pre-migration wiring and unchanged afterwards.
Phase 2b landed in bc0a355. Notes that resend-invite's regression test can only assert reachability past the auth gate, not success, so phase 5 must exercise it manually, and records the harmless delete error-precedence change.
Attaching an existing user to an organization now sends them an "added to organization" email through Resend, mirroring the welcome email sent to brand-new users. Delivery is best-effort: failures are logged and never propagate, so a membership that already committed is never undone by an email problem, and an unconfigured template id degrades to a logged warning rather than an empty template id. OrganizationAdminAccess now carries the organizations::Model it already fetched for its 404 check instead of discarding it and exposing only the id. Handlers read organization.id, so this removes a would-be duplicate query on a path that can already consume three DB connections. Behavior is unchanged: same 400/403/404 and same statuses everywhere. ADDED_TO_ORGANIZATION_EMAIL_TEMPLATE_ID is threaded through both compose files and both deploy workflows as a vars. reference.
Phase 3 landed in 49425d9. Records that deleting the notify call from attach_role leaves all web tests green, so phase 5 must confirm delivery manually; the same gap predates this branch on the welcome-email path.
Phase 4 landed in fe 3fb7c5fb. Records the hand-verified wire contract between the repos, which neither suite can check, and the pre-existing MemberCard delete bug that would have swallowed the new 409.
The old partial unique index keyed on (user_id, organization_id, role), so a user could hold both User and Admin in the same organization. Role resolution then depends on row order, and one of the possible answers is Admin. The application-level check in attach_to_organization is a check-then-act: two concurrent grants carrying different roles both see "no existing role" and both insert. Replace it with a partial unique index on (user_id, organization_id) and drop the three-column one, which the new index strictly subsumes: a pair unique on (user_id, organization_id) is necessarily unique with role appended, and the (user_id, organization_id) prefix still serves the same lookups. The global-role index for organization_id IS NULL is untouched. up() refuses, naming every offending pair, when duplicates already exist rather than deleting rows and silently changing effective privileges. entity_api::user_role::create now maps a duplicate-key error naming the new index to UserAlreadyInOrganization, so the racing writer gets the same 409 as the sequential path instead of a 500.
Adding a member and assigning their coach were two requests, and the backend sent the invitation email at the end of the first one. A failed coach assignment therefore left the person invited into an organization where their coach was never assigned. A row is recoverable, a sent email is not. Both member endpoints now accept an optional coach_id and create the coaching relationship inside the membership's own transaction, so nothing commits unless both succeed. The email calls stay where they are, after the domain call: they cannot be rolled back, so they have to follow the commit. Also rejects a user as their own coach in entity_api, which benefits the pre-existing Assign Coach flow too.
The handler forwarding coach_id was unpinned: hardcoding None at both call sites, silently discarding every coach assignment, passed all 644 backend tests. Each layer was tested in isolation and the seam between them was not. Also adds the org-admin half of the scoped lookup's condition, which only had super-admin coverage, and the two documented NotFound paths on remove_from_organization and attach_to_organization. All four verified by sabotage: each fails when the behavior it guards is broken.
Live end-to-end testing surfaced a case no unit test could: the admin of a freshly created organization cannot add anyone to it, because the lookup only returns users sharing an organization they already administer, and a new organization is empty. Records the cause and three options for resolving it.
GET /users/{user_id}/coaching_sessions and its counts sibling filtered only
on the caller being coach or coachee of the relationship, so a user who
belongs to more than one organization saw every organization's sessions
regardless of the selected one. The defect predates multi-org membership but
was unreachable then: nobody could belong to two organizations, so a
user-scoped query was accidentally equivalent to an org-scoped one.
Both queries already join coaching_relationships, so this adds a filter on an
existing join rather than a new one. organization_id is optional on both
params structs; omitting it preserves the previous behavior exactly.
Phase 8 fixed the reported Upcoming/Previous list. Records the four session
hook call sites and the /users/{id}/actions endpoint that still return data
from every organization, ranked for follow-up, plus a correction: the actions
kanban is org-safe via a server-side org-scoped endpoint, not client filtering.
jhodapp
changed the title
Removing a member deleted their coaching relationships in that organization. coaching_sessions references coaching_relationships with NO ACTION, so any member who had ever had a session scheduled hit a foreign key violation and got a 503. Verified against the live database, where the endpoint worked only for members with no history at all. The relationships that do cascade made it worse: goals and coaching session series would have been destroyed for third parties, since removing a coach tears down their coachees' relationships too. Refuses with a structured 409 naming the relationship and session counts, mirroring organization_not_empty, and leaves the operator to unwind deliberately. Members with no sessions are still removed as before. Also from review: - Duplicate-key detection matches the constraint name via SqlErr instead of substrings of the Postgres message, which is locale and version sensitive. The users.email site was the looser of the two: any unique index whose name contained "email" was reported as a membership collision. - Pins the wire shape of all four new 409 bodies. The FE branches on these and lands in lockstep. One of them pins a non-disclosure: the multi-org conflict deliberately omits the organization count. - Controllers no longer fabricate domain errors to pick a status code. WebErrorKind gains Forbidden and NotFound so the web layer stays in its own vocabulary. - The lookup admin gate now requires an organization on the Admin role, matching the SuperAdmin arm beside it. - Indexes LOWER(email), which the case-insensitive lookup needs and which users_email_key cannot serve. - Drops UserIsNotSelf, orphaned when both call sites moved into handlers.
jhodapp
marked this pull request as ready for review
August 7, 2026 16:49
Contributor
Greptile SummaryThe PR adds multi-organization memberships, scoped user lookup and administration, organization-aware session queries, membership notifications, and database constraints for role and email uniqueness.
Confidence Score: 4/5The PR is not yet safe to merge because account deletion can still erase a membership that a concurrent attachment successfully commits. The new account-deletion checks read the target user’s organization set without locking it, while attachment uses an independent transaction; a role committed after those checks remains reachable by the later unscoped role deletion. Files Needing Attention: domain/src/user.rs and domain/src/user_role.rs
|
| Filename | Overview |
|---|---|
| domain/src/user.rs | Adds multi-organization and last-admin guards to global account deletion, but the membership snapshot remains uncoordinated with concurrent attachment. |
| domain/src/user_role.rs | Adds transactional membership attachment, scoped removal, and visibility helpers with organization-specific behavior. |
| entity_api/src/user_role.rs | Adds membership CRUD, organization counts, visibility queries, and row-locking admin counts. |
| web/src/extractors/organization_admin_access.rs | Adds organization-scoped administrator authorization for membership-management routes. |
| migration/src/m20260806_000000_user_roles_one_role_per_org.rs | Adds the partial unique index enforcing one organization-scoped role per user. |
| migration/src/m20260807_000000_users_lower_email_index.rs | Adds case-insensitive email uniqueness to support reliable existing-user lookup. |
Sequence Diagram
sequenceDiagram
participant Admin
participant Web
participant Domain
participant DB
participant Email
Admin->>Web: Attach existing user to organization
Web->>Web: OrganizationAdminAccess
Web->>Domain: attach_to_organization
Domain->>DB: Begin transaction
Domain->>DB: Validate organization and membership
Domain->>DB: Insert organization role
opt Coach supplied
Domain->>DB: Insert coaching relationship
end
Domain->>DB: Commit
Domain->>Email: Send added-to-organization notification
Domain-->>Web: Organization-scoped user
Web-->>Admin: Created
Reviews (5): Last reviewed commit: "refactor(migration): use execute_unprepa..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The guard counted admins and then deleted in the same transaction without locking. Under read committed two concurrent removals both observe two admins, both pass, and both commit, leaving the organization with no administrator. Counting now takes FOR UPDATE on the rows it counts, so the second removal waits for the first to commit and then re-reads a count of one. Verified against Postgres: a second session blocks for the full lock_timeout and is cancelled "while locking tuple in relation user_roles". Postgres rejects FOR UPDATE alongside an aggregate, so the rows are counted in Rust. Also groups imports as standard library, external crates, then crate-local in the two files this branch reordered.
Deleting an account drops every role row it holds, but the global delete only refused multi-organization users. A super admin deleting the sole admin of an organization left that organization with no administrator, going around the guard the remove-from-organization path enforces. Deletion now refuses when the account is the last admin of any organization it belongs to, reusing the same locked count. The organization-count check moved inside the transaction so both guards see one consistent snapshot. Not reachable by an organization admin: passing the org gate means the organization has another admin, and the not-self guard blocks deleting yourself. The super admin path is the live one.
The driving use case, an admin of two organizations moving a member of one into the other, had no backend test. Only the super admin path was covered, and that path short-circuits the visibility probe, so it stays green even when org admins lose the ability entirely. Confirmed by sabotage: forcing the visibility predicate to false fails both new tests while the super admin test still passes. Also pins the conflict an org admin of a single organization always hits, since every user they can see is already a member. The graph-level claim behind that is exercised live rather than here, where the membership graph is whatever the mock returns.
Matches the 39 other migrations that run plain DDL. Statement::from_string earns its keep only where a result set is read back, as the one-role-per-org migration does for its duplicate pre-check.
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.
Description
Lets a user hold roles in more than one organization under a single account, and gives a SuperAdmin (or an org admin, scoped to people they can already see) a way to find an existing user by email and attach them to another organization with an explicit role.
Driving use case, verified end to end through the UI: take the existing Ehab Bandar in Refactor Group, create a BigTable organization, add him there as org Admin, add Jim as a member with Ehab as his coach, and schedule a BigTable session between them.
The schema already allowed multi-org membership. What was missing was any write path: the only way to gain membership was
create_by_organization, which always inserts a newusersrow, andusers.emailis globally unique. Multiple emails mapping to one profile is explicitly out of scope.Pairs with refactor-group/refactor-platform-fe#433. They must land together: the session endpoints gained an
organization_idparameter the frontend now sends.Changes
entity_api: membership primitives (create,find_by_user_and_organization,delete_by_user_and_organization, admin/org counts) plusshares_administered_organization, the visibility predicate behind scoped lookup.entity_api:scope_roles_to_organizationfilters the serializedrolesarray to the org in scope. Without it,GET /organizations/{id}/userswould start disclosing every member's other organizations the moment multi-org membership became possible.domain:attach_to_organization(transactional, with optional coach),remove_from_organization, andlookup_by_email_scoped.web:GET /users?email=,POSTandDELETE /organizations/{organization_id}/users/{user_id}/role, gated by a newOrganizationAdminAccessextractor.web: migratedcreate,resend_inviteanddeleteoffprotect/middleware onto the same extractor, closing theUserIsAdmin -> AdminAccessitem on Refactor authorization middleware to use Axum FromRequestParts extractor pattern #218.m20260806_000000_user_roles_one_role_per_org: partial unique index on(user_id, organization_id) WHERE organization_id IS NOT NULL.organization_idfilter onGET /users/{user_id}/coaching_sessionsand/counts.coaching_sessionsforeign key.Breaking changes
POST /organizations/{id}/usersnow requires an org admin. The gate moved fromOrganizationMemberAccesstoOrganizationAdminAccessas part of the Refactor authorization middleware to use Axum FromRequestParts extractor pattern #218 migration. Any non-admin who could previously create org users gets a 403. This surfaces as a support ticket, not a build failure, so it needs calling out at deploy time alongside theorganization_idparameter.organization_idon the user session endpoints, so Multi-organization membership refactor-platform-fe#433 must not land before this.Testing Strategy
cargo fmt,cargo clippy --workspace --all-targets -- -D warnings, and:domain 237, entity_api 271, web 144, all green. Never
cargo test --workspace --features mock(sea-orm/mock dropsDatabaseConnection: Clone).Verified beyond the suite:
[]. The scope check runs unconditionally so query count and timing carry no signal.GET /organizations/{id}/usersshows only the in-scope org's role. Invisible from the UI, so it is easy to regress.Concerns
docs/implementation-plans/multi-org-membership.md; none is taken here.ADDED_TO_ORGANIZATION_EMAIL_TEMPLATE_IDis configured and no test covers the call site. The Resend template needs creating, and the id set as a GitHub Actions variable (vars., notsecrets.).GET /users/{user_id}/actions, plus two latent endpoints. Tracked in Scope remaining user-scoped read endpoints by organization (actions, goals, coaching-relationships) #374, deliberately not widened into this PR.DELETE .../users/{id}now 409s for multi-org users. That is a small disclosure (the caller learns the member belongs to at least one other org, not which), and much better than silently destroying the account.