fix(auth): report token info in status when using GOOGLE_WORKSPACE_CLI_TOKEN#546
fix(auth): report token info in status when using GOOGLE_WORKSPACE_CLI_TOKEN#546dumko2001 wants to merge 5 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 07beab6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, 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 addresses a previous limitation in the Highlights
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. 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 correctly adds support for GOOGLE_WORKSPACE_CLI_TOKEN in the gws auth status command. However, the refactoring has introduced a regression where error reporting for failed token refreshes (e.g., due to an expired refresh token) is lost. My review includes a suggestion to restore this important feedback for the user.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the authentication status command by allowing it to use an access token provided directly via the GOOGLE_WORKSPACE_CLI_TOKEN environment variable. The handle_status function in src/auth_commands.rs was modified to prioritize this environment variable for obtaining an access token. If the environment variable is not set, the existing logic for loading credentials and exchanging a refresh token is used. Subsequently, the retrieved access token (from either source) is used to fetch user information and granted scopes from Google APIs, improving clarity in the reported authentication status.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability for the gws auth status command to report token information when an access token is provided via the GOOGLE_WORKSPACE_CLI_TOKEN environment variable, in addition to existing credential store methods. However, a regression was introduced where the error reporting for failed refresh token exchanges was removed. Previously, the system would capture and display error_description and set token_valid: false, but now, if the exchange fails, the access_token is simply None, leading to silent failures and potentially confusing user output.
|
Restored the error reporting for failed token refresh in |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly adds support for GOOGLE_WORKSPACE_CLI_TOKEN to the auth status command and refactors the logic to handle both direct tokens and tokens from credentials. My review includes two high-severity suggestions: one to improve performance by reusing the reqwest::Client instance, and another to improve correctness by properly URL-encoding the access token in the tokeninfo request.
src/auth_commands.rs
Outdated
| let tokeninfo_url = format!("https://oauth2.googleapis.com/tokeninfo?access_token={}", at); | ||
| if let Ok(info_resp) = http_client.get(&tokeninfo_url).send().await { |
There was a problem hiding this comment.
The access token is being directly embedded into the URL string. While Google access tokens are typically URL-safe, it's best practice to URL-encode parameters to prevent potential issues if a token ever contains special characters. Using reqwest's .query() method handles this encoding for you automatically and makes the code more robust.
if let Ok(info_resp) = http_client
.get("https://oauth2.googleapis.com/tokeninfo")
.query(&[("access_token", &at)])
.send()
.await
{
|
Refactored |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly adds support for GOOGLE_WORKSPACE_CLI_TOKEN in the gws auth status command, allowing it to report token information when this environment variable is used. The implementation is functional, but I've identified a significant maintainability issue in src/auth_commands.rs where the logic for retrieving the access token has become overly complex and includes side effects within a let binding. I've left a comment with a recommendation to refactor this for better clarity and separation of concerns.
dumko2001
force-pushed
the
fix/auth-status-token-v2
branch
from
960d2b9 to
07beab6
Compare
|
Refactored the access token retrieval logic into a separate |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly adds support for GOOGLE_WORKSPACE_CLI_TOKEN to the auth status command by refactoring the token acquisition logic. My review identifies two high-severity issues: a performance regression due to using synchronous file I/O in an async context, and a bug where a JSON parsing error is not handled, leading to silent failure. Code suggestions are provided for both issues.
| let creds_json_str = if enc_path.exists() { | ||
| credential_store::load_encrypted().ok() | ||
| } else if plain_path.exists() { | ||
| std::fs::read_to_string(&plain_path).ok() | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
This implementation uses a synchronous file read (std::fs::read_to_string) within an async function. This blocks the execution thread and is a performance regression from the previous implementation which used tokio::fs::read_to_string. To avoid blocking, you should use the asynchronous version and restructure the logic slightly. Note that credential_store::load_encrypted() is also synchronous, but this suggestion focuses on the immediate regression introduced in this change.
| let creds_json_str = if enc_path.exists() { | |
| credential_store::load_encrypted().ok() | |
| } else if plain_path.exists() { | |
| std::fs::read_to_string(&plain_path).ok() | |
| } else { | |
| None | |
| }; | |
| let mut creds_json_str = if enc_path.exists() { | |
| credential_store::load_encrypted().ok() | |
| } else { | |
| None | |
| }; | |
| if creds_json_str.is_none() && plain_path.exists() { | |
| creds_json_str = tokio::fs::read_to_string(&plain_path).await.ok(); | |
| } |
| if let Ok(token_json) = resp.json::<serde_json::Value>().await { | ||
| if let Some(access_token) = token_json.get("access_token").and_then(|v| v.as_str()) { | ||
| Some(access_token.to_string()) | ||
| } else { | ||
| output["token_valid"] = serde_json::json!(false); | ||
| if let Some(err) = token_json.get("error_description").and_then(|v| v.as_str()) { | ||
| output["token_error"] = serde_json::json!(err); | ||
| } | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
There's a missing error handling case here. If resp.json::<serde_json::Value>().await fails (e.g., due to a malformed response from the server or a network issue), the else branch on line 979 is taken, which returns None without updating the output JSON value. This means the user will not see any error message and might be confused about why their authentication status is not being reported correctly. You should handle this error case and populate token_valid and token_error in the output.
| if let Ok(token_json) = resp.json::<serde_json::Value>().await { | |
| if let Some(access_token) = token_json.get("access_token").and_then(|v| v.as_str()) { | |
| Some(access_token.to_string()) | |
| } else { | |
| output["token_valid"] = serde_json::json!(false); | |
| if let Some(err) = token_json.get("error_description").and_then(|v| v.as_str()) { | |
| output["token_error"] = serde_json::json!(err); | |
| } | |
| None | |
| } | |
| } else { | |
| None | |
| } | |
| match resp.json::<serde_json::Value>().await { | |
| Ok(token_json) => { | |
| if let Some(access_token) = token_json.get("access_token").and_then(|v| v.as_str()) { | |
| Some(access_token.to_string()) | |
| } else { | |
| output["token_valid"] = serde_json::json!(false); | |
| if let Some(err) = token_json.get("error_description").and_then(|v| v.as_str()) { | |
| output["token_error"] = serde_json::json!(err); | |
| } | |
| None | |
| } | |
| } | |
| Err(e) => { | |
| output["token_valid"] = serde_json::json!(false); | |
| output["token_error"] = serde_json::json!(format!("Failed to parse token response: {e}")); | |
| None | |
| } | |
| } |
2 participants
Description
Ensure that
gws auth statuscorrectly reports the validity, user, and scopes of an access token provided via theGOOGLE_WORKSPACE_CLI_TOKENenvironment variable. Previously, the status command only checked stored credentials, which could be confusing when using direct token injection.Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.