Replies: 4 comments 4 replies
-
Hi Theo, First of all, thanks for your cogent review of the code; I'm looking forward to collaborating with you on this. A lot of what you touched on in this post is the result of an iterative development cycle during the process of implementing support for the new UI components concerning MCP management, so coming back to it now with a more retrospective view of requirements is probably wise. I'll do my best here to help explain the thought process behind the current implementation and address each of your points, but at a glance I believe your suggestions seem reasonable and could potentially help clear up the current state of the startup logs for MCP initialization, which have gotten messy and perhaps even misleading in this process. App-Level OAuth Token Logic
Your first prediction is correct; originally OAuth was handled in the console during startup, however now that it is handled in the UI, we opted to skip all detected OAuth flows at startup, with the reasoning being that OAuth connections will currently always be scoped at the user level, rather than app level. However, I know Danny has been working on granular permissions and RBAC, so I would hold off on deciding whether removal of the token retrieval logic is justified until after those changes, since I can foresee a scenario in which there might be higher level OAuth for something like a group-scoped MCP server. OAuth Discovery via Failure
Certainly from an implementation perspective, relying on explicit configuration for OAuth flagging would reduce code complexity since we would be able to remove the entirety of the discovery mechanism. I had considered this approach myself when implementing the startup: false Configuration
The github-mcp:
type: streamable-http
url: "https://api.githubcopilot.com/mcp/"
headers:
Authorization: "{{PAT_TOKEN}}"
customUserVars:
PAT_TOKEN:
title: "GitHub PAT Token"
description: "GitHub Personal Access Token" The issue at hand was that github-mcp required a user-defined PAT_TOKEN, which would be supplied post-startup from within the If we were to eliminate this flag, impact would be minimal (a single failed connection at startup which could be reinitialized from within the UI). However I would suggest keeping this flag (especially if we move forward with explicit configuration via the For your second and third questions, if we were to keep discoverability, I see the inconsistent discovery as a non-issue since, to my knowledge, these flags are used in separate non-overlapping authentication flows, and should not interfere with one another in a normal configuration (i.e. there is no configuration in which a server would both have a Hopefully this helps explain the current state of MCPs a bit more. |
Beta Was this translation helpful? Give feedback.
-
I just want to add that OAuth discovery is what the MCP inspector does and we try to follow the patterns there. I believe this is aligned with MCP spec: authorization is optional, but when auth is provided, discovery must be implemented. While a
For the Also, having a |
Beta Was this translation helpful? Give feedback.
-
@nhtruong I have moved the discussion here, do not want to piss off the reviewers :) thanks for the response. I just want to run a OAuth enabled MCP server for my internal team so that I can add more MCP servers, I am tired of trying multiple things and not able to make this work. Some pointers will definitely help. Thanks
|
Beta Was this translation helpful? Give feedback.
-
I'm closing this discussion because the issues has been resolved. @ronak21691 I see that your discussion went on in that PR instead. If it's still an issue, you should create a new discussion :) |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Hi! I've been reviewing the MCP connection management code and have some questions about the design
decisions. Looking for clarification on the intended behavior and potential simplifications.
1. App-Level OAuth Token Logic
Current behavior: App-level connections go through complex OAuth token retrieval but then immediately skip OAuth and fail at startup.
Questions:
2. OAuth Discovery via Failure
Current behavior: The system discovers OAuth requirements by attempting connections and using
failures as a side effect to populate
oauthServers
set.Questions:
requiresOAuth: true
config flag be cleaner than probing via failed connections?3. startup: false Configuration
Current behavior: Servers with
startup: false
are skipped entirely, which means they bypass OAuthdiscovery.
Questions:
startup: false
?oauthServers
setProposed Simplification
This would:
Thoughts on this approach? Happy to discuss implementation details if this direction makes sense.
Beta Was this translation helpful? Give feedback.
All reactions