Skip to content
Closed
Changes from 2 commits
Commits
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
54 changes: 46 additions & 8 deletions crates/google-workspace-cli/src/auth_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ pub const MINIMAL_SCOPES: &[&str] = &[
"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",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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
  1. Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

"https://www.googleapis.com/auth/calendar",
"https://www.googleapis.com/auth/documents",
"https://www.googleapis.com/auth/presentations",
Expand All @@ -289,6 +290,7 @@ pub const FULL_SCOPES: &[&str] = &[
"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",
"https://www.googleapis.com/auth/calendar",
"https://www.googleapis.com/auth/documents",
"https://www.googleapis.com/auth/presentations",
Expand Down Expand Up @@ -857,14 +859,20 @@ fn filter_redundant_restrictive_scopes(scopes: Vec<String>) -> Vec<String> {
// broader scopes. Each entry maps a restrictive scope to the broader scopes
// that make it redundant. The restrictive scope is removed only if at least
// one of its broader alternatives is already in the list.
const RESTRICTIVE_SCOPES: &[(&str, &[&str])] = &[(
"https://www.googleapis.com/auth/gmail.metadata",
&[
"https://mail.google.com/",
"https://www.googleapis.com/auth/gmail.modify",
"https://www.googleapis.com/auth/gmail.readonly",
],
)];
const RESTRICTIVE_SCOPES: &[(&str, &[&str])] = &[
(
"https://www.googleapis.com/auth/gmail.metadata",
&[
"https://mail.google.com/",
"https://www.googleapis.com/auth/gmail.modify",
"https://www.googleapis.com/auth/gmail.readonly",
],
),
(
"https://www.googleapis.com/auth/gmail.settings.basic",
&["https://mail.google.com/"],
Comment on lines +871 to +872
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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",
            ],

),
];

let scope_set: std::collections::HashSet<String> = scopes.iter().cloned().collect();

Expand Down Expand Up @@ -1549,6 +1557,10 @@ const SCOPE_ENTRIES: &[ScopeEntry] = &[
scope: "https://www.googleapis.com/auth/gmail.modify",
label: "Gmail",
},
ScopeEntry {
scope: "https://www.googleapis.com/auth/gmail.settings.basic",
label: "Gmail Settings",
},
ScopeEntry {
scope: "https://www.googleapis.com/auth/calendar",
label: "Google Calendar",
Expand Down Expand Up @@ -1745,6 +1757,9 @@ mod tests {
let scopes = run_resolve_scopes(ScopeMode::Default, None);
assert_eq!(scopes.len(), DEFAULT_SCOPES.len());
assert_eq!(scopes[0], "https://www.googleapis.com/auth/drive");
assert!(
scopes.contains(&"https://www.googleapis.com/auth/gmail.settings.basic".to_string())
);
}

#[test]
Expand Down Expand Up @@ -1789,6 +1804,9 @@ mod tests {
fn resolve_scopes_full_returns_full_scopes() {
let scopes = run_resolve_scopes(ScopeMode::Full, None);
assert_eq!(scopes.len(), FULL_SCOPES.len());
assert!(
scopes.contains(&"https://www.googleapis.com/auth/gmail.settings.basic".to_string())
);
}

#[test]
Expand Down Expand Up @@ -2360,6 +2378,26 @@ mod tests {
assert_eq!(result, scopes);
}

#[test]
fn filter_restrictive_removes_settings_basic_when_full_gmail_present() {
let scopes = vec![
"https://mail.google.com/".to_string(),
"https://www.googleapis.com/auth/gmail.settings.basic".to_string(),
];
let result = filter_redundant_restrictive_scopes(scopes);
assert_eq!(result, vec!["https://mail.google.com/"]);
}

#[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);
}
Comment on lines +2390 to +2398
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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"]);
    }


#[test]
fn mask_secret_long_string() {
let masked = mask_secret("GOCSPX-abcdefghijklmnopqrstuvwxyz");
Expand Down
Loading