Skip to content

Commit

Permalink
feat: default PKCE_REQUIRED to True
Browse files Browse the repository at this point in the history
BREAKING CHANGE: set to False to maintain legacy behavior
  • Loading branch information
n2ygk authored and dopry committed Mar 28, 2022
1 parent 2212144 commit 596a831
Show file tree
Hide file tree
Showing 10 changed files with 41 additions and 16 deletions.
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,4 @@ Eduardo Oliveira
Andrea Greco
Dominik George
David Hill
Darrel O'Pry
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
This completes the view to provide all the REQUIRED and RECOMMENDED [OpenID Provider Metadata](https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata).

### Changed
* #1129 (**Breaking**) Changed default value of PKCE_REQUIRED to True. This is a **breaking change**. Clients without
PKCE enabled will fail to authenticate. This breaks with [section 5 of RFC7636](https://datatracker.ietf.org/doc/html/rfc7636)
in favor of the [OAuth2 Security Best Practices for Authorization Code Grants](`Authorization Code Grant <https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#section-2.1.>`_ )
If you want to retain the pre-2.x behavior, set `PKCE_REQUIRED = False ` in your settings.py

* #1093 (**Breaking**) Changed to implement [hashed](https://docs.djangoproject.com/en/stable/topics/auth/passwords/)
client_secret values. This is a **breaking change** that will migrate all your existing
cleartext `application.client_secret` values to be hashed with Django's default password hashing algorithm
Expand Down
16 changes: 14 additions & 2 deletions docs/settings.rst
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,21 @@ will be used.

PKCE_REQUIRED
~~~~~~~~~~~~~
Default: ``False``
Default: ``True``

Can be either a bool or a callable that takes a client id and returns a bool.

Whether or not `Proof Key for Code Exchange <https://oauth.net/2/pkce/>`_ is required.

According to `OAuth 2.0 Security Best Current Practice <https://oauth.net/2/oauth-best-practice/>`_ related to the
`Authorization Code Grant <https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#section-2.1.>`_

- Public clients MUST use PKCE `RFC7636 <https://datatracker.ietf.org/doc/html/rfc7636>`_
- For confidential clients, the use of PKCE `RFC7636 <https://datatracker.ietf.org/doc/html/rfc7636>`_ is RECOMMENDED.




Whether or not PKCE is required. Can be either a bool or a callable that takes a client id and returns a bool.


OIDC_RSA_PRIVATE_KEY
Expand Down
2 changes: 1 addition & 1 deletion oauth2_provider/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@
"RESOURCE_SERVER_INTROSPECTION_CREDENTIALS": None,
"RESOURCE_SERVER_TOKEN_CACHING_SECONDS": 36000,
# Whether or not PKCE is required
"PKCE_REQUIRED": False,
"PKCE_REQUIRED": True,
# Whether to re-create OAuthlibCore on every request.
# Should only be required in testing.
"ALWAYS_RELOAD_OAUTHLIB_CORE": False,
Expand Down
1 change: 1 addition & 0 deletions tests/presets.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"openid": "OpenID connect",
},
"DEFAULT_SCOPES": ["read", "write"],
"PKCE_REQUIRED": False,
}
OIDC_SETTINGS_RO = deepcopy(OIDC_SETTINGS_RW)
OIDC_SETTINGS_RO["DEFAULT_SCOPES"] = ["read"]
Expand Down
1 change: 1 addition & 0 deletions tests/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,3 +159,4 @@

CLEAR_EXPIRED_TOKENS_BATCH_SIZE = 1
CLEAR_EXPIRED_TOKENS_BATCH_INTERVAL = 0
PKCE_REQUIRED = False
17 changes: 5 additions & 12 deletions tests/test_authorization_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def setUp(self):
self.dev_user = UserModel.objects.create_user("dev_user", "dev@example.com", "123456")

self.oauth2_settings.ALLOWED_REDIRECT_URI_SCHEMES = ["http", "custom-scheme"]
self.oauth2_settings.PKCE_REQUIRED = False

