From e0a3d937309a82b8beea8f41b17d8b6298da2a86 Mon Sep 17 00:00:00 2001 From: Alexander Gaevsky Date: Fri, 5 Feb 2016 16:46:19 +0200 Subject: [PATCH] Fixed #25232 -- Made ModelBackend/RemoteUserBackend reject inactive users. --- django/contrib/auth/backends.py | 28 ++++++++-- docs/howto/auth-remote-user.txt | 17 ++++-- docs/ref/contrib/auth.txt | 71 +++++++++++++++++++++++--- docs/releases/1.10.txt | 9 ++++ docs/topics/auth/customizing.txt | 19 +++++-- tests/auth_tests/models/__init__.py | 10 ++-- tests/auth_tests/models/custom_user.py | 9 ++++ tests/auth_tests/test_auth_backends.py | 57 ++++++++++++++++++--- tests/auth_tests/test_forms.py | 3 ++ tests/auth_tests/test_remote_user.py | 15 ++++++ tests/test_client/tests.py | 8 ++- 11 files changed, 216 insertions(+), 30 deletions(-) diff --git a/django/contrib/auth/backends.py b/django/contrib/auth/backends.py index 9f08259038..43d8627d0c 100644 --- a/django/contrib/auth/backends.py +++ b/django/contrib/auth/backends.py @@ -15,12 +15,21 @@ class ModelBackend(object): username = kwargs.get(UserModel.USERNAME_FIELD) try: user = UserModel._default_manager.get_by_natural_key(username) - if user.check_password(password): - return user except UserModel.DoesNotExist: # Run the default password hasher once to reduce the timing # difference between an existing and a non-existing user (#20760). UserModel().set_password(password) + else: + if user.check_password(password) and self.user_can_authenticate(user): + return user + + def user_can_authenticate(self, user): + """ + Reject users with is_active=False. Custom user models that don't have + that attribute are allowed. + """ + is_active = getattr(user, 'is_active', None) + return is_active or is_active is None def _get_user_permissions(self, user_obj): return user_obj.user_permissions.all() @@ -90,9 +99,15 @@ class ModelBackend(object): def get_user(self, user_id): UserModel = get_user_model() try: - return UserModel._default_manager.get(pk=user_id) + user = UserModel._default_manager.get(pk=user_id) except UserModel.DoesNotExist: return None + return user if self.user_can_authenticate(user) else None + + +class AllowAllUsersModelBackend(ModelBackend): + def user_can_authenticate(self, user): + return True class RemoteUserBackend(ModelBackend): @@ -140,7 +155,7 @@ class RemoteUserBackend(ModelBackend): user = UserModel._default_manager.get_by_natural_key(username) except UserModel.DoesNotExist: pass - return user + return user if self.user_can_authenticate(user) else None def clean_username(self, username): """ @@ -158,3 +173,8 @@ class RemoteUserBackend(ModelBackend): By default, returns the user unmodified. """ return user + + +class AllowAllUsersRemoteUserBackend(RemoteUserBackend): + def user_can_authenticate(self, user): + return True diff --git a/docs/howto/auth-remote-user.txt b/docs/howto/auth-remote-user.txt index 7cc1e1608a..1680baa92b 100644 --- a/docs/howto/auth-remote-user.txt +++ b/docs/howto/auth-remote-user.txt @@ -64,9 +64,20 @@ remote users. These interfaces work with users stored in the database regardless of ``AUTHENTICATION_BACKENDS``. .. note:: - Since the ``RemoteUserBackend`` inherits from ``ModelBackend``, you will - still have all of the same permissions checking that is implemented in - ``ModelBackend``. + + Since the ``RemoteUserBackend`` inherits from ``ModelBackend``, you will + still have all of the same permissions checking that is implemented in + ``ModelBackend``. + + Users with :attr:`is_active=False + ` won't be allowed to + authenticate. Use + :class:`~django.contrib.auth.backends.AllowAllUsersRemoteUserBackend` if + you want to allow them to. + + .. versionchanged:: 1.10 + + In older versions, inactive users weren't rejected as described above. If your authentication mechanism uses a custom HTTP header and not ``REMOTE_USER``, you can subclass ``RemoteUserMiddleware`` and set the diff --git a/docs/ref/contrib/auth.txt b/docs/ref/contrib/auth.txt index 778fb10b3e..831de3b5a6 100644 --- a/docs/ref/contrib/auth.txt +++ b/docs/ref/contrib/auth.txt @@ -76,15 +76,26 @@ Fields This doesn't necessarily control whether or not the user can log in. Authentication backends aren't required to check for the ``is_active`` - flag, and the default backends do not. If you want to reject a login - based on ``is_active`` being ``False``, it's up to you to check that in - your own login view or a custom authentication backend. However, the + flag but the default backend + (:class:`~django.contrib.auth.backends.ModelBackend`) and the + :class:`~django.contrib.auth.backends.RemoteUserBackend` do. You can + use :class:`~django.contrib.auth.backends.AllowAllUsersModelBackend` + or :class:`~django.contrib.auth.backends.AllowAllUsersRemoteUserBackend` + if you want to allow inactive users to login. In this case, you'll also + want to customize the :class:`~django.contrib.auth.forms.AuthenticationForm` used by the - :func:`~django.contrib.auth.views.login` view (which is the default) - *does* perform this check, as do the permission-checking methods such - as :meth:`~django.contrib.auth.models.User.has_perm` and the - authentication in the Django admin. All of those functions/methods will - return ``False`` for inactive users. + :func:`~django.contrib.auth.views.login` view as it rejects inactive + users. Be aware that the permission-checking methods such as + :meth:`~django.contrib.auth.models.User.has_perm` and the + authentication in the Django admin all return ``False`` for inactive + users. + + .. versionchanged:: 1.10 + + In older versions, + :class:`~django.contrib.auth.backends.ModelBackend` and + :class:`~django.contrib.auth.backends.RemoteUserBackend` allowed + inactive users to authenticate. .. attribute:: is_superuser @@ -488,6 +499,32 @@ The following backends are available in :mod:`django.contrib.auth.backends`: Returns whether the ``user_obj`` has any permissions on the app ``app_label``. + .. method:: ModelBackend.user_can_authenticate() + + .. versionadded:: 1.10 + + Returns whether the user is allowed to authenticate. To match the + behavior of :class:`~django.contrib.auth.forms.AuthenticationForm` + which :meth:`prohibits inactive users from logging in + `, + this method returns ``False`` for users with :attr:`is_active=False + `. Custom user models that + don't have an :attr:`~django.contrib.auth.models.CustomUser.is_active` + field are allowed. + +.. class:: AllowAllUsersModelBackend + + .. versionadded:: 1.10 + + Same as :class:`ModelBackend` except that it doesn't reject inactive users + because :meth:`~ModelBackend.user_can_authenticate` always returns ``True``. + + When using this backend, you'll likely want to customize the + :class:`~django.contrib.auth.forms.AuthenticationForm` used by the + :func:`~django.contrib.auth.views.login` view by overriding the + :meth:`~django.contrib.auth.forms.AuthenticationForm.confirm_login_allowed` + method as it rejects inactive users. + .. class:: RemoteUserBackend Use this backend to take advantage of external-to-Django-handled @@ -529,3 +566,21 @@ The following backends are available in :mod:`django.contrib.auth.backends`: new user is created, and can be used to perform custom setup actions, such as setting the user's groups based on attributes in an LDAP directory. Returns the user object. + +.. method:: RemoteUserBackend.user_can_authenticate() + + .. versionadded:: 1.10 + + Returns whether the user is allowed to authenticate. This method returns + ``False`` for users with :attr:`is_active=False + `. Custom user models that don't + have an :attr:`~django.contrib.auth.models.CustomUser.is_active` field are + allowed. + +.. class:: AllowAllUsersRemoteUserBackend + + .. versionadded:: 1.10 + + Same as :class:`RemoteUserBackend` except that it doesn't reject inactive + users because :attr:`~RemoteUserBackend.user_can_authenticate` always + returns ``True``. diff --git a/docs/releases/1.10.txt b/docs/releases/1.10.txt index d9bacc7ede..d6be8e2f09 100644 --- a/docs/releases/1.10.txt +++ b/docs/releases/1.10.txt @@ -669,6 +669,15 @@ Miscellaneous calling ``Command.execute()``, pass the command object as the first argument to ``call_command()``. +* :class:`~django.contrib.auth.backends.ModelBackend` and + :class:`~django.contrib.auth.backends.RemoteUserBackend` now reject inactive + users. This means that inactive users can't login and will be logged + out if they are switched from ``is_active=True`` to ``False``. If you need + the previous behavior, use the new + :class:`~django.contrib.auth.backends.AllowAllUsersModelBackend` or + :class:`~django.contrib.auth.backends.AllowAllUsersRemoteUserBackend` + in :setting:`AUTHENTICATION_BACKENDS` instead. + .. _deprecated-features-1.10: Features deprecated in 1.10 diff --git a/docs/topics/auth/customizing.txt b/docs/topics/auth/customizing.txt index e4b3d330ed..271d7f52f7 100644 --- a/docs/topics/auth/customizing.txt +++ b/docs/topics/auth/customizing.txt @@ -235,10 +235,17 @@ for example, to control anonymous access. Authorization for inactive users ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -An inactive user is a one that is authenticated but has its attribute -``is_active`` set to ``False``. However this does not mean they are not -authorized to do anything. For example they are allowed to activate their -account. +An inactive user is a one that has its +:attr:`~django.contrib.auth.models.User.is_active` field set to ``False``. The +:class:`~django.contrib.auth.backends.ModelBackend` and +:class:`~django.contrib.auth.backends.RemoteUserBackend` authentication +backends prohibits these users from authenticating. If a custom user model +doesn't have an :attr:`~django.contrib.auth.models.CustomUser.is_active` field, +all users will be allowed to authenticate. + +You can use :class:`~django.contrib.auth.backends.AllowAllUsersModelBackend` +or :class:`~django.contrib.auth.backends.AllowAllUsersRemoteUserBackend` if you +want to allow inactive users to authenticate. The support for anonymous users in the permission system allows for a scenario where anonymous users have permissions to do something while inactive @@ -247,6 +254,10 @@ authenticated users do not. Do not forget to test for the ``is_active`` attribute of the user in your own backend permission methods. +.. versionchanged:: 1.10 + + In older versions, the :class:`~django.contrib.auth.backends.ModelBackend` + allowed inactive users to authenticate. Handling object permissions ~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/tests/auth_tests/models/__init__.py b/tests/auth_tests/models/__init__.py index d7275e177f..d3e6c73d90 100644 --- a/tests/auth_tests/models/__init__.py +++ b/tests/auth_tests/models/__init__.py @@ -1,12 +1,14 @@ from .custom_permissions import CustomPermissionsUser -from .custom_user import CustomUser, ExtensionUser +from .custom_user import ( + CustomUser, CustomUserWithoutIsActiveField, ExtensionUser, +) from .invalid_models import CustomUserNonUniqueUsername from .is_active import IsActiveTestUser1 from .uuid_pk import UUIDUser from .with_foreign_key import CustomUserWithFK, Email __all__ = ( - 'CustomUser', 'CustomPermissionsUser', 'CustomUserWithFK', 'Email', - 'ExtensionUser', 'IsActiveTestUser1', 'UUIDUser', - 'CustomUserNonUniqueUsername', + 'CustomUser', 'CustomUserWithoutIsActiveField', 'CustomPermissionsUser', + 'CustomUserWithFK', 'Email', 'ExtensionUser', 'IsActiveTestUser1', + 'UUIDUser', 'CustomUserNonUniqueUsername', ) diff --git a/tests/auth_tests/models/custom_user.py b/tests/auth_tests/models/custom_user.py index 62dbf9c8be..469cada31e 100644 --- a/tests/auth_tests/models/custom_user.py +++ b/tests/auth_tests/models/custom_user.py @@ -97,6 +97,15 @@ class RemoveGroupsAndPermissions(object): PermissionsMixin._meta.local_many_to_many = self._old_pm_local_m2m +class CustomUserWithoutIsActiveField(AbstractBaseUser): + username = models.CharField(max_length=150, unique=True) + email = models.EmailField(unique=True) + + objects = UserManager() + + USERNAME_FIELD = 'username' + + # The extension user is a simple extension of the built-in user class, # adding a required date_of_birth field. This allows us to check for # any hard references to the name "User" in forms/handlers etc. diff --git a/tests/auth_tests/test_auth_backends.py b/tests/auth_tests/test_auth_backends.py index fee1a66bd4..e3c0109c96 100644 --- a/tests/auth_tests/test_auth_backends.py +++ b/tests/auth_tests/test_auth_backends.py @@ -15,7 +15,10 @@ from django.test import ( SimpleTestCase, TestCase, modify_settings, override_settings, ) -from .models import CustomPermissionsUser, CustomUser, ExtensionUser, UUIDUser +from .models import ( + CustomPermissionsUser, CustomUser, CustomUserWithoutIsActiveField, + ExtensionUser, UUIDUser, +) class CountingMD5PasswordHasher(MD5PasswordHasher): @@ -200,19 +203,35 @@ class ModelBackendTest(BaseModelBackendTest, TestCase): Tests for the ModelBackend using the default User model. """ UserModel = User + user_credentials = {'username': 'test', 'password': 'test'} def create_users(self): - self.user = User.objects.create_user( - username='test', - email='test@example.com', - password='test', - ) + self.user = User.objects.create_user(email='test@example.com', **self.user_credentials) self.superuser = User.objects.create_superuser( username='test2', email='test2@example.com', password='test', ) + def test_authenticate_inactive(self): + """ + An inactive user can't authenticate. + """ + self.assertEqual(authenticate(**self.user_credentials), self.user) + self.user.is_active = False + self.user.save() + self.assertIsNone(authenticate(**self.user_credentials)) + + @override_settings(AUTH_USER_MODEL='auth_tests.CustomUserWithoutIsActiveField') + def test_authenticate_user_without_is_active_field(self): + """ + A custom user without an `is_active` field is allowed to authenticate. + """ + user = CustomUserWithoutIsActiveField.objects._create_user( + username='test', email='test@example.com', password='test', + ) + self.assertEqual(authenticate(username='test', password='test'), user) + @override_settings(AUTH_USER_MODEL='auth_tests.ExtensionUser') class ExtensionUserModelBackendTest(BaseModelBackendTest, TestCase): @@ -676,3 +695,29 @@ class SelectingBackendTests(TestCase): user = User.objects.create_user(self.username, 'email', self.password) self.client._login(user, self.other_backend) self.assertBackendInSession(self.other_backend) + + +@override_settings(AUTHENTICATION_BACKENDS=['django.contrib.auth.backends.AllowAllUsersModelBackend']) +class AllowAllUsersModelBackendTest(TestCase): + """ + Inactive users may authenticate with the AllowAllUsersModelBackend. + """ + user_credentials = {'username': 'test', 'password': 'test'} + + @classmethod + def setUpTestData(cls): + cls.user = User.objects.create_user( + email='test@example.com', is_active=False, + **cls.user_credentials + ) + + def test_authenticate(self): + self.assertFalse(self.user.is_active) + self.assertEqual(authenticate(**self.user_credentials), self.user) + + def test_get_user(self): + self.client.force_login(self.user) + request = HttpRequest() + request.session = self.client.session + user = get_user(request) + self.assertEqual(user, self.user) diff --git a/tests/auth_tests/test_forms.py b/tests/auth_tests/test_forms.py index f43a814c88..0e0ba71504 100644 --- a/tests/auth_tests/test_forms.py +++ b/tests/auth_tests/test_forms.py @@ -166,6 +166,9 @@ class UserCreationFormTest(TestDataMixin, TestCase): self.assertEqual(form.cleaned_data['password2'], data['password2']) +# To verify that the login form rejects inactive users, use an authentication +# backend that allows them. +@override_settings(AUTHENTICATION_BACKENDS=['django.contrib.auth.backends.AllowAllUsersModelBackend']) class AuthenticationFormTest(TestDataMixin, TestCase): def test_invalid_username(self): diff --git a/tests/auth_tests/test_remote_user.py b/tests/auth_tests/test_remote_user.py index a413b97ee4..4e916d80ec 100644 --- a/tests/auth_tests/test_remote_user.py +++ b/tests/auth_tests/test_remote_user.py @@ -145,6 +145,11 @@ class RemoteUserTest(TestCase): # In backends that do not create new users, it is '' (anonymous user) self.assertNotEqual(response.context['user'].username, 'knownuser') + def test_inactive_user(self): + User.objects.create(username='knownuser', is_active=False) + response = self.client.get('/remote_user/', **{self.header: 'knownuser'}) + self.assertTrue(response.context['user'].is_anonymous()) + class RemoteUserNoCreateBackend(RemoteUserBackend): """Backend that doesn't create unknown users.""" @@ -166,6 +171,16 @@ class RemoteUserNoCreateTest(RemoteUserTest): self.assertEqual(User.objects.count(), num_users) +class AllowAllUsersRemoteUserBackendTest(RemoteUserTest): + """Backend that allows inactive users.""" + backend = 'django.contrib.auth.backends.AllowAllUsersRemoteUserBackend' + + def test_inactive_user(self): + user = User.objects.create(username='knownuser', is_active=False) + response = self.client.get('/remote_user/', **{self.header: self.known_user}) + self.assertEqual(response.context['user'].username, user.username) + + class CustomRemoteUserBackend(RemoteUserBackend): """ Backend that overrides RemoteUserBackend methods. diff --git a/tests/test_client/tests.py b/tests/test_client/tests.py index 741dd0be54..2ffc1d6896 100644 --- a/tests/test_client/tests.py +++ b/tests/test_client/tests.py @@ -437,6 +437,12 @@ class ClientTest(TestCase): login = self.client.login(username='inactive', password='password') self.assertFalse(login) + @override_settings( + AUTHENTICATION_BACKENDS=[ + 'django.contrib.auth.backends.ModelBackend', + 'django.contrib.auth.backends.AllowAllUsersModelBackend', + ] + ) def test_view_with_inactive_force_login(self): "Request a page that is protected with @login, but use an inactive login" @@ -445,7 +451,7 @@ class ClientTest(TestCase): self.assertRedirects(response, '/accounts/login/?next=/login_protected_view/') # Log in - self.client.force_login(self.u2) + self.client.force_login(self.u2, backend='django.contrib.auth.backends.AllowAllUsersModelBackend') # Request a page that requires a login response = self.client.get('/login_protected_view/')