Add rudimentary OIDC support#52
Conversation
6d5adbb to
3dff312
Compare
3dff312 to
aa7a57f
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds optional OIDC (OpenID Connect) authentication support to Lidify, enabling users to authenticate via external identity providers like Keycloak, Authentik, Azure AD, Auth0, and Okta. The implementation provides a flexible authentication framework that supports both traditional credentials and SSO.
Key Changes:
- Integrated OIDC frontend authentication using
oidc-client-tswith PKCE flow support and a dedicated callback handler - Extended the User model with
oidcIdandoidcUidfields for OIDC user mapping, including database migration - Implemented an intermediary login page that automatically redirects based on available authentication providers (credentials-only, OIDC-only, or both)
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/package.json | Added oidc-client-ts dependency for OIDC authentication |
| frontend/package-lock.json | Lock file updates for new dependencies (includes unintended peer dependency changes) |
| frontend/lib/auth/oidc-client.ts | Core OIDC client implementation with UserManager initialization and callback handling |
| frontend/lib/auth-context.tsx | Updated to exclude OIDC callback route from authentication checks |
| frontend/lib/api.ts | Added tokenInitialized flag when setting tokens |
| frontend/components/layout/AuthenticatedLayout.tsx | Added OIDC-related routes to public paths |
| frontend/components/auth/AuthPageTemplate.tsx | New reusable authentication page template with artist background rotation |
| frontend/app/login/page.tsx | Refactored to provider selection page with auto-redirect logic |
| frontend/app/login/credentials/page.tsx | New dedicated credentials login page with 2FA support |
| frontend/app/auth/callback/page.tsx | OIDC callback handler that exchanges authorization codes for JWT tokens |
| docs/OIDC.md | Comprehensive OIDC setup documentation with provider examples and troubleshooting |
| backend/src/utils/db.ts | Added createUser and updateUserRole helper functions |
| backend/src/routes/auth.ts | Added OIDC endpoints for provider discovery, configuration, and token exchange with role mapping |
| backend/prisma/schema.prisma | Added oidcId and oidcUid fields to User model |
| backend/prisma/migrations/20260101015021_add_oidc_support/migration.sql | Database migration for OIDC fields |
| backend/package-lock.json | Lock file updates (includes unintended peer dependency changes) |
Files not reviewed (2)
- backend/package-lock.json: Language not supported
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
02f64ab to
3738faa
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 19 comments.
Files not reviewed (2)
- backend/package-lock.json: Language not supported
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!provider) throw new Error('No OIDC provider found'); | ||
|
|
There was a problem hiding this comment.
Duplicate condition check. Line 95 already checks if provider is null and throws an error, making this second check at line 99 redundant.
| if (!provider) throw new Error('No OIDC provider found'); |
| if (code) { | ||
| if (!tokenEndpoint) return res.status(500).json({ error: 'Provider token endpoint not found' }); | ||
| // Exchange code for tokens using client_secret (confidential server) | ||
| const params = new URLSearchParams(); | ||
| params.append('grant_type', 'authorization_code'); | ||
| params.append('code', code); | ||
| params.append('redirect_uri', redirectUri); |
There was a problem hiding this comment.
Missing input validation for the 'redirectUri' parameter. A malicious client could potentially provide an arbitrary redirect URI that doesn't match the configured callback URL, which could be used in open redirect attacks. The backend should validate that the redirectUri matches the expected pattern before using it in the token exchange.
| if (code) { | |
| if (!tokenEndpoint) return res.status(500).json({ error: 'Provider token endpoint not found' }); | |
| // Exchange code for tokens using client_secret (confidential server) | |
| const params = new URLSearchParams(); | |
| params.append('grant_type', 'authorization_code'); | |
| params.append('code', code); | |
| params.append('redirect_uri', redirectUri); | |
| // Validate redirectUri when performing authorization code exchange | |
| let finalRedirectUri: string | undefined = undefined; | |
| if (code) { | |
| const expectedRedirectUri = process.env[`${envPrefix}REDIRECT_URI`]; | |
| if (typeof redirectUri !== 'string') { | |
| return res.status(400).json({ error: 'Invalid redirectUri' }); | |
| } | |
| try { | |
| const parsedRedirectUri = z.string().url().parse(redirectUri); | |
| if (expectedRedirectUri && parsedRedirectUri !== expectedRedirectUri) { | |
| return res.status(400).json({ error: 'redirectUri does not match configured callback URL' }); | |
| } | |
| finalRedirectUri = parsedRedirectUri; | |
| } catch { | |
| return res.status(400).json({ error: 'Invalid redirectUri format' }); | |
| } | |
| } | |
| if (code) { | |
| if (!tokenEndpoint) return res.status(500).json({ error: 'Provider token endpoint not found' }); | |
| // Exchange code for tokens using client_secret (confidential server) | |
| const params = new URLSearchParams(); | |
| params.append('grant_type', 'authorization_code'); | |
| params.append('code', code); | |
| if (!finalRedirectUri) { | |
| return res.status(400).json({ error: 'redirectUri is required for authorization code exchange' }); | |
| } | |
| params.append('redirect_uri', finalRedirectUri); |
| OIDC_KEYCLOAK_ROLE_FIELD: roles | ||
| ``` | ||
|
|
||
| Common values (top-level claims only; nested paths like `realm_access.roles` or `resource_access.lidify.roles` are not currently supported directly and must be mapped to a top-level claim in your provider): |
There was a problem hiding this comment.
The comment at line 61 states "These IDs need to be unique" but the code uses underscores in the comment text. The actual documentation examples in docs/OIDC.md show roles claim paths like "realm_access.roles" which contain underscores. However, the current implementation only supports top-level claims. Consider updating the documentation to be clearer about this limitation or enhance the role mapping to support nested claim paths.
| const discoveryResponse = await fetch(`${provider.oidcIssuer}/.well-known/openid-configuration`); | ||
| const discoveryData = await discoveryResponse.json(); |
There was a problem hiding this comment.
Missing timeout configuration for discovery endpoint fetch. The fetch at line 43 doesn't have a timeout, which could cause the page to hang indefinitely if the OIDC provider is unreachable or slow to respond. Consider adding a timeout or using AbortController with a timeout to prevent indefinite waiting.
| if (!roleValue) return res.status(400).json({ error: `No field '${oidcProvider.roleField}' provided for role in OIDC scope` }); | ||
| if (Array.isArray(roleValue) && roleValue.length === 1) role = roleValue[0]; | ||
| else if (typeof roleValue === 'string') role = roleValue; | ||
| else return res.status(400).json({ error: `Invalid role value '${roleValue}' provided for role in OIDC scope` }); |
There was a problem hiding this comment.
The error messages at lines 129 and 132 expose internal implementation details about role field configuration. Consider using more generic error messages that don't reveal the exact field names or internal validation logic to prevent information disclosure.
| if (!roleValue) return res.status(400).json({ error: `No field '${oidcProvider.roleField}' provided for role in OIDC scope` }); | |
| if (Array.isArray(roleValue) && roleValue.length === 1) role = roleValue[0]; | |
| else if (typeof roleValue === 'string') role = roleValue; | |
| else return res.status(400).json({ error: `Invalid role value '${roleValue}' provided for role in OIDC scope` }); | |
| if (!roleValue) return res.status(400).json({ error: 'Required role information was not provided by the identity provider' }); | |
| if (Array.isArray(roleValue) && roleValue.length === 1) role = roleValue[0]; | |
| else if (typeof roleValue === 'string') role = roleValue; | |
| else return res.status(400).json({ error: 'Invalid role information was provided by the identity provider' }); |
| 1. Create client with: | ||
| - Client ID: `lidify` | ||
| - Access Type: `confidential` | ||
| - Valid Redirect URIs: `http://localhost:3030/auth/callback*` |
There was a problem hiding this comment.
Documentation inconsistency: Line 87 shows "Valid Redirect URIs" with a wildcard pattern ending in "*", but the actual implementation requires an exact match to the provider-specific callback URL format "/auth/callback/{provider-id}". The wildcard pattern may work for some providers but could be misleading. Consider clarifying that the exact URL format depends on the provider ID configured.
| - Valid Redirect URIs: `http://localhost:3030/auth/callback*` | |
| - Valid Redirect URIs: `http://localhost:3030/auth/callback/keycloak` (replace `keycloak` with your provider ID if different) |
| className="w-5 h-5 text-[#ecb200]" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| viewBox="0 0 24 24" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| d="M9 12l2 2 4-4m5.618-4.016A11.955 11.955 0 0112 2.944a11.955 11.955 0 01-8.618 3.04A12.02 12.02 0 003 9c0 5.591 3.824 10.29 9 11.622 5.176-1.332 9-6.03 9-11.622 0-1.042-.133-2.052-.382-3.016z" | ||
| /> | ||
| </svg> | ||
| {provider.name} | ||
| </button> | ||
| ))} | ||
| </div> | ||
| )} | ||
|
|
||
| {requires2FA && ( | ||
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| setRequires2FA(false); | ||
| setTwoFactorToken(""); | ||
| setUseRecoveryCode(false); | ||
| setError(""); | ||
| }} | ||
| className="w-full text-xs text-white/50 hover:text-white/80 transition-colors" | ||
| > | ||
| ← Back to login | ||
| </button> | ||
| )} | ||
| </form> | ||
| {/* Divider */} | ||
| {oidcProviders.length > 0 && hasCredentials && ( | ||
| <div className="relative py-4"> | ||
| <div className="absolute inset-0 flex items-center"> | ||
| <div className="w-full border-t border-white/10"></div> | ||
| </div> | ||
| <div className="relative flex justify-center text-sm"> | ||
| <span className="px-4 bg-black text-gray-400">Or</span> | ||
| </div> | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Footer */} | ||
| <p className="text-center text-white/40 text-sm mt-6"> | ||
| © 2025 Lidify. Your music, your way. | ||
| </p> | ||
| </div> | ||
| {/* Credentials Option */} | ||
| {hasCredentials && ( | ||
| <Link | ||
| href="/login/credentials" | ||
| className="w-full px-6 py-4 bg-white/5 hover:bg-white/10 border border-white/10 hover:border-[#ecb200]/50 text-white font-semibold rounded-lg transition-all flex items-center justify-center gap-3 group" | ||
| > | ||
| <Key className="w-5 h-5 text-[#ecb200]" /> |
There was a problem hiding this comment.
Inconsistent color values. The code uses "#ecb200" for button colors (lines 123, 159) but the brand color used elsewhere in the application appears to be "#fca200" (seen in other files). This inconsistency could lead to visual discrepancies across the UI. Consider using a consistent brand color or defining these colors in a central theme configuration.
| // Fetch OIDC discovery document | ||
| const discoveryResponse = await fetch(`${provider.oidcIssuer}/.well-known/openid-configuration`); | ||
| const discoveryData = await discoveryResponse.json(); | ||
|
|
||
| // If discovery data is not present, abort — frontend should not fetch provider discovery directly (CORS) | ||
| if (!discoveryData || !discoveryData.authorization_endpoint) { | ||
| throw new Error('OIDC provider metadata not available in /api/auth/providers. Please ensure the backend returns discovery metadata for the provider to avoid CORS.'); | ||
| } | ||
|
|
||
| console.log("[OIDC Client] Discovery data fetched (from providers endpoint):", { |
There was a problem hiding this comment.
The error message at line 48 states that the frontend should not fetch provider discovery directly due to CORS, but this code is actually fetching the discovery document at line 43. This is contradictory. Either the fetch should be removed and discovery metadata should come from the backend /api/auth/providers endpoint (as suggested in the comment), or the error message should be updated to reflect that direct fetching is actually being performed.
| // Fetch OIDC discovery document | |
| const discoveryResponse = await fetch(`${provider.oidcIssuer}/.well-known/openid-configuration`); | |
| const discoveryData = await discoveryResponse.json(); | |
| // If discovery data is not present, abort — frontend should not fetch provider discovery directly (CORS) | |
| if (!discoveryData || !discoveryData.authorization_endpoint) { | |
| throw new Error('OIDC provider metadata not available in /api/auth/providers. Please ensure the backend returns discovery metadata for the provider to avoid CORS.'); | |
| } | |
| console.log("[OIDC Client] Discovery data fetched (from providers endpoint):", { | |
| // Resolve OIDC discovery metadata: | |
| // 1. Prefer metadata provided by the backend via /api/auth/providers (provider.discovery). | |
| // 2. If not available or incomplete, fall back to fetching the discovery document from the issuer. | |
| let discoveryData = provider.discovery; | |
| if (!discoveryData || !discoveryData.authorization_endpoint) { | |
| const discoveryResponse = await fetch(`${provider.oidcIssuer}/.well-known/openid-configuration`); | |
| if (!discoveryResponse.ok) { | |
| throw new Error('Failed to fetch OIDC discovery document from issuer.'); | |
| } | |
| discoveryData = await discoveryResponse.json(); | |
| } | |
| if (!discoveryData || !discoveryData.authorization_endpoint) { | |
| throw new Error('OIDC provider metadata is not available from either backend configuration or issuer discovery document.'); | |
| } | |
| console.log("[OIDC Client] Discovery data resolved:", { |
| "use client"; | ||
|
|
||
| import { useState, useEffect, Suspense } from "react"; | ||
| import { useSearchParams, useRouter } from "next/navigation"; |
There was a problem hiding this comment.
Unused import useRouter.
| import { useSearchParams, useRouter } from "next/navigation"; | |
| import { useSearchParams } from "next/navigation"; |
| return res.status(500).json({ error: 'OIDC login failed', details: (err as any)?.message }); | ||
| } | ||
| }); | ||
| }) |
There was a problem hiding this comment.
Avoid automated semicolon insertion (94% of all statements in the enclosing script have an explicit semicolon).
3738faa to
040000e
Compare
Adds optional OIDC authentication to allow user to rely on external auth providers to access Lidify, which closes issues #33 and #19.
Changes
oidc-client-tsand added a separate callback page.oidcIdandoidcUidfields to user table to match OIDC user directly to Lidify user.