diff --git a/cms/menu.py b/cms/menu.py index 98b2cb11753..01b210ba27b 100644 --- a/cms/menu.py +++ b/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 ( diff --git a/cms/middleware/toolbar.py b/cms/middleware/toolbar.py old mode 100755 new mode 100644 diff --git a/cms/models/managers.py b/cms/models/managers.py index 5d67c249fda..4af1da5dde5 100644 --- a/cms/models/managers.py +++ b/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 diff --git a/cms/models/pagemodel.py b/cms/models/pagemodel.py index 7e88cef3e00..6c9268255dc 100644 --- a/cms/models/pagemodel.py +++ b/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: diff --git a/cms/models/permissionmodels.py b/cms/models/permissionmodels.py index c49140a9db0..2f7c78c35ca 100644 --- a/cms/models/permissionmodels.py +++ b/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') diff --git a/cms/static/cms/js/libs/jquery.min.js b/cms/static/cms/js/libs/jquery.min.js old mode 100755 new mode 100644 diff --git a/cms/static/cms/swf/expressInstall.swf b/cms/static/cms/swf/expressInstall.swf old mode 100755 new mode 100644 diff --git a/cms/tests/admin.py b/cms/tests/admin.py index 2b14649f571..8c948c9f711 100644 --- a/cms/tests/admin.py +++ b/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('Test', 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/')) diff --git a/cms/tests/permmod.py b/cms/tests/permmod.py index d91de8fb661..93943596309 100644 --- a/cms/tests/permmod.py +++ b/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)) diff --git a/cms/utils/admin.py b/cms/utils/admin.py index d2619e564c6..daac98fbda6 100644 --- a/cms/utils/admin.py +++ b/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: diff --git a/cms/utils/permissions.py b/cms/utils/permissions.py index be952e8918c..67cbe62b899 100644 --- a/cms/utils/permissions.py +++ b/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)