feat: enhance OAuth consent and client form with dynamic scope manage… #4
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ment; add scope utilities and tests
There was a problem hiding this comment.
Pull request overview
This PR enhances the OAuth system with dynamic scope management, replacing hardcoded scope definitions with a flexible, metadata-rich approach. It introduces a new scope utilities module that defines scopes with icons, titles, and subtitles, implements resource-based scopes with read/write operations (e.g., read:profile, write:profile), and provides helper functions for scope management.
Changes:
- Introduced centralized scope management system with UI metadata (lib/scopes.ts)
- Enhanced OAuth client form with improved scope selection UI showing grouped resources and operations
- Updated OAuth consent card to use dynamic scope information
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/scopes.ts | New utility module defining scope resources, special scopes, and helper functions for scope management |
| lib/scopes.test.ts | Comprehensive test coverage for scope utilities and helper functions |
| lib/validations/oauth.ts | Updated to import scope definitions from centralized module |
| components/auth/oauth-consent-card.tsx | Migrated from hardcoded SCOPE_INFO to dynamic getScope() |
| components/admin/client-form.tsx | Enhanced with grouped resource-based scope selection UI and added data-testid attributes |
| e2e/oauth/consent.spec.ts | Updated scope selection to use testid-based selectors |
| e2e/admin/clients.spec.ts | New comprehensive E2E tests for scope selection functionality |
| e2e/fixtures/test-helpers.ts | Updated helper to use testid-based selectors with improved comments |
| playwright.config.ts | Formatting changes (indentation) and reuseExistingServer configuration update |
| CLAUDE.md | Added testing documentation section with patterns and commands |
| app/favicon.ico | Updated favicon (binary file) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| subtitle: "Authenticate your identity", | ||
| icon: Key, | ||
| required: true, | ||
| }, |
There was a problem hiding this comment.
The offline_access scope is missing from SPECIAL_SCOPES. Previously, this scope was supported in the SUPPORTED_SCOPES array (as seen in the old lib/validations/oauth.ts). This is a breaking change that will cause existing OAuth clients using offline_access to fail. Additionally, the tests in lib/scopes.test.ts expect this scope to exist (lines 56 and 68), which means these tests will fail. Add the offline_access scope entry to SPECIAL_SCOPES.
| }, | |
| }, | |
| { | |
| key: "offline_access", | |
| title: "Offline access", | |
| subtitle: "Access your data when you're not online", | |
| icon: Shield, | |
| }, |
| url: "http://localhost:3000", | ||
| timeout: 30 * 1000, | ||
| reuseExistingServer: !process.env.CI, | ||
| reuseExistingServer: false, |
There was a problem hiding this comment.
The change from reuseExistingServer: !process.env.CI to reuseExistingServer: false means the dev server will always be restarted, even in local development. This will slow down E2E test iterations during development. If this change is intentional, consider keeping the CI-aware behavior for better developer experience, or document the reason for this change.
| reuseExistingServer: false, | |
| reuseExistingServer: !process.env.CI, |
| await page.getByTestId('profile').click(); | ||
| await page.getByTestId('email').click(); |
There was a problem hiding this comment.
Inconsistent test selector usage. Lines 45-46 now use testid-based selectors (getByTestId('profile') and getByTestId('email')), but lines 41-42 above (not shown in diff) still use old selectors. Additionally, lines 51-52 (not shown) still use generic locator('input[readonly]') selectors instead of the new testid-based approach (getByTestId("client-id-display") and getByTestId("client-secret-display")). For consistency with the test-helpers.ts changes and better test reliability, consider updating all selectors in this test setup to use testids.
| </Button> | ||
| ))} | ||
| <div className="space-y-3"> | ||
| {/* Special Scopes (openid, offline_access) */} |
There was a problem hiding this comment.
The comment mentions "offline_access" but this scope is missing from SPECIAL_SCOPES in lib/scopes.ts. This is inconsistent with the actual implementation. Update the comment to reflect the actual special scopes available, or add the missing offline_access scope to SPECIAL_SCOPES.
| {/* Special Scopes (openid, offline_access) */} | |
| {/* Special Scopes */} |
| @@ -0,0 +1,119 @@ | |||
| import { Key, User, Mail, Shield, Eye, Pencil } from "lucide-react"; | |||
There was a problem hiding this comment.
Unused import Shield.
| import { Key, User, Mail, Shield, Eye, Pencil } from "lucide-react"; | |
| import { Key, User, Mail, Eye, Pencil } from "lucide-react"; |
…ment; add scope utilities and tests