Wire identityFromToken into the OAuth2 upstream provider

[jhrozek]

Summary

• The embedded auth server's pure-OAuth2 path resolved identity in two ways: a UserInfo HTTP fetch when userInfo was configured, otherwise a synthesized tk-... subject (PR Allow OAuth2 upstreams to omit userInfo config #5094). Upstreams that expose identity directly in the token-endpoint response (Snowflake's username, Slack v2's authed_user.id) had no good answer — they would either go through synthesis (losing real subject identity for users row creation) or require a fake userInfo URL.
• This PR wires IdentityFromToken into BaseOAuth2Provider.ExchangeCodeForIdentity as a third resolution mode that runs ahead of userInfo. The existing tokenResponseRewriter already reads and parses every successful token-endpoint response to normalise non-standard envelopes; it now extracts identity claims from the same body using gjson dot-notation paths when an operator configures it.
• New priority chain: (1) IdentityFromToken extracts identity from the raw pre-rewrite body, (2) UserInfo fetches via HTTP (existing), (3) synthesizeIdentity falls back (existing). When extraction is configured but fails, the operator-actionable extractor error (path name + type description, never body content) is surfaced through ErrIdentityResolutionFailed rather than silently falling through to userinfo or synthesis — "identityFromToken set" is an explicit operator claim.
• RefreshTokens deliberately passes nil for the identity config: providers like Snowflake omit username on refresh; identity is cached at auth-code time and read from session storage on subsequent requests.
• The OIDC provider discards the rewriter's identity return value with a defensive WARN if a future config-loader bug ever sets IdentityFromToken on an OIDC base config (structurally absent on the OIDC CRD type today).
• Two slog.Info calls in exchangeCodeForTokens were downgraded to slog.Debug for silent-success-at-INFO compliance.

Closes #5156

Type of change

• New feature

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

New tests cover the happy path, the userinfo-bypass tripwire (asserts no userinfo HTTP call when identityFromToken is configured), the userInfo-only regression, the synthesis fallback, the refresh path (no identity extraction), the @upstreamjwt modifier path through the provider, and an information-disclosure assertion that the raw token body never appears in error messages.

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

This PR adds an additive optional field to the runtime OAuth2Config only; the matching CRD type lands in #5155.

Does this introduce a user-facing change?

No direct CRD-level user-facing change in this PR. End-to-end functionality is gated on the runtime translation in #5157 (the next PR in the stack), which wires the v1beta1 CRD field into this runtime config.

Implementation plan

Approved implementation plan