self.application = Application.objects.create(
name="Test Application",
Expand All @@ -73,6 +74,7 @@ class TestRegressionIssue315(BaseTest):
"""

def test_request_is_not_overwritten(self):
self.oauth2_settings.PKCE_REQUIRED = False
self.client.login(username="test_user", password="123456")
response = self.client.get(
reverse("oauth2_provider:authorize"),
Expand All @@ -94,6 +96,7 @@ def test_skip_authorization_completely(self):
"""
If application.skip_authorization = True, should skip the authorization page.
"""
self.oauth2_settings.PKCE_REQUIRED = False
self.client.login(username="test_user", password="123456")
self.application.skip_authorization = True
self.application.save()
Expand Down Expand Up @@ -132,6 +135,7 @@ def test_pre_auth_valid_client(self):
"""
Test response for a valid client_id with response_type: code
"""
self.oauth2_settings.PKCE_REQUIRED = False
self.client.login(username="test_user", password="123456")

query_data = {
Expand Down Expand Up @@ -644,7 +648,6 @@ 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",
Expand Down Expand Up @@ -1115,7 +1118,6 @@ def test_public_pkce_S256_authorize_get(self):
self.application.client_type = Application.CLIENT_PUBLIC
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("S256")
self.oauth2_settings.PKCE_REQUIRED = True

query_data = {
"client_id": self.application.client_id,
Expand Down Expand Up @@ -1143,7 +1145,6 @@ def test_public_pkce_plain_authorize_get(self):
self.application.client_type = Application.CLIENT_PUBLIC
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("plain")
self.oauth2_settings.PKCE_REQUIRED = True

query_data = {
"client_id": self.application.client_id,
Expand Down Expand Up @@ -1171,7 +1172,6 @@ def test_public_pkce_S256(self):
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("S256")
authorization_code = self.get_pkce_auth(code_challenge, "S256")
self.oauth2_settings.PKCE_REQUIRED = True

token_request_data = {
"grant_type": "authorization_code",
Expand Down Expand Up @@ -1200,7 +1200,6 @@ def test_public_pkce_plain(self):
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("plain")
authorization_code = self.get_pkce_auth(code_challenge, "plain")
self.oauth2_settings.PKCE_REQUIRED = True

token_request_data = {
"grant_type": "authorization_code",
Expand Down Expand Up @@ -1228,7 +1227,6 @@ def test_public_pkce_invalid_algorithm(self):
self.application.client_type = Application.CLIENT_PUBLIC
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("invalid")
self.oauth2_settings.PKCE_REQUIRED = True

query_data = {
"client_id": self.application.client_id,
Expand All @@ -1250,13 +1248,13 @@ def test_public_pkce_missing_code_challenge(self):
Request an access token using client_type: public
and PKCE enabled but with the code_challenge missing
"""
self.oauth2_settings.PKCE_REQUIRED = True
self.client.login(username="test_user", password="123456")

self.application.client_type = Application.CLIENT_PUBLIC
self.application.skip_authorization = True
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("S256")
self.oauth2_settings.PKCE_REQUIRED = True

query_data = {
"client_id": self.application.client_id,
Expand All @@ -1282,7 +1280,6 @@ def test_public_pkce_missing_code_challenge_method(self):
self.application.client_type = Application.CLIENT_PUBLIC
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("S256")
self.oauth2_settings.PKCE_REQUIRED = True

query_data = {
"client_id": self.application.client_id,
Expand All @@ -1308,7 +1305,6 @@ def test_public_pkce_S256_invalid_code_verifier(self):
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("S256")
authorization_code = self.get_pkce_auth(code_challenge, "S256")
self.oauth2_settings.PKCE_REQUIRED = True

token_request_data = {
"grant_type": "authorization_code",
Expand All @@ -1332,7 +1328,6 @@ def test_public_pkce_plain_invalid_code_verifier(self):
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("plain")
authorization_code = self.get_pkce_auth(code_challenge, "plain")
self.oauth2_settings.PKCE_REQUIRED = True

token_request_data = {
"grant_type": "authorization_code",
Expand All @@ -1356,7 +1351,6 @@ def test_public_pkce_S256_missing_code_verifier(self):
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("S256")
authorization_code = self.get_pkce_auth(code_challenge, "S256")
self.oauth2_settings.PKCE_REQUIRED = True

token_request_data = {
"grant_type": "authorization_code",
Expand All @@ -1379,7 +1373,6 @@ def test_public_pkce_plain_missing_code_verifier(self):
self.application.save()
code_verifier, code_challenge = self.generate_pkce_codes("plain")
authorization_code = self.get_pkce_auth(code_challenge, "plain")
self.oauth2_settings.PKCE_REQUIRED = True

token_request_data = {
"grant_type": "authorization_code",
Expand Down
2 changes: 1 addition & 1 deletion tests/test_hybrid.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def setUp(self):
self.factory = RequestFactory()
self.hy_test_user = UserModel.objects.create_user("hy_test_user", "test_hy@example.com", "123456")
self.hy_dev_user = UserModel.objects.create_user("hy_dev_user", "dev_hy@example.com", "123456")

self.oauth2_settings.PKCE_REQUIRED = False
self.oauth2_settings.ALLOWED_REDIRECT_URI_SCHEMES = ["http", "custom-scheme"]

self.application = Application(
Expand Down
7 changes: 7 additions & 0 deletions tests/test_scopes.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ def test_scopes_saved_in_grant(self):
"""
Test scopes are properly saved in grant
"""
self.oauth2_settings.PKCE_REQUIRED = False
self.client.login(username="test_user", password="123456")

# retrieve a valid authorization code
Expand All @@ -105,6 +106,7 @@ def test_scopes_save_in_access_token(self):
"""
Test scopes are properly saved in access token
"""
self.oauth2_settings.PKCE_REQUIRED = False
self.client.login(username="test_user", password="123456")

# retrieve a valid authorization code
Expand Down Expand Up @@ -141,6 +143,7 @@ def test_scopes_protection_valid(self):
"""
Test access to a scope protected resource with correct scopes provided
"""
self.oauth2_settings.PKCE_REQUIRED = False
self.client.login(username="test_user", password="123456")

# retrieve a valid authorization code
Expand Down Expand Up @@ -183,6 +186,7 @@ def test_scopes_protection_fail(self):
"""
Test access to a scope protected resource with wrong scopes provided
"""
self.oauth2_settings.PKCE_REQUIRED = False
self.client.login(username="test_user", password="123456")

# retrieve a valid authorization code
Expand Down Expand Up @@ -225,6 +229,7 @@ def test_multi_scope_fail(self):
"""
Test access to a multi-scope protected resource with wrong scopes provided
"""
self.oauth2_settings.PKCE_REQUIRED = False
self.client.login(username="test_user", password="123456")

# retrieve a valid authorization code
Expand Down Expand Up @@ -267,6 +272,7 @@ def test_multi_scope_valid(self):
"""
Test access to a multi-scope protected resource with correct scopes provided
"""
self.oauth2_settings.PKCE_REQUIRED = False
self.client.login(username="test_user", password="123456")

# retrieve a valid authorization code
Expand Down Expand Up @@ -308,6 +314,7 @@ def test_multi_scope_valid(self):

class TestReadWriteScope(BaseTest):
def get_access_token(self, scopes):
self.oauth2_settings.PKCE_REQUIRED = False
self.client.login(username="test_user", password="123456")

# retrieve a valid authorization code
Expand Down
5 changes: 5 additions & 0 deletions tests/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,8 @@ def test_generating_iss_endpoint_type_error(oauth2_settings):
with pytest.raises(TypeError) as exc:
oauth2_settings.oidc_issuer(None)
assert str(exc.value) == "request must be a django or oauthlib request: got None"


def test_pkce_required_is_default():
settings = OAuth2ProviderSettings()
assert settings.PKCE_REQUIRED is True

0 comments on commit 596a831

Please sign in to comment.