Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

FEAT: Open ID Connect authentication#4010

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
oechsler wants to merge30 commits intoNginxProxyManager:develop
base:develop
Choose a base branch
Loading
fromoechsler:FEAT/open-id-connect-authentication

Conversation

oechsler
Copy link

Continuation of:#2630

  • Mergedevelop into the feature branch, resolving conflicts
  • Fix issues with the settings list template, where theoidc-config would have the text of thedefault-site

Tested with Authentik 2024.8.2

P.S.: I hope we can get this finally merged! 🤣

CrazyWolf13, mrhackcz, zzzz0317, knelldev, 1broseidon, lusu007, acidRain-burns, vladwing, rijnhard, TheGameProfi, and 12 more reacted with heart emoji1broseidon, lusu007, TheGameProfi, grevian, apocaliss92, and erathaowl reacted with rocket emoji
marekfuland others added15 commitsFebruary 24, 2023 15:15
* 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 existsNote: Only Open ID Connect Discovery is supported which most modernIdentity Providers offer.Tested with Authentik 2023.2.2 and Keycloak 18.0.2
@oechsleroechsler changed the titleFEAT: Open ID Cconnect authenticationFEAT: Open ID Connect authenticationSep 19, 2024
@CrazyWolf13
Copy link

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.

oechsler reacted with thumbs up emoji

@Shocktrooper
Copy link

@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

CrazyWolf13 and 1broseidon reacted with thumbs up emoji

@zanda8893
Copy link

Any update on this? I would love to see it merged

@jc21
Copy link
Member

There are conflicts, and CI won't build you a docker image until they're resolved

@oechsler
Copy link
Author

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.

CrazyWolf13 reacted with heart emoji

@jcebrianalonso
Copy link

I'm glad this PR will be finally merged! Thanks a lot for the effort of all the people involved@jc21@oechsler@Shocktrooper

@jc21
Copy link
Member

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

IxPrumxI and jcebrianalonso reacted with eyes emoji

@@ -21,6 +21,8 @@
"moment": "^2.29.4",
"mysql2": "^3.11.1",
"node-rsa": "^1.0.8",
"nodemon": "^2.0.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nodemon is already indevDependencies - adding it here is unnecessary and will add bloat to the final image

as it is already in devDependencies
@oechsler
Copy link
Author

oechsler commentedOct 31, 2024
edited
Loading

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

@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 😉

@svenwanzenried
Copy link

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.

CrazyWolf13 reacted with thumbs up emoji

@chutch1122
Copy link

chutch1122 commentedDec 10, 2024
edited
Loading

I made some changes to@oechsler 's branch to address some of the requested items (PRhere).

  • Cypress automated tests for updating the OIDC config
  • Documentation for setting up SSO with OIDC

I also made the following UI/UX changes:

  • A more user-friendly error message for when a user does not exist in NPM and they are attempting to sign in via an IdP
  • Fixing a typo in the "hint" description so that the name of "Redirect URL" matches the field label in the modal.
acidRain-burns, oechsler, and svenwanzenried reacted with heart emoji

@oechsler
Copy link
Author

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!

svenwanzenried and ticapix reacted with heart emoji

@chutch1122
Copy link

chutch1122 commentedDec 11, 2024
edited
Loading

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.
Edit 2: Figured it out. Some things changed with error messages since this branch was created. Merging indevelop and updating some of the test expectations seems to do the trick.

@nginxproxymanagerci
Copy link

Docker Image for build 4 is available on
DockerHub
asnginxproxymanager/nginx-proxy-manager-dev:pr-4010

Note: ensure you backup your NPM instance before testing this image! Especially if there are database changes
Note: this is a different docker image namespace than the official image

@chutch1122
Copy link

chutch1122 commentedDec 13, 2024
edited
Loading

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

@jc21
Copy link
Member

I've recently added OIDC support to thev3 branch, and that also adds a locally running Authentik server pre-configured.

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 thev3 test I've written.

@jc21
Copy link
Member

Thedevelop branch now has postgres support and Authentik setup in both CI and dev.

To bring up the dev environment:

./scripts/start-dev# and remove it after you're done./scripts/destroy-dev

Then access the web hosts with:

Authentik notes:

  • log in withakadmin:admin
  • Support for LDAP and Oauth/OpenID is ready to go
  • Login with Oauth/OpenID usingcypress:fqXBfUYqHvYqiwBHWW7f

See the following cypress tests, they are skipped currently however

to run cypress locally:

cd testsyarn installyarn cypress:headless --spec cypress/e2e/api/Ldap.cy.js
Biohive, francescocaponio, and alexsalex reacted with thumbs up emoji

@Shocktrooper
Copy link

@jc21 Any updates?

@LuKePicci
Copy link

Would be nice to see this merged. Hope you manage to complete testing this asap

@adamoutler
Copy link

Looking forward to seeing this in the primary docker soon.

@gdeeble
Copy link

Just wondering if any progress has been made with this? I have Authentik setup myself and would be happy to test if additional testing is necessary.

@WirtsLegs
Copy link

Would love to see this merged, looking forward to working with my keycloak instance without needing some kind of auth proxy middleware

@Billos
Copy link

Billos commentedMar 13, 2025
edited
Loading

Sorry to add another useless message, but please, when you see that the last 4 messages are nothing but people being impatient about the feature, maybe you could just think twice before posting the 5th message about being impatient about this feature.

rachelf42, iRaven4522, CrazyWolf13, Kogoro, and Vale54321 reacted with thumbs up emoji

@alexsalex
Copy link

alexsalex commentedMar 28, 2025
edited
Loading

Could somebody explained me why this PR can’t be merged from September 2024? Is any unresolved issues? Or are we waiting NPMv3? Please, let us know!

@adamoutler
Copy link

Could somebody explained me why this PR can’t be merged from September 2024? Is any unresolved issues? Or we’re waiting NPMv3? Please, let us know!

As far as I can see in the comments here, this is running in Dev Branch. The developer wants to ensure high quality before unleashing upon the general population. There are some major changes coming.

erathaowl reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jc21jc21Awaiting requested review from jc21

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

15 participants
@oechsler@CrazyWolf13@Shocktrooper@zanda8893@jc21@jcebrianalonso@chutch1122@svenwanzenried@LuKePicci@adamoutler@gdeeble@WirtsLegs@Billos@alexsalex@marekful

[8]ページ先頭

©2009-2025 Movatter.jp