Skip to content

Enable UsePkce by default for Google and Facebook auth #45235

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

Merged
merged 19 commits into from
Nov 24, 2022
Merged

Enable UsePkce by default for Google and Facebook auth #45235

merged 19 commits into from
Nov 24, 2022

Conversation

FranklinWhale
Copy link
Contributor

@FranklinWhale FranklinWhale commented Nov 22, 2022

Enable UsePkce by default for Google and Facebook auth

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

  1. Set UsePkce to true in GoogleOptions and FacebookOptions
  2. BuildChallengeUrl in GoogleHandler now calls base.BuildChallengeUrl
  3. Rename AddQueryString to SetQueryParam in GoogleHandler
  4. Refactor ChallengeWillTriggerRedirection in GoogleTests to check query dictionary instead of string matching

Contributes to #4684

@ghost ghost added area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer community-contribution Indicates that the PR has been added by a community member labels Nov 22, 2022
@ghost
Copy link

ghost commented Nov 22, 2022

Thanks for your PR, @FranklinWhale. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@FranklinWhale
Copy link
Contributor Author

@Tratcher Unfortunately, with the new BuildChallengeUrlQuery method, the build cannot be successful. If having a new method is not preferred, it is possible to call QueryHelpers.ParseQuery(base.BuildChallengeUrl(properties, redirectUri)) in GoogleHandler then modify the Dictionary. What do you think?

@FranklinWhale FranklinWhale marked this pull request as ready for review November 22, 2022 16:41
@Tratcher
Copy link
Member

Tratcher commented Nov 22, 2022

Changing a public/protected API signature is not allowed under our breaking changes guidelines. Try the other approach.

@FranklinWhale FranklinWhale marked this pull request as draft November 23, 2022 11:44
@BrennanConroy BrennanConroy marked this pull request as ready for review November 23, 2022 23:44
@FranklinWhale
Copy link
Contributor Author

Added comment and fixed the test.

@Tratcher Tratcher merged commit afb6ec4 into dotnet:main Nov 24, 2022
@Tratcher
Copy link
Member

Thanks

@ghost ghost added this to the 8.0-preview1 milestone Nov 24, 2022
@FranklinWhale FranklinWhale deleted the GoogleFacebookPkce branch November 24, 2022 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants