diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..96d3622 --- /dev/null +++ b/.gitignore @@ -0,0 +1,4 @@ +*.pyc +*.egg-info +*.sql +docs/build/* diff --git a/AUTHORS b/AUTHORS index 56d0532..8192db5 100644 --- a/AUTHORS +++ b/AUTHORS @@ -1,4 +1,7 @@ Jannis Leidel Martin Mahner Diego Búrigo Zacarão -James Pic \ No newline at end of file +James Pic +Wes Winham +Kyle Gibson +Jason Ward diff --git a/README b/README index 3ff0ede..0040d13 100644 --- a/README +++ b/README @@ -52,6 +52,14 @@ html version using the setup.py:: Changelog: ========== +0.5dev (2012-09-24): +----------------- + +* It is now possible to minimize the number of queries when using + django-authority by caching the results of the Permission query. This can be + done by adding ``AUTHORITY_USE_SMART_CACHE = True`` to your settings.py + + 0.4 (2010-01-15): ----------------- diff --git a/authority/managers.py b/authority/managers.py index 218158a..28aa7a7 100644 --- a/authority/managers.py +++ b/authority/managers.py @@ -2,6 +2,7 @@ from django.db import models from django.db.models import Q from django.contrib.contenttypes.models import ContentType + class PermissionManager(models.Manager): def get_content_type(self, obj): @@ -13,25 +14,37 @@ class PermissionManager(models.Manager): def for_object(self, obj, approved=True): return self.get_for_model(obj).select_related( 'user', 'creator', 'group', 'content_type' - ).filter(object_id=obj.id,approved=approved) + ).filter(object_id=obj.id, approved=approved) def for_user(self, user, obj, check_groups=True): perms = self.get_for_model(obj) if not check_groups: return perms.select_related('user', 'creator').filter(user=user) - return perms.select_related('user', 'user__groups', 'creator').filter( - Q(user=user) | Q(group__in=user.groups.all())) - def user_permissions(self, user, perm, obj, approved=True, check_groups=True): - return self.for_user(user, obj, check_groups).filter(codename=perm, - approved=approved) + # Hacking user to user__pk to workaround deepcopy bug: + # http://bugs.python.org/issue2460 + # Which is triggered by django's deepcopy which backports that fix in + # Django 1.2 + return perms.select_related('user', 'user__groups', 'creator').filter( + Q(user__pk=user.pk) | Q(group__in=user.groups.all())) + + def user_permissions( + self, user, perm, obj, approved=True, check_groups=True): + return self.for_user( + user, + obj, + check_groups, + ).filter( + codename=perm, + approved=approved, + ) def group_permissions(self, group, perm, obj, approved=True): """ Get objects that have Group perm permission on """ return self.get_for_model(obj).select_related( - 'user', 'group', 'creator').filter(group=group, codename=perm, + 'user', 'group', 'creator').filter(group=group, codename=perm, approved=approved) def delete_objects_permissions(self, obj): @@ -50,5 +63,3 @@ class PermissionManager(models.Manager): return perms = self.user_permissions(user, perm, obj).filter(object_id=obj.id) perms.delete() - - \ No newline at end of file diff --git a/authority/permissions.py b/authority/permissions.py index 434ae2b..62869d4 100644 --- a/authority/permissions.py +++ b/authority/permissions.py @@ -1,13 +1,14 @@ -import copy - -from django.db.models.base import Model, ModelBase -from django.template.defaultfilters import slugify +from django.conf import settings from django.contrib.auth.models import Permission as DjangoPermission from django.contrib.contenttypes.models import ContentType +from django.db.models import Q +from django.db.models.base import Model, ModelBase +from django.template.defaultfilters import slugify from authority.exceptions import NotAModel, UnsavedModelInstance from authority.models import Permission + class PermissionMetaclass(type): """ Used to generate the default set of permission checks "add", "change" and @@ -25,6 +26,7 @@ class PermissionMetaclass(type): new_class.checks = [check.lower() for check in new_class.checks] return new_class + class BasePermission(object): """ Base Permission class to be used to define app permissions. @@ -40,16 +42,146 @@ class BasePermission(object): self.group = group super(BasePermission, self).__init__(*args, **kwargs) - def has_user_perms(self, perm, obj, approved, check_groups=True): + def _get_cached_perms(self): + """ + Set up both the user and group caches. + """ + if not self.user: + return {}, {} + perms = Permission.objects.filter( + Q(user__pk=self.user.pk) | Q(group__in=self.user.groups.all()), + ) + user_permissions = {} + group_permissions = {} + for perm in perms: + if perm.user_id == self.user.pk: + user_permissions[( + perm.object_id, + perm.content_type_id, + perm.codename, + perm.approved, + )] = True + # If the user has the permission do for something, but perm.user != + # self.user then by definition that permission came from the + # group. + else: + group_permissions[( + perm.object_id, + perm.content_type_id, + perm.codename, + perm.approved, + )] = True + return user_permissions, group_permissions + + def _prime_perm_caches(self): + """ + Prime both the user and group caches and put them on the ``self.user``. + In addition add a cache filled flag on ``self.user``. + """ + perm_cache, group_perm_cache = self._get_cached_perms() + self.user._authority_perm_cache = perm_cache + self.user._authority_group_perm_cache = group_perm_cache + self.user._authority_perm_cache_filled = True + + @property + def _perm_cache(self): + """ + cached_permissions will generate the cache in a lazy fashion. + """ + # Check to see if the cache has been primed. + if not self.user: + return {} + cache_filled = getattr( + self.user, + '_authority_perm_cache_filled', + False, + ) + if cache_filled: + # Don't really like the name for this, but this matches how Django + # does it. + return self.user._authority_perm_cache + + # Prime the cache. + self._prime_perm_caches() + return self.user._authority_perm_cache + + @property + def _group_perm_cache(self): + """ + cached_permissions will generate the cache in a lazy fashion. + """ + # Check to see if the cache has been primed. + if not self.user: + return {} + cache_filled = getattr( + self.user, + '_authority_perm_cache_filled', + False, + ) + if cache_filled: + return self.user._authority_group_perm_cache + + # Prime the cache. + self._prime_perm_caches() + return self.user._authority_group_perm_cache + + def invalidate_permissions_cache(self): + """ + In the event that the Permission table is changed during the use of a + permission the Permission cache will need to be invalidated and + regenerated. By calling this method the invalidation will occur, and + the next time the cached_permissions is used the cache will be + re-primed. + """ if self.user: - if self.user.is_superuser: + self.user._authority_perm_cache_filled = False + + @property + def use_smart_cache(self): + # AUTHORITY_USE_SMART_CACHE defaults to False to maintain backwards + # compatibility. + use_smart_cache = getattr(settings, 'AUTHORITY_USE_SMART_CACHE', True) + return self.user and use_smart_cache + + def has_user_perms(self, perm, obj, approved, check_groups=True): + if not self.user: + return False + if self.user.is_superuser: + return True + if not self.user.is_active: + return False + + if self.use_smart_cache: + content_type_pk = Permission.objects.get_content_type(obj).pk + + def _user_has_perms(cached_perms): + # Check to see if the permission is in the cache. + return cached_perms.get(( + obj.pk, + content_type_pk, + perm, + approved, + )) + + # Check to see if the permission is in the cache. + if _user_has_perms(self._perm_cache): return True - if not self.user.is_active: - return False - # check if a Permission object exists for the given params - return Permission.objects.user_permissions(self.user, perm, obj, - approved, check_groups).filter(object_id=obj.id) - return False + + # Optionally check group permissions + if check_groups: + return _user_has_perms(self._group_perm_cache) + return False + + # Actually hit the DB, no smart cache used. + return Permission.objects.user_permissions( + self.user, + perm, + obj, + approved, + check_groups, + ).filter( + object_id=obj.pk, + ).exists() def has_group_perms(self, perm, obj, approved): """ @@ -58,7 +190,7 @@ class BasePermission(object): if self.group: perms = Permission.objects.group_permissions(self.group, perm, obj, approved) - return perms.filter(object_id=obj.id) + return perms.filter(object_id=obj.pk) return False def has_perm(self, perm, obj, check_groups=True, approved=True): @@ -97,19 +229,26 @@ class BasePermission(object): perms = perms or self.has_perm(perm, obj) return perms - def get_django_codename(self, check, model_or_instance, generic=False, without_left=False): + def get_django_codename( + self, check, model_or_instance, generic=False, without_left=False): if without_left: perm = check else: perm = '%s.%s' % (model_or_instance._meta.app_label, check.lower()) if generic: - perm = '%s_%s' % (perm, model_or_instance._meta.object_name.lower()) + perm = '%s_%s' % ( + perm, + model_or_instance._meta.object_name.lower(), + ) return perm def get_codename(self, check, model_or_instance, generic=False): perm = '%s.%s' % (self.label, check.lower()) if generic: - perm = '%s_%s' % (perm, model_or_instance._meta.object_name.lower()) + perm = '%s_%s' % ( + perm, + model_or_instance._meta.object_name.lower(), + ) return perm def assign(self, check=None, content_object=None, generic=False): @@ -150,26 +289,37 @@ class BasePermission(object): for check in checks: if isinstance(content_object, Model): # make an authority per object permission - codename = self.get_codename(check, content_object, generic) + codename = self.get_codename( + check, + content_object, + generic, + ) try: perm = Permission.objects.get( - user = self.user, - codename = codename, - approved = True, - content_type = content_type, - object_id = content_object.pk) + user=self.user, + codename=codename, + approved=True, + content_type=content_type, + object_id=content_object.pk, + ) except Permission.DoesNotExist: perm = Permission.objects.create( - user = self.user, - content_object = content_object, - codename = codename, - approved = True) + user=self.user, + content_object=content_object, + codename=codename, + approved=True, + ) result.append(perm) elif isinstance(content_object, ModelBase): # make a Django permission - codename = self.get_django_codename(check, content_object, generic, without_left=True) + codename = self.get_django_codename( + check, + content_object, + generic, + without_left=True, + ) try: perm = DjangoPermission.objects.get(codename=codename) except DjangoPermission.DoesNotExist: @@ -177,9 +327,10 @@ class BasePermission(object): if '_' in name: name = name[0:name.find('_')] perm = DjangoPermission( - name = name, - codename = codename, - content_type = content_type) + name=name, + codename=codename, + content_type=content_type, + ) perm.save() self.user.user_permissions.add(perm) result.append(perm) diff --git a/authority/tests.py b/authority/tests.py index 45350a6..79d8d46 100644 --- a/authority/tests.py +++ b/authority/tests.py @@ -1,19 +1,31 @@ -from django.test import TestCase -from django.contrib.auth.models import User +from django.conf import settings from django.contrib.auth.models import Permission as DjangoPermission +from django.contrib.auth.models import User, Group +from django.test import TestCase import authority from authority import permissions from authority.models import Permission from authority.exceptions import NotAModel, UnsavedModelInstance + class UserPermission(permissions.BasePermission): checks = ('browse',) label = 'user_permission' authority.register(User, UserPermission) -class BehaviourTest(TestCase): + +class GroupPermission(permissions.BasePermission): + checks = ('browse',) + label = 'group_permission' +authority.register(Group, GroupPermission) + + +class DjangoPermissionChecksTestCase(TestCase): """ + Django permission objects have certain methods that are always present, + test those here. + self.user will be given: - django permission add_user (test_add) - authority to delete_user which is him (test_delete) @@ -21,7 +33,7 @@ class BehaviourTest(TestCase): This permissions are given in the test case and not in the fixture, for later reference. """ - fixtures = ['tests.json',] + fixtures = ['tests.json'] def setUp(self): self.user = User.objects.get(username='jezdez') @@ -53,39 +65,44 @@ class BehaviourTest(TestCase): self.assertFalse(self.check.delete_user()) self.assertTrue(self.check.delete_user(self.user)) + class AssignBehaviourTest(TestCase): - """ - self.user will be given: - - permission add_user (test_add), - - permission delete_user for him (test_delete), - - all existing codenames permissions: a/b/c/d (test_all), - """ - fixtures = ['tests.json',] + """ + self.user will be given: + - permission add_user (test_add), + - permission delete_user for him (test_delete), + - all existing codenames permissions: a/b/c/d (test_all), + """ + fixtures = ['tests.json'] - def setUp(self): - self.user = User.objects.get(username='jezdez') - self.check = UserPermission(self.user) + def setUp(self): + self.user = User.objects.get(username='jezdez') + self.check = UserPermission(self.user) - def test_add(self): - result = self.check.assign(check='add_user') + def test_add(self): + result = self.check.assign(check='add_user') - self.assertTrue(isinstance(result[0], DjangoPermission)) - self.assertTrue(self.check.add_user()) + self.assertTrue(isinstance(result[0], DjangoPermission)) + self.assertTrue(self.check.add_user()) - def test_delete(self): - result = self.check.assign(content_object=self.user, check='delete_user') + def test_delete(self): + result = self.check.assign( + content_object=self.user, + check='delete_user', + ) - self.assertTrue(isinstance(result[0], Permission)) - self.assertFalse(self.check.delete_user()) - self.assertTrue(self.check.delete_user(self.user)) + self.assertTrue(isinstance(result[0], Permission)) + self.assertFalse(self.check.delete_user()) + self.assertTrue(self.check.delete_user(self.user)) + + def test_all(self): + result = self.check.assign(content_object=self.user) + self.assertTrue(isinstance(result, list)) + self.assertTrue(self.check.browse_user(self.user)) + self.assertTrue(self.check.delete_user(self.user)) + self.assertTrue(self.check.add_user(self.user)) + self.assertTrue(self.check.change_user(self.user)) - def test_all(self): - result = self.check.assign(content_object=self.user) - self.assertTrue(isinstance(result, list)) - self.assertTrue(self.check.browse_user(self.user)) - self.assertTrue(self.check.delete_user(self.user)) - self.assertTrue(self.check.add_user(self.user)) - self.assertTrue(self.check.change_user(self.user)) class GenericAssignBehaviourTest(TestCase): """ @@ -93,7 +110,7 @@ class GenericAssignBehaviourTest(TestCase): - permission add (test_add), - permission delete for him (test_delete), """ - fixtures = ['tests.json',] + fixtures = ['tests.json'] def setUp(self): self.user = User.objects.get(username='jezdez') @@ -106,17 +123,23 @@ class GenericAssignBehaviourTest(TestCase): self.assertTrue(self.check.add_user()) def test_delete(self): - result = self.check.assign(content_object=self.user, check='delete', generic=True) + result = self.check.assign( + content_object=self.user, + check='delete', + generic=True, + ) self.assertTrue(isinstance(result[0], Permission)) self.assertFalse(self.check.delete_user()) self.assertTrue(self.check.delete_user(self.user)) + class AssignExceptionsTest(TestCase): """ - Tests that exceptions are thrown if assign() was called with inconsistent arguments. + Tests that exceptions are thrown if assign() was called with inconsistent + arguments. """ - fixtures = ['tests.json',] + fixtures = ['tests.json'] def setUp(self): self.user = User.objects.get(username='jezdez') @@ -135,3 +158,193 @@ class AssignExceptionsTest(TestCase): except NotAModel: return True self.fail() + + +class SmartCachingTestCase(TestCase): + """ + The base test case for all tests that have to do with smart caching. + """ + fixtures = ['tests.json'] + + def setUp(self): + # Create a user. + self.user = User.objects.get(username='jezdez') + + # Create a group. + self.group = Group.objects.create() + self.group.user_set.add(self.user) + + # Make the checks + self.user_check = UserPermission(user=self.user) + self.group_check = GroupPermission(group=self.group) + + # Ensure we are using the smart cache. + settings.AUTHORITY_USE_SMART_CACHE = True + + def _old_permission_check(self): + # This is what the old, pre-cache system would check to see if a user + # had a given permission. + return Permission.objects.user_permissions( + self.user, + 'foo', + self.user, + approved=True, + check_groups=True, + ) + + +class PerformanceTest(SmartCachingTestCase): + """ + Tests that permission are actually cached and that the number of queries + stays constant. + """ + + def test_has_user_perms(self): + # Show that when calling has_user_perms multiple times no additional + # queries are done. + + # Make sure the has_user_perms check does not get short-circuited. + assert not self.user.is_superuser + assert self.user.is_active + + # Regardless of how many times has_user_perms is called, the number of + # queries is the same. + with self.assertNumQueries(1): + self.user_check.has_user_perms('foo', self.user, True, False) + self.user_check.has_user_perms('foo', self.user, True, False) + self.user_check.has_user_perms('foo', self.user, True, False) + + def test_has_user_perms_check_group(self): + # Regardless of the number groups permissions, it should only take one + # query to check both users and groups. + with self.assertNumQueries(1): + self.user_check.has_user_perms( + 'foo', + self.user, + approved=True, + check_groups=True, + ) + + def test_invalidate_permissions_cache(self): + # Show that calling invalidate_permissions_cache will cause extra + # queries. + with self.assertNumQueries(2): + self.user_check.has_user_perms('foo', self.user, True, False) + + # Invalidate the cache to show that a query will be generated when + # checking perms again. + self.user_check.invalidate_permissions_cache() + + # One query to re generate the cache. + self.user_check.has_user_perms('foo', self.user, True, False) + + def test_has_user_perms_check_group_multiple(self): + # Create a permission with just a group. + Permission.objects.create( + content_type=Permission.objects.get_content_type(User), + object_id=self.user.pk, + codename='foo', + group=self.group, + approved=True, + ) + + # Check the number of queries. + with self.assertNumQueries(1): + self.user_check.has_user_perms('foo', self.user, True, True) + + # Create a second group. + new_group = Group.objects.create(name='new_group') + new_group.user_set.add(self.user) + + # Create a permission object for it. + Permission.objects.create( + content_type=Permission.objects.get_content_type(User), + object_id=self.user.pk, + codename='foo', + group=new_group, + approved=True, + ) + + self.user_check.invalidate_permissions_cache() + + # Make sure it is the same number of queries. + with self.assertNumQueries(1): + self.user_check.has_user_perms('foo', self.user, True, True) + + +class GroupPermissionCacheTestCase(SmartCachingTestCase): + """ + Tests that peg expected behaviour + """ + + def test_has_user_perms_with_groups(self): + perms = self._old_permission_check() + self.assertEqual([], list(perms)) + + # Use the new cached user perms to show that the user does not have the + # perms. + can_foo_with_group = self.user_check.has_user_perms( + 'foo', + self.user, + approved=True, + check_groups=True, + ) + self.assertFalse(can_foo_with_group) + + # Create a permission with just that group. + perm = Permission.objects.create( + content_type=Permission.objects.get_content_type(User), + object_id=self.user.pk, + codename='foo', + group=self.group, + approved=True, + ) + + # Old permission check + perms = self._old_permission_check() + self.assertEqual([perm], list(perms)) + + # Invalidate the cache. + self.user_check.invalidate_permissions_cache() + can_foo_with_group = self.user_check.has_user_perms( + 'foo', + self.user, + approved=True, + check_groups=True, + ) + self.assertTrue(can_foo_with_group) + + def test_has_group_perms_no_user(self): + # Make sure calling has_user_perms on a permission that does not have a + # user does not throw any errors. + can_foo_with_group = self.group_check.has_group_perms( + 'foo', + self.user, + approved=True, + ) + self.assertFalse(can_foo_with_group) + + self.assertEqual(self.group_check._perm_cache, {}) + self.assertEqual(self.group_check._group_perm_cache, {}) + + # Create a permission with just that group. + Permission.objects.create( + content_type=Permission.objects.get_content_type(User), + object_id=self.user.pk, + codename='foo', + group=self.group, + approved=True, + ) + + # Invalidate the cache. + self.group_check.invalidate_permissions_cache() + + can_foo_with_group = self.group_check.has_group_perms( + 'foo', + self.user, + approved=True, + ) + self.assertTrue(can_foo_with_group) + + self.assertEqual(self.group_check._perm_cache, {}) + self.assertEqual(self.group_check._group_perm_cache, {}) diff --git a/docs/conf.py b/docs/conf.py index 1137de2..84cd272 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -46,10 +46,10 @@ copyright = u'2009, the django-authority team' # built documents. # # The short X.Y version. -version = '0.4' +version = '0.5' # The full version, including alpha/beta/rc tags. -release = '0.4dev' +release = '0.5dev' # The language for content autogenerated by Sphinx. Refer to documentation # for a list of supported languages. diff --git a/docs/configuration.txt b/docs/configuration.txt index bcf734a..c0aa19f 100644 --- a/docs/configuration.txt +++ b/docs/configuration.txt @@ -32,6 +32,11 @@ context processors:: 'django.core.context_processors.request', ) +django-authority defaults to using a smart cache when checking permissions. +This can be disabled by adding the following line to ``settings.py``:: + + AUTHORITY_USE_SMART_CACHE = False + urls.py ======= diff --git a/docs/index.txt b/docs/index.txt index 6f9fda9..baa244c 100644 --- a/docs/index.txt +++ b/docs/index.txt @@ -23,6 +23,11 @@ This application provides three abilities: voodoo-code to Django's ``contrib.auth`` system, it keeps your existing permission system intact! +django-authority uses a cache that is stored on the user object to help improve +performance. However, if the ``Permission`` table changes the cache will need +to be invalidated. More information about this can be found in the tips and +tricks section. + .. warning:: We have just started with the documentation and it's far from being perfect. If you find glitches, errors or just have feedback, please contact the team: :ref:`support`. diff --git a/docs/tips_tricks.txt b/docs/tips_tricks.txt index 6f77da7..ff5ae71 100644 --- a/docs/tips_tricks.txt +++ b/docs/tips_tricks.txt @@ -31,3 +31,38 @@ Within a permission class, you can refer to Django's basic permissions:: # ... authority.register(Flatpage, FlagpagePermisson) + +If the ``Permission`` table changes during the lifespan of a django-authority +permission instance and the smart cache is being used, you will need to call +invalidate_permissions_cache in order to see that changes:: + + class UserPermission(permission.BasePermission): + label = 'user_permission' + checks = ('do_foo',) + authority.register(User, UserPermission) + + user_permission = UserPermission(user) + + # can_foo is False here since the permission has not yet been added. + can_foo = user_permission.has_user_perms('foo', user) + + Permission.objects.create( + content_type=Permission.objects.get_content_type(User), + object_id=user.pk, + codename='foo', + user=user, + approved=True, + ) + + # can_foo is still False because the permission cache has not been + invalidated yet. + can_foo = user_permission.has_user_perms('foo', user) + + user_permission.invalidate_permissions_cache() + + # can_foo is now True + can_foo = user_permission.has_user_perms('foo', user) + +This is particularly useful if you are using the permission instances during a +request, where it is unlikely that the state of the ``Permission`` table will +change.