Skip to content

feat: Add support for forward auth#1341

Closed
sgmitchell wants to merge 8 commits into
seerr-team:developfrom
sgmitchell:fw-auth
Closed

feat: Add support for forward auth#1341
sgmitchell wants to merge 8 commits into
seerr-team:developfrom
sgmitchell:fw-auth

Conversation

@sgmitchell

Copy link
Copy Markdown

Description

Adds support for automatically authenticating as the user specified in the X-Forwarded-User header when the setting enableForwardAuth is set.

This is basically the same as #580 (the first 2 commits are cherry picked from that PR) but rebased and with suggestions applied

Screenshot (if UI-related)

image

To-Dos

  • Successful build pnpm build
  • Translation keys pnpm i18n:extract

Issues Fixed or Closed

@ishanjain28

Copy link
Copy Markdown
Contributor

Hii, I don't see any role/group here. Does this only work for imported users ?
If not, how does it distinguish between jellyfin admins and jellyfin users ?

@sgmitchell

Copy link
Copy Markdown
Author

Hii, I don't see any role/group here. Does this only work for imported users ?

Yes. Any user would need to already be created for this to work. It doesn't provision users automatically.

Comment thread overseerr-api.yml
@gauthier-th gauthier-th mentioned this pull request Feb 11, 2025
1 task
Comment thread server/middleware/auth.ts Outdated
Comment thread src/utils/localRequestHelper.ts
Comment thread Dockerfile
Comment thread Dockerfile
gauthier-th
gauthier-th previously approved these changes Feb 13, 2025

@gauthier-th gauthier-th left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I'd like a few poeple to test this and report it's working well before merging though, as I'm not using forwardAuth myself.
I'll create a preview tag soon.

@tsallou

tsallou commented Feb 13, 2025

Copy link
Copy Markdown

Hello, thank you for this timely contribution! I was actually looking for ways to authenticate my setup.

I tested your code in a test environment using Authentik and Nginx Proxy Manager. It works as expected, which is great, and it's quite simple to set up.

I did notice one issue with the username in the header. I have a user with an email address set up in Jellyseer, and authentication failed for this user. It only worked for users without an email address. I think we need to make it work in all cases, as I use LDAP with Jellyfin to create users in Jellyseer, and they don't have emails by default (though my admin account does).

From a security perspective, I believe merging this PR as-is poses a significant risk. There's nothing to warn the user (either in the UI or the documentation) that enabling this feature without a properly configured reverse proxy exposes the instance completely to anyone who knows a username or email address used in the application.

At the very least, we should clearly state in the UI that enabling this option can seriously compromise the instance's security.

Some more permanent solutions could be:

  • Hiding the setting in a configuration file (this is what Sonarr/Radarr do for similar parameters).
  • Enforcing that the parameter is only considered when the request comes from a trusted proxy IP.

@sgmitchell

Copy link
Copy Markdown
Author

It only worked for users without an email address.

Hmmm. I have it tested working with both local and jellyfin users with and without emails. When a user is added in the UI and no email is set, it will default the email to the username (src). Additionally if you dump the schema by running something like sqlite3 /app/config/db/db.sqlite3 '.schema User --indent', email is the only field with a unique constraint so either that or the integer id are the only 2 fields that I think would be safe to use.

I'm not sure what your ldap plugin is doing but could you inspect your local database and see if your LDAP client is following the process as jellyseerr's source?

There's nothing to warn the user (either in the UI or the documentation)
...
At the very least, we should clearly state in the UI that enabling this option can seriously compromise the instance's security.

I've put a screenshot showing the advanced label and extra text stating Only enable when secured behind a trusted proxy. Do you have a suggestion as to better wording to make that clear?

@jcbxz

jcbxz commented Feb 14, 2025

Copy link
Copy Markdown

Hi 👋

Thanks for picking this up, I'm also interested in this functionality.

I'm chiming in with a slightly different setup, I am not using Authentik but a combo of Authelia and Traefik. It seems as though Authelia will provide slightly different headers compared to Authentik: https://www.authelia.com/integration/trusted-header-sso/introduction/#response-headers.

Would it possible for the header name to be configurable so I can have Jellyseerr look for the appropriate header for Authelia rather than requiring header rewrites in Traefik?

Another fairly minor thing I've noticed is that every page refresh will redirect to the /login before redirecting to the dashboard, so you can lose your place in the app. eg. if you refresh on /discover/tv it will take you back to the dashboard instead of where you were.

@ishanjain28

Copy link
Copy Markdown
Contributor

@jcbxz These headers are returned by authelia in the forward auth response and they have to be copied to the X-... header in trafeik from the forward auth request. It would be nice to have this as a configurable option but it doesn't change any thing. You still need to copy those headers in the reverse proxy.

@jcbxz

jcbxz commented Feb 15, 2025

Copy link
Copy Markdown

@jcbxz These headers are returned by authelia in the forward auth response and they have to be copied to the X-... header in trafeik from the forward auth request. It would be nice to have this as a configurable option but it doesn't change any thing. You still need to copy those headers in the reverse proxy.

Sure, I have it configured to copy the headers from authelia in the traefik middleware with the label they recommend in their example, eg:

labels:
    traefik.enable: 'true'
    traefik.http.routers.authelia.rule: 'Host(`example.com`)'
    traefik.http.middlewares.authelia.forwardAuth.address: 'http://authelia:9091/api/authz/forward-auth'
    traefik.http.middlewares.authelia.forwardAuth.trustForwardHeader: 'true'
    traefik.http.middlewares.authelia.forwardAuth.authResponseHeaders: 'Remote-User,Remote-Groups,Remote-Email,Remote-Name'

