Groups service client, behind the terrain.groups.backend toggle - #333
Open
johnworth wants to merge 24 commits into
Open
Groups service client, behind the terrain.groups.backend toggle#333johnworth wants to merge 24 commits into
johnworth wants to merge 24 commits into
Conversation
Introduce terrain.clients.groups, a client for the new Keycloak-backed Groups service that will replace iplant-groups. This first increment implements the subject family: find-subjects, lookup-subject(s), lookup-subject-add-empty, format-like-trellis, and list-groups-for-user. The client synthesizes the contract-required :type and :id_index fields that the new service does not return, so responses continue to validate against the existing group schemas. Add terrain.groups.base-url, terrain.groups.admin-user, and a terrain.groups.backend switch to support a config-gated cutover from iplant-groups. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Complete the Groups client's collaborator-list operations (list, add, get, update, delete, and membership management) plus remove-de-user, using name<->UUID resolution over the flat Groups service and the de:users:<user>:collaborator-lists:<short> naming convention. Membership results are enriched with source_id/subject_name from a bulk subject lookup so responses satisfy the existing group schemas. Introduce terrain.clients.grouping (and a lightweight terrain.clients.grouping.subjects to avoid the user-attributes dependency cycle) that dispatch to either backend based on terrain.groups.backend, and route the collaborator, subject, user-info, QMS, permanent-id-request, and app-metadata callers through the facades. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The search used to resolve a collaborator list's name already returns the full group, so return it directly instead of following up with a lookup by UUID. A test with only the search route registered locks in the single round trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Groups service now returns source_id and subject_name with each membership-change result, so drop the per-write bulk subject lookup and simply normalize a blank source_id/subject_name to satisfy the group membership schema. This removes an N-user Keycloak resolution pass from every collaborator-list membership change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implement team operations in terrain.clients.groups against the new Groups service: CRUD, membership, join/leave, privileges, and admins. Teams are stored as groups named de:teams:<creator>:<short> (external name <creator>:<short>). Public/joinable teams are modeled by granting the well-known all-users subject (GrouperAll, matching Sonora's grouper.allUsers config) read on the team, surfaced as the `view` privilege. Group-management privileges are recorded in the permissions service and translated to the DE privilege vocabulary: own/admin -> admin, write/read -> read, public -> view. Self join/leave are performed as the administrative user, gated on the team being public. Dispatch the team operations through terrain.clients.grouping and point terrain.services.teams at the facade. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implement community operations in terrain.clients.groups against the new Groups service: listing (with per-user membership), admin listing, CRUD, members, admins (get/add/remove), and join/leave. Communities are stored as groups named de:communities:<short>; public communities grant the all-users subject read. Community admins hold the admin privilege and are also members. The listing's :privileges is left empty on purpose, since the DE UI derives admin/follower status from the /admins and /members endpoints and computing per-community privileges would cost one request per community. App-tag retagging on community rename needs the apps and metadata clients, which would create a dependency cycle inside terrain.clients.groups, so it lives in a cycle-safe helper (terrain.clients.grouping.retag) invoked by the facade's update-community for the new backend. Retagging matches only tags stored in the new de:communities:<name> format; migrating legacy tag values is a separate data concern. Add the community operations to terrain.clients.grouping and point terrain.services.communities at the facade. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Groups client was written against the abandoned Keycloak-backed service, which was flat and addressed groups by UUID, so terrain encoded the DE's hierarchy in the group name and resolved that name by searching and filtering. The Postgres-backed service models type, owner, and short name directly, so all of that goes away: strip-folder, the six folder/full-name builders, find-group-by-name, search-groups, community-names-under, user-community-names, and the three per-entity formatters. Groups are now resolved through GET /groups/lookup by structured identity, and listings push their filters (owner, member, search) to the service rather than fetching a folder and filtering in terrain. The external contract is composed in exactly one place -- format-group, which is also the only code that knows a team's external name embeds its owner. It now builds the contract shape explicitly rather than passing the service's group through, so the structured fields do not leak. Two live bugs disappear with the packing layer: - get-teams filtered creators by string prefix, so `bob` also matched `bobby`'s teams. The creator is now an exact owner match in SQL. - update-team rebuilt the packed name and re-split it on the first colon, so a team whose short name contained a colon did not round trip. The owner is a separate field and is never re-packed. Deletes now report the group resolved beforehand, because the service returns no body for a delete and callers need the id to clean up permissions. Also drops app retagging on community rename. A tag will name its community by ID, so a rename has nothing to rewrite and nothing to block; the retag-apps and force-rename flags still drive the legacy backend and are accepted and ignored on this one.
terrain forwards these bodies verbatim, so its part is the schema: accepting AppCommunityListRequest lets the community_ids form through instead of rejecting it during coercion. Adds the matching DELETE /apps/:app-id/communities/:community-id passthrough. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
johnworth
marked this pull request as ready for review
August 4, 2026 23:04
public_privileges carries Grouper's vocabulary: `view` makes a group discoverable and joinable, `read` also exposes its members. Communities send both; public teams send only `view`. Collapsing them to a single grant published every public team's membership. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The legacy client rejected adding, removing, joining, or leaving with the Grouper admin user; the rewrite dropped all four. The new client filters that account out of every listing, so admitting it creates a member the UI can neither show nor remove. Also records what I could not settle here: Grouper granted the all-users subject `optin` on groups that could be joined directly, and only `view` on public teams -- I confirmed against the running Grouper that it refuses a self-join on a view-only team and allows one on an optin community. Nothing in the new schema carries `optin`, so join-team is more permissive than what the DE used. Left as a documented divergence rather than guessed at: `members_public` happens to coincide with it in current data but means something else, and conflating them would be wrong the first time they diverge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Grouper granted the all-users subject `optin` on groups a user could add themselves to, and only `view` on public teams -- it refused a self-join against those, and the DE routes them through the join-request flow where an administrator approves. Gating on the public marker alone let anyone join any public team directly, bypassing that approval. joinable is set at creation from `optin` in public_privileges, the same way members_public is set from `read`. public-group? had no other caller and is gone: joinability is a property of the group now, not something inferred from its permission list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The DE granted every member `optout` and `read` together, so Grouper's leave revoked both and an ex-member kept nothing. The importer turns that `read` into an explicit grant -- 1,579 of them in production -- which removing membership does not touch, so an ex-member kept read access to the group and its member list. Only `read` is revoked. Grouper's leave revoked the member privileges alone, so someone holding `admin` who left kept administering the group; revoking whatever level is present would silently demote owners. The revoke runs before the removal: if the removal then fails the caller is still a member, which is consistent, where the reverse order leaves them out of the group and still holding read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
join-team, leave-team, join-community, and leave-community never guarded against the acting user being the Groups administrative account. Join is especially dangerous: the membership change is performed AS the admin account, whose service-side permission bypass means nothing downstream would refuse it, and the resulting membership is filtered out of every listing so the UI could neither show nor remove it. Guard all four up front, matching the add/remove member guards, and cover the previously untested remove-community-admins guard as well. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
leave-team and leave-community revoked the member's read grant before removing the membership itself. A removal that failed partway then left the worst of both states: the user was still a member, but with the read grant already gone. Since side effects outside a transaction do not roll back, order the steps to favor the recoverable state: remove membership first, then revoke read -- a failed revoke merely leaves an ex-member with read, which a retry cleans up. The leave tests now pin the request order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The community admin endpoints and the fake-auth user lookup passed (config/grouper-user) unconditionally, but on the groups backend the administrative account is (config/groups-admin-user); the two only agree because both default to de_grouper, so overriding either config value would silently act as the wrong account. Route both call sites through the backend-aware admin-user helper in terrain.clients.grouping.subjects (now public). The legacy iplant-groups client is the only remaining grouper-user call site, which is correct: it can only ever act against Grouper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three weaknesses let regressions slip through: - The delete tests matched any verb on the group URL, so a client that issued GET or PUT instead of DELETE still passed. Register the routes as method maps so only DELETE matches. - update-team-privileges-test funneled the revoke through a catch-all handler that asserted nothing, so dropping the revoke entirely went unnoticed. Grant and revoke are now captured and asserted separately. - The membership add and bulk subject lookup bodies were never inspected, so renaming the :members or :subject_ids wire fields would not fail any test. Capture and assert both request bodies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
list-groups sent no limit or offset, and the Groups service caps one listing response at 1000 groups -- so any listing past that bound was silently truncated. Request explicit 1000-group pages and advance the offset until a short page arrives. All listing callers go through the one private fn, so teams, collaborator lists, and communities all page. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The client dropped the details flag on every listing, so Sonora's Teams listing Creator column -- bound to detail.created_by_detail.name -- came up blank on the new backend. When details are requested on a team, collaborator-list, or user-group listing, each owned group now gets a :detail block built from the raw group's owner: created_by, a created_by_detail subject whose name falls back to the bare owner id, created_at in ms since the epoch, and the has_composite / is_composite_factor flags the schema requires (always false; the new backend has no composites). The distinct owners of a whole listing are resolved with a single bulk subject lookup. Communities carry no owner, so their listings deliberately attach no :detail. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
find-group catches only 404, but the Groups service responds 403 when a group exists and the caller cannot read it. Spell that out where nil triggers a create, so future callers do not assume nil covers every inaccessible group. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The branch built against 3.4.22-SNAPSHOT while the release was pending; the real release is now on Clojars, so nothing mutable is left in the dependency tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # project.clj
Validate terrain.groups.backend at startup. The selector was read with get-optional-prop and compared for equality at each dispatch site, so an unrecognized or explicitly-empty value silently selected the legacy backend: a cutover shipping "Groups" or a templating miss would have served 200s while reading and writing Grouper for the whole maintenance window. The accepted values now live next to the property and an unknown one fails validate-config. The docstring also records that the groups backend is only safe alongside the paired apps and Sonora images, since community app tags are named by the apps image rather than by terrain. Default a blank subject name to the subject id in format-subject. The member and subject listing schemas require a non-blank display_name and response coercion is live, so one nameless member would have failed the whole listing. Give every Groups request a socket and connection timeout, following the info-typer client. Without them a service that accepts connections but stops responding pins Jetty workers on paths that reach well past group management. Share the toggle predicate and the subject helpers. new-backend? now lives only in terrain.clients.grouping.subjects, and the identical format-like-trellis, empty-user-info, and lookup-and-swallow bodies move to a new terrain.clients.subject-info that both group clients require. Skip the bulk subject lookup when no ids were collected, in list-privileges and admins-of as attach-details already did. Rewrite a team's members_public and joinable flags when the public subject's privileges change. Those flags were only ever set at creation, so a team made public after the fact kept its member list hidden, and one stripped of public privileges stayed readable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjd21NTp4Ead7JYhx5sQUT
GET /communities returned an empty :privileges for every community, including ones the caller administers, while CommunityListingEntry documents the field as "the privileges the authenticated user has for the community". Anything deciding whether to offer community-admin affordances from that field lost them for every user on the new backend. The groups service now answers the whole listing in one request: GET /subjects/:subject-id/permissions reports the level held on every group, memberships inherited through nesting included. The listing already fetches the caller's memberships once rather than once per community; privileges are fetched the same way. One level becomes one privilege name, where Grouper could report several at once. That is the collapse privileges->level already performs in the other direction, when a privilege is granted. 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
2 participants
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.
Terrain's half of the move off Grouper. Adds a client for the new Groups
service and a dispatch facade selecting between it and iplant-groups, so the
backend is a config flip rather than a deploy.
Nothing changes at runtime.
terrain.groups.backenddefaults toiplant-groups, and that path is untouched —terrain.clients.iplant-groupsis not modified by this branch.
What's here
terrain.clients.groups— the client. Groups are addressed by structuredidentity (type, owner, short name) through
GET /groups/lookup, and listingspush their
owner/member/searchfilters to the service rather thanfetching a folder and filtering in terrain.
terrain.clients.grouping— the dispatch facade, andgrouping.subjects,which exists separately so
terrain.auth.user-attributescan depend onsubject lookups without a dependency cycle.
terrain.groups.{base-url,admin-user,backend}config.The external HTTP contract is unchanged. It is composed in exactly one place —
format-group— which builds the contract shape explicitly rather than passingthe service's group through, so the service's structured fields cannot leak
into a response whose schema is closed.
Two bugs fixed
Both fall out of dropping the name-packing layer, and both are verified by
mutation — reverting either behavior fails the suite.
get-teamsfiltered creators by string prefix, sobobalso matchedbobby's teams. It is now an exactownermatch in SQL.update-teamrebuilt a packed name and re-split it on the first colon, so ateam whose short name contained a colon did not round-trip. The owner is a
separate field and is never re-packed.
One behavior change
Team search narrowed. Grouper searched the full path, so typing a creator's
name matched their teams; the service searches short name and description only.
Reproducing the old behavior would mean fetching all teams and filtering in
terrain, which is the pattern this removes.
Merge ordering
Two things must happen before this can be flipped on anywhere:
Phase 6a (community app tags) — now implemented, and this branch carries
terrain's half of it. This branch deletes community rename-blocking and the
retag helper on the new backend, on the premise that a tag names its community
by ID. That premise now holds:
appsstores the community ID and resolvesnames and legacy colon paths to it (Read groups from the groups service, and tag apps with community IDs apps#303), Sonora sends IDs
(Identify collections by ID when tagging apps sonora#723), and a one-off command rewrites the existing tags
(Deploy the groups service, the Grouper importer, and the apps/group-propagator cutover config deployments#86). terrain's part is the schema — these routes
forward the body verbatim, so accepting
AppCommunityListRequestis what letsthe new shape through instead of rejecting it during coercion — plus the
matching
DELETE /apps/:app-id/communities/:community-idpassthrough.What remains is ordering, not design: this and
appsare pinned to thereleased
common-swagger-api3.4.22(Add a community-list request schema for app community tags common-swagger-api#101,released to Clojars), and the tag rewrite has to run before the toggle is
flipped.
Exposure is still zero in the meantime, because the toggle defaults to
iplant-groups, whose path retags and blocks as before.appsandgroup-propagatormust migrate in the same window. They stillread Grouper, so group writes made through terrain would be invisible to them
and iRODS ACLs would drift.
The
retag-appsandforce-renamequery parameters are deliberately kept: theystill drive the legacy backend, and Sonora sends
retag-appstoday. They areaccepted and ignored on the new backend.
Also outstanding: golden fixtures of the current QA responses. The contract is
"must not change" and the tests here assert terrain's shape and request pattern,
but nothing yet proves the new responses match what QA serves for real Grouper
data.
🤖 Generated with Claude Code