feat: initial support for OpenID Connect authentication#2715
Open
michaelhthomas wants to merge 30 commits intoseerr-team:developfrom
Open
feat: initial support for OpenID Connect authentication#2715michaelhthomas wants to merge 30 commits intoseerr-team:developfrom
michaelhthomas wants to merge 30 commits intoseerr-team:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds OpenID Connect (OIDC) authentication and linking: server discovery/exchange with PKCE/state, LinkedAccount entity and migrations, API endpoints for OIDC login/callback and linked-account GET/DELETE, client utilities and UI components, settings/schema 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 /auth/oidc/login/:slug?returnUrl=...
activate Seerr
Seerr->>Provider: GET /.well-known/openid-configuration
Provider-->>Seerr: discovery metadata
Seerr->>Seerr: generate PKCE & state, set signed cookies
Seerr-->>Browser: { redirectUrl }
deactivate Seerr
Browser->>Provider: GET /authorize?code_challenge=...&state=...
Provider-->>Browser: redirect to callback?code=...&state=...
Browser->>Seerr: POST /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
Seerr-->>Browser: 204 No Content (session set if applicable)
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
src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx
Show resolved
Hide resolved
src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx
Outdated
Show resolved
Hide resolved
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
src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx
Show resolved
Hide resolved
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.
michaelhthomas
force-pushed
the
feat/oidc-login-basic
branch
from
ed3b51c to
dbe8796
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
docs/using-seerr/settings/users/oidc.md (2)
70-72:⚠️ Potential issue | 🟡 MinorSpell out the
requiredClaimsmatch semantics.This still reads like claim presence or truthiness may be enough. The server only accepts claims whose value is the boolean
true, so a provider config can look correct here and still fail login.Suggested change
-Space-separated list of boolean claims that are required to log in +Space-separated list of claim names that must be present and set to the boolean `true` for login to succeedBased on learnings OIDC required-claims semantics: in
server/routes/auth.ts,hasRequiredClaimsis intentionally strict and only passes when the claim value is booleantrue.🤖 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 70 - 72, Update the docs for the requiredClaims setting to explicitly state that each required claim must be present and have the boolean value true (not just exist or be truthy); reference the server-side check implemented in hasRequiredClaims in server/routes/auth.ts so readers know the server only accepts claims with value === true, and update the description for the requiredClaims section to reflect this strict match semantics.
107-113:⚠️ Potential issue | 🟡 MinorAvoid recommending a wildcard redirect URL.
The exact Seerr callback paths are already documented above. Suggesting
/*broadens the IdP client configuration unnecessarily and can hide redirect mismatches during setup.Suggested change
-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 exact callback URLs listed above instead of a wildcard.🤖 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 107 - 113, The paragraph that tells users to "set the root url to your Seerr instance's URL, and a wildcard `/*` as a valid redirect URL" should be changed to avoid recommending a wildcard redirect; instead instruct readers to use the exact Seerr OAuth callback URLs (the callback paths already documented earlier in this doc) as the valid redirect URIs in the IdP client configuration, and provide a concrete example format using the Seerr instance URL plus the documented callback path (e.g., https://<seerr-url>/<callback-path>) to replace the `/*` suggestion; update the sentence referencing the wildcard so it points readers to the documented callback paths above and removes the wildcard recommendation.seerr-api.yml (1)
257-260:⚠️ Potential issue | 🟠 MajorKeep
/settings/publiccontract aligned withopenIdProviders.Line 257 adds
openIdProviderstoMainSettings, butPublicSettings(used by/settings/public) still omits it, so generated clients can miss this field.🧾 Proposed OpenAPI fix
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, PublicSettings (used by /settings/public) is missing the openIdProviders property that was added to MainSettings, causing generated clients to omit it; update the components schema for PublicSettings to include an openIdProviders property with the same type and items reference as MainSettings (type: array, items: $ref '#/components/schemas/PublicOidcProvider') so the public settings contract matches MainSettings and clients receive the field.server/routes/auth.ts (4)
936-940:⚠️ Potential issue | 🟠 MajorEmail case normalization missing in OIDC user lookup.
The
existingUserlookup usesfullUserInfo.emaildirectly, but the codebase normalizes emails withtoLowerCase()elsewhere (e.g., lines 81, 615). An IdP returningAlice@Example.comwould miss an existingalice@example.comuser, potentially causing duplicate account creation or constraint violations.🔧 Proposed fix
+ const normalizedEmail = fullUserInfo.email?.toLowerCase(); - if (!user && fullUserInfo.email != null && provider.newUserLogin) { + if (!user && normalizedEmail && provider.newUserLogin) { const existingUser = await userRepository.findOne({ - where: { email: fullUserInfo.email }, + where: { email: normalizedEmail }, }); ... user = new User({ ... - email: fullUserInfo.email, + email: normalizedEmail, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 936 - 940, The OIDC lookup uses fullUserInfo.email as-is which can miss matches due to case differences; update the existingUser query (around the block checking if (!user && fullUserInfo.email != null && provider.newUserLogin)) to normalize the email before querying (e.g., compute a lowercasedEmail = fullUserInfo.email.trim().toLowerCase()) and use that in userRepository.findOne({ where: { email: lowercasedEmail } }); also ensure any downstream creation of the new user uses the same normalized email to avoid duplicates (refer to existingUser, fullUserInfo.email, provider.newUserLogin, and userRepository.findOne).
967-975:⚠️ Potential issue | 🟠 MajorNon-atomic User + LinkedAccount creation persists.
If
userRepository.save(user)succeeds for the User but the cascaded LinkedAccount insert fails (e.g., unique constraint race), an orphaned user without any linked identity remains. Subsequent OIDC attempts will hit theexistingUsercheck and be denied. Wrap both writes in a transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 967 - 975, The code currently assigns a new LinkedAccount to user.linkedAccounts and calls userRepository.save(user) which can persist the User while the cascaded LinkedAccount insert fails, leaving an orphaned user; change this to perform both inserts inside a single database transaction (e.g., using your ORM's transaction runner or manager) so that creation of LinkedAccount and User are atomic: begin a transaction, save the LinkedAccount and associate it to the User (or save the User with its linkedAccounts) via the transactional entity manager, commit on success and rollback on error, and ensure errors (like unique constraint conflicts) cause the whole transaction to roll back instead of leaving an orphaned user.
920-929:⚠️ Potential issue | 🟠 MajorRace condition on LinkedAccount creation remains unaddressed.
The existence check before insert doesn't prevent concurrent callbacks from racing. The DB unique constraint on
(provider, sub)will reject one insert, surfacing as a 500 error. Consider catching unique-constraint errors and mapping to an appropriate response, or using an upsert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 920 - 929, The LinkedAccount creation has a race: the pre-check can be lost to concurrent inserts causing linkedAccountsRepository.save(newLinkedAccount) to throw a DB unique-constraint error; update the block that constructs and saves a new LinkedAccount (new LinkedAccount, linkedAccountsRepository.save) to handle this by either performing an upsert/insert-or-ignore at the repository level or wrapping the save in a try/catch that catches the DB unique-constraint error for (provider.slug, fullUserInfo.sub), then in the catch load the existing LinkedAccount (by provider and sub) and treat that as the successful result (or return a 409/appropriate response), so concurrent callbacks no longer produce a 500.
798-812:⚠️ Potential issue | 🔴 CriticalFail-fast when OIDC correlation cookies are missing.
The callback proceeds to
authorizationCodeGranteven whenpkceCodeVerifierorexpectedStateareundefined(missing or tampered cookies). While the openid-client library may catch state mismatches, if the IdP doesn't enforce PKCE or doesn't return state in certain error flows, an unsolicited callback could succeed. Add an explicit guard to fail closed.🔒 Proposed fix
const pkceCodeVerifier: string | undefined = req.signedCookies[OIDC_CODE_VERIFIER_KEY]; const expectedState: string | undefined = req.signedCookies[OIDC_STATE_KEY]; res.clearCookie(OIDC_CODE_VERIFIER_KEY); res.clearCookie(OIDC_STATE_KEY); + if (!pkceCodeVerifier || !expectedState) { + logger.warn('Rejected OIDC callback: missing correlation cookies', { + label: 'Auth', + provider: provider.slug, + ip: req.ip, + }); + return next({ + status: 400, + error: ApiErrorCode.OidcAuthorizationFailed, + }); + } + const redirectUrl = new URL(req.body.callbackUrl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 798 - 812, Read the signed cookies into pkceCodeVerifier and expectedState and add an explicit guard that returns a 400 (or throws) if either is undefined before proceeding to call openIdClient.authorizationCodeGrant; reference the cookie keys OIDC_CODE_VERIFIER_KEY and OIDC_STATE_KEY and the variables pkceCodeVerifier/expectedState, perform the check immediately after reading them (and preferably before or instead of clearing cookies), and stop the handler early with an error response so authorizationCodeGrant is never invoked with missing correlation data.
🧹 Nitpick comments (1)
server/routes/auth.ts (1)
707-729: Consider addingsameSiteandmaxAgeto OIDC correlation cookies.The signed cookies provide integrity, but explicitly setting
sameSite: 'lax'and a reasonablemaxAge(e.g., 10 minutes) would improve clarity and limit the cookie lifetime:🔧 Proposed enhancement
res.cookie(OIDC_CODE_VERIFIER_KEY, code_verifier, { httpOnly: true, secure: req.protocol === 'https', signed: true, + sameSite: 'lax', + maxAge: 10 * 60 * 1000, // 10 minutes }); ... res.cookie(OIDC_STATE_KEY, state, { httpOnly: true, secure: req.protocol === 'https', signed: true, + sameSite: 'lax', + maxAge: 10 * 60 * 1000, // 10 minutes });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 707 - 729, Add explicit sameSite and maxAge options to the OIDC correlation cookies to limit lifetime and improve CSRF protection: when setting the code_verifier cookie (res.cookie with OIDC_CODE_VERIFIER_KEY, code_verifier) and the state cookie (res.cookie with OIDC_STATE_KEY, state), include sameSite: 'lax' and a maxAge (e.g., 10 minutes in milliseconds) in the cookie options alongside httpOnly, secure, and signed; update both cookie option objects so the changes apply consistently wherever getOidcRedirectUrl and openIdClient.randomState() are used to initiate the OIDC flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 79: Update the Node.js engine requirement in package.json to ensure
ESM-only deps like openid-client and jose work: locate the "engines" entry in
package.json (the "node" field) and change its value from "^22.0.0" to
"^22.12.0" so the project requires Node versions supporting require(esm) (i.e.,
^20.19.0 || ^22.12.0 || >=23.0.0).
In `@seerr-api.yml`:
- Around line 5156-5157: The 403 response description in seerr-api.yml currently
reads "Invalid credentials" which is misleading; update the '403' description
for the linked-account listing operation to indicate an authorization/forbidden
error (e.g., "Forbidden — insufficient permissions" or "Insufficient permissions
to list linked accounts") so API consumers understand it's a permissions failure
rather than invalid authentication.
In `@server/routes/user/usersettings.ts`:
- Around line 350-358: The count currently uses linkedAccountsRepository.count({
where: { user: { id: user.id } } }) and thus counts all LinkedAccount rows;
change the queries that compute oidcAccountCount (and the similar checks at the
other locations) to only count linked accounts whose provider is one of the
active provider slugs used by the listing route (use the same
activeProviderSlugs variable defined in this file). Concretely, modify the
count/query for LinkedAccount (or use a queryBuilder join to Provider) to add a
filter like provider.slug IN activeProviderSlugs (or provider:
In(activeProviderSlugs)) so the fallback sign-in check only considers active
OIDC providers; apply the same change for the checks around the other ranges
mentioned (the blocks that compute and check oidcAccountCount at 509-517 and
599-613).
---
Duplicate comments:
In `@docs/using-seerr/settings/users/oidc.md`:
- Around line 70-72: Update the docs for the requiredClaims setting to
explicitly state that each required claim must be present and have the boolean
value true (not just exist or be truthy); reference the server-side check
implemented in hasRequiredClaims in server/routes/auth.ts so readers know the
server only accepts claims with value === true, and update the description for
the requiredClaims section to reflect this strict match semantics.
- Around line 107-113: The paragraph that tells users to "set the root url to
your Seerr instance's URL, and a wildcard `/*` as a valid redirect URL" should
be changed to avoid recommending a wildcard redirect; instead instruct readers
to use the exact Seerr OAuth callback URLs (the callback paths already
documented earlier in this doc) as the valid redirect URIs in the IdP client
configuration, and provide a concrete example format using the Seerr instance
URL plus the documented callback path (e.g.,
https://<seerr-url>/<callback-path>) to replace the `/*` suggestion; update the
sentence referencing the wildcard so it points readers to the documented
callback paths above and removes the wildcard recommendation.
In `@seerr-api.yml`:
- Around line 257-260: PublicSettings (used by /settings/public) is missing the
openIdProviders property that was added to MainSettings, causing generated
clients to omit it; update the components schema for PublicSettings to include
an openIdProviders property with the same type and items reference as
MainSettings (type: array, items: $ref
'#/components/schemas/PublicOidcProvider') so the public settings contract
matches MainSettings and clients receive the field.
In `@server/routes/auth.ts`:
- Around line 936-940: The OIDC lookup uses fullUserInfo.email as-is which can
miss matches due to case differences; update the existingUser query (around the
block checking if (!user && fullUserInfo.email != null &&
provider.newUserLogin)) to normalize the email before querying (e.g., compute a
lowercasedEmail = fullUserInfo.email.trim().toLowerCase()) and use that in
userRepository.findOne({ where: { email: lowercasedEmail } }); also ensure any
downstream creation of the new user uses the same normalized email to avoid
duplicates (refer to existingUser, fullUserInfo.email, provider.newUserLogin,
and userRepository.findOne).
- Around line 967-975: The code currently assigns a new LinkedAccount to
user.linkedAccounts and calls userRepository.save(user) which can persist the
User while the cascaded LinkedAccount insert fails, leaving an orphaned user;
change this to perform both inserts inside a single database transaction (e.g.,
using your ORM's transaction runner or manager) so that creation of
LinkedAccount and User are atomic: begin a transaction, save the LinkedAccount
and associate it to the User (or save the User with its linkedAccounts) via the
transactional entity manager, commit on success and rollback on error, and
ensure errors (like unique constraint conflicts) cause the whole transaction to
roll back instead of leaving an orphaned user.
- Around line 920-929: The LinkedAccount creation has a race: the pre-check can
be lost to concurrent inserts causing
linkedAccountsRepository.save(newLinkedAccount) to throw a DB unique-constraint
error; update the block that constructs and saves a new LinkedAccount (new
LinkedAccount, linkedAccountsRepository.save) to handle this by either
performing an upsert/insert-or-ignore at the repository level or wrapping the
save in a try/catch that catches the DB unique-constraint error for
(provider.slug, fullUserInfo.sub), then in the catch load the existing
LinkedAccount (by provider and sub) and treat that as the successful result (or
return a 409/appropriate response), so concurrent callbacks no longer produce a
500.
- Around line 798-812: Read the signed cookies into pkceCodeVerifier and
expectedState and add an explicit guard that returns a 400 (or throws) if either
is undefined before proceeding to call openIdClient.authorizationCodeGrant;
reference the cookie keys OIDC_CODE_VERIFIER_KEY and OIDC_STATE_KEY and the
variables pkceCodeVerifier/expectedState, perform the check immediately after
reading them (and preferably before or instead of clearing cookies), and stop
the handler early with an error response so authorizationCodeGrant is never
invoked with missing correlation data.
---
Nitpick comments:
In `@server/routes/auth.ts`:
- Around line 707-729: Add explicit sameSite and maxAge options to the OIDC
correlation cookies to limit lifetime and improve CSRF protection: when setting
the code_verifier cookie (res.cookie with OIDC_CODE_VERIFIER_KEY, code_verifier)
and the state cookie (res.cookie with OIDC_STATE_KEY, state), include sameSite:
'lax' and a maxAge (e.g., 10 minutes in milliseconds) in the cookie options
alongside httpOnly, secure, and signed; update both cookie option objects so the
changes apply consistently wherever getOidcRedirectUrl and
openIdClient.randomState() are used to initiate the OIDC flow.
🪄 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: feedebbb-629c-4c3b-a4ac-f8a7b0dd7c27
⛔ 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 (6)
- gen-docs/docusaurus.config.ts
- server/constants/error.ts
- server/migration/postgres/1742858617989-AddLinkedAccount.ts
- server/interfaces/api/userSettingsInterfaces.ts
- src/utils/oidc.ts
- server/lib/settings/index.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- server/test/index.mts
- src/context/SettingsContext.tsx
- server/interfaces/api/settingsInterfaces.ts
- src/components/Common/Button/index.tsx
- server/entity/User.ts
- server/entity/LinkedAccount.ts
- server/migration/sqlite/1742858484395-AddLinkedAccounts.ts
- src/components/Login/PlexLoginButton.tsx
- src/components/Common/ButtonWithLoader/index.tsx
- src/components/Login/OidcLoginButton.tsx
- src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx
- server/routes/auth.test.ts
- src/components/Login/index.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (2)
server/routes/auth.ts (2)
949-975:⚠️ Potential issue | 🟠 MajorNormalize OIDC email before lookup and persistence.
Line 952 and Line 974 use raw IdP email casing/whitespace. This can miss existing users and create duplicate-account conflicts depending on DB collation. Normalize once and reuse.
🔧 Suggested fix
- if (!user && fullUserInfo.email != null && provider.newUserLogin) { + const normalizedEmail = fullUserInfo.email?.trim().toLowerCase(); + if (!user && normalizedEmail != null && provider.newUserLogin) { // Check if a user with this email already exists const existingUser = await userRepository.findOne({ - where: { email: fullUserInfo.email }, + where: { email: normalizedEmail }, }); @@ - logger.info(`Creating user for ${fullUserInfo.email}`, { + logger.info(`Creating user for ${normalizedEmail}`, { ip: req.ip, - email: fullUserInfo.email, + email: normalizedEmail, }); const avatar = fullUserInfo.picture ?? - gravatarUrl(fullUserInfo.email, { default: 'mm', size: 200 }); + gravatarUrl(normalizedEmail, { default: 'mm', size: 200 }); user = new User({ avatar: avatar, username: fullUserInfo.preferred_username, - email: fullUserInfo.email, + email: normalizedEmail, permissions: settings.main.defaultPermissions, plexToken: '', userType: UserType.LOCAL, }); @@ provider: provider.slug, sub: fullUserInfo.sub, - username: fullUserInfo.preferred_username ?? fullUserInfo.email, + username: fullUserInfo.preferred_username ?? normalizedEmail, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 949 - 975, Normalize the OIDC email once (trim + toLowerCase) and reuse it for lookup and persistence: compute a normalizedEmail from fullUserInfo.email early in the provider.newUserLogin block, use normalizedEmail in userRepository.findOne(...) instead of fullUserInfo.email, use normalizedEmail when generating gravatarUrl(...) and when constructing the new User(...). Ensure any logging that prints the email uses the normalizedEmail as well so lookups and created user records are consistent.
933-943:⚠️ Potential issue | 🟠 MajorHandle linked-account uniqueness races explicitly.
Line 942 and Line 988 can still surface a generic 500 under concurrent callbacks due to unique
(provider, sub)collisions. Catch and translate duplicate-key failures to deterministic auth responses.🔧 Suggested hardening
+const isLinkedAccountUniqueViolation = (error: unknown): boolean => { + if (!(error instanceof Error)) return false; + // Keep DB-agnostic fallback; can be refined per driver in follow-up. + return /unique|duplicate|constraint/i.test(error.message); +}; @@ if (linkedAccount == null) { const newLinkedAccount = new LinkedAccount({ user: req.user, provider: provider.slug, sub: fullUserInfo.sub, username: fullUserInfo.preferred_username ?? req.user.displayName, }); - - await linkedAccountsRepository.save(newLinkedAccount); + try { + await linkedAccountsRepository.save(newLinkedAccount); + } catch (error) { + if (isLinkedAccountUniqueViolation(error)) { + const existing = await linkedAccountsRepository.findOne({ + relations: { user: true }, + where: { provider: provider.slug, sub: fullUserInfo.sub }, + }); + if (existing && existing.user.id !== req.user.id) { + return next({ + status: 409, + error: ApiErrorCode.OidcAccountAlreadyLinked, + }); + } + return res.sendStatus(204); + } + throw error; + } } @@ - await userRepository.save(user); + try { + await userRepository.save(user); + } catch (error) { + if (isLinkedAccountUniqueViolation(error)) { + return next({ + status: 409, + error: ApiErrorCode.OidcAccountAlreadyLinked, + }); + } + throw error; + }#!/bin/bash set -euo pipefail echo "1) Confirm linked-account uniqueness constraints" fd LinkedAccount.ts server/entity --exec sed -n '1,220p' {} echo echo "2) Inspect OIDC linked-account creation sites and current error handling" rg -n -C4 'new LinkedAccount|linkedAccountsRepository\.save|OidcAccountAlreadyLinked|userRepository\.save\(user\)' server/routes/auth.tsAlso applies to: 980-989
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 933 - 943, The code creating a new LinkedAccount can race under concurrent callbacks and surface a generic 500 on unique (provider, sub) violations; wrap the save of the new LinkedAccount (the new LinkedAccount(...) and linkedAccountsRepository.save(...) call) in a try/catch that detects duplicate-key/unique-constraint database errors and translates them into a deterministic auth error response (e.g., throw or return the existing OidcAccountAlreadyLinked / a specific AuthError) mentioning provider.slug and fullUserInfo.sub; apply the same handling to the other similar creation site that calls linkedAccountsRepository.save (the second block around the other OIDC-linked-account creation) so duplicate-key failures are consistently mapped to the same auth response rather than a 500.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/routes/auth.ts`:
- Around line 949-975: Normalize the OIDC email once (trim + toLowerCase) and
reuse it for lookup and persistence: compute a normalizedEmail from
fullUserInfo.email early in the provider.newUserLogin block, use normalizedEmail
in userRepository.findOne(...) instead of fullUserInfo.email, use
normalizedEmail when generating gravatarUrl(...) and when constructing the new
User(...). Ensure any logging that prints the email uses the normalizedEmail as
well so lookups and created user records are consistent.
- Around line 933-943: The code creating a new LinkedAccount can race under
concurrent callbacks and surface a generic 500 on unique (provider, sub)
violations; wrap the save of the new LinkedAccount (the new LinkedAccount(...)
and linkedAccountsRepository.save(...) call) in a try/catch that detects
duplicate-key/unique-constraint database errors and translates them into a
deterministic auth error response (e.g., throw or return the existing
OidcAccountAlreadyLinked / a specific AuthError) mentioning provider.slug and
fullUserInfo.sub; apply the same handling to the other similar creation site
that calls linkedAccountsRepository.save (the second block around the other
OIDC-linked-account creation) so duplicate-key failures are consistently mapped
to the same auth response rather than a 500.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f95f2ed5-8c9b-4b58-984e-b70d054281c2
📒 Files selected for processing (2)
server/routes/auth.test.tsserver/routes/auth.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/routes/auth.test.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Adds initial OpenID Connect (OIDC) authentication support across the Seerr server, API schema, UI, settings, migrations, and documentation to enable OIDC login and account linking.
Changes:
- Implement OIDC auth endpoints (login + callback), linked-account persistence, and new error codes.
- Expose configured OIDC providers via public settings and add UI for OIDC login + linked account management.
- Add database migrations, API docs/schema updates, and server tests covering core OIDC flows.
Reviewed changes
Copilot reviewed 27 out of 35 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/oidc.ts | Client-side helpers for OIDC login initiation, callback processing, and error messaging |
| src/pages/_app.tsx | Prevent redirect-on-login when handling OIDC callback code |
| src/i18n/locale/en.json | Adds OIDC-related UI strings |
| src/context/SettingsContext.tsx | Extends default public settings with openIdProviders |
| src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx | Adds OIDC linked account listing, linking, callback handling, and unlink logic |
| src/components/Login/index.tsx | Renders OIDC provider buttons and updates login button behavior/layout |
| src/components/Login/PlexLoginButton.tsx | Switches to shared ButtonWithLoader for consistent loading UX |
| src/components/Login/OidcLoginButton.tsx | New OIDC provider login button with callback + deep-link support |
| src/components/Common/ButtonWithLoader/index.tsx | New shared button wrapper with inline spinner/loading disable behavior |
| src/components/Common/Button/index.tsx | Exports ButtonProps and improves child layout constraints |
| server/test/index.mts | Forces test runner to exit to avoid hangs on failing tests |
| server/routes/user/usersettings.ts | Adds linked-account list/delete endpoints and adjusts unlink guards to consider linked accounts |
| server/routes/auth.ts | Adds OIDC login + callback endpoints, PKCE/state correlation cookies, and linked-account handling |
| server/routes/auth.test.ts | Adds OIDC flow tests (discovery, state/PKCE, existing user, new user, error cases) |
| server/migration/sqlite/1742858484395-AddLinkedAccounts.ts | SQLite migration to add linked_accounts table and indexes |
| server/migration/postgres/1742858617989-AddLinkedAccount.ts | Postgres migration to add linked_accounts table and indexes |
| server/lib/settings/index.ts | Adds OIDC settings types, public provider exposure, deep-partial merging, and reset() |
| server/interfaces/api/userSettingsInterfaces.ts | Adds API types for linked OIDC accounts |
| server/interfaces/api/settingsInterfaces.ts | Extends public settings response with openIdProviders |
| server/index.ts | Uses settings-derived cookie signing secret and includes optional error in JSON error responses |
| server/entity/User.ts | Adds linkedAccounts relation and filters it from serialized output |
| server/entity/LinkedAccount.ts | New entity for provider/sub-based linked accounts |
| server/constants/error.ts | Adds OIDC-specific API error codes |
| seerr-api.yml | Documents public OIDC providers and OIDC auth + linked-account endpoints |
| public/images/openid.svg | Adds default OpenID logo asset |
| pnpm-lock.yaml | Locks new deps (openid-client, fetch-mock, jose) |
| package.json | Adds runtime/dev dependencies for OIDC and tests |
| gen-docs/docusaurus.config.ts | Enables diff highlighting for docs snippets |
| docs/using-seerr/settings/users/oidc.md | Adds OIDC configuration guide (settings.json-based) |
| docs/using-seerr/settings/users/index.md | Adds initial “User Settings” docs entry |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
seerr-api.yml (1)
257-260:⚠️ Potential issue | 🟡 MinorExpose
openIdProvidersonPublicSettingstoo.This addition only updates
MainSettings, but/settings/publicstill returnsPublicSettings. The public contract therefore still omits the configured OIDC provider list.📘 Proposed OpenAPI update
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 PublicSettings schema currently lacks the openIdProviders array (you added it only to MainSettings), so update the PublicSettings schema to include an openIdProviders property mirroring MainSettings: add openIdProviders: type: array with items: $ref: '#/components/schemas/PublicOidcProvider' (and add to required if appropriate). Locate the PublicSettings definition in seerr-api.yml and insert the property so /settings/public returns the configured OIDC provider list.
🧹 Nitpick comments (1)
server/routes/user/usersettings.ts (1)
541-543: Minor: Empty 404 response body.These 404 responses send no body, while other handlers in this file return
res.status(404).json({ message: 'User not found.' }). Consider aligning for consistency, though this is a minor detail.♻️ Optional: align 404 response format
if (!user) { - return res.status(404).send(); + return res.status(404).json({ message: 'User not found.' }); }Also applies to: 581-583
🤖 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 541 - 543, Replace the empty 404 responses that use "if (!user) return res.status(404).send();" with the consistent JSON payload used elsewhere: "res.status(404).json({ message: 'User not found.' })". Update both locations where the "user" check returns an empty 404 so the handlers return the same structured error message (match other handlers in usersettings.ts).
🤖 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 4124-4133: Update the 400 response schema to include the
OIDC_AUTHORIZATION_FAILED error returned by the OIDC callback when PKCE/state
cookies are missing or tampered; specifically add an additional example or enum
entry for "OIDC_AUTHORIZATION_FAILED" in the 'error' property so the OpenAPI
spec documents both OIDC_MISSING_EMAIL and OIDC_AUTHORIZATION_FAILED (aligning
with server/routes/auth.ts which returns ApiErrorCode.OidcAuthorizationFailed on
PKCE/state verification failures). Ensure the description or examples plainly
note this path occurs before account creation and maps to the same 400 response.
In `@src/components/Login/OidcLoginButton.tsx`:
- Line 11: The effect that auto-starts OIDC uses loading and the presence of the
query param to call initiateOidcLogin repeatedly on failure; add a guard state
(e.g., attemptedAutoLoginProvider) tracked in the Login component and checked
inside the useEffect so you only call initiateOidcLogin(slug) when
attemptedAutoLoginProvider !== slug, set attemptedAutoLoginProvider to slug
immediately before calling initiateOidcLogin, and on rejection leave that flag
set (and clear loading as you already do) so the effect will not retry the same
provider; update references to loading and the query param check in the
useEffect to include the attemptedAutoLoginProvider guard.
---
Duplicate comments:
In `@seerr-api.yml`:
- Around line 257-260: The PublicSettings schema currently lacks the
openIdProviders array (you added it only to MainSettings), so update the
PublicSettings schema to include an openIdProviders property mirroring
MainSettings: add openIdProviders: type: array with items: $ref:
'#/components/schemas/PublicOidcProvider' (and add to required if appropriate).
Locate the PublicSettings definition in seerr-api.yml and insert the property so
/settings/public returns the configured OIDC provider list.
---
Nitpick comments:
In `@server/routes/user/usersettings.ts`:
- Around line 541-543: Replace the empty 404 responses that use "if (!user)
return res.status(404).send();" with the consistent JSON payload used elsewhere:
"res.status(404).json({ message: 'User not found.' })". Update both locations
where the "user" check returns an empty 404 so the handlers return the same
structured error message (match other handlers in usersettings.ts).
🪄 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: 04357517-f8af-434e-af0b-5849377ec7cd
📒 Files selected for processing (7)
package.jsonseerr-api.ymlserver/entity/User.tsserver/routes/auth.test.tsserver/routes/auth.tsserver/routes/user/usersettings.tssrc/components/Login/OidcLoginButton.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- server/routes/auth.test.ts
michaelhthomas
force-pushed
the
feat/oidc-login-basic
branch
from
6af7d47 to
86dcadd
Compare
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