Allowing command line authentication with JSON copy-and-paste #210
Conversation
Summary of ChangesHello @karmel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a new command-line authentication flow, enabling users to authenticate with the MCP server by manually copying JSON credentials after visiting an authentication URL. This is particularly useful in environments where automatic browser launching is not supported. The changes involve extending the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new command-line authentication flow using JSON copy-and-paste, which is a useful addition for CLI tools. The implementation involves refactoring the authentication logic in AuthManager and adding a new auth.login tool. The code is generally well-structured. I've identified a high-severity issue where the token refresh listener is not attached in the new flow, which could lead to authentication failures. I've also included a couple of medium-severity suggestions to improve input validation and error handling for a better user experience. Overall, these changes are a good step forward, and with the suggested fixes, the new authentication mechanism will be more robust.
| this.client = new google.auth.OAuth2(CLIENT_ID); | ||
| this.client.setCredentials(tokens); | ||
| await OAuthCredentialStorage.saveCredentials(tokens); |
There was a problem hiding this comment.
The new OAuth2Client created here does not have the 'tokens' event listener attached. This listener, configured in getAuthenticatedClient, is crucial for persisting auto-refreshed tokens. Without it, if an access token expires and is auto-refreshed by the library, the new token won't be saved, leading to authentication failures on subsequent runs.
A better approach is to save the credentials and then invalidate the cached client. This ensures that the next call to getAuthenticatedClient will create a new, properly configured client with the listener attached, using the credentials just saved.
await OAuthCredentialStorage.saveCredentials(tokens);
// Invalidate the cached client. The next call to getAuthenticatedClient()
// will create a new client with the new credentials and the 'tokens' listener.
this.client = null;| const tokens = JSON.parse(jsonStr); | ||
| // Validate input has required fields | ||
| if (!tokens.refresh_token || !tokens.access_token) { | ||
| throw new Error('Invalid credentials JSON: missing required fields'); | ||
| } |
There was a problem hiding this comment.
The validation for the credentials JSON is basic. It only checks for the existence of refresh_token and access_token but doesn't validate their types or ensure they are non-empty. Since the project already uses zod, it would be more robust and consistent to define a schema for the credentials and parse the JSON against it. This would provide stronger type safety and clearer error messages for invalid input.
You'll need to add import { z } from 'zod'; at the top of the file.
| const tokens = JSON.parse(jsonStr); | |
| // Validate input has required fields | |
| if (!tokens.refresh_token || !tokens.access_token) { | |
| throw new Error('Invalid credentials JSON: missing required fields'); | |
| } | |
| const credentialsSchema = z.object({ | |
| refresh_token: z.string().min(1, { message: 'refresh_token cannot be empty' }), | |
| access_token: z.string().min(1, { message: 'access_token cannot be empty' }), | |
| scope: z.string().optional(), | |
| token_type: z.string().optional(), | |
| expiry_date: z.number().optional(), | |
| }); | |
| const tokens = credentialsSchema.parse(JSON.parse(jsonStr)); |
| if (input.credentialsJson) { | ||
| // Complete the flow | ||
| await authManager.saveCredentialsFromJson(input.credentialsJson); | ||
| return { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: 'Authentication successful! Credentials saved.', | ||
| }, | ||
| ], | ||
| }; | ||
| } |
There was a problem hiding this comment.
The call to authManager.saveCredentialsFromJson is not wrapped in a try...catch block. If the user provides invalid JSON, saveCredentialsFromJson will throw an error, which will likely result in a generic server error response. To improve user experience, it would be better to catch this specific error and return a user-friendly message explaining that the provided JSON was invalid.
if (input.credentialsJson) {
try {
// Complete the flow
await authManager.saveCredentialsFromJson(input.credentialsJson);
return {
content: [
{
type: 'text',
text: 'Authentication successful! Credentials saved.',
},
],
};
} catch (error) {
const errorMessage = error instanceof Error ? error.message : 'Invalid JSON provided.';
return {
content: [
{
type: 'text',
text: `Authentication failed: ${errorMessage}`,
},
],
};
}
}
|
also see #213 😄 |
2 participants
This allows me to run the MCP server from within a command line tool and still auth-- a link is presented to the user, the user visits the link and is forwarded to a json blob which the user then pastes back at the command line.
This code was written by Antigravity, and it worked, so I didn't look too closely :) Take what's useful, or ignore.