Forward porting of #1128

cms/menu.py

@@ -74,12 +74,8 @@ def get_visible_pages(request, pages, site=None):
 
     # authenticated user and global permission
     if is_auth_user:
-        query = dict()
-        query['group__' + user_related_query_name] = request.user
-        global_page_perm_q = Q(
-            Q(user=request.user) | Q(**query)
-        ) & Q(can_view=True) & Q(Q(sites__in=[site.pk]) | Q(sites__isnull=True))
-        global_view_perms = GlobalPagePermission.objects.filter(global_page_perm_q).exists()
+        global_view_perms = GlobalPagePermission.objects.user_has_view_permission(
+            request.user, site.pk).exists()
 
         #no page perms edge case - all visible
         if ((is_setting_public_all or (

cms/models/managers.py

@@ -217,7 +217,8 @@ def set_or_create(self, request, page, form, language):
 class BasicPagePermissionManager(models.Manager):
     """Global page permission manager accessible under objects.
 
-    !IMPORTANT: take care, PagePermissionManager extends this manager
+    !IMPORTANT: take care, PagePermissionManager and GlobalPagePermissionManager
+    both inherit from this manager
     """
 
     def with_user(self, user):
@@ -237,6 +238,29 @@ def with_can_change_permissions(self, user):
         return self.with_user(user).filter(can_change_permissions=True)
 
 
+class GlobalPagePermissionManager(BasicPagePermissionManager):
+
+    def user_has_permission(self, user, site_id, perm):
+        """
+        Provide a single point of entry for deciding whether any given global
+        permission exists.
+        """
+        # if the user has add rights to this site explicitly
+        this_site = Q(**{perm: True, 'sites__in':[site_id]})
+        # if the user can add to all sites
+        all_sites = Q(**{perm: True, 'sites__isnull': True})
+        return self.with_user(user).filter(this_site | all_sites)
+
+    def user_has_add_permission(self, user, site_id):
+        return self.user_has_permission(user, site_id, 'can_add')
+
+    def user_has_change_permission(self, user, site_id):
+        return self.user_has_permission(user, site_id, 'can_change')
+
+    def user_has_view_permission(self, user, site_id):
+        return self.user_has_permission(user, site_id, 'can_view')
+
+
 class PagePermissionManager(BasicPagePermissionManager):
     """Page permission manager accessible under objects.
     """
@@ -463,10 +487,8 @@ def __get_id_list(self, user, site, attr):
         if cached is not None:
             return cached
             # check global permissions
-        global_permissions = GlobalPagePermission.objects.with_user(user)
-        if global_permissions.filter(**{
-            attr: True, 'sites__in': [site]
-        }).exists():
+        global_perm = GlobalPagePermission.objects.user_has_permission(user, site, attr).exists()
+        if global_perm:
             # user or his group are allowed to do `attr` action
             # !IMPORTANT: page permissions must not override global permissions
             return PagePermissionsPermissionManager.GRANT_ALL

cms/models/pagemodel.py

@@ -936,12 +936,8 @@ def has_view_permission(self, request):
         # inherited and direct
         is_restricted = PagePermission.objects.for_page(page=self).filter(can_view=True).exists()
         if request.user.is_authenticated():
-            site = current_site(request)
-            global_perms_q = Q(can_view=True) & Q(
-                Q(sites__in=[site]) | Q(sites__isnull=True)
-            )
-            global_view_perms = GlobalPagePermission.objects.with_user(
-                request.user).filter(global_perms_q).exists()
+            global_view_perms = GlobalPagePermission.objects.user_has_view_permission(
+                request.user, current_site(request)).exists()
 
             # a global permission was given to the request's user
             if global_view_perms:

cms/models/permissionmodels.py

@@ -10,7 +10,9 @@
 
 from cms.compat import is_user_swapped, user_model_label
 from cms.models import Page
-from cms.models.managers import BasicPagePermissionManager, PagePermissionManager
+from cms.models.managers import (BasicPagePermissionManager,
+                                 PagePermissionManager,
+                                 GlobalPagePermissionManager)
 from cms.utils.helpers import reversion_register
 from cms.utils.compat.dj import force_unicode, python_2_unicode_compatible
 
@@ -51,7 +53,7 @@
     (ACCESS_PAGE_AND_CHILDREN, _('Page and children (immediate)')),
     (ACCESS_DESCENDANTS, _('Page descendants')),
     (ACCESS_PAGE_AND_DESCENDANTS, _('Page and descendants')),
-    )
+)
 
 class AbstractPagePermission(models.Model):
     """Abstract page permissions
@@ -95,7 +97,7 @@ class GlobalPagePermission(AbstractPagePermission):
     can_recover_page = models.BooleanField(_("can recover pages"), default=True, help_text=_("can recover any deleted page"))
     sites = models.ManyToManyField(Site, null=True, blank=True, help_text=_('If none selected, user haves granted permissions to all sites.'), verbose_name=_('sites'))
 
-    objects = BasicPagePermissionManager()
+    objects = GlobalPagePermissionManager()
 
     class Meta:
         verbose_name = _('Page global permission')

cms/tests/admin.py

@@ -1055,7 +1055,6 @@ def test_plugins_copy_placeholder_ref(self):
         self.assertEqual(CMSPlugin.objects.count(), 4)
         self.assertEqual(Placeholder.objects.count(), 3)
 
-
     def test_plugins_copy_language(self):
         """User tries to copy plugin but has no permissions. He can copy plugins after he got the permissions"""
         plugin = self._create_plugin()
@@ -1275,7 +1274,7 @@ def test_render_edit_mode(self):
         user = self.get_superuser()
         self.assertEqual(Placeholder.objects.all().count(), 4)
         with self.login_user_context(user):
-            with self.assertNumQueries(FuzzyInt(40, 63)):
+            with self.assertNumQueries(FuzzyInt(40, 65)):
                 output = force_unicode(self.client.get('/en/?edit').content)
             self.assertIn('<b>Test</b>', output)
             self.assertEqual(Placeholder.objects.all().count(), 9)
@@ -1301,7 +1300,7 @@ def test_tree_view_queries(self):
 
         user = self.get_superuser()
         with self.login_user_context(user):
-            with self.assertNumQueries(FuzzyInt(14, 25)):
+            with self.assertNumQueries(FuzzyInt(18, 33)):
                 output = force_unicode(self.client.get('/en/admin/cms/page/'))
 
 

cms/tests/permmod.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 from __future__ import with_statement
 
-import urllib
+from cms.admin.forms import save_permissions
 from cms.constants import PUBLISHER_STATE_PENDING
 
 from cms.management.commands.subcommands.moderator import log
@@ -12,19 +12,25 @@
 from cms.compat import get_user_model, user_related_name
 
 from cms.models import Page, CMSPlugin, Title
-from cms.models.permissionmodels import ACCESS_DESCENDANTS, ACCESS_PAGE_AND_DESCENDANTS
+from cms.models.permissionmodels import (ACCESS_DESCENDANTS,
+                                         ACCESS_PAGE_AND_DESCENDANTS)
 from cms.models.permissionmodels import PagePermission, GlobalPagePermission
 from cms.plugin_pool import plugin_pool
 from cms.test_utils.testcases import (URL_CMS_PAGE_ADD, URL_CMS_PLUGIN_REMOVE,
                                       SettingsOverrideTestCase,
                                       URL_CMS_PLUGIN_ADD, CMSTestCase)
 from cms.test_utils.util.context_managers import SettingsOverride, disable_logger
+from cms.test_utils.util.request_factory import RequestFactory
+from cms.test_utils.util.fuzzy_int import FuzzyInt
 from cms.utils.i18n import force_language
 from cms.utils.page_resolver import get_page_from_path
-from cms.utils.permissions import has_generic_permission
+from cms.utils.permissions import (has_page_add_permission,
+                                   has_page_change_permission,
+                                   has_generic_permission)
 from cms.utils.compat.urls import unquote
 
-from django.contrib.auth.models import Permission, AnonymousUser, Group
+from django.contrib.admin.sites import site
+from django.contrib.auth.models import User, Permission, AnonymousUser, Group
 from django.contrib.sites.models import Site
 from django.core.management import call_command
 from django.core.urlresolvers import reverse
@@ -1023,3 +1029,114 @@ def test_page_permission_cache_invalidation(self):
         # the permission_user_cache attribute set
         page = Page.objects.get(pk=page.pk)
         self.assertFalse(page.has_change_permissions_permission(request))
+
+
+class GlobalPermissionTests(SettingsOverrideTestCase):
+
+    def test_sanity_check(self):
+        """ Because we have a new manager, we'll do some basic checks."""
+        # manager is still named the same.
+        self.assertTrue(hasattr(GlobalPagePermission, 'objects'))
+        self.assertEqual(0, GlobalPagePermission.objects.all().count())
+
+        # we are correctly inheriting from BasicPagePermissionManager
+        self.assertTrue(hasattr(GlobalPagePermission.objects, 'with_user'))
+
+        # If we're using the new manager, we have extra methods which ensure
+        # This site access OR all site access.
+        self.assertTrue(hasattr(GlobalPagePermission.objects, 'user_has_permission'))
+        # these are just convienence methods for the above.
+        self.assertTrue(hasattr(GlobalPagePermission.objects, 'user_has_add_permission'))
+        self.assertTrue(hasattr(GlobalPagePermission.objects, 'user_has_change_permission'))
+        self.assertTrue(hasattr(GlobalPagePermission.objects, 'user_has_view_permission'))
+
+    def test_emulate_admin_index(self):
+        """ Call methods that emulate the adminsite instance's index.
+        This test was basically the reason for the new manager, in light of the
+        problem highlighted in ticket #1120, which asserts that giving a user
+        no site-specific rights when creating a GlobalPagePermission should
+        allow access to all sites.
+        """
+        # create and then ignore this user.
+        superuser = self._create_user("super", is_staff=True, is_active=True,
+                                      is_superuser=True)
+        superuser.set_password("super")
+        superuser.save()
+        # create 2 staff users
+        SITES = [
+            Site.objects.get(pk=1),
+            Site.objects.create(domain='example2.com', name='example2.com'),
+        ]
+        USERS = [
+            self._create_user("staff", is_staff=True, is_active=True),
+            self._create_user("staff_2", is_staff=True, is_active=True),
+        ]
+        for user in USERS:
+            user.set_password('staff')
+            # re-use the same methods the UserPage form does.
+            # Note that it internally calls .save(), as we've not done so.
+            save_permissions({
+                'can_add_page': True,
+                'can_change_page': True,
+                'can_delete_page': False
+            }, user)
+
+        GlobalPagePermission.objects.create(can_add=True, can_change=True,
+                                            can_delete=False, user=USERS[0])
+        # we're querying here to ensure that even though we've created two users
+        # above, we should have successfully filtered to just one perm.
+        self.assertEqual(1, GlobalPagePermission.objects.with_user(USERS[0]).count())
+
+        # this will confirm explicit permissions still work, by adding the first
+        # site instance to the many2many relationship 'sites'
+        GlobalPagePermission.objects.create(can_add=True, can_change=True,
+                                            can_delete=False,
+                                            user=USERS[1]).sites.add(SITES[0])
+        self.assertEqual(1, GlobalPagePermission.objects.with_user(USERS[1]).count())
+
+        homepage = create_page(title="master", template="nav_playground.html",
+                               language="en", in_navigation=True, slug='/')
+        publish_page(page=homepage, user=superuser, language='en')
+
+        with SettingsOverride(CMS_PERMISSION=True):
+            # for all users, they should have access to site 1
+            request = RequestFactory().get(path='/', data={'site__exact': 1})
+            # we need a session attribute for current_site(request), which is
+            # used by has_page_add_permission and has_page_change_permission
+            request.session = {}
+            for user in USERS:
+                # has_page_add_permission and has_page_change_permission both test
+                # for this explicitly, to see if it's a superuser.
+                request.user = user
+                # Note, the query count is inflated by doing additional lookups
+                # because there's a site param in the request.
+                with self.assertNumQueries(FuzzyInt(6,7)):
+                    # PageAdmin swaps out the methods called for permissions
+                    # if the setting is true, it makes use of cms.utils.permissions
+                    self.assertTrue(has_page_add_permission(request))
+                    self.assertTrue(has_page_change_permission(request))
+                    # internally this calls PageAdmin.has_[add|change|delete]_permission()
+                    self.assertEqual({'add': True, 'change': True, 'delete': False},
+                                     site._registry[Page].get_model_perms(request))
+
+            # can't use the above loop for this test, as we're testing that
+            # user 1 has access, but user 2 does not, as they are only assigned
+            # to site 1
+            request = RequestFactory().get('/', data={'site__exact': 2})
+            request.session = {}
+            # As before, the query count is inflated by doing additional lookups
+            # because there's a site param in the request
+            with self.assertNumQueries(FuzzyInt(11, 20)):
+                # this user shouldn't have access to site 2
+                request.user = USERS[1]
+                self.assertTrue(not has_page_add_permission(request))
+                self.assertTrue(not has_page_change_permission(request))
+                self.assertEqual({'add': False, 'change': False, 'delete': False},
+                                 site._registry[Page].get_model_perms(request))
+                # but, going back to the first user, they should.
+                request = RequestFactory().get('/', data={'site__exact': 2})
+                request.user = USERS[0]
+                self.assertTrue(has_page_add_permission(request))
+                self.assertTrue(has_page_change_permission(request))
+                self.assertEqual({'add': True, 'change': True, 'delete': False},
+                                 site._registry[Page].get_model_perms(request))

cms/utils/admin.py

@@ -9,7 +9,7 @@
 from django.template.context import RequestContext
 from django.contrib.sites.models import Site
 
-from cms.models import Page
+from cms.models import Page, GlobalPagePermission
 from cms.utils import permissions, get_language_from_request, get_language_list, get_cms_setting
 from cms.utils.permissions import has_global_page_permission
 from django.utils.encoding import smart_str
@@ -62,8 +62,9 @@ def get_admin_menu_item_context(request, page, filtered=False):
     has_add_on_same_level_permission = False
     opts = Page._meta
     if get_cms_setting('PERMISSION'):
-        perms = has_global_page_permission(request, page.site_id, can_add=True)
-        if (request.user.has_perm(opts.app_label + '.' + opts.get_add_permission()) and perms):
+        global_add_perm = GlobalPagePermission.objects.user_has_add_permission(
+            request.user, page.site_id).exists()
+        if request.user.has_perm(opts.app_label + '.' + opts.get_add_permission()) and global_add_perm:
             has_add_on_same_level_permission = True
 
     if not has_add_on_same_level_permission and page.parent_id:

cms/utils/permissions.py

@@ -55,8 +55,10 @@ def has_page_add_permission(request):
             page = Page.objects.get(pk=target)
         except Page.DoesNotExist:
             return False
-        if (request.user.has_perm(opts.app_label + '.' + opts.get_add_permission()) and
-            has_global_page_permission(request, page.site_id, can_add=True)):
+        global_add_perm = GlobalPagePermission.objects.user_has_add_permission(
+            request.user, site).exists()
+        if (request.user.has_perm(opts.app_label + '.' + opts.get_add_permission())
+                and global_add_perm):
             return True
         if position in ("first-child", "last-child"):
             return page.has_add_permission(request)
@@ -66,8 +68,10 @@ def has_page_add_permission(request):
     else:
         from cms.utils.plugins import current_site
         site = current_site(request)
-        if (request.user.has_perm(opts.app_label + '.' + opts.get_add_permission()) and
-            has_global_page_permission(request, site, can_add=True)):
+        global_add_perm = GlobalPagePermission.objects.user_has_add_permission(
+            request.user, site).exists()
+        if (request.user.has_perm(opts.app_label + '.' + opts.get_add_permission())
+                and global_add_perm):
             return True
     return False
 
@@ -91,13 +95,12 @@ def has_page_change_permission(request):
     """
     from cms.utils.plugins import current_site
     opts = Page._meta
+    site = current_site(request)
+    global_change_perm = GlobalPagePermission.objects.user_has_change_permission(
+        request.user, site).exists()
     return request.user.is_superuser or (
         request.user.has_perm(opts.app_label + '.' + opts.get_change_permission())
-        and (
-            not get_cms_setting('PERMISSION') or
-            has_global_page_permission(request, current_site(request),
-                                       can_change=True) or
-            has_any_page_change_permissions(request)))
+        and global_change_perm or has_any_page_change_permissions(request))
 
 
 def has_global_page_permission(request, site=None, **filters):
@@ -260,6 +263,7 @@ def has_global_change_permissions_permission(request):
 def has_generic_permission(page_id, user, attr, site):
     """
     Permission getter for single page with given id.
+    Internally, this calls a method on PagePermissionsPermissionManager
     """
     func = getattr(Page.permissions, "get_%s_id_list" % attr)
     permission = func(user, site)