overseerr icon indicating copy to clipboard operation
overseerr copied to clipboard

OIDC Support - Attempt 2

Open lenaxia opened this issue 1 year ago • 15 comments

Description

This PR rebases the oidc changes to the current Overseerr mainline (develop), and fixes some improper OIDC implementation. Because of this, it now works with authelia. Changes have also been revalidated to work with a basic configuration of Authentik. I have not tested this with other OIDC providers.

Fixes include:

  • Adding support for scope and error parameters in the oidc-callback endpoint (these are all optional parameters)
  • fixing callback redirect_uri protocol generation to base on the initiating protocol, as opposed to assumping https, which is what it did previously
  • add support for the aud (audience) callback parameter being an array. the OIDC spec allows aud to be either a string, or an array of strings. Previous implementation here only allowed string when doing a oidc validation. This is now fixed to support both string and array
  • Added improved logging and error handling to make future debugging easier.
  • change default logging level to be info if LOG_LEVEL env variable is not defined (previously was debug)
  • Improved JWT token validation robustness

Bug Fix:

  • Plex New Login button was incorrectly tied to the localLogin settings flag. It also was disabled unnecessarily and did not match the formatting of the local login (and now OIDC) buttons. This has been fixed.

Screenshot (if UI-related)

To-Dos

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

Issues Fixed or Closed

  • Fixes: https://github.com/sct/overseerr/pull/2792

lenaxia avatar Jan 10 '24 07:01 lenaxia

@sct Let's try this again, this should only include the OIDC changes now, plus some small bug fixes.

lenaxia avatar Jan 10 '24 07:01 lenaxia

Okay, addressed eslint and logging injection attack. Further, improved middleware logging to scrub sensitive data from log messages and sanitize against injection attacks.

ES Lint output:


/home/ubuntu/workspace/overseerr-oidc/server/api/rating/imdbRadarrProxy.ts
   20:20  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
   30:11  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  137:11  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  138:13  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  139:17  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  140:10  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

✖ 6 problems (6 errors, 0 warnings)

yarn build output:

ubuntu@terraform:~/workspace/overseerr-oidc(⎈|prod:networking)$ docker build --build-arg COMMIT_TAG=b4256043424658626e02bd002de1d4369f541bdb -t overseerr-oidc .
[+] Building 596.0s (18/18) FINISHED                                                                                                                               docker:default
 => [internal] load build definition from Dockerfile                                                                                                                         0.1s
 => => transferring dockerfile: 996B                                                                                                                                         0.0s
 => [internal] load .dockerignore                                                                                                                                            0.1s
 => => transferring context: 392B                                                                                                                                            0.0s
 => [internal] load metadata for docker.io/library/node:18.18-alpine                                                                                                         1.2s
 => [internal] load build context                                                                                                                                            0.2s
 => => transferring context: 53.21kB                                                                                                                                         0.2s
 => [build_image  1/11] FROM docker.io/library/node:18.18-alpine@sha256:16b46e5ea9fb5c2d13dda36f0feb670fa89de6a412725007555f2eee9a126b60                                     0.0s
 => CACHED [build_image  2/11] WORKDIR /app                                                                                                                                  0.0s
 => CACHED [build_image  3/11] RUN   case "linux/amd64" in   'linux/arm64' | 'linux/arm/v7')   apk update &&   apk add --no-cache python3 make g++ gcc libc6-compat bash &&  0.0s
 => CACHED [build_image  4/11] COPY package.json yarn.lock ./                                                                                                                0.0s
 => CACHED [build_image  5/11] RUN CYPRESS_INSTALL_BINARY=0 yarn install --frozen-lockfile --network-timeout 1000000                                                         0.0s
 => [build_image  6/11] COPY . ./                                                                                                                                            0.6s
 => [build_image  7/11] RUN yarn build                                                                                                                                     483.0s
 => [build_image  8/11] RUN yarn install --production --ignore-scripts --prefer-offline                                                                                     21.8s
 => [build_image  9/11] RUN rm -rf src server .next/cache                                                                                                                    0.7s
 => [build_image 10/11] RUN touch config/DOCKER                                                                                                                              0.7s
 => [build_image 11/11] RUN echo "{"commitTag": "b4256043424658626e02bd002de1d4369f541bdb"}" > committag.json                                                                0.6s
 => CACHED [stage-1 3/4] RUN apk add --no-cache tzdata tini && rm -rf /tmp/*                                                                                                 0.0s
 => [stage-1 4/4] COPY --from=BUILD_IMAGE /app ./                                                                                                                           33.3s
 => exporting to image                                                                                                                                                      24.2s
 => => exporting layers                                                                                                                                                     24.1s
 => => writing image sha256:bc03784e0d0e1914db7968888b72233a1e81cd9ac9ca2672cf4fec8a41631865                                                                                 0.0s
 => => naming to docker.io/library/overseerr-oidc                                                                                                                            0.0s
ubuntu@terraform:~/workspace/overseerr-oidc(⎈|prod:networking)$

lenaxia avatar Jan 10 '24 20:01 lenaxia

@sct got eslint passing (for code I touched) and addressed log injection risk.

lenaxia avatar Jan 10 '24 20:01 lenaxia

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 Mar 13 '24 09:03 stale[bot]

Not stale

@sct can we get someone to look at this?

lenaxia avatar Mar 14 '24 00:03 lenaxia

merged latest develop into the branch.

lenaxia avatar Mar 22 '24 17:03 lenaxia

I've just tested this fork with my Authelia setup, and it works; thank you, @lenaxia, for the docker image and for keeping this PR up to date.

Some quick thoughts after using this:

  • The setting "Enable New Plex Sign-In (Allow Plex users to sign in without first being imported)" could be changed to include OIDC users that log in.
  • I would love it if there was a way to disable sign-in via Plex to only allow OIDC logins, just like there is an option to disable local logins. (The logic could allow admins to disable sign-in methods like local, Plex, and OIDC as long as one method is enabled).

@sct is there any reason why this PR isn't being accepted?

[Edit: I realised you might be waiting for #3015 to be merged before accepting this one - thanks for your work on this cool project. I recognise you are doing this in your spare time, so please don't take my comment as an attempt to hassle you. More to understand the logic/sequence you had in mind.]

tenfourty avatar Apr 22 '24 10:04 tenfourty

Has this pull request stalled as well?

nebb00 avatar Jun 25 '24 06:06 nebb00

I'm not 100% sure I got this right but it does work to some extent. When I tested one of my plex users, on the first run, if they're logged via authentik already, they're still prompted with a choice to login with plex or authentik on the overseerr page (when i expected the oauth to see them as an existing plex user). When i check the console in Chrome i see:

GET https://req.mydomain.com/api/v1/auth/me 403 (Forbidden) {"status":403,"error":"You do not have permission to access this endpoint"}

If i have that user click on the authentik login it works from that point on. Is there some setting on authentik that I should know about to translate the username or email to overseerr?

tscibilia avatar Jul 11 '24 01:07 tscibilia

This would be an amazing feature. Why does this keep getting ignored?

xTerm1nus avatar Jul 17 '24 03:07 xTerm1nus

Hello owner!!! Please review!!!

alexsalex avatar Jul 23 '24 18:07 alexsalex

@sct @samwiseg0 @OwsleyJr @danshilm @TheCatLady Please, help to merge.

alexsalex avatar Jul 23 '24 18:07 alexsalex

@lenaxia This branch is out-of-date with the base branch

alexsalex avatar Jul 23 '24 18:07 alexsalex