[mashkovd]

Refs #377. PR-B slice 2c (task B5). This is stacked on #600 (2b), which is stacked on #598 (2a). Review only the top two commits.

What

In TENANT_AUTHZ_MODE=principal, both the surface relay (relaySubject) and agent delegation (agentPrincipalGate) now decide the subject's tenants with the same auth.PrincipalAuthz the middleware applies to a direct caller:

• Tenants: the subject's tenants come from its principal's GitOps membership. The login-keyed TenantResolver is not consulted.
• Admin: the subject is never a platform admin, even when its principal is in ADMIN_PRINCIPALS. This uses the actingPrincipal guard in Apply.
• Refusal: if the principal is unresolved, the membership is unreadable, or PrincipalAuthz is not configured, the request gets 503 (surface_identity_unavailable / delegation_unavailable), never an empty tenant set. For the relay, this replaces "relay without a principal id" in this mode, which matches delegation (it already fails closed).

main.go builds the PrincipalAuthz once and passes it to both the middleware and Options.PrincipalAuthz, so a relayed decision and a direct decision cannot drift.

Behaviour outside principal mode does not change, and ParseTenantAuthzMode still refuses principal.

Tests

• TestSurfaceRelay_PrincipalMode (Postgres, through the router):
  • member by principal → 201
  • member by login only → 403
  • principal in ADMIN_PRINCIPALS with no membership → 403
  • membership unreadable → 503
  • principal unresolved → 503
  • not configured → 503
• TestAgentPrincipalGate_PrincipalMode:
  • the subject has the principal's groups, not the login's, and is not admin
  • membership unreadable → 503
  • not configured → 503
• Mutation-checked: removing either authorizeSubject call, or the relayed-admin guard in Apply, fails these tests.
• The full suite passes against Postgres.

Remaining

• 2d: non-blocking membership snapshot, revoking refresh families by principal_id on unlink, then unlocking ParseTenantAuthzMode.
• B7 lands in mctl-gitops.
• Out of scope: a ZITADEL-only human (no GitHub login) still cannot link a surface or be named by a grant, because both name a github: subject. That is follow-up work, not a regression.

🤖 Generated with Claude Code

https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @mashkovd's task in 4m 12s —— View job

---

Review complete ✓

Todo list

• Gather context (diff, CLAUDE.md, changed files)
• Understand the request (code review of pinned commit cbed4dd)
• Review correctness/logic
• Review security (auth/authz)
• Review concurrency
• Review error handling
• Review test coverage
• Post inline findings (none needed)
• Submit final PR review verdict

Summary

Reviewed git diff 69a77dd...cbed4dd (the two commits in scope: b820604 feat and cbed4dd docs).

