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.

Sign up for GitHub

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

Jump to bottom

[Prototype] Update the OpenID module to use the OpenIddict client #14021

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[Prototype] Update the OpenID module to use the OpenIddict client #14021

wants to merge 1 commit into from
+350 −85

Conversation

kevinchalet
Copy link
Member

@kevinchalet kevinchalet commented Jul 21, 2023 •
edited by Piedone
Loading

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

@kevinchalet kevinchalet added this to the backlog milestone Jul 21, 2023
@kevinchalet kevinchalet self-assigned this Jul 21, 2023
@kevinchalet kevinchalet mentioned this pull request Jul 21, 2023
Open
kevinchalet
kevinchalet commented Aug 9, 2023
View reviewed changes
NuGet.config Outdated Show resolved Hide resolved
@kevinchalet kevinchalet mentioned this pull request Sep 12, 2023
Merged
@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.

@Piedone Piedone mentioned this pull request Jan 18, 2024
Closed
@github-actions 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.

@Piedone Piedone marked this pull request as draft April 21, 2024 19:13
@kevinchalet kevinchalet mentioned this pull request Apr 23, 2024
Closed
@github-actions GitHub Actions
Copy link
Contributor

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

@github-actions 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 Jul 23, 2024
@kevinchalet kevinchalet removed the stale label Jul 23, 2024
@kevinchalet
Copy link
Member Author

Rebased on top of main.

@github-actions GitHub Actions
Copy link
Contributor

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

@github-actions GitHub Actions
Copy link
Contributor

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

@github-actions GitHub Actions
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@kevinchalet kevinchalet

Labels
don't merge merge conflict OpenId
Projects
None yet
Milestone
backlog
Development

Successfully merging this pull request may close these issues.
2 participants
@kevinchalet @Piedone