Skip to content

fix(auth): pass all valid method scopes to authenticator #539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dumko2001 wants to merge 5 commits into from
Closed

fix(auth): pass all valid method scopes to authenticator #539

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a66de00
fix(auth): improve scope selection heuristic (fixes #519)
dumko2001 Mar 18, 2026
743fe92
chore: add changeset for scope negotiation fix
dumko2001 Mar 18, 2026
5f2ed39
chore: correct changeset package name
dumko2001 Mar 18, 2026
1ea7a09
refactor(auth): simplify scope selection heuristic
dumko2001 Mar 18, 2026
43ffb1c
refactor(auth): refine scope selection heuristic using ends_with
dumko2001 Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .changeset/fix-issue-519-multi-scope.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
---\n"@googleworkspace/cli": patch\n---\n\nfix(auth): improve scope selection heuristic to prefer standard/readonly scopes over restrictive ones
60 changes: 52 additions & 8 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,15 +298,42 @@ async fn run() -> Result<(), GwsError> {
.map(|_| ())
}

/// Select the best scope from a method's scope list.
/// Select the best scope for the method from its list of alternatives.
///
/// Discovery Documents list method scopes as alternatives — any single scope
/// grants access. The first scope is typically the broadest. Using all scopes
/// causes issues when restrictive scopes (e.g., `gmail.metadata`) are included,
/// as the API enforces that scope's restrictions even when broader scopes are
/// also present.
/// grants access. We pick the most appropriate one based on a heuristic:
/// 1. Prefer narrower scopes (e.g., `.readonly`) as they are more likely to
/// be present in the token cache (fixes #519).
/// 2. Avoid restrictive scopes (e.g., `.metadata`) if broader alternatives
/// are available, as the API may enforce the most restrictive scope's
/// limitations even when broader ones are present.
pub(crate) fn select_scope(scopes: &[String]) -> Option<&str> {
scopes.first().map(|s| s.as_str())
if scopes.is_empty() {
return None;
}

let mut best_scope: Option<&str> = None;
let mut best_priority = 100;

for scope in scopes {
// Priority mapping (lower is better)
let priority = if scope.contains(".readonly") {
1 // Most compatible with typical user logins
} else if scope.contains(".metadata") {
10 // Restrictive, avoid if broader is available
} else if scope.contains("cloud-platform") {
50 // Extremely broad, avoid if possible
} else {
5 // Standard service scopes (e.g., drive, gmail.modify)
};

if priority < best_priority {
best_priority = priority;
best_scope = Some(scope.as_str());
}
}

best_scope.or_else(|| scopes.first().map(|s| s.as_str()))
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This implementation can be simplified using Iterator::min_by_key to make it more idiomatic and concise. This approach also naturally handles an empty scopes slice by returning None, removing the need for an explicit check.

Additionally, the or_else call in the original implementation is redundant. If scopes is not empty, best_scope is guaranteed to be Some, so the closure in or_else would never be executed.

For improved readability and maintainability, consider defining the priority values as constants.

    scopes
        .iter()
        .min_by_key(|scope| {
            // Priority mapping (lower is better)
            if scope.contains(".readonly") {
                1 // Most compatible with typical user logins
            } else if scope.contains(".metadata") {
                10 // Restrictive, avoid if broader is available
            } else if scope.contains("cloud-platform") {
                50 // Extremely broad, avoid if possible
            } else {
                5 // Standard service scopes (e.g., drive, gmail.modify)
            }
        })
        .map(|s| s.as_str())

}
Comment on lines 310 to 329
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This function can be simplified and made more robust.

  1. The or_else call at the end is redundant. If the scopes slice is not empty, best_scope will always be Some because best_priority is initialized to 100 and all possible priorities are lower. This makes the or_else closure unreachable.
  2. The logic can be expressed more idiomatically and concisely using iter().min_by_key().
  3. Using ends_with for .readonly and .metadata is more robust than contains, as it prevents accidental matches on substrings that are not at the end of the scope string.

Here is a suggested implementation that addresses these points:

pub(crate) fn select_scope(scopes: &[String]) -> Option<&str> {
    scopes
        .iter()
        .min_by_key(|scope| {
            // Using `ends_with` is more robust for suffixes like `.readonly` and `.metadata`
            // to avoid accidentally matching them in the middle of a scope segment.
            match scope.as_str() {
                s if s.ends_with(".readonly") => 1, // Most compatible with typical user logins
                s if s.ends_with(".metadata") => 10, // Restrictive, avoid if broader is available
                s if s.contains("cloud-platform") => 50, // Extremely broad, avoid if possible
                _ => 5, // Standard service scopes
            }
        })
        .map(|s| s.as_str())
}

Comment on lines 310 to 329
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current implementation for selecting a scope has a potential robustness issue and can be simplified.

  1. Potential Bug: Using scope.contains(".readonly") and scope.contains(".metadata") is not robust. It could incorrectly match scopes that contain these substrings but don't end with them, leading to incorrect priority assignment. For example, a scope like example.readonly.but.not.really would be miscategorized. Using ends_with would be more precise for these cases.

  2. Redundancy: The or_else call on line 336 is redundant. Given the initial best_priority of 100 and the priority values assigned, best_scope will always be Some if the input scopes slice is not empty, making the fallback to scopes.first() unnecessary.

  3. Idiomatic Rust: The function can be written more idiomatically using iterators, which would make it more concise, remove the need for mutable state, and address the other points.

Here is a suggested refactoring that addresses these points:

pub(crate) fn select_scope(scopes: &[String]) -> Option<&str> {
    scopes
        .iter()
        .min_by_key(|scope| {
            if scope.ends_with(".readonly") {
                1 // Most compatible with typical user logins
            } else if scope.ends_with(".metadata") {
                10 // Restrictive, avoid if broader is available
            } else if scope.contains("cloud-platform") {
                50 // Extremely broad, avoid if possible
            } else {
                5 // Standard service scopes (e.g., drive, gmail.modify)
            }
        })
        .map(|s| s.as_str())
}


fn parse_pagination_config(matches: &clap::ArgMatches) -> executor::PaginationConfig {
Expand Down Expand Up @@ -710,14 +737,31 @@ mod tests {
}

#[test]
fn test_select_scope_picks_first() {
fn test_select_scope_prefers_readonly() {
let scopes = vec![
"https://mail.google.com/".to_string(),
"https://www.googleapis.com/auth/gmail.metadata".to_string(),
"https://www.googleapis.com/auth/gmail.modify".to_string(),
"https://www.googleapis.com/auth/gmail.readonly".to_string(),
];
assert_eq!(select_scope(&scopes), Some("https://mail.google.com/"));
// .readonly should be preferred over the first one (mail.google.com)
assert_eq!(
select_scope(&scopes),
Some("https://www.googleapis.com/auth/gmail.readonly")
);
}

#[test]
fn test_select_scope_avoids_metadata() {
let scopes = vec![
"https://www.googleapis.com/auth/gmail.metadata".to_string(),
"https://www.googleapis.com/auth/gmail.modify".to_string(),
];
// .modify should be preferred over .metadata
assert_eq!(
select_scope(&scopes),
Some("https://www.googleapis.com/auth/gmail.modify")
);
}

#[test]
Expand Down
Loading