Merge -r29793:29797 tseaver-catalog_getObject_raises:

CatalogBrains' 'getObject' now raises by default.
Old "return None" behavior can be restored with the deprecated
"catalog_getObject_raises" ZConfig option.

doc/CHANGES.txt

@@ -28,6 +28,15 @@ Zope Changes
 
     Features added
 
+      - ZCatalog.CatalogBrains:  'getObject' now raises errors, rather than
+        returning None, in cases where the path points either to a nonexistent
+        object (in which case it raises NotFound) or to one which the user
+        cannot access (raising Unauthorized).  Sites which rely on the old
+        behavior can restore setting a new zope.conf option,
+        'catalog-getObject-raises', to "off".
+        
+        This compatibility option will be removed in Zope 2.10.
+
       - PluginIndexes: the ZCatalog's "Indexes" tab now show the number of
         distinct values indexed by each index instead of a mixture of indexed
         objects versus number of distinct values. Indexes derived from UnIndex 

lib/python/Products/ZCatalog/CatalogBrains.py

@@ -14,6 +14,13 @@
 __version__ = "$Revision$"[11:-2]
 
 import Acquisition, Record
+from zExceptions import NotFound
+from zExceptions import Unauthorized
+from ZODB.POSException import ConflictError
+
+# Switch for new behavior, raise NotFound instead of returning None.
+# Use 'catalog-getOb-raises off' in zope.conf to restore old behavior.
+GETOBJECT_RAISES = True
 
 class AbstractCatalogBrain(Record.Record, Acquisition.Implicit):
     """Abstract base brain that handles looking up attributes as
@@ -54,10 +61,25 @@ class AbstractCatalogBrain(Record.Record, Acquisition.Implicit):
             return None
         parent = self.aq_parent
         if len(path) > 1:
-            parent = parent.unrestrictedTraverse('/'.join(path[:-1]), None)
-            if parent is None:
+            try:
+                parent = parent.unrestrictedTraverse(path[:-1])
+            except ConflictError:
+                raise
+            except:
+                if GETOBJECT_RAISES:
+                    raise
                 return None
-        return parent.restrictedTraverse(path[-1], None)
+
+        try:
+            target = parent.restrictedTraverse(path[-1])
+        except ConflictError:
+            raise
+        except:
+            if GETOBJECT_RAISES:
+                raise
+            return None
+
+        return target
 
     def getRID(self):
         """Return the record ID for this object."""

lib/python/Products/ZCatalog/tests/testBrains.py

@@ -56,7 +56,8 @@ class DummyCatalog(Acquisition.Implicit):
     # This is sooooo ugly
 
     def unrestrictedTraverse(self, path, default=None):
-        assert path == '' # for these tests...
+        # for these tests...
+        assert path == '' or path == ('') or path == [''], path
         return self
 
     def restrictedTraverse(self, path, default=_marker):
@@ -66,7 +67,7 @@ class DummyCatalog(Acquisition.Implicit):
             ob = self._objs[path].__of__(self)
             ob.check()
             return ob
-        except (KeyError, Unauthorized):
+        except KeyError:
             if default is not _marker:
                 return default
             raise
@@ -86,60 +87,99 @@ class ConflictingCatalog(DummyCatalog):
     def getpath(self, rid):
         raise ConflictError
 
-class TestBrains(unittest.TestCase):
+class BrainsTestBase:
+
+    _old_flag = None
     
     def setUp(self):
         self.cat = DummyCatalog()
         self.cat.REQUEST = DummyRequest()
+        self._init_getOb_flag()
+
+    def tearDown(self):
+        if self._old_flag is not None:
+            self._restore_getOb_flag()
+
+    def _init_getOb_flag(self):
+        from Products.ZCatalog import CatalogBrains
+        self._old_flag = CatalogBrains.GETOBJECT_RAISES
+        CatalogBrains.GETOBJECT_RAISES = self._flag_value()
+
+    def _restore_getOb_flag(self):
+        from Products.ZCatalog import CatalogBrains
+        CatalogBrains.GETOBJECT_RAISES = self._old_flag
     
-    def makeBrain(self, rid):
+    def _makeBrain(self, rid):
         from Products.ZCatalog.CatalogBrains import AbstractCatalogBrain
         class Brain(AbstractCatalogBrain):
             __record_schema__ = {'test_field': 0, 'data_record_id_':1}
         return Brain(('test', rid)).__of__(self.cat)
-    
+
     def testHasKey(self):
-        b = self.makeBrain(1)
+        b = self._makeBrain(1)
         self.failUnless(b.has_key('test_field'))
         self.failUnless(b.has_key('data_record_id_'))
         self.failIf(b.has_key('godel'))
     
     def testGetPath(self):
-        b = [self.makeBrain(rid) for rid in range(3)]
+        b = [self._makeBrain(rid) for rid in range(3)]
         self.assertEqual(b[0].getPath(), '/conflicter')
         self.assertEqual(b[1].getPath(), '/happy')
         self.assertEqual(b[2].getPath(), '/secret')
     
     def testGetPathPropagatesConflictErrors(self):
         self.cat = ConflictingCatalog()
-        b = self.makeBrain(0)
+        b = self._makeBrain(0)
         self.assertRaises(ConflictError, b.getPath)
         
     def testGetURL(self):
-        b = self.makeBrain(0)
+        b = self._makeBrain(0)
         self.assertEqual(b.getURL(), 'http://superbad.com/conflicter')
     
     def testGetRID(self):
-        b = self.makeBrain(42)
+        b = self._makeBrain(42)
         self.assertEqual(b.getRID(), 42)
     
     def testGetObjectHappy(self):
-        b = self.makeBrain(1)
+        b = self._makeBrain(1)
         self.assertEqual(b.getPath(), '/happy')
         self.failUnless(b.getObject().aq_base is self.cat.getobject(1).aq_base)
     
     def testGetObjectPropagatesConflictErrors(self):
-        b = self.makeBrain(0)
+        b = self._makeBrain(0)
         self.assertEqual(b.getPath(), '/conflicter')
         self.assertRaises(ConflictError, b.getObject)
+
+class TestBrains(BrainsTestBase, unittest.TestCase):
     
+    def _flag_value(self):
+        return True
+
+    def testGetObjectRaisesUnauthorized(self):
+        from zExceptions import Unauthorized
+        b = self._makeBrain(2)
+        self.assertEqual(b.getPath(), '/secret')
+        self.assertRaises(Unauthorized, b.getObject)
+    
+    def testGetObjectRaisesNotFoundForMissing(self):
+        from zExceptions import NotFound
+        b = self._makeBrain(3)
+        self.assertEqual(b.getPath(), '/zonked')
+        self.assertRaises(KeyError, self.cat.getobject, 3)
+        self.assertRaises((NotFound, AttributeError, KeyError), b.getObject)
+    
+class TestBrainsOldBehavior(BrainsTestBase, unittest.TestCase):
+    
+    def _flag_value(self):
+        return False
+
     def testGetObjectReturnsNoneForUnauthorized(self):
-        b = self.makeBrain(2)
+        b = self._makeBrain(2)
         self.assertEqual(b.getPath(), '/secret')
         self.assertEqual(b.getObject(), None)
     
     def testGetObjectReturnsNoneForMissing(self):
-        b = self.makeBrain(3)
+        b = self._makeBrain(3)
         self.assertEqual(b.getPath(), '/zonked')
         self.assertRaises(KeyError, self.cat.getobject, 3)
         self.assertEqual(b.getObject(), None)        
@@ -147,6 +187,7 @@ class TestBrains(unittest.TestCase):
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest(unittest.makeSuite(TestBrains))
+    suite.addTest(unittest.makeSuite(TestBrainsOldBehavior))
     return suite
 
 if __name__ == '__main__':

lib/python/Products/ZCatalog/tests/testCatalog.py

@@ -590,6 +590,8 @@ class PickySecurityManager:
 class TestZCatalogGetObject(unittest.TestCase):
     # Test what objects are returned by brain.getObject()
 
+    _old_flag = None
+
     def setUp(self):
         from Products.ZCatalog.ZCatalog import ZCatalog
         catalog = ZCatalog('catalog')
@@ -601,6 +603,17 @@ class TestZCatalogGetObject(unittest.TestCase):
 
     def tearDown(self):
         noSecurityManager()
+        if self._old_flag is not None:
+            self._restore_getObject_flag()
+    
+    def _init_getObject_flag(self, flag):
+        from Products.ZCatalog import CatalogBrains
+        self._old_flag = CatalogBrains.GETOBJECT_RAISES
+        CatalogBrains.GETOBJECT_RAISES = flag
+
+    def _restore_getObject_flag(self):
+        from Products.ZCatalog import CatalogBrains
+        CatalogBrains.GETOBJECT_RAISES = self._old_flag
 
     def test_getObject_found(self):
         # Check normal traversal
@@ -612,8 +625,47 @@ class TestZCatalogGetObject(unittest.TestCase):
         self.assertEqual(brain.getPath(), '/ob')
         self.assertEqual(brain.getObject().getId(), 'ob')
 
-    def test_getObject_missing(self):
+    def test_getObject_missing_raises_NotFound(self):
+        # Check that if the object is missing None is returned
+        from zExceptions import NotFound
+        self._init_getObject_flag(True)
+        root = self.root
+        catalog = root.catalog
+        root.ob = Folder('ob')
+        catalog.catalog_object(root.ob)
+        brain = catalog.searchResults()[0]
+        del root.ob
+        self.assertRaises((NotFound, AttributeError, KeyError), brain.getObject)
+
+    def test_getObject_restricted_raises_Unauthorized(self):
+        # Check that if the object's security does not allow traversal,
+        # None is returned
+        from zExceptions import NotFound
+        self._init_getObject_flag(True)
+        root = self.root
+        catalog = root.catalog
+        root.fold = Folder('fold')
+        root.fold.ob = Folder('ob')
+        catalog.catalog_object(root.fold.ob)
+        brain = catalog.searchResults()[0]
+        # allow all accesses
+        pickySecurityManager = PickySecurityManager()
+        setSecurityManager(pickySecurityManager)
+        self.assertEqual(brain.getObject().getId(), 'ob')
+        # disallow just 'ob' access
+        pickySecurityManager = PickySecurityManager(['ob'])
+        setSecurityManager(pickySecurityManager)
+        self.assertRaises(Unauthorized, brain.getObject)
+        # disallow just 'fold' access
+        pickySecurityManager = PickySecurityManager(['fold'])
+        setSecurityManager(pickySecurityManager)
+        ob = brain.getObject()
+        self.failIf(ob is None)
+        self.assertEqual(ob.getId(), 'ob')
+
+    def test_getObject_missing_returns_None(self):
         # Check that if the object is missing None is returned
+        self._init_getObject_flag(False)
         root = self.root
         catalog = root.catalog
         root.ob = Folder('ob')
@@ -622,9 +674,10 @@ class TestZCatalogGetObject(unittest.TestCase):
         del root.ob
         self.assertEqual(brain.getObject(), None)
 
-    def test_getObject_restricted(self):
+    def test_getObject_restricted_returns_None(self):
         # Check that if the object's security does not allow traversal,
         # None is returned
+        self._init_getObject_flag(False)
         root = self.root
         catalog = root.catalog
         root.fold = Folder('fold')

lib/python/Zope2/Startup/handlers.py

@@ -124,6 +124,20 @@ def cgi_maxlen(value):
 def http_header_max_length(value):
     return value
 
+def catalog_getObject_raises(value):
+
+    if value is not None:
+
+        import warnings
+        warnings.warn(
+        "'catalog-getObject-raises' option will be removed in Zope 2.10:\n",
+        DeprecationWarning)
+
+        from Products.ZCatalog import CatalogBrains 
+        CatalogBrains.GETOBJECT_RAISES = bool(value)
+
+    return value
+
 # server handlers
 
 def root_handler(config):

lib/python/Zope2/Startup/zopeschema.xml

@@ -768,6 +768,17 @@
      </description>
   </key>
 
+  <key name="catalog-getObject-raises" datatype="boolean"
+       handler="catalog_getObject_raises">
+     <description>
+     If this directive is set to "on" (the deafult), ZCatalog brains objects
+     will raise NotFound exceptions from 'getObject' for unreachable objects,
+     and Unauthorized for disallowed objects.  If the option is "off", they
+     will return None in such cases (which was the old behavior)
+     </description>
+     <metadefault>off</metadefault>
+  </key>
+
   <multisection type="ZServer.server" name="*" attribute="servers"/>
   <key name="port-base" datatype="integer" default="0">
     <description>