Skip to content

Conversation

@skarzi
Copy link
Contributor

@skarzi skarzi commented Nov 11, 2020

Hi 👋

first of all thanks for developing and maintaining a great project!

Description of the Change

Pass code_challenge and code_challenge_method from query string to AuthorizationView form in get().
Without this, it was impossible to use authorization code grant flow with GET, because code_challenge and code_challenge_method data were never passed to form, so they weren't in form.cleaned_data, which causes creating Grant with always empty code_challenge and code_challenge_method.

This issue was quite hard bug to discover because there are already few tests for authorization code flow pkce, however, they weren't checking form rendering in GET request, but only response.status_code, I have added asserts for these 2 values, please look at the changes in test_public_pkce_plain_authorize_get and test_public_pkce_S256_authorize_get tests in test_authorization_code.py.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@skarzi skarzi changed the title Fix/views/base/pass pkce fields to form pass PKCE fields to AuthorizationView form Nov 11, 2020
@coveralls
Copy link

coveralls commented Nov 11, 2020

Pull Request Test Coverage Report for Build 1494

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 94.821%

Totals Coverage Status
Change from base Build 1485: 0.02%
Covered Lines: 1300
Relevant Lines: 1371

💛 - Coveralls

@MattBlack85
Copy link
Contributor

looks good @skarzi , checked the tests locally and they're ok, thanks!

@MattBlack85 MattBlack85 merged commit a3c085e into django-oauth:master Nov 12, 2020
@skarzi skarzi deleted the fix/views/base/pass-PKCE-fields-to-form branch November 12, 2020 09:52
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.

4 participants