-
Notifications
You must be signed in to change notification settings - Fork 6k
[MCP] Add the ability to explicitly specify a credentials store #4857
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
18b8e7c to
072c57b
Compare
072c57b to
0f21d19
Compare
|
@codex review this |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting |
| pub mcp_servers: HashMap<String, McpServerConfig>, | ||
|
|
||
| /// Preferred store for MCP OAuth credentials. | ||
| /// keyring: Use an OS-specific keyring service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implies we are guaranteed to support only one keyring service per OS. Does that feel future-proof?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could achieve that with a second level field like keyring_provider but it's hard to know exactly what we'll want in the future here.
codex-rs/rmcp-client/src/oauth.rs
Outdated
| url: &str, | ||
| store_mode: OAuthCredentialsStoreMode, | ||
| ) -> Result<bool> { | ||
| let keyring_store = RealKeyringStore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a bit hard to follow why some functions take a KeyringStore and others inline RealKeyringStore.
I would expect taking it as a param (or expanding the trait to have more methods, which can have default implementations) to be more common.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The public one doesn't take a store, then there is an internal wrapper that takes the store as a param so the public API can pass in the default impl while tests can pass in a mock. tl;dr it's for unit tests.
This lets users/companies explicitly choose whether to force/disallow the keyring/fallback file storage for mcp credentials.
People who develop with Codex will want to use this until we sign binaries or else each ad-hoc debug builds will require keychain access on every build. I don't love this and am open to other ideas for how to handle that.
Defaults to
auto