Conversation
appleboy
commented
Mar 3, 2026
- Change the default OAuth scopes from read write to email profile across handlers, services, seed data, and documentation
- Add a test to verify the default client scope behavior
- Replace the plain scopes text input in the admin client form with an interactive scope tag picker
- Introduce JavaScript logic to manage adding, removing, and syncing scopes in the admin UI
- Add new styles for the scope tag picker, tags, and preset buttons in admin forms
- Change the default OAuth scopes from read write to email profile across handlers, services, seed data, and documentation - Add a test to verify the default client scope behavior - Replace the plain scopes text input in the admin client form with an interactive scope tag picker - Introduce JavaScript logic to manage adding, removing, and syncing scopes in the admin UI - Add new styles for the scope tag picker, tags, and preset buttons in admin forms Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Updates the project’s default OAuth scope behavior (now defaulting to email profile) and introduces an admin UI scope tag picker to make client scope management easier and less error-prone.
Changes:
- Changed default OAuth scopes from
read writetoemail profilein service logic, device handler defaults, and SQLite seed data. - Added a service test to assert default scope behavior when scopes aren’t provided.
- Replaced the admin “scopes” text input with a scope tag picker UI (CSS + template + inline JS).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/templates/static/css/pages/admin-forms.css | Adds styling for the new admin scope tag picker, tags, and preset chips. |
| internal/templates/admin_client_form.templ | Replaces scopes text field with hidden input + interactive scope picker and inline JS behavior. |
| internal/store/sqlite.go | Updates seeded default OAuth client scopes to email profile. |
| internal/services/client_test.go | Adds a test asserting the default scopes on client creation. |
| internal/services/client.go | Changes CreateClient default scopes to email profile. |
| internal/handlers/device.go | Updates device code endpoint default scopes and swagger docstring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function addScope(raw) { | ||
| var s = raw.trim().toLowerCase(); | ||
| if (!s || selected.indexOf(s) !== -1) return; | ||
| selected.push(s); | ||
| render(); |
There was a problem hiding this comment.
The scope picker JS lowercases scopes (addScope/init use toLowerCase). OAuth scope values are case-sensitive, and the backend scope checks compare strings exactly, so this will silently mutate any mixed-case/custom scopes when an admin edits/saves a client. Preserve the original casing (or implement a consistent normalization strategy server-side and everywhere it’s used).
| var s = raw.trim().toLowerCase(); | ||
| if (!s || selected.indexOf(s) !== -1) return; | ||
| selected.push(s); | ||
| render(); |
There was a problem hiding this comment.
addScope(raw) treats the entire raw string as a single scope token. If a user pastes or blurs with multiple scopes (e.g. "email profile"), the UI will render it as one tag but syncHidden will serialize it with spaces, causing a mismatch between what’s displayed and what the server receives. Consider splitting raw input on whitespace and adding each token individually.
| var s = raw.trim().toLowerCase(); | |
| if (!s || selected.indexOf(s) !== -1) return; | |
| selected.push(s); | |
| render(); | |
| if (!raw) return; | |
| var added = false; | |
| raw.split(/\s+/).forEach(function (part) { | |
| var s = part.trim().toLowerCase(); | |
| if (!s || selected.indexOf(s) !== -1) return; | |
| selected.push(s); | |
| added = true; | |
| }); | |
| if (added) { | |
| render(); | |
| } |
| <label class="admin-form-label">Scopes</label> | ||
| if props.Client != nil { | ||
| <input type="text" id="scopes" name="scopes" class="admin-form-input" value={ props.Client.Scopes }/> | ||
| <input type="hidden" id="scopes" name="scopes" value={ props.Client.Scopes }/> | ||
| } else { | ||
| <input type="text" id="scopes" name="scopes" class="admin-form-input" value="read write"/> | ||
| <input type="hidden" id="scopes" name="scopes" value="email profile"/> | ||
| } | ||
| <small class="admin-form-hint">Space-separated list of scopes (e.g., "read write")</small> | ||
| <div class="admin-scope-picker" id="scopePicker" tabindex="0"> | ||
| <div class="admin-scope-tags" id="scopeTags"></div> | ||
| <input type="text" class="admin-scope-text-input" id="scopeTextInput" | ||
| placeholder="Add scope..." autocomplete="off" spellcheck="false"/> | ||
| </div> |
There was a problem hiding this comment.
The visible scope input isn’t associated with the "Scopes" label anymore (label has no for=..., and the input has no name/aria-label). This makes the control harder to use with screen readers, and the extra tabindex on the picker div adds an extra tab stop without clear keyboard behavior. Consider wiring the label to the actual text input (or using aria-labelledby/aria-describedby), and either remove tabindex or forward focus/keyboard interaction from the picker container to the text input.
| scope := c.PostForm("scope") | ||
| if scope == "" { | ||
| if s, exists := c.Get("scope"); exists { | ||
| scope = s.(string) | ||
| } else { | ||
| scope = "read write" | ||
| scope = "email profile" | ||
| } |
There was a problem hiding this comment.
The default scope for the device code endpoint changed to "email profile", but there doesn’t appear to be any handler-level test coverage for DeviceCodeRequest’s defaulting behavior. Adding an integration/handler test that omits the scope parameter and asserts the issued device code uses the default scopes would help prevent regressions.
- Improve form accessibility by associating the Scopes label with its input - Remove keyboard focus handling from the scope picker container - Allow entering multiple scopes at once by splitting whitespace and de-duplicating entries Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>