Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OIDC Support - Attempt 2 #3746

Open
wants to merge 35 commits into
base: develop
Choose a base branch
from
Open

Conversation

lenaxia
Copy link

@lenaxia lenaxia commented Jan 10, 2024

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

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

Issues Fixed or Closed

ankarhem and others added 30 commits October 1, 2022 19:08
feat: oidc 2

feat: oidc
@lenaxia lenaxia mentioned this pull request Jan 10, 2024
2 tasks
@lenaxia
Copy link
Author

lenaxia commented Jan 10, 2024

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

server/index.ts Outdated Show resolved Hide resolved
@lenaxia
Copy link
Author

lenaxia commented Jan 10, 2024

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
Copy link
Author

lenaxia commented Jan 10, 2024

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

This comment was marked as resolved.

@stale stale bot added the stale label Mar 13, 2024
@lenaxia
Copy link
Author

lenaxia commented Mar 14, 2024

Not stale

@sct can we get someone to look at this?

@stale stale bot removed the stale label Mar 14, 2024
@lenaxia
Copy link
Author

lenaxia commented Mar 22, 2024

merged latest develop into the branch.

@tenfourty
Copy link

tenfourty commented Apr 22, 2024

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.]

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.

None yet

3 participants