The problem is this behavior can only copy headers, not rename them - I can't just put X-Forwarded-User there and expect it to work because Authelia doesn't provide that header. As far as I'm aware X-Forwarded-User isn't a standardized name so perhaps it shouldn't be assumed that's where the header will always be.

but it doesn't change any thing

It changes whether a traefik plugin to modify response headers is required for Jellyseerr. It's not impossible, but it's a bit unfortunate if the feature doesn't cater towards different headers like those Authelia provides.

Just to throw out an example of another app that offers this functionality, this is a screenshot from the Authelia docs of Organizr's configuration page:

image

My 2c, but would it be an option to keep it fairly simple and change the toggle on/off in the settings into an input field which accepts the header name?

@sgmitchell

Copy link
Copy Markdown
Author

My 2c, but would it be an option to keep it fairly simple and change the toggle on/off in the settings into an input field which accepts the header name?

I'm not very familiar with the frameworks in this project but I think in order to allow an arbitrary header, we'd need to:

  • find another way to express the changes to the openapi spec or remove it altogether
  • modify serverSidePropsHelpers to pass along all headers, not just a safe subset of them

Both are doable but I'd defer to someone who is more familiar with the project to weigh in on if that's actually a good change to make

@ishanjain28

Copy link
Copy Markdown
Contributor

Hi @sgmitchell I don't know if you are in the group's discord and I am sure the maintainers will weigh in here.
I had mentioned this PR in the discord and they share the same opinion that the header here should be configurable. I'd be happy to help with this if you like!

@github-actions github-actions Bot added the merge conflict Cannot merge due to merge conflicts label Feb 20, 2025
@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@ishanjain28

Copy link
Copy Markdown
Contributor

I noticed another issue here. It's matching x-forwarded-user header against email field in the DB which can be the same as jellyfinUsername if users were imported from there but it can also be the email address of the user if that was configured for email notifications or any other reason.

X-Forwarded-User should be matched against jellyfinUsername column. I have deployed this PR in my personal instance after making that change and it works perfectly.

@WickedColdfront

Copy link
Copy Markdown

I noticed another issue here. It's matching x-forwarded-user header against email field in the DB which can be the same as jellyfinUsername if users were imported from there but it can also be the email address of the user if that was configured for email notifications or any other reason.

X-Forwarded-User should be matched against jellyfinUsername column. I have deployed this PR in my personal instance after making that change and it works perfectly.

Would it be possible to have this as a configurable option?

I have Jellyseerr behind a Cloudflare tunnel with “One-time PIN” authentication, which allows access if an email adddress is in a predefined list.

Cloudlare passes the authenticated email address in the “Cf-Access-Authenticated-User-Email” header.

There doesn’t seem to be a way within Cloudflare to translate an email address to an arbitrary username, so this change would cause issues with this use case.

@ishanjain28

Copy link
Copy Markdown
Contributor

Would it be possible to have this as a configurable option?

Yes this can be a configurable option. The header name itself and it can be matched against email || jellyfinUsername.

@gauthier-th

gauthier-th commented Feb 21, 2025

Copy link
Copy Markdown
Member

I noticed another issue here. It's matching x-forwarded-user header against email field in the DB which can be the same as jellyfinUsername if users were imported from there but it can also be the email address of the user if that was configured for email notifications or any other reason.

X-Forwarded-User should be matched against jellyfinUsername column. I have deployed this PR in my personal instance after making that change and it works perfectly.

This does not take into account Plex, or local users.

I'm not very familiar with a typical forwardAuth workflow, but here are a few info on how Jellyseerr works with accounts:

  • The unique identifier used by Jellyseerr is the email. This is something inherited from Overseerr. An Overseerr user can't change his email.
  • Things are getting tougher for Jellyfin/emby users: email is not mandatory, so we fill in the email field (that is mandatory) with the Jellyfin username. But a Jellyfin user can change his email directly on Jellyseerr.
  • Local users can change their email like they want.

Yes this can be a configurable option. The header name itself and it can be matched against email || jellyfinUsername.

IMO something like this would be more accurate: user.email || user.plexUsername || user.jellyfinUsername

@fallenbagel

Copy link
Copy Markdown
Member

@sgmitchell could you rebase develop when you can?

@github-actions github-actions Bot removed the merge conflict Cannot merge due to merge conflicts label Mar 18, 2025
Comment thread Dockerfile
Comment thread Dockerfile
@fallenbagel fallenbagel mentioned this pull request Mar 19, 2025
@sgmitchell

Copy link
Copy Markdown
Author

Thanks for rebasing @gauthier-th. Apologies for being absent these past few weeks.

Local users can change their email like they want.

Yup. But one thing to note is that email is the one field that is guaranteed to be set and be unique. So a user can set it to something but not to another user's email.

IMO something like this would be more accurate: user.email || user.plexUsername || user.jellyfinUsername

I'm not sure it's necessarily needed but it wouldn't hurt as long as the union of those fields is always unique. The only benefit I can think of is if a jellyfin user edited their email to something else in jellyseerr.

Unfortunately I don't think I'll have the bandwidth to continue working on this in the near future so I appreciate you helping to keep this moving.

@gauthier-th

Copy link
Copy Markdown
Member

Closed in favor of #1564

@gauthier-th gauthier-th closed this Apr 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for Forward Auth

8 participants