- fixed permission check and error handling in DeleteCollection

src/webdav/Collection.py

@@ -89,7 +89,7 @@ class Collection(Resource):
         url = urlfix(REQUEST['URL'], 'DELETE')
         name = unquote(filter(None, url.split( '/'))[-1])
         parent = self.aq_parent
-        user = getSecurityManager().getUser()
+        sm = getSecurityManager()
         token = None
 
 #        if re.match("/Control_Panel",REQUEST['PATH_INFO']):
@@ -119,7 +119,7 @@ class Collection(Resource):
                 if ifhdr.find(tok) > -1:
                     token = tok
         cmd = DeleteCollection()
-        result = cmd.apply(self, token, user, REQUEST['URL'])
+        result = cmd.apply(self, token, sm, REQUEST['URL'])
 
         if result:
             # There were conflicts, so we need to report them

src/webdav/davcmds.py

@@ -18,6 +18,7 @@ from cStringIO import StringIO
 from urllib import quote
 
 import transaction
+from AccessControl.Permissions import delete_objects
 from AccessControl.SecurityManagement import getSecurityManager
 from Acquisition import aq_base
 from Acquisition import aq_parent
@@ -26,11 +27,12 @@ from zExceptions import BadRequest
 from zExceptions import Forbidden
 
 from webdav.common import absattr
+from webdav.common import isDavCollection
+from webdav.common import Locked
+from webdav.common import PreconditionFailed
 from webdav.common import urlbase
 from webdav.common import urlfix
 from webdav.common import urljoin
-from webdav.common import isDavCollection
-from webdav.common import PreconditionFailed
 from webdav.interfaces import IWriteLock
 from webdav.LockItem import LockItem
 from webdav.xmltools import XmlParser
@@ -492,7 +494,7 @@ class DeleteCollection:
     checking *all* descendents (deletes on collections are always of depth
     infinite) for locks and if the locks match. """
 
-    def apply(self, obj, token, user, url=None, result=None, top=1):
+    def apply(self, obj, token, sm, url=None, result=None, top=1):
         if result is None:
             result = StringIO()
             url = urlfix(url, 'DELETE')
@@ -502,7 +504,7 @@ class DeleteCollection:
         parent = aq_parent(obj)
 
         islockable = IWriteLock.providedBy(obj)
-        if parent and (not user.has_permission('Delete objects', parent)):
+        if parent and (not sm.checkPermission(delete_objects, parent)):
             # User doesn't have permission to delete this object
             errmsg = "403 Forbidden"
         elif islockable and obj.wl_isLocked():
@@ -514,8 +516,10 @@ class DeleteCollection:
 
         if errmsg:
             if top and (not iscol):
-                err = errmsg[4:]
-                raise err
+                if errmsg == "403 Forbidden":
+                    raise Forbidden()
+                if errmsg == "423 Locked":
+                    raise Locked()
             elif not result.getvalue():
                 # We haven't had any errors yet, so our result is empty
                 # and we need to set up the XML header
@@ -530,7 +534,7 @@ class DeleteCollection:
                 dflag = hasattr(ob,'_p_changed') and (ob._p_changed == None)
                 if hasattr(ob, '__dav_resource__'):
                     uri = urljoin(url, absattr(ob.getId()))
-                    self.apply(ob, token, user, uri, result, top=0)
+                    self.apply(ob, token, sm, uri, result, top=0)
                     if dflag:
                         ob._p_deactivate()
         if not top:

src/webdav/tests/test_davcmds.py

 import unittest
 
+from AccessControl.SecurityManagement import getSecurityManager
+from AccessControl.SecurityManagement import newSecurityManager
+from AccessControl.SecurityManagement import noSecurityManager
+from AccessControl.SecurityManager import setSecurityPolicy
+from zExceptions import Forbidden
+from zope.interface import implements
+
+
+class _DummySecurityPolicy(object):
+
+    def checkPermission(self, permission, object, context):
+        return False
+
+
+class _DummyContent(object):
+
+    from webdav.interfaces import IWriteLock
+    implements(IWriteLock)
+
+    def __init__(self, token=None):
+        self.token = token
+
+    def wl_hasLock(self, token):
+        return self.token == token
+
+    def wl_isLocked(self):
+        return bool(self.token)
+
+
 class TestUnlock(unittest.TestCase):
 
     def _getTargetClass(self):
         from webdav.davcmds import Unlock
+
         return Unlock
 
-    def _makeOne(self):
-        klass = self._getTargetClass()
-        return klass()
-
-    def _makeLockable(self, locktoken):
-        from webdav.interfaces import IWriteLock
-        from zope.interface import implements
-        class Lockable:
-            implements(IWriteLock)
-            def __init__(self, token):
-                self.token = token
-            def wl_hasLock(self, token):
-                return self.token == token
-        return Lockable(locktoken)
+    def _makeOne(self, *args, **kw):
+        return self._getTargetClass()(*args, **kw)
 
     def test_apply_bogus_lock(self):
         """
