fix(auth): pass all valid method scopes to authenticator#539
fix(auth): pass all valid method scopes to authenticator#539dumko2001 wants to merge 5 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 43ffb1c 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 |
googleworkspace-bot
added
the
area: core
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 application's authentication mechanism by refining how API scopes are handled. Instead of relying on a single, potentially broad scope, the system now provides all valid scopes for a given method to the authenticator. This change significantly improves the flexibility of token reuse, allowing the application to successfully authenticate with tokens that possess any of the acceptable scopes, thereby resolving issues where authentication previously failed due to scope mismatches. 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 changes the authentication scope handling to pass all available scopes for a method to the authenticator, instead of just the first one. The goal is to improve token reuse. While the code change is straightforward, it appears to re-introduce a previously addressed issue where including restrictive scopes alongside broader ones can lead to unexpected API behavior. I've added a critical comment requesting clarification on why this is no longer a concern, as it could represent a significant regression.
src/main.rs
Outdated
| // Select the scopes for the method. Discovery Documents list scopes as | ||
| // alternatives (any one grants access). We pass all of them so the authenticator | ||
| // can find a cached token that satisfies any of these alternatives. | ||
| let scopes: Vec<&str> = method.scopes.iter().map(|s| s.as_str()).collect(); |
There was a problem hiding this comment.
This change passes all available scopes to the authenticator, which differs from the previous implementation that deliberately selected only the first (usually broadest) scope.
The removed select_scope function and associated comments highlighted a key reason for this:
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.
This change appears to re-introduce this potential issue. For example, if an API call is made with a token that has both a broad scope (e.g., gmail.readonly) and a restrictive scope (gmail.metadata), the API might enforce the limitations of the more restrictive scope, causing unexpected failures for operations that should be allowed by the broader scope.
Could you clarify why this is no longer a concern? Has the underlying Google API behavior changed, or is there another mechanism now in place to mitigate this problem? Without addressing this, the change risks introducing a significant regression for some APIs.
dumko2001
force-pushed
the
fix/issue-519-multi-scope-v2
branch
from
c10b7e4 to
a66de00
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the scope selection logic by introducing a priority-based heuristic. The implementation has a small logical issue with a redundant fallback and can be made more robust and idiomatic. I've suggested a refactoring of the select_scope function to improve its clarity, correctness, and robustness.
| 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())) | ||
| } |
There was a problem hiding this comment.
This function can be simplified and made more robust.
- The
or_elsecall at the end is redundant. If thescopesslice is not empty,best_scopewill always beSomebecausebest_priorityis initialized to 100 and all possible priorities are lower. This makes theor_elseclosure unreachable. - The logic can be expressed more idiomatically and concisely using
iter().min_by_key(). - Using
ends_withfor.readonlyand.metadatais more robust thancontains, 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())
}
|
Reverted the approach of passing all scopes simultaneously, which could trigger unwanted permission requests and exact-set matching issues in |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the scope selection logic to be more intelligent, preferring narrower and more common scopes over broad or restrictive ones. This is a good improvement that should fix the issue with token cache misses. The implementation in select_scope is sound, but it can be made more concise and idiomatic using Rust's iterator methods. I've suggested a refactoring that simplifies the code and removes a redundant or_else call. The new tests correctly validate the intended behavior.
src/main.rs
Outdated
| 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())) |
There was a problem hiding this comment.
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())
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a more sophisticated heuristic for selecting authentication scopes, which is a great improvement over picking the first one available. My review focuses on a potential robustness issue in the new logic. The use of contains for checking scope suffixes like .readonly could lead to incorrect prioritization. I've suggested a refactoring to use ends_with for more precise matching, which also simplifies the function to be more idiomatic Rust and removes some redundant code.
| 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())) | ||
| } |
There was a problem hiding this comment.
The current implementation for selecting a scope has a potential robustness issue and can be simplified.
-
Potential Bug: Using
scope.contains(".readonly")andscope.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 likeexample.readonly.but.not.reallywould be miscategorized. Usingends_withwould be more precise for these cases. -
Redundancy: The
or_elsecall on line 336 is redundant. Given the initialbest_priorityof 100 and the priority values assigned,best_scopewill always beSomeif the inputscopesslice is not empty, making the fallback toscopes.first()unnecessary. -
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())
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the OAuth scope selection by introducing a priority-based heuristic. This is a good improvement over the previous implementation which simply picked the first available scope. The new logic prefers more specific and common scopes like .readonly, which is beneficial for security and user experience. I've suggested one improvement to the heuristic to better differentiate between specific scopes and broad alias scopes, further strengthening the principle of least privilege.
src/main.rs
Outdated
| let priority = if s.contains(".readonly") { | ||
| 1 // Most compatible with typical user logins | ||
| } else if s.contains(".metadata") { | ||
| 10 // Restrictive, avoid if broader is available | ||
| } else if s.contains("cloud-platform") { | ||
| 50 // Extremely broad, avoid if possible | ||
| } else { | ||
| 5 // Standard service scopes (e.g., drive, gmail.modify) | ||
| }; |
There was a problem hiding this comment.
The current heuristic for "Standard service scopes" gives the same priority (5) to both specific scopes like .../gmail.modify and very broad alias scopes like https://mail.google.com/. Because min_by_key is stable, if a broad alias appears before a more specific scope with the same priority, the broader one will be chosen. This undermines the principle of least privilege that this PR aims to improve.
Consider differentiating these broad alias scopes, which often end in a /, by giving them a lower preference (a higher priority number) than standard specific scopes. This will make the scope selection more robust and ensure the most appropriate, least-privileged scope is chosen.
let priority = if s.contains(".readonly") {
1 // Most compatible with typical user logins
} else if s.contains(".metadata") {
10 // Restrictive, avoid if broader is available
} else if s.ends_with('/') {
20 // Broad alias scopes (e.g., https://mail.google.com/), prefer specific scopes
} else if s.contains("cloud-platform") {
50 // Extremely broad, avoid if possible
} else {
5 // Standard service scopes (e.g., drive, gmail.modify)
};
|
Refined the scope selection heuristic to use |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the scope selection heuristic by implementing a priority-based choice among method alternatives. The new select_scope function correctly prioritizes narrower, more common scopes like .readonly while avoiding overly restrictive or broad scopes when better alternatives are available. The updated tests adequately cover the new logic, ensuring its correctness. This change directly addresses the issue of scope selection and enhances compatibility with token caching, which is a valuable improvement.
2 participants
Description
Improve scope selection heuristic by implementing a priority-based choice among method alternatives. This prefers narrower, more common scopes (like
.readonly) which are more likely to be present in the user's token cache (fixing #519), while still avoiding restrictive.metadatascopes when broader alternatives are available.This approach is more robust than passing all scopes to the authenticator, as it avoids requesting unnecessary permissions and stays compatible with
yup-oauth2's exact-set matching for cached tokens.Fixes #519
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.