fix(auth): include Gmail settings scope#794
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 updates the Google Workspace CLI authentication configuration to include the 'gmail.settings.basic' scope. This change ensures that users have the necessary permissions to access basic Gmail settings, and includes corresponding updates to the manual scope picker and verification tests to maintain configuration integrity. 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 adds the gmail.settings.basic scope to the minimal and full scope definitions, the scope registry, and associated tests. Feedback indicates that the filter_redundant_restrictive_scopes function should also be updated to include this new scope in its RESTRICTIVE_SCOPES list to ensure proper handling when broader permissions are granted.
| "https://www.googleapis.com/auth/drive", | ||
| "https://www.googleapis.com/auth/spreadsheets", | ||
| "https://www.googleapis.com/auth/gmail.modify", | ||
| "https://www.googleapis.com/auth/gmail.settings.basic", |
There was a problem hiding this comment.
Adding https://www.googleapis.com/auth/gmail.settings.basic to the default scope sets requires an update to the filter_redundant_restrictive_scopes function (located around line 862). This function is designed to remove restrictive scopes that are redundant when broader scopes (like https://mail.google.com/) are present, preventing Google from enforcing the narrower scope's limitations on the access token. Please add an entry for this new scope to the RESTRICTIVE_SCOPES constant within that function to ensure it is filtered out when full Gmail access is granted.
There was a problem hiding this comment.
Thanks for catching this. I added gmail.settings.basic to the redundant restrictive-scope filter when https://mail.google.com/ is present, with regression coverage for both filtering and keeping it when only gmail.modify is present.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds the https://www.googleapis.com/auth/gmail.settings.basic scope to the Google Workspace CLI, updating the minimal and full scope lists, redundancy filtering logic, and unit tests. Feedback indicates that adding this restricted scope to MINIMAL_SCOPES contradicts existing documentation and may increase authentication risks for unverified OAuth applications.
| "https://www.googleapis.com/auth/drive", | ||
| "https://www.googleapis.com/auth/spreadsheets", | ||
| "https://www.googleapis.com/auth/gmail.modify", | ||
| "https://www.googleapis.com/auth/gmail.settings.basic", |
There was a problem hiding this comment.
Adding https://www.googleapis.com/auth/gmail.settings.basic to MINIMAL_SCOPES contradicts the documentation above (lines 258-262), which states that these scopes should never trigger Google's restricted_client or unverified-app blocks. This scope is explicitly classified as Restricted in setup.rs (line 194). While other restricted scopes like gmail.modify are already present, adding more restricted scopes increases the likelihood of authentication failures for users with unverified OAuth apps.
References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
There was a problem hiding this comment.
Good point. I removed gmail.settings.basic from MINIMAL_SCOPES so the default login set stays unchanged. The scope remains available in the picker and --full, which addresses the filters use case without making the default scope set broader.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds the gmail.settings.basic scope to the Google Workspace CLI, including it in the full scope list and marking it as redundant when the root Gmail scope is present. The reviewer pointed out that this scope is also redundant when gmail.modify is present, recommending that gmail.modify be added as a broader alternative and that the unit tests be updated to reflect this logic.
| "https://www.googleapis.com/auth/gmail.settings.basic", | ||
| &["https://mail.google.com/"], |
There was a problem hiding this comment.
The gmail.settings.basic scope is redundant when https://www.googleapis.com/auth/gmail.modify is present, as the latter grants full read/write access to Gmail resources and their settings. To maintain consistency with the handling of gmail.metadata and to avoid potential token limitations enforced by Google when redundant restrictive scopes are included, gmail.modify should be added to the list of broader alternatives.
"https://www.googleapis.com/auth/gmail.settings.basic",
&[
"https://mail.google.com/",
"https://www.googleapis.com/auth/gmail.modify",
],| #[test] | ||
| fn filter_restrictive_keeps_settings_basic_without_full_gmail() { | ||
| let scopes = vec![ | ||
| "https://www.googleapis.com/auth/gmail.modify".to_string(), | ||
| "https://www.googleapis.com/auth/gmail.settings.basic".to_string(), | ||
| ]; | ||
| let result = filter_redundant_restrictive_scopes(scopes.clone()); | ||
| assert_eq!(result, scopes); | ||
| } |
There was a problem hiding this comment.
This test case incorrectly asserts that gmail.settings.basic should be retained when gmail.modify is present. Since gmail.modify is a broader scope that encompasses settings management, gmail.settings.basic is redundant and should be filtered out by filter_redundant_restrictive_scopes. The test should be updated to verify this deduplication logic, ensuring consistency with how other Gmail scopes like gmail.metadata are handled.
#[test]
fn filter_restrictive_removes_settings_basic_when_modify_present() {
let scopes = vec![
"https://www.googleapis.com/auth/gmail.modify".to_string(),
"https://www.googleapis.com/auth/gmail.settings.basic".to_string(),
];
let result = filter_redundant_restrictive_scopes(scopes);
assert_eq!(result, vec!["https://www.googleapis.com/auth/gmail.modify"]);
}2 participants
Closes #668.
Summary
https://www.googleapis.com/auth/gmail.settings.basicto the minimal and full OAuth scope setsTesting