[Zope-Checkins] SVN: Zope/trunk/ Fixed ObjectManager to not swallow exceptions during object deletion (in

[Florent Guillaume]

Log message for revision 28478:
  Fixed ObjectManager to not swallow exceptions during object deletion (in
  debug mode and if the user is not Manager). This allows for better
  debugging, while still keeping the possibility for a Manager to delete
  buggy objects.
  
  

Changed:
  U   Zope/trunk/doc/CHANGES.txt
  U   Zope/trunk/lib/python/OFS/ObjectManager.py
  U   Zope/trunk/lib/python/OFS/tests/testObjectManager.py
  U   Zope/trunk/lib/python/OFS/tests/testOrderSupport.py

-=-
Modified: Zope/trunk/doc/CHANGES.txt
===================================================================
--- Zope/trunk/doc/CHANGES.txt	2004-11-19 14:04:46 UTC (rev 28477)
+++ Zope/trunk/doc/CHANGES.txt	2004-11-19 18:36:26 UTC (rev 28478)
@@ -26,6 +26,11 @@
 
     Features added
 
+      - Fixed ObjectManager to not swallow exceptions during object
+        deletion (in debug mode and if the user is not Manager). This
+        allows for better debugging, while still keeping the possibility
+        for a Manager to delete buggy objects.
+
       - Added a ZConfig directive 'large-file-threshold' to control
         the request content-size threshold at which a temporary file
         gets created. Use the same value for deciding between reading

Modified: Zope/trunk/lib/python/OFS/ObjectManager.py
===================================================================
--- Zope/trunk/lib/python/OFS/ObjectManager.py	2004-11-19 14:04:46 UTC (rev 28477)
+++ Zope/trunk/lib/python/OFS/ObjectManager.py	2004-11-19 18:36:26 UTC (rev 28478)
@@ -307,7 +307,10 @@
             except:
                 LOG('Zope',ERROR,'manage_beforeDelete() threw',
                     error=sys.exc_info())
-                pass
+                # In debug mode when non-Manager, let exceptions propagate.
+                if getConfiguration().debug_mode:
+                    if not getSecurityManager().getUser().has_role('Manager'):
+                        raise
             if s is None: object._p_deactivate()
 
     def _delObject(self, id, dp=1):
@@ -319,9 +322,12 @@
         except ConflictError:
             raise
         except:
-            LOG('Zope',ERROR,'manage_beforeDelete() threw',
+            LOG('Zope', ERROR, '_delObject() threw',
                 error=sys.exc_info())
-            pass
+            # In debug mode when non-Manager, let exceptions propagate.
+            if getConfiguration().debug_mode:
+                if not getSecurityManager().getUser().has_role('Manager'):
+                    raise
         self._objects=tuple(filter(lambda i,n=id: i['id']!=n, self._objects))
         self._delOb(id)
 

Modified: Zope/trunk/lib/python/OFS/tests/testObjectManager.py
===================================================================
--- Zope/trunk/lib/python/OFS/tests/testObjectManager.py	2004-11-19 14:04:46 UTC (rev 28477)
+++ Zope/trunk/lib/python/OFS/tests/testObjectManager.py	2004-11-19 18:36:26 UTC (rev 28478)
@@ -1,5 +1,6 @@
 import unittest
 
+from App.config import getConfiguration
 from Acquisition import Implicit, aq_base, aq_parent
 from AccessControl.SecurityManagement import newSecurityManager
 from AccessControl.SecurityManagement import noSecurityManager
@@ -30,12 +31,35 @@
 
         return self._id
 
+class DeleteFailed(Exception):
+    pass
+
+class ItemForDeletion(SimpleItem):
+
+    def __init__(self, fail_on_delete=False):
+        self.id = 'stuff'
+        self.before_delete_called = False
+        self.fail_on_delete = fail_on_delete
+
+    def manage_beforeDelete(self, item, container):
+        self.before_delete_called = True
+        if self.fail_on_delete:
+            raise DeleteFailed
+        return SimpleItem.manage_beforeDelete(self, item, container)
+
+
 class ObjectManagerTests( unittest.TestCase ):
 
+    def setUp(self):
+        self.saved_cfg_debug_mode = getConfiguration().debug_mode
+
     def tearDown( self ):
-
         noSecurityManager()
+        getConfiguration().debug_mode = self.saved_cfg_debug_mode
 
+    def setDebugMode(self, mode):
+        getConfiguration().debug_mode = mode
+
     def _getTargetClass( self ):
 
         from OFS.ObjectManager import ObjectManager
@@ -219,6 +243,66 @@
 
         self.assertEqual( si.__ac_local_roles__, None )
 
+    def test_delObject_before_delete(self):
+        # Test that manage_beforeDelete is called
+        om = self._makeOne()
+        ob = ItemForDeletion()
+        om._setObject(ob.getId(), ob)
+        self.assertEqual(ob.before_delete_called, False)
+        om._delObject(ob.getId())
+        self.assertEqual(ob.before_delete_called, True)
+
+    def test_delObject_exception_manager(self):
+        # Test exception behavior in manage_beforeDelete
+        # Manager user
+        self.setDebugMode(False)
+        newSecurityManager(None, system) # Manager
+        om = self._makeOne()
+        ob = ItemForDeletion(fail_on_delete=True)
+        om._setObject(ob.getId(), ob)
+        om._delObject(ob.getId())
+
+    def test_delObject_exception(self):
+        # Test exception behavior in manage_beforeDelete
+        # non-Manager user
+        self.setDebugMode(False)
+        om = self._makeOne()
+        ob = ItemForDeletion(fail_on_delete=True)
+        om._setObject(ob.getId(), ob)
+        om._delObject(ob.getId())
+
+    def test_delObject_exception_debug_manager(self):
+        # Test exception behavior in manage_beforeDelete in debug mode
+        # Manager user
+        self.setDebugMode(True)
+        newSecurityManager(None, system) # Manager
+        om = self._makeOne()
+        ob = ItemForDeletion(fail_on_delete=True)
+        om._setObject(ob.getId(), ob)
+        om._delObject(ob.getId())
+
+    def test_delObject_exception_debug(self):
+        # Test exception behavior in manage_beforeDelete in debug mode
+        # non-Manager user
+        # It's the only special case: we let exceptions propagate.
+        self.setDebugMode(True)
+        om = self._makeOne()
+        ob = ItemForDeletion(fail_on_delete=True)
+        om._setObject(ob.getId(), ob)
+        self.assertRaises(DeleteFailed, om._delObject, ob.getId())
+
+    def test_delObject_exception_debug_deep(self):
+        # Test exception behavior in manage_beforeDelete in debug mode
+        # non-Manager user
+        # Test for deep subobjects.
+        self.setDebugMode(True)
+        om1 = self._makeOne()
+        om2 = self._makeOne()
+        ob = ItemForDeletion(fail_on_delete=True)
+        om1._setObject('om2', om2, set_owner=False)
+        om2._setObject(ob.getId(), ob)
+        self.assertRaises(DeleteFailed, om1._delObject, 'om2')
+
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest( unittest.makeSuite( ObjectManagerTests ) )

Modified: Zope/trunk/lib/python/OFS/tests/testOrderSupport.py
===================================================================
--- Zope/trunk/lib/python/OFS/tests/testOrderSupport.py	2004-11-19 14:04:46 UTC (rev 28477)
+++ Zope/trunk/lib/python/OFS/tests/testOrderSupport.py	2004-11-19 18:36:26 UTC (rev 28478)
@@ -17,6 +17,8 @@
         return 1
     def manage_afterAdd(self, item, container):
         return
+    def manage_beforeDelete(self, item, container):
+        return
     def wl_isLocked(self):
         return 0