Skip to content

fix: use a catalog entry's static OAuth credentials when generating tool previews - #6831

Open
we4sz wants to merge 1 commit into
obot-platform:mainfrom
we4sz:fix/preview-oauth-entry-creds-main
Open

fix: use a catalog entry's static OAuth credentials when generating tool previews#6831
we4sz wants to merge 1 commit into
obot-platform:mainfrom
we4sz:fix/preview-oauth-entry-creds-main

Conversation

@we4sz

@we4sz we4sz commented Jun 4, 2026

Copy link
Copy Markdown

Generating tool previews for an OAuth-protected remote catalog entry fails even when the entry has static OAuth credentials configured.

Problem

GenerateToolPreviews launches an ephemeral, never-persisted MCP server (tool-preview-<hash>) and connects to it to enumerate tools. For an OAuth-protected remote server this should use the catalog entry's configured static OAuth client, but it never can:

  • tempServerAndConfig builds the temp server without setting Spec.MCPServerCatalogEntryName.
  • The OAuth ClientCredLookup (mcpOAuthHandler.Lookup) resolves the static client by fetching the server from storage by name and reading MCPServerCatalogEntryName off it. The temp preview server isn't persisted, so that Get fails, Lookup returns "no credentials found", and the client falls back to Dynamic Client Registration — which strict IdPs (e.g. Keycloak) reject (invalid_client_metadata), surfacing as a 500.

Fix

  • Set MCPServerCatalogEntryName on the temp preview server (all tempServerAndConfig call sites pass the entry / component catalog-entry name).
  • Carry the entry name onto mcpOAuthHandler (set in CheckForMCPAuth from the passed mcpServer).
  • Lookup uses the carried entry name to resolve the static OAuth credentials directly, falling back to the storage lookup for persisted servers.

Testing

Built locally and verified against an OAuth-protected remote server (Keycloak-fronted) with static OAuth credentials configured on the catalog entry: "Populate Tool Preview" now resolves the entry's static client, performs the normal login flow, and lists tools — instead of falling back to DCR and returning a 500.

gofmt clean; affected packages build.

Copilot AI review requested due to automatic review settings June 4, 2026 16:46
Copilot AI reviewed Jun 4, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds support for resolving static OAuth credentials for ephemeral “tool preview” MCP servers by carrying the originating catalog entry name through preview server creation and OAuth lookup.

Changes:

  • Pass catalog entry name/ID into tempServerAndConfig and stamp it onto the temporary MCPServer spec.
  • Carry MCPServerCatalogEntryName into the OAuth handler so credentials can be resolved without fetching a persisted server.
  • Update OAuth credential lookup to prefer the carried catalog entry name, with fallback to reading it from a persisted server.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
pkg/api/handlers/mcpgateway/oauth/mcpoauthhandler.go Carries catalog entry name into the OAuth handler and uses it to resolve static OAuth credentials.
pkg/api/handlers/mcpcatalogs.go Threads catalog entry name into temp preview server creation so OAuth lookup can resolve credentials for non-persisted servers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/api/handlers/mcpcatalogs.go
Comment on lines 943 to 949
req.ObotNamespace,
entry.Namespace,
catalogName,
componentEntry.CatalogEntryID,
componentEntry.Manifest,
config.Config,
config.URL,
Comment thread pkg/api/handlers/mcpcatalogs.go Outdated
}

func tempServerAndConfig(ctx context.Context, gatewayClient *gclient.Client, client client.Client, localK8sClient client.Client, obotNamespace, namespace, catalogName string, entryManifest types.MCPServerCatalogEntryManifest, config map[string]string, url, baseURL string) (v1.MCPServer, mcp.ServerConfig, error) {
func tempServerAndConfig(ctx context.Context, gatewayClient *gclient.Client, client client.Client, localK8sClient client.Client, obotNamespace, namespace, catalogName, catalogEntryName string, entryManifest types.MCPServerCatalogEntryManifest, config map[string]string, url, baseURL string) (v1.MCPServer, mcp.ServerConfig, error) {
Comment thread pkg/api/handlers/mcpgateway/oauth/mcpoauthhandler.go Outdated
Comment on lines 140 to 144
gatewayClient *client.Client
stateMgr *stateManager
mcpID string
catalogEntryName string
mcpURL string
@we4sz
fix: use a catalog entry's static OAuth credentials when generating t…
db6abcf 
…ool previews

GenerateToolPreviews launches an ephemeral, never-persisted MCP server
(tool-preview-<hash>) to enumerate tools. For an OAuth-protected remote server
this needs the catalog entry's configured static OAuth client, but it could
never be found: the temp server had no MCPServerCatalogEntryName, and the OAuth
ClientCredLookup resolves the static client by fetching the server from storage
by name and reading that field off it. Since the temp server isn't persisted,
the lookup returned 'no credentials found' and fell back to Dynamic Client
Registration, which strict IdPs (e.g. Keycloak) reject.

Set MCPServerCatalogEntryName on the temp preview server, carry it on the OAuth
handler (from the passed mcpServer), and have Lookup use the carried entry name
directly, falling back to the storage lookup for persisted servers.
@we4sz
we4sz force-pushed the fix/preview-oauth-entry-creds-main branch from 5805ea6 to db6abcf Compare June 4, 2026 16:54
@we4sz

we4sz commented Jun 4, 2026

Copy link
Copy Markdown
Author

Thanks for the review. Addressing the three points:

  1. CatalogEntryID vs catalog entry name (composite sites): these are the same value in this codebase — CatalogEntryID is the catalog entry’s object name. Obot itself stamps it onto the spec field this resolves against (pkg/controller/handlers/mcpserver/mcpserver.go: MCPServerCatalogEntryName: component.CatalogEntryID) and Gets the entry by CatalogEntryID, so system.MCPOAuthCredentialName(...) resolves correctly for component previews. The single-server sites pass entry.Name. No change needed.

  2. Long positional parameter list: agreed it’s not ideal, but tempServerAndConfig already took 11 positional args before this change; I kept the fix minimal and focused rather than expanding its scope. Happy to follow up with a params-struct refactor in a separate PR if you’d prefer.

  3. Field/param naming: renamed catalogEntryName → mcpServerCatalogEntryName to align with the MCPServerCatalogEntryName spec field and avoid confusion with catalogName. Pushed.

gofmt clean; affected packages build. Verified end-to-end against a Keycloak-fronted remote server with static OAuth configured on the entry — tool previews now resolve the entry’s static client and complete the login flow instead of falling back to DCR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@we4sz