From f9c9c332174c134e5c5b1a092db26ef894854afd 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] fix: token request throws an error when client is provided in body It ends up overwriting request.client which should be the application. This fix ensures request client is set to the client_id --- AUTHORS | 1 + CHANGELOG.md | 6 +++- oauth2_provider/oauth2_validators.py | 31 ++++++++++++------ tests/test_authorization_code.py | 21 ++++++++++++ tests/test_oauth2_validators.py | 48 ++++++++++++++++++++++++++-- 5 files changed, 95 insertions(+), 12 deletions(-) diff --git a/AUTHORS b/AUTHORS index 2d8d5465b..e35ed4cb9 100644 --- a/AUTHORS +++ b/AUTHORS @@ -102,6 +102,7 @@ Peter Karman Peter McDonald Petr Dlouhý pySilver +@realsuayip Rodney Richardson Rustem Saiargaliev Rustem Saiargaliev diff --git a/CHANGELOG.md b/CHANGELOG.md index 1821a8ae1..eadc051d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,12 +9,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added * Support for Django 5.2 * Support for Python 3.14 (Django >= 5.2.8) +* #1539 Add device authorization grant support + ### Fixed +* #1252 Fix crash when 'client' is in token request body + @@ -27,7 +32,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * #1506 Support for Wildcard Origin and Redirect URIs - Adds a new setting [ALLOW_URL_WILDCARDS](https://django-oauth-toolkit.readthedocs.io/en/latest/settings.html#allow-uri-wildcards). This feature is useful for working with CI service such as cloudflare, netlify, and vercel that offer branch deployments for development previews and user acceptance testing. * #1586 Turkish language support added -* #1539 Add device authorization grant support ### Changed The project is now hosted in the django-oauth organization. diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index ec974b0c6..a202a6a82 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -214,19 +214,31 @@ 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' - + if request.client: + # check for cached client, to save the db hit if this has already been loaded + if not isinstance(request.client, Application): + # resetting request.client (client_id=%r): not an Application, something else set request.client erroneously + request.client = None + elif request.client.client_id != client_id: + # resetting request.client (client_id=%r): request.client.client_id does not match the given client_id + request.client = None + elif not request.client.is_usable(request): + # resetting request.client (client_id=%r): request.client is a valid Application, but is not usable + request.client = None + else: + # request.client is a valid Application, reusing it + return request.client try: - request.client = request.client or 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)) + # cache not hit, loading application from database for client_id %r + client = Application.objects.get(client_id=client_id) + if not client.is_usable(request): + # Failed to load application: Application %r is not usable return None + request.client = client + # Loaded application with client_id %r from database return request.client except Application.DoesNotExist: - log.debug("Failed body authentication: Application %r does not exist" % (client_id)) + # Failed to load application: Application with client_id %r does not exist return None def _set_oauth2_error_on_request(self, request, access_token, scopes): @@ -289,6 +301,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 diff --git a/tests/test_authorization_code.py b/tests/test_authorization_code.py index 360fac957..369b1939f 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): + """ + 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() + + 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 diff --git a/tests/test_oauth2_validators.py b/tests/test_oauth2_validators.py index 7e7e46de7..3fb292060 100644 --- a/tests/test_oauth2_validators.py +++ b/tests/test_oauth2_validators.py @@ -216,8 +216,52 @@ 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() + + @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()))