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

Backport ASP.NET Core PKCE Support to OpenIdConnectAuthenticationHandler #389

Merged
merged 6 commits into from
Dec 10, 2020

Conversation

rzontar
Copy link
Contributor

@rzontar rzontar commented Nov 27, 2020

This adds PKCE Support to the OpenIdConnectAuthenticationHandler from #334

The implementation is a port from ASP.NET Core respecting some of the missing APIs:
https://github.com/dotnet/aspnetcore/blob/8a81194f372fa6fe63ded2d932d379955854d080/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs#L386
and
https://github.com/dotnet/aspnetcore/blob/8a81194f372fa6fe63ded2d932d379955854d080/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs#L1134

I've added some unit test to cover the challenge redirects and tested the implementation using the sandbox and IdentityServer4.

It could be possible to upgrade the IdentityModel.* packages as well. But I'm not aware if this would cause any braking changes.

Backport the ASP.NET Core PKCE implementation to the OWIN OpenIdConnect middleware.
Add basic PKCE related unit test. Port from ASP.NET Core.
@Tratcher
Copy link
Member

Tratcher commented Dec 1, 2020

Thanks for the PR, I should be able to look it over next week.

FYI we don't have any releases scheduled for this product right now, there's no telling when this feature would be released.

We've also temporarily disabled publishing the nightly builds due to an infrastructure change.

@rzontar
Copy link
Contributor Author

rzontar commented Dec 2, 2020

No hurry. We have developed a workaround using the OpenIdConnectAuthenticationNotifications. But there, we have to do some thing twice, like parsing and unprotecting the state parameter.

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very clean PR. I've also verified this locally for IS4 and AAD.

Fix up the defaults and this should be good to go.

Adjust comment since RunAuthorizationCodeReceivedEventAsync only exists in Core.
@Tratcher Tratcher merged commit 1fba529 into aspnet:dev Dec 10, 2020
@Tratcher
Copy link
Member

Thanks

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

Successfully merging this pull request may close these issues.

2 participants