feat: Update PKCE_REQUIRED to true by default

[dopry]

Assuming PKCE is properly implemented this change should be backwards
compatible with clients that do not support PKCE, see:
https://datatracker.ietf.org/doc/html/rfc7636#section-5

applies to #1122

Description of the Change

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

[dopry]

This is a draft. After reviewing the compatibility section of RFC7636 Proof Key for Code Exchange by OAuth Public Clients. It should be backward compatible, so maybe it would be preferable to remove the PKCE setting from userspace and just set it to true in all cases.

[dopry]

This is a draft. After reviewing the compatibility section of RFC7636 Proof Key for Code Exchange by OAuth Public Clients. It should be backward compatible, so maybe it would be preferable to remove the PKCE setting from userspace and just set it to true in all cases.

I actually looked at OAuthLib and it looks like this is a flag to require PKCE in a way that isn't officially specification compliant (it lacks backward compatibility). The questions that remain in my mind are

1. is the breaking change worth it or should I just update the documentation to better inform users, maybe add some sort of 'Warning' for users who don't set this to true.
2. If we pursue the breaking change, should we follow a deprecation cycle like Django and introduce a deprecation warning for a release?

[n2ygk]

@dopry Have you run the tox tests locally to see that breaks them? I usually start testing with just a single py and dj version:

tox -e py39-dj32

That still takes a while but then you can focus on the specific tests that fail and run them in your IDE if necessary to determine what happened. I'm guessing most of the failures are because the test case does not use PKCE.

Regarding it being a breaking change: That's OK for release 2.0 which will have many breaking changes. I don't think it's necessary to put this through a deprecation cycle.

[n2ygk]

BTW, If you change the test cases to use PKCE, also make a few failing tests where it's required as well as override the setting to have some successful tests where PKCE_REQUIRED=False.

It looks like test_scopes is the only set of tests that fail.

tests/test_scopes.py FFFFFFFF.FF.

So now I can focus just on those tests:

django-oauth-toolkit$ tox -e py39-dj32 tests/test_scopes.py

It appears to only be one error:

E       KeyError: 'code'

Probably because PKCE needs to be provided here:

django-oauth-toolkit/tests/test_scopes.py

Lines 89 to 96 in 2212144

	authcode_data = {
	"client_id": self.application.client_id,
	"state": "random_state_string",
	"scope": "scope1 scope2",
	"redirect_uri": "http://example.org",
	"response_type": "code",
	"allow": True,
	}

I'm pretty sure you need to add these fields and implement a verifier.

{
    # ...
    "code_challenge": CHALLENGE,
    "code_challenge_method": METHOD
}

There's already code that does this in test_authorization_code:

django-oauth-toolkit/tests/test_authorization_code.py

Lines 630 to 661 in 2212144

	def generate_pkce_codes(self, algorithm, length=43):
	"""
	Helper method to generate pkce codes
	"""
	code_verifier = get_random_string(length)
	if algorithm == "S256":
	code_challenge = (
	base64.urlsafe_b64encode(hashlib.sha256(code_verifier.encode()).digest()).decode().rstrip("=")
	)
	else:
	code_challenge = code_verifier
	return code_verifier, code_challenge
	def get_pkce_auth(self, code_challenge, code_challenge_method):
	"""
	Helper method to retrieve a valid authorization code using pkce
	"""
	self.oauth2_settings.PKCE_REQUIRED = True
	authcode_data = {
	"client_id": self.application.client_id,
	"state": "random_state_string",
	"scope": "read write",
	"redirect_uri": "http://example.org",
	"response_type": "code",
	"allow": True,
	"code_challenge": code_challenge,
	"code_challenge_method": code_challenge_method,
	}
	response = self.client.post(reverse("oauth2_provider:authorize"), data=authcode_data)
	query_dict = parse_qs(urlparse(response["Location"]).query)
	return query_dict["code"].pop()

[dopry]

@n2ygk I wasn't asking about the testing. That's easy. My question is about how to handle the breaking change of defaulting PKCE_REQUIRED to true. Do we need a deprecation cycle, or is it cool to make the change and just document it.

[n2ygk]

@n2ygk I wasn't asking about the testing. That's easy. My question is about how to handle the breaking change of defaulting PKCE_REQUIRED to true. Do we need a deprecation cycle, or is it cool to make the change and just document it.

@dopry Read all the way to the bottom where I said:

Regarding it being a breaking change: That's OK for release 2.0 which will have many breaking changes. I don't think it's necessary to put this through a deprecation cycle.

[codecov]

Codecov Report

Merging #1129 (a580130) into master (c79eae2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1129   +/-   ##
=======================================
  Coverage   96.87%   96.87%           
=======================================
  Files          31       31           
  Lines        1790     1790           
=======================================
  Hits         1734     1734           
  Misses         56       56           

Impacted Files	Coverage Δ
oauth2_provider/settings.py	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 c79eae2...a580130. Read the comment docs.

[dopry]

@n2ygk I missed that paragraph. I believe this should be ready to merge now. I'm assuming that PKCE_REQUIRED as a feature is already well covered by the existing suite of tests. I did add a test to verify that the default value is True.

Let me know if there is anything you think I missed.

side note: the test_models is failing for me locally, token clearing doesn't seem to be working properly in my local environment. 1) refresh token creation is failing because it seems to expect the model and not the id of the access token.

[n2ygk]

Looks good. Just a minor change to CHANGELOG.md please