feat: retain admin bootstrap credential for periodic per-user MCP tool discovery refresh - #5717
Conversation
oauth_configs to new mcp_oauth_flows table
#5709
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change retains admin credentials for per-user MCP clients. Credential stores resolve OAuth bearer headers or filtered headers. Disconnected per-user clients use these credentials for tool discovery. Storage, token refresh, synchronization, tests, and UI copy support the flow. ChangesAdmin MCP discovery flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant performSync
participant performAdminToolDiscovery
participant MCPCredentialStore
participant MCPServer
performSync->>performAdminToolDiscovery: discover tools for disconnected per-user client
performAdminToolDiscovery->>MCPCredentialStore: resolve retained admin headers
performAdminToolDiscovery->>MCPServer: verify connection and request tools
MCPServer-->>performSync: return discovered tool mappings
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/oauth2/access_token_test.go`:
- Around line 165-176: Update TestAccessToken_MissingToken_ReturnsError to seed
an active token linked to the same OAuth configuration but using the opposite
auth_mode for each getter case, while leaving the requested mode without a
token. Keep the existing missing-token assertions so the test verifies
credentials are isolated between shared and admin modes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a8939b97-5c43-4461-9205-fbbc3938a474
📒 Files selected for processing (37)
core/mcp/admin_tool_discovery_test.gocore/mcp/auth_retry_test.gocore/mcp/clientmanager.gocore/mcp/credstore/credstore.gocore/mcp/credstore/none.gocore/mcp/credstore/none_test.gocore/mcp/credstore/per_user_headers.gocore/mcp/credstore/per_user_headers_test.gocore/mcp/credstore/per_user_oauth.gocore/mcp/credstore/per_user_oauth_test.gocore/mcp/credstore/shared_headers.gocore/mcp/credstore/shared_headers_test.gocore/mcp/credstore/shared_oauth.gocore/mcp/credstore/shared_oauth_test.gocore/mcp/reauth_state_test.gocore/mcp/toolsync.gocore/mcp/toolsync_test.gocore/schemas/mcp.gocore/schemas/oauth.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/rdb_mcp_admin_auth_mode_test.goframework/configstore/store.goframework/configstore/tables/mcpheaders.goframework/configstore/tables/mcpoauth2.goframework/mcp_headers/credential_test.goframework/mcp_headers/main.goframework/mcp_headers/main_test.goframework/oauth2/access_token_test.goframework/oauth2/main.goframework/oauth2/sync.goframework/oauth2/sync_test.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config_test.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpHeadersAuthorizer.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
🚧 Files skipped from review as they are similar to previous changes (32)
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- framework/oauth2/sync_test.go
- core/mcp/clientmanager.go
- core/mcp/reauth_state_test.go
- core/mcp/toolsync.go
- core/schemas/mcp.go
- framework/configstore/rdb.go
- core/mcp/credstore/none_test.go
- transports/bifrost-http/lib/config_test.go
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
- core/schemas/oauth.go
- core/mcp/credstore/shared_oauth_test.go
- transports/bifrost-http/handlers/mcp.go
- core/mcp/credstore/per_user_oauth_test.go
- core/mcp/credstore/shared_headers.go
- framework/configstore/tables/mcpoauth2.go
- framework/mcp_headers/main.go
- core/mcp/credstore/credstore.go
- core/mcp/credstore/per_user_headers_test.go
- core/mcp/admin_tool_discovery_test.go
- core/mcp/toolsync_test.go
- core/mcp/credstore/shared_headers_test.go
- framework/configstore/tables/mcpheaders.go
- framework/configstore/store.go
- core/mcp/credstore/none.go
- framework/configstore/migrations.go
- core/mcp/auth_retry_test.go
- ui/app/workspace/mcp-registry/views/mcpHeadersAuthorizer.tsx
- framework/oauth2/main.go
- framework/mcp_headers/main_test.go
- core/mcp/credstore/shared_oauth.go
- core/mcp/credstore/per_user_oauth.go
2fc7ca2 to
bd1f787
Compare
ef71e78 to
97de528
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/configstore/rdb_mcp_admin_auth_mode_test.go (1)
22-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove manual GORM timestamp initialization.
GORM populates
CreatedAtandUpdatedAtduringCreate. These tests do not assert the supplied values. Removenow, the timestamp fields, and the unusedtimeimport.Based on learnings: models with GORM automatic timestamp fields must rely on GORM rather than
time.Now().Also applies to: 56-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/configstore/rdb_mcp_admin_auth_mode_test.go` around lines 22 - 31, Remove the manual now := time.Now() setup and the CreatedAt and UpdatedAt assignments from both TableMCPOauthToken fixtures in the test setup. Delete the now-unused time import and rely on GORM's automatic timestamps during Create.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@framework/configstore/rdb_mcp_admin_auth_mode_test.go`:
- Around line 22-31: Remove the manual now := time.Now() setup and the CreatedAt
and UpdatedAt assignments from both TableMCPOauthToken fixtures in the test
setup. Delete the now-unused time import and rely on GORM's automatic timestamps
during Create.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 264b663f-be07-45a0-8e18-8396c29cf28f
📒 Files selected for processing (37)
core/mcp/admin_tool_discovery_test.gocore/mcp/auth_retry_test.gocore/mcp/clientmanager.gocore/mcp/credstore/credstore.gocore/mcp/credstore/none.gocore/mcp/credstore/none_test.gocore/mcp/credstore/per_user_headers.gocore/mcp/credstore/per_user_headers_test.gocore/mcp/credstore/per_user_oauth.gocore/mcp/credstore/per_user_oauth_test.gocore/mcp/credstore/shared_headers.gocore/mcp/credstore/shared_headers_test.gocore/mcp/credstore/shared_oauth.gocore/mcp/credstore/shared_oauth_test.gocore/mcp/reauth_state_test.gocore/mcp/toolsync.gocore/mcp/toolsync_test.gocore/schemas/mcp.gocore/schemas/oauth.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/rdb_mcp_admin_auth_mode_test.goframework/configstore/store.goframework/configstore/tables/mcpheaders.goframework/configstore/tables/mcpoauth2.goframework/mcp_headers/credential_test.goframework/mcp_headers/main.goframework/mcp_headers/main_test.goframework/oauth2/access_token_test.goframework/oauth2/main.goframework/oauth2/sync.goframework/oauth2/sync_test.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config_test.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpHeadersAuthorizer.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
🚧 Files skipped from review as they are similar to previous changes (33)
- core/mcp/auth_retry_test.go
- core/mcp/clientmanager.go
- core/mcp/credstore/per_user_headers_test.go
- framework/configstore/tables/mcpheaders.go
- framework/configstore/tables/mcpoauth2.go
- ui/app/workspace/mcp-registry/views/mcpHeadersAuthorizer.tsx
- framework/configstore/rdb.go
- core/mcp/credstore/shared_oauth.go
- core/mcp/credstore/per_user_oauth.go
- transports/bifrost-http/lib/config_test.go
- framework/oauth2/sync_test.go
- core/schemas/oauth.go
- core/mcp/reauth_state_test.go
- framework/configstore/store.go
- framework/mcp_headers/main.go
- core/mcp/credstore/none.go
- core/mcp/credstore/shared_headers_test.go
- core/mcp/credstore/shared_headers.go
- core/mcp/credstore/none_test.go
- framework/oauth2/main.go
- core/mcp/credstore/per_user_headers.go
- core/mcp/credstore/shared_oauth_test.go
- core/mcp/toolsync_test.go
- core/schemas/mcp.go
- core/mcp/admin_tool_discovery_test.go
- framework/mcp_headers/main_test.go
- transports/bifrost-http/handlers/mcp.go
- core/mcp/credstore/credstore.go
- core/mcp/credstore/per_user_oauth_test.go
- framework/configstore/migrations.go
- core/mcp/toolsync.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
bd1f787 to
ef8c8b2
Compare
97de528 to
ad0fdc1
Compare
Merge activity
|
The base branch was changed.
…eturnsError to pin shared/admin credential isolation
ad0fdc1 to
db39b12
Compare

