feat(teams): persist + refresh IdP subject token at login (spec 074, MCP-1036) - #601
Merged
Conversation
…MCP-1036) Today HandleCallback fetched userinfo then discarded the provider token. When teams.store_idp_tokens is enabled, capture the IdP access + refresh token at login and persist it encrypted (AES-256-GCM) via the credential store as an idp_subject_token record keyed by userID (empty serverKey). Adds GetValidIDPSubjectToken: returns a non-expired token, refreshing via the provider refresh_token grant when expired/near-expiry; when no valid token can be produced (absent, store disabled, expired-and-not-refreshable, or refresh failed) it returns ErrReauthRequired — never a stale token. This is the prerequisite seam consumed by the credential resolver (Path A token exchange, MCP-1039). Default-off: with store_idp_tokens false (default) or no encryption key, login behaves exactly as before — no storage. Persist is best-effort: a write failure never breaks login. - OAuthProvider.RefreshAccessToken: refresh_token grant (RFC 6749 §6) - OAuthHandler.credStore + SetCredentialStore; wired in teams/setup.go - TDD: capture-when-enabled, no-storage-when-disabled, refresh-near-expiry, expired-not-refreshable -> re-auth, absent -> re-auth, store-disabled -> re-auth FR-004, FR-005, FR-006. Related #588 (T2 config), #587 (T1 store). Co-Authored-By: Paperclip <noreply@paperclip.ing>
Deploying mcpproxy-docs with
|
| Latest commit: |
29f9bd8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://16c1d1f8.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://074-t3-idp-subject-token.mcpproxy-docs.pages.dev |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 27271359858 --repo smart-mcp-proxy/mcpproxy-go
|
When teams.store_idp_tokens is on, the login authorization request must ask providers for a durable refresh token so GetValidIDPSubjectToken can actually refresh instead of returning ErrReauthRequired after expiry (Codex finding on PR #601). Provider-specific contracts (FR-006 default-off — login unchanged otherwise): - Google: access_type=offline + prompt=consent (OfflineAuthParams) - Microsoft: offline_access scope (OfflineAccessScopes) - GitHub: unchanged (no refresh-token mechanism in scope) BuildAuthURL gains an offlineAccess bool; caller (HandleLogin) sets it from config.StoreIDPTokens. New tests: - TestBuildAuthURL_OfflineAccess_Google / _Microsoft - TestHandleLogin_RequestsOfflineAccess (default-off assertion added to TestHandleLogin_Redirects) Co-Authored-By: Paperclip <noreply@paperclip.ing>
…tial_encryption_key) Satisfies ENG-9 requirement from Codex R2 review on PR #601: document the operator/security model introduced by idp_subject_token.go and setup.go: - store_idp_tokens config field (default off) - credential_encryption_key / MCPPROXY_CRED_KEY env override - AES-256-GCM at-rest encryption model, key generation, key rotation caveat - Token lifecycle (login → use → refresh → re-auth) Co-Authored-By: Paperclip <noreply@paperclip.ing>
Member
Author
|
CEO Review — MCP-1801 Final Sign-off (head 49339c3) All R1 blockers resolved:
This PR is clear for merge. |
Dumbris
enabled auto-merge (squash)
June 10, 2026 09:53
CodexReviewer caught that idp-token-storage.md named the key-rotation cleanup bucket as 'upstream_credentials' but the actual store is 'user_upstream_credentials' (internal/teams/broker/bbolt_aes.go:28) — following the doc would clear the wrong bucket and leave IdP tokens behind. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Gatekeeper approval — Codex review verdict: ACCEPT.
This approval is posted automatically by the MCPProxy Gatekeeper App on behalf of the Codex reviewer (verdict of record lives in the Paperclip review thread). Author≠approver satisfied; QA + CI gates enforced separately.
Auto-approved per Model B (MCP-1249).
Dumbris
added a commit
that referenced
this pull request
Jun 15, 2026
…am (spec 074, MCP-1039) (#688) * feat(broker): CredentialResolver — per-user-only ordering + policy seam (spec 074, MCP-1039) Add the per-user credential resolver (T6) that selects which brokered credential to inject on a proxied request. Strict per-user-only ordering (FR-013/FR-014), with no shared or static fallback: 1. valid cached per-user credential (refreshed if near-expiry); 2. else token-exchange / Entra OBO from the stored IdP subject token; 3. else, for oauth_connect upstreams the user has not connected, an actionable NotConnectedError carrying the connect URL; 4. else ErrNoCredential. - Single-flight coalescing per (user, server) so concurrent acquisitions do not trigger duplicate upstream token flows (golang.org/x/sync). - PolicyHook seam (FR-015) evaluated per call before a credential is returned; ships with an allow-all default (no policy engine yet). - Unauthenticated callers and a disabled store are rejected up front. - Exchanger/Connector/ConnectorProvider interfaces decouple the resolver from the concrete TokenExchanger (T4) and OAuthConnector (T5); both satisfy the interfaces via compile-time assertions. TDD: each ordering branch, near-expiry refresh (token-exchange + connect), unconnected -> connect-URL error, unauthenticated reject, store-disabled, policy-denied, no-static-fallback, and single-flight under -race. Related: spec 074 (MCP-1033). Builds on T3 (#601), T4 (#600), T5 (#602), all merged to main. Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(broker): address review on CredentialResolver — singleflight ctx, double-exchange, reconnect error Review (MCP-2490 / Critic) on PR #688: - must-fix: detach the caller's context inside the single-flight closure with context.WithoutCancel. The flight runs the acquisition once for all co-pending callers; inheriting the first caller's cancellation let a single client disconnect/timeout broadcast ctx.Err() to every waiter for the same (user, server). Per-caller cancellation still applies at the policy/return layer (caller's original ctx). - advisory: collapse the per-mode acquire/refresh paths so a near-expiry token-exchange miss no longer calls Exchange twice (refresh-then-fallthrough); a single Exchange now covers both cache-miss and near-expiry. - advisory: an already-connected oauth_connect user whose refresh fails now gets an actionable reconnect error (NotConnectedError.Reason) instead of a misleading "never connected" message; the connect URL is still surfaced. - advisory: document that the Exchanger (T4) and Connector (T5) persist their results themselves, so the resolver never calls store.Put. Tests: add single-flight caller-cancellation detach, no-double-exchange on near-expiry failure, and connect-flow refresh-fail -> reconnect. Full broker suite green under -tags server -race; golangci-lint v2.5.0 clean. Co-Authored-By: Paperclip <noreply@paperclip.ing> * docs(auth-broker): document per-user credential resolution ordering (spec 074, MCP-1039) Add a "Credential resolution" section describing the resolver's strict per-user-only ordering (cached/refresh -> token-exchange/OBO -> actionable connect-URL error -> no-credential), the no-shared/static-fallback guarantee, single-flight coalescing, and the policy-decision seam. Keeps the feature doc consistent with the CredentialResolver added in this PR (review follow-up). Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
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
2 participants
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.
Summary
Spec 074 T3 (MCP-1036). Builds on T1 (#587 credential store) and T2 (#588 config), both merged.
Today
HandleCallbackfetches userinfo then discards the provider token. This PR captures it whenteams.store_idp_tokensis enabled and adds a get-or-refresh seam that never serves a stale token.What changed
oauth_handler.go): after code exchange + user upsert, whenstore_idp_tokensis on, persist the IdP access + refresh token encrypted (AES-256-GCM) via the credential store as anidp_subject_tokenrecord keyed by userID (emptyserverKey). Best-effort — a write failure never breaks login.idp_subject_token.go):GetValidIDPSubjectToken(ctx, userID)returns a non-expired token, refreshing via the providerrefresh_tokengrant when expired/near-expiry (60s skew). When no valid token can be produced — absent, store disabled, expired-and-not-refreshable, or refresh failed — it returnsErrReauthRequired, never a stale token (FR-005). This is the prerequisite consumed by the credential resolver (Path A, MCP-1039).oauth_providers.go):OAuthProvider.RefreshAccessToken(refresh_token grant, RFC 6749 §6).teams/setup.go): construct the broker store fromMCPPROXY_CRED_KEY/teams.credential_encryption_keyand attach viaSetCredentialStore.Default-off (FR-006)
With
store_idp_tokensfalse (default) or no encryption key configured, login behaves exactly as before — no storage, store constructed disabled, capture silently skipped.Tests (TDD,
-tags server)ErrReauthRequiredErrReauthRequiredErrReauthRequiredVerification
go build -tags server ./cmd/mcpproxy✅ andgo build ./cmd/mcpproxy(personal unaffected) ✅go test -tags server ./internal/teams/... -race✅ (all packages)gofmt/go vetclean; CI lint config clean on changed filesDocs
No docs change, matching T1/T2 precedent (neither shipped docs for these keys). The feature is server-edition, default-off, and not yet user-consumable until the resolver + header injection land (T6+). Consolidated spec-074 user docs belong with that completion.
Related #588, #587. Blocks MCP-1039 (T6 resolver).
Gate 3: opening for review/CI only — I do not merge.