• relaySubject (internal/api/handlers_surface_identity.go) and agentPrincipalGate (internal/api/handlers_agent_identity.go) both now call the new authorizeSubject (internal/api/tenant_authz_principal.go) in principal mode, in the correct place: after AttachPrincipal resolves the subject's principal (and, for delegation, after the grant/principal cross-check), and before the subject reaches next.ServeHTTP.
• authorizeSubject delegates to auth.PrincipalAuthz.Apply, the same decision the middleware applies to a direct caller. Apply's u.platformAdmin = p.Admins[prn] && u.actingPrincipal == "" guard correctly denies admin to a relayed/delegated subject even when its principal is in ADMIN_PRINCIPALS, since NewRelayedUser/NewDelegatedUser always set actingPrincipal.
• The h.opts.TenantResolver != nil && !h.principalMode() guard in both handlers correctly confines the login-keyed fallback to legacy/shadow mode, leaving that behavior unchanged.
• cmd/api/main.go now builds principalAuthz once and passes the same pointer to the middleware and to Options.PrincipalAuthz, matching the PR's stated goal of preventing drift between a relayed/delegated decision and a direct one (previously only the middleware had this struct).
• Verified the 503 error codes (surface_identity_unavailable, delegation_unavailable) match what's documented.
• New tests in relay_principal_test.go cover: member by principal, member by login only, admin principal with no membership, membership unreadable, principal unresolved, and PrincipalAuthz not configured — matching the fail-closed 503 behavior described in the PR body, plus the agent-delegation analogue (TestAgentPrincipalGate_PrincipalMode).
• Traced the supporting pieces this diff depends on (auth.PrincipalAuthz.Apply, User.Identity(), AttachPrincipal) to confirm relayed/delegated subjects resolve as KindHuman and that an unresolved principal (prn == "") correctly yields ErrPrincipalAuthzUnavailable → 503, rather than silently granting no tenants.
• Note: could not execute go build/go test in this sandbox (commands required approval that wasn't available), so this review is based on careful static reading of the diff and its dependencies, cross-checked against the existing test helpers (sidEnv, tenantsByPrincipal, relayPrincipals) they reuse.

Verdict: Approved — no P1/P2 findings, no P3 nits worth flagging.
· branch feat/377-relay-by-principal

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit cbed4dd.

No significant issues found.

[claude]

No P1/P2 findings (0 P3). authorizeSubject is wired in the correct order in both relaySubject and agentPrincipalGate (after AttachPrincipal/grant cross-check, before the subject reaches the handler); PrincipalAuthz.Apply's actingPrincipal guard correctly denies admin to relayed/delegated subjects even when their principal is in ADMIN_PRINCIPALS; the TenantResolver skip in principal mode (!h.principalMode()) is correctly gated so legacy/shadow behavior is unchanged; the dedup of cfg.principalAuthz(gitReader) into one shared instance for the middleware and Options.PrincipalAuthz matches the stated goal of preventing drift. Test coverage (relay_principal_test.go) covers member-by-principal, member-by-login-only, admin-principal-relayed, membership-unreadable, principal-unresolved, and not-configured, matching the 503 fail-closed behavior described in the PR body. Good to merge.

The base branch was changed.

[claude]

Claude finished @mashkovd's task in 2m 32s —— View job

---

Code Review complete ✓

Todo list

• Gather context (diff, CLAUDE.md, changed files)
• Understand the request (automated review of pinned commit 63a108e)
• Review correctness/logic
• Review security (auth/authz)
• Review concurrency
• Review error handling
• Review test coverage
• Post inline findings (none needed)
• Submit final PR review verdict

Summary

Reviewed git diff 9d2f1dc...63a108e (the two commits in scope).

• authorizeSubject (internal/api/tenant_authz_principal.go) is called in relaySubject and agentPrincipalGate in the correct order: after AttachPrincipal resolves the subject's principal (and, for delegation, after the grant/principal cross-check), and before the subject reaches next.ServeHTTP.
• authorizeSubject delegates to auth.PrincipalAuthz.Apply. Traced Apply's u.platformAdmin = p.Admins[prn] && u.actingPrincipal == "" guard: since NewRelayedUser/NewDelegatedUser always set actingPrincipal, a relayed/delegated subject is correctly denied admin even when its principal is in ADMIN_PRINCIPALS.
• Confirmed NewRelayedUser/NewDelegatedUser set githubLogin: true with no githubID, so Identity() resolves KindHuman, and Apply correctly reaches the GitOps-membership branch for them (not the "non-human, no groups" branch).
• The h.opts.TenantResolver != nil && !h.principalMode() guard in both handlers correctly confines the login-keyed fallback to legacy/shadow mode, leaving that behavior unchanged outside principal mode.
• cmd/api/main.go now builds principalAuthz once (cfg.principalAuthz(gitReader), the only call site) and passes the same pointer to the middleware and Options.PrincipalAuthz — matches the PR's stated goal of preventing drift. The struct is read-only after construction, so sharing it across concurrent requests is safe; Apply only mutates the per-request *User.
• Verified the relay's changed refusal path: a generic AttachPrincipal failure (principal store down, etc.) now falls through to authorizeSubject, which sees an empty principalID and returns ErrPrincipalAuthzUnavailable → 503 — correctly replacing the old "relay without a principal id" behavior in this mode only, matching delegation's existing fail-closed behavior.
• New tests in relay_principal_test.go (TestSurfaceRelay_PrincipalMode, TestAgentPrincipalGate_PrincipalMode) cover: member by principal, member by login only, admin principal relayed/delegated (never admin), membership unreadable, principal unresolved, and PrincipalAuthz not configured — matching the fail-closed 503 behavior described in the PR body. Confirmed the delegation path has no "principal unresolved" test because AttachPrincipal already fails closed before authorizeSubject is reached there (unlike the relay), so that case is correctly unreachable rather than an untested gap.
• Could not execute go build/go test in this sandbox (commands required approval that wasn't available); this review is based on static reading of the diff and its dependencies (auth.PrincipalAuthz.Apply, User.Identity(), AttachPrincipal, NewRelayedUser/NewDelegatedUser), cross-checked against existing test helpers (tenantsByPrincipal, sidEnv, relayPrincipals, fakeDelegation) they reuse.

Verdict: Approved — No P1/P2 findings (0 P3). Good to merge.
· branch feat/377-relay-by-principal

[claude]

No P1/P2 findings (0 P3). authorizeSubject (internal/api/tenant_authz_principal.go) is wired in the correct order in both relaySubject and agentPrincipalGate (after AttachPrincipal/grant cross-check, before the subject reaches the handler); PrincipalAuthz.Apply's actingPrincipal guard correctly denies admin to relayed/delegated subjects even when their principal is in ADMIN_PRINCIPALS; the TenantResolver skip (!h.principalMode()) is correctly scoped to leave legacy/shadow behavior unchanged; the dedup of cfg.principalAuthz(gitReader) into one shared instance (cmd/api/main.go) for the middleware and Options.PrincipalAuthz matches the stated goal of preventing drift between a relayed decision and a direct one. Verified NewRelayedUser/NewDelegatedUser set githubLogin with no githubID, so Identity() resolves KindHuman and Apply correctly decides tenants/admin for them. Test coverage (relay_principal_test.go) covers member-by-principal, member-by-login-only, admin-principal-relayed, membership-unreadable, principal-unresolved, and not-configured for the relay, and the delegation analogue, matching the 503 fail-closed behavior described in the PR body. Good to merge.

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit 63a108e.

No significant issues found.