Summary
Per-user MCP clients (
per_user_oauthandper_user_headers) never hold a persistent connection, so the periodic tool syncer was silently skipping them entirely — their tool lists could never be refreshed after initial bootstrap. This PR introduces an "admin credential" retention mechanism: the bootstrap-verification credential (OAuth token or header values) submitted by the admin during one-time setup is now persisted underauth_mode='admin'and reused by the tool syncer to run ephemeral connect-discover-close cycles, keeping per-user clients' tool lists up to date without requiring a real end-user request.Changes
Admin credential retention at bootstrap time:
completeMCPClientOAuthnow re-tags the admin's bootstrap OAuth token fromauth_mode='shared'toauth_mode='admin'(instead of revoking it) after a successful verification.verifyMCPClientHeadersnow persists the admin's sample header values as anauth_mode='admin'credential row. Both are best-effort — a retention failure does not fail the request.performAdminToolDiscoveryonMCPManager: New method that resolves the retained admin credential viacredStore.AdminConnectionHeadersand runs a one-shot ephemeral connect-discover-close cycle using the sameVerifyPerUserOAuthConnection/VerifyHeadersConnectionfunctions the bootstrap flow uses.ClientToolSyncer.performSyncwidened: Theconn == nilbranch now distinguishes per-user auth types (per_user_oauth,per_user_headers) from shared-connection types. Per-user clients with no live conn are routed intoperformAdminToolDiscoveryinstead of being silently skipped; shared-connection clients mid-reconnect continue to be skipped as before.AdminConnectionHeadersadded toMCPCredentialStoreinterface and all resolvers:per_user_oauthandper_user_headersresolvers implement it meaningfully (OAuth token lookup viaGetAdminAccessToken; header credential lookup viaGetCredentialByModewithMCPAuthModeAdmin). All shared-credential resolvers (none,headers,oauth) return an explicit unsupported error.GetAdminAccessTokenadded toOAuth2Provider: Resolves theauth_mode='admin'token row viaGetAdminOauthTokenByConfigIDand funnels it through the extractedresolveAccessTokenhelper (shared withGetAccessTokento avoid duplicating status/expiry/refresh/sanitize logic).TokenRefreshWorkerdefaultAuthModeswidened to["shared", "admin"]: Admin-mode tokens back only the tool syncer and have no live caller to trigger lazy refresh, so they need the same proactive background sweep as shared tokens.MCPAuthModeAdminsupport inGetMCPPerUserHeaderCredentialByModeandUpsertMCPPerUserHeaderCredential: Admin-mode rows are scoped bymcp_client_idalone (no per-caller identity). The identity guard inmcp_headers.Provider.GetCredentialByModeis widened to allow an empty identity forMCPAuthModeAdminonly.Database migration
add_mcp_admin_auth_mode_indexes: Adds partial unique indexes onmcp_oauth_tokensandmcp_per_user_header_credentialsforauth_mode='admin', enforcing exactly one admin credential per MCP client.UI copy updates: Admin bootstrap flows for both OAuth and headers now accurately describe that the credential is retained for periodic tool-list refresh rather than discarded after a one-time test.
Type of change
Affected areas
How to test
per_user_oauthMCP client through the admin OAuth flow and confirm the token row inmcp_oauth_tokenshasauth_mode='admin'after completion.per_user_headersMCP client and confirm a row withauth_mode='admin'appears inmcp_per_user_header_credentials.headers,none,oauth) withconn == nil(mid-reconnect) are still silently skipped and not routed into admin discovery.Breaking changes
MCPCredentialStoreandOAuth2Providerare interfaces with new required methods (AdminConnectionHeadersandGetAdminAccessTokenrespectively). Any external implementations of these interfaces must add stub or real implementations of these methods. TheConfigStoreinterface also gainsGetAdminOauthTokenByConfigID. TheTokenRefreshWorkerdefaultAuthModeschanges from["shared"]to["shared", "admin"], which may cause additional token refresh queries if admin-mode rows exist.Security considerations
The retained admin credential (
auth_mode='admin') is used exclusively by the internal tool syncer for tool-list discovery — it is never injected into real end-user requests. Theper_user_oauthresolver'sAdminConnectionHeadersand theper_user_headersresolver'sAdminConnectionHeadersboth enforce this separation at the resolver level. The partial unique indexes ensure at most one admin credential exists per MCP client, preventing accumulation of stale bootstrap credentials.Checklist
docs/contributing/README.mdand followed the guidelines