feat(auth): groups and group-based route grants (#18 Phase 2) - #24
Merged
Conversation
Groups become a subject a route can be granted to, and a fact protected upstreams are told about. Schema: `Group` (unique normalized name, mirroring the User precedent) and `GroupMember`; `RouteAccessGrant.UserId` becomes nullable alongside a new `GroupId`, with a CHECK constraint making "exactly one subject" a schema fact rather than a handler convention, and the single unique index replaced by one partial index per subject kind. The SQLite table rebuild in `AddGroups` preserves existing grants (they keep their user id and get a null group). Policy: `IsAuthorizedAsync` and `AccessibleRouteIdsAsync` now share one `GrantAdmits` predicate — a grant matches when it names the account or a group the account is in — so all four call sites gain group semantics at once and the two entry points cannot drift. Membership folds into the grant query, keeping verify at one round trip. Forwarding: `Remote-Groups` and `X-Auth-Request-Groups` move from strip-only to strip-and-copy for their mode (names sorted ordinal, comma-joined, omitted when empty); the JWT gains an always-present `groups` claim and UserInfo answers `groups` as of now. Both channels are fed from one membership read per request, so an upstream cannot see two different answers in one request.
`groups.list/create/rename/delete/getMembers/setMembers`, every handler
`[RequireRole("Admin")]` — adding an account to a group grants it every route
that group is named on, so it is exactly as privileged as writing the grant.
Names are validated where they enter the system rather than escaped where they
leave it: trimmed, non-empty, at most 64 characters, printable ASCII and no
comma. The comma rule is load-bearing — group names travel comma-joined in the
forwarded group header, so a group called `viewers,admins` would hand its
members a role nobody granted.
`setMembers` is a whole-set replace with the `setAccess` reconciliation shape:
every id validated before any write, unchanged members untouched, audit row
after the commit. Four new audit kinds record the acts that can change access
without a route being touched.
`proxy.setAccess` takes an optional trailing `grantedGroupIds` (a client that
predates groups keeps today's behaviour rather than being rejected) and both
handlers report the two grant sets separately; leaving `Restricted` still
clears both.
…Info GroupsModuleTests covers the module through its real generated pipelines: the name charset (a comma, a control character and a non-ASCII name are all refused, because each would make the forwarded name mean something other than what was typed), case-insensitive uniqueness, whole-set membership reconciliation, the fail-whole validation, both cascade directions, and the Admin denial for all six handlers. RouteAccessPolicyGrantsTests extends the anti-drift estate to every grant shape — direct, via a group, via a group the account is not in, and both kinds on one route — and adds revocation by leaving the group, by deleting it, and the fail-closed reading of an unknown mode with only a group grant behind it. ProxyAccessModuleTests round-trips group grants, reconciles the two axes independently, and asserts an unknown group id is refused before any write. AccessVerifyTests covers it at the wire: a member of a granted group passes with headers, a non-member gets 403, revoking membership closes the route on the next request, group names arrive comma-joined and sorted under the mode's own name only, a JWT-only route still carries them in the assertion, and a smuggled `Remote-Groups: admins` is replaced by the real membership.
…ialog
New `groups` module gated `{ module: 'Groups', role: 'Admin' }` and slotted
right after Users in the sidebar — the two are read together, since a group is
only a set of those accounts. The page follows the Users conventions: DataList
with a mobile card, create/rename dialogs sharing one name form, ConfirmDialog
for delete, and a Members dialog using the same checkbox roster the Access
dialog uses, saving the whole set in one call.
The copy carries the two things an operator cannot see from the screen: the
group name is forwarded to protected apps (so a rename is visible to them, and
commas are refused), and membership is evaluated per request (so adding or
removing a member takes effect immediately, and deleting a group revokes what
it granted).
The route Access dialog gains a Groups roster beside the users one for
`Restricted`, submitted as `grantedGroupIds`; both are cleared client-side for
the modes they do not belong to, matching what the server does.
rpc-schema.json regenerated: six `groups.*` methods, and `grantedGroupIds`
optional on setAccess input, present on both access responses.
…er, tests RenameGroup read its member count after the commit with the request token, so a caller hanging up could drop the `group.renamed` audit row — the exact hole the CancellationToken.None discipline exists to close. The count moved above the commit; a rename cannot change membership, so it reads the same either way. The AddGroups Down() gets its first hand-edit: a `DELETE FROM route_access_grants WHERE group_id IS NOT NULL` ahead of the scaffolded rebuild. The v1 schema cannot represent a group grant, so a downgrade has to drop them — and without this the rebuild's `IFNULL(user_id, 0)` would silently rewrite every group grant into a grant for user 0, while the non-partial unique index it recreates would fail outright for a route holding two of them. Up() is untouched; the generated down SQL now starts with the DELETE. IdentityForwarding's group filter dropped comma-bearing names but not control/non-ASCII ones, contradicting its own comment: such a name would fail HeaderSafe on the *joined* value and cost the account every other group it is in. It now applies exactly the creation-time charset rule, so one bad row costs one group rather than all of them. Both kinds still ride the JWT intact. New RouteAccessGrantSchemaTests pins what three handler comments lean on: the CHECK rejects a grant naming both subjects or neither, the partial unique index rejects a duplicate (route, group), and several group grants coexist on one route despite their shared null user_id. Group-header formatting gains unit coverage for the sort, the absent-when-empty rule, and per-name dropping. Three near-identical group seeders collapse into a new Application.Tests/AccessTestEstate (the counterpart of the Api.Tests one), which also sets a real CreatedAt — as does the Api.Tests seeder now.
design.md and README.md still said groups were Phase 2 while the new code cites them as authority. Surgical updates only: - design.md §3 gains the real `Group`/`GroupMember` sketches and the actual `RouteAccessGrant` shape (nullable UserId + GroupId under a CHECK), plus a paragraph on why uniqueness is two partial indexes and why membership is resolved per request rather than materialised into grant rows. - §2.3 documents the always-present `groups` JWT claim (empty array when the account is in none — "no memberships" has to be distinguishable from "not answered") and the per-mode ecosystem group header: sorted, comma-joined, omitted when empty. - The status line, §2.8 staging, §7 modules and §8 frontend now say what is actually built; OIDC, TOTP and template inheritance stay listed as remaining. - README's per-mode header table gains the group headers with the encoding and the omitted-when-empty rule, UserInfo now lists `groups`, and the limitations section swaps "no groups yet" for the real remaining gap — group membership is Watchtower's own directory, and tenant routes do not inherit policy.
- design.md §10's milestone list still had "groups + group grants" as Phase 2 future work; it now gets the same strikethrough/(done) treatment as §2.8 and the status line. Swept the rest of both docs — no other stale pairing left. - Dropped the dead `Password` const from GroupsModuleTests (its only consumer was the SeedUserAsync that moved to AccessTestEstate) and the now-unused Identity using from RouteAccessPolicyGrantsTests. GroupsModuleTests keeps its Identity using — the delete-cascade test still resolves a UserManager. - Removed a stray double blank line inside the §3 data-model fence, where every other entity is separated by one.
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.
Delivers groups + group-based grants from #18 (Phase 2 item 2): route access grants can now name a user or a group, membership feeds policy evaluation and identity forwarding, and admins get a Groups management surface.
What changed
Schema — new
Group(unique normalized name) andGroupMemberentities;RouteAccessGrantextended in place per the design doc's v2 note:UserIdnullable +GroupId, a CHECK constraint enforcing exactly one subject, and two partial unique indexes replacing the old(RouteId, UserId)one. MigrationAddGroupsis data-preserving for existing grants; itsDown()carries the repo's first hand-edited migration line (deleting group grants before the rebuild — the v1 schema cannot represent them, and the scaffolded rollback would otherwise corrupt them into user-0 grants).Policy —
IsAuthorizedAsync/AccessibleRouteIdsAsynckeep their signatures; a sharedGrantAdmitsexpression folds group membership into the grant query itself, so all four call sites (verify, callback, login dance, tenant discovery) gained group semantics in one round trip and the two entry points structurally cannot drift.Forwarding — deliberately no bespoke
X-Watchtower-Groupsheader, despite the issue's shorthand: the codebase's stated contract (IdentityForwarding remarks, central-auth README) forwards via the two adopted ecosystem vocabularies.Remotemode emitsRemote-Groups,AuthRequestemitsX-Auth-Request-Groups— comma-joined, sorted ordinal, omitted when empty, both names strip-and-copy per the superset invariant. The signed JWT always carries agroupsclaim (empty array = "in no group", the fail-closed reading), and UserInfo now answersgroups. One membership read per verified request feeds both channels. Group names are validated to comma-free printable ASCII at creation so they are header-safe by construction, with a symmetric defense-in-depth filter at format time.Groups module —
groups.list/create/rename/delete/getMembers/setMembers, all admin-only, with audit events (group.*kinds) written post-commit.proxy.getAccess/setAccessgaingrantedGroupIdsas a non-breaking trailing optional; validation is fail-fast before any write and reconciliation is per subject kind.Frontend — new Groups admin page (gated
{ module: 'Groups', role: 'Admin' }) with a members roster dialog, and a Groups roster in the route Access dialog forRestrictedmode.Docs —
docs/central-auth/design.mdand README updated to describe groups as shipped (data model, JWT claims, per-mode header tables), since the new code cites them as authority.Review notes
RenameGrouppost-commit cancellation window, the migration rollback corruption, and schema-constraint test coverage) followed by a final architecture gate.rpc-schema.jsonre-export produces no diff; frontend codegen + typecheck + build clean.Closes nothing on its own — #18 stays open for the remaining Phase 2 items.