Repository navigation
feat(api): sign in a ZITADEL-only principal (#377 PR-B 2b) - #600
Conversation
In principal mode a ZITADEL identity with no single GitHub login now signs in as its own principal (provisioned on first sign-in) instead of being sent to the link page: the token's sub and pid are the principal id and it carries no groups. Tenant access then comes only from a GitOps member entry naming that principalId (proposal Q5, acceptance criterion 4). Outside principal mode nothing changes. The link store and the principal resolver must agree on the principal; when they do not, the sign-in is a 503 rather than a guess. The end-to-end test covers B6 on the API side: sign-in, token, middleware and role check give a ZITADEL-only member developer on its tenant, removing the entry removes it on the same token, and an unlinked identity with the same email gets nothing. Claude-Session: https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE
The sign-in table gets the principal-mode answers. /whoami reports admins in namespaces but never in groups for a principal-mode admin, so clients must read isAdmin (deferred P3 from #595). Claude-Session: https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE
|
Claude finished @mashkovd's task in 4m 7s —— View job Review complete — PR #600
Reviewed Findings: 0 P1, 0 P2, 1 P3 (posted inline on
What I checked and found sound:
Not run: Posted: 1 inline P3 comment, and a verdict of APPROVE bound to commit |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| login, principalID = gh.Display, p.ID | ||
| case h.opts.TenantAuthzMode == TenantAuthzPrincipal && zitadelOnly(p, err): | ||
| // Principal mode needs no GitHub login (mctl-api#377, Q5): the | ||
| // identity signs in as its principal, provisioned on first sight, |
There was a problem hiding this comment.
P3: this branch provisions a principal on first sight, which makes the file-level comment at line 40 ("Nothing is linked or provisioned here") stale — worth a one-line amendment so it doesn't contradict the behavior this PR adds in principal mode.
There was a problem hiding this comment.
Fixed in c90fe4f: the comment now says nothing is linked there, and that only principal mode provisions (the resolver does it on first sight). The same push carries the deferred #598 P3s: the token-principal claim is confined to the local OAuth provider, and the switch-criterion and Metrics docs are completed.
Review P3s from #598 and #600. checkProviderContract now refuses a token-principal claim from any provider but the local OAuth one, symmetric with the reserved principal namespace. The switch criterion gains the zero-mismatch condition and the pid-coverage check, and the Metrics table lists the new counter and sign-in results. Two stale or misplaced comments are corrected. Claude-Session: https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE
|
Claude finished @mashkovd's task in 6m 46s —— View job Review complete — PR #600
Reviewed the pinned range Findings: 0 P1, 0 P2, 3 P3 — approved, bound to P3s (non-blocking, all posted inline)
What I checked and found sound
Not run
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| an unresolved principal is 503. | ||
| - a platform-skill role binding on `admins` means platform admin, never a | ||
| tenant named `admins`. | ||
| - `/whoami` reports `admins` in `namespaces` for a platform admin, but |
There was a problem hiding this comment.
P3: this newly written /whoami contract covers admins/isAdmin but omits the one field this slice changes: for a ZITADEL-only principal the token has no login, so ValidateJWT builds User{ID: pid} and /whoami answers "id": "prn_01J..." instead of a GitHub login (internal/api/handlers_read.go:69). Any client that renders id as a GitHub handle (or keys local state on it) will see a prn_ there the first time a ZITADEL-only member signs in. Worth one line here, since /whoami is the contract clients read.
There was a problem hiding this comment.
Fixed in d58da55: the whoami contract in docs/principals.md now states that id is the prn_… for a ZITADEL-only principal and must not be treated as a GitHub login.
| // access tokens would be refused anyway, and re-deriving the principal from | ||
| // the login is what principal mode removes. The holder signs in again. The | ||
| // error reads as an invalid grant, which is what tells a client to do that. | ||
| func (s *OAuthServer) refreshablePrincipal(principalID string) error { |
There was a problem hiding this comment.
P3: this guards the pid-less session but not the login-less one. If principal mode is ever rolled back off (the mode is a config flag, and 2d explicitly plans a switch), a ZITADEL-only session still holds a valid refresh token with Login == "" and a good PrincipalID. RefreshAccessToken then succeeds, IssueJWT("", pid) mints sub = pid, and ValidateJWT (now PrincipalMode == false) refuses that very token with "this token is for a principal without a GitHub login, which is accepted only when tenant authorization is by principal" — a 401 loop rather than the invalid_grant that would tell the client to sign in again. TestRefresh_CarriesThePrincipal already drives this exact combination ({"", pzZitadel} with principalMode=false) but only asserts payload.PID, so nothing pins the outcome. Refusing a login-less session outside principal mode, next to the pid-less one, would make the rollback symmetric with the switch.
There was a problem hiding this comment.
Fixed in d58da55: refreshablePrincipal(login, principalID) now also refuses a login-less session outside principal mode (invalid_grant, so the client signs in again). TestRefresh_CarriesThePrincipal asserts the refusal for {"", pzZitadel} with principalMode=false; mutation-verified (dropping the clause fails it).
| prn, err := h.opts.Principals.ResolvePrincipal(r.Context(), id) | ||
| switch { | ||
| case err == nil: | ||
| return prn, "" |
There was a problem hiding this comment.
P3: (prn == "", err == nil) is taken as success. oauth_upstream.go handles the mirror-image store contract violation explicitly ("A refusal that names no principal where it must is a failed read, never this", with a test case for it), but a resolver that answers an empty principal and no error slips through here: in principal mode on the ZITADEL path principalID stays "", the disagreement guard is skipped (p == nil on ErrNotFound), IssueCode accepts "", and the failure only surfaces at /oauth/token as a 500 from IssueJWT ("neither a login nor a principal") after the code was already handed to the client. A prn == "" check in this branch would turn it into the documented 503 principal_unavailable at sign-in, symmetric with how the store’s equivalent violation is treated.
There was a problem hiding this comment.
Fixed in d58da55: a nil error with an empty or malformed principal id is treated as a failed lookup, so in principal mode it is 503 principal_unavailable. New TestSigninPrincipal cases (empty answer in both modes, malformed answer in principal mode); mutation-verified.
There was a problem hiding this comment.
No P1/P2 findings (3 P3). Good to merge.
The principal binding is fail-closed everywhere I could follow it: checkTokenPrincipal clears principalID on any check failure, PrincipalAuthz.Apply then answers 503 rather than "member of nothing", a nil resolver is a nil interface so the PrincipalChecker assertion fails rather than panicking, and checkProviderContract confines both the principal namespace and the TokenPrincipal claim to the local-OAuth provider. prn_ cannot collide with a GitHub login, so sub stays unambiguous across modes and older builds.
P3s (all non-blocking): the new /whoami contract omits that id becomes a prn_ for a ZITADEL-only principal; refreshablePrincipal guards the pid-less session but not the login-less one, so a rollback out of principal mode refreshes ZITADEL-only sessions into tokens ValidateJWT always refuses; and signinPrincipal takes (\"\", nil) from the resolver as success, which surfaces as a 500 at /oauth/token instead of the documented 503.
Not run: go build/go vet/go test/gofmt — every go/gofmt invocation required approval in this sandbox and none was granted, so this is static reading plus cross-checking the called contracts (Store.LinkedGitHub, Store.Get, Resolver.Forget, PrincipalAuthz.Apply, attachPrincipal). Please allow go test, go vet and gofmt in this job's --allowedTools; this is the second run in a row that could not compile the diff.
The merge-base changed after approval.
Review round on #600: - refreshablePrincipal also refuses a session with no login when not in principal mode. ValidateJWT refuses its access tokens there, so a refresh after a rollback from principal mode would only mint tokens that answer 401 and leave the client looping instead of signing in. - signinPrincipal treats a resolver answering no error but no valid principal id as a failed lookup (503 principal_unavailable in principal mode) rather than going on with an empty principal. - docs: /whoami id is the prn for a ZITADEL-only principal. - lint: mark the test fixture principal id as not a credential (G101). Claude-Session: https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE
|
Claude finished @mashkovd's task in 5m 36s —— View job Review complete — PR #600
Reviewed the pinned range Findings: 0 P1, 0 P2, 2 P3 — approved, bound to The fixes in
|
|
|
||
| Over the same window, `oauth_token_principal_mismatch_total` must stay at | ||
| zero. Before the flip, check pid coverage too: a live session minted | ||
| before the release that records `pid` is refused at the switch and its |
There was a problem hiding this comment.
P3: the sentence is inverted — it should be a session that records no pid. As written ("a live session minted before the release that records `pid` is refused at the switch") it says the opposite of the behaviour, and of the paragraph 50 lines above ("sessions minted before this release carry no `pid`, so their holders sign in again") and of ValidateJWT, which refuses exactly the token whose pid is absent. This is a switch-criterion checklist an operator reads before flipping TENANT_AUTHZ_MODE, so the inversion is worth fixing even though the following sentence ("count the live refresh rows with an empty `principal_id`") makes the intent recoverable.
| - In `principal` mode a token without a `pid` is refused (401), a | ||
| principal-only token must have `sub == pid`, and a refresh of a session | ||
| with no recorded principal answers `invalid_grant`. Outside it, a | ||
| principal-only token is refused. |
There was a problem hiding this comment.
P3: this bullet is the mode-by-mode token contract, and it now carries only half of the symmetry refreshablePrincipal gained in d58da55. Principal mode gets both its rules written down (a pid-less token is 401, a pid-less session refresh is invalid_grant), but outside it only the access-token rule is ("a principal-only token is refused") — the matching refresh rule, that a login-less session is no longer refreshable outside principal mode either, is not. That is the rollback-day counterpart of the switch-day paragraph just below, with the same observable consequence (rotate-then-refuse, so a retry past the grace window trips reuse detection and logs oauth refresh token reuse detected; family revoked for ZITADEL-only sessions rather than stolen tokens). One clause here plus a sentence in that paragraph would make a rollback as pre-documented as the switch already is.
There was a problem hiding this comment.
Fixed in 901fa08: the contract bullet now states the outside-principal-mode refresh rule (login-less session → invalid_grant), and the switch-day paragraph notes its rollback-day twin, including the same reuse-detection warnings.
There was a problem hiding this comment.
No P1/P2 findings (2 P3). Good to merge.
Both P3s from the previous round are addressed in d58da55 and the fixes hold up: refreshablePrincipal now refuses the login-less session outside principal mode (both the store and the in-memory path, with TestRefresh_CarriesThePrincipal driving each), and signinPrincipal rejects a resolver answer that is not a principal id, turning the former 500-at-/oauth/token into the documented 503 at sign-in. Every IssueJWT/IssueCode/IssueRefreshToken and refreshstore call site is threaded through; PrincipalMode and TenantAuthzMode both derive from cfg.tenantAuthzMode() at the one construction site, so the two cannot disagree. The pid claim stays confined to *localOAuthProvider (checkProviderContract plus ProviderPrincipal in reservedProviderNames), and no login-less refresh session can exist outside principal mode in the first place, so the new refusal cannot strand a legitimate client.
The two P3s are documentation: the switch-criterion sentence at docs/principals.md:442 is inverted ("records `pid`" should be "records no `pid`"), and the mode-by-mode contract at :387 writes down the principal-mode refresh refusal but not its new rollback-day twin.
Not run: go build/go vet/go test/gofmt — every go and gofmt invocation required approval in this sandbox and none was granted, so this is static reading plus cross-checking the called contracts (Store.LinkedGitHub, refreshstore.Store, Resolver.resolve/Forget, the /oauth/token error mapping). This is the third run that could not compile the diff; please allow go test, go vet and gofmt in this job's --allowedTools.
The merge-base changed after approval.
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Findings[P3] Missing test coverage for
|
|
agy P3 on d58da55 (no test for |
|
Claude finished @mashkovd's task in 5m 44s —— View job Review complete — PR #600
Reviewed the pinned range Findings: 0 P1, 0 P2, 1 P3 — approved, bound to The docs fixes land
The one P3 (posted inline,
|
|
|
||
| Over the same window, `oauth_token_principal_mismatch_total` must stay at | ||
| zero. Before the flip, check pid coverage too: a live session minted | ||
| before the release that records no `pid` is refused at the switch and its |
There was a problem hiding this comment.
P3: this sentence is still ambiguous, and my previous round's finding on it was itself a mis-parse — worth saying so that the wording lands right rather than oscillating.
The clause that records no pid`` sits next to the release, so the sentence parses two ways:
- A:
a live session minted before [the release that records no pid]— nonsense, since the release is the one that does record it. - B:
a live session ... that records no pid— the intended meaning.
The previous wording (the release that records pid``) was correct under parse A and wrong under parse B; this one is the mirror image. Hoisting the clause removes the ambiguity in either direction:
a live session that records no
pid— one minted before this release — is refused at the switch and its holder must sign in again.
Non-blocking, but this paragraph is the checklist an operator reads before flipping TENANT_AUTHZ_MODE, and the following sentence (count the live refresh rows with an empty principal_id) is the only thing that currently disambiguates it.
There was a problem hiding this comment.
No P1/P2 findings (1 P3). Good to merge.
The head commit 901fa08 is docs-only and lands both P3s from the previous round: the mode-by-mode token contract now carries the rollback-day refresh rule alongside the switch-day one, and the switch-criterion paragraph spells out that a rollback does to ZITADEL-only sessions what the switch does to pre-#377 ones. The remaining P3 is that the pid-coverage sentence is still ambiguous in the mirror direction — my own prior finding on that line was a mis-parse, which I say so inline.
Re-verified the code this round rather than resting on the earlier passes. refreshablePrincipal(login, principalID) guards both directions on both refresh paths; TestRefresh_CarriesThePrincipal (internal/auth/token_principal_test.go:243) drives store x principalMode and asserts the refusal for the login-less session outside principal mode and the pid-less one inside it — so the informational P3 above claiming no test covers RefreshAccessToken is mistaken; it looked only at oauth_server_test.go. No legitimate caller is stranded: IssueCode is reached with an empty login only under TenantAuthzPrincipal, so a login-less refresh row exists outside the mode only after a rollback.
Also confirmed directly: LinkedGitHub (internal/principals/signin.go:104-120) returns ErrIdentityRefused/ErrPrincipalDisabled with p == nil and ahead of the three no-single-login sentinels, so zitadelOnly matches its contract exactly and a disabled principal still 403s on the linked path. The new ProviderPrincipal identity shape never reaches provisioning: all three other User.Identity() consumers (principal_authz.go:100, tenant_role.go:111, tenant_authz_shadow.go:130) read only ok and Kind, and AttachPrincipal short-circuits to checkTokenPrincipal before ResolvePrincipal. checkProviderContract confines both the principal namespace and Claims.TokenPrincipal to *localOAuthProvider, and ValidPrincipalID is prn_ plus 26 Crockford base32, which no GitHub login can match. Every IssueCode/IssueJWT/IssueRefreshToken and refreshstore call site is threaded through, the two mode flags both derive from cfg.tenantAuthzMode() at the one construction site, the cachedLookup refactor preserves the mutex/gen/refusal-never-cached discipline, and the sign-in metric is single-counted on both callbacks. The store-path rotate-then-refuse consequence is written down rather than left to be discovered.
Not run: go build, go vet, go test, gofmt — every go and gofmt invocation required approval in this sandbox and none was granted, so this is static reading plus cross-checking the called contracts. This is the fourth run in a row that could not compile the diff; please allow go test, go vet and gofmt in this job's --allowedTools.
The merge-base changed after approval.
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit FindingsFinding 1
|
|
Remaining P3s on 901fa08 deferred to slice 2d (gate is 0 P1/P2): the switch-criterion sentence wording at docs/principals.md:446 (reword to put "that records no |
|
Claude finished @mashkovd's task in 6m 9s —— View job Review complete — PR #600
Reviewed Findings: 0 P1, 0 P2, 1 P3 — approved, bound to The earlier rounds' P3s are answered, and the fixes hold up
The one P3 (posted inline,
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| // The GitHub callback's tail, for the linked principal's GitHub | ||
| // login: same groups, same admin decision, same code. The principal | ||
| // is the one the store just found the link on. | ||
| login, principalID = gh.Display, p.ID |
There was a problem hiding this comment.
P3: the linked path is now the only sign-in that can mint a pid-less token in principal mode.
signinPrincipal was just hardened so a resolver answering ("", nil) becomes a 503 at sign-in rather than a broken token (oauth_signin_principal.go:59). This line takes the mirror-image answer from the other boundary — Store.LinkedGitHub, an interface in OAuthZitadelOptions — on trust: if it returns err == nil with a p whose ID is empty, principalID stays "", and in principal mode nothing downstream catches it. IssueCode only rejects a non-empty malformed id (oauth_server.go:1346), and IssueJWT only errors when both login and principal are empty (:1460) — here the login is non-empty, so the exchange succeeds and mints a token with no pid, which ValidateJWT then refuses on every request with ErrTokenWithoutPrincipal (401). The holder gets a code, a successful token exchange, and a session that never works.
The zitadelOnly branch already states this rule in a comment ("A refusal that names no principal where it must is a failed read, never this") and TestZitadelSigninRefusalWithoutAPrincipalIsAFailedRead pins it for the refusal side; the success side has no equivalent. Folding it in would be one clause:
case err == nil && (h.opts.TenantAuthzMode != TenantAuthzPrincipal || auth.ValidPrincipalID(p.ID)):
login, principalID = gh.Display, p.IDwhich sends a contract violation to refuseZitadelSignin → 503 store_unavailable, the same answer the resolver’s version now gets. DB-sourced in practice, hence P3 rather than P2.
There was a problem hiding this comment.
Deferred to slice 2d: treat LinkedGitHub answering err == nil with an empty/malformed p.ID as principal_unavailable, mirroring signinPrincipal.
There was a problem hiding this comment.
No P1/P2 findings (1 P3). Good to merge.
The pinned base is now the #598 merge, so this range is the slice itself (9 files, +274/-36). All three P3s from the earlier rounds are answered in it and the fixes hold up: refreshablePrincipal(login, principalID) refuses the login-less session outside principal mode on both refresh paths (TestRefresh_CarriesThePrincipal drives store x principalMode, with the continue scoped to the tc loop so no assertion is skipped); signinPrincipal treats a resolver answer that is not a prn_ as a failed lookup; checkProviderContract now confines Claims.TokenPrincipal to *localOAuthProvider alongside the principal namespace, with a table case for it; and the docs carry the rollback-day refresh rule and the /whoami id contract.
Verified this round rather than carried over: zitadelOnly matches LinkedGitHub's real contract (internal/principals/signin.go:106-119 — ErrNotFound with p == nil, ErrIdentityRefused/ErrPrincipalDisabled ahead of the three no-single-login sentinels, so a disabled principal still 403s without reaching signinPrincipal); the principal branch can only exit with a valid prn, since signinPrincipal sees the same h.opts and so stops on every failure in principal mode; PrincipalMode (cmd/api/main.go:185) and TenantAuthzMode (:943) both derive from cfg.tenantAuthzMode() at the one construction site, so sign-in cannot mint what ValidateJWT refuses; the new login-less refresh refusal strands no legitimate client, since the only IssueCode call reached with an empty login is the principal-mode ZITADEL branch; Claims.TokenPID stays harmless because it is read only in the ProviderGitHub arm of userFromVerified (a reserved namespace, exempt only for *localOAuthProvider) and only feeds the mismatch counter; and ParseTenantAuthzMode still refuses principal, so the slice is inert. No new concurrency surface.
P3 (posted inline, internal/api/oauth_upstream.go:331): the ZITADEL linked path is now the only sign-in that can mint a pid-less token in principal mode. It takes p.ID from the LinkedGitHub interface on trust, and an empty one passes IssueCode and IssueJWT (the login is non-empty), so the holder gets a code and a token that ValidateJWT then refuses forever — the mirror image of the resolver violation signinPrincipal just started catching.
The two P3s deferred to slice 2d (the pid-coverage wording at docs/principals.md:446 and agy's cross-replica pid round-trip test) are unchanged here and not re-raised.
Not run: go build, go vet, go test, gofmt — every go and gofmt invocation required approval in this sandbox and none was granted, so this is static reading plus cross-checking the called contracts. This is the fifth run in a row that could not compile the diff; please allow go test, go vet and gofmt in this job's --allowedTools.
Refs #377. PR-B slice 2b, stacked on #598 (2a,
pidclaim). Review only the top two commits; this retargets tomainonce #598 merges.What
In
TENANT_AUTHZ_MODE=principal, a ZITADEL identity that has no single GitHub login now signs in as its own principal instead of being sent to the link page:sub=pid= the principal id and no groups. Tenant access comes only from a GitOps member entry that names thatprincipalId(proposal Q5, acceptance criterion 4).store_unavailable, never a guess. A disabled principal gives 403principal_refused. An unresolvable principal gives 503principal_unavailable.Nothing changes outside principal mode.
ParseTenantAuthzModestill refusesprincipal, so the slice stays inert in production.Tests
TestZitadelSigninPrincipalMode: 12 cases covering linked, provisioned, own principal, unknown login, ambiguous, resolver disagreement, no resolver, resolver down, disabled, revoked, store down, and a refusal that names no principal. Each asserts thesub, thepid, the empty groups and the sign-in counter.TestZitadelOnlyPrincipal_SigninToTenantRole(B6, API side), end to end through sign-in →/oauth/tokenexchange →auth.MiddlewarewithPrincipalAuthz→checkTenantRole:p != nilguard each fail a test.Docs
docs/principals.mdgains the principal-mode answers, and the READMEOAUTH_UPSTREAMrow is updated./whoamicontract is now written down: a principal-mode admin hasadminsinnamespacesbut never ingroups, so clients readisAdmin. This is the P3 deferred from feat(api): principal mode core for tenant authz (#377 PR-B slice 1) #595.Remaining slices
Apply(B5).principal_idon unlink, then unlockingParseTenantAuthzMode.wft-create-tenantwritesprincipalId) lands in mctl-gitops.🤖 Generated with Claude Code
https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE