-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
FEAT: Open ID Connect authentication #4010
base: develop
Are you sure you want to change the base?
FEAT: Open ID Connect authentication #4010
Conversation
* add `oidc-config` setting allowing an admin user to configure parameters * modify login page to show another button when oidc is configured * add dependency `openid-client` `v5.4.0` * add backend route to process "OAuth2 Authorization Code" flow initialisation * add backend route to process callback of above flow * sign in the authenticated user with internal jwt token if internal user with email matching the one retrieved from oauth claims exists Note: Only Open ID Connect Discovery is supported which most modern Identity Providers offer. Tested with Authentik 2023.2.2 and Keycloak 18.0.2
…ct-authentication
It's probably worth adding those PRs: Seems like there are multiple PRs implementing the same feature, it's probably the best to wait for @jc21 to decide which one to merge. |
@CrazyWolf13 This is a continuation of the abandoned #2630 PR and #3952 was done as part of this PR it looks like. We just need @jc21 to take a look and merge if everything is good |
Any update on this? I would love to see it merged |
There are conflicts, and CI won't build you a docker image until they're resolved |
Thanks for clarifying, I will resolve them later today. |
I'm glad this PR will be finally merged! Thanks a lot for the effort of all the people involved @jc21 @oechsler @Shocktrooper |
Yes many thanks for the contribution but don't count your chickens just yet. This is a big PR and it will need thorough testing both with OIDC and without. I'd feel happier if there was some cypress tests for both scenarios as well. Besides Authentik, can we get this tested with other providers too? Also some documentation will need to be added around this |
backend/package.json
Outdated
@@ -21,6 +21,8 @@ | |||
"moment": "^2.29.4", | |||
"mysql2": "^3.11.1", | |||
"node-rsa": "^1.0.8", | |||
"nodemon": "^2.0.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodemon
is already in devDependencies
- adding it here is unnecessary and will add bloat to the final image
as it is already in devDependencies
@jc21 - Thank you very much for the review and your feedback on this topic! Could you possibly elaborate a bit more on what the tests should specifically cover? I don’t have much experience with the Cypress framework, but I’m more than willing to invest the time needed to bring this PR to completion. As for testing with other OIDC providers, I’d leave that to others in this thread for now, as I currently only have Authentik running. However, with the new Docker test image, it should be straightforward to set up an instance. I’m also happy to take on the documentation. Given that OIDC isn’t always the most straightforward topic, I agree it would be beneficial to provide clear guidance. Should we add a dedicated section to the documentation for this, or integrate it into an existing section? - Nonetheless I would also be happy if somebody else could lend a hand 😉 |
I just tested this with Auth0 by Okta and can confirm that it works. You need to set up an Application as a "Single Page Application". Auth0 also requires that your Callback URL (aka "Redirect URL") be secured with HTTPS. Then, the config maps as follows:
I also tested it with my own Authentik installation and can confirm that it works there as well. |
Just tested this with Authelia and can confirm that it works. This is my snippet from authelia configuration.yml:
As long as the email in Authelia matches the E-Mail in NPM, it works. However, this is not the cleanest solution, as the email is not guaranteed to be unique in the OIDC standard. Howerver, this is something that I came across ofthen when implementing OIDC services and doesn't bother me much in an home lab environement (which is think is the main target of NPM, anyway). Some Clients use preferred_name some use the email, and some the ID. It's not very consistent across the board... |
One thing to note @svenwanzenried is that unless NPM intends to support multiple concurrent OIDC providers email should be fine to match. I believe that NPM would need to rework users to a large degree if it started to allow multiple users to all use the same email seeing as how email is currently used in the tool |
that may be so. and as i said, it's probably fine. But if you would define multiple users in Authelia with same email but different username (e.g. Sven and SvenAdmin) which is absolutely possible, then you'd have a conflict on login. You'd probably just end up in the same Account in NPM.. Which.. considering you have control over the same email address may or may not be an issue. |
I made some changes to @oechsler 's branch to address some of the requested items (PR here).
I also made the following UI/UX changes:
|
Hey @chutch1122, and thank you also, @svenwanzenried, for testing with other providers, dedicating your time to implementing the Cypress tests, and writing the documentation. I’ll take a look at the changes over the next few days and merge them into the PR branch as soon as possible. I’m really happy to see the momentum behind this feature —great work! |
…tion Add Cypress tests and documentation
I also added some additional UI-based end to end tests. These tests update the OIDC config to be enabled/disabled and ensure that users can still log in. I'd like to add some additional tests around logging in with a "dummy" OIDC provider, but this should provide some good coverage and peace of mind that users won't get locked out. Edit: Not sure why the build is failing. The tests ran on my machine. |
…nect-authentication
…tion Merge upstream and update test expectations
Docker Image for build 4 is available on Note: ensure you backup your NPM instance before testing this image! Especially if there are database changes |
@jc21 Do you want E2E tests with an actual mock OIDC provider to be included before merging this in? Other than that, is there anything else you'd like added? Otherwise, I think it's ready for review again and includes your requests from above. |
I've recently added OIDC support to the I'd suggest waiting for me to merge one of the outstanding postgres PR's which I can then add the authentic db to that, then the cypress e2e tests can be improved from there. Here's the v3 test I've written. |
The To bring up the dev environment:
Then access the web hosts with:
Authentik notes:
See the following cypress tests, they are skipped currently however to run cypress locally:
|
Continuation of: #2630
develop
into the feature branch, resolving conflictsoidc-config
would have the text of thedefault-site
Tested with Authentik 2024.8.2
P.S.: I hope we can get this finally merged! 🤣