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

Login with OAuth via OpenID Connect (OIDC) #2860

Closed
wants to merge 71 commits into from

Conversation

cmintey
Copy link
Contributor

@cmintey cmintey commented Dec 18, 2023

What type of PR is this?

  • feature

What this PR does / why we need it:

This PR adds the popularly requested login via OpenID Connect (OIDC), specifically using the Authorization Code Flow with PCKE. The work in this PR was originally based off of #1891, but has been modified to work with the switch over to the frontend being a SPA.

This PR comes complete with end-to-end tests for testing the login flow using OIDC. These tests make use of a mock oidc server docker container and Playwright to run the UI tests.

Special notes for your reviewer:

Since we are close to a v1 release, this could maybe wait until after the release so as not to push it back. While I don't think there are any breaking changes with this, various configurations should be tested by users to make sure it covers as many use cases as possible.

Testing

New E2E tests and manual

@cmintey cmintey changed the title OIDC Login with OAuth via OpenID Connect (OIDC) Dec 18, 2023
@hay-kot hay-kot added the post-v1 Merge after v1 stable release label Dec 18, 2023
@hay-kot
Copy link
Collaborator

hay-kot commented Dec 18, 2023

This PR comes complete with end-to-end tests for testing the login flow using OIDC. These tests make use of a mock oidc server docker container and Playwright to run the UI tests.

🔥 🔥 🔥


Couple of questions from an idiot.

  • Why OIDC over something like SSO via Forward Auth Headers?
  • If move to OIDC, should we still support LDAP (long term) i.e. is there any reasons why LDAP users couldn't easily switch to OIDC implementation?
  • I've been looking at https://github.com/markbates/goth for another project as an "auth abstraction". They have this model of a Provider/Session interface and they use that to support multiple authentication models. You've look at the Auth code probably more than anyone else. Do you think there's a benefit here in spending some time to abstract the Username/Password, LDAP, and now OIDC into a more generic Provider interface?
  • Are you interested in being added as a maintainer for Mealie?

@cmintey
Copy link
Contributor Author

cmintey commented Dec 18, 2023

Why OIDC over something like SSO via Forward Auth Headers?

Mainly because I have more experience with OAuth and OIDC. From my understanding, Forward Auth Headers are great for adding authentication to an application that otherwise doesn't have authentication. I've also seen that popular providers like Authelia, Authentik and Keycloak offer both Forward Auth and OIDC, so if someone is using one of those providers, it's probably easier to set up auth via OIDC instead.

Finally, because its pretty much "built-in" with nuxt-auth. Obviously I ended up having to create a custom provider because we serve the frontend as a SPA, most of the work is handled by that package, which is great.

If Header Auth is something the community wants, I know there was some great work done in #2040, which I could certainly take a look at also getting over the finish line.

If move to OIDC, should we still support LDAP (long term) i.e. is there any reasons why LDAP users couldn't easily switch to OIDC implementation?

I think it is still worthwhile to support both methods. I can see someone wanting to just have an LDAP server without setting up an authentication provider. Especially if all their self-hosted apps all support LDAP (which many do), there would really be no need. I think as long as LDAP is used and doesn't become a chore to maintain, we should keep it.

I've been looking at https://github.com/markbates/goth for another project as an "auth abstraction". They have this model of a Provider/Session interface and they use that to support multiple authentication models. You've look at the Auth code probably more than anyone else. Do you think there's a benefit here in spending some time to abstract the Username/Password, LDAP, and now OIDC into a more generic Provider interface?

I think it could be worth looking into. I would like to see if the OIDC implementation could be cleaned up a bit still. One thing I dislike is that now, our token verification is different between OIDC vs credential and LDAP. I should have some time next week to look into potential solutions

Are you interested in being added as a maintainer for Mealie?

Maybe. I got a new job about halfway through the year, so I've been much more busy than I was earlier in the year when I was fixing LDAP issues. Let me think on it some more

@hay-kot hay-kot enabled auto-merge (squash) February 18, 2024 23:58
@cmintey
Copy link
Contributor Author

cmintey commented Feb 22, 2024

Hey @hay-kot, sorry for the delay, I was on vacation. I'll get this branch up to date.

I've been using this branch for a few weeks and did find a bug with the JWT not expiring leading to basically never expiring sessions. I should probably fix this bug before merging

@cmintey cmintey marked this pull request as draft February 23, 2024 19:39
@cmintey cmintey marked this pull request as ready for review February 24, 2024 01:11
@cmintey
Copy link
Contributor Author

cmintey commented Feb 24, 2024

Alright, I think I've squashed the bug now. This is ready for a review again

@hay-kot hay-kot self-assigned this Mar 9, 2024
@hay-kot hay-kot removed the post-v1 Merge after v1 stable release label Mar 9, 2024
@cmintey cmintey deleted the oidc branch March 10, 2024 19:27
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