Added tests for placeholderadmin and pageadmin plugin operations secu…

…rity

Added permission checks to placeholders and placeholderadmin
Fixed limits not being respected by placeholderadmin

Conflicts:

	cms/test/testcases.py
	cms/tests/placeholder.py
	cms/tests/plugins.py

cms/admin/pageadmin.py

@@ -1074,6 +1074,9 @@ def change_innavigation(self, request, page_id):
 
     @create_on_success
     def add_plugin(self, request):
+        '''
+        Could be either a page or a parent - if it's a parent we get the page via parent.
+        '''
         if 'history' in request.path or 'recover' in request.path:
             return HttpResponse(str("error"))
         if request.method == "POST":
@@ -1108,14 +1111,20 @@ def add_plugin(self, request):
                 parent = get_object_or_404(CMSPlugin, pk=parent_id)
                 placeholder = parent.placeholder
                 page = get_page_from_placeholder_if_exists(placeholder)
+                if not page: # Make sure we do have a page
+                    raise Http404
                 language = parent.language
                 position = None
             # placeholder (non-page) add-plugin
             else:
-                position = None
-                language = request.POST['language'] or get_language_from_request(request)
-            if page and not page.has_change_permission(request):
-                return HttpResponseForbidden(unicode(_("You do not have permission to change this page")))
+                # do NOT allow non-page placeholders to use this method, they
+                # should use their respective admin!
+                raise Http404
+
+            if not page.has_change_permission(request):
+                # we raise a 404 instead of 403 for a slightly improved security
+                # and to be consistent with placeholder admin
+                raise Http404
 
             # Sanity check to make sure we're not getting bogus values from JavaScript:
             if not language or not language in [ l[0] for l in settings.LANGUAGES ]:
@@ -1175,7 +1184,7 @@ def edit_plugin(self, request, plugin_id):
             page = get_page_from_placeholder_if_exists(cms_plugin.placeholder)
             instance, plugin_admin = cms_plugin.get_plugin_instance(self.admin_site)
             if page and not page.has_change_permission(request):
-                raise PermissionDenied
+                raise Http404
         else:
             # history view with reversion
             from reversion.models import Version

cms/admin/placeholderadmin.py

@@ -122,29 +122,66 @@ def get_urls(self):
         return url_patterns + super(PlaceholderAdmin, self).get_urls()
 
     def add_plugin(self, request):
+        # only allow POST
         if request.method != "POST":
             raise Http404
         plugin_type = request.POST['plugin_type']
         placeholder_id = request.POST.get('placeholder', None)
         position = None
         language = get_language_from_request(request)
-        if not placeholder_id:
+        parent = None
+        # check if we got a placeholder (id)
+        if placeholder_id:
+            placeholder = get_object_or_404(Placeholder, pk=placeholder_id)
+        else: # else get the parent_id
             parent_id = request.POST.get('parent_id', None)
-            if not parent_id:
+            if not parent_id: # if we get neither a placeholder nor a parent, bail out
                 raise Http404
             parent = get_object_or_404(CMSPlugin, pk=parent_id)
-            plugin = CMSPlugin(language=language, plugin_type=plugin_type,
-                position=position, parent=parent, placeholder=parent.placeholder)
-        else:
-            placeholder = get_object_or_404(Placeholder, pk=placeholder_id)
-            plugin = CMSPlugin(language=language, plugin_type=plugin_type,
-                position=position, placeholder=placeholder) 
+            placeholder = parent.placeholder
+
+        # check add permissions on placeholder
+        if not placeholder.has_add_permission(request):
+            raise Http404
+
+        # check the limits defined in CMS_PLACEHOLDER_CONF for this placeholder
+        limits = settings.CMS_PLACEHOLDER_CONF.get(placeholder.slot, {}).get('limits', None)
+        if limits:
+            count = placeholder.cmsplugin_set.count()
+            global_limit = limits.get("global", None)
+            type_limit = limits.get(plugin_type, None)
+            # check the global limit first
+            if global_limit and count >= global_limit:
+                return HttpResponseBadRequest(
+                    "This placeholder already has the maximum number of plugins."
+                )
+            elif type_limit: # then check the type specific limit
+                type_count = CMSPlugin.objects.filter(
+                    language=language, placeholder=placeholder, plugin_type=plugin_type
+                ).count()
+                if type_count >= type_limit:
+                    return HttpResponseBadRequest(
+                        "This placeholder already has the maximum number (%s) "
+                        "of %s plugins." % (type_limit, plugin_type)
+                    )
+
+        # actually add the plugin
+        plugin = CMSPlugin(language=language, plugin_type=plugin_type,
+            position=position, placeholder=placeholder, parent=parent) 
         plugin.save()
