refactor: use oidc well-known to discover urls#351
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change refactors OIDC configuration across the backend and frontend to use a single issuer URL with dynamic endpoint discovery, replacing explicit endpoint variables. Backend services, configuration, and routes are updated accordingly. The frontend adapts to new API endpoints, updates OIDC forms, and improves error handling and user feedback for OIDC authentication flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant OIDC Provider
User->>Frontend: Clicks "Login with OIDC"
Frontend->>Backend: GET /oidc/url
Backend->>Backend: Discover endpoints via issuer URL (if needed)
Backend->>Frontend: Returns OIDC auth URL
Frontend->>User: Redirects to OIDC Provider
User->>OIDC Provider: Authenticates
OIDC Provider->>Frontend: Redirects with code & state to /auth/oidc/callback
Frontend->>Backend: POST /oidc/callback with code & state
Backend->>Backend: Discover endpoints (if needed)
Backend->>OIDC Provider: Exchanges code for tokens
OIDC Provider->>Backend: Returns tokens
Backend->>OIDC Provider: Fetches userinfo
OIDC Provider->>Backend: Returns userinfo
Backend->>Frontend: Returns user info & session
Frontend->>User: Shows dashboard or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.env.example (1)
18-18: Add missing blank line at end of file.OIDC_SCOPES="openid email profile" +backend/internal/config/config.go (1)
33-33: Environment variable name could be more consistent.The environment variable is parsed as
OIDC_ENABLEDbut the struct field isOidcEnabled. Consider using a consistent naming pattern.- oidcEnabled, _ := strconv.ParseBool(os.Getenv("OIDC_ENABLED")) + oidcEnabled, _ := strconv.ParseBool(os.Getenv("OIDC_ENABLED"))Note: The current implementation is actually fine, just flagging for consistency awareness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.env.example(1 hunks)backend/internal/api/oidc_handler.go(3 hunks)backend/internal/api/routes.go(2 hunks)backend/internal/bootstrap/services_bootstrap.go(1 hunks)backend/internal/config/config.go(4 hunks)backend/internal/models/settings.go(1 hunks)backend/internal/services/auth_service.go(4 hunks)backend/internal/services/oidc_service.go(6 hunks)frontend/src/lib/services/api/oidc-api-service.ts(2 hunks)frontend/src/lib/types/settings.type.ts(1 hunks)frontend/src/routes/auth/oidc/callback/+page.svelte(4 hunks)frontend/src/routes/auth/oidc/login/+page.svelte(3 hunks)frontend/src/routes/settings/security/+page.svelte(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/internal/bootstrap/services_bootstrap.go (1)
backend/internal/services/oidc_service.go (1)
NewOidcService(57-67)
backend/internal/api/routes.go (2)
backend/internal/config/config.go (1)
Config(13-30)backend/internal/api/oidc_handler.go (1)
NewOidcHandler(17-23)
frontend/src/lib/services/api/oidc-api-service.ts (2)
backend/internal/models/settings.go (1)
OidcConfig(12-23)frontend/src/lib/types/settings.type.ts (1)
OidcConfig(10-21)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 17-17: [QuoteCharacter] The value has quote characters (', ")
[warning] 18-18: [EndingBlankLine] No blank line at the end of the file
🔇 Additional comments (44)
backend/internal/bootstrap/services_bootstrap.go (1)
28-28: LGTM - Service initialization properly updated for OIDC refactoring.The addition of the
cfgparameter toNewOidcServiceis consistent with the updated constructor signature that now requires configuration access for dynamic OIDC endpoint discovery and redirect URI construction.backend/internal/api/oidc_handler.go (3)
53-53: Minor formatting improvement.The added blank line improves code readability by separating variable declaration from validation logic.
78-78: Good documentation practice.The comment clarifying that 600 seconds equals 10 minutes improves code readability and maintenance.
155-156: LGTM - Proper implementation of dynamic OIDC configuration.The changes correctly implement the refactored OIDC approach:
GetOidcRedirectURI()enables dynamic redirect URI construction based on app URL- Addition of
issuerUrlfield supports the new discovery-based configuration modelThese changes align well with the overall refactoring to use OIDC well-known endpoints.
.env.example (1)
17-17: OIDC configuration successfully simplified.The consolidation from multiple explicit endpoint variables to a single
OIDC_ISSUER_URLaligns perfectly with the dynamic discovery refactoring approach. This significantly simplifies OIDC configuration management.backend/internal/models/settings.go (1)
13-23: Excellent refactoring of OIDC configuration model.The structural changes effectively implement the dynamic discovery approach:
- Simplified core configuration:
IssuerURLreplaces static endpoint management- Flexible endpoint handling: Optional fields with
omitemptytags support both auto-discovery and manual overrides- Complete OIDC support: Addition of
JwksURIfield enables full OpenID Connect capabilities- Clear documentation: Comment explains the auto-discovery and caching behavior
This design allows the system to work with minimal configuration while preserving advanced customization options.
frontend/src/lib/types/settings.type.ts (1)
12-21: Frontend types properly aligned with backend OIDC refactoring.The interface changes correctly reflect the new discovery-based configuration model:
- Required
issuerUrl: Establishes the primary configuration point- Optional endpoints: Support both auto-discovery and manual override scenarios
- Optional
clientSecret: Accommodates public OIDC clients (though verify this aligns with your security requirements)The TypeScript definitions will provide proper type safety for the refactored OIDC implementation.
Verify that making
clientSecretoptional aligns with your OIDC security requirements. Most production OIDC implementations use confidential clients that require client secrets.backend/internal/config/config.go (3)
6-6: LGTM! Import addition is appropriate.The
stringspackage import is correctly added to support the newGetOidcRedirectURI()method.
23-23: Good refactoring to support OIDC discovery.Replacing
OidcRedirectURIwithOidcIssuerURLaligns with the PR's objective to use well-known discovery instead of static configuration.
51-54: Dynamic redirect URI construction is well-implemented.The method correctly handles trailing slashes and constructs a consistent callback URL. This approach is more maintainable than static configuration.
frontend/src/routes/auth/oidc/login/+page.svelte (5)
12-12: Good UX improvement changing default redirect.Redirecting to
/dashboardinstead of/provides a better post-login experience for users.
14-22: Excellent proactive OIDC configuration validation.Adding upfront validation prevents users from attempting authentication when OIDC is misconfigured, providing better error feedback.
26-28: More descriptive error message improves user experience.The updated error message provides clearer guidance for users and administrators.
38-48: Enhanced error parsing provides better user feedback.The conditional error message handling based on error content (discovery, network, timeout) gives users more actionable information.
62-66: UI improvements enhance visual consistency.The styling updates use semantic color classes and improve the overall user experience with better visual hierarchy.
frontend/src/routes/settings/security/+page.svelte (5)
23-28: Good refactoring of OIDC configuration structure.The removal of
redirectUriand addition ofissuerUrlwith default scopes aligns with the discovery-based approach. The comment about legacy fields maintains backward compatibility.
78-78: Form submission correctly uses issuer URL.The form now properly submits
issuerUrlinstead of the removedredirectUrifield.
341-347: Issuer URL input is well-designed.The field has appropriate labeling and placeholder text to guide users in configuring OIDC discovery.
358-394: Excellent advanced configuration section.The collapsible manual endpoint configuration provides flexibility for edge cases while encouraging the recommended auto-discovery approach. The UI clearly indicates these fields are optional and will override discovery.
398-410: Good UX for redirect URI display.Showing the redirect URI as read-only with explanatory text helps users understand what to configure in their OIDC provider while making it clear this is auto-generated.
backend/internal/services/auth_service.go (4)
126-135: Good integration of issuer URL with legacy support.The sync logic properly includes the new
IssuerURLandScopesfields while maintaining backward compatibility with explicit endpoints. The comment clarifies the legacy support approach.
157-161: Improved validation logic for flexible OIDC configuration.The updated validation correctly requires either an issuer URL or explicit endpoints, providing flexibility while ensuring minimum configuration requirements. The warning message is clear and actionable.
205-210: Consistent environment configuration validation.The environment configuration check properly validates that either issuer URL or explicit endpoints are provided, maintaining consistency with the sync logic.
221-227: Comprehensive database configuration validation.The database configuration validation correctly requires either issuer URL or all explicit endpoints, ensuring proper OIDC setup regardless of configuration method.
frontend/src/routes/auth/oidc/callback/+page.svelte (6)
20-20: Consistent redirect destination improves UX.Changing the default redirect to
/dashboardmaintains consistency with the login page and provides a better post-authentication experience.
23-36: Excellent OIDC provider error handling.The explicit handling of provider error parameters with specific messages for
access_deniedandinvalid_requestprovides much better user feedback than generic error handling.
48-59: Enhanced authentication failure parsing.The improved error message parsing that checks for "state" and "expired" keywords provides more specific user feedback, helping users understand what went wrong.
73-73: Better role handling from authentication result.Using
authResult.user.groupswhen available instead of defaulting to['user']provides more accurate role assignment from the OIDC provider.
78-80: Good post-authentication flow.The sequence of invalidating cached data, showing success feedback, and redirecting provides a smooth user experience after successful authentication.
89-96: Comprehensive error handling with network awareness.The catch block properly distinguishes network-related errors and provides appropriate user messaging, improving the debugging experience for connectivity issues.
backend/internal/api/routes.go (3)
33-34: Good architectural decision to separate OIDC routesSeparating OIDC routes from authentication routes improves modularity and follows the single responsibility principle.
80-89: LGTM!The OIDC routes are properly organized and use appropriate HTTP methods for each endpoint.
91-91: Function signature correctly simplifiedThe removal of
appConfigparameter is appropriate since it was only needed for OIDC routes which have been moved to their own function.frontend/src/lib/services/api/oidc-api-service.ts (3)
7-9: Interface properties correctly updatedMaking
nameoptional and reorderingpreferred_usernamealigns with standard OIDC claims where these fields may not always be present.
16-22: Well-structured status interfaceThe
OidcStatusinterface clearly represents the different configuration states (environment vs database, forced vs configured).
25-48: API endpoints correctly updatedAll OIDC endpoints have been updated to match the backend route reorganization, removing the
/authprefix.backend/internal/services/oidc_service.go (8)
15-19: Required imports addedThe new imports support the caching mechanism and configuration injection needed for OIDC discovery.
22-32: Well-structured discovery documentThe
OidcDiscoveryDocumentstruct correctly represents the standard OIDC discovery response with all essential fields.
33-40: Thread-safe caching implementationThe service struct is well-designed with proper mutex protection for the discovery cache and expiry tracking.
69-133: Robust OIDC discovery implementationThe discovery method implements proper caching with expiry, comprehensive error handling, and validation. The error messages provide excellent debugging context.
135-164: Excellent backward compatibility handlingThe method elegantly handles both legacy configurations with explicit endpoints and new issuer-based discovery, ensuring smooth migration.
166-192: Method correctly updated for discovery supportThe
GenerateAuthURLmethod properly uses the effective configuration and obtains the redirect URI from the app config.
238-242: Consistent configuration usageThe callback handling correctly uses the effective configuration with discovered endpoints.
260-266: Redirect URI handling updatedThe token exchange correctly obtains the redirect URI from the app config instead of the OIDC config.
1 participant
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores