feat(serveredition): per-user brokered-credential REST surfaces (spec 074 T8)#692
Open
Dumbris wants to merge 3 commits into
Open
feat(serveredition): per-user brokered-credential REST surfaces (spec 074 T8)#692Dumbris wants to merge 3 commits into
Dumbris wants to merge 3 commits into
Conversation
… 074, MCP-1041)
Add the T8 REST API for per-user upstream credentials (server edition),
mounted behind the session/JWT auth middleware:
- GET /api/v1/user/credentials list connection status +
non-secret metadata (connected/expired/not_connected/unavailable);
access/refresh tokens are NEVER serialized (FR-026).
- DELETE /api/v1/user/credentials/{server} disconnect/revoke the caller's
own credential.
- GET /api/v1/user/credentials/{server}/connect initiate Path B; builds
a PKCE authorize URL bound to the user and 302-redirects upstream.
- GET /api/v1/user/credentials/{server}/callback validates state, exchanges
the code, persists the per-user credential, redirects to the Web UI.
Every operation is scoped to the authenticated caller (FR-027): list/delete key
the store by the caller's userID, and the connect callback persists under the
*initiating* user (the connector binds state->userID), so a callback cannot
write into another user's record.
A connectorProvider caches one OAuthConnector (T5) per oauth_connect upstream so
the in-memory PKCE/state survives between the connect redirect and the callback,
and satisfies broker.ConnectorProvider so the T6 CredentialResolver can mint
connect URLs through the same connectors.
TDD: secret redaction, status mapping, store-disabled, delete-removes,
unknown-server 404, cross-user isolation, connect redirect, connect+callback
end-to-end credential persistence, denied callback, unauthenticated 401.
Docs: CLAUDE.md server API table + docs/features/idp-token-storage.md credentials
API section. swagger.yaml is generated from personal-edition swag annotations
(server-edition routes are build-tagged and not scanned), consistent with every
other /api/v1/user/* and /api/v1/auth/* server-edition endpoint.
Related spec 074, MCP-1041 (blocked-by T5/T6, now resolved).
Deploying mcpproxy-docs with
|
| Latest commit: |
61f0c32
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f1f9c72f.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://074-t8-credentials-rest.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 27570311373 --repo smart-mcp-proxy/mcpproxy-go
|
Member
Author
Review verdict: REQUEST CHANGES (not ACCEPT)Reviewed at head 🔴 Blocker —
|
The T8 server-API table addition pushed CLAUDE.md from 39,949 to 40,411 chars, tripping the claude-md-check (check-size) 40,000-char gate. The credential endpoints are already fully documented in docs/features/idp-token-storage.md (the canonical server-edition home), so drop the redundant CLAUDE.md table rows rather than trim unrelated content. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Member
Author
CodexReviewer verdict: ACCEPTScope: Spec 074 T8 — per-user brokered-credential REST surfaces (server edition), head Verification (run locally on PR head)
Why ACCEPT
Non-blocking notes (future hardening, not for this PR)
No correctness, security, or test-quality blockers. Ready for the Gatekeeper sweep to approve. |
Member
Author
CodexReviewer (recovery by CEO) — Verdict: ACCEPT ✅Reviewed at head Verification
Correctness & security
Minor (non-blocking)
No blocking issues. ACCEPT. |
Maintainer fix-before-merge: PR #692 tripped the PR-size review gate at ~913 added lines. Remove the docs/features/idp-token-storage.md credentials API section from this PR so it reduces to handlers + tests only (~852 lines). The docs land in a separate follow-up PR so they're not lost. 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 T8 — the per-user brokered-credential REST API for the server edition, mounted behind the session/JWT auth middleware. Builds on T5 (
OAuthConnector) and T6 (CredentialResolver), whose blockers just resolved.GET /api/v1/user/credentialsDELETE /api/v1/user/credentials/{server}GET /api/v1/user/credentials/{server}/connectGET /api/v1/user/credentials/{server}/callback/ui/Security (FR-026 / FR-027)
CredentialStatus) has noaccess_token/refresh_tokenfields; only status + non-secret metadata (mode, scopes, token_type, audience, obtained_via, expiry). Tests assert seeded token strings never appear in the body.userID; the connect callback persists under the initiating user (the connector bindsstate -> userID), so a callback cannot write into another user's record. A cross-user test proves user A can neither see nor delete user B's credential.Design
connectorProvidercaches onebroker.OAuthConnectorperoauth_connectupstream (keyed by serverKey) so the in-memory PKCE/state survives between the connect redirect and the callback. It satisfiesbroker.ConnectorProvider, so T7's resolver can mint connect URLs through the same connectors (exposed viaCredentialHandlers.ConnectorProvider()).connected/expired/not_connected/unavailable(store disabled).oauth_connectupstreams that aren't usable expose an actionableconnect_path.Tests (TDD)
internal/serveredition/api/credential_handlers_test.go: secret redaction, status mapping, store-disabled, delete-removes, unknown-server 404, cross-user isolation, connect redirect, connect→callback end-to-end persistence (via httptest upstream token endpoint), denied callback, unauthenticated 401.Verification
go build -tags server ./cmd/mcpproxyandgo build ./cmd/mcpproxy— both green.go test -tags server -race ./internal/serveredition/...— all packages pass.golangci-lint v2.5.0 --config .github/.golangci.yml --build-tags server ./internal/serveredition/...— new files clean (2 remaining findings are pre-existing in unrelated test files and are not linted by CI, which doesn't pass--build-tags server).Docs (ENG-9)
CLAUDE.md server API table +
docs/features/idp-token-storage.mdcredentials-API section.oas/swagger.yamlis unchanged: it's generated from personal-edition swag annotations (make swagger), and server-edition routes are build-tagged and not scanned — consistent with every other/api/v1/user/*and/api/v1/auth/*server-edition endpoint, none of which appear in swagger.yaml.verify-oas-coverage.shonly scansinternal/httpapi/server.go; this PR adds zero routes there, so personal-edition coverage is unaffected.Related spec 074, MCP-1041.