feat(auth): redesign scope picker to show services instead of raw scopes#241
feat(auth): redesign scope picker to show services instead of raw scopes#241
Conversation
Fixes #234 - Group scopes by service prefix (drive, gmail, calendar, etc.) - Show capitalized service names with descriptions from registry - Display scope count and classification badges per service - Templates now select/deselect services, not individual scopes - Title changed to 'Select services to authorize' - Scope resolution still produces correct URLs for selected services
🦋 Changeset detectedLatest commit: 6e833ed 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 introduces a significant user experience improvement to the OAuth scope selection process. By shifting from a technical, raw scope list to a service-first interface, users can now intuitively choose the Google APIs they wish to authorize. This change reduces cognitive load and provides clearer context for permissions, making the authorization flow more user-friendly and transparent. Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
googleworkspace-bot
added
the
cla: yes
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
==========================================
- Coverage 57.69% 57.52% -0.18%
==========================================
Files 38 38
Lines 14328 14370 +42
==========================================
- Hits 8267 8266 -1
- Misses 6061 6104 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 is a great improvement to the user experience, redesigning the scope picker to be service-oriented rather than showing a raw list of scopes. The implementation is well-structured, grouping scopes by service and providing clear information to the user. I've found one critical issue that could lead to a panic under certain data conditions, which should be addressed.
| let label = svc_entry | ||
| .map(|e| { | ||
| // Capitalize the first alias for display | ||
| let name = e.aliases[0]; | ||
| let mut chars = name.chars(); | ||
| match chars.next() { | ||
| None => String::new(), | ||
| Some(c) => c.to_uppercase().collect::<String>() + chars.as_str(), | ||
| } | ||
| }) | ||
| .unwrap_or_else(|| svc_key.clone()); |
There was a problem hiding this comment.
There's a potential for a panic here if a ServiceEntry has an empty aliases slice. The code at line 830 accesses e.aliases[0] without checking if the slice is empty. While all current ServiceEntry definitions in src/services.rs have at least one alias, this is fragile and could lead to a crash if a new entry is added incorrectly in the future.
To make the code more robust, you should safely access the first element of the aliases slice.
| let label = svc_entry | |
| .map(|e| { | |
| // Capitalize the first alias for display | |
| let name = e.aliases[0]; | |
| let mut chars = name.chars(); | |
| match chars.next() { | |
| None => String::new(), | |
| Some(c) => c.to_uppercase().collect::<String>() + chars.as_str(), | |
| } | |
| }) | |
| .unwrap_or_else(|| svc_key.clone()); | |
| let label = svc_entry | |
| .and_then(|e| { | |
| // Capitalize the first alias for display | |
| e.aliases.first().map(|name| { | |
| let mut chars = name.chars(); | |
| match chars.next() { | |
| None => String::new(), | |
| Some(c) => c.to_uppercase().collect::<String>() + chars.as_str(), | |
| } | |
| }) | |
| }) | |
| .unwrap_or_else(|| svc_key.clone()); |
googleworkspace-bot
added
the
gemini: reviewed
Addresses review comments: - Use aliases.first() instead of aliases[0] to prevent potential panic on empty aliases slice. - Add '🔧 Custom' template option that falls through to the existing individual scope picker (run_simple_scope_picker), giving users full control over individual scope selection when needed.
googleworkspace-bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the user experience of the scope picker by grouping scopes into services. The code is well-structured and the new service-centric approach is much more intuitive. I have one suggestion regarding the default behavior for custom service selections to better align with the principle of least privilege.
src/auth_commands.rs
Outdated
| } else if recommended && !full { | ||
| // In Recommended mode, use recommended scope selection logic | ||
| for s in scopes { | ||
| if is_recommended_scope(s, &all_shorts, services_filter.is_some()) { | ||
| selected.push(s.url.clone()); | ||
| } | ||
| } | ||
| } else { | ||
| // Full or custom: include top-level (non-subsumed) scopes | ||
| for s in scopes { | ||
| if !is_subsumed_scope(&s.short, &all_shorts) { | ||
| selected.push(s.url.clone()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When a user makes a custom selection of services (i.e., not using one of the 'Recommended', 'Read Only', or 'Full Access' templates exclusively), the current logic defaults to selecting the broadest, non-subsumed scopes for each service. This 'full access' default for custom selections might grant more permissions than the user intends, violating the principle of least privilege.
It would be safer to default to 'recommended' scopes for custom selections. This ensures that even when customizing, the user starts with a more secure, curated set of permissions. The 'Full Access' template can still be used explicitly when broad permissions are required.
| } else if recommended && !full { | |
| // In Recommended mode, use recommended scope selection logic | |
| for s in scopes { | |
| if is_recommended_scope(s, &all_shorts, services_filter.is_some()) { | |
| selected.push(s.url.clone()); | |
| } | |
| } | |
| } else { | |
| // Full or custom: include top-level (non-subsumed) scopes | |
| for s in scopes { | |
| if !is_subsumed_scope(&s.short, &all_shorts) { | |
| selected.push(s.url.clone()); | |
| } | |
| } | |
| } | |
| } else if full && !recommended && !readonly { | |
| // Full Access template: include top-level (non-subsumed) scopes | |
| for s in scopes { | |
| if !is_subsumed_scope(&s.short, &all_shorts) { | |
| selected.push(s.url.clone()); | |
| } | |
| } | |
| } else { | |
| // Recommended or custom: use recommended scope selection logic | |
| for s in scopes { | |
| if is_recommended_scope(s, &all_shorts, services_filter.is_some()) { | |
| selected.push(s.url.clone()); | |
| } | |
| } | |
| } |
googleworkspace-bot
added
the
gemini: reviewed
Addresses review: custom service selection now uses recommended scope logic (principle of least privilege) instead of full access. Only the explicit Full Access template gets broad scopes.
googleworkspace-bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great user experience improvement, redesigning the scope picker to be service-oriented instead of showing a flat list of raw scope names. The implementation is well-structured, grouping scopes by service and providing clear descriptions and badges. However, I found a critical logic issue in how combinations of templates (like 'Read Only' and 'Full Access') are handled, which can lead to an incorrect set of scopes being selected for authorization. My review includes a detailed comment and a suggested fix for this issue.
| // Determine which services are selected | ||
| let selected_service_keys: Vec<&str> = if full && !recommended && !readonly { | ||
| all_services.iter().map(|s| s.as_str()).collect() | ||
| } else if recommended && !full && !readonly { | ||
| // Recommended: consumer scopes only (or top-level scopes if filtered). | ||
| for entry in &filtered_scopes { | ||
| if is_app_only_scope(&entry.url) { | ||
| continue; | ||
| } | ||
| if is_recommended_scope(entry, &all_shorts, services_filter.is_some()) { | ||
| selected.push(entry.url.to_string()); | ||
| } | ||
| } | ||
| recommended_services.iter().map(|s| s.as_str()).collect() | ||
| } else if readonly && !full && !recommended { | ||
| for entry in &filtered_scopes { | ||
| if is_app_only_scope(&entry.url) { | ||
| continue; | ||
| } | ||
| if entry.is_readonly { | ||
| selected.push(entry.url.to_string()); | ||
| } | ||
| } | ||
| readonly_services.iter().map(|s| s.as_str()).collect() | ||
| } else { | ||
| for (i, item) in items.iter().enumerate().skip(template_count) { | ||
| if item.selected { | ||
| let picker_idx = i - template_count; | ||
| if let Some(&scope_idx) = valid_scope_indices.get(picker_idx) { | ||
| if let Some(entry) = filtered_scopes.get(scope_idx) { | ||
| selected.push(entry.url.to_string()); | ||
| // Individual service selection | ||
| items | ||
| .iter() | ||
| .enumerate() | ||
| .skip(template_count) | ||
| .filter(|(_, item)| item.selected) | ||
| .filter_map(|(i, _)| { | ||
| service_keys_in_order | ||
| .get(i - template_count) | ||
| .map(|s| s.as_str()) | ||
| }) | ||
| .collect() | ||
| }; | ||
|
|
||
| // Map selected services to scope URLs | ||
| let mut selected: Vec<String> = Vec::new(); | ||
| let is_readonly_mode = readonly && !full && !recommended; | ||
|
|
||
| for svc_key in &selected_service_keys { | ||
| if let Some(scopes) = service_groups.get(*svc_key) { | ||
| if is_readonly_mode { | ||
| // In Read Only mode, prefer readonly scopes | ||
| for s in scopes { | ||
| if s.is_readonly { | ||
| selected.push(s.url.clone()); | ||
| } | ||
| } | ||
| } else if full && !recommended && !readonly { | ||
| // Full Access template: include top-level (non-subsumed) scopes | ||
| for s in scopes { | ||
| if !is_subsumed_scope(&s.short, &all_shorts) { | ||
| selected.push(s.url.clone()); | ||
| } | ||
| } | ||
| } else { | ||
| // Recommended or custom: use recommended scope selection logic | ||
| for s in scopes { | ||
| if is_recommended_scope(s, &all_shorts, services_filter.is_some()) { | ||
| selected.push(s.url.clone()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a logic issue in how selected services and their corresponding scopes are determined when multiple templates are selected. The current implementation uses mutually exclusive conditions (e.g., full && !recommended && !readonly) for both determining the list of selected services and for choosing which scopes to grant. This approach fails for combinations of templates.
For example, if a user selects both 'Full Access' and 'Read Only', the logic falls through to the final else block, which applies 'Recommended' scope logic. This is likely not the intended behavior. A user would probably expect 'Read Only' to take precedence.
This can be fixed by simplifying the logic:
- Always determine the selected services by inspecting the UI state, since the TUI updates the service selections when a template is toggled.
- Establish a clear priority for which scope-selection logic to use (e.g., Read Only > Full Access > Recommended).
This ensures that template combinations behave predictably and correctly.
// Determine which services are selected by looking at the UI state,
// which is updated by the TUI when templates are toggled.
let selected_service_keys: Vec<&str> = items
.iter()
.enumerate()
.skip(template_count)
.filter(|(_, item)| item.selected)
.filter_map(|(i, _)| {
service_keys_in_order
.get(i - template_count)
.map(|s| s.as_str())
})
.collect();
// Map selected services to scope URLs
let mut selected: Vec<String> = Vec::new();
// Establish priority for templates: Read Only > Full Access > Recommended/Custom
let use_readonly = readonly;
let use_full = full && !use_readonly;
for svc_key in &selected_service_keys {
if let Some(scopes) = service_groups.get(*svc_key) {
if use_readonly {
// In Read Only mode, prefer readonly scopes
for s in scopes {
if s.is_readonly {
selected.push(s.url.clone());
}
}
} else if use_full {
// Full Access mode: include top-level (non-subsumed) scopes
for s in scopes {
if !is_subsumed_scope(&s.short, &all_shorts) {
selected.push(s.url.clone());
}
}
} else {
// Recommended or individual selection: use recommended scope selection logic
for s in scopes {
if is_recommended_scope(s, &all_shorts, services_filter.is_some()) {
selected.push(s.url.clone());
}
}
}
}
}
googleworkspace-bot
added
the
gemini: reviewed
|
Consolidating this thread into the central discussion for the auth UX/user journey work: #245 |
3 participants
Summary
Fixes #234
Redesigns the scope picker from showing raw scope names (
drive.metadata.readonly,gmail.compose) to showing services with human-readable labels:What changed
src/auth_commands.rs:run_discovery_scope_picker()now:SERVICESregistrysrc/setup.rs: Added#[allow(dead_code)]onDiscoveredScope.description(now used at service level rather than per-scope).Why service-first?
Test plan
cargo clippy -- -D warningsclean