Repository: jazzband/django-oauth-toolkit (language: python)

Issue:
Enable PKCE by default and set RS-256 as the default signing algorithm. 
**Is your feature request related to a problem? Please describe.**

I was setting up authorization for a SPA and had to do quite a bit of digging to figure out how best to configure the application to make it secure.  The various options on the Application form are unclear.  I have quite a bit of experience with OAuth and OIDC but this form confused me as I saw options that were not necessarily applicable to my chosen flows and some that were completely unfamiliar like skip authorization


**Describe the solution you'd like**
- [x] set PKCE_REQUIRED to true by default, WIP: #1129 
- [ ] set default algorithm to RS256 since it is the accepted security best practice. 
- [ ] add help text to Client type, linking it to https://datatracker.ietf.org/doc/html/rfc6749#section-2.1.
- [ ] update 'Algorithm' label to JWT Signing Algorithm, add help text describing the options , https://auth0.com/docs/get-started/applications/signing-algorithms has good descriptions.
- [ ] make Authorization grant type the first input on the application form and vary the available fields depending on the selected grant type. 
- [ ] add help text to skip_authorization linking to the appropriate help docs...




Discussion / hints:
See #1098. I think a lot of this falls under that heading.

Fix patch:
diff --git a/AUTHORS b/AUTHORS
index 962cc7d00..8ec6667e2 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -73,3 +73,4 @@ Eduardo Oliveira
 Andrea Greco
 Dominik George
 David Hill
+Darrel O'Pry
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 148d5e50e..baae70de8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -25,6 +25,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   on using Celery to automate clearing expired tokens.
 
 ### 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](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
diff --git a/docs/settings.rst b/docs/settings.rst
index 0ba12df11..2ac31ccda 100644
--- a/docs/settings.rst
+++ b/docs/settings.rst
@@ -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
diff --git a/oauth2_provider/settings.py b/oauth2_provider/settings.py
index 3b7dea3f8..00a4e631c 100644
--- a/oauth2_provider/settings.py
+++ b/oauth2_provider/settings.py
@@ -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,


Test patch:
diff --git a/tests/presets.py b/tests/presets.py
index fa2d7a34c..6411687a4 100644
--- a/tests/presets.py
+++ b/tests/presets.py
@@ -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"]
diff --git a/tests/settings.py b/tests/settings.py
index d2fbe6a56..27dcfe9a3 100644
--- a/tests/settings.py
+++ b/tests/settings.py
@@ -159,3 +159,4 @@
 
 CLEAR_EXPIRED_TOKENS_BATCH_SIZE = 1
 CLEAR_EXPIRED_TOKENS_BATCH_INTERVAL = 0
+PKCE_REQUIRED = False
diff --git a/tests/test_authorization_code.py b/tests/test_authorization_code.py
index 25447b9dd..8bface719 100644
--- a/tests/test_authorization_code.py
+++ b/tests/test_authorization_code.py
@@ -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",
@@ -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"),
@@ -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()
@@ -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 = {
@@ -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",
@@ -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,
@@ -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,
@@ -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",
@@ -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",
@@ -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,
@@ -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,
@@ -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,
@@ -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",
@@ -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",
@@ -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",
@@ -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",
diff --git a/tests/test_hybrid.py b/tests/test_hybrid.py
index 3f4048698..2e85b05b1 100644
--- a/tests/test_hybrid.py
+++ b/tests/test_hybrid.py
@@ -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(
diff --git a/tests/test_scopes.py b/tests/test_scopes.py
index 39601ed3b..548cc060c 100644
--- a/tests/test_scopes.py
+++ b/tests/test_scopes.py
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/tests/test_settings.py b/tests/test_settings.py
index 52bdafe03..f9f540339 100644
--- a/tests/test_settings.py
+++ b/tests/test_settings.py
@@ -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