fix(auth): enforce readonly scopes by revoking stale tokens and adding client-side guard#681
Conversation
🦋 Changeset detectedLatest commit: d1ed759 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 an issue where read-only authentication sessions were not effectively enforced, allowing users to retain broader write access from previous logins. By persisting scope configurations and implementing a client-side guard, the CLI now ensures that write operations are strictly blocked during read-only sessions. Additionally, the system now proactively revokes old tokens upon scope changes to force a clean re-authentication, preventing users from inadvertently re-granting excessive permissions. 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 implements read-only session enforcement by persisting the configured scope set and adding client-side guards to the token retrieval process. It also introduces automatic token revocation and credential cleanup when scope changes are detected during login. Feedback points out a performance issue with frequent disk I/O in the authentication path, a networking bug where token revocation bypasses shared client configurations like proxies, and a logic flaw in scope comparison that may cause unnecessary re-authentications due to identity scope mismatches.
|
@jpoehnelt This is the rebased version of #520 — single clean commit on latest main, all conflicts resolved. Ready for review when you have a chance. |
…g client-side guard Rebased on main after workspace refactor (googleworkspace#613). All changes now target crates/google-workspace-cli/src/. - Persist configured scopes to scopes.json on login - Revoke old refresh token when scopes change (extracted into attempt_token_revocation helper) - Client-side scope guard in auth::get_token blocks write ops in readonly sessions (covers helpers like +send) - load_saved_scopes returns Result to surface corrupt scopes.json - Show scope_mode in auth status - Clean up scopes.json on logout - Sanitize error output via sanitize_for_terminal - Add 'profile' as non-write scope alias Fixes googleworkspace#168
gerfalcon
force-pushed
the
fix/issue-168-readonly-scope-enforcement
branch
from
b1a01e9 to
d1ed759
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements read-only scope enforcement by persisting the authorized scope set and adding client-side guards. It also introduces automatic OAuth token revocation and local credential cleanup when switching between scope sets. Feedback identifies a potential stale state issue caused by caching the session's read-only status and a logic error in the scope comparison that could lead to redundant re-authentication prompts.
| static CACHE: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| if let Some(&val) = CACHE.get() { | ||
| return Ok(val); | ||
| } | ||
| let res = load_saved_scopes() | ||
| .await? | ||
| .map(|scopes| scopes.iter().all(|s| is_non_write_scope(s))) | ||
| .unwrap_or(false); | ||
| let _ = CACHE.set(res); | ||
| Ok(res) |
There was a problem hiding this comment.
Caching the readonly status in a OnceLock can lead to stale state within the same process execution. For example, an orchestration command like gws auth setup --login might trigger a token check (caching the old state) before performing a fresh login that changes the scope mode. Since reading the small scopes.json file is negligible in terms of performance for a CLI, it is safer to remove the cache to ensure correctness.
| static CACHE: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | |
| if let Some(&val) = CACHE.get() { | |
| return Ok(val); | |
| } | |
| let res = load_saved_scopes() | |
| .await? | |
| .map(|scopes| scopes.iter().all(|s| is_non_write_scope(s))) | |
| .unwrap_or(false); | |
| let _ = CACHE.set(res); | |
| Ok(res) | |
| let res = load_saved_scopes() | |
| .await? | |
| .map(|scopes| scopes.iter().all(|s| is_non_write_scope(s))) | |
| .unwrap_or(false); | |
| Ok(res) |
| let prev_set: HashSet<&str> = prev_scopes | ||
| .iter() | ||
| .map(|s| s.as_str()) | ||
| .filter(|s| !is_non_write_scope(s) || s.ends_with(".readonly")) | ||
| .collect(); | ||
| let new_set: HashSet<&str> = scopes.iter().map(|s| s.as_str()).collect(); |
There was a problem hiding this comment.
The comparison between prev_set and new_set is inconsistent because identity scopes (like openid) are filtered out of prev_set but not new_set. If a user explicitly includes identity scopes in their login command (e.g., via --scopes), new_set will contain them while prev_set will not, triggering an unnecessary token revocation and re-authentication prompt even if the functional scopes haven't changed.
let prev_set: HashSet<&str> = prev_scopes
.iter()
.map(|s| s.as_str())
.filter(|s| !is_non_write_scope(s) || s.ends_with(".readonly"))
.collect();
let new_set: HashSet<&str> = scopes
.iter()
.map(|s| s.as_str())
.filter(|s| !is_non_write_scope(s) || s.ends_with(".readonly"))
.collect();2 participants
Summary
Fixes #168. Supersedes #520 (closed due to force-push issues).
gws auth login --readonlydoesn't actually enforce read-only access when the user previously logged in with broader scopes. The refresh token keeps its original grants, and Google ignores thescopeparam on refresh — so the token silently has write access.What this PR does
scopes.jsonon login so we can detect scope changes laterauth::get_tokenso helpers like+sendare also covered)scope_modeingws auth statusfor transparencyWhy just
prompt=consentisn't enoughGoogle's consent screen shows previously-granted scopes pre-checked. Users click "Allow" and unknowingly re-grant broad access. Revoking the token first removes the prior grant entirely.
Test plan
cargo clippy -- -D warningscleangws gmail +sendin readonly session → blocked with clear error:"This operation requires scope '...' (write access), but the current session uses read-only scopes."gws calendar +agendain readonly session → works (uses.readonlyscope)gws drive files listin readonly session → blocked (Discovery doc's first scope is broaddrive)gws auth status→ showsscope_mode: readonlyandconfigured_scopesscopes.jsonto default (write) scopes →gws gmail +send --dry-runpasses scope guardgws auth login --readonly→gws auth login(full) → prints"Scopes changed — revoked previous credentials."and re-authenticates with full scopesgws auth logout→scopes.jsonlisted inremovedand confirmed deleted