drive: map scope numbers to actual values in config#9136
drive: map scope numbers to actual values in config#9136L4z3x wants to merge 1 commit intorclone:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This change fixes how Google Drive OAuth scopes are derived from user input during configuration, ensuring numeric scope selections are mapped to the correct scope strings instead of being used literally.
Changes:
- Introduced a
validScopeslookup indriveScopesto map numeric choices (1–5) to their corresponding Drive scope names. - Allowed direct use of known scope names via the same map to validate and normalize input.
- Logged a message and still accepted unknown scope values for forward compatibility while avoiding failures on unexpected input.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| // For unknown values, still accept them (for future-proofing) | ||
| // Log as info since this could be a user mistake | ||
| fs.Logf(nil, "Using unrecognized scope %q - valid options: 1-5 or drive, drive.readonly, drive.file, drive.appfolder, drive.metadata.readonly", scope) |
There was a problem hiding this comment.
The comment says "Log as info" but this uses fs.Logf, which logs at the default (notice) level, so the implementation doesn’t match the stated intent. Consider either switching to fs.Infof to align with the comment (and make this a lower-visibility informational message about potentially mistaken scopes) or updating the comment to match the chosen log level.
| fs.Logf(nil, "Using unrecognized scope %q - valid options: 1-5 or drive, drive.readonly, drive.file, drive.appfolder, drive.metadata.readonly", scope) | |
| fs.Infof(nil, "Using unrecognized scope %q - valid options: 1-5 or drive, drive.readonly, drive.file, drive.appfolder, drive.metadata.readonly", scope) |
| validScopes := map[string]string{ | ||
| "1": "drive", | ||
| "2": "drive.readonly", | ||
| "3": "drive.file", | ||
| "4": "drive.appfolder", | ||
| "5": "drive.metadata.readonly", | ||
| // Also allow direct scope names | ||
| "drive": "drive", | ||
| "drive.readonly": "drive.readonly", | ||
| "drive.file": "drive.file", | ||
| "drive.appfolder": "drive.appfolder", | ||
| "drive.metadata.readonly": "drive.metadata.readonly", | ||
| } |
There was a problem hiding this comment.
The new numeric-to-scope mapping logic in validScopes isn’t covered by tests: TestDriveScopes in backend/drive/drive_internal_test.go only exercises named scopes. Please extend TestDriveScopes with cases like "1,4" (and possibly a mixed input with direct names) to verify that numeric inputs are correctly mapped to drive, drive.appfolder, etc., and to guard against future regressions in this behavior.
1 participant
What is the purpose of this change?
Fixes an issue where users entering scope numbers during interactive configuration (e.g.,
1,4) would have those numbers used literally in OAuth URLs instead of being mapped to the actual scope values (drive,drive.appfolder).This was causing invalid OAuth scope URLs like
https://www.googleapis.com/auth/1instead ofhttps://www.googleapis.com/auth/drive.The fix adds a lookup table in
driveScopes()to map numeric choices (1-5) to their corresponding scope strings. It still accepts direct scope names and unknown values (with a warning) for backward compatibility and flexibility.Was the change discussed in an issue or in the forum before?
No prior issue or forum discussion. Discovered during configuration when testing scope selection.
Checklist