Revert client secret hash #1020 (#1082)

* Revert "Add migration that alters client_secret to ClientSecretField. (#1075)"

This reverts commit 58f4f5f.

* revert 250120d

* bad merge

Author: pkarman

AUTHORS

@@ -64,6 +64,5 @@ Jadiel Teófilo
 pySilver
 Łukasz Skarżyński
 Shaheed Haque
-Peter Karman
 Vinay Karanam
 Eduardo Oliveira

CHANGELOG.md

@@ -19,7 +19,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 * #651 Batch expired token deletions in `cleartokens` management command
 * Added pt-BR translations.
-* #729 Add support for [hashed client_secret values](https://django-oauth-toolkit.readthedocs.io/en/latest/settings.html#client-secret-hasher).
 
 ### Fixed
 * #1012 Return status for introspecting a nonexistent token from 401 to the correct value of 200 per [RFC 7662](https://datatracker.ietf.org/doc/html/rfc7662#section-2.2).

docs/settings.rst

@@ -88,13 +88,6 @@ CLIENT_SECRET_GENERATOR_LENGTH
 The length of the generated secrets, in characters. If this value is too low,
 secrets may become subject to bruteforce guessing.
 
-CLIENT_SECRET_HASHER
-~~~~~~~~~~~~~~~~~~~~
-If set to one of the Django password hasher algorithm names, client_secret values will be
-stored as `hashed Django passwords <https://docs.djangoproject.com/en/stable/topics/auth/passwords/#how-django-stores-passwords>`_.
-See the official list in the django.contrib.auth.hashers namespace.
-Default is none (stored as plain text).
-
 EXTRA_SERVER_KWARGS
 ~~~~~~~~~~~~~~~~~~~
 A dictionary to be passed to oauthlib's Server class. Three options
@@ -104,19 +97,19 @@ of those three can be a callable) must be passed here directly and classes
 must be instantiated (callables should accept request as their only argument).
 
 GRANT_MODEL
-~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~
 The import string of the class (model) representing your grants. Overwrite
 this value if you wrote your own implementation (subclass of
 ``oauth2_provider.models.Grant``).
 
 APPLICATION_ADMIN_CLASS
-~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~
 The import string of the class (model) representing your application admin class.
 Overwrite this value if you wrote your own implementation (subclass of
 ``oauth2_provider.admin.ApplicationAdmin``).
 
 ACCESS_TOKEN_ADMIN_CLASS
-~~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~
 The import string of the class (model) representing your access token admin class.
 Overwrite this value if you wrote your own implementation (subclass of
 ``oauth2_provider.admin.AccessTokenAdmin``).
@@ -128,7 +121,7 @@ Overwrite this value if you wrote your own implementation (subclass of
 ``oauth2_provider.admin.GrantAdmin``).
 
 REFRESH_TOKEN_ADMIN_CLASS
-~~~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~
 The import string of the class (model) representing your refresh token admin class.
 Overwrite this value if you wrote your own implementation (subclass of
 ``oauth2_provider.admin.RefreshTokenAdmin``).
@@ -161,7 +154,7 @@ If you don't change the validator code and don't run cleartokens all refresh
 tokens will last until revoked or the end of time. You should change this.
 
 REFRESH_TOKEN_GRACE_PERIOD_SECONDS
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 The number of seconds between when a refresh token is first used when it is
 expired. The most common case of this for this is native mobile applications
 that run into issues of network connectivity during the refresh cycle and are
@@ -185,7 +178,7 @@ See also: validator's rotate_refresh_token method can be overridden to make this
 when close to expiration, theoretically).
 
 REFRESH_TOKEN_GENERATOR
-~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~~~~
 See `ACCESS_TOKEN_GENERATOR`. This is the same but for refresh tokens.
 Defaults to access token generator if not provided.
 
@@ -272,7 +265,7 @@ Default: ``""``
 The RSA private key used to sign OIDC ID tokens. If not set, OIDC is disabled.
 
 OIDC_RSA_PRIVATE_KEYS_INACTIVE
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~
 Default: ``[]``
 
 An array of *inactive* RSA private keys. These keys are not used to sign tokens,
@@ -283,7 +276,7 @@ This is useful for providing a smooth transition during key rotation.
 should be retained in this inactive list.
 
 OIDC_JWKS_MAX_AGE_SECONDS
-~~~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~
 Default: ``3600``
 
 The max-age value for the Cache-Control header on jwks_uri.
@@ -358,9 +351,9 @@ Time of sleep in seconds used by ``cleartokens`` management command between batc
 
 
 Settings imported from Django project
--------------------------------------
+--------------------------
 
 USE_TZ
-~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 Used to determine whether or not to make token expire dates timezone aware.

oauth2_provider/migrations/0006_alter_application_client_secret.py

@@ -6,7 +6,6 @@
 
 from django.apps import apps
 from django.conf import settings
-from django.contrib.auth.hashers import make_password
 from django.core.exceptions import ImproperlyConfigured
 from django.db import models, transaction
 from django.urls import reverse
@@ -25,19 +24,6 @@
 logger = logging.getLogger(__name__)
 
 
