Add group storage to the permissions schema - #43
Open
johnworth wants to merge 12 commits into
Open
Conversation
Groups move out of Grouper and into the DE database so that expanding a subject to its groups becomes a join inside the permission lookup query rather than a call to another service. A group is keyed on its permissions.subjects.id row: subjects already models groups via subject_type, and the permission hot path works in internal subject uuids, so permissions.subject_id and the membership tables join directly. The externally visible group identifier stays subjects.subject_id, which means groups imported from Grouper keep their original identifiers and neither the existing group subjects nor the iRODS @grouper-<id> names need migrating. subject_id gains a default that mints the same dashless 32-hex form so groups created later are indistinguishable from imported ones, and widens to varchar(512) because membership now requires a subject row for every member and the longest usernames already exceed 64 characters. Group structure is explicit -- type, owner, short name -- instead of Grouper's colon-delimited paths, with owner_required in group_types enforcing via foreign key that teams and collaborator lists have an owner while communities and system groups do not. Nesting is preserved: production has 156 nested memberships across ~120 collaborator lists, 5 communities, and 4 teams, and Grouper resolves them transitively today. group_memberships holds direct membership and may reference a group; group_effective_members holds the transitive closure restricted to users, so permission lookup stays a plain join instead of a recursive CTE. The closure is derived state, and deleting a nested group is the case where keeping it correct is not merely a convention. The cascade removes the membership row and destroys the path back to the containing groups before any AFTER trigger or calling service could react, leaving those containers granting access to the deleted group's members. A BEFORE DELETE trigger detaches and recomputes at the one point where the containers are still findable, and it covers deletes from any caller including a cascade from subjects. recompute_group_closure and group_ancestors are SQL functions so the trigger and the groups service share one implementation rather than maintaining a copy each. The identity index uses coalesce rather than NULLS NOT DISTINCT so it works on PostgreSQL before 15, since the production server version is unverified. Verified against a throwaway PostgreSQL 18 container: all 54 migrations apply, up/down/up round-trips, 14 constraint checks each fail against the constraint they target, a deleted nested group no longer leaves its members with inherited access, a from-scratch recomputation of the closure matches row for row, and a membership cycle terminates instead of recursing forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
permissions.subjects.subject_id is a bare username while public.users.username carries a domain suffix, so joining the two means reconstructing the suffixed form at every call site. Nullable and best-effort: a subject can be created before the user has ever logged in to the DE, and 5 of the 19,728 production user subjects correspond to no DE user at all. ON DELETE SET NULL rather than CASCADE, because removing a DE user must not delete the subject and cascade away their memberships and grants. A CHECK keeps group subjects from naming a user, and a partial unique index keeps two subjects from claiming the same one. Not backfilled here: matching needs the username suffix, which is deployment-specific, so population belongs to the services and the Grouper importer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The importer reconciles -- it removes memberships and grants Grouper no longer has -- which is correct only while Grouper is authoritative. Run it once after cutover and it deletes whatever was created natively in the meantime. A single-row table records which system owns group data, and the importer refuses to start unless it says 'grouper'. There is deliberately no override flag: the only way to run a destructive reconcile afterwards is to set this back, which is an attributed, timestamped write rather than something typed at the end of an argument list. Flipping it to 'native' is an ordered step of the cutover, performed before anything is pointed at the new store. A BEFORE UPDATE trigger maintains changed_at, so the record of when cutover happened cannot be left stale or set by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The functions reference the group tables and each other unqualified, so they resolved correctly only for callers that happened to have this schema on their search_path. The groups service and the Grouper importer both set it on the connection, so this was invisible to them -- but deleting a group from any other client, including the permissions service and psql, failed inside the delete trigger with "function group_ancestors(uuid[]) does not exist". That trigger is what stops a deleted group from leaving stale effective membership behind, so the failure mode was a cascade that aborts rather than one that silently corrupts, but it made group deletion impossible for those callers. Found by the permissions service's integration suite, whose fixtures delete subjects without setting a search_path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Grouper distinguished `viewers` -- the group is discoverable and joinable, its member list is not public -- from `readers`, which exposes membership too, and applied the first to public teams and the second to public communities. The permissions service has no level weaker than read, so both import as the same GrouperAll grant and the distinction is lost, silently making every public team's membership world-readable. It is recorded on the group instead, because GrouperAll is a sentinel with no members rather than a real subject. Defaults to false so migrating cannot publish a membership that was not already public. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The BEFORE DELETE trigger recomputes the closure of every group containing the one being deleted. When several nested groups go at once -- a cascade from subjects deletes them row by row -- a container can already be gone by the time its child is deleted, and recomputing it inserts closure rows for a group no longer in `groups`, violating the foreign key and failing the whole statement. A container that is itself being deleted has no closure worth rebuilding, so those are skipped. The single-group case the service performs is unchanged. Found by deleting a 2,593-group dataset with 165 nested edges; three groups was not enough to hit it, because the cascade order happened to be favourable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two concurrent membership writes touching a shared ancestor collided on group_effective_members_pkey, rolling one of them back and losing the write outright. Separately -- and silently -- group_ancestors is computed from the writer's snapshot, so a parent being attached concurrently was never recomputed: both transactions committed and the new member was permanently absent from the new parent's effective membership, so permissions granted to that parent never reached them. Locking the groups in id order serializes overlapping recomputations and leaves disjoint ones concurrent; the ordering is what prevents deadlock when two writers' ancestor sets intersect in opposite orders. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Grouper spent three privileges on the all-users subject: viewers (discoverable), readers (members listable), and optins (self-join). The DE gave public communities read+optin and public teams view alone, so Grouper refused a self-join on a public team -- those use the join-request flow, where an administrator approves. Without optin recorded, every public team became directly joinable and the approval workflow was bypassed. members_public is not a usable proxy: the two coincide in current data but it records `readers`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
000057 serializes recomputations that share a group. It cannot fix the second race, because the two writers lock disjoint sets: one recomputes A and its ancestors, which do not yet include Q; the other attaches A under Q and recomputes Q from a snapshot that does not yet include the new member. Both commit and nothing recomputes Q again, so every permission granted to Q silently fails to reach that member. Reproduced deterministically: with the trigger removed the new parent ends up holding only the pre-existing member; with it in place it holds both. The two writers do share the group being attached, so locking it there gives them a meeting point -- the attach waits for an in-flight recomputation and then reads committed membership. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
000059 closed the attach race for additions only, and for an unstated reason: an INSERT's FK check takes a KEY SHARE lock on the group row that meets the attach trigger's FOR UPDATE. A DELETE performs no RI check and takes no lock, so removing a member while the group was being attached to a new parent left the removed user in the parent's closure permanently -- the over-permissive direction. One trigger function now takes the locks for every membership write: attaches lock the member group as before (and now refuse a group-typed member with no groups row instead of silently orphaning closure rows), and removals lock the group being detached from plus the member when it is a group. The trigger also moves to the trigger_* naming convention. Also corrects the 000057 comment, which overclaimed that id-ordered locking rules out deadlock (trigger and tuple locks sit outside that ordering; 40P01 remains possible and callers retry), fixes its inverted de-users example, restores a comment line 000056's down migration dropped, and replaces the group_effective_members table comment that still told the service to hand-collect containers before a group delete -- work the delete trigger has done itself since 000054. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Main claimed 000054 through 000056 for the job duration limit and the notifications move while this branch was using the same three numbers for group storage, so the group migrations are renumbered to 000057 through 000063. Applying two files with one version number is an error in golang-migrate, not a silent reordering, so the numbering had to move before the merged branch could run at all. Renumbering is safe here because nothing has applied these migrations outside development: the groups service that reads them is not deployed. The comments that name sibling migrations, and the group_data_source row that records which migration seeded it, are renumbered to match. The two halves do not overlap. Main's changes are in the public schema and this branch's are in the permissions schema; the only shared table is public.users, which subjects.user_id and notifications.user_id reference independently. Verified against a fresh PostgreSQL 18: all 63 migrations up, down -all, and up again, plus a functional check that nested-group closure, the detach-before-delete trigger, and the subject-to-user correlation still behave alongside the notifications tables. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017LHY69HajsmEFQeodU3CRB
000062 and 000063 made an attach and a concurrent membership change meet by
locking exactly the group whose membership row is written: inserting into
group_memberships takes an FK KEY SHARE lock on that group's row, and the
attach trigger takes FOR UPDATE on the group being nested. Those are the same
row only when the group being changed is the group being attached. One level
deeper they are different rows, neither writer waits, and the closure is
silently wrong.
With B nested in A and Q standalone: one transaction adds a member to B and
recomputes the ancestors it can see, {B, A}. Another attaches A under Q and
recomputes Q from a snapshot in which B does not yet hold the new member. Both
commit with no error; A and B list the member and Q never will. The removal
case is worse, because it fails open: the remover's ancestors are again {B, A},
so the removed user disappears from A and B but stays in Q, keeping every
permission Q grants.
The lock the recomputation needs is not on the groups it rebuilds -- those it
already locks -- but on the whole subtree it reads to rebuild them, which no
fixed set of row locks can name in advance. Every group-graph write now takes
one transaction-scoped advisory lock instead, so a recomputation's read of the
descendant subtree cannot overlap an attach or detach anywhere within it.
Group-graph writes are rare next to the permission reads that consume the
closure, so serializing them globally costs little, and it retires the deadlock
risk 000060 documented as an accepted cost.
The three functions are redefined in a new migration rather than edited in
place: 000057 through 000063 are already applied to QA and to local clusters,
so an edited file would never re-run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wjd21NTp4Ead7JYhx5sQUT
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
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.
Migration
000057, the storage half of moving DE group data out of Grouper andinto the
permissionsschema of the DE database. Groups sit beside the tablesthe permissions service owns so that expanding a subject to its groups becomes a
join inside the permission lookup rather than a call to another service.
No service depends on this yet — the
groupsservice reads it, but is notdeployed anywhere. It is safe to apply and roll back on its own.
What it adds
group_types— reference rows (collaborator_list,team,community,system) with anowner_requiredflag, so adding a kind is an INSERT ratherthan a migration.
groups— keyed on the internalsubjects.id, so a permission granted toa group expands to its members without translating between identifier spaces.
The external identifier stays
subjects.subject_id. Identity is structured(type / owner / short name) rather than Grouper's colon-delimited paths, and
legacy_namekeeps the Grouper path for provenance.group_memberships— direct membership. Members may be users or groups:production Grouper has 156 nested memberships, across collaborator lists,
teams, and communities, so nesting had to be preserved.
group_effective_members— the materialized transitive closure, restrictedto users. Permission lookup is the hottest query in the DE, so the expansion is
precomputed on write instead of recursing at read time.
group_ancestors()/recompute_group_closure()— one implementation ofthe closure, shared by the service and the delete trigger.
trigger_groups_detach_before_delete— see below.subjects.user_id— a nullable correlation topublic.users.Two changes to
subjectsworth attentionsubject_idwidens fromvarchar(64)tovarchar(512), matchingpublic.users.username. QA already has a 65-character username, and membershipnow requires every member to have a subject row, so that user would otherwise
fail a constraint they do not hit today.
subject_idalso gains a default ofreplace(uuid_generate_v1()::text,'-','').Groups imported from Grouper keep their original 32-hex identifiers so existing
permission grants and iRODS
@grouper-<id>names stay valid; groups createdafter the migration must be indistinguishable from them, or the DE ends up with
two identifier shapes and two iRODS group-name shapes.
The delete trigger is load-bearing
Deleting a nested group cascades its membership row away, which destroys the path
back to the containing groups before any AFTER trigger or calling service could
react — leaving those groups still granting access to the deleted group's
members. This was verified as a real leak before the fix. A
BEFORE DELETEtrigger detaches and recomputes at the one point the containers are still
findable, and covers every caller including a cascade from
subjects.subjects.user_idsubject_idis a bare username whilepublic.users.usernamecarries a domainsuffix, so joining the two means reconstructing the suffixed form everywhere.
It is nullable and best-effort: a subject can be created before the user has
ever logged in to the DE, and 5 of the 19,728 production user subjects correspond
to no DE user at all. A NULL means "not correlated", never "no such user".
ON DELETE SET NULLrather than CASCADE — removing a DE user must not delete thesubject, because that would cascade away their group memberships and every
permission granted to them. A CHECK stops a group subject from naming a user, and
a partial unique index stops two subjects claiming the same one.
It is deliberately not backfilled here: matching needs the username suffix,
which is deployment-specific. Population belongs to the services and the Grouper
importer, which have it configured. Match on the fully suffixed username rather
than the bare name — production
public.usersholds 128,110 rows of adoubled-suffix form (
user@@iplantcollaborative.org) which are near-inert (34jobs between all of them, against 1,477,921 for the 142,368 real rows) but would
make a bare-name match ambiguous for almost every user.
Compatibility
NULLS NOT DISTINCTis avoided in favour of acoalesce-based unique index,so the migration does not require PostgreSQL 15+.
UNIONrather thanUNION ALL, so a membership cycleterminates instead of recursing forever. The service also rejects cycles at
write time.
Verification
up/down/upagainst a fresh PostgreSQL 18 database with all precedingmigrations applied. The down migration restores
subject_idtovarchar(64),failing rather than truncating if a longer identifier exists — silently
discarding part of an identity is worse than refusing to roll back.
The constraints were exercised individually against a clean database, and the
delete-trigger behaviour is covered by integration tests in the
groupsservice(dropping the trigger produces exactly the two expected failures).
group_data_sourceThe importer reconciles — it removes memberships and grants Grouper no longer
has — which is correct only while Grouper is authoritative. Run it once after
cutover and it deletes whatever was created natively in the meantime.
A single-row table records which system owns group data, seeded to
grouper.The importer will refuse to start unless it says so. There is deliberately no
override flag: the only way to run a destructive reconcile afterwards is to set
this back, which is an attributed, timestamped write rather than something typed
at the end of an argument list. Flipping it to
nativeis an ordered step ofcutover, performed before anything is pointed at the new store.
A
BEFORE UPDATEtrigger maintainschanged_at, so the record of when cutoverhappened cannot be left stale — verified by confirming an explicit
changed_at = '2020-01-01'is overridden.Single-row-ness, the
sourcevocabulary, and a non-blankchanged_byare allenforced by constraints and were exercised individually against a live database.
🤖 Generated with Claude Code