This is phase 3 of the Snowflake / identityFromToken story (#5150):

• Phase 1 (Add identity extractor for OAuth2 token responses #5152, merged via PR Add identity extractor for OAuth2 token responses #5200): pure helper extractIdentityFromTokenResponse and the @upstreamjwt gjson modifier.
• Phase 2 (Add identityFromToken to MCPExternalAuthConfig CRD #5155): IdentityFromTokenConfig CRD type on MCPExternalAuthConfig v1beta1.
• Phase 3 (this PR, Wire identityFromToken into the OAuth2 upstream provider #5156): Wire the helper into BaseOAuth2Provider.ExchangeCodeForIdentity via the existing tokenResponseRewriter. New priority chain. Identity-source set on the rewriter, consumed by the caller after Exchange returns.
• Phase 4 (Translate identityFromToken from CRD to runtime config #5157): Operator-side translation from CRD to runtime config; bootstrap call to RegisterModifiers() so @upstreamjwt paths actually work in production.
• Phases 5-6 (thv llm setup doesn't append provider path prefix for Envoy AI Gateway #5158, Recognise identityFromToken in synthesis-mode detection #5159): Examples + synthesis-detection refinement so the IdentitySynthesized controller condition no longer fires when IdentityFromToken is configured.

Design constraints surfaced and addressed in this PR:

• Identity extraction runs on the raw pre-rewrite body. rewriteTokenResponse only relocates standard OAuth fields today and never touches identity-shaped fields, but extracting first makes the ordering invariant independent of that assumption.
• The rewriter is single-use by construction (wrapHTTPClientForTokenExchange creates a fresh one per call); no state shared across goroutines.
• Extraction failure does NOT fall through to userinfo. Operators who configure IdentityFromToken are claiming identity is in the token response; we surface the failure rather than mask it.
• The extractor's error format is subjectPath %q %s where %s is a static validator description ("path not found in token response", etc.). Never body bytes. The ExchangeCodeForIdentity caller wraps this directly so operators see the misconfigured path name in the returned error.

Special notes for reviewers

• Stack ordering: The doc comments at pkg/authserver/upstream/oauth2.go:160-164 and pkg/authserver/upstream/identity_from_token.go:31-32 reference cmd/thv-operator/api/v1alpha1.IdentityFromTokenConfig (the v1beta1 CRD type). That type lands in Add identityFromToken to MCPExternalAuthConfig CRD #5155. Until Add identityFromToken to MCPExternalAuthConfig CRD #5155 merges, those references dangle on this branch in isolation. End-to-end functionality requires Translate identityFromToken from CRD to runtime config #5157 too, which adds the operator-to-runner translation and the RegisterModifiers() bootstrap call.
• The docs/arch/11-auth-server-storage.md update reframes the previous "Synthesis-mode subjects" section as a 3-priority chain. It also flags a known gap: the controller predicate SyntheticIdentityUpstreams() checks only userInfo == nil and does not yet account for IdentityFromToken, so an upstream with only IdentityFromToken configured will still trigger IdentitySynthesizedActive until Recognise identityFromToken in synthesis-mode detection #5159 lands.
• The synthesize fallback subtest in oauth2_test.go was rewritten to use NewOAuth2Provider instead of hand-building a struct literal, aligning it with the rest of the suite.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 89.33333% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.97%. Comparing base (d62dd0e) to head (eb6ee4b).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/authserver/upstream/oauth2.go	86.36%	5 Missing and 1 partial ⚠️
pkg/authserver/upstream/oidc.go	77.77%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5222      +/-   ##
==========================================
- Coverage   68.02%   67.97%   -0.05%     
==========================================
  Files         615      615              
  Lines       62960    62998      +38     
==========================================
- Hits        42829    42824       -5     
- Misses      16929    16973      +44     
+ Partials     3202     3201       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tgrunnagle]

Multi-agent review — /dev:pr-review

Reviewed by 6 specialist agents (security, concurrency, error-handling, test-coverage, docs-and-api, code-quality). Codex cross-review skipped (CLI not installed).

Recommendation: COMMENT — no blocking issues. The PR's design is sound, the priority chain is correct and well-tested, and the architecture doc is updated alongside the code. Findings below are quality improvements (doc accuracy, test rigor, log consistency) rather than correctness blockers.

Consensus findings (11 surviving from ~80 raw)

#	Finding	Severity	Score
F1	Rewriter mutable state lacks single-use / no-concurrent-use doc	MEDIUM	9
F2	CRD type forward-reference points to v1alpha1 but sibling types live in v1beta1	MEDIUM	9
F3	Refresh-path test reconstructs the helper call instead of observing RefreshTokens behavior	MEDIUM	8
F4	TestTokenResponseRewriter_IdentityFromRawBody doesn't exercise the ordering invariant it claims	MEDIUM	8
F5	Constructor WARN "using synthesis mode" still fires when IdentityFromToken is configured	MEDIUM	8
F6	OIDC defensive WARN is currently unreachable and would be misleading if it fired	MEDIUM	8
F7	@upstreamjwt trust-boundary deserves an explicit unit test pinning unsigned-payload behavior	LOW	7
F8	"Unreachable in practice" comment overstates an oauth2-library-dependent invariant	LOW	7
F9	INFO→DEBUG downgrade is asymmetric: RefreshTokens still uses slog.Info	LOW	7
F10	Happy-path test does not assert identity.Synthetic == false	LOW	7
F11	TestWrapHTTPClientWithMapping_NilMapping retains stale name after helper rename	LOW	7

Holistic assessment

Strengths verified across multiple agents:

• Pre-rewrite extraction ordering means SubjectPath always resolves against the original provider response, immune to future rewriteTokenResponse changes.
• Information-disclosure guard is real and tested: extraction errors carry path name + static type description only, never body bytes (regression test in place).
• sha256("") collision protection preserved through the new priority chain.
• No downgrade attack on the refresh path: identity is anchored at auth-code time and RefreshTokens consumes the cached UserID from storage.
• Error wrapping chain holds through errors.Is(err, ErrIdentityResolutionFailed) for both extraction-failure and userInfo-failure branches.
• Synthetic = false defaults correctly for the priority-1 identity, routing through UserResolver.ResolveUser.
• The userinfo-tripwire test pins the early-return invariant.

Architecture. This is phase 3 of a 6-PR stack (#5156). Forward references to the CRD type (lands in #5155) and operator translation (#5157) are appropriately gated. The SyntheticIdentityUpstreams() predicate gap is correctly flagged as a known forward-looking limitation. The additive OAuth2Config.IdentityFromToken field is non-breaking.

Cross-cutting clusters. The MEDIUM findings cluster around two themes:

1. Doc-code synchronization (F2): a one-line factual error in two locations that future readers will be confused by.
2. Concurrency-invariant documentation (F1): the rewriter has grown mutable per-call state without a doc comment stating the single-use lifecycle. The current caller pattern is correct, but the contract is implicit.

The operator-visible log inconsistencies (F5, F6, F9) form a smaller cluster — none affect runtime correctness, but they undermine the architecture doc's claim that operator visibility is a designed property of the new path.

Dropped findings (< 7 consensus, no action expected)

Validate() doesn't check NamePath/EmailPath gjson syntax; missing OIDC IdentityFromToken WARN test (branch unreachable); Validate() table missing field combinations; missing edge-case test for misconfigured EmailPath at integration layer (covered at unit layer); no concurrent-use test for rewriter (subsumed by F1); missing rewriter-level negative test (subsumed by F4); tokenExchangeResult doc missing mutual-exclusion invariant (related to F1); extraction failure may double-log; RefreshTokens silently drops identity fields (acceptable trade-off per PR description); fmt.Errorf %w + %s loses validator chain depth (errors.Is still works); test helpers duplicated; identity vs extractedIdentity naming; awkward refresh-path comment; SPDX header format (pre-existing); rewriter mapping-time error not surfaced on refresh; rewriter retained longer than needed.

---

Generated by /dev:pr-review (multi-agent consensus review). Inline comments follow.

[tgrunnagle]

The implementation LGTM. The agent-raised issues could be addressed in a separate PR, so approving in case you want to do that.

[jhrozek]

The implementation LGTM. The agent-raised issues could be addressed in a separate PR, so approving in case you want to do that.

Thank you, since there will be more PRs in this stacked series, I am going to include the fixes in the next PR.