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

OpenId Module based on OpenIddict Client #90

Open
hishamco opened this issue Mar 28, 2023 · 9 comments
Open

OpenId Module based on OpenIddict Client #90

hishamco opened this issue Mar 28, 2023 · 9 comments

Comments

@hishamco
Copy link
Member

As mentioned here we could create a new OpenId module based on OpenIddict client

@kevinchalet while you're our OpenId guru, is there any advantages to use OpenIddict Client? Any features we could added if it's possible. Please your suggestion coz you might drive this by your thoughts & suggestions

@kevinchalet
Copy link

@kevinchalet while you're our OpenId guru, is there any advantages to use OpenIddict Client? Any features we could added if it's possible. Please your suggestion coz you might drive this by your thoughts & suggestions

Like the MSFT OIDC handler, the new OpenIddict client supports OIDC, but it also supports OAuth 2.0-only servers, which allows integrating with tons of services that can't be used with Microsoft's OIDC handler.

It also supports things like token refreshing (grant_type=refresh_token), has more security features, better interoperability (and can be used not only in ASP.NET Core apps, but also in legacy ASP.NET 4.6.1 and Windows/Linux desktop apps, but it's not something OrchardCore would benefit from directly).

That said, I'm not so sure developing a new module from scratch would make a ton of sense. If we think it's worth having something based on the OpenIddict client, we should likely just update OrchardCore.OpenId (and OrchardCore.Google and OrchardCore.Twitter if we're feeling brave) to use it in lieu of the MSFT OIDC handler. This would avoid having to maintain two separate code bases.

@hishamco
Copy link
Member Author

Thanks for your response, updating the OC modules needs to be approved by the team not only me & you :). My aim was either to copy what we did in OC and use the OpenIddict client or create a new module on top of OC.OpenId

Frankly I didn't work with this module before, so your feedback and guidance is very important here

@kevinchalet
Copy link

Thanks for your response, updating the OC modules needs to be approved by the team not only me & you :)

Sure, but if we think it's worth investing on that and come with a list of pros and cons that tends to confirm it would be a good idea, I don't see why it couldn't happen 😃
After all, whether the OpenID module uses the MSFT client or the OpenIddict client is mostly an implementation detail we're free to change if we all agree it's the right thing to do.

My aim was either to copy what we did in OC and use the OpenIddict client or create a new module on top of OC.OpenId

It's certainly doable, but it will basically be a completely separate module that you'll need to maintain. Also, since it won't be part of OC itself, it will have less visibility and will likely receive less contributions (and hem, the "official" OpenID OC module already doesn't receive a lot of love from external contributors 😅)

To be clear, I'm not trying to deter you from working on such a module, but it may be a lot of work. Specially if you want to support all the OAuth 2.0/OpenID Connect services the OpenIddict client supports (the complete list is here: https://github.com/openiddict/openiddict-core/blob/dev/src/OpenIddict.Client.WebIntegration/OpenIddictClientWebIntegrationProviders.xml).

I'd love to hear what @MichaelPetrinolis (who contributed the OpenID client feature), @Skrypt, @jtkech and @sebastienros think about that 😃

@Skrypt
Copy link

Skrypt commented Mar 30, 2023

If it is a breaking change then it needs to be a new module. Then, we could have both.

@hishamco
Copy link
Member Author

I don't think having both modules on the same repo is a good idea, either we replace the MSFT client or create a community module

@MichaelPetrinolis
Copy link

I agree with @kevinchalet, if we decide to implement clients with OpenIdDict, we should replace the Microsoft implementation maintaining the compatibility at OrchardCore module level, in order to maintain one codebase.

I took a look at Kevin's announcement about The OpenIdDict client, and he states that the handler is registered as a single scheme. So we could introduce a new module that can handle and configure all the available providers. Then we could replace the implemented features (OpenId, Google and Twitter) to use that module services for their implementation. We should also add special handling in the account Controller for the OpenIdDict scheme (what providers are configured etc.)

@kevinchalet
Copy link

I took a look at Kevin's aspnet-contrib/AspNet.Security.OAuth.Providers#694 about The OpenIdDict client, and he states that the handler is registered as a single scheme. So we could introduce a new module that can handle and configure all the available providers. Then we could replace the implemented features (OpenId, Google and Twitter) to use that module services for their implementation. We should also add special handling in the account Controller for the OpenIdDict scheme (what providers are configured etc.)

OpenIddict 4.7 will introduce a new built-in authentication scheme forwarding mechanism that should make that unnecessary: openiddict/openiddict-core#1839

I started working on prototype of the OpenID client feature using the OpenIddict client and it's really promising 😃

@hishamco
Copy link
Member Author

Thanks for your efforts @kevinchalet

@kevinchalet
Copy link

@hishamco you're welcome 😄

I opened OrchardCMS/OrchardCore#14021: feel free to give it a try and torture it 🤣

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

No branches or pull requests

4 participants