fix: gracefully fallback on legacy credentials instead of bailing#173
fix: gracefully fallback on legacy credentials instead of bailing#173haunchen wants to merge 2 commits intogoogleworkspace:mainfrom
Conversation
…oses googleworkspace#156, googleworkspace#151, googleworkspace#137) resolve_account() previously bailed with an error when legacy credentials.enc existed without accounts.json, creating a deadlock where login output was rejected by auth. Now returns Ok(None) so get_token falls back to the legacy file directly. Also adds stderr warnings when userinfo email fetch fails and when credentials are saved in legacy format, improving debuggability.
🦋 Changeset detectedLatest commit: 0fa605e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 critical deadlock scenario where the CLI would fail to operate if legacy credentials ( Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request addresses a deadlock issue in the gws auth login command when legacy credentials exist without an accounts.json file. The changes include modifying resolve_account() to gracefully fallback to legacy credentials, adding a warning message in handle_login() when credentials are saved in legacy format, and adding stderr logging in fetch_userinfo_email() for HTTP client build failures and non-success status codes. A new test case is also added to verify the behavior of resolve_account() with legacy credentials. The changes improve the user experience by preventing the deadlock and providing informative messages when legacy credentials are used or when email fetching fails.
I am having trouble creating individual review comments. Click here to see my feedback.
src/auth.rs (138-143)
This anyhow::bail! will immediately exit the function with an error, which is what the PR is trying to avoid. The new code changes this to a warning message, which is an improvement.
src/auth_commands.rs (407-409)
Returning None without logging the error makes it difficult to diagnose issues. The added logging improves error visibility.
src/auth_commands.rs (414-417)
Adding a warning log here is a good way to notify the user that the email could not be determined. This provides better feedback to the user.
src/auth_commands.rs (417)
Returning None without logging the HTTP status code makes it difficult to diagnose issues. The added logging improves error visibility.
src/auth_commands.rs (426-430)
Adding a warning log here is a good way to notify the user that the email could not be determined. This provides better feedback to the user.
|
/gemini review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #173 +/- ##
==========================================
+ Coverage 55.19% 55.36% +0.16%
==========================================
Files 38 38
Lines 13166 13187 +21
==========================================
+ Hits 7267 7301 +34
+ Misses 5899 5886 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request addresses a deadlock issue with legacy credentials by gracefully falling back instead of erroring out. The changes involve updating resolve_account to print a note and return Ok(None), and adding warnings in handle_login and more detailed error logging in fetch_userinfo_email when user info cannot be fetched. A new test case is also added to verify the new behavior for legacy credentials. The changes look good and effectively solve the reported issues. I have one suggestion regarding the new test to improve its robustness.
Note: Security Review did not run due to the size of the PR.
Preserve the original GOOGLE_WORKSPACE_CLI_CONFIG_DIR value before modifying it in the test, consistent with other tests in the file.
Summary
Fixes #156, #151, #137
gws auth loginwithout--accountcallsfetch_userinfo_email()to get the user's email. If this fails (e.g., scope insufficient, network issue), credentials are saved as legacycredentials.encwithoutaccounts.json. Subsequently,resolve_account()bails with "Legacy credentials found" — creating a deadlock where login output is rejected by auth.Multiple users reported this: login succeeds with
"account": "(unknown)", then all API calls return 401.Changes
resolve_account(): Replacebail!on legacycredentials.encwitheprintlnnote +Ok(None), soget_tokenfalls back to the legacy file directlyhandle_login(): Add warning when credentials are saved in legacy format (email unavailable)fetch_userinfo_email(): Add stderr logging on HTTP client build failure and non-success status codestest_resolve_account_legacy_credentials_returns_none— verifiescredentials.encwithoutaccounts.jsonreturnsOk(None), notErrTest plan
cargo test— 413 tests passcargo clippy -- -D warnings— no warningsaccounts.json, keepcredentials.enc, rungws drive files list --params '{"pageSize":1}'— should succeed with a note about legacy formatgws auth loginwithout--account— should warn about legacy format if email fetch fails