fix(auth): show env credential overrides#811
Conversation
|
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 enhances the 'gws auth status' command to provide better transparency regarding OAuth credential sources. By surfacing environment variable overrides and masking sensitive identifiers, it helps users diagnose configuration issues where environment variables might unintentionally take precedence over local files. 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 the 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 counterproductive. 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 updates the auth status command to include information about OAuth environment variable overrides. It introduces the add_oauth_env_status_fields function to check for GOOGLE_WORKSPACE_CLI_CLIENT_ID and GOOGLE_WORKSPACE_CLI_CLIENT_SECRET, along with a unit test. A review comment identifies an inconsistency where empty environment variables are filtered out in the status check but not in the actual credential resolution logic, which could lead to misleading status reports.
| let env_client_id = std::env::var("GOOGLE_WORKSPACE_CLI_CLIENT_ID") | ||
| .ok() | ||
| .filter(|value| !value.is_empty()); | ||
| let env_client_secret_set = std::env::var("GOOGLE_WORKSPACE_CLI_CLIENT_SECRET") | ||
| .ok() | ||
| .filter(|value| !value.is_empty()) | ||
| .is_some(); |
There was a problem hiding this comment.
The environment variable check here is inconsistent with the actual credential resolution logic in resolve_client_credentials (lines 718-750). Specifically, resolve_client_credentials does not filter out empty strings, meaning it will attempt to use them if they are set in the environment. By filtering them here, gws auth status will report that no environment overrides are active even when the application is actually using them (albeit likely failing later). To ensure the status output accurately reflects the application's state, these filters should be removed, or resolve_client_credentials should be updated to also ignore empty strings.
let env_client_id = std::env::var("GOOGLE_WORKSPACE_CLI_CLIENT_ID").ok();
let env_client_secret_set = std::env::var("GOOGLE_WORKSPACE_CLI_CLIENT_SECRET").is_ok();
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves OAuth credential handling by ignoring empty environment variables and enhances the status command to report environment variable overrides. Review feedback highlights an opportunity to refactor duplicated environment variable resolution logic into a shared helper to ensure consistency. Additionally, a logic inconsistency was identified in the status command where the existing credential source detection does not yet account for empty environment variables, which could lead to inaccurate reporting.
| let env_id = std::env::var("GOOGLE_WORKSPACE_CLI_CLIENT_ID") | ||
| .ok() | ||
| .filter(|value| !value.is_empty()); | ||
| let env_secret = std::env::var("GOOGLE_WORKSPACE_CLI_CLIENT_SECRET") | ||
| .ok() | ||
| .filter(|value| !value.is_empty()); |
There was a problem hiding this comment.
The logic for resolving credentials from environment variables is now duplicated in add_oauth_env_status_fields. To ensure consistency and improve maintainability, consider refactoring this into a shared helper function. This is particularly important for security-sensitive environment variables where the definition of "set" (e.g., handling empty vs. whitespace-only strings) should be uniform across the application.
| } | ||
| } | ||
|
|
||
| add_oauth_env_status_fields(&mut output, has_config); |
There was a problem hiding this comment.
The addition of add_oauth_env_status_fields introduces a logic inconsistency with the credential_source detection later in this function (lines 1281-1284 in the full file). While the new status fields correctly filter out empty environment variables, the credential_source logic still uses is_ok(), which will incorrectly report environment_variables as the source if the variables are present but empty. This directly contradicts the PR's goal of accurately showing environment overrides.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors OAuth credential resolution by introducing the oauth_client_env_credentials helper function, which ensures that empty environment variables are ignored. It also updates the status command to include information about OAuth environment variables in the output, including masked client IDs. Comprehensive unit tests were added to validate the new logic and ensure correct handling of empty strings. I have no feedback to provide.
2 participants
Summary
gws auth statusenv_overrides_client_configflag when env credentials shadowclient_secret.jsonFixes #715
Tests