Skip to content

Implement multi-upstream sequential authorization chain#4283

Open
jhrozek wants to merge 7 commits intomainfrom
authserver-multi-upstream/2-chain
Open

Implement multi-upstream sequential authorization chain#4283
jhrozek wants to merge 7 commits intomainfrom
authserver-multi-upstream/2-chain

Conversation

@jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Mar 19, 2026

Summary

  • The auth server previously supported only a single upstream IDP. This PR implements RFC-0052 Phase 2: a sequential authorization chain that transparently redirects users through every configured upstream IDP before issuing a single authorization code. This enables scenarios like corporate-idp + github + atlassian requiring tokens from all three providers.
  • Refactors Handler from a single upstream field to an ordered []NamedUpstream slice, wires SessionID generation into AuthorizeHandler (threaded across all chain legs), and adds continueChainOrComplete logic in CallbackHandler that either redirects to the next upstream or issues the final auth code.
  • Includes defense-in-depth: identity consistency verification across chain legs, cleanup of upstream tokens on any chain failure, fresh PKCE/state/nonce per leg, and IDP mix-up protection via upstreamByName lookup.

Closes #4137

Type of change

  • New feature

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)
  • Manual testing: this PR is cherry-picked from a larger integration branch (thv-as-vmcp) where the full multi-upstream flow was tested end-to-end with real OIDC providers against the Entra OBO test environment.

Changes

File Change
pkg/authserver/server/handlers/handler.go Replace single upstream field with []NamedUpstream slice; add NamedUpstream type, nextMissingUpstream, upstreamByName helpers; harden NewHandler constructor with validation
pkg/authserver/server/handlers/authorize.go Generate SessionID at chain start; target upstreams[0] as first leg; use slice-based provider lookup
pkg/authserver/server/handlers/callback.go Add continueChainOrComplete chain logic; carry identity from first leg through ResolvedUser* fields; add identity mismatch detection; clean up tokens on all failure paths
pkg/authserver/server/handlers/user.go Downgrade user creation log from Info to Debug
pkg/authserver/server_impl.go Build ordered upstream slice from all cfg.Upstreams; pass to NewHandler; update UpstreamTokenRefresher to dispatch by provider name
pkg/authserver/config.go Remove len > 1 guard on upstreams; update doc comments
pkg/authserver/refresher.go Accept provider map for multi-upstream token refresh dispatch
pkg/authserver/server/handlers/handler_chain_test.go New: tests for nextMissingUpstream (all-satisfied, first-missing, partial, storage error)
pkg/authserver/server/handlers/callback_test.go New: 8 multi-upstream chain tests (first-leg redirect, second-leg code issuance, identity carry-forward, identity mismatch rejection, fresh secrets, auth URL error cleanup, store-pending error cleanup)
pkg/authserver/integration_test.go New: full sequential chain integration test with two mock OIDC providers (authorize → leg-1 callback → leg-2 callback → token exchange)
pkg/authserver/server/handlers/helpers_test.go Add multiUpstreamTestSetup, baseTestSetupOption, multi-provider storage mock wiring

Does this introduce a user-facing change?

Multiple upstream IDPs can now be configured. The auth server will sequentially authorize through each before issuing a token.

Special notes for reviewers

  • Identity resolution strategy: The first upstream in the chain is the "identity provider" — it resolves/creates the internal user. Subsequent legs carry the resolved identity forward via PendingAuthorization.ResolvedUser* fields rather than re-resolving.
  • nextMissingUpstream returns error (not fail-safe restart): The issue spec suggested returning upstreamOrder[0] on storage error. The implementation instead returns the error, allowing the caller to clean up and return a proper error to the client. A silent restart could loop indefinitely if storage is persistently down.
  • 302 vs 303 as chain signal: Chain-continuation redirects use 302 (Found) to the next upstream, while the final auth code redirect uses fosite's 303 (See Other). Tests assert on this distinction.

Large PR Justification

The size owes itself mostly to tests

Generated with Claude Code

jhrozek and others added 5 commits March 19, 2026 21:50
Move the auth server handler from a single-upstream model to a
map-of-providers with an ordered list. This is the mechanical
refactor that threads multi-upstream support through config
validation, Handler construction, AuthorizeHandler, CallbackHandler,
server construction, and all existing tests.

No new behavior — the chain still completes after the first upstream.
After the handler refactor, wire up the actual chain behavior:
the callback handler now walks the upstream order, redirects to
the next missing provider, and only issues the authorization code
once every upstream has been satisfied. Identity fields from the
first upstream carry through the chain via PendingAuthorization,
and the refresher dispatches by ProviderID.
Unit tests for chain state inspection and multi-upstream callback
flow, plus an integration test that drives two mock OIDC providers
through the full sequential chain end-to-end.
Per silent-success convention, successful operations should not log
at Info level.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace map+slice pair with []NamedUpstream, eliminating the
cross-validation logic that kept them in sync. Return error from
NewHandler instead of panicking on invalid inputs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added the size/XL Extra large PR: 1000+ lines changed label Mar 19, 2026
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.


This review will be automatically dismissed once you add the justification section.

@codecov
Copy link

codecov bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 77.38095% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.18%. Comparing base (f52b6d0) to head (ea954ab).

Files with missing lines Patch % Lines
pkg/authserver/server/handlers/callback.go 74.22% 20 Missing and 5 partials ⚠️
pkg/authserver/server_impl.go 74.07% 5 Missing and 2 partials ⚠️
pkg/authserver/server/handlers/handler.go 81.81% 2 Missing and 2 partials ⚠️
pkg/authserver/server/handlers/authorize.go 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4283      +/-   ##
==========================================
+ Coverage   68.54%   69.18%   +0.64%     
==========================================
  Files         471      471              
  Lines       47732    47761      +29     
==========================================
+ Hits        32717    33045     +328     
+ Misses      12247    12149      -98     
+ Partials     2768     2567     -201     

☔ 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.

@jhrozek jhrozek requested a review from amirejaz as a code owner March 19, 2026 23:02
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Mar 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement multi-upstream sequential authorization chain in handler layer (RFC-0052 Phase 2)

1 participant