@@ -36,7 +54,7 @@ class TestUnlock(unittest.TestCase):
         This was caught by litmus locks.notowner_lock test #10.
         """
         inst = self._makeOne()
-        lockable = self._makeLockable(None)
+        lockable = _DummyContent()
         result = inst.apply(lockable, 'bogus',
                             url='http://example.com/foo/UNLOCK', top=0)
         result = result.getvalue()
@@ -44,15 +62,16 @@ class TestUnlock(unittest.TestCase):
             result.find('<d:status>HTTP/1.1 400 Bad Request</d:status>'),
             -1)
 
+
 class TestPropPatch(unittest.TestCase):
 
     def _getTargetClass(self):
         from webdav.davcmds import PropPatch
+
         return PropPatch
 
-    def _makeOne(self, request):
-        klass = self._getTargetClass()
-        return klass(request)
+    def _makeOne(self, *args, **kw):
+        return self._getTargetClass()(*args, **kw)
 
     def test_parse_xml_property_values_with_namespaces(self):
         """
@@ -79,8 +98,50 @@ class TestPropPatch(unittest.TestCase):
         self.assertEqual(len(inst.values), 1)
         self.assertEqual(inst.values[0][3]['__xml_attrs__'], {})
 
+
+class TestDeleteCollection(unittest.TestCase):
+
+    def _getTargetClass(self):
+        from webdav.davcmds import DeleteCollection
+
+        return DeleteCollection
+
+    def _makeOne(self, *args, **kw):
+        return self._getTargetClass()(*args, **kw)
+
+    def setUp(self):
+        self._oldPolicy = setSecurityPolicy(_DummySecurityPolicy())
+        newSecurityManager(None, object())
+
+    def tearDown(self):
+        noSecurityManager()
+        setSecurityPolicy(self._oldPolicy)
+
+    def test_apply_no_parent(self):
+        cmd = self._makeOne()
+        obj = _DummyContent()
+        sm = getSecurityManager()
+        self.assertEqual(cmd.apply(obj, None, sm, '/foo/DELETE'), '')
+
+    def test_apply_no_col_Forbidden(self):
+        cmd = self._makeOne()
+        obj = _DummyContent()
+        obj.__parent__ = _DummyContent()
+        sm = getSecurityManager()
+        self.assertRaises(Forbidden, cmd.apply, obj, None, sm, '/foo/DELETE')
+
+    def test_apply_no_col_Locked(self):
+        from webdav.common import Locked
+
+        cmd = self._makeOne()
+        obj = _DummyContent('LOCKED')
+        sm = getSecurityManager()
+        self.assertRaises(Locked, cmd.apply, obj, None, sm, '/foo/DELETE')
+
+
 def test_suite():
     return unittest.TestSuite((
         unittest.makeSuite(TestUnlock),
         unittest.makeSuite(TestPropPatch),
+        unittest.makeSuite(TestDeleteCollection),
         ))