feat: simultaneous multiple auth methods#2690
Closed
afonsojramos wants to merge 19 commits intoseerr-team:developfrom
Closed
feat: simultaneous multiple auth methods#2690afonsojramos wants to merge 19 commits intoseerr-team:developfrom
afonsojramos wants to merge 19 commits intoseerr-team:developfrom
Conversation
Rename mediaServerType→primaryMediaServer, mediaServerLogin→enabledAuthMethods, newPlexLogin→newUserLogin. Add settings migration 0009. Add adminToken to PlexSettings. Add isAuthMethodEnabled helper. No behavioral changes — field renames only.
Replace mutual exclusion auth guards with enabledAuthMethods.includes() checks. Add Plex admin token fallback for secondary Plex auth. Fix logout device cleanup to use user.userType. Add email collision rejection across auth methods.
Replace binary media server / local toggle with stacked layout showing all enabled auth methods simultaneously. Primary server auth on top, secondary below, local login at bottom with "or" dividers.
- Show both Plex and Jellyfin/Emby tabs when multiple servers are configured - Replace single media server login toggle with individual checkboxes per auth method - Add Plex admin token fallback for secondary server configuration - Add POST /settings/plex/auth endpoint for secondary Plex OAuth - Add GET /settings/plex/status endpoint to check Plex connection - Update i18n keys for new auth method messages
📝 WalkthroughWalkthroughRenames global media server fields to a primary server and adds multi-auth support: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant AuthAPI
participant Settings as SettingsSvc
participant DB
participant PlexAPI
User->>Client: Select auth method & submit credentials
Client->>AuthAPI: POST /auth/login {method, creds}
AuthAPI->>SettingsSvc: isAuthMethodEnabled(method)?
SettingsSvc->>DB: read enabledAuthMethods & primaryMediaServer
DB-->>SettingsSvc: enabledAuthMethods, primaryMediaServer
SettingsSvc-->>AuthAPI: allowed / not allowed
alt allowed
AuthAPI->>AuthAPI: route to method handler
alt Plex
AuthAPI->>SettingsSvc: getPlexToken()
SettingsSvc-->>AuthAPI: admin or user token
AuthAPI->>PlexAPI: validate token/credentials
PlexAPI-->>AuthAPI: validation result
else Jellyfin/Emby
AuthAPI->>AuthAPI: validate via Jellyfin/Emby handler
end
AuthAPI->>DB: create/update user (userType from method/primaryMediaServer)
DB-->>AuthAPI: user saved
AuthAPI-->>Client: 200 OK + session
else not allowed
AuthAPI-->>Client: 403 Auth method disabled
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Seerr’s authentication configuration to support multiple simultaneous auth providers (Plex + Jellyfin/Emby + local), including settings migration and UI updates, while also bundling several unrelated improvements (notifications, trending filters, tests, i18n, etc.).
Changes:
- Introduces
primaryMediaServer+enabledAuthMethods[](replacingmediaServerType+mediaServerLogin) and adds a settings migration (0009_multi_auth). - Updates login/settings UI to handle multiple auth methods and adds Plex secondary-token endpoints (
/settings/plex/auth,/settings/plex/status). - Adds unit test runner plumbing (node:test) + DB seeding utilities, plus various notification/trending/i18n enhancements.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
server/routes/auth.ts |
Switches auth enablement checks to enabledAuthMethods and adds email-collision logic (but currently misses setup initialization for enabledAuthMethods). |
server/lib/settings/migrations/0009_multi_auth.ts |
Migrates legacy settings to the new multi-auth shape. |
src/components/Login/index.tsx |
Refactors login page to show one method at a time with collapsible alternatives. |
src/components/Settings/SettingsUsers/index.tsx |
Adds per-method toggles backed by enabledAuthMethods (but currently renders Jellyfin/Emby as a single combined toggle). |
server/routes/settings/index.ts |
Adds Plex token helper + new /plex/auth and /plex/status endpoints. |
server/routes/discover.ts |
Adds mediaType + timeWindow query support for trending. |
server/test/*, server/utils/seedTestDb.ts, server/datasource.ts |
Adds node:test runner integration and deterministic test DB seeding. |
server/lib/notifications/*, src/components/Settings/Notifications/*, docs |
Adds webhook custom headers + ntfy priority support (plus documentation updates). |
src/pages/_error.tsx + various components |
Renames default error export/import to ErrorPage to avoid Error collisions. |
Comments suppressed due to low confidence (1)
server/routes/user/usersettings.ts:384
- Linked-account routes still gate Jellyfin/Emby linking on
primaryMediaServerbeing Jellyfin/Emby. With multi-auth, Jellyfin/Emby could be enabled even when not primary, so this should be based on the enabled auth methods (e.g.,settings.isAuthMethodEnabled(MediaServerType.JELLYFIN/EMBY)) rather thanprimaryMediaServer.
// Make sure jellyfin login is enabled
if (
settings.main.primaryMediaServer !== MediaServerType.JELLYFIN &&
settings.main.primaryMediaServer !== MediaServerType.EMBY
) {
return res
.status(500)
.json({ message: 'Jellyfin/Emby login is disabled' });
}
💡 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.
Author
|
I was not expecting two people to be working on the same feature at the same time 😅 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/TvDetails/index.tsx (1)
343-351:⚠️ Potential issue | 🟡 Minor4K play label is incorrect for Emby/Jellyfin branches
getAvailable4kMediaServerName()usesmessages.play(non-4K) on Line 343 and Line 351. This drops the “4K” label for those providers.💡 Suggested fix
function getAvailable4kMediaServerName() { if (settings.currentSettings.primaryMediaServer === MediaServerType.EMBY) { - return intl.formatMessage(messages.play, { mediaServerName: 'Emby' }); + return intl.formatMessage(messages.play4k, { mediaServerName: 'Emby' }); } if (settings.currentSettings.primaryMediaServer === MediaServerType.PLEX) { return intl.formatMessage(messages.play4k, { mediaServerName: 'Plex' }); } - return intl.formatMessage(messages.play, { mediaServerName: 'Jellyfin' }); + return intl.formatMessage(messages.play4k, { mediaServerName: 'Jellyfin' }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/TvDetails/index.tsx` around lines 343 - 351, The getAvailable4kMediaServerName() implementation currently uses messages.play (non-4K) for the Emby and Jellyfin branches; update those branches so they use messages.play4k instead of messages.play. Specifically, change the branch that checks settings.currentSettings.primaryMediaServer === MediaServerType.EMBY to call intl.formatMessage(messages.play4k, { mediaServerName: 'Emby' }) and change the final return (Jellyfin) to intl.formatMessage(messages.play4k, { mediaServerName: 'Jellyfin' }), leaving the Plex branch as-is.src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/LinkJellyfinModal.tsx (1)
70-70:⚠️ Potential issue | 🟡 MinorTypo in Transition
enterToprop.
opacuty-100should beopacity-100. This typo will prevent the enter transition from working correctly.✏️ Proposed fix
- enterTo="opacuty-100" + enterTo="opacity-100"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/LinkJellyfinModal.tsx` at line 70, The Transition prop in LinkJellyfinModal has a typo: the enterTo value uses "opacuty-100" which prevents the enter transition; update the prop in the LinkJellyfinModal component (the Transition element's enterTo prop) to "opacity-100" so the CSS class is correct and the enter animation works as intended.src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx (1)
116-142:⚠️ Potential issue | 🟠 MajorLinking options may be too restrictive for multi-auth scenarios.
The
hideconditions only checkprimaryMediaServer, but with multi-auth support, users should be able to link accounts for any enabled auth method, not just the primary one. For example, if Plex is configured as a secondary auth method viaenabledAuthMethods, users won't see the option to link their Plex account.Consider also checking
enabledAuthMethods:🐛 Proposed fix
const linkable = [ { name: 'Plex', action: () => { plexOAuth.preparePopup(); setTimeout(() => linkPlexAccount(), 1500); }, hide: - settings.currentSettings.primaryMediaServer !== MediaServerType.PLEX || + (settings.currentSettings.primaryMediaServer !== MediaServerType.PLEX && + !settings.currentSettings.enabledAuthMethods?.includes(MediaServerType.PLEX)) || accounts.some((a) => a.type === LinkedAccountType.Plex), }, { name: 'Jellyfin', action: () => setShowJellyfinModal(true), hide: - settings.currentSettings.primaryMediaServer !== - MediaServerType.JELLYFIN || + (settings.currentSettings.primaryMediaServer !== MediaServerType.JELLYFIN && + !settings.currentSettings.enabledAuthMethods?.includes(MediaServerType.JELLYFIN)) || accounts.some((a) => a.type === LinkedAccountType.Jellyfin), }, { name: 'Emby', action: () => setShowJellyfinModal(true), hide: - settings.currentSettings.primaryMediaServer !== MediaServerType.EMBY || + (settings.currentSettings.primaryMediaServer !== MediaServerType.EMBY && + !settings.currentSettings.enabledAuthMethods?.includes(MediaServerType.EMBY)) || accounts.some((a) => a.type === LinkedAccountType.Emby), }, ].filter((l) => !l.hide);🤖 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 116 - 142, The link option visibility logic in the linkable array is too restrictive because hide only checks settings.currentSettings.primaryMediaServer; update each hide predicate in the linkable entries (for Plex, Jellyfin, Emby) to also allow linking when the corresponding auth method is enabled in settings.currentSettings.enabledAuthMethods (e.g., treat a linkable item as visible if primaryMediaServer === MediaServerType.PLEX OR enabledAuthMethods includes the Plex auth constant), while keeping the existing accounts.some(...) check for already-linked accounts; adjust the conditions for LinkedAccountType.Plex, LinkedAccountType.Jellyfin and LinkedAccountType.Emby respectively in the linkable array.
🧹 Nitpick comments (1)
server/routes/auth.ts (1)
263-270: Error message doesn't mention Emby.The error message says "Jellyfin login is disabled" but this route handles both Jellyfin and Emby. Consider a more accurate message.
✏️ Suggested improvement
if ( settings.main.primaryMediaServer !== MediaServerType.NOT_CONFIGURED && !settings.isAuthMethodEnabled(MediaServerType.JELLYFIN) && !settings.isAuthMethodEnabled(MediaServerType.EMBY) ) { - return res.status(500).json({ error: 'Jellyfin login is disabled' }); + return res.status(500).json({ error: 'Jellyfin/Emby login is disabled' }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.ts` around lines 263 - 270, The error response is misleading for Emby; update the check in the auth route so the returned error message covers both servers: when primaryMediaServer is configured but neither settings.isAuthMethodEnabled(MediaServerType.JELLYFIN) nor settings.isAuthMethodEnabled(MediaServerType.EMBY) is true, change the res.status(500).json call in the auth route to a message that mentions both Jellyfin and Emby (or use a generic "media server login is disabled") and keep the same status and structure.
🤖 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 245-252: Update the OpenAPI schema for primaryMediaServer and
enabledAuthMethods to restrict them to the MediaServerType enum and make
enabledAuthMethods unique: replace the free number type for primaryMediaServer
with an enum containing the exact numeric values from the MediaServerType
definition (so requests can only pick supported media servers) and change
enabledAuthMethods from a plain numeric array to an array whose items use that
same MediaServerType enum and include uniqueItems: true so duplicate/unsupported
auth-method IDs are rejected at the API boundary; locate the settings schema
definitions for primaryMediaServer and enabledAuthMethods in seerr-api.yml and
apply these changes.
In `@server/lib/overseerrMerge.ts`:
- Around line 12-14: The migration gate only checks
settings.main.primaryMediaServer, so legacy configs that only have
settings.main.mediaServerType can slip through and trigger the Overseerr merge;
update the gate in overseerrMerge.ts to also consider the legacy field by
returning false when either settings.main.primaryMediaServer or
settings.main.mediaServerType is present (i.e., change the condition around the
existing if that references settings.main.primaryMediaServer to include
settings.main.mediaServerType).
In `@server/routes/auth.ts`:
- Around line 102-103: The code currently instantiates PlexTvAPI with a possibly
empty plexToken (derived from mainUser.plexToken || settings.plex.adminToken ||
''), which can lead to misleading behavior when checkUserAccess() simply returns
false; before creating new PlexTvAPI or calling checkUserAccess(), explicitly
validate that at least one token exists (check mainUser.plexToken and
settings.plex.adminToken), and if neither is present return the intended 403
response (or log a clear error) rather than proceeding to instantiate PlexTvAPI;
update the block around plexToken / new PlexTvAPI(...) and the subsequent call
to PlexTvAPI.checkUserAccess to short-circuit when token is empty.
In `@server/routes/settings/index.ts`:
- Around line 266-328: The OpenAPI spec is missing operations for the new
endpoints settingsRoutes.post('/plex/auth') and
settingsRoutes.get('/plex/status'), which breaks validation and documentation;
update seerr-api.yml to add a POST /settings/plex/auth operation (request body
with authToken, 200 response schema with username/email, and 400/500 responses)
and a GET /settings/plex/status operation (200 response schema covering
configured, serverName, username, and connectionError cases), ensuring
operationIds/method names align with existing patterns and any security
requirements match other settings routes so the upstream OpenAPI validator
accepts requests to Plex auth/status.
- Around line 66-74: The token precedence in getPlexToken is reversed: change
the return logic in getPlexToken so it prefers the stored settings token first
(settings.plex.adminToken) and falls back to the user's token (admin.plexToken)
only if the settings value is empty, returning '' as final fallback; update the
return expression that currently reads admin.plexToken ||
settings.plex.adminToken || '' to settings.plex.adminToken || admin.plexToken ||
'' (ensure you reference getPlexToken, admin.plexToken and
settings.plex.adminToken).
In `@server/routes/user/index.ts`:
- Around line 595-601: The code constructs plexToken from mainUser.plexToken and
settings.plex.adminToken and then instantiates PlexTvAPI with it; add validation
to ensure plexToken is non-empty before creating PlexTvAPI (check the computed
plexToken variable after the fallback), and if missing throw or return a clear
error/log (mentioning where: mainUser, settings.plex.adminToken) so PlexTvAPI is
not called with an empty string; ensure the error path uses the same control
flow as surrounding logic (e.g., throw an Error or call next/error handler) so
callers see a meaningful message.
- Around line 714-716: The userType assignment currently relies solely on
settings.isAuthMethodEnabled(MediaServerType.EMBY) which will pick EMBy when
both are enabled; change the logic in the user creation path (the code assigning
userType) to first inspect primaryMediaServer (or equivalent property on the
import/source object) and set UserType based on that (e.g., if
primaryMediaServer === MediaServerType.JELLYFIN then UserType.JELLYFIN, else if
=== MediaServerType.EMBY then UserType.EMBY), and only fall back to
settings.isAuthMethodEnabled(MediaServerType.EMBY) ? UserType.EMBY :
UserType.JELLYFIN when primaryMediaServer is not present; update references
around userType, settings.isAuthMethodEnabled, MediaServerType.EMBY,
UserType.EMBY and UserType.JELLYFIN accordingly.
In `@server/routes/user/usersettings.ts`:
- Around line 279-281: The current checks in the Plex, Jellyfin, and Emby
account link/unlink routes only allow operations when
settings.main.primaryMediaServer equals the specific MediaServerType (e.g.,
MediaServerType.PLEX); update these to also allow the operation when the
corresponding server is present in settings.main.enabledAuthMethods (e.g.,
settings.main.enabledAuthMethods.includes(MediaServerType.PLEX)). Concretely,
replace the primary-only guard used in the Plex route (where
settings.main.primaryMediaServer !== MediaServerType.PLEX) and the similar
guards for Jellyfin/Emby with a condition that permits the request if either
settings.main.primaryMediaServer === <TYPE> OR
settings.main.enabledAuthMethods?.includes(<TYPE>); apply this pattern to the
Plex link/unlink handlers and the Jellyfin/Emby checks (referencing
MediaServerType.PLEX, MediaServerType.JELLYFIN, MediaServerType.EMBY and
settings.main.enabledAuthMethods) so the API matches the client-side
availability logic.
In `@src/components/Login/index.tsx`:
- Around line 72-76: The defaultMethod logic can fall back to 'plex' even when
no Plex UI exists; change it to pick the first actually enabled method or
surface a misconfiguration state: compute defaultMethod from formMethods[0] if
present, otherwise use 'local' only when localLogin is true, and if neither is
available set activeMethod to a sentinel (e.g., undefined or 'none') and update
the render path to show an explicit misconfiguration message; update the
identifiers defaultMethod, formMethods, localLogin, activeMethod and
setActiveMethod accordingly so the component renders the first enabled auth
method or a clear error instead of an empty card.
- Around line 19-20: The import statement only pulls hooks so the React
namespace isn't in scope, causing unsafe references to React types; update the
imports to include the React namespace so types like React.FC and React.SVGProps
are available — e.g., import React along with useEffect/useState (so references
in the Login component where React.FC and any SVG prop types are used are
valid). Ensure the top-level import brings in React (the namespace) while
keeping existing hook imports.
In `@src/components/Settings/SettingsJellyfin.tsx`:
- Around line 257-267: The mediaServerName in mediaServerFormatValues can be
undefined when neither Jellyfin nor Emby is selected; update the assignment in
SettingsJellyfin to provide a safe fallback (e.g., use the value of
primaryMediaServer or a generic string like "Media Server") so UI messages that
consume mediaServerFormatValues always receive a string; change the logic that
computes mediaServerName (referencing
settings.currentSettings.primaryMediaServer,
settings.currentSettings.enabledAuthMethods, MediaServerType, and
mediaServerFormatValues) to return Jellyfin/Emby when matched and otherwise fall
back to the chosen primaryMediaServer or a default label.
In `@src/components/Settings/SettingsLayout.tsx`:
- Around line 119-130: The getAvailableMediaServerName function can return
undefined when primaryMediaServer is Plex and both jellyfinConfigured and
embyConfigured are true; update getAvailableMediaServerName to handle the
both-enabled case explicitly (e.g., prefer one server by priority, return a
combined label like "Jellyfin / Emby", or a safe fallback string such as "Media
Server") so mediaServerName is never undefined before calling
intl.formatMessage; refer to getAvailableMediaServerName, primaryMediaServer,
jellyfinConfigured, embyConfigured, and MediaServerType to implement the
explicit branching and ensure the formatted message always receives a concrete
mediaServerName value.
In
`@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/LinkJellyfinModal.tsx`:
- Around line 59-62: The modal title uses
settings.currentSettings.primaryMediaServer to compute mediaServerName, which
can be incorrect when the user explicitly selects an account type from the
dropdown; update the LinkJellyfinModal component to accept a prop (e.g.,
selectedMediaServer or mediaServerType) and use that prop instead of
settings.currentSettings.primaryMediaServer when determining mediaServerName
(and anywhere else the modal needs the selected server type); update the parent
that opens the modal to pass the user-selected value (matching MediaServerType
enum) into LinkJellyfinModal so the title and behavior reflect the user’s
dropdown choice.
---
Outside diff comments:
In `@src/components/TvDetails/index.tsx`:
- Around line 343-351: The getAvailable4kMediaServerName() implementation
currently uses messages.play (non-4K) for the Emby and Jellyfin branches; update
those branches so they use messages.play4k instead of messages.play.
Specifically, change the branch that checks
settings.currentSettings.primaryMediaServer === MediaServerType.EMBY to call
intl.formatMessage(messages.play4k, { mediaServerName: 'Emby' }) and change the
final return (Jellyfin) to intl.formatMessage(messages.play4k, {
mediaServerName: 'Jellyfin' }), leaving the Plex branch as-is.
In
`@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsx`:
- Around line 116-142: The link option visibility logic in the linkable array is
too restrictive because hide only checks
settings.currentSettings.primaryMediaServer; update each hide predicate in the
linkable entries (for Plex, Jellyfin, Emby) to also allow linking when the
corresponding auth method is enabled in
settings.currentSettings.enabledAuthMethods (e.g., treat a linkable item as
visible if primaryMediaServer === MediaServerType.PLEX OR enabledAuthMethods
includes the Plex auth constant), while keeping the existing accounts.some(...)
check for already-linked accounts; adjust the conditions for
LinkedAccountType.Plex, LinkedAccountType.Jellyfin and LinkedAccountType.Emby
respectively in the linkable array.
In
`@src/components/UserProfile/UserSettings/UserLinkedAccountsSettings/LinkJellyfinModal.tsx`:
- Line 70: The Transition prop in LinkJellyfinModal has a typo: the enterTo
value uses "opacuty-100" which prevents the enter transition; update the prop in
the LinkJellyfinModal component (the Transition element's enterTo prop) to
"opacity-100" so the CSS class is correct and the enter animation works as
intended.
---
Nitpick comments:
In `@server/routes/auth.ts`:
- Around line 263-270: The error response is misleading for Emby; update the
check in the auth route so the returned error message covers both servers: when
primaryMediaServer is configured but neither
settings.isAuthMethodEnabled(MediaServerType.JELLYFIN) nor
settings.isAuthMethodEnabled(MediaServerType.EMBY) is true, change the
res.status(500).json call in the auth route to a message that mentions both
Jellyfin and Emby (or use a generic "media server login is disabled") and keep
the same status and structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 572ef217-18da-4117-af11-c262b650b162
📒 Files selected for processing (39)
cypress/config/settings.cypress.jsonseerr-api.ymlserver/api/jellyfin.tsserver/entity/Media.tsserver/interfaces/api/settingsInterfaces.tsserver/job/schedule.tsserver/lib/availabilitySync.tsserver/lib/overseerrMerge.tsserver/lib/scanners/jellyfin/index.tsserver/lib/settings/index.tsserver/lib/settings/migrations/0009_multi_auth.tsserver/routes/auth.tsserver/routes/avatarproxy.tsserver/routes/settings/index.tsserver/routes/user/index.tsserver/routes/user/usersettings.tssrc/components/ExternalLinkBlock/index.tsxsrc/components/IssueDetails/index.tsxsrc/components/Login/JellyfinLogin.tsxsrc/components/Login/index.tsxsrc/components/ManageSlideOver/index.tsxsrc/components/MovieDetails/index.tsxsrc/components/PermissionEdit/index.tsxsrc/components/Settings/SettingsJellyfin.tsxsrc/components/Settings/SettingsJobsCache/index.tsxsrc/components/Settings/SettingsLayout.tsxsrc/components/Settings/SettingsUsers/index.tsxsrc/components/Setup/index.tsxsrc/components/StatusBadge/index.tsxsrc/components/TvDetails/index.tsxsrc/components/UserList/JellyfinImportModal.tsxsrc/components/UserList/PlexImportModal.tsxsrc/components/UserList/index.tsxsrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/LinkJellyfinModal.tsxsrc/components/UserProfile/UserSettings/UserLinkedAccountsSettings/index.tsxsrc/context/SettingsContext.tsxsrc/hooks/useDeepLinks.tssrc/i18n/locale/en.jsonsrc/pages/_app.tsx
And that too on the same day lol. Mine is very much rough right now. I mainly opened the PR here to get code reviews from coderabbit. Don't know why github isn't letting me convert the PR into draft. |
7 tasks
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/routes/user/index.ts (1)
595-601:⚠️ Potential issue | 🟡 MinorEmpty Plex token fallback may cause API failure.
If both
mainUser.plexTokenandsettings.plex.adminTokenare empty/undefined,plexTokenbecomes an empty string. The subsequentPlexTvAPIcall at line 601 will fail without a meaningful error message to the caller.🛡️ Proposed fix to validate token before API call
// Use admin's Plex token, falling back to secondary Plex admin token const mainUser = await userRepository.findOneOrFail({ select: { id: true, plexToken: true }, where: { id: 1 }, }); const plexToken = mainUser.plexToken || settings.plex.adminToken || ''; + if (!plexToken) { + return next({ + status: 400, + message: 'No Plex authentication token configured.', + }); + } const mainPlexTv = new PlexTvAPI(plexToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/user/index.ts` around lines 595 - 601, The code constructs PlexTvAPI with a potentially empty token (plexToken) derived from mainUser.plexToken or settings.plex.adminToken; validate that plexToken is non-empty before calling new PlexTvAPI (e.g., in the same block that uses userRepository.findOneOrFail and before instantiating PlexTvAPI) and if it's empty throw or return a clear error (including context like "missing Plex admin token" and the affected user id) so the caller receives a meaningful message instead of a downstream API failure.
🤖 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 444-451: The code is resolving ServerType using
settings.main.primaryMediaServer (via MediaServerType) which misclassifies users
for multi-primary setups; change all such ternaries that produce ServerType (the
expressions using settings.main.primaryMediaServer === MediaServerType.JELLYFIN
? ServerType.JELLYFIN : ServerType.EMBY) to derive the server type from the
active login/auth method (e.g., the request/session auth provider variable such
as loginMethod/provider/authMethod that indicates whether the current auth flow
is Jellyfin or Emby) and update the corresponding log messages that reference
updating user with X to use that resolved value; apply this replacement where
those ternaries appear (the same logic at the three affected branches) so
downstream behavior uses the active login method instead of
settings.main.primaryMediaServer.
- Around line 699-707: The axios.delete call that removes the Jellyfin/Emby
device (the await axios.delete(...) in server/routes/auth.ts using
getHostname(), user.jellyfinDeviceId, getAppVersion(), and
settings.jellyfin.apiKey) must include an explicit timeout and not block logout;
add a timeout property (e.g. timeout: 5000) to the request config passed to
axios.delete and ensure the call is wrapped in a try/catch so any timeout or
network error is logged/ignored but does not prevent the subsequent session
destruction code from running.
---
Duplicate comments:
In `@server/routes/user/index.ts`:
- Around line 595-601: The code constructs PlexTvAPI with a potentially empty
token (plexToken) derived from mainUser.plexToken or settings.plex.adminToken;
validate that plexToken is non-empty before calling new PlexTvAPI (e.g., in the
same block that uses userRepository.findOneOrFail and before instantiating
PlexTvAPI) and if it's empty throw or return a clear error (including context
like "missing Plex admin token" and the affected user id) so the caller receives
a meaningful message instead of a downstream API failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8067431-606b-41f8-9ed4-1c0d2dc92f8c
📒 Files selected for processing (9)
seerr-api.ymlserver/lib/overseerrMerge.tsserver/routes/auth.tsserver/routes/settings/index.tsserver/routes/user/index.tsserver/routes/user/usersettings.tssrc/components/Login/index.tsxsrc/components/Settings/SettingsJellyfin.tsxsrc/components/Settings/SettingsLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Login/index.tsx
- server/lib/overseerrMerge.ts
- src/components/Settings/SettingsLayout.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 266-270: The current gating uses a combined check against both
Jellyfin and Emby which allows a disabled provider to be used when the other
provider is enabled; update the logic to determine the actual provider (use the
existing resolvedServerType / settings.main.primaryMediaServer resolution) and
call settings.isAuthMethodEnabled(resolvedType) for that single provider,
returning/blocking when that specific provider is disabled; apply the same fix
to the other occurrences that reference the dual check (the later block around
resolvedServerType fallback and the checks at the regions noted) so every
auth-path validates the resolved provider via
settings.isAuthMethodEnabled(resolvedType) instead of checking both
MediaServerType.JELLYFIN and MediaServerType.EMBY.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e16cde0e-d6ae-4f1e-b073-100e38681649
📒 Files selected for processing (1)
server/routes/auth.ts
…h Jellyfin and Emby
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Member
|
I haven’t reviewed any of the PRs yet, so I can’t say which approach we prefer. However, this won’t be worked on anytime soon, as we’re currently focusing on OIDC before tackling this feature. |
Collaborator
|
I have not reviewed this but based on the description, here's my two cents: We already have a linked accounts feature that was built specifically for allowing multiple auth methods on a single account. The right approach here would be to extend that system to support cross-server linking, rather than introducing a parallel Currently linked accounts is gated to only the configured Also as @M0NsTeRRR mentioned, OIDC is the current priority before this feature which properly makes use of this linked accounts feature (you can take a look at it to see how it's implemented). |
Author
|
@fallenbagel @M0NsTeRRR Thanks for the feedback — that’s a really good point about extending the linked accounts system. I hadn’t considered that OIDC would build on top of it too, which makes alignment there more important. The main gap I was trying to fill is allowing direct sign-in via a secondary server without needing a pre-existing account + manual linking step. But I can see how extending linked accounts + relaxing the auth route guards would be a simpler foundation, especially if OIDC is coming first. Happy to rework toward that approach or wait until after OIDC lands to see what makes sense. |
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
Fixes #100
So, first time contributor, but contributing towards a feature that I've wanted for a loooooong time, and looking at the issue number I am addressing, I don't think I was the only one.
I went with the approach of adding
primaryMediaServer+enabledAuthMethods[]alongside the existing fields rather than a full media server registry because it would be the minimal change needed — no major database schema changes, no new entities, and the existingplexId/jellyfinUserIdcolumns on the User model already support multi-server identity out of the box. Users have a single identity regardless of which method they authenticate with, and email collisions across auth providers are rejected (login is denied if the email is already associated with a different auth method). The login page reuses the existing collapsible button pattern from the original codebase, showing one form at a time with branded toggle buttons for alternatives, keeping Plex as always-a-button since it's just an OAuth redirect. The setup wizard is intentionally left unchanged (this only applies to already-configured instances), and a settings migration handles the rename frommediaServerType→primaryMediaServerandnewPlexLogin→newUserLoginso existing configurations migrate seamlessly. Multi-library support across servers was explicitly scoped out as a separate concern.This PR is purely about allowing users to sign in with multiple auth providers simultaneously, instead of needing multiple instances per server (since there are multiple cases of people who have both Jellyfin and Plex for the same media).
Changes
mediaServerTypewithprimaryMediaServer+enabledAuthMethods[]array to support simultaneous Plex, Jellyfin, and Emby authentication0009_multi_auth) to seamlessly migrate existing configurations/plex/auth,/plex/status) for secondary Plex configurationKey Design Decisions
How Has This Been Tested?
Screenshots / Logs
AI Assistance Disclosure
This PR was developed with Claude Code as a coding assistant. I directed all design decisions: the approach (adding
enabledAuthMethods[]vs. a full registry), scope boundaries (auth-only, no multi-library), email collision strategy, and UX pattern (collapsible login matching the existing codebase). Claude Code generated code under my direction and I reviewed each change iteratively, testing locally against multiple configurations (Plex-primary, Jellyfin-primary, both enabled) before submitting. I understand the code produced and can answer questions about any part of it.Checklist
pnpm buildpnpm i18n:extract0009_multi_auth.ts(no DB schema change)Summary by CodeRabbit
New Features
Improvements
Chores