-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[Prototype] Update the OpenID module to use the OpenIddict client #14021
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
Conversation
03addeb to
22dacf6
Compare
7196e60 to
74c7d43
Compare
74c7d43 to
e832b7f
Compare
|
So this depends on #7891, right? |
Technically, we could use the same X.509 certificates generation logic as the server feature but since it's something we want to get rid of once the secrets module is ready, it's likely better to just wait so yeah 😃 |
|
OK then, thank you. I think it's better as a draft, then, for now. |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
e832b7f to
206fa94
Compare
206fa94 to
1ea1213
Compare
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
1ea1213 to
4a1b7a5
Compare
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
3b00f87 to
46462d1
Compare
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
46462d1 to
97b8d82
Compare
c615cfa to
8914f57
Compare
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
8914f57 to
9a61c38
Compare
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
I think we should. I would it its common for one tenant to want to connect to multiple OpenId servers.
Can't we keep the module in tact but add the corresponding registration in the proper module? For example, in the GitHub module we can register using something like this |
We definitely can, but OpenIddict already offers more than 100 provider integrations: while it's extremely unlikely we'll want to support all of them in OC, we'll probably have demand for some of them over time: if we keep the 1 provider = 1 package pattern, it may quickly go out of hand: all these packages have a cost that can't be neglected (e.g increased loading times in VS, longer build times, etc.) |
|
The rule we follow when creating new modules "if the module requires a new dependency, it's likely that it should have it's own module". So if we don't need to install extra packages for each provider, then we can move them into one module added one feature per provider. |
|
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
|
Closing this pull request because it has been stale for very long. If you think this is still relevant, feel free to reopen it. |
This is a very crude PR that replaces the MSFT OIDC handler in
OrchardCore.OpenIdby the OpenIddict client. I deliberately didn't touch the OpenID client recipe to avoid a source/binary breaking change and used multiple mappings to avoid behavior changes.Open questions:
OpenIddict's web providers? If so, how do we architecture things? Do we want to merge the Facebook/Google/GitHub/Entra ID/Twitter modules intoOrchardCore.OpenId?Related discussion: OrchardCoreContrib/OrchardCoreContrib.Modules#90.
Depends on #7891.
/cc @hishamco @MichaelPetrinolis