feat: initial support for OpenID Connect authentication#2715
Open
michaelhthomas wants to merge 29 commits intoseerr-team:developfrom
Open
feat: initial support for OpenID Connect authentication#2715michaelhthomas wants to merge 29 commits intoseerr-team:developfrom
michaelhthomas wants to merge 29 commits intoseerr-team:developfrom
Conversation
📝 WalkthroughWalkthroughAdds OpenID Connect (OIDC) support across server and client: discovery, PKCE/state flows, token exchange, optional userinfo, LinkedAccount entity and migrations, API endpoints for login/callback and linked-account management, client utilities and UI components, settings/schema/types updates, tests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (Client)
participant Seerr as Seerr Server
participant Provider as OIDC Provider
Browser->>Seerr: GET /api/v1/auth/oidc/login/:slug?returnUrl=...
activate Seerr
Seerr->>Provider: GET /.well-known/openid-configuration
Provider-->>Seerr: discovery metadata
Seerr->>Seerr: generate PKCE verifier/challenge & state, set signed cookies
Seerr-->>Browser: { redirectUrl } (authorization URL)
deactivate Seerr
Browser->>Provider: GET /authorize?code_challenge=...&state=...
Provider-->>Browser: redirect to callback (code & state)
Browser->>Seerr: POST /api/v1/auth/oidc/callback/:slug { callbackUrl }
activate Seerr
Seerr->>Seerr: read & clear signed cookies (verifier, state)
Seerr->>Provider: POST /token (code, code_verifier)
Provider-->>Seerr: { id_token, access_token }
Seerr->>Provider: GET /userinfo (optional)
Provider-->>Seerr: userinfo
Seerr->>Seerr: validate claims, find/create LinkedAccount & User, set session
Seerr-->>Browser: 204 No Content
deactivate Seerr
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
michaelhthomas
changed the title
michaelhthomas
force-pushed
the
feat/oidc-login-basic
branch
from
c8f20ec to
995628b
Compare
michaelhthomas
commented
Mar 17, 2026
michaelhthomas
force-pushed
the
feat/oidc-login-basic
branch
from
995628b to
20ffef0
Compare
3 tasks
michaelhthomas
force-pushed
the
feat/oidc-login-basic
branch
from
20ffef0 to
ede8be1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
src/utils/oidc.ts (1)
49-59: Consider storing provider slug after successful API response.The provider slug is stored in localStorage before the API call. If
axios.getfails, the stale slug remains. While this doesn't cause incorrect behavior (callers handle errors, and the slug gets overwritten on retry), movinglocalStorage.setItemafter a successful response would be cleaner:♻️ Optional cleanup for error resilience
export async function initiateOidcLogin( providerSlug: string, returnUrl: string ): Promise<void> { - localStorage.setItem(OIDC_PROVIDER_KEY, providerSlug); const res = await axios.get<{ redirectUrl: string }>( `/api/v1/auth/oidc/login/${encodeURIComponent(providerSlug)}`, { params: { returnUrl } } ); + localStorage.setItem(OIDC_PROVIDER_KEY, providerSlug); window.location.href = res.data.redirectUrl; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/oidc.ts` around lines 49 - 59, The code stores OIDC_PROVIDER_KEY in localStorage before calling the network request in initiateOidcLogin, leaving a stale entry if axios.get throws; move localStorage.setItem(OIDC_PROVIDER_KEY, providerSlug) to after the successful response (after the axios.get resolves and before setting window.location.href = res.data.redirectUrl) so the provider slug is only persisted on a successful API response and not on failures.src/components/Common/Button/index.tsx (1)
105-116: Apply the same width constraint to the anchor branch for consistency.
max-w-fullis only on the<button>path right now, soas="a"can still behave differently on long content.Proposed consistency patch
- <span className="flex items-center">{children}</span> + <span className="flex max-w-full items-center">{children}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Common/Button/index.tsx` around lines 105 - 116, The anchor branch rendering when as="a" uses <span className="flex items-center">{children}</span> which lacks the max-w-full constraint present in the <button> branch; update the anchor branch’s span to include max-w-full (i.e., change the span class to "flex max-w-full items-center") so both branches use the same width constraint and retain consistent behavior; locate this in the component that builds the anchor path alongside buttonStyle/twMerge, ref, children and apply the same class change.seerr-api.yml (1)
258-260: Consider setting an upper bound foropenIdProviders.Adding a
maxItemslimit on this array improves schema hardening and addresses the static-analysis warning for unbounded arrays.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seerr-api.yml` around lines 258 - 260, The openIdProviders array schema is unbounded; add a sensible maxItems constraint to the array definition for openIdProviders to harden the schema and satisfy static analysis. Locate the array that references components/schemas/PublicOidcProvider (the openIdProviders entry) and add a maxItems property (e.g., maxItems: 50 or another project-appropriate limit) alongside type: array and items: $ref: '#/components/schemas/PublicOidcProvider' so the OpenAPI spec enforces an upper bound.server/routes/auth.ts (3)
959-970: RedundantuserRepository.save(user)call.The user is saved on line 959, then
linkedAccountsis assigned and saved again on line 970. The second save only updates the in-memory relation array, which doesn't require a database write since theLinkedAccountalready has the correctuserIdforeign key set.♻️ Remove redundant save
await userRepository.save(user); const linkedAccount = new LinkedAccount({ user, provider: provider.slug, sub: fullUserInfo.sub, username: fullUserInfo.preferred_username ?? fullUserInfo.email, }); await linkedAccountsRepository.save(linkedAccount); - - user.linkedAccounts = [linkedAccount]; - await userRepository.save(user); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 959 - 970, The second database write is redundant: after creating and persisting LinkedAccount (linkedAccount and linkedAccountsRepository.save), you only set the in-memory relation user.linkedAccounts = [linkedAccount] but then call await userRepository.save(user) again; remove that second await userRepository.save(user) call and keep only the initial await userRepository.save(user) before creating the LinkedAccount (or, if you need the relation in-memory, set user.linkedAccounts = [linkedAccount] without saving). Ensure LinkedAccount construction uses user (or user.id) so the foreign key is persisted by linkedAccountsRepository.save and no extra userRepository.save is performed.
703-731: Consider addingSameSiteattribute to OIDC cookies for improved security.The OIDC flow cookies (
oidc-code-verifierandoidc-state) are set without an explicitSameSiteattribute. AddingSameSite: 'Lax'ensures the cookies are sent during the OIDC redirect while providing protection against CSRF in other contexts.🔒 Proposed cookie configuration
res.cookie('oidc-code-verifier', code_verifier, { maxAge: 60000, httpOnly: true, secure: req.protocol === 'https', + sameSite: 'lax', }); // ... and similarly for oidc-state cookie: res.cookie('oidc-state', state, { maxAge: 60000, httpOnly: true, secure: req.protocol === 'https', + sameSite: 'lax', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 703 - 731, Add an explicit SameSite attribute (SameSite: 'Lax') to the OIDC flow cookies so they are sent during the auth redirect but provide CSRF protection otherwise; update the res.cookie calls that set 'oidc-code-verifier' and 'oidc-state' (the res.cookie(...) invocations in this block) to include SameSite: 'Lax' in their options objects alongside maxAge, httpOnly and secure.
768-793: OIDC provider discovery is performed twice (login and callback).The
openIdClient.discovery()call is made in both the login endpoint (line 675) and the callback endpoint (line 770). While this ensures fresh configuration, it adds latency and an external network call on every callback. Consider caching the configuration with a reasonable TTL if performance becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 768 - 793, The OIDC discovery call (openIdClient.discovery) is being invoked in both the login and callback flows causing duplicate external calls; introduce a simple in-memory cache keyed by provider.issuerUrl (and clientId) at module scope to store the resulting openIdClient.Configuration with a TTL, update both places (where openIdClient.discovery is called) to first check the cache and return the cached Configuration if still valid, and fall back to calling openIdClient.discovery and storing the fresh result on success; ensure the cache entry stores the timestamp so entries older than the TTL are refreshed, and use the same cache key when referencing provider.issuerUrl/provider.clientId to locate the stored config for functions handling login and callback.server/migration/sqlite/1742858484395-AddLinkedAccounts.ts (1)
6-9: Same recommendation: add a unique constraint on(provider, sub)for SQLite.For consistency with the Postgres migration recommendation, consider adding a unique index on
(provider, sub)to enforce data integrity at the database level.🔒 Proposed migration change
public async up(queryRunner: QueryRunner): Promise<void> { await queryRunner.query( `CREATE TABLE "linked_accounts" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "provider" varchar(255) NOT NULL, "sub" varchar(255) NOT NULL, "username" varchar NOT NULL, "userId" integer, CONSTRAINT "FK_2c77d2a0c06eeab6e62dc35af64" FOREIGN KEY ("userId") REFERENCES "user" ("id") ON DELETE CASCADE ON UPDATE NO ACTION)` ); + await queryRunner.query( + `CREATE UNIQUE INDEX "UQ_linked_accounts_provider_sub" ON "linked_accounts" ("provider", "sub")` + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/migration/sqlite/1742858484395-AddLinkedAccounts.ts` around lines 6 - 9, The migration currently creates the "linked_accounts" table in AddLinkedAccounts.ts within the up(queryRunner: QueryRunner) method but lacks a uniqueness constraint on (provider, sub); update the migration to enforce uniqueness by either adding UNIQUE("provider","sub") to the CREATE TABLE statement for "linked_accounts" or issuing a CREATE UNIQUE INDEX (e.g., IDX_linked_accounts_provider_sub) immediately after table creation using queryRunner.query, and ensure the corresponding down migration drops that unique index/constraint when rolling back.server/routes/auth.test.ts (1)
595-622: Consider cleaning up LinkedAccount records between tests to ensure test isolation.The test creates a
LinkedAccountrecord that may persist across tests, potentially causing test pollution. Each test in the OIDC suite that creates linked accounts should clean up after itself or use a fresh database state.Similar pattern exists in lines 650-665.
🧹 Suggested cleanup approach
it('callback endpoint successfully authorizes existing user', async function () { // Link the seeded friend user to the OIDC provider const userRepo = getRepository(User); const linkedAccountRepo = getRepository(LinkedAccount); const user = await userRepo.findOneOrFail({ where: { email: 'friend@seerr.dev' }, }); const linkedAccount = new LinkedAccount({ user, provider: 'test', sub: EXISTING_USER_CLAIMS.sub, username: 'friend', }); await linkedAccountRepo.save(linkedAccount); // Setup mock to return the existing user's claims await setupFetchMock({ supportsPKCE: false, idTokenClaims: EXISTING_USER_CLAIMS, userinfoResponse: EXISTING_USER_CLAIMS, }); const response = await performOidcCallback(); assert.strictEqual(response.status, 204); + + // Cleanup + await linkedAccountRepo.delete({ id: linkedAccount.id }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.test.ts` around lines 595 - 622, This test creates a persistent LinkedAccount record (via new LinkedAccount and linkedAccountRepo.save) which can pollute other OIDC tests; update the test (or the OIDC test suite setup/teardown) to remove the created LinkedAccount after the test or reset the repository before each test: locate the usage of LinkedAccount/linkedAccountRepo and either call linkedAccountRepo.delete(...) or use an afterEach/beforeEach to clear LinkedAccount entries (or truncate the table) so performOidcCallback and subsequent tests run against an isolated DB state; ensure the cleanup references the same provider/sub or the saved entity instance to remove the exact record.server/migration/postgres/1742858617989-AddLinkedAccount.ts (1)
6-12: Consider adding a unique constraint on(provider, sub)to prevent duplicate linked accounts.The combination of
providerandsubshould be unique to prevent the same OIDC identity from being linked multiple times. Without this constraint, the same OIDC account could potentially be linked to multiple users, bypassing the application-level check in the callback handler if there's a race condition.🔒 Proposed migration change
public async up(queryRunner: QueryRunner): Promise<void> { await queryRunner.query( `CREATE TABLE "linked_accounts" ("id" SERIAL NOT NULL, "provider" character varying(255) NOT NULL, "sub" character varying(255) NOT NULL, "username" character varying NOT NULL, "userId" integer, CONSTRAINT "PK_445bf7a50aeeb7f0084052935a6" PRIMARY KEY ("id"))` ); await queryRunner.query( `ALTER TABLE "linked_accounts" ADD CONSTRAINT "FK_2c77d2a0c06eeab6e62dc35af64" FOREIGN KEY ("userId") REFERENCES "user"("id") ON DELETE CASCADE ON UPDATE NO ACTION` ); + await queryRunner.query( + `CREATE UNIQUE INDEX "UQ_linked_accounts_provider_sub" ON "linked_accounts" ("provider", "sub")` + ); }And update
down()to drop the index before the table.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/migration/postgres/1742858617989-AddLinkedAccount.ts` around lines 6 - 12, Add a unique constraint on the linked_accounts table for the (provider, sub) pair to prevent duplicate OIDC identities: modify the up() migration (in AddLinkedAccount.ts -> up(queryRunner: QueryRunner)) to add a UNIQUE constraint or unique index on ("provider","sub") after creating "linked_accounts", and update down(queryRunner: QueryRunner) to drop that unique index/constraint before dropping the "linked_accounts" table so rollback succeeds.src/components/Login/LoginButton.tsx (1)
11-34: Potential conflict ifdisabledis passed viabuttonProps.If a consumer passes
disabled={true}throughbuttonProps, it will be overwritten bydisabled={loading}(spread happens before the explicitdisabledprop). This could be intentional (loading always controls disabled state) or a subtle bug. Consider either:
- Documenting this behavior
- Merging the disabled states:
disabled={loading || buttonProps.disabled}♻️ Option to merge disabled states
export default function LoginButton({ loading, className, children, ...buttonProps }: LoginButtonProps) { return ( <Button className={twMerge( 'relative min-w-0 flex-grow bg-transparent disabled:opacity-50', className )} - disabled={loading} {...buttonProps} + disabled={loading || buttonProps.disabled} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Login/LoginButton.tsx` around lines 11 - 34, The component LoginButton currently spreads buttonProps into the Button and then sets disabled={loading}, which overwrites any disabled passed by consumers; update LoginButton so the disabled state is merged instead of overwritten: compute a combinedDisabled from loading and buttonProps.disabled (or remove disabled from buttonProps before spreading) and pass that combinedDisabled to the Button (reference LoginButton, buttonProps, loading, and the Button element), ensuring consumer-provided disabled is respected while still disabling during loading.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seerr-api.yml`:
- Around line 257-260: MainSettings now includes the openIdProviders array, but
PublicSettings (used by the /settings/public response) lacks this field; update
the PublicSettings schema to add an openIdProviders property with type: array
and items: $ref: '#/components/schemas/PublicOidcProvider' (or include it as
nullable/optional if intended), and ensure the /settings/public response
references the updated PublicSettings so generated clients see the same contract
as MainSettings.
In `@server/routes/auth.ts`:
- Around line 864-880: The current required-claims check in hasRequiredClaims
(which iterates requiredClaims and reads fullUserInfo[claim]) only accepts
boolean true and will reject valid non-boolean claim values; change the
predicate to verify claim presence/truthiness instead of strict === true (e.g.,
treat any non-null/undefined or any truthy value as valid by using a check like
value !== undefined && value !== null or Boolean(value)), or if you require
booleans, add documentation and keep the strict check; update references in the
error/logging path (logger.info using provider.requiredClaims and the next(...)
unauthorized branch) only if you change semantics so logs remain accurate.
In `@server/routes/user/usersettings.ts`:
- Around line 529-546: Current logic returns [] when settings.main.oidcLogin is
false and also filters LinkedAccount.find by activeProviders, hiding accounts
for disabled/removed providers; change this so linked accounts are always
fetched for the user (use linkedAccountsRepository.find with where.user.id only)
regardless of settings.main.oidcLogin or settings.oidc.providers, then compute
an "isActive" flag per account by checking activeProviders =
settings.oidc.providers.map(p=>p.slug) and
activeProviders.includes(account.provider) so the UI can show/unlink accounts
for inactive/removed providers; keep using LinkedAccount,
linkedAccountsRepository and req.params.id to locate records.
In `@src/components/Login/OidcLoginButton.tsx`:
- Around line 9-10: Replace App Router hooks with Pages Router APIs in
OidcLoginButton: import useRouter from 'next/router' (not next/navigation),
remove useSearchParams, and switch logic that reads query params to use
router.query. In the effects that rely on query/OIDC params (the useEffect
blocks around the logic referenced at lines ~55 and ~61), add a guard checking
router.isReady before accessing router.query and include router.isReady and
relevant router.query keys in the dependency arrays so the effects re-run when
the router becomes ready; also handle possibly undefined query values
(null/undefined) before using them to trigger the OIDC callback or provider
flow. Ensure all references to useSearchParams and next/navigation hooks are
replaced with useRouter().query and readiness checks.
In
`@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx`:
- Around line 313-323: The current condition lets a user unlink an OIDC account
even when it is their only login method; update the ConfirmButton onclick guard
around deleteRequest to block unlinking when acct.type ===
LinkedAccountType.OpenIdConnect and the user has no password and the account is
not a root/admin (i.e., implement or call a helper like canUnlinkOidc(user,
acct) or isLastOidcLogin(user, acct) and only call deleteRequest when that
returns true), keep enableMediaServerUnlink behavior for Plex/Jellyfin
unchanged, and surface a disabled state or tooltip on the ConfirmButton when
unlinking is disallowed.
- Around line 91-114: The router.replace call in the useEffect that handles the
OIDC callback currently clears all query params; update it to preserve existing
router.query values while removing only the OIDC-specific params (e.g.,
code/state) so route params like userId are not lost. In the useEffect (around
getOidcProviderSlug, clearOidcProviderSlug, processOidcCallback), build a
newQuery object from router.query, delete the OIDC keys (code, state, etc.), and
pass that newQuery as the second argument to router.replace(router.pathname, {
query: newQuery }, { shallow: true }) so you preserve other query params while
stripping OIDC parameters. Ensure this change is applied where router.replace is
called and keep existing calls to getOidcProviderSlug, clearOidcProviderSlug,
processOidcCallback, revalidateLinkedAccounts, and setError unchanged.
---
Nitpick comments:
In `@seerr-api.yml`:
- Around line 258-260: The openIdProviders array schema is unbounded; add a
sensible maxItems constraint to the array definition for openIdProviders to
harden the schema and satisfy static analysis. Locate the array that references
components/schemas/PublicOidcProvider (the openIdProviders entry) and add a
maxItems property (e.g., maxItems: 50 or another project-appropriate limit)
alongside type: array and items: $ref: '#/components/schemas/PublicOidcProvider'
so the OpenAPI spec enforces an upper bound.
In `@server/migration/postgres/1742858617989-AddLinkedAccount.ts`:
- Around line 6-12: Add a unique constraint on the linked_accounts table for the
(provider, sub) pair to prevent duplicate OIDC identities: modify the up()
migration (in AddLinkedAccount.ts -> up(queryRunner: QueryRunner)) to add a
UNIQUE constraint or unique index on ("provider","sub") after creating
"linked_accounts", and update down(queryRunner: QueryRunner) to drop that unique
index/constraint before dropping the "linked_accounts" table so rollback
succeeds.
In `@server/migration/sqlite/1742858484395-AddLinkedAccounts.ts`:
- Around line 6-9: The migration currently creates the "linked_accounts" table
in AddLinkedAccounts.ts within the up(queryRunner: QueryRunner) method but lacks
a uniqueness constraint on (provider, sub); update the migration to enforce
uniqueness by either adding UNIQUE("provider","sub") to the CREATE TABLE
statement for "linked_accounts" or issuing a CREATE UNIQUE INDEX (e.g.,
IDX_linked_accounts_provider_sub) immediately after table creation using
queryRunner.query, and ensure the corresponding down migration drops that unique
index/constraint when rolling back.
In `@server/routes/auth.test.ts`:
- Around line 595-622: This test creates a persistent LinkedAccount record (via
new LinkedAccount and linkedAccountRepo.save) which can pollute other OIDC
tests; update the test (or the OIDC test suite setup/teardown) to remove the
created LinkedAccount after the test or reset the repository before each test:
locate the usage of LinkedAccount/linkedAccountRepo and either call
linkedAccountRepo.delete(...) or use an afterEach/beforeEach to clear
LinkedAccount entries (or truncate the table) so performOidcCallback and
subsequent tests run against an isolated DB state; ensure the cleanup references
the same provider/sub or the saved entity instance to remove the exact record.
In `@server/routes/auth.ts`:
- Around line 959-970: The second database write is redundant: after creating
and persisting LinkedAccount (linkedAccount and linkedAccountsRepository.save),
you only set the in-memory relation user.linkedAccounts = [linkedAccount] but
then call await userRepository.save(user) again; remove that second await
userRepository.save(user) call and keep only the initial await
userRepository.save(user) before creating the LinkedAccount (or, if you need the
relation in-memory, set user.linkedAccounts = [linkedAccount] without saving).
Ensure LinkedAccount construction uses user (or user.id) so the foreign key is
persisted by linkedAccountsRepository.save and no extra userRepository.save is
performed.
- Around line 703-731: Add an explicit SameSite attribute (SameSite: 'Lax') to
the OIDC flow cookies so they are sent during the auth redirect but provide CSRF
protection otherwise; update the res.cookie calls that set 'oidc-code-verifier'
and 'oidc-state' (the res.cookie(...) invocations in this block) to include
SameSite: 'Lax' in their options objects alongside maxAge, httpOnly and secure.
- Around line 768-793: The OIDC discovery call (openIdClient.discovery) is being
invoked in both the login and callback flows causing duplicate external calls;
introduce a simple in-memory cache keyed by provider.issuerUrl (and clientId) at
module scope to store the resulting openIdClient.Configuration with a TTL,
update both places (where openIdClient.discovery is called) to first check the
cache and return the cached Configuration if still valid, and fall back to
calling openIdClient.discovery and storing the fresh result on success; ensure
the cache entry stores the timestamp so entries older than the TTL are
refreshed, and use the same cache key when referencing
provider.issuerUrl/provider.clientId to locate the stored config for functions
handling login and callback.
In `@src/components/Common/Button/index.tsx`:
- Around line 105-116: The anchor branch rendering when as="a" uses <span
className="flex items-center">{children}</span> which lacks the max-w-full
constraint present in the <button> branch; update the anchor branch’s span to
include max-w-full (i.e., change the span class to "flex max-w-full
items-center") so both branches use the same width constraint and retain
consistent behavior; locate this in the component that builds the anchor path
alongside buttonStyle/twMerge, ref, children and apply the same class change.
In `@src/components/Login/LoginButton.tsx`:
- Around line 11-34: The component LoginButton currently spreads buttonProps
into the Button and then sets disabled={loading}, which overwrites any disabled
passed by consumers; update LoginButton so the disabled state is merged instead
of overwritten: compute a combinedDisabled from loading and buttonProps.disabled
(or remove disabled from buttonProps before spreading) and pass that
combinedDisabled to the Button (reference LoginButton, buttonProps, loading, and
the Button element), ensuring consumer-provided disabled is respected while
still disabling during loading.
In `@src/utils/oidc.ts`:
- Around line 49-59: The code stores OIDC_PROVIDER_KEY in localStorage before
calling the network request in initiateOidcLogin, leaving a stale entry if
axios.get throws; move localStorage.setItem(OIDC_PROVIDER_KEY, providerSlug) to
after the successful response (after the axios.get resolves and before setting
window.location.href = res.data.redirectUrl) so the provider slug is only
persisted on a successful API response and not on failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e26bc5ae-2f0b-4ecb-bc45-534e860d9aa8
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpublic/images/openid.svgis excluded by!**/*.svg
📒 Files selected for processing (24)
package.jsonseerr-api.ymlserver/constants/error.tsserver/entity/LinkedAccount.tsserver/entity/User.tsserver/index.tsserver/interfaces/api/settingsInterfaces.tsserver/interfaces/api/userSettingsInterfaces.tsserver/lib/settings/index.tsserver/migration/postgres/1742858617989-AddLinkedAccount.tsserver/migration/sqlite/1742858484395-AddLinkedAccounts.tsserver/routes/auth.test.tsserver/routes/auth.tsserver/routes/user/usersettings.tssrc/components/Common/Button/index.tsxsrc/components/Login/LoginButton.tsxsrc/components/Login/OidcLoginButton.tsxsrc/components/Login/PlexLoginButton.tsxsrc/components/Login/index.tsxsrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/context/SettingsContext.tsxsrc/i18n/locale/en.jsonsrc/pages/_app.tsxsrc/utils/oidc.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Adds initial OpenID Connect (OIDC) authentication support across the server and UI, enabling configured providers to appear on the login screen and allowing users to link/unlink OIDC identities to their existing accounts.
Changes:
- Add OIDC auth endpoints (
/auth/oidc/login/:slug,/auth/oidc/callback/:slug) plus linked-account persistence. - Expose configured public OIDC providers via public settings and surface them in the login + “Linked Accounts” UI.
- Add DB migrations, API schema/types, and server-side tests for OIDC flows.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/oidc.ts | Client-side helpers for initiating OIDC login, handling callbacks, and mapping error codes to i18n messages |
| src/pages/_app.tsx | Adds openIdProviders to default public settings and avoids redirecting away from login during OIDC callback (?code=) |
| src/i18n/locale/en.json | Adds English strings for OIDC-specific error messages |
| src/context/SettingsContext.tsx | Extends default settings to include openIdProviders |
| src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx | Adds OIDC linked-account listing, linking via provider buttons, callback handling, and unlink support |
| src/components/Login/index.tsx | Renders OIDC login buttons and introduces shared LoginButton styling/behavior |
| src/components/Login/PlexLoginButton.tsx | Refactors to use LoginButton and unify loading UI |
| src/components/Login/OidcLoginButton.tsx | New OIDC provider login button with callback handling |
| src/components/Login/LoginButton.tsx | New shared button wrapper with consistent styling + loading spinner |
| src/components/Common/Button/index.tsx | Exports ButtonProps and improves child layout to better support truncation |
| server/routes/user/usersettings.ts | Adds linked-account list + delete endpoints for OIDC accounts |
| server/routes/auth.ts | Implements OIDC discovery, auth URL creation, callback/token exchange, linking, and optional new-user creation |
| server/routes/auth.test.ts | Adds server tests for OIDC login/callback, PKCE/state handling, new-user creation rules, and error cases |
| server/migration/sqlite/1742858484395-AddLinkedAccounts.ts | SQLite migration creating linked_accounts table |
| server/migration/postgres/1742858617989-AddLinkedAccount.ts | Postgres migration creating linked_accounts table |
| server/lib/settings/index.ts | Adds OIDC settings types, public provider exposure, deep-merge improvements, and a reset() helper |
| server/interfaces/api/userSettingsInterfaces.ts | Adds API types for linked OIDC accounts |
| server/interfaces/api/settingsInterfaces.ts | Adds openIdProviders to public settings response type |
| server/index.ts | Extends API error response to optionally include an error code field |
| server/entity/User.ts | Adds linkedAccounts relation |
| server/entity/LinkedAccount.ts | New entity for persisting OIDC linked identities |
| server/constants/error.ts | Adds OIDC-related API error codes |
| seerr-api.yml | Documents new OIDC endpoints, linked accounts endpoints, and provider schemas |
| public/images/openid.svg | Adds OpenID fallback icon asset |
| package.json | Adds openid-client runtime dependency and test deps (fetch-mock, jose) |
| pnpm-lock.yaml | Locks new dependencies |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Member
|
Please use the dedicated thread to discuss about anything not related to the code review PR: #2721 |
michaelhthomas
force-pushed
the
feat/oidc-login-basic
branch
from
ede8be1 to
8d1ddac
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx (1)
168-208:⚠️ Potential issue | 🟡 MinorUse a stable unique key for each link option instead of the display name.
The current code uses
key={name}at line 271, but the Login component elsewhere in this codebase establishes the pattern of usingprovider.slugfor OIDC provider keys. Since OIDC provider names are admin-defined and can collide with each other or with the built-in names (Plex, Jellyfin, Emby), duplicate keys will cause React to reuse the wrong dropdown state.♻️ Suggested change
const linkable = [ { + key: 'plex', name: 'Plex', action: () => { plexOAuth.preparePopup(); setTimeout(() => linkPlexAccount(), 1500); }, @@ { + key: 'jellyfin', name: 'Jellyfin', action: () => setShowJellyfinModal(true), hide: settings.currentSettings.mediaServerType !== MediaServerType.JELLYFIN || accounts.some((a) => a.type === LinkedAccountType.Jellyfin), }, { + key: 'emby', name: 'Emby', action: () => setShowJellyfinModal(true), hide: settings.currentSettings.mediaServerType !== MediaServerType.EMBY || accounts.some((a) => a.type === LinkedAccountType.Emby), }, ...settings.currentSettings.openIdProviders.map((p) => ({ + key: `oidc:${p.slug}`, name: p.name, action: async () => { try { await initiateOidcLogin(p.slug, window.location.href); @@ {currentUser?.id === user?.id && !!linkable.length && ( <div> <Dropdown text="Link Account" buttonType="ghost"> - {linkable.map(({ name, action }) => ( - <Dropdown.Item key={name} onClick={action} buttonType="ghost"> + {linkable.map(({ key, name, action }) => ( + <Dropdown.Item key={key} onClick={action} buttonType="ghost"> {name} </Dropdown.Item> ))} </Dropdown> </div>Also applies to: 267-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx` around lines 168 - 208, The linkable array builds menu items using display names which can collide; change the items to include a stable unique id for each entry and use that as the React key (e.g., add an id/slug field for built-ins and OIDC entries). For built-ins (Plex, Jellyfin, Emby) set id values like 'plex', 'jellyfin', 'emby', and for OIDC providers use the provider.slug from settings.currentSettings.openIdProviders; ensure the component rendering these items (where key={name}) uses that id instead of name and that the action handlers (plexOAuth.preparePopup, linkPlexAccount, setShowJellyfinModal, initiateOidcLogin) remain unchanged.
♻️ Duplicate comments (2)
server/routes/user/usersettings.ts (1)
569-583:⚠️ Potential issue | 🟠 MajorEnforce the unlink safety check on the API too.
The UI now blocks removing the last login path, but this endpoint still deletes any matching OIDC link. A passwordless
UserType.LOCALuser with a single linked provider can call this route directly and strand the account.🔒 Suggested guard
userSettingsRoutes.delete<{ id: string; acctId: string }>( '/linked-accounts/:acctId', isOwnProfileOrAdmin(), async (req, res) => { + const userRepository = getRepository(User); const linkedAccountsRepository = getRepository(LinkedAccount); + const user = await userRepository.findOne({ + select: ['id', 'password', 'userType'], + where: { id: Number(req.params.id) }, + }); + + if (!user) { + return res.status(404).send(); + } + const condition: FindOptionsWhere<LinkedAccount> = { id: Number(req.params.acctId), user: { - id: Number(req.params.id), + id: user.id, }, }; if (await linkedAccountsRepository.exist({ where: condition })) { + const linkedAccountCount = await linkedAccountsRepository.count({ + where: { user: { id: user.id } }, + }); + const hasAlternativeLogin = + !!user.password || + user.userType !== UserType.LOCAL || + linkedAccountCount > 1; + + if (!hasAlternativeLogin) { + return res.status(400).json({ + message: 'User must keep at least one login method.', + }); + } + await linkedAccountsRepository.delete(condition); return res.status(204).send(); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/user/usersettings.ts` around lines 569 - 583, The delete handler userSettingsRoutes.delete currently removes any matching LinkedAccount without ensuring it isn't the user's last authentication method; to fix, load the target linked account (via linkedAccountsRepository.findOne with relations ['user']) and/or count the user's linked accounts (linkedAccountsRepository.count({ where: { user: { id: ... } } })), then if the owning user has UserType.LOCAL and the count === 1 and the user has no local credential (e.g., no passwordHash / password field), block the deletion and return an error (409 or 400) instead of deleting; only proceed to linkedAccountsRepository.delete(condition) when this safety check passes. Ensure you reference LinkedAccount, userSettingsRoutes.delete, and UserType.LOCAL in the change.seerr-api.yml (1)
257-260:⚠️ Potential issue | 🟡 Minor
openIdProvidersis still missing from the public settings schema.The provider list was added to
MainSettings, but/settings/publicstill returnsPublicSettings, which does not defineopenIdProviders. That leaves the unauthenticated login contract out of sync with the new OIDC surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seerr-api.yml` around lines 257 - 260, PublicSettings is missing the new openIdProviders list introduced in MainSettings, causing /settings/public to omit the OIDC provider surface; add an openIdProviders property to the PublicSettings schema that mirrors MainSettings by declaring type: array and items: $ref: '#/components/schemas/PublicOidcProvider' (make it optional if MainSettings treats it as optional) so the unauthenticated settings contract returned by /settings/public includes the provider list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seerr-api.yml`:
- Around line 4055-4062: Change the OpenAPI parameter definition for returnUrl
to accept URI references instead of only absolute URIs: locate the parameter
named returnUrl in the OpenAPI spec (the parameter block containing schema:
type: string and format: uri) and replace format: uri with format: uri-reference
so relative paths like /login are valid; ensure the description and
allowReserved remain unchanged and the schema remains type: string to preserve
backward compatibility.
In `@server/routes/auth.ts`:
- Around line 935-977: The signup creates a User then a LinkedAccount separately
so a failure saving LinkedAccount leaves a stray User; wrap both writes in a
single transaction using the repository/ORM transaction API (e.g., call
userRepository.manager.transaction or similar) so creation of User and
LinkedAccount happens atomically; inside the transaction create the new User
(User constructor), save via userRepository.save, create LinkedAccount
(LinkedAccount constructor) and save via linkedAccountsRepository.save using the
same transactional entity manager, set user.linkedAccounts and save user before
committing, and keep the existing checks (existingUser, provider.newUserLogin,
fullUserInfo.email) outside or as preconditions.
- Around line 700-707: The current code stores a single fixed cookie
'oidc-code-verifier' with a 60s TTL which collides across concurrent OIDC flows
and can expire too soon; instead generate a unique per-auth-flow key (use the
existing OIDC state or generate a UUID) and store the PKCE code_verifier keyed
to that value (e.g., cookie name like oidc-code-verifier-<state> or store a
small server-side map keyed by state) and set the cookie/entry TTL to match the
auth-code lifetime (several minutes) so callbacks can retrieve the correct
code_verifier; update all usages that set/read code_verifier and state
(including the code_challenge creation and the callback handlers referenced
around the other occurrences) to use the per-flow key.
In `@src/components/Login/OidcLoginButton.tsx`:
- Around line 53-65: The effect in OidcLoginButton currently only checks
query.code and ignores OIDC error callbacks (query.error); update the useEffect
guard to also detect when query.error is present for the same provider
(getOidcProviderSlug() === provider.slug), call clearOidcProviderSlug(), and
invoke handleCallback() so error callbacks are processed; apply the same change
to the duplicate logic in UserLinkedAccountsSettings (same query/provider slug
checks) so cancelled/denied authorizations are handled consistently.
---
Outside diff comments:
In
`@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx`:
- Around line 168-208: The linkable array builds menu items using display names
which can collide; change the items to include a stable unique id for each entry
and use that as the React key (e.g., add an id/slug field for built-ins and OIDC
entries). For built-ins (Plex, Jellyfin, Emby) set id values like 'plex',
'jellyfin', 'emby', and for OIDC providers use the provider.slug from
settings.currentSettings.openIdProviders; ensure the component rendering these
items (where key={name}) uses that id instead of name and that the action
handlers (plexOAuth.preparePopup, linkPlexAccount, setShowJellyfinModal,
initiateOidcLogin) remain unchanged.
---
Duplicate comments:
In `@seerr-api.yml`:
- Around line 257-260: PublicSettings is missing the new openIdProviders list
introduced in MainSettings, causing /settings/public to omit the OIDC provider
surface; add an openIdProviders property to the PublicSettings schema that
mirrors MainSettings by declaring type: array and items: $ref:
'#/components/schemas/PublicOidcProvider' (make it optional if MainSettings
treats it as optional) so the unauthenticated settings contract returned by
/settings/public includes the provider list.
In `@server/routes/user/usersettings.ts`:
- Around line 569-583: The delete handler userSettingsRoutes.delete currently
removes any matching LinkedAccount without ensuring it isn't the user's last
authentication method; to fix, load the target linked account (via
linkedAccountsRepository.findOne with relations ['user']) and/or count the
user's linked accounts (linkedAccountsRepository.count({ where: { user: { id:
... } } })), then if the owning user has UserType.LOCAL and the count === 1 and
the user has no local credential (e.g., no passwordHash / password field), block
the deletion and return an error (409 or 400) instead of deleting; only proceed
to linkedAccountsRepository.delete(condition) when this safety check passes.
Ensure you reference LinkedAccount, userSettingsRoutes.delete, and
UserType.LOCAL in the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4487d7e8-04f7-4b4f-bb5d-1d0be67131a4
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpublic/images/openid.svgis excluded by!**/*.svg
📒 Files selected for processing (24)
package.jsonseerr-api.ymlserver/constants/error.tsserver/entity/LinkedAccount.tsserver/entity/User.tsserver/index.tsserver/interfaces/api/settingsInterfaces.tsserver/interfaces/api/userSettingsInterfaces.tsserver/lib/settings/index.tsserver/migration/postgres/1742858617989-AddLinkedAccount.tsserver/migration/sqlite/1742858484395-AddLinkedAccounts.tsserver/routes/auth.test.tsserver/routes/auth.tsserver/routes/user/usersettings.tssrc/components/Common/Button/index.tsxsrc/components/Login/LoginButton.tsxsrc/components/Login/OidcLoginButton.tsxsrc/components/Login/PlexLoginButton.tsxsrc/components/Login/index.tsxsrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/context/SettingsContext.tsxsrc/i18n/locale/en.jsonsrc/pages/_app.tsxsrc/utils/oidc.ts
✅ Files skipped from review due to trivial changes (6)
- src/context/SettingsContext.tsx
- server/interfaces/api/userSettingsInterfaces.ts
- package.json
- server/constants/error.ts
- server/entity/LinkedAccount.ts
- server/interfaces/api/settingsInterfaces.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/components/Common/Button/index.tsx
- server/index.ts
- server/migration/postgres/1742858617989-AddLinkedAccount.ts
- src/components/Login/LoginButton.tsx
- src/components/Login/PlexLoginButton.tsx
- server/migration/sqlite/1742858484395-AddLinkedAccounts.ts
- server/routes/auth.test.ts
- src/i18n/locale/en.json
- src/utils/oidc.ts
- server/lib/settings/index.ts
gauthier-th
reviewed
Mar 25, 2026
Member
gauthier-th
left a comment
gauthier-th
left a comment
There was a problem hiding this comment.
Overall looks good.
Documentation on how to setup OIDC should also be included in this PR.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
server/entity/User.ts (1)
104-105: KeeplinkedAccountsout of genericUser.filter()responses.
filter()serializes every enumerable property that is not listed infilteredFields. Once aUseris loaded withlinkedAccounts, generic user payloads can start leaking linked-provider usernames outside the dedicated linked-accounts settings API.♻️ Proposed guardrail
static readonly filteredFields: string[] = [ 'email', 'plexId', 'password', 'resetPasswordGuid', 'jellyfinDeviceId', 'jellyfinAuthToken', 'plexToken', 'settings', + 'linkedAccounts', ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/entity/User.ts` around lines 104 - 105, The User entity's linkedAccounts array is being serialized by the generic User.filter() path and can leak provider usernames; update the User.filter() implementation (or its filteredFields list) to explicitly exclude the linkedAccounts property by adding "linkedAccounts" to the filteredFields set or conditionally skipping properties named linkedAccounts, ensuring linked-provider data is only exposed via the dedicated linked-accounts API; reference the User.filter() method and the linkedAccounts property on the User class when making the change.src/components/Common/ButtonWithLoader/index.tsx (1)
19-23: KeepButtonWithLoaderbehavior-focused.
flex-growandbg-transparentare login-page styling choices, not loader behavior. Baking them into a sharedCommoncomponent will surprise the next caller and makes reuse harder.♻️ Proposed split between shared behavior and call-site styling
<Button className={twMerge( - 'relative min-w-0 flex-grow bg-transparent disabled:opacity-50', + 'relative min-w-0 disabled:opacity-50', className )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Common/ButtonWithLoader/index.tsx` around lines 19 - 23, The ButtonWithLoader component mixes behavior and page-specific styling by including 'flex-grow' and 'bg-transparent' in the default class list passed to Button; remove those layout/appearance classes and keep only behavior-focused classes (e.g., 'relative min-w-0 disabled:opacity-50') in the twMerge call inside ButtonWithLoader, leaving callers to supply layout classes via the className prop; update any callers that relied on those defaults (such as the login page) to add 'flex-grow' and 'bg-transparent' to their ButtonWithLoader usage so appearance is preserved.src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx (1)
287-289: Consider using stable keys instead of array index.Using index as
keycan cause unnecessary re-renders or state issues if the list order changes. Each account has a unique identifier available:♻️ Suggested improvement
- {accounts.map((acct, i) => ( - <li - key={i} + {accounts.map((acct) => ( + <li + key={ + acct.type === LinkedAccountType.OpenIdConnect + ? `oidc-${acct.id}` + : acct.type + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx` around lines 287 - 289, The list rendering uses the array index as the React key (in the accounts.map callback where key={i}), which can cause incorrect re-renders; change the key to a stable unique identifier from each account object (e.g., acct.id or acct.providerAccountId) in the <li> elements so React can track items reliably — locate the accounts.map(...) block and replace key={i} with key={acct.<uniqueIdField>} where <uniqueIdField> is the account's unique property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seerr-api.yml`:
- Around line 5191-5211: Add a 403 response to the DELETE "Remove a linked
account for a user" operation so permission denials are documented; update the
operation that takes path parameters userId and acctId to include a '403'
response entry (e.g., description: "Forbidden — insufficient permissions
(requires MANAGE_USERS to edit other users)") alongside the existing '204' and
'404' responses so clients can handle authorization failures explicitly.
- Around line 4063-4075: The OpenAPI spec for the /auth/oidc/callback/{slug}
route only documents a 200 redirectUrl and a 403 but omits the error payload
emitted when authorization URL generation fails; update the responses for this
operation to include the failure response (e.g., 400 or 500 as appropriate) and
add a JSON schema for the error payload (include properties like "error"
(string) and optional "error_description" (string) and "details" (object/string)
so clients know the contract). Ensure the new response entry references the same
path /auth/oidc/callback/{slug} and clearly documents the error payload shape
returned when authorization URL generation (OIDC initiation) fails.
In
`@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx`:
- Around line 248-250: canUnlinkMediaServer is too permissive compared to
server-side checks: it currently uses canUnlink (which allows accounts.length >
1) and user?.id !== 1, causing the UI to show unlink for media servers even when
the user lacks a local password/email and the server will reject the request;
update the logic in UserLinkedAccountsSettings where canUnlink and
canUnlinkMediaServer are defined so that canUnlinkMediaServer requires
passwordInfo?.hasPassword (and/or user.email) in addition to existing checks
(references: canUnlink, canUnlinkMediaServer, passwordInfo, accounts, user?.id)
so the client only shows the delete button when the server will accept the
unlink request.
---
Nitpick comments:
In `@server/entity/User.ts`:
- Around line 104-105: The User entity's linkedAccounts array is being
serialized by the generic User.filter() path and can leak provider usernames;
update the User.filter() implementation (or its filteredFields list) to
explicitly exclude the linkedAccounts property by adding "linkedAccounts" to the
filteredFields set or conditionally skipping properties named linkedAccounts,
ensuring linked-provider data is only exposed via the dedicated linked-accounts
API; reference the User.filter() method and the linkedAccounts property on the
User class when making the change.
In `@src/components/Common/ButtonWithLoader/index.tsx`:
- Around line 19-23: The ButtonWithLoader component mixes behavior and
page-specific styling by including 'flex-grow' and 'bg-transparent' in the
default class list passed to Button; remove those layout/appearance classes and
keep only behavior-focused classes (e.g., 'relative min-w-0
disabled:opacity-50') in the twMerge call inside ButtonWithLoader, leaving
callers to supply layout classes via the className prop; update any callers that
relied on those defaults (such as the login page) to add 'flex-grow' and
'bg-transparent' to their ButtonWithLoader usage so appearance is preserved.
In
`@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx`:
- Around line 287-289: The list rendering uses the array index as the React key
(in the accounts.map callback where key={i}), which can cause incorrect
re-renders; change the key to a stable unique identifier from each account
object (e.g., acct.id or acct.providerAccountId) in the <li> elements so React
can track items reliably — locate the accounts.map(...) block and replace
key={i} with key={acct.<uniqueIdField>} where <uniqueIdField> is the account's
unique property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edd2c691-69a7-417a-8804-4ea130a82b8f
📒 Files selected for processing (10)
seerr-api.ymlserver/entity/User.tsserver/index.tsserver/routes/auth.tssrc/components/Common/ButtonWithLoader/index.tsxsrc/components/Login/OidcLoginButton.tsxsrc/components/Login/PlexLoginButton.tsxsrc/components/Login/index.tsxsrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/utils/oidc.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/Login/OidcLoginButton.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Login/PlexLoginButton.tsx
- server/routes/auth.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/using-seerr/settings/users/oidc.md (1)
113-114: Consider recommending specific redirect URIs over wildcard.The guide suggests using a wildcard
/*as the valid redirect URL, which is permissive and allows any path. While this works, it's more secure to configure only the specific redirect URIs needed (as documented in lines 91-92 of the general setup section). Consider revising the Keycloak guide to recommend the specific paths instead of the wildcard.🔒 Proposed revision for more specific redirect URIs
-1. Finally, set the root url to your Seerr instance's URL, and a wildcard `/*` as a valid redirect URL. +1. Finally, set the root url to your Seerr instance's URL, and add the following as valid redirect URLs: + - `/login` + - `/profile/settings/linked-accounts` 🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/using-seerr/settings/users/oidc.md` around lines 113 - 114, The docs currently advise using a wildcard `/*` as the valid redirect URL in the Keycloak step (the paragraph that ends with "and a wildcard `/*` as a valid redirect URL" next to the Keycloak Step 4 image); update this to recommend and show the explicit redirect URIs used by Seerr (the specific paths mentioned in the general setup section around lines 91–92) instead of the wildcard, and adjust the explanatory text and the Keycloak Step 4 caption to list those exact redirect URIs for tighter security.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/using-seerr/settings/users/oidc.md`:
- Line 45: Fix the typo in the documentation sentence by changing "inital" to
"initial" in the line that reads "Unique identifier for the provider. This
should not be changed after inital setup." so it reads "Unique identifier for
the provider. This should not be changed after initial setup."
- Around line 55-59: Remove the tip block that references a non-existent "search
icon" UI and the selfh.st/icons browse feature (the text starting with "The
search icon at the right of the logo field opens the [selfh.st/icons]..."
through the closing :::), since OIDC provider UI is not implemented; update the
docs/using-seerr/settings/users/oidc.md content by deleting that tip paragraph
so the page only documents manual configuration in settings.json.
---
Nitpick comments:
In `@docs/using-seerr/settings/users/oidc.md`:
- Around line 113-114: The docs currently advise using a wildcard `/*` as the
valid redirect URL in the Keycloak step (the paragraph that ends with "and a
wildcard `/*` as a valid redirect URL" next to the Keycloak Step 4 image);
update this to recommend and show the explicit redirect URIs used by Seerr (the
specific paths mentioned in the general setup section around lines 91–92)
instead of the wildcard, and adjust the explanatory text and the Keycloak Step 4
caption to list those exact redirect URIs for tighter security.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92594e63-080c-4495-a930-807139133af7
⛔ Files ignored due to path filters (5)
docs/using-seerr/settings/users/assets/oidc_keycloak_1.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_2.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_3.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_4.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_5.pngis excluded by!**/*.png
📒 Files selected for processing (3)
docs/using-seerr/settings/users/index.mddocs/using-seerr/settings/users/oidc.mdgen-docs/docusaurus.config.ts
✅ Files skipped from review due to trivial changes (1)
- gen-docs/docusaurus.config.ts
michaelhthomas
force-pushed
the
feat/oidc-login-basic
branch
from
6ac0ab6 to
78a7d7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
seerr-api.yml (1)
257-260:⚠️ Potential issue | 🟡 Minor
openIdProvidersis modeled on the wrong settings schema.
MainSettingsis reused as the/settings/mainrequest schema, so adding this field here makes it look writable. The login flow needs the public provider list, butPublicSettingsstill doesn't declare it.Suggested change
MainSettings: type: object properties: @@ openIdProviders: type: array + readOnly: true items: $ref: '#/components/schemas/PublicOidcProvider' @@ PublicSettings: type: object properties: initialized: type: boolean example: false + openIdProviders: + type: array + items: + $ref: '#/components/schemas/PublicOidcProvider'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seerr-api.yml` around lines 257 - 260, The openIdProviders field was added to MainSettings (making it appear writable via the /settings/main schema) but should be exposed only on the public settings schema used by the login flow; remove the openIdProviders entry from MainSettings and instead add a declaration on PublicSettings as: openIdProviders: type: array, items: $ref: '#/components/schemas/PublicOidcProvider' (or equivalent), ensuring the PublicSettings schema exports the provider list while MainSettings no longer contains openIdProviders.src/components/Login/OidcLoginButton.tsx (1)
53-68:⚠️ Potential issue | 🟠 MajorAdd
router.isReadyguard to ensure query params are available.The effect runs once on mount with an empty dependency array. Since the login page is statically optimized (no
getServerSideProps),router.querywill be empty on the initial render until after hydration completes androuter.isReadybecomes true. This means if a user lands directly on/login?code=..., the effect runs with empty query params and the callback is never triggered, since the effect won't run again.Adding
router.isReadyto the guard ensures the query params are populated before the effect logic executes:useEffect(() => { - if (loading) return; + if (loading || !router.isReady) return; // OIDC provider has redirected back with an authorization code or error const isCallback = query.code != null || query.error != null; ... - }, []); + }, [router.isReady]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Login/OidcLoginButton.tsx` around lines 53 - 68, The effect that checks query params should wait for Next router readiness; update the useEffect so it returns early if loading is true or router.isReady is false, and add router.isReady to the dependency array so the effect reruns when query params populate; keep the existing checks for isCallback (using query.code/query.error) and the logic that calls getOidcProviderSlug(), clearOidcProviderSlug(), handleCallback(), and redirectToLogin() unchanged.
🧹 Nitpick comments (3)
server/routes/auth.test.ts (1)
462-487: Assert the authenticated session after the callback.
performOidcCallback()returns a bare response fromrequest(app), so the success-path tests never prove that the callback actually establishes a session. A regression that stops settingconnect.sidwould still pass.Suggested change
async function performOidcCallback() { - const loginResponse = await request(app) + const agent = request.agent(app); + const loginResponse = await agent .get('/auth/oidc/login/test') .set('Accept', 'application/json'); @@ - const response = await request(app) + const response = await agent .post('/auth/oidc/callback/test') .set('Accept', 'application/json') .set('Cookie', cookieHeader) .send({ callbackUrl: callbackUrl.toString() }); - return response; + return { agent, response }; }Then in the success-path tests:
const { agent, response } = await performOidcCallback(); assert.strictEqual(response.status, 204); const me = await agent.get('/auth/me'); assert.strictEqual(me.status, 200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.test.ts` around lines 462 - 487, performOidcCallback currently returns the raw response so tests never verify a session cookie (e.g. connect.sid); change performOidcCallback to use a persistent supertest agent (create with request.agent(app)), perform the login and the callback via that agent so cookies are preserved (use agent.get('/auth/oidc/login/test') and agent.post('/auth/oidc/callback/test')), then return both the agent and the callback response (e.g. { agent, response }). Update success-path tests to assert response.status === 204 and then call agent.get('/auth/me') and assert that returns 200 to prove the session was established.server/routes/user/usersettings.ts (1)
559-573: Non-null assertion is safe given the query filter, but consider a defensive guard.The
!on line 562 relies on theIn(activeProviders)filter at line 552 to ensure only accounts matching current providers are returned. This is correct given the current logic, but a future refactor could inadvertently break this invariant.A quick defensive check would prevent a silent crash:
const linkedAccountInfo = linkedAccounts.map((acc) => { const provider = settings.oidc.providers.find( (p) => p.slug === acc.provider - )!; + ); + if (!provider) return null; return { id: acc.id, ... } satisfies UserSettingsLinkedAccount; - }); + }).filter((p): p is UserSettingsLinkedAccount => p != null);Low priority—current logic is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/user/usersettings.ts` around lines 559 - 573, The mapping block that builds linkedAccountInfo uses a non-null assertion on the result of settings.oidc.providers.find, which can throw if the invariant breaks; update the linkedAccounts.map in usersettings.ts to defensively handle a missing provider by checking the find result (the provider variable returned by settings.oidc.providers.find) and either skipping that account (filtering out) or logging/throwing a clear error, then only return an object conforming to UserSettingsLinkedAccount when provider is present; ensure the code references linkedAccountInfo, linkedAccounts.map, settings.oidc.providers.find, and UserSettingsLinkedAccount so reviewers can locate the change.src/utils/oidc.ts (1)
84-88: OIDC error parameter handling is simplified but acceptable.The IdP may return specific error codes (
access_denied,login_required,consent_required, etc.) that could provide more informative messages. Currently, all IdP-side errors map toOidcAuthorizationFailed.This is fine for the initial implementation—IdP error descriptions aren't always user-friendly anyway. Consider logging the actual error/error_description for debugging if issues arise in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/oidc.ts` around lines 84 - 88, The current OIDC error handling reads the URLSearchParams 'error' into errorParam and always returns ApiErrorCode.OidcAuthorizationFailed; update this to also read 'error_description' (e.g., params.get('error_description')) and emit a debug log containing both error and error_description for diagnostics while keeping the returned value as { type: 'error', errorCode: ApiErrorCode.OidcAuthorizationFailed }; locate the code that constructs params and errorParam in this module (URLSearchParams usage) and add a concise log call that records errorParam and the description so production debugging can surface IdP-provided details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/using-seerr/settings/users/oidc.md`:
- Around line 70-72: Clarify that requiredClaims must be present and set to the
boolean true (not just present or truthy) by updating the docs for
requiredClaims to explicitly state the match semantics used by hasRequiredClaims
in server/routes/auth.ts: only claims with the boolean value true pass
validation; other values (e.g., "true", "1", non-boolean, or mere presence) will
not satisfy the requirement.
- Around line 107-113: Remove the recommendation to use a wildcard `/*` redirect
and instead instruct users to configure only the specific Seerr callback paths
already documented above; update the sentence that currently reads "set the root
url to your Seerr instance's URL, and a wildcard `/*` as a valid redirect URL"
to say to add the exact callback URLs listed earlier (do not use wildcard
redirects) and keep the Issuer URL guidance ("Use
`https://<keycloak-url>/realms/<realm>/` as the Issuer URL") unchanged.
In `@server/entity/LinkedAccount.ts`:
- Around line 15-17: The LinkedAccount constructor currently requires an
argument which breaks TypeORM hydration; make the constructor optional by
changing its signature to accept an optional options parameter (e.g.,
constructor(options?: Omit<LinkedAccount,'id'>)) and only call
Object.assign(this, options) when options is provided, so TypeORM can
instantiate empty instances during DB loads; update any call sites that relied
on the old required signature to pass the options or nothing as appropriate.
In `@server/routes/auth.ts`:
- Around line 651-658: getOidcRedirectUrl currently trusts the returnUrl query
and constructs a URL that can point to external origins; validate and only allow
safe redirects by ensuring returnUrl is either a relative path (starts with "/")
or, if absolute, has the same origin as getSettings().main.applicationUrl (or
`${req.protocol}://${req.headers.host}`) before calling new URL(); reject or
fallback to '/login' for anything else. Use the existing symbols returnUrl,
getOidcRedirectUrl, getSettings(), and req to perform the check and normalize
the safe returnUrl prior to creating the URL object.
- Line 810: Validate req.body.callbackUrl before calling new URL(): check that
req.body.callbackUrl is a non-empty string, then wrap the URL parsing in a
try/catch (e.g., when creating redirectUrl with new URL(req.body.callbackUrl))
and on error log the failure and return a 400/invalid-callback response rather
than allowing an unhandled exception; update the logic around redirectUrl and
any downstream use to handle the invalid case gracefully.
---
Duplicate comments:
In `@seerr-api.yml`:
- Around line 257-260: The openIdProviders field was added to MainSettings
(making it appear writable via the /settings/main schema) but should be exposed
only on the public settings schema used by the login flow; remove the
openIdProviders entry from MainSettings and instead add a declaration on
PublicSettings as: openIdProviders: type: array, items: $ref:
'#/components/schemas/PublicOidcProvider' (or equivalent), ensuring the
PublicSettings schema exports the provider list while MainSettings no longer
contains openIdProviders.
In `@src/components/Login/OidcLoginButton.tsx`:
- Around line 53-68: The effect that checks query params should wait for Next
router readiness; update the useEffect so it returns early if loading is true or
router.isReady is false, and add router.isReady to the dependency array so the
effect reruns when query params populate; keep the existing checks for
isCallback (using query.code/query.error) and the logic that calls
getOidcProviderSlug(), clearOidcProviderSlug(), handleCallback(), and
redirectToLogin() unchanged.
---
Nitpick comments:
In `@server/routes/auth.test.ts`:
- Around line 462-487: performOidcCallback currently returns the raw response so
tests never verify a session cookie (e.g. connect.sid); change
performOidcCallback to use a persistent supertest agent (create with
request.agent(app)), perform the login and the callback via that agent so
cookies are preserved (use agent.get('/auth/oidc/login/test') and
agent.post('/auth/oidc/callback/test')), then return both the agent and the
callback response (e.g. { agent, response }). Update success-path tests to
assert response.status === 204 and then call agent.get('/auth/me') and assert
that returns 200 to prove the session was established.
In `@server/routes/user/usersettings.ts`:
- Around line 559-573: The mapping block that builds linkedAccountInfo uses a
non-null assertion on the result of settings.oidc.providers.find, which can
throw if the invariant breaks; update the linkedAccounts.map in usersettings.ts
to defensively handle a missing provider by checking the find result (the
provider variable returned by settings.oidc.providers.find) and either skipping
that account (filtering out) or logging/throwing a clear error, then only return
an object conforming to UserSettingsLinkedAccount when provider is present;
ensure the code references linkedAccountInfo, linkedAccounts.map,
settings.oidc.providers.find, and UserSettingsLinkedAccount so reviewers can
locate the change.
In `@src/utils/oidc.ts`:
- Around line 84-88: The current OIDC error handling reads the URLSearchParams
'error' into errorParam and always returns ApiErrorCode.OidcAuthorizationFailed;
update this to also read 'error_description' (e.g.,
params.get('error_description')) and emit a debug log containing both error and
error_description for diagnostics while keeping the returned value as { type:
'error', errorCode: ApiErrorCode.OidcAuthorizationFailed }; locate the code that
constructs params and errorParam in this module (URLSearchParams usage) and add
a concise log call that records errorParam and the description so production
debugging can surface IdP-provided details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b05f04de-c154-4a63-8062-1a7b526f1945
⛔ Files ignored due to path filters (7)
docs/using-seerr/settings/users/assets/oidc_keycloak_1.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_2.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_3.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_4.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_5.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpublic/images/openid.svgis excluded by!**/*.svg
📒 Files selected for processing (28)
docs/using-seerr/settings/users/index.mddocs/using-seerr/settings/users/oidc.mdgen-docs/docusaurus.config.tspackage.jsonseerr-api.ymlserver/constants/error.tsserver/entity/LinkedAccount.tsserver/entity/User.tsserver/index.tsserver/interfaces/api/settingsInterfaces.tsserver/interfaces/api/userSettingsInterfaces.tsserver/lib/settings/index.tsserver/migration/postgres/1742858617989-AddLinkedAccount.tsserver/migration/sqlite/1742858484395-AddLinkedAccounts.tsserver/routes/auth.test.tsserver/routes/auth.tsserver/routes/user/usersettings.tsserver/test/index.mtssrc/components/Common/Button/index.tsxsrc/components/Common/ButtonWithLoader/index.tsxsrc/components/Login/OidcLoginButton.tsxsrc/components/Login/PlexLoginButton.tsxsrc/components/Login/index.tsxsrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/context/SettingsContext.tsxsrc/i18n/locale/en.jsonsrc/pages/_app.tsxsrc/utils/oidc.ts
✅ Files skipped from review due to trivial changes (7)
- server/interfaces/api/userSettingsInterfaces.ts
- gen-docs/docusaurus.config.ts
- package.json
- server/constants/error.ts
- src/context/SettingsContext.tsx
- server/index.ts
- src/i18n/locale/en.json
🚧 Files skipped from review as they are similar to previous changes (11)
- server/interfaces/api/settingsInterfaces.ts
- src/components/Common/Button/index.tsx
- src/pages/_app.tsx
- src/components/Login/PlexLoginButton.tsx
- server/migration/sqlite/1742858484395-AddLinkedAccounts.ts
- server/entity/User.ts
- server/migration/postgres/1742858617989-AddLinkedAccount.ts
- src/components/Common/ButtonWithLoader/index.tsx
- src/components/Login/index.tsx
- server/lib/settings/index.ts
- src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx (1)
248-250:⚠️ Potential issue | 🟠 MajorMedia-server unlink is still looser than the backend.
canUnlinkMediaServerstill inherits theaccounts.length > 1branch fromcanUnlink, so a user with OIDC plus Plex/Jellyfin/Emby but no local credentials will still see a delete button thatserver/routes/user/usersettings.tsrejects with 400. Keep the broader rule for OpenID Connect rows, but mirror the media-server requirement here.🔧 Proposed fix
- const canUnlink = !!passwordInfo?.hasPassword || accounts.length > 1; + const canUnlinkOidc = !!passwordInfo?.hasPassword || accounts.length > 1; // primary admin cannot unlink their media server account - const canUnlinkMediaServer = canUnlink && user?.id !== 1; + const canUnlinkMediaServer = + !!user?.email && !!passwordInfo?.hasPassword && user?.id !== 1; @@ - {(acct.type === LinkedAccountType.OpenIdConnect - ? canUnlink + {(acct.type === LinkedAccountType.OpenIdConnect + ? canUnlinkOidc : canUnlinkMediaServer) && (Based on learnings: media-server unlink in this UI should require local credentials and the primary-admin restriction; multiple linked accounts alone is insufficient for
/linked-accounts/plexand/linked-accounts/jellyfin.Also applies to: 320-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx` around lines 248 - 250, canUnlinkMediaServer is too permissive because it inherits accounts.length > 1 from canUnlink and allows media-server rows to show a delete button even when the user has no local credentials; change the media-server rule to require local credentials plus the primary-admin check by making canUnlinkMediaServer = !!passwordInfo?.hasPassword && user?.id !== 1 (remove the accounts.length > 1 clause), and apply the same stricter check to the other duplicated media-server unlink logic (the second occurrence of this rule around the other linked-account rows).src/components/Login/OidcLoginButton.tsx (1)
53-68:⚠️ Potential issue | 🟠 MajorWait for the Pages Router query to be ready before running this effect.
In the Pages Router,
router.queryis not guaranteed on the first render. With the empty dependency array at Line 68, the?provider=deep link and the?code=/?error=callback path can be missed permanently if hydration populates the query after mount.🔧 Proposed fix
- const { query } = router; + const { isReady, query } = router; @@ useEffect(() => { - if (loading) return; + if (loading || !isReady) return; @@ if (isCallback && getOidcProviderSlug() === provider.slug) { clearOidcProviderSlug(); - handleCallback(); - } - // Support direct redirect via ?provider=slug query param - else if (!isCallback && query.provider === provider.slug) { - redirectToLogin(); + void handleCallback(); + } else if ( + !isCallback && + typeof query.provider === 'string' && + query.provider === provider.slug + ) { + void redirectToLogin(); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [ + handleCallback, + isReady, + loading, + provider.slug, + query.code, + query.error, + query.provider, + redirectToLogin, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Login/OidcLoginButton.tsx` around lines 53 - 68, The effect runs too early because router.query may be empty on first render; update the useEffect to wait for the Pages Router to be ready (use router.isReady) and include router.isReady and query in the dependency array so the effect re-runs when query becomes available, then perform the same checks and calls (getOidcProviderSlug(), clearOidcProviderSlug(), handleCallback(), redirectToLogin()) only when router.isReady is true; remove the eslint-disable for exhaustive-deps and ensure provider.slug and loading are included in the dependencies as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/entity/User.ts`:
- Around line 104-106: The OneToMany relation linkedAccounts can leak through
User.filter(), so update the filtering logic to keep linked account data
private: add the property name "linkedAccounts" to the User.filteredFields (or
the array/constant used to drive User.filter()) so User.filter() excludes it,
and/or annotate the linkedAccounts property with a non-serializing decorator
(e.g., `@Exclude` from class-transformer) to prevent automatic exposure; refer to
the linkedAccounts property and the User.filter() / filteredFields symbols when
making the change.
---
Duplicate comments:
In `@src/components/Login/OidcLoginButton.tsx`:
- Around line 53-68: The effect runs too early because router.query may be empty
on first render; update the useEffect to wait for the Pages Router to be ready
(use router.isReady) and include router.isReady and query in the dependency
array so the effect re-runs when query becomes available, then perform the same
checks and calls (getOidcProviderSlug(), clearOidcProviderSlug(),
handleCallback(), redirectToLogin()) only when router.isReady is true; remove
the eslint-disable for exhaustive-deps and ensure provider.slug and loading are
included in the dependencies as well.
In
`@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx`:
- Around line 248-250: canUnlinkMediaServer is too permissive because it
inherits accounts.length > 1 from canUnlink and allows media-server rows to show
a delete button even when the user has no local credentials; change the
media-server rule to require local credentials plus the primary-admin check by
making canUnlinkMediaServer = !!passwordInfo?.hasPassword && user?.id !== 1
(remove the accounts.length > 1 clause), and apply the same stricter check to
the other duplicated media-server unlink logic (the second occurrence of this
rule around the other linked-account rows).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 183bea7f-f845-4e04-bede-a04a8bfb47d9
⛔ Files ignored due to path filters (7)
docs/using-seerr/settings/users/assets/oidc_keycloak_1.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_2.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_3.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_4.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_5.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpublic/images/openid.svgis excluded by!**/*.svg
📒 Files selected for processing (28)
docs/using-seerr/settings/users/index.mddocs/using-seerr/settings/users/oidc.mdgen-docs/docusaurus.config.tspackage.jsonseerr-api.ymlserver/constants/error.tsserver/entity/LinkedAccount.tsserver/entity/User.tsserver/index.tsserver/interfaces/api/settingsInterfaces.tsserver/interfaces/api/userSettingsInterfaces.tsserver/lib/settings/index.tsserver/migration/postgres/1742858617989-AddLinkedAccount.tsserver/migration/sqlite/1742858484395-AddLinkedAccounts.tsserver/routes/auth.test.tsserver/routes/auth.tsserver/routes/user/usersettings.tsserver/test/index.mtssrc/components/Common/Button/index.tsxsrc/components/Common/ButtonWithLoader/index.tsxsrc/components/Login/OidcLoginButton.tsxsrc/components/Login/PlexLoginButton.tsxsrc/components/Login/index.tsxsrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/context/SettingsContext.tsxsrc/i18n/locale/en.jsonsrc/pages/_app.tsxsrc/utils/oidc.ts
✅ Files skipped from review due to trivial changes (9)
- gen-docs/docusaurus.config.ts
- server/interfaces/api/settingsInterfaces.ts
- server/constants/error.ts
- server/interfaces/api/userSettingsInterfaces.ts
- package.json
- server/entity/LinkedAccount.ts
- src/context/SettingsContext.tsx
- server/migration/sqlite/1742858484395-AddLinkedAccounts.ts
- server/lib/settings/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/pages/_app.tsx
- src/components/Common/Button/index.tsx
- server/migration/postgres/1742858617989-AddLinkedAccount.ts
- server/routes/auth.test.ts
- src/i18n/locale/en.json
- server/routes/user/usersettings.ts
- server/routes/auth.ts
👮 Files not reviewed due to content moderation or server errors (5)
- docs/using-seerr/settings/users/oidc.md
- src/components/Common/ButtonWithLoader/index.tsx
- src/components/Login/PlexLoginButton.tsx
- src/components/Login/index.tsx
- seerr-api.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/entity/User.ts (1)
105-106: Narrow relation cascade scope to['insert']only.Verification confirms
linkedAccountsmutations are always performed directly viaLinkedAccountsRepository:
- Deletes at usersettings.ts:623 use
linkedAccountsRepository.delete()directly- Saves at auth.ts:935 use
linkedAccountsRepository.save()directly- User cascade is only needed for inserts during account creation (auth.ts:980:
user.linkedAccounts = [linkedAccount]; await userRepository.save(user))Since
updateandremoveoperations are never cascaded, restricting tocascade: ['insert']eliminates unnecessary side effect risk without impact.Suggested change
- `@OneToMany`(() => LinkedAccount, (link) => link.user, { cascade: true }) + `@OneToMany`(() => LinkedAccount, (link) => link.user, { + cascade: ['insert'], + }) public linkedAccounts: LinkedAccount[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/entity/User.ts` around lines 105 - 106, Change the cascade setting on the User entity's linkedAccounts relation from a blanket true to the narrower insert-only cascade: update the decorator on the linkedAccounts property (the `@OneToMany`(() => LinkedAccount, (link) => link.user, { cascade: true }) line) to use { cascade: ['insert'] } so only inserts are cascaded while updates and removes remain handled by LinkedAccountsRepository.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/entity/User.ts`:
- Around line 105-106: Change the cascade setting on the User entity's
linkedAccounts relation from a blanket true to the narrower insert-only cascade:
update the decorator on the linkedAccounts property (the `@OneToMany`(() =>
LinkedAccount, (link) => link.user, { cascade: true }) line) to use { cascade:
['insert'] } so only inserts are cascaded while updates and removes remain
handled by LinkedAccountsRepository.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6bfa523-1c0c-41df-ac7e-b2793aeca59c
📒 Files selected for processing (1)
server/entity/User.ts
9 tasks
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/routes/auth.ts (1)
660-661:⚠️ Potential issue | 🟠 MajorKey the OIDC cookies per auth attempt.
Dropping the old short TTL helped, but these cookies are still shared across every OIDC flow. A second login/link attempt overwrites the first verifier/state and makes the earlier callback fail nondeterministically. Namespace the transient cookie(s) by a per-flow value and read/clear that specific entry in the callback.
Also applies to: 703-710, 727-734, 804-808
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 660 - 661, The OIDC state and code-verifier cookies (OIDC_STATE_KEY and OIDC_CODE_VERIFIER_KEY) are global and get clobbered across concurrent flows; change to per-flow keys by generating a random flowId on auth start, set cookies using `${OIDC_STATE_KEY}-${flowId}` and `${OIDC_CODE_VERIFIER_KEY}-${flowId}` and include that flowId in the outbound redirect (e.g., as a query param or encoded in state). In the callback handlers that currently read/clear OIDC_STATE_KEY and OIDC_CODE_VERIFIER_KEY (the code around the blocks you flagged at 703-710, 727-734, 804-808), read and clear the specific `${...}-${flowId}` entries based on the returned flowId instead of the shared names so each auth attempt resolves and clears only its own cookies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/auth.ts`:
- Around line 933-942: The current linked account creation (new LinkedAccount +
linkedAccountsRepository.save in the callback handling) can race despite the
pre-check because the DB enforces UNIQUE (provider, sub); modify the save logic
in the route handler to either perform an atomic upsert/transaction or catch
unique-constraint DB errors from linkedAccountsRepository.save (e.g., Postgres
error code 23505 or ORM UniqueConstraintError) and translate them into the
appropriate conflict/deny HTTP response instead of letting a generic 500 bubble
up; apply the same fix to the other creation site around the LinkedAccount code
at the later block (the second occurrence noted).
- Around line 948-975: Normalize the incoming OIDC email claim before any lookup
or creation: compute a normalizedEmail = fullUserInfo.email.trim().toLowerCase()
(or call your existing normalizeEmail helper) and use that for
userRepository.findOne({ where: { email: normalizedEmail } }) and for the new
User({ email: normalizedEmail, ... }) assignment; also use normalizedEmail when
generating the gravatar URL and any logging so all email comparisons in auth
flow (including userRepository.findOne and the User constructor) are
case-insensitive and whitespace-tolerant.
---
Duplicate comments:
In `@server/routes/auth.ts`:
- Around line 660-661: The OIDC state and code-verifier cookies (OIDC_STATE_KEY
and OIDC_CODE_VERIFIER_KEY) are global and get clobbered across concurrent
flows; change to per-flow keys by generating a random flowId on auth start, set
cookies using `${OIDC_STATE_KEY}-${flowId}` and
`${OIDC_CODE_VERIFIER_KEY}-${flowId}` and include that flowId in the outbound
redirect (e.g., as a query param or encoded in state). In the callback handlers
that currently read/clear OIDC_STATE_KEY and OIDC_CODE_VERIFIER_KEY (the code
around the blocks you flagged at 703-710, 727-734, 804-808), read and clear the
specific `${...}-${flowId}` entries based on the returned flowId instead of the
shared names so each auth attempt resolves and clears only its own cookies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6dd5d310-6699-4d8a-bc23-aaec6ccf26cc
📒 Files selected for processing (1)
server/routes/auth.ts
michaelhthomas
force-pushed
the
feat/oidc-login-basic
branch
from
a5cf3e9 to
ed3b51c
Compare
…n single transaction
This improves CSRF protection somewhat, and also improves compatibilty with Authelia and Authentik, which both have strange handling of the `state` parameter either beyond or non-compliant with the specification.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
michaelhthomas
force-pushed
the
feat/oidc-login-basic
branch
from
3fcf9f4 to
98d6d8f
Compare
github-actions
bot
removed
the
merge conflict
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx (1)
253-255:⚠️ Potential issue | 🟠 MajorRequire a local password before showing media-server unlink.
canUnlinkMediaServerstill inheritsaccounts.length > 1, but the media-server delete endpoints only allow users with local credentials. A user with OIDC + Plex/Jellyfin and no password will still see a delete action that always 400s.🔧 Proposed fix
const canUnlink = !!passwordInfo?.hasPassword || accounts.length > 1; // primary admin cannot unlink their media server account - const canUnlinkMediaServer = canUnlink && user?.id !== 1; + const canUnlinkMediaServer = !!passwordInfo?.hasPassword && user?.id !== 1;Based on learnings: In
server/routes/user/usersettings.ts, the DELETE endpoints for media servers (/linked-accounts/plexand/linked-accounts/jellyfin) require the user to have a local email and password; having multiple linked accounts alone is not sufficient. Client UI should gate media-server unlink onhasPassword(and primary-admin restriction) rather thanaccounts.length > 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx` around lines 253 - 255, canUnlinkMediaServer currently inherits accounts.length > 1 and can show a media-server unlink button even for users without local credentials; change the gating so media-server unlink requires a local password and non-primary admin status: use passwordInfo?.hasPassword (e.g. !!passwordInfo?.hasPassword) combined with user?.id !== 1 to compute canUnlinkMediaServer, leaving canUnlink as-is for other providers; update any usages of canUnlinkMediaServer accordingly so the UI only shows Plex/Jellyfin unlink when the user has a local password and is not the primary admin.seerr-api.yml (2)
4132-4141:⚠️ Potential issue | 🟡 MinorAdvertise the PKCE/state failure branch on the callback
400response.The callback can fail with
OIDC_AUTHORIZATION_FAILEDbefore account creation when the signed verifier/state cookies are missing or tampered, but this schema only documentsOIDC_MISSING_EMAIL.🧾 Proposed OpenAPI fix
'400': - description: Unable to create account (e.g. missing email address) + description: Authorization callback invalid or unable to create account (e.g. missing/tampered PKCE/state cookies, missing email address) content: application/json: schema: type: object properties: error: type: string - example: 'OIDC_MISSING_EMAIL' + description: Possible values include OIDC_AUTHORIZATION_FAILED and OIDC_MISSING_EMAIL. + example: 'OIDC_AUTHORIZATION_FAILED'Based on learnings: In
server/routes/auth.ts, the OIDC callback must fail closed with HTTP 400 andApiErrorCode.OidcAuthorizationFailedwhen either the signed PKCE code-verifier or state cookie is missing/tampered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seerr-api.yml` around lines 4132 - 4141, The 400 response schema for the OIDC callback in seerr-api.yml only documents 'OIDC_MISSING_EMAIL' but must also advertise the PKCE/state failure branch; update the '400' response under the callback endpoint to include an additional possible error string (e.g. 'OIDC_AUTHORIZATION_FAILED' or mirror ApiErrorCode.OidcAuthorizationFailed) in the schema examples/enum and adjust the description to mention missing/tampered signed PKCE code_verifier or state cookies (this corresponds to the behavior enforced in server/routes/auth.ts where the callback returns HTTP 400 when the signed verifier/state cookies are missing or tampered).
257-260:⚠️ Potential issue | 🟡 MinorKeep
/settings/publicin sync with the new OIDC provider field.
openIdProvidersis added toMainSettings, butPublicSettingsstill omits it later in the spec. That leaves the unauthenticated settings contract out of sync for clients that need to render OIDC buttons before login.🧾 Proposed OpenAPI fix
PublicSettings: type: object required: - initialized - plexClientIdentifier properties: initialized: type: boolean example: false plexClientIdentifier: type: string format: uuid description: Instance Plex OAuth client identifier example: 6919275e-142a-48d8-be6b-93594cbd4626 + openIdProviders: + type: array + items: + $ref: '#/components/schemas/PublicOidcProvider'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@seerr-api.yml` around lines 257 - 260, PublicSettings is missing the newly added openIdProviders field from MainSettings, so add an openIdProviders property to the PublicSettings schema that matches the MainSettings definition: type: array with items: $ref: '#/components/schemas/PublicOidcProvider' (or reference MainSettings' property), ensuring the unauthenticated /settings/public contract includes the same OIDC provider array so clients can render OIDC buttons before login.
🧹 Nitpick comments (1)
server/lib/settings/index.ts (1)
417-420: Constructor relies on synchronous code path in asyncload().The constructor calls
load(initialSettings)withoutawait, which works because whenoverrideSettingsis provided,load()executes synchronously before returning a Promise. The data is set before the constructor returns.This is correct but somewhat subtle. If future changes add async operations to the
overrideSettingsbranch, this would break silently. Consider adding a brief comment noting this intentional design:// Sync path: when initialSettings provided, load() merges synchronously if (initialSettings) this.load(initialSettings);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/settings/index.ts` around lines 417 - 420, The constructor currently calls this.load(initialSettings) without awaiting; document that this is intentional because load() runs a synchronous merge when overrideSettings is provided so data is set before the constructor returns—add a concise inline comment in the constructor near the call to load() explaining the "Sync path: when initialSettings provided, load() merges synchronously" and note that if load() later becomes async this must be revisited (reference: constructor and load() and the overrideSettings branch).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@seerr-api.yml`:
- Around line 4132-4141: The 400 response schema for the OIDC callback in
seerr-api.yml only documents 'OIDC_MISSING_EMAIL' but must also advertise the
PKCE/state failure branch; update the '400' response under the callback endpoint
to include an additional possible error string (e.g. 'OIDC_AUTHORIZATION_FAILED'
or mirror ApiErrorCode.OidcAuthorizationFailed) in the schema examples/enum and
adjust the description to mention missing/tampered signed PKCE code_verifier or
state cookies (this corresponds to the behavior enforced in
server/routes/auth.ts where the callback returns HTTP 400 when the signed
verifier/state cookies are missing or tampered).
- Around line 257-260: PublicSettings is missing the newly added openIdProviders
field from MainSettings, so add an openIdProviders property to the
PublicSettings schema that matches the MainSettings definition: type: array with
items: $ref: '#/components/schemas/PublicOidcProvider' (or reference
MainSettings' property), ensuring the unauthenticated /settings/public contract
includes the same OIDC provider array so clients can render OIDC buttons before
login.
In
`@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx`:
- Around line 253-255: canUnlinkMediaServer currently inherits accounts.length >
1 and can show a media-server unlink button even for users without local
credentials; change the gating so media-server unlink requires a local password
and non-primary admin status: use passwordInfo?.hasPassword (e.g.
!!passwordInfo?.hasPassword) combined with user?.id !== 1 to compute
canUnlinkMediaServer, leaving canUnlink as-is for other providers; update any
usages of canUnlinkMediaServer accordingly so the UI only shows Plex/Jellyfin
unlink when the user has a local password and is not the primary admin.
---
Nitpick comments:
In `@server/lib/settings/index.ts`:
- Around line 417-420: The constructor currently calls
this.load(initialSettings) without awaiting; document that this is intentional
because load() runs a synchronous merge when overrideSettings is provided so
data is set before the constructor returns—add a concise inline comment in the
constructor near the call to load() explaining the "Sync path: when
initialSettings provided, load() merges synchronously" and note that if load()
later becomes async this must be revisited (reference: constructor and load()
and the overrideSettings branch).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90e3c42c-b7b1-477e-bf49-f76549d9a505
⛔ Files ignored due to path filters (7)
docs/using-seerr/settings/users/assets/oidc_keycloak_1.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_2.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_3.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_4.pngis excluded by!**/*.pngdocs/using-seerr/settings/users/assets/oidc_keycloak_5.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpublic/images/openid.svgis excluded by!**/*.svg
📒 Files selected for processing (28)
docs/using-seerr/settings/users/index.mddocs/using-seerr/settings/users/oidc.mdgen-docs/docusaurus.config.tspackage.jsonseerr-api.ymlserver/constants/error.tsserver/entity/LinkedAccount.tsserver/entity/User.tsserver/index.tsserver/interfaces/api/settingsInterfaces.tsserver/interfaces/api/userSettingsInterfaces.tsserver/lib/settings/index.tsserver/migration/postgres/1742858617989-AddLinkedAccount.tsserver/migration/sqlite/1742858484395-AddLinkedAccounts.tsserver/routes/auth.test.tsserver/routes/auth.tsserver/routes/user/usersettings.tsserver/test/index.mtssrc/components/Common/Button/index.tsxsrc/components/Common/ButtonWithLoader/index.tsxsrc/components/Login/OidcLoginButton.tsxsrc/components/Login/PlexLoginButton.tsxsrc/components/Login/index.tsxsrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/context/SettingsContext.tsxsrc/i18n/locale/en.jsonsrc/pages/_app.tsxsrc/utils/oidc.ts
✅ Files skipped from review due to trivial changes (11)
- server/test/index.mts
- gen-docs/docusaurus.config.ts
- server/constants/error.ts
- server/interfaces/api/userSettingsInterfaces.ts
- package.json
- server/entity/LinkedAccount.ts
- server/migration/postgres/1742858617989-AddLinkedAccount.ts
- src/components/Login/PlexLoginButton.tsx
- src/components/Common/ButtonWithLoader/index.tsx
- server/migration/sqlite/1742858484395-AddLinkedAccounts.ts
- src/utils/oidc.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- server/index.ts
- src/components/Common/Button/index.tsx
- src/i18n/locale/en.json
- server/routes/auth.test.ts
- src/components/Login/OidcLoginButton.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds basic support for logging in with OpenID Connect. Here's a basic overview of what's included:
newUserLoginsetting (configured at the provider level) enabled, users can log in with their OpenID Connect provider and a new account will be created for themWhat isn't included (yet)
settings.jsonAll of the above have been implemented and are coming soon, but have been omitted from this PR to ease the review process.
How Has This Been Tested?
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
New Features
UX
Settings
Internationalization
Documentation
Tests
Chores