We don't need placeholder permissions checking in view mode

CHANGELOG.txt

@@ -27,6 +27,8 @@
   a placeholder.
 * Fixed an issue where plugin permissions where not checked when deleting
   a page or page translation.
+* Fixed a useless placeholders edit permissions checking when not in edit
+  mode.
 
 
 === 3.3.0 (2016-05-26) ===

cms/plugin_rendering.py

@@ -125,12 +125,30 @@ def render_placeholder(placeholder, context_to_copy, name_fallback="Placeholder"
     request = context['request']
     if not hasattr(request, 'placeholders'):
         request.placeholders = {}
-    perms = (placeholder.has_change_permission(request) or not placeholder.cache_placeholder)
-    if not perms or placeholder.slot not in request.placeholders:
-        request.placeholders[placeholder.slot] = (placeholder, perms)
+
+    # Prepend frontedit toolbar output if applicable
+    toolbar = getattr(request, 'toolbar', None)
+    if (getattr(toolbar, 'edit_mode', False) and
+            getattr(toolbar, "show_toolbar", False) and
+            getattr(placeholder, 'is_editable', True) and editable):
+        from cms.middleware.toolbar import toolbar_plugin_processor
+        processors = (toolbar_plugin_processor, )
+        edit = True
+    else:
+        processors = None
+        edit = False
+
+    if edit:
+        perms = (placeholder.has_change_permission(request) or not placeholder.cache_placeholder)
+        if not perms or placeholder.slot not in request.placeholders:
+            request.placeholders[placeholder.slot] = (placeholder, perms)
+        else:
+            request.placeholders[placeholder.slot] = (
+                placeholder, perms and request.placeholders[placeholder.slot][1]
+            )
     else:
         request.placeholders[placeholder.slot] = (
-            placeholder, perms and request.placeholders[placeholder.slot][1]
+            placeholder, False
         )
     if hasattr(placeholder, 'content_cache'):
         return mark_safe(placeholder.content_cache)
@@ -148,18 +166,6 @@ def render_placeholder(placeholder, context_to_copy, name_fallback="Placeholder"
         lang = get_language_from_request(request)
         save_language = lang
 
-    # Prepend frontedit toolbar output if applicable
-    toolbar = getattr(request, 'toolbar', None)
-    if (getattr(toolbar, 'edit_mode', False) and
-            getattr(toolbar, "show_toolbar", False) and
-            getattr(placeholder, 'is_editable', True) and editable):
-        from cms.middleware.toolbar import toolbar_plugin_processor
-        processors = (toolbar_plugin_processor, )
-        edit = True
-    else:
-        processors = None
-        edit = False
-
     use_cache = use_cache and not request.user.is_authenticated()
     if get_cms_setting('PLACEHOLDER_CACHE') and use_cache:
         if not edit and placeholder and not hasattr(placeholder, 'cache_checked'):

cms/tests/test_placeholder.py

@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 import itertools
 
+from cms.tests.test_toolbar import ToolbarTestBase
 from django.conf import settings
 from django.contrib import admin
 from django.contrib.auth import get_user_model, get_permission_codename
@@ -810,7 +811,7 @@ def test_mlng_placeholder_actions_no_placeholder(self):
         de = Translations.objects.get(language_code='de')
 
 
-class PlaceholderModelTests(CMSTestCase):
+class PlaceholderModelTests(ToolbarTestBase, CMSTestCase):
     def get_mock_user(self, superuser):
         return AttributeObject(
             is_superuser=superuser,
@@ -859,8 +860,8 @@ def test_request_placeholders_permission_check_model(self):
         self.assertNotIn(ex.placeholder, editable)
 
         # request.placeholders is populated for superuser
-        context_en['request'] = self.get_request(language="en", page=page_en)
-        context_en['request'].user = self.get_superuser()
+        superuser = self.get_superuser()
+        context_en['request'] = self.get_page_request(page_en, superuser, edit=True)
         render_placeholder(ex.placeholder, context_en, use_cache=False)
         editable = [ph for ph, perms in getattr(context_en['request'], 'placeholders', {}).values() if perms]
         self.assertEqual(len(editable), 1)
@@ -877,40 +878,54 @@ def test_request_placeholders_permission_check_model(self):
 
         # request.placeholders is populated for staff user with permission on the model
         user.user_permissions.add(Permission.objects.get(codename=get_permission_codename('change', ex._meta)))
-        context_en['request'] = self.get_request(language="en", page=page_en)
-        context_en['request'].user = get_user_model().objects.get(pk=user.pk)
+        user = self.reload(user)
+        context_en['request'] = self.get_page_request(page_en, user, edit=True)
         render_placeholder(ex.placeholder, context_en, use_cache=False)
         editable = [ph for ph, perms in getattr(context_en['request'], 'placeholders', {}).values() if perms]
         self.assertEqual(len(editable), 1)
         self.assertIn(ex.placeholder, editable)
 
+        # request.placeholders is not populated for staff user with permission on the model
+        # but not in edit mode
+        context_en['request'] = self.get_page_request(page_en, user, edit=False)
+        render_placeholder(ex.placeholder, context_en, use_cache=False)
+        editable = [ph for ph, perms in getattr(context_en['request'], 'placeholders', {}).values() if perms]
+        self.assertEqual(len(editable), 0)
+        self.assertNotIn(ex.placeholder, editable)
+
+        # request.placeholders is not populated for super user not in edit mode
+        context_en['request'] = self.get_page_request(page_en, superuser, edit=False)
+        render_placeholder(ex.placeholder, context_en, use_cache=False)
+        editable = [ph for ph, perms in getattr(context_en['request'], 'placeholders', {}).values() if perms]
+        self.assertEqual(len(editable), 0)
+        self.assertNotIn(ex.placeholder, editable)
+
     def test_request_placeholders_permission_check_page(self):
         page_en = create_page('page_en', 'col_two.html', 'en')
         placeholder_en = page_en.placeholders.get(slot='col_left')
 
         context_en = SekizaiContext()
 
         # request.placeholders is populated for superuser
-        context_en['request'] = self.get_request(language="en", page=page_en)
-        context_en['request'].user = self.get_superuser()
+        superuser = self.get_superuser()
+        context_en['request'] = self.get_page_request(page_en, superuser, edit=True)
         render_placeholder(placeholder_en, context_en)
         editable = [ph for ph, perms in getattr(context_en['request'], 'placeholders', {}).values() if perms]
         self.assertEqual(len(editable), 1)
         self.assertIn(placeholder_en, editable)
 
         # request.placeholders is not populated for staff user with no permission
         user = self.get_staff_user_with_no_permissions()
-        context_en['request'] = self.get_request(language="en", page=page_en)
-        context_en['request'].user = user
+        context_en['request'] = self.get_page_request(page_en, user, edit=True)
         render_placeholder(placeholder_en, context_en)
         editable = [ph for ph, perms in getattr(context_en['request'], 'placeholders', {}).values() if perms]
         self.assertEqual(len(editable), 0)
         self.assertNotIn(placeholder_en, editable)
 
         # request.placeholders is populated for staff user with permission on the model
         user.user_permissions.add(Permission.objects.get(codename='change_page'))
-        context_en['request'] = self.get_request(language="en", page=page_en)
-        context_en['request'].user = get_user_model().objects.get(pk=user.pk)
+        user = self.reload(user)
+        context_en['request'] = self.get_page_request(page_en, user, edit=True)
         render_placeholder(placeholder_en, context_en)
         editable = [ph for ph, perms in getattr(context_en['request'], 'placeholders', {}).values() if perms]
         self.assertEqual(len(editable), 1)
@@ -928,26 +943,25 @@ def test_request_placeholders_permission_check_templatetag(self):
         context = {'ex1': ex1}
 
         # request.placeholders is populated for superuser
-        request = self.get_request(language="en", page=page_en)
-        request.user = self.get_superuser()
+        superuser = self.get_superuser()
+        request = self.get_page_request(page_en, superuser, edit=True)
         self.render_template_obj(template, context, request)
         editable = [ph for ph, perms in getattr(request, 'placeholders', {}).values() if perms]
         self.assertEqual(len(editable), 1)
         self.assertIn(ex1.placeholder, editable)
 
         # request.placeholders is not populated for staff user with no permission
         user = self.get_staff_user_with_no_permissions()
-        request = self.get_request(language="en", page=page_en)
-        request.user = user
+        request = self.get_page_request(page_en, user, edit=True)
         self.render_template_obj(template, context, request)
         editable = [ph for ph, perms in getattr(request, 'placeholders', {}).values() if perms]
         self.assertEqual(len(editable), 0)
         self.assertNotIn(ex1.placeholder, editable)
 
         # request.placeholders is populated for staff user with permission on the model
         user.user_permissions.add(Permission.objects.get(codename='change_example1'))
-        request = self.get_request(language="en", page=page_en)
-        request.user = get_user_model().objects.get(pk=user.pk)
+        user = self.reload(user)
+        request = self.get_page_request(page_en, user, edit=True)
         self.render_template_obj(template, context, request)
         editable = [ph for ph, perms in getattr(request, 'placeholders', {}).values() if perms]
         self.assertEqual(len(editable), 1)