From 14bc650197ee333f89e8a27b2ccf024fe7c2be9c Mon Sep 17 00:00:00 2001 From: zxkeyy Date: Thu, 5 Dec 2024 18:55:53 +0100 Subject: [PATCH 1/8] Feat: Add authentication class and authentication rule that allows inactive user authentication --- rest_framework_simplejwt/authentication.py | 41 ++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/rest_framework_simplejwt/authentication.py b/rest_framework_simplejwt/authentication.py index 13767e1ee..bca943594 100644 --- a/rest_framework_simplejwt/authentication.py +++ b/rest_framework_simplejwt/authentication.py @@ -164,6 +164,37 @@ def get_user(self, validated_token: Token) -> AuthUser: return api_settings.TOKEN_USER_CLASS(validated_token) +class JWTInactiveUserAuthentication(JWTAuthentication): + """ + An authentication plugin that authenticates requests through a JSON web + token provided in a request header, allowing inactive users to authenticate. + """ + + def get_user(self, validated_token: Token) -> AuthUser: + """ + Attempts to find and return a user using the given validated token. + """ + try: + user_id = validated_token[api_settings.USER_ID_CLAIM] + except KeyError: + raise InvalidToken(_("Token contained no recognizable user identification")) + + try: + user = self.user_model.objects.get(**{api_settings.USER_ID_FIELD: user_id}) + except self.user_model.DoesNotExist: + raise AuthenticationFailed(_("User not found"), code="user_not_found") + + if api_settings.CHECK_REVOKE_TOKEN: + if validated_token.get( + api_settings.REVOKE_TOKEN_CLAIM + ) != get_md5_hash_password(user.password): + raise AuthenticationFailed( + _("The user's password has been changed."), code="password_changed" + ) + + return user + + JWTTokenUserAuthentication = JWTStatelessUserAuthentication @@ -176,3 +207,13 @@ def default_user_authentication_rule(user: AuthUser) -> bool: # users from authenticating to enforce a reasonable policy and provide # sensible backwards compatibility with older Django versions. return user is not None and user.is_active + + +def allow_inactive_users_authentication_rule(user: AuthUser) -> bool: + """ + Authentication rule that allows both active and inactive users. + + This rule differs from the default authentication rule by + removing the is_active check. + """ + return user is not None From 731d489f8019d33d27a4167fb2f5ff4ed1e4c489 Mon Sep 17 00:00:00 2001 From: zxkeyy Date: Thu, 5 Dec 2024 21:09:16 +0100 Subject: [PATCH 2/8] Test: Add tests for inactive user authentication and serializer validation rules --- tests/test_authentication.py | 27 +++++++++++++++++++++++++++ tests/test_serializers.py | 25 ++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 99f0b5525..0609ac8a6 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -241,3 +241,30 @@ def username(self): # Restore default TokenUser for future tests api_settings.TOKEN_USER_CLASS = temp + + +class TestJWTInactiveUserAuthentication(TestCase): + def setUp(self): + self.backend = authentication.JWTInactiveUserAuthentication() + + def test_get_user(self): + payload = {"some_other_id": "foo"} + + # Should raise error if no recognizable user identification + with self.assertRaises(InvalidToken): + self.backend.get_user(payload) + + payload[api_settings.USER_ID_CLAIM] = 42 + + # Should raise exception if user not found + with self.assertRaises(AuthenticationFailed): + self.backend.get_user(payload) + + u = User.objects.create_user(username="markhamill") + u.is_active = False + u.save() + + payload[api_settings.USER_ID_CLAIM] = getattr(u, api_settings.USER_ID_FIELD) + + # Otherwise, should return correct user + self.assertEqual(self.backend.get_user(payload).id, u.id) \ No newline at end of file diff --git a/tests/test_serializers.py b/tests/test_serializers.py index 6ec6a8083..c6dcbb17c 100644 --- a/tests/test_serializers.py +++ b/tests/test_serializers.py @@ -5,7 +5,7 @@ from django.conf import settings from django.contrib.auth import get_user_model from django.core import exceptions as django_exceptions -from django.test import TestCase +from django.test import TestCase, override_settings from rest_framework import exceptions as drf_exceptions from rest_framework_simplejwt.exceptions import TokenError @@ -105,6 +105,29 @@ def test_it_should_raise_if_user_not_active(self): with self.assertRaises(drf_exceptions.AuthenticationFailed): s.is_valid() + @override_settings( + AUTHENTICATION_BACKENDS=[ + 'django.contrib.auth.backends.AllowAllUsersModelBackend', + 'django.contrib.auth.backends.ModelBackend', + ] +) + @override_api_settings( + USER_AUTHENTICATION_RULE="rest_framework_simplejwt.authentication.allow_inactive_users_authentication_rule" + ) + def test_it_should_validate_if_user_inactive_but_rule_allows(self): + self.user.is_active = False + self.user.save() + + s = TokenObtainSerializer( + context=MagicMock(), + data={ + TokenObtainSerializer.username_field: self.username, + "password": self.password, + }, + ) + + self.assertTrue(s.is_valid()) + class TestTokenObtainSlidingSerializer(TestCase): def setUp(self): From 6d22c210359ab8435ae62aaf0ab5f8e975a2ab61 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 5 Dec 2024 20:53:31 +0000 Subject: [PATCH 3/8] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- rest_framework_simplejwt/authentication.py | 4 ++-- tests/test_authentication.py | 2 +- tests/test_serializers.py | 12 ++++++------ 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/rest_framework_simplejwt/authentication.py b/rest_framework_simplejwt/authentication.py index bca943594..16df72ca6 100644 --- a/rest_framework_simplejwt/authentication.py +++ b/rest_framework_simplejwt/authentication.py @@ -212,8 +212,8 @@ def default_user_authentication_rule(user: AuthUser) -> bool: def allow_inactive_users_authentication_rule(user: AuthUser) -> bool: """ Authentication rule that allows both active and inactive users. - - This rule differs from the default authentication rule by + + This rule differs from the default authentication rule by removing the is_active check. """ return user is not None diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 0609ac8a6..2608f8394 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -267,4 +267,4 @@ def test_get_user(self): payload[api_settings.USER_ID_CLAIM] = getattr(u, api_settings.USER_ID_FIELD) # Otherwise, should return correct user - self.assertEqual(self.backend.get_user(payload).id, u.id) \ No newline at end of file + self.assertEqual(self.backend.get_user(payload).id, u.id) diff --git a/tests/test_serializers.py b/tests/test_serializers.py index c6dcbb17c..4f2bdd236 100644 --- a/tests/test_serializers.py +++ b/tests/test_serializers.py @@ -106,11 +106,11 @@ def test_it_should_raise_if_user_not_active(self): s.is_valid() @override_settings( - AUTHENTICATION_BACKENDS=[ - 'django.contrib.auth.backends.AllowAllUsersModelBackend', - 'django.contrib.auth.backends.ModelBackend', - ] -) + AUTHENTICATION_BACKENDS=[ + "django.contrib.auth.backends.AllowAllUsersModelBackend", + "django.contrib.auth.backends.ModelBackend", + ] + ) @override_api_settings( USER_AUTHENTICATION_RULE="rest_framework_simplejwt.authentication.allow_inactive_users_authentication_rule" ) @@ -125,7 +125,7 @@ def test_it_should_validate_if_user_inactive_but_rule_allows(self): "password": self.password, }, ) - + self.assertTrue(s.is_valid()) From df7c68acda2f4ffc0555f943c431a805bead01df Mon Sep 17 00:00:00 2001 From: zxkeyy Date: Sat, 7 Dec 2024 18:02:01 +0100 Subject: [PATCH 4/8] Add check if user is active setting, Update default_user_authentication_rule to use the setting, Update JWTAuthentication.get_user() to test user against authenrication rule instead of is_active flag --- rest_framework_simplejwt/authentication.py | 15 +++------------ rest_framework_simplejwt/settings.py | 1 + 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/rest_framework_simplejwt/authentication.py b/rest_framework_simplejwt/authentication.py index 16df72ca6..87b00aed2 100644 --- a/rest_framework_simplejwt/authentication.py +++ b/rest_framework_simplejwt/authentication.py @@ -131,7 +131,8 @@ def get_user(self, validated_token: Token) -> AuthUser: except self.user_model.DoesNotExist: raise AuthenticationFailed(_("User not found"), code="user_not_found") - if not user.is_active: + # Ensure authentication rule passes + if not api_settings.USER_AUTHENTICATION_RULE(user): raise AuthenticationFailed(_("User is inactive"), code="user_inactive") if api_settings.CHECK_REVOKE_TOKEN: @@ -206,14 +207,4 @@ def default_user_authentication_rule(user: AuthUser) -> bool: # `AllowAllUsersModelBackend`. However, we explicitly prevent inactive # users from authenticating to enforce a reasonable policy and provide # sensible backwards compatibility with older Django versions. - return user is not None and user.is_active - - -def allow_inactive_users_authentication_rule(user: AuthUser) -> bool: - """ - Authentication rule that allows both active and inactive users. - - This rule differs from the default authentication rule by - removing the is_active check. - """ - return user is not None + return user is not None and (not api_settings.CHECK_USER_IS_ACTIVE or user.is_active) diff --git a/rest_framework_simplejwt/settings.py b/rest_framework_simplejwt/settings.py index 7691bb863..dadcd93a7 100644 --- a/rest_framework_simplejwt/settings.py +++ b/rest_framework_simplejwt/settings.py @@ -44,6 +44,7 @@ "SLIDING_TOKEN_REFRESH_SERIALIZER": "rest_framework_simplejwt.serializers.TokenRefreshSlidingSerializer", "CHECK_REVOKE_TOKEN": False, "REVOKE_TOKEN_CLAIM": "hash_password", + "CHECK_USER_IS_ACTIVE": True, } IMPORT_STRINGS = ( From 7789c87a9fb4db0cbef2a974110f7a0b50c7e678 Mon Sep 17 00:00:00 2001 From: zxkeyy Date: Sat, 7 Dec 2024 18:02:12 +0100 Subject: [PATCH 5/8] Add test for retrieving inactive user in JWT authentication --- tests/test_authentication.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 2608f8394..5ff4b9a63 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -161,6 +161,31 @@ def test_get_user(self): # Otherwise, should return correct user self.assertEqual(self.backend.get_user(payload).id, u.id) + @override_api_settings( + CHECK_USER_IS_ACTIVE=False, + ) + def test_get_inactive_user(self): + payload = {"some_other_id": "foo"} + + # Should raise error if no recognizable user identification + with self.assertRaises(InvalidToken): + self.backend.get_user(payload) + + payload[api_settings.USER_ID_CLAIM] = 42 + + # Should raise exception if user not found + with self.assertRaises(AuthenticationFailed): + self.backend.get_user(payload) + + u = User.objects.create_user(username="markhamill") + u.is_active = False + u.save() + + payload[api_settings.USER_ID_CLAIM] = getattr(u, api_settings.USER_ID_FIELD) + + # should return correct user + self.assertEqual(self.backend.get_user(payload).id, u.id) + @override_api_settings( CHECK_REVOKE_TOKEN=True, REVOKE_TOKEN_CLAIM="revoke_token_claim" ) From adefffec80982e2c34df02bc33fa3361d0e6d29c Mon Sep 17 00:00:00 2001 From: zxkeyy Date: Sat, 7 Dec 2024 18:16:18 +0100 Subject: [PATCH 6/8] Use new setting in serializer test --- tests/test_serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_serializers.py b/tests/test_serializers.py index 4f2bdd236..0fe03a9f1 100644 --- a/tests/test_serializers.py +++ b/tests/test_serializers.py @@ -112,7 +112,7 @@ def test_it_should_raise_if_user_not_active(self): ] ) @override_api_settings( - USER_AUTHENTICATION_RULE="rest_framework_simplejwt.authentication.allow_inactive_users_authentication_rule" + CHECK_USER_IS_ACTIVE=False, ) def test_it_should_validate_if_user_inactive_but_rule_allows(self): self.user.is_active = False From 18f62dc5f7e5090fc859694039a3b2b3600c3957 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 7 Dec 2024 17:16:39 +0000 Subject: [PATCH 7/8] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- rest_framework_simplejwt/authentication.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rest_framework_simplejwt/authentication.py b/rest_framework_simplejwt/authentication.py index 87b00aed2..e4be0f95e 100644 --- a/rest_framework_simplejwt/authentication.py +++ b/rest_framework_simplejwt/authentication.py @@ -207,4 +207,6 @@ def default_user_authentication_rule(user: AuthUser) -> bool: # `AllowAllUsersModelBackend`. However, we explicitly prevent inactive # users from authenticating to enforce a reasonable policy and provide # sensible backwards compatibility with older Django versions. - return user is not None and (not api_settings.CHECK_USER_IS_ACTIVE or user.is_active) + return user is not None and ( + not api_settings.CHECK_USER_IS_ACTIVE or user.is_active + ) From 8ab5c920371cfe394c6460e352fc0e55f3fd0781 Mon Sep 17 00:00:00 2001 From: zxkeyy Date: Sun, 8 Dec 2024 16:03:50 +0100 Subject: [PATCH 8/8] Update get_user to use new setting and remove unnecessary class --- rest_framework_simplejwt/authentication.py | 34 +--------------------- tests/test_authentication.py | 27 ----------------- 2 files changed, 1 insertion(+), 60 deletions(-) diff --git a/rest_framework_simplejwt/authentication.py b/rest_framework_simplejwt/authentication.py index e4be0f95e..d9ebc164f 100644 --- a/rest_framework_simplejwt/authentication.py +++ b/rest_framework_simplejwt/authentication.py @@ -131,8 +131,7 @@ def get_user(self, validated_token: Token) -> AuthUser: except self.user_model.DoesNotExist: raise AuthenticationFailed(_("User not found"), code="user_not_found") - # Ensure authentication rule passes - if not api_settings.USER_AUTHENTICATION_RULE(user): + if api_settings.CHECK_USER_IS_ACTIVE and not user.is_active: raise AuthenticationFailed(_("User is inactive"), code="user_inactive") if api_settings.CHECK_REVOKE_TOKEN: @@ -165,37 +164,6 @@ def get_user(self, validated_token: Token) -> AuthUser: return api_settings.TOKEN_USER_CLASS(validated_token) -class JWTInactiveUserAuthentication(JWTAuthentication): - """ - An authentication plugin that authenticates requests through a JSON web - token provided in a request header, allowing inactive users to authenticate. - """ - - def get_user(self, validated_token: Token) -> AuthUser: - """ - Attempts to find and return a user using the given validated token. - """ - try: - user_id = validated_token[api_settings.USER_ID_CLAIM] - except KeyError: - raise InvalidToken(_("Token contained no recognizable user identification")) - - try: - user = self.user_model.objects.get(**{api_settings.USER_ID_FIELD: user_id}) - except self.user_model.DoesNotExist: - raise AuthenticationFailed(_("User not found"), code="user_not_found") - - if api_settings.CHECK_REVOKE_TOKEN: - if validated_token.get( - api_settings.REVOKE_TOKEN_CLAIM - ) != get_md5_hash_password(user.password): - raise AuthenticationFailed( - _("The user's password has been changed."), code="password_changed" - ) - - return user - - JWTTokenUserAuthentication = JWTStatelessUserAuthentication diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 5ff4b9a63..cb6c3dc3f 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -266,30 +266,3 @@ def username(self): # Restore default TokenUser for future tests api_settings.TOKEN_USER_CLASS = temp - - -class TestJWTInactiveUserAuthentication(TestCase): - def setUp(self): - self.backend = authentication.JWTInactiveUserAuthentication() - - def test_get_user(self): - payload = {"some_other_id": "foo"} - - # Should raise error if no recognizable user identification - with self.assertRaises(InvalidToken): - self.backend.get_user(payload) - - payload[api_settings.USER_ID_CLAIM] = 42 - - # Should raise exception if user not found - with self.assertRaises(AuthenticationFailed): - self.backend.get_user(payload) - - u = User.objects.create_user(username="markhamill") - u.is_active = False - u.save() - - payload[api_settings.USER_ID_CLAIM] = getattr(u, api_settings.USER_ID_FIELD) - - # Otherwise, should return correct user - self.assertEqual(self.backend.get_user(payload).id, u.id)