Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fixed #17903 -- Modified ModelBackend to eliminate permissions on ina…

…ctive users.

Thanks to @SmileyChris for the report and @timgraham for review.
  • Loading branch information...
commit c33447a50c1b0a96c6e2261f7c45d2522a3fe28d 1 parent 0a8c0ed
@jorgecarleitao jorgecarleitao authored timgraham committed
View
4 django/contrib/auth/backends.py
@@ -35,7 +35,7 @@ def _get_permissions(self, user_obj, obj, from_name):
be either "group" or "user" to return permissions from
`_get_group_permissions` or `_get_user_permissions` respectively.
"""
- if user_obj.is_anonymous() or obj is not None:
+ if not user_obj.is_active or obj is not None:
return set()
perm_cache_name = '_%s_perm_cache' % from_name
@@ -63,7 +63,7 @@ def get_group_permissions(self, user_obj, obj=None):
return self._get_permissions(user_obj, obj, 'group')
def get_all_permissions(self, user_obj, obj=None):
- if user_obj.is_anonymous() or obj is not None:
+ if not user_obj.is_active or obj is not None:
return set()
if not hasattr(user_obj, '_perm_cache'):
user_obj._perm_cache = self.get_user_permissions(user_obj)
View
28 django/contrib/auth/tests/test_auth_backends.py
@@ -112,6 +112,34 @@ def test_has_no_object_perm(self):
self.assertEqual(user.has_perm('auth.test'), True)
self.assertEqual(user.get_all_permissions(), set(['auth.test']))
+ def test_inactive_has_no_permissions(self):
+ """
+ #17903 -- Inactive users shouldn't have permissions in
+ ModelBackend.get_(all|user|group)_permissions().
+ """
+ backend = ModelBackend()
+
+ user = self.UserModel._default_manager.get(pk=self.user.pk)
+ content_type = ContentType.objects.get_for_model(Group)
+ user_perm = Permission.objects.create(name='test', content_type=content_type, codename='test_user')
+ group_perm = Permission.objects.create(name='test2', content_type=content_type, codename='test_group')
+ user.user_permissions.add(user_perm)
+
+ group = Group.objects.create(name='test_group')
+ user.groups.add(group)
+ group.permissions.add(group_perm)
+
+ self.assertEqual(backend.get_all_permissions(user), set(['auth.test_user', 'auth.test_group']))
+ self.assertEqual(backend.get_user_permissions(user), set(['auth.test_user', 'auth.test_group']))
+ self.assertEqual(backend.get_group_permissions(user), set(['auth.test_group']))
+
+ user.is_active = False
+ user.save()
+
+ self.assertEqual(backend.get_all_permissions(user), set())
+ self.assertEqual(backend.get_user_permissions(user), set())
+ self.assertEqual(backend.get_group_permissions(user), set())
+
def test_get_all_superuser_permissions(self):
"""A superuser has all permissions. Refs #14795."""
user = self.UserModel._default_manager.get(pk=self.superuser.pk)
View
13 docs/ref/contrib/auth.txt
@@ -446,21 +446,20 @@ The following backends are available in :mod:`django.contrib.auth.backends`:
.. versionadded:: 1.8
Returns the set of permission strings the ``user_obj`` has from their
- own user permissions. Returns an empty set if the user
- :meth:`~django.contrib.auth.models.AbstractBaseUser.is_anonymous`.
+ own user permissions. Returns an empty set if the user is not
+ :meth:`active <django.contrib.auth.models.CustomUser.is_active>`.
.. method:: get_group_permissions(user_obj, obj=None)
Returns the set of permission strings the ``user_obj`` has from the
permissions of the groups they belong. Returns an empty set if the user
- :meth:`~django.contrib.auth.models.AbstractBaseUser.is_anonymous`.
+ is not :meth:`active <django.contrib.auth.models.CustomUser.is_active>`.
.. method:: get_all_permissions(user_obj, obj=None)
- Returns the set of permission strings the ``user_obj`` has, including
- both user permissions and groups permissions. Returns an empty set if
- the user
- :meth:`~django.contrib.auth.models.AbstractBaseUser.is_anonymous`.
+ Returns the set of permission strings the ``user_obj`` has, including both
+ user permissions and group permissions. Returns an empty set if the
+ user is not :meth:`active <django.contrib.auth.models.CustomUser.is_active>`.
.. method:: has_perm(user_obj, perm, obj=None)

3 comments on commit c33447a

@apollo13
Owner

@SmileyChris, @timgraham: Imo this is a security sensitive change and I'd prefer to still have the anonymous check there (eg custom users could have anonymous = true but is_active also true)

@timgraham
Owner

Done in 150d88c.

@jorgecarleitao

I'm sorry I underestimated the problem, and thanks for the report and the fix.

Please sign in to comment.
Something went wrong with that request. Please try again.