Skip to content

Conversation

@kevinchalet
Copy link
Member

@kevinchalet kevinchalet commented Jul 21, 2023

This is a very crude PR that replaces the MSFT OIDC handler in OrchardCore.OpenId by 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:

  • Do we want to support configuring multiple client registrations (to connect with multiple OpenID Connect servers)? It's not something currently supported by the OpenID client feature and would likely require changes to the UI/recipes.
  • Do we want to use OpenIddict's web providers? If so, how do we architecture things? Do we want to merge the Facebook/Google/GitHub/Entra ID/Twitter modules into OrchardCore.OpenId?

Related discussion: OrchardCoreContrib/OrchardCoreContrib.Modules#90.

Depends on #7891.

/cc @hishamco @MichaelPetrinolis

@Piedone
Copy link
Member

Piedone commented Jan 18, 2024

So this depends on #7891, right?

@kevinchalet
Copy link
Member Author

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 😃

@Piedone
Copy link
Member

Piedone commented Jan 18, 2024

OK then, thank you. I think it's better as a draft, then, for now.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2024

This pull request has merge conflicts. Please resolve those before requesting a review.

@github-actions
Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@github-actions
Copy link
Contributor

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.

@github-actions
Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@github-actions
Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot added the stale label Nov 16, 2024
@kevinchalet kevinchalet removed the stale label Nov 16, 2024
@kevinchalet kevinchalet force-pushed the openiddict_client branch 2 times, most recently from c615cfa to 8914f57 Compare December 27, 2024 20:05
@github-actions
Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot added the stale label Mar 30, 2025
@github-actions
Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@MikeAlhayek
Copy link
Member

Do we want to support configuring multiple client registrations (to connect with multiple OpenID Connect servers)? It's not something currently supported by the OpenID client feature and would likely require changes to the UI/recipes.

I think we should. I would it its common for one tenant to want to connect to multiple OpenId servers.

Do we want to use OpenIddict's web providers? If so, how do we architecture things? Do we want to merge the Facebook/Google/GitHub/Entra ID/Twitter modules into OrchardCore.OpenId

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

            options.UseWebProviders()
                   .AddGitHub(options =>
                   {
                       // these would be coming from the IOptions<GitHubAuthenticationSettings>
                       options.SetClientId("")
                              .SetClientSecret("")
                              .SetRedirectUri("");
                   })

@kevinchalet
Copy link
Member Author

kevinchalet commented Apr 25, 2025

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

@MikeAlhayek
Copy link
Member

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.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2025

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.

@github-actions github-actions bot added the stale label Aug 9, 2025
@github-actions
Copy link
Contributor

Closing this pull request because it has been stale for very long. If you think this is still relevant, feel free to reopen it.

@github-actions github-actions bot closed this Aug 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants