overseerr icon indicating copy to clipboard operation
overseerr copied to clipboard

feat(auth): add ForwardAuth support via `X-Plex-Token` header

Open tobz opened this issue 3 years ago • 9 comments

Description

This is an attempt to partially add ForwardAuth support to Overseerr, specifically for Plex-based logins.

#1555 and #1638 have already covered much of the ground on the wants/needs of Overseerr users when it comes to authentication, but this solution focuses purely on the ForwardAuth usecase: an application running behind a reverse proxy (or similar) system that derives authentication information itself (or via some other mechanism) and "forwards" that authentication to the proxied application. This PR does not concern itself with augmenting Overseerr's existing authentication methods with support for external identity providers, and so on.

In the ForwardAuth model, "trusted headers" are sent to the underlying application which carry the authentication information: username, e-mail, etc. In this PR, we're exclusively focusing on support for passing an existing Plex token as a trusted header, which drives newly-added logic to automatically create and/or log in the user based on that Plex token.

This new feature obeys the following rules/logic:

  • we do not create a new user if an imported one does not already exist and "Enable New Plex Sign-In" is disabled
  • we have added a new setting, "Enable Forward Auth Via Plex Token", which must be enabled for any of this new logic to take effect
  • we always attempt to create and/or login a user via this mechanism so long as the above constraints are meant, regardless of which page they access

Essentially, the login flow is skipped entirely if the above constraints are met: on a normal load to /, the first call to /api/v1/auth/me would log the user in via their Plex token, create and initialize their session, set their session cookie, and allow the UI to send them straight to the normal dashboard landing page.

To say it out loud: this is predicated on at least sending the ForwardAuth header along to the /api/v1/auth/me endpoint, but generally ForwardAuth implementations will send their authentication headers uniformly to all requests being proxied.

Beyond that, I've tried to refactor some of the Plex login code to be reusable between the actual Plex login endpoint and the code I've added for ForwardAuth support. I'm not a TypeScript developer by trade, so apologies if things are in the wrong place, not idiomatic, etc.

Screenshot (if UI-related)

To-Dos

  • [x] Successful build yarn build
  • [ ] Translation keys yarn i18n:extract
  • [ ] Database migration (if required)

tobz avatar Nov 01 '22 02:11 tobz

Request to make the header configurable. To simplify it should be configurable via environment variable (no need for something as fancy as a UX). I use Authelia which uses the Remote-User and would absolutely be onboard for using this. I just added a comment in #1638 revisiting why forward auth is superior to OIDC, however it really limits the use of this feature if its limited to just Plex's token. Not all my users have plex accounts (in my case they share a plex pass account) since Plex doesn't allow loading of local accounts, but they do have their own accounts for other services.

lenaxia avatar Dec 14 '22 16:12 lenaxia

Request to make the header configurable. To simplify it should be configurable via environment variable (no need for something as fancy as a UX). I use Authelia which uses the Remote-User and would absolutely be onboard for using this. I just added a comment in https://github.com/sct/overseerr/issues/1638 revisiting why forward auth is superior to OIDC, however it really limits the use of this feature if its limited to just Plex's token. Not all my users have plex accounts (in my case they share a plex pass account) since Plex doesn't allow loading of local accounts, but they do have their own accounts for other services.

I can understand your use case, and it makes sense. That said, adding support here to additionally support local accounts via ForwardAuth would be a significant increase in logic on top of what I've already built... which, given the seemingly low chance that this PR ever gets a real review and approved/merged, is just that much more work for (potentially) nothing.

tobz avatar Dec 14 '22 22:12 tobz

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Feb 18 '23 00:02 stale[bot]

@sct @TheCatLady @danshilm

Apologies for the direct ping, but, is it possible to review this? Or, if nothing else, to tell me whether or not you're even willing to accept this feature?

tobz avatar Feb 19 '23 17:02 tobz

Hi @tobz. Sorry for the wait. We are still very busy, but we want to get to this! I am also in the process of refactoring some login and user type logic with the update to decouple plex in #3015. This will likely affect some of the logic you have here. So please give us a little longer.

sct avatar Feb 20 '23 12:02 sct

Hi @tobz. Sorry for the wait. We are still very busy, but we want to get to this! I am also in the process of refactoring some login and user type logic with the update to decouple plex in #3015. This will likely affect some of the logic you have here. So please give us a little longer.

Understandable. 👍🏻

I'll rebase my branch on top of yours at some point and see what it looks like, and get it as close as I reasonably can for whenever #3015 lands.

tobz avatar Feb 20 '23 17:02 tobz

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Apr 25 '23 19:04 stale[bot]

@sct Checking in again.

It looks like your work on the Plex decoupling stuff stalled out around 2-3 months ago. No judgment at all whatsoever, but given that we talked about that work landing before coming back to revisit this PR getting merged... could we potentially come to a decision around whether to merge this or not?

The PR has been open for almost a year at this point. 😅

tobz avatar Oct 15 '23 16:10 tobz

Would love to see this implemented

geman220 avatar Dec 22 '23 18:12 geman220