-class ClientSecretField(models.CharField):
-    def pre_save(self, model_instance, add):
-        if oauth2_settings.CLIENT_SECRET_HASHER:
-            plain_secret = getattr(model_instance, self.attname)
-            if "$" not in plain_secret:  # not yet hashed
-                hashed_secret = make_password(
-                    plain_secret, salt=model_instance.client_id, hasher=oauth2_settings.CLIENT_SECRET_HASHER
-                )
-                setattr(model_instance, self.attname, hashed_secret)
-                return hashed_secret
-        return super().pre_save(model_instance, add)
-
-
 class AbstractApplication(models.Model):
     """
     An Application instance represents a Client on the Authorization server.
@@ -104,7 +90,7 @@ class AbstractApplication(models.Model):
     )
     client_type = models.CharField(max_length=32, choices=CLIENT_TYPES)
     authorization_grant_type = models.CharField(max_length=32, choices=GRANT_TYPES)
-    client_secret = ClientSecretField(
+    client_secret = models.CharField(
         max_length=255, blank=True, default=generate_client_secret, db_index=True
     )
     name = models.CharField(max_length=255, blank=True)

oauth2_provider/models.py

@@ -11,7 +11,6 @@
 import requests
 from django.conf import settings
 from django.contrib.auth import authenticate, get_user_model
-from django.contrib.auth.hashers import check_password
 from django.core.exceptions import ObjectDoesNotExist
 from django.db import transaction
 from django.db.models import Q
@@ -123,17 +122,6 @@ def _authenticate_basic_auth(self, request):
         elif request.client.client_id != client_id:
             log.debug("Failed basic auth: wrong client id %s" % client_id)
             return False
-        # we use the "$" as a sentinel character to determine
-        # whether a secret has been hashed like a Django password or not.
-        # We can do this because the default oauthlib.common.UNICODE_ASCII_CHARACTER_SET
-        # used by our default generator does not include the "$" character.
-        # However, if a different character set was used to generate the secret, this sentinel
-        # might be a false positive.
-        elif "$" in request.client.client_secret and request.client.client_secret != client_secret:
-            if not check_password(client_secret, request.client.client_secret):
-                log.debug("Failed basic auth: wrong hashed client secret %s" % client_secret)
-                return False
-            return True
         elif request.client.client_secret != client_secret:
             log.debug("Failed basic auth: wrong client secret %s" % client_secret)
             return False

oauth2_provider/oauth2_validators.py

@@ -37,7 +37,6 @@
     "CLIENT_ID_GENERATOR_CLASS": "oauth2_provider.generators.ClientIdGenerator",
     "CLIENT_SECRET_GENERATOR_CLASS": "oauth2_provider.generators.ClientSecretGenerator",
     "CLIENT_SECRET_GENERATOR_LENGTH": 128,
-    "CLIENT_SECRET_HASHER": None,
     "ACCESS_TOKEN_GENERATOR": None,
     "REFRESH_TOKEN_GENERATOR": None,
     "EXTRA_SERVER_KWARGS": {},

oauth2_provider/settings.py

@@ -44,6 +44,3 @@
     "READ_SCOPE": "read",
     "WRITE_SCOPE": "write",
 }
-
-# default django auth hasher as of version 3.2
-CLIENT_SECRET_HASHER = {"CLIENT_SECRET_HASHER": "pbkdf2_sha256"}

tests/presets.py

@@ -24,8 +24,6 @@
 AccessToken = get_access_token_model()
 UserModel = get_user_model()
 
-CLIENT_SECRET = "abcdefghijklmnopqrstuvwxyz1234567890"
-
 
 # mocking a protected resource view
 class ResourceView(ProtectedResourceView):
@@ -46,7 +44,6 @@ def setUp(self):
             user=self.dev_user,
             client_type=Application.CLIENT_PUBLIC,
             authorization_grant_type=Application.GRANT_CLIENT_CREDENTIALS,
-            client_secret=CLIENT_SECRET,
         )
 
     def tearDown(self):
@@ -82,29 +79,6 @@ def test_client_credential_access_allowed(self):
         response = view(request)
         self.assertEqual(response, "This is a protected resource")
 
-    @pytest.mark.oauth2_settings(presets.CLIENT_SECRET_HASHER)
-    def test_client_credential_with_hashed_client_secret(self):
-        """
-        Verify client_secret is hashed before writing to the db,
-        and comparison on request uses same hashing algo.
-        """
-        self.assertNotEqual(self.application.client_secret, CLIENT_SECRET)
-        self.assertIn("$", self.application.client_secret)
-        self.assertIn(presets.CLIENT_SECRET_HASHER["CLIENT_SECRET_HASHER"], self.application.client_secret)
-
-        token_request_data = {
-            "grant_type": "client_credentials",
-        }
-        auth_headers = get_basic_auth_header(self.application.client_id, CLIENT_SECRET)
-
-        response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers)
-        self.assertEqual(response.status_code, 200)
-
-        # secret mismatch should return a 401
-        auth_headers = get_basic_auth_header(self.application.client_id, "not-the-secret")
-        response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers)
-        self.assertEqual(response.status_code, 401)
-
     def test_client_credential_does_not_issue_refresh_token(self):
         token_request_data = {
             "grant_type": "client_credentials",