+
+        # returns it's ID as response
         return HttpResponse(str(plugin.pk))
 
     def edit_plugin(self, request, plugin_id):
         plugin_id = int(plugin_id)
+        # get the plugin to edit of bail out
         cms_plugin = get_object_or_404(CMSPlugin, pk=plugin_id)
+
+        # check that the user has permission to change this plugin
+        if not cms_plugin.placeholder.has_change_permission(request):
+            raise Http404
+
         instance, plugin_admin = cms_plugin.get_plugin_instance(self.admin_site)
 
         plugin_admin.cms_plugin_instance = cms_plugin
@@ -185,46 +222,72 @@ def edit_plugin(self, request, plugin_id):
         return response
 
     def move_plugin(self, request):
-        if request.method == "POST":
-            pos = 0
-            if 'ids' in request.POST:
-                for id in request.POST['ids'].split("_"):
-                    plugin = CMSPlugin.objects.get(pk=id)
-                    if plugin.position != pos:
-                        plugin.position = pos
-                        plugin.save()
-                    pos += 1
-            elif 'plugin_id' in request.POST:
-                plugin = CMSPlugin.objects.get(pk=int(request.POST['plugin_id']))
-                placeholder = plugin.placeholder
-                # plugin positions are 0 based, so just using count here should give us 'last_position + 1'
-                position = CMSPlugin.objects.filter(placeholder=placeholder).count()
-                plugin.position = position
-                plugin.save()
-            else:
-                HttpResponse(str("error"))
-            return HttpResponse(str("ok"))
-        else:
+        # only allow POST
+        if request.method != "POST":
             return HttpResponse(str("error"))
+        pos = 0
+        if 'ids' in request.POST: # multiple plugins
+            whitelisted_placeholders = []
+            for id in request.POST['ids'].split("_"):
+                plugin = CMSPlugin.objects.get(pk=id)
+
+                # check the permissions for *each* plugin, but cache them locally
+                # per placeholder
+                if plugin.placeholder.pk not in whitelisted_placeholders:
+                    if plugin.placeholder.has_change_permission(request):
+                        whitelisted_placeholders.append(plugin.placeholder.pk)
+                    else:
+                        raise Http404
+
+                # actually do the moving
+                if plugin.position != pos:
+                    plugin.position = pos
+                    plugin.save()
+                pos += 1
+        elif 'plugin_id' in request.POST: # single plugin moving
+            plugin = CMSPlugin.objects.get(pk=int(request.POST['plugin_id']))
 
-    def remove_plugin(self, request):
-        if request.method == "POST":
-            plugin_id = request.POST['plugin_id']
-            plugin = get_object_or_404(CMSPlugin, pk=plugin_id)
+            # check permissions
+            if not plugin.placeholder.has_change_permission(request):
+                raise Http404
+
             placeholder = plugin.placeholder
-            plugin.delete_with_public()
-            plugin_name = unicode(plugin_pool.get_plugin(plugin.plugin_type).name)
-            comment = _(u"%(plugin_name)s plugin at position %(position)s in %(placeholder)s was deleted.") % {'plugin_name':plugin_name, 'position':plugin.position, 'placeholder':plugin.placeholder}
-            return HttpResponse("%s,%s" % (plugin_id, comment))
-        raise Http404
+            # plugin positions are 0 based, so just using count here should give us 'last_position + 1'
+            position = CMSPlugin.objects.filter(placeholder=placeholder).count()
+            plugin.position = position
+            plugin.save()
+        else:
+            HttpResponse(str("error"))
+        return HttpResponse(str("ok"))
+
+    def remove_plugin(self, request):
+        if request.method != "POST": # only allow POST
+            raise Http404
+        plugin_id = request.POST['plugin_id']
+        plugin = get_object_or_404(CMSPlugin, pk=plugin_id)
+
+        # check the permissions!
+        if not plugin.placeholder.has_delete_permission(request):
+            raise Http404
+
+        plugin.delete_with_public()
+        plugin_name = unicode(plugin_pool.get_plugin(plugin.plugin_type).name)
+        comment = _(u"%(plugin_name)s plugin at position %(position)s in %(placeholder)s was deleted.") % {'plugin_name':plugin_name, 'position':plugin.position, 'placeholder':plugin.placeholder}
+        return HttpResponse("%s,%s" % (plugin_id, comment))
 
     def copy_plugins(self, request):
+        # only allow POST
         if request.method != "POST":
             raise Http404
         placeholder_id = request.POST['placeholder']
         placeholder = get_object_or_404(Placeholder, pk=placeholder_id)
 
+        # check permissions
+        if not placeholder.has_add_permission(request):
+            raise Http404
 
+        # the placeholder actions are responsible for copying, they should return
+        # a list of plugins if successful.
         plugins = placeholder.actions.copy(
             target_placeholder=placeholder,
             source_language=request.POST['copy_from'],

cms/models/placeholdermodel.py

@@ -11,17 +11,40 @@ class Placeholder(models.Model):
     slot = models.CharField(_("slot"), max_length=50, db_index=True, editable=False)
     default_width = models.PositiveSmallIntegerField(_("width"), null=True, editable=False)
 
-    def __unicode__(self):
-        return self.slot
-
     class Meta:
         app_label = 'cms'
 
-    def has_change_permission(self, request):
-        opts = self._meta
+    def __unicode__(self):
+        return self.slot
+
+    def _get_permission(self, request, key):
+        """
+        Generic method to check the permissions for a request for a given key,
+        the key can be: 'add', 'change' or 'delete'.
+        """
         if request.user.is_superuser:
             return True
-        return request.user.has_perm(opts.app_label + '.' + opts.get_change_permission())
+        found = False
+        # check all attached models for change permissions
+        for model in self._get_attached_models():
+            opts = model._meta
+            perm_accessor = getattr(opts, 'get_%s_permission' % key)
+            perm_code = '%s.%s' % (opts.app_label, perm_accessor())
+            # if they don't have the permission for this attached model, bail out
+            if not request.user.has_perm(perm_code):
+                return False
+            else:
+                found = True
+        return found
+
+    def has_change_permission(self, request):
+        return self._get_permission(request, 'change')
+
+    def has_add_permission(self, request):
+        return self._get_permission(request, 'add')
+
+    def has_delete_permission(self, request):
+        return self._get_permission(request, 'delete')
 
     def render(self, context, width):
         from cms.plugin_rendering import render_placeholder
@@ -37,6 +60,18 @@ def get_media(self, request, context):
             return reduce(operator.add, media_classes)
         return Media()
 
+    def _get_attached_fields(self):
+        """
+        Returns an ITERATOR of all non-cmsplugin reverse foreign key related fields.
+        """
+        from cms.models import CMSPlugin
+        for rel in self._meta.get_all_related_objects():
+             if isinstance(rel.model, CMSPlugin):
+                continue
+            field = getattr(self, rel.get_accessor_name())
+            if field.count():
+                yield rel.field
+
     def _get_attached_field(self):
         from cms.models import CMSPlugin
         if not hasattr(self, '_attached_field_cache'):
@@ -60,6 +95,13 @@ def _get_attached_model(self):
         if field:
             return field.model
         return None
+
+    def _get_attached_models(self):
+        """
+        Returns a list of models of attached to this placeholder.
+        """
+        return [field.model for field in self._get_attached_fields()]
+
 
     def get_plugins_list(self):
         return list(self.get_plugins())