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

Add OpenIddict module. #12084

Merged
merged 39 commits into from
May 6, 2022
Merged

Add OpenIddict module. #12084

merged 39 commits into from
May 6, 2022

Conversation

maliming
Copy link
Member

@maliming maliming commented Mar 25, 2022

https://github.com/abpframework/abp/blob/dev/docs/en/Modules/OpenIddict.md

I can merge these commits into one to beautify the commit.

@maliming maliming added this to the 6.0-preview milestone Mar 25, 2022
@maliming maliming changed the base branch from dev to rel-6.0 March 25, 2022 13:12
@maliming maliming marked this pull request as ready for review April 1, 2022 12:54
@hikalkan hikalkan self-requested a review April 1, 2022 13:01
@codecov
Copy link

codecov bot commented Apr 3, 2022

Codecov Report

❗ No coverage uploaded for pull request base (rel-6.0@793b071). Click here to learn what that means.
The diff coverage is 100.00%.

❗ Current head c7bf5a4 differs from pull request most recent head 407d5db. Consider uploading reports for the commit 407d5db to get more accurate results

@@            Coverage Diff             @@
##             rel-6.0   #12084   +/-   ##
==========================================
  Coverage           ?   48.13%           
==========================================
  Files              ?     3147           
  Lines              ?    94563           
  Branches           ?        0           
==========================================
  Hits               ?    45521           
  Misses             ?    49042           
  Partials           ?        0           
Impacted Files Coverage Δ
...omain/Volo/Abp/Identity/AbpIdentityDomainModule.cs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 793b071...407d5db. Read the comment docs.

@maliming
Copy link
Member Author

maliming commented Apr 5, 2022

DisableAccessTokenEncryption has been removed, I updated the documentation to explain it.

https://github.com/abpframework/abp/blob/Module-OpenIddict/docs/en/Modules/OpenIddict.md#token-encryption

@kevinchalet
Copy link

kevinchalet commented Apr 8, 2022

👋🏻

@hikalkan sent me the link to this PR so I took a (very) brief look 😄

Three things worth noting:

  • For token validation, the OpenIddict validation handler is the recommended option, as it uses a more recent JWT stack and comes with additional security checks that are not implemented by the MSFT JWT bearer handler (e.g token type validation, to ensure identity tokens are not treated as valid access tokens).

  • I see you're trying to localize error messages eventually returned by OpenIddict: I did that in OpenIddict 3.0 beta4 (we even supported Turkish!) and it didn't end well for the reasons explained in this post: https://kevinchalet.com/2020/11/17/introducing-openiddict-3-0-s-first-release-candidate-version/

  • In OpenIddictSupportedLoginModel.cs, you use a dangerous approach that consists in faking an "OpenIddict request" and injecting it in the OpenIddict context to force it to consider the HTTP request as an authorization request. It's not 100% clear to me what this class does but I suspect it's basically a login page that needs to be aware of certain OIDC parameters initially sent as part of the authorization request. If so, you may want to consider a different approach: in your authorization controller, when calling Challenge("scheme of your cookie handler"), add the parameters you need to flow from the authorization endpoint to the login page to AuthenticationProperties and customize the RedirectToLogin event to add them to the query string: https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs#L474. Then, you'll be able to extract them without even using any OpenIddict type in your login stuff.

Cheers.

@kevinchalet
Copy link

In your most recent commit, I see you implemented wildcard support for (post_logout_)redirect_uris. As it goes against the OIDC specification - that requires using simple string comparisons - you need to be extremely careful when implementing that. I see two potential issues:

  • I don't see context.Reject() ever called in your handlers so I don't think invalid redirect_uris will be blocked (I suspect your handlers will basically no-op).
  • When using wildcard support, it seems a single domain is considered valid for all clients.

It's important to get this part right as these two issues can basically ruin the entire security of your authorization server, as it allows stealing authorization codes/access tokens by redirecting the user agent to an untrusted domain (or to the trusted redirect_uri of a different client).

@maliming
Copy link
Member Author

maliming commented Apr 9, 2022

Thank you so much @kevinchalet, You made a great project.

I will make some changes based on your review, thank you for following ABP.

@hikalkan hikalkan merged commit e499f40 into rel-6.0 May 6, 2022
@hikalkan hikalkan deleted the Module-OpenIddict branch May 6, 2022 11:44
@hikalkan hikalkan modified the milestones: 6.0-preview, 5.4-preview May 6, 2022
@beriniwlew
Copy link
Contributor

Amazing work, keep it up!

@hikalkan hikalkan modified the milestones: 5.4-preview, 6.0-preview May 24, 2022
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.

6 participants