From 7e628c4bd7f5ad045570e4c4453c13fb8b68279e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=9Fuayip=20=C3=BCz=C3=BClmez?= Date: Tue, 28 Feb 2023 23:43:50 +0300 Subject: [PATCH 01/10] Fix possible crash in validator when 'client' key is mentioned in request body --- oauth2_provider/oauth2_validators.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index db459a446..49dfe4ab8 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -207,7 +207,8 @@ def _load_application(self, client_id, request): assert hasattr(request, "client"), '"request" instance has no "client" attribute' try: - request.client = request.client or Application.objects.get(client_id=client_id) + if not isinstance(request.client, Application): + request.client = Application.objects.get(client_id=client_id) # Check that the application can be used (defaults to always True) if not request.client.is_usable(request): log.debug("Failed body authentication: Application %r is disabled" % (client_id)) From 71d26056a2c7e11320be96cbb7ca0a1d77b6ee55 Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Sun, 2 Nov 2025 21:02:43 -0500 Subject: [PATCH 02/10] chore: test to demonstrate bug https://github.com/django-oauth/django-oauth-toolkit/pull/1252 --- tests/test_authorization_code.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/test_authorization_code.py b/tests/test_authorization_code.py index 360fac957..7bf4006b6 100644 --- a/tests/test_authorization_code.py +++ b/tests/test_authorization_code.py @@ -1308,6 +1308,27 @@ def test_request_body_params(self): self.assertEqual(content["scope"], "read write") self.assertEqual(content["expires_in"], self.oauth2_settings.ACCESS_TOKEN_EXPIRE_SECONDS) + def test_request_body_params_client_typo(self): + """ + Request an access token using client_type: public + """ + self.client.login(username="test_user", password="123456") + authorization_code = self.get_auth() + + token_request_data = { + "grant_type": "authorization_code", + "code": authorization_code, + "redirect_uri": "http://example.org", + "client": self.application.client_id, + "client_secret": CLEARTEXT_SECRET, + } + + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data) + self.assertEqual(response.status_code, 401) + + content = json.loads(response.content.decode("utf-8")) + self.assertEqual(content["error"], "invalid_client") + def test_public(self): """ Request an access token using client_type: public From f45085589d6698cf2ddba084eec761ef01da79e5 Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Sun, 2 Nov 2025 22:09:48 -0500 Subject: [PATCH 03/10] fix: token throws an error when client is provided as a string instead of the client_id --- oauth2_provider/oauth2_validators.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index 49dfe4ab8..7fb520848 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -208,6 +208,7 @@ def _load_application(self, client_id, request): try: if not isinstance(request.client, Application): + log.debug("invalid client type, Loading application for client_id %r", client_id) request.client = Application.objects.get(client_id=client_id) # Check that the application can be used (defaults to always True) if not request.client.is_usable(request): @@ -216,6 +217,7 @@ def _load_application(self, client_id, request): return request.client except Application.DoesNotExist: log.debug("Failed body authentication: Application %r does not exist" % (client_id)) + request.client = None return None def _set_oauth2_error_on_request(self, request, access_token, scopes): @@ -278,6 +280,7 @@ def client_authentication_required(self, request, *args, **kwargs): pass self._load_application(request.client_id, request) + log.debug("Determining if client authentication is required for client %r", request.client) if request.client: return request.client.client_type == AbstractApplication.CLIENT_CONFIDENTIAL From da0f147982d24a8fe7462828c8e2cfa35febf040 Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Sun, 2 Nov 2025 22:14:49 -0500 Subject: [PATCH 04/10] fix: token throws an error when client is provided as a string instead of the client_id, cleaner implementation --- oauth2_provider/oauth2_validators.py | 34 ++++++++++++++++++---------- tests/test_oauth2_validators.py | 32 ++++++++++++++++++++++++-- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index 7fb520848..721a8cf63 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -202,22 +202,32 @@ def _load_application(self, client_id, request): If request.client was not set, load application instance for given client_id and store it in request.client """ - - # we want to be sure that request has the client attribute! - assert hasattr(request, "client"), '"request" instance has no "client" attribute' - - try: + if request.client: + """ check for cached client, to save the db hit if this has alredy been loaded """ if not isinstance(request.client, Application): - log.debug("invalid client type, Loading application for client_id %r", client_id) - request.client = Application.objects.get(client_id=client_id) - # Check that the application can be used (defaults to always True) - if not request.client.is_usable(request): - log.debug("Failed body authentication: Application %r is disabled" % (client_id)) + log.debug("request.client is not an Application, something else set request.client erroroneously, resetting request.client.") + request.client = None + elif request.client.client_id != client_id: + log.debug("request.client client_id does not match the given client_id, resetting request.client.") + request.client = None + elif not request.client.is_usable(request): + log.debug("request.client is a valid Application, but is not usable, resetting request.client.") + request.client = None + else: + log.debug("request.client is a valid Application, reusing it.") + return request.client + try: + """ cache wasn't hit, load from db """ + log.debug("cache not hit, Loading application from database for client_id %r", client_id) + client = Application.objects.get(client_id=client_id) + if not client.is_usable(request): + log.debug("Failed to load application: Application %r is not usable" % (client_id)) return None + log.debug("Loaded application %r from database", client) + request.client = client return request.client except Application.DoesNotExist: - log.debug("Failed body authentication: Application %r does not exist" % (client_id)) - request.client = None + log.debug("Failed to load application: Application %r does not exist" % (client_id)) return None def _set_oauth2_error_on_request(self, request, access_token, scopes): diff --git a/tests/test_oauth2_validators.py b/tests/test_oauth2_validators.py index 14c74506e..0a67dcbbb 100644 --- a/tests/test_oauth2_validators.py +++ b/tests/test_oauth2_validators.py @@ -210,8 +210,36 @@ def test_client_authentication_required(self): self.request.client = "" self.assertTrue(self.validator.client_authentication_required(self.request)) - def test_load_application_fails_when_request_has_no_client(self): - self.assertRaises(AssertionError, self.validator.authenticate_client_id, "client_id", {}) + def test_load_application_loads_client_id_when_request_has_no_client(self): + self.request.client = None + application = self.validator._load_application("client_id", self.request) + self.assertEqual(application, self.application) + + def test_load_application_uses_cached_when_request_has_valid_client_matching_client_id(self): + self.request.client = self.application + application = self.validator._load_application("client_id", self.request) + self.assertIs(application, self.application) + self.assertIs(self.request.client, self.application) + + def test_load_application_succeeds_when_request_has_invalid_client_valid_client_id(self): + self.request.client = 'invalid_client' + application = self.validator._load_application("client_id", self.request) + self.assertEqual(application, self.application) + self.assertEqual(self.request.client, self.application) + + def test_load_application_overwrites_client_on_client_id_mismatch(self): + another_application = Application.objects.create( + client_id="another_client_id", + client_secret=CLEARTEXT_SECRET, + user=self.user, + client_type=Application.CLIENT_PUBLIC, + authorization_grant_type=Application.GRANT_PASSWORD, + ) + self.request.client = another_application + application = self.validator._load_application("client_id", self.request) + self.assertEqual(application, self.application) + self.assertEqual(self.request.client, self.application) + another_application.delete() def test_rotate_refresh_token__is_true(self): self.assertTrue(self.validator.rotate_refresh_token(mock.MagicMock())) From 611cd52677fbb7bbd44ec5183642bf23bf3381a1 Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Mon, 3 Nov 2025 09:06:57 -0500 Subject: [PATCH 05/10] chore: complete test coverage --- tests/test_oauth2_validators.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/test_oauth2_validators.py b/tests/test_oauth2_validators.py index 0a67dcbbb..031ef2f29 100644 --- a/tests/test_oauth2_validators.py +++ b/tests/test_oauth2_validators.py @@ -241,6 +241,22 @@ def test_load_application_overwrites_client_on_client_id_mismatch(self): self.assertEqual(self.request.client, self.application) another_application.delete() + @mock.patch.object(Application, "is_usable") + def test_load_application_returns_none_when_client_not_usable_cached(self, mock_is_usable): + mock_is_usable.return_value = False + self.request.client = self.application + application = self.validator._load_application("client_id", self.request) + self.assertIsNone(application) + self.assertIsNone(self.request.client) + + @mock.patch.object(Application, "is_usable") + def test_load_application_returns_none_when_client_not_usable_db_lookup(self, mock_is_usable): + mock_is_usable.return_value = False + self.request.client = None + application = self.validator._load_application("client_id", self.request) + self.assertIsNone(application) + self.assertIsNone(self.request.client) + def test_rotate_refresh_token__is_true(self): self.assertTrue(self.validator.rotate_refresh_token(mock.MagicMock())) From e5723899d9e15a2931081dc54ed6c2457e10b16c Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Wed, 5 Nov 2025 17:01:11 -0500 Subject: [PATCH 06/10] chore: address copilot feedback. https://github.com/django-oauth/django-oauth-toolkit/pull/1252#pullrequestreview-3419497330 --- oauth2_provider/oauth2_validators.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index 721a8cf63..ff404b7a8 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -203,9 +203,9 @@ def _load_application(self, client_id, request): client_id and store it in request.client """ if request.client: - """ check for cached client, to save the db hit if this has alredy been loaded """ + # check for cached client, to save the db hit if this has already been loaded if not isinstance(request.client, Application): - log.debug("request.client is not an Application, something else set request.client erroroneously, resetting request.client.") + log.debug("request.client is not an Application, something else set request.client erroneously, resetting request.client.") request.client = None elif request.client.client_id != client_id: log.debug("request.client client_id does not match the given client_id, resetting request.client.") @@ -217,7 +217,7 @@ def _load_application(self, client_id, request): log.debug("request.client is a valid Application, reusing it.") return request.client try: - """ cache wasn't hit, load from db """ + # cache wasn't hit, load from db log.debug("cache not hit, Loading application from database for client_id %r", client_id) client = Application.objects.get(client_id=client_id) if not client.is_usable(request): From 0fd1830234bc4702f930c4dc53748279a7d63f53 Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Wed, 5 Nov 2025 18:10:38 -0500 Subject: [PATCH 07/10] chore: copilot comments https://github.com/django-oauth/django-oauth-toolkit/pull/1252#pullrequestreview-3424665079 --- oauth2_provider/oauth2_validators.py | 12 ++++++------ tests/test_authorization_code.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index ff404b7a8..b75db909e 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -157,7 +157,7 @@ def _authenticate_basic_auth(self, request): try: client_id, client_secret = map(unquote_plus, auth_string_decoded.split(":", 1)) except ValueError: - log.debug("Failed basic auth, Invalid base64 encoding.") + log.debug("Failed basic auth, Invalid base64 encoding") return False if self._load_application(client_id, request) is None: @@ -205,16 +205,16 @@ def _load_application(self, client_id, request): if request.client: # check for cached client, to save the db hit if this has already been loaded if not isinstance(request.client, Application): - log.debug("request.client is not an Application, something else set request.client erroneously, resetting request.client.") + log.debug("request.client is not an Application, something else set request.client erroneously, resetting request.client") request.client = None elif request.client.client_id != client_id: - log.debug("request.client client_id does not match the given client_id, resetting request.client.") + log.debug("request.client client_id does not match the given client_id, resetting request.client") request.client = None elif not request.client.is_usable(request): - log.debug("request.client is a valid Application, but is not usable, resetting request.client.") + log.debug("request.client is a valid Application, but is not usable, resetting request.client") request.client = None else: - log.debug("request.client is a valid Application, reusing it.") + log.debug("request.client is a valid Application, reusing it") return request.client try: # cache wasn't hit, load from db @@ -223,8 +223,8 @@ def _load_application(self, client_id, request): if not client.is_usable(request): log.debug("Failed to load application: Application %r is not usable" % (client_id)) return None - log.debug("Loaded application %r from database", client) request.client = client + log.debug("Loaded application %r from database", client) return request.client except Application.DoesNotExist: log.debug("Failed to load application: Application %r does not exist" % (client_id)) diff --git a/tests/test_authorization_code.py b/tests/test_authorization_code.py index 7bf4006b6..369b1939f 100644 --- a/tests/test_authorization_code.py +++ b/tests/test_authorization_code.py @@ -1310,7 +1310,7 @@ def test_request_body_params(self): def test_request_body_params_client_typo(self): """ - Request an access token using client_type: public + Verify that using incorrect parameter name (client instead of client_id) returns invalid_client error """ self.client.login(username="test_user", password="123456") authorization_code = self.get_auth() From effc706e18e677fa4dbc310dd6bcd530f35a57a6 Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Wed, 5 Nov 2025 18:19:57 -0500 Subject: [PATCH 08/10] chore: copilot comment https://github.com/django-oauth/django-oauth-toolkit/pull/1252#pullrequestreview-3424914256 --- oauth2_provider/oauth2_validators.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index b75db909e..dec69cd5b 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -224,7 +224,7 @@ def _load_application(self, client_id, request): log.debug("Failed to load application: Application %r is not usable" % (client_id)) return None request.client = client - log.debug("Loaded application %r from database", client) + log.debug("Loaded application with client_id %r from database", client.client_id) return request.client except Application.DoesNotExist: log.debug("Failed to load application: Application %r does not exist" % (client_id)) From 3f4c9b9eeab79da24319fa2d953b4bde180e7e0a Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Thu, 6 Nov 2025 10:44:59 -0500 Subject: [PATCH 09/10] chore: copilot comment https://github.com/django-oauth/django-oauth-toolkit/pull/1252#pullrequestreview-3424938998 --- oauth2_provider/oauth2_validators.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index dec69cd5b..d37c52dd2 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -218,7 +218,7 @@ def _load_application(self, client_id, request): return request.client try: # cache wasn't hit, load from db - log.debug("cache not hit, Loading application from database for client_id %r", client_id) + log.debug("cache not hit, loading application from database for client_id %r", client_id) client = Application.objects.get(client_id=client_id) if not client.is_usable(request): log.debug("Failed to load application: Application %r is not usable" % (client_id)) From fbeb75f6ff495f8c2be8fcb02a66b0213ad1066e Mon Sep 17 00:00:00 2001 From: Darrel O'Pry Date: Thu, 6 Nov 2025 21:24:59 -0500 Subject: [PATCH 10/10] Update oauth2_provider/oauth2_validators.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- oauth2_provider/oauth2_validators.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index d37c52dd2..236820730 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -205,13 +205,13 @@ def _load_application(self, client_id, request): if request.client: # check for cached client, to save the db hit if this has already been loaded if not isinstance(request.client, Application): - log.debug("request.client is not an Application, something else set request.client erroneously, resetting request.client") + log.debug("resetting request.client (client_id=%r): not an Application, something else set request.client erroneously", client_id) request.client = None elif request.client.client_id != client_id: - log.debug("request.client client_id does not match the given client_id, resetting request.client") + log.debug("resetting request.client (client_id=%r): request.client client_id does not match the given client_id", client_id) request.client = None elif not request.client.is_usable(request): - log.debug("request.client is a valid Application, but is not usable, resetting request.client") + log.debug("resetting request.client (client_id=%r): request.client is a valid Application, but is not usable", client_id) request.client = None else: log.debug("request.client is a valid Application, reusing it")