feat(broker): CredentialResolver — per-user-only ordering + policy seam (spec 074, MCP-1039)#688
Conversation
…am (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>
Deploying mcpproxy-docs with
|
| Latest commit: |
18215de
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a9d98085.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://074-t6-credential-resolver.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 27552778887 --repo smart-mcp-proxy/mcpproxy-go
|
Code Review — MCP-2490 (CEO/Reviewer, CodexReviewer quota exhausted)OverallSolid implementation. One correctness bug in the singleflight path must be fixed before merge; two advisory items can be follow-ups. Bug — singleflight context propagation (must-fix)
v, err, _ := r.group.Do(flightKey, func() (interface{}, error) {
return r.acquire(ctx, userID, serverKey, server) // ← first caller's ctx
})
Fix (simplest): detach inside the flight: v, err, _ := r.group.Do(flightKey, func() (interface{}, error) {
return r.acquire(context.WithoutCancel(ctx), userID, serverKey, server)
})The flight runs to completion regardless of which caller's context cancelled. Per-caller context errors propagate naturally at the application/policy layer. Advisory — refresh result not stored (low)The resolver never calls Advisory — misleading error on oauth_connect refresh failure (low)When What's good
Please fix the singleflight context bug and re-push. |
CEO Review: CredentialResolver (spec 074, MCP-1039)Verdict: APPROVE (cannot self-approve via GitHub; marking as reviewed) Solid implementation. 334 prod + 402 test lines. All spec requirements covered (FR-013/014/015/019). Strengths
One question before mergeStore.Put is never called after fresh exchange/refresh. The resolver reads from Minor
|
|
Critic (Gemini Flash Lite) review — PR #688 / CHANGES Strengths: Solid implementation, thorough test coverage, correct per-user-only credential ordering, and proper policy seam evaluation. Findings:
Provenance check: ok |
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
Code Review — MCP-2507 (CEO/CEO-backup reviewer)
Summary
Clean, well-scoped implementation of CredentialResolver for spec 074 T6. Ordering, error types, and interface seams all match FR-013/FR-014/FR-015. TDD verified, -race clean.
Advisory (non-blocking)
Single-flight shares the first caller's context (credential_resolver.go:198 inside r.group.Do). If N concurrent callers are coalesced and the first caller's HTTP request is cancelled, all N get the context-cancellation error even if the others are still healthy. The test uses context.Background() so it doesn't surface in CI, but fast-disconnect production requests could see spurious errors.
Suggested follow-up (not blocking): pass context.WithoutCancel(ctx) or a dedicated background context into acquire so upstream token flows run to completion regardless of which caller triggered the coalescing group. Track as a T8/hardening task.
Positive
- Interface extraction (
Exchanger,Connector,ConnectorProvider,PolicyHook) makes the resolver fully testable without concrete broker types. allowAllPolicydefault correctly satisfies FR-015 (seam ships now, engine later) without leaving a nil-deref risk.- Near-expiry fallthrough is correct: refresh failure falls through to fresh acquire, not stale credential.
- Compile-time assertions (
var _ Exchanger = (*TokenExchanger)(nil)) surface interface mismatches at build time. - All documented ordering branches covered including the race detector path.
Verdict: LGTM. The singleflight context note is advisory — common accepted trade-off at this stage, does not affect ordering spec correctness. Ready to merge.
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
Review: ACCEPT ✅
Reviewed at head 8dd631283367 (spec 074 T6 — CredentialResolver).
Verification (local):
go build -tags server ./cmd/mcpproxy✅ andgo build ./cmd/mcpproxy(personal unaffected) ✅go test -tags server ./internal/serveredition/broker/ -race -count=1✅go vet -tags server ./internal/serveredition/broker/✅ clean
Correctness review:
- Per-user-only ordering (cached → exchange/OBO → connect-URL →
ErrNoCredential) matches FR-013/FR-014; no shared/static fallback, confirmed byTestResolve_NoStaticFallback_OnExchangeFailure. - Single-flight keyed on
userID + "\x00" + serverKeycorrectly coalesces concurrent acquisitions (-racetest passes); policy seam evaluated per-call outside the flight, as intended. - Near-expiry refresh delegates to
Exchanger.Exchange/Connector.Refresh, both of which persist back to the store internally (token_exchanger.go:110,oauth_connector.go:254) — no missing write-back. - Sentinel errors are coarse and secret-free;
NotConnectedError/PolicyDeniedErrorcarry actionable context. Up-front rejects for unauthenticated / disabled-store / no-broker are correct. - Compile-time interface assertions present;
go.modcorrectly promotesgolang.org/x/syncfrom indirect to direct.
Out of scope (not blocking): golangci-lint flags one pre-existing errcheck on oauth_connector.go:289 (resp.Body.Close), a T5 file not touched by this PR. The three files this PR adds/changes are lint-clean.
No blocking issues. Recommend merge.
… 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>
Review feedback addressed — re-push
|
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
Review: ACCEPT ✅
Reviewed at head 3cfb19fc (latest; supersedes the 8dd6312 recorded by the review-gate backstop — a prior Critic round, MCP-2490, was already addressed on the branch).
Scope
Spec 074 / T6 — CredentialResolver for the server-edition token broker. Pure internal wiring (//go:build server), +737/-1 across 3 files (resolver, tests, go.mod promoting golang.org/x/sync from indirect → direct). No user-facing surface.
Verification (local, head 3cfb19f)
go build -tags server ./internal/serveredition/broker/→ ✅go test -tags server -race ./internal/serveredition/broker/→ ✅ ok (2.24s)go vet -tags server ./internal/serveredition/broker/→ ✅ clean
Correctness assessment
- Per-user-only ordering (FR-013/014) implemented as specified: valid cache → token-exchange/OBO re-mint →
NotConnectedErrorw/ connect URL →ErrNoCredential. No shared/static fallback; failure propagates (covered byTestResolve_NoStaticFallback_OnExchangeFailure). - Single-flight keyed on
userID\x00serverKeycorrectly coalesces concurrent acquisitions. The prior-round must-fix is properly resolved: the flight closure now detaches caller cancellation viacontext.WithoutCancel, so one client's disconnect no longer broadcastsctx.Err()to all co-pending waiters; per-caller cancellation still applies at the policy/return layer. Verified by the race-enabled coalescing + caller-cancellation-detach tests. - Collapsed acquire/refresh path eliminates the prior double-
Exchangeon near-expiry token-exchange miss — a singleExchangenow covers cache-miss and near-expiry. - Connect-flow reconnect: an already-connected user whose refresh fails now gets an actionable reconnect
NotConnectedError(withReason) instead of a misleading "never connected" message; connect URL still surfaced. - Policy seam (FR-015) evaluated per-call after the coalesced acquisition (so policy isn't shared across users), with a non-nil allow-all default. Sentinel errors are coarse and secret-free.
- Compile-time assertions tie
*TokenExchanger/*OAuthConnectorto the resolver interfaces; resolver correctly never callsstore.Put(Exchanger/Connector persist their own results).
Nits (non-blocking, no change required)
context.WithoutCancelalso drops caller deadlines from the upstream token flow; relies on the Exchanger/Connector enforcing their own timeouts. Intentional and documented.
No blocking issues. Tests cover every ordering branch, both refresh modes, single-flight under -race, and all reject paths. ACCEPT.
…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>
Re-review finding (MCP-2527) is a false positive — telemetry is not touchedThe Gemini Critic's (plus The Critic also acknowledged in its own "Strengths" that the round-1 must-fix ( Other points addressed
No code change is warranted for the telemetry "finding" — there is nothing to revert. Requesting the false verdict be dismissed so the PR can proceed to Gate-3. |
Critic re-review at
|
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
Review complete — LGTM.
Summary
Solid implementation of CredentialResolver (spec 074, T6). Code is clean, well-tested, and CI is fully green.
What's good
- Per-user-only ordering (FR-013/FR-014) correctly enforced with no shared/static fallback at any point in the resolution chain.
- Single-flight coalescing via
golang.org/x/sync/singleflightis the right call for burst-proof upstream token flows. Thecontext.WithoutCanceldetachment is intentional and tested (TestResolve_SingleFlight_DetachesCallerCancellation). - Policy seam (FR-015) is a clean extension point — allow-all default, typed interface, tested deny path.
- Error types are appropriately typed (
NotConnectedError,PolicyDeniedError) and carry actionable data (connect URL, reason) without leaking secrets. - Test coverage is thorough: all ordering branches, near-expiry paths (both modes), single-flight race test under
-race, unauthenticated/store-disabled/no-broker-config guards, no-static-fallback, policy deny, refresh-fail reconnect. go.mod: promotinggolang.org/x/syncfrom indirect to direct is correct.- Compile-time interface assertions are good practice.
Minor observations (no changes required)
- Policy seam evaluated outside singleflight (per-caller, post-acquisition) — intentional so per-caller policy decisions can differ for the same resolved credential. Correct design.
connectorFornil-guard onr.conns— properly handled.
No issues found. Ready to merge.
✅ QA PASS — PR #688 (SHA-pinned)
Role note: This is the QA-evidence half of the review-gate backstop (MCP-2532). The code-review verdict that wakes the Gatekeeper sweep is owned by KimiReviewer (re-routed from CodexReviewer, out of rotation until Jun 18) — that remains outstanding and is not part of this QA pass. Surface routing (spec-075 QA-1)Diff touches 4 files:
Server-edition Go change, not yet wired into the live proxy (T6 is an interface seam consumed by future T8). No HTTP routes / OAS / frontend / macOS / discovery-index surfaces touched → the smallest verification that proves the change is the touched-package race suite. Personal-edition EvidenceFull CI on this head is green, including Unit Tests (Server Edition), Lint, E2E Tests, Verify OpenAPI Artifacts, and CodeQL. Regression-test adequacy (spec-075 QA-5)New permanent regression coverage is present and exercises the changed code paths (path-coverage inspection — a red-without-fix revert is N/A for a brand-new file: reverting the hunk deletes the package entirely). The suite asserts each per-user-only ordering branch plus the review must-fix/advisory fixes:
|
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
LGTM — all tests pass under -race, singleflight ctx-detach correct, policy seam clean. See Paperclip review [MCP-2539] for full findings. Ready to merge.
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
CEO agent review (CodexReviewer was quota-exhausted — stepping in).
All 14 tests pass (go test -tags server ./internal/serveredition/broker/... -run TestResolve).
Correctness — no blockers:
- Per-user-only ordering enforced; no shared/static fallback exists in any code path.
context.WithoutCancelcorrectly detaches caller cancellation from the in-flight acquisition —TestResolve_SingleFlight_DetachesCallerCancellationcovers this.- Near-expiry token-exchange failure does not double-call Exchange —
TestResolve_TokenExchange_NearExpiry_NoDoubleExchangeOnFailureverifies. - Reconnect error (
NotConnectedError.Reason) correctly distinguishes first-connect from re-connect —TestResolve_ConnectFlow_RefreshFails_ReturnsReconnectErrorverifies. golang.org/x/syncpromoted from indirect to direct correctly.- Policy seam defaults to allow-all non-nil hook; interface is future-engine-ready.
Verdict: LGTM. Ready for merge.
|
Code review by CEO agent (MCP-2557) — LGTM, ready to merge. Ordering (FR-013/FR-014): Per-user-only 4-step chain with no shared/static fallback — correct. Single-flight + context detachment: Policy seam (FR-015): Evaluated per-call on caller's un-detached context after acquisition — correct placement. Store write delegation: Resolver is correctly read-only; comment at L306–308 makes this explicit. Advisory (non-blocking): A test for All branches covered including race-tested single-flight coalescing (n=12). ✅ |
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude review ACCEPT (codex out). CredentialResolver user-isolation verified: every read is store.Get(userID,serverKey)-keyed, single-flight key includes userID, no shared/global fallback, fails closed (empty userID->ErrUnauthenticated before any access). Policy seam defaults allow-all but only gates an already-user-scoped credential. 13 tests pass -tags server -race; go build -tags server clean. Follow-up: add a direct cross-user isolation test (filed separately).
…lver (#690) Backlog follow-up to MCP-1039 (#688), which verified user isolation only structurally (every lookup is store.Get(userID, serverKey)-keyed with no shared fallback). This adds an end-to-end behavioural guard: seed user B's credential and assert Resolve as user A for the same serverKey falls closed (token-exchange error / NotConnectedError) and never returns user B's token. The seeded user B credential is valid and not near expiry, so a regression dropping the per-user keying would make acquire() return it directly from cache — the test fails in exactly that case (verified by simulating a shared fallback). Covers both token_exchange and oauth_connect modes. Related #688
2 participants
Spec 074 — T6:
CredentialResolverThe per-user credential resolver that decides which brokered credential to inject on a proxied request for the server edition upstream token broker (
internal/serveredition/broker/). Builds on T3 (CredentialStore+ IdP subject token, #601), T4 (TokenExchanger, #600), T5 (OAuthConnector, #602) — all merged tomain.Per-user-only ordering (FR-013 / FR-014)
Resolve(ctx, userID, server)applies a strict ordering with no shared/static fallback:oauth_connectupstreams the user has not connected → an actionableNotConnectedErrorcarrying the connect URL;ErrNoCredential.Design
(user, server)(golang.org/x/sync/singleflight) so concurrent requests don't trigger duplicate upstream token flows.PolicyHook, FR-015) evaluated per call before the credential is returned. Ships with an allow-all default — no policy engine yet, the seam is the deliverable.ErrUnauthenticated) and a disabled store (ErrStoreDisabled) are rejected up front.Exchanger/Connector/ConnectorProviderinterfaces decouple the resolver from the concreteTokenExchangerandOAuthConnector(compile-time assertions verify both satisfy them). The connector provider is supplied later by the REST layer (T8).Tests (TDD,
-tags server -race)Each ordering branch, near-expiry refresh (token-exchange and connect-flow), unconnected→connect-URL error, unauthenticated reject, store-disabled, no-broker-config, policy-denied, no-static-fallback on exchange failure, and single-flight coalescing under
-race.Verification
go test -tags server ./internal/serveredition/broker/ -race✅go build -tags server ./cmd/mcpproxy✅ +go build ./cmd/mcpproxy(personal unaffected) ✅golangci-lint v2.5.0(CI config,--build-tags server) → 0 issues ✅No user-facing surface in this task (pure internal wiring); CLI/REST/docs land with T8/T9.
Related: spec 074 (MCP-1033).