- refactored browser:view and browser:page directives

doc/CHANGES.rst

@@ -40,6 +40,10 @@ Bugs Fixed
 Features Added
 ++++++++++++++
 
+- Five: Refactored ``browser:view`` and ``browser:page`` directives.
+  This makes their implementation more similar to that in ``zope.browserpage``
+  and adds allowed_interface support for the ``browser:view`` directive.
+
 - Optimized the `OFS.Traversable.getPhysicalPath` method to avoid excessive
   amounts of method calls.
 

src/Products/Five/browser/metaconfigure.py

@@ -33,9 +33,12 @@
 from zope.security.zcml import Permission
 
 import zope.browserpage.metaconfigure
-from zope.browserpage.metaconfigure import providesCallable
-from zope.browserpage.metaconfigure import _handle_menu
+from zope.browserpage.metaconfigure import _handle_allowed_attributes
+from zope.browserpage.metaconfigure import _handle_allowed_interface
 from zope.browserpage.metaconfigure import _handle_for
+from zope.browserpage.metaconfigure import _handle_menu
+from zope.browserpage.metaconfigure import _handle_permission
+from zope.browserpage.metaconfigure import providesCallable
 from zope.browserpage.metadirectives import IViewDirective
 
 from AccessControl.class_init import InitializeClass
@@ -52,13 +55,47 @@
 from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile
 from Products.Five.metaclass import makeClass
 
+def _configure_z2security(_context, new_class, required):
+    _context.action(
+        discriminator=('five:protectClass', new_class),
+        callable=protectClass,
+        args=(new_class, required.pop(''))
+        )
+    for attr, permission in required.iteritems():
+        _context.action(
+            discriminator=('five:protectName', new_class, attr),
+            callable=protectName,
+            args=(new_class, attr, permission)
+            )
+    # Make everything else private
+    private_attrs = [name for name in dir(new_class)
+                     if (not name.startswith('_')) and
+                        (name not in required) and
+                        ismethod(getattr(new_class, name))]
+    for attr in private_attrs:
+        _context.action(
+            discriminator=('five:protectName', new_class, attr),
+            callable=protectName,
+            args=(new_class, attr, CheckerPrivateId, False)
+            )
+    # Protect the class
+    _context.action(
+        discriminator=('five:initialize:class', new_class),
+        callable=InitializeClass,
+        args=(new_class,)
+        )
+
+# page
 
 def page(_context, name, permission, for_=Interface,
          layer=IDefaultBrowserLayer, template=None, class_=None,
          allowed_interface=None, allowed_attributes=None,
          attribute='__call__', menu=None, title=None, 
          ):
     _handle_menu(_context, menu, title, [for_], name, permission, layer)
+    required = {}
+
+    permission = _handle_permission(_context, permission)
 
     if not (class_ or template):
         raise ConfigurationError("Must specify a class or template")
@@ -77,6 +114,7 @@ def page(_context, name, permission, for_=Interface,
         if not os.path.isfile(template):
             raise ConfigurationError("No such file", template)
 
+    # TODO: new __name__ attribute must be tested
     if class_:
         if attribute != '__call__':
             if not hasattr(class_, attribute):
@@ -122,54 +160,25 @@ def page(_context, name, permission, for_=Interface,
         # template
         new_class = makeClassForTemplate(template, name=name)
 
-    if allowed_attributes is None:
-        allowed_attributes = []
-    if allowed_interface is not None:
-        for interface in allowed_interface:
-            allowed_attributes.extend(interface.names(all=True))
+    for n in ('', attribute):
+        required[n] = permission
+
+    _handle_allowed_interface(_context, allowed_interface, permission,
+                              required)
+    _handle_allowed_attributes(_context, allowed_attributes, permission,
+                               required)
 
     _handle_for(_context, for_)
 
+    _configure_z2security(_context, new_class, required)
+
     _context.action(
         discriminator = ('view', (for_, layer), name, IBrowserRequest),
         callable = handler,
         args = ('registerAdapter',
                 new_class, (for_, layer), Interface, name, _context.info),
         )
 
-        # Security
-
-    _context.action(
-        discriminator = ('five:protectClass', new_class),
-        callable = protectClass,
-        args = (new_class, permission)
-        )
-    if allowed_attributes:
-        for attr in allowed_attributes:
-            _context.action(
-                discriminator = ('five:protectName', new_class, attr),
-                callable = protectName,
-                args = (new_class, attr, permission)
-                )
-    # Make everything else private
-    allowed = [attribute] + (allowed_attributes or [])
-    private_attrs = [name for name in dir(new_class)
-                     if (not name.startswith('_')) and
-                        (name not in allowed) and
-                        ismethod(getattr(new_class, name))]
-    for attr in private_attrs:
-        _context.action(
-            discriminator = ('five:protectName', new_class, attr),
-            callable = protectName,
-            args = (new_class, attr, CheckerPrivateId)
-            )
-    # Protect the class
-    _context.action(
-        discriminator = ('five:initialize:class', new_class),
-        callable = InitializeClass,
-        args = (new_class,)
-        )
-
 
 class pages(zope.browserpage.metaconfigure.pages):
 
@@ -274,8 +283,17 @@ def publishTraverse(self, request, name,
         cdict['__name__'] = name
         newclass = makeClass(cname, bases, cdict)
 
+        for n in ('',):
+            required[n] = permission
+
+        _handle_allowed_interface(_context, allowed_interface, permission,
+                                  required)
+        _handle_allowed_attributes(_context, allowed_attributes, permission,
+                                   required)
         _handle_for(_context, for_)
 
+        _configure_z2security(_context, newclass, required)
+
         if self.provides is not None:
             _context.action(
                 discriminator = None,
@@ -291,41 +309,6 @@ def publishTraverse(self, request, name,
                     _context.info),
             )
 
-        # Security
-
-        _context.action(
-            discriminator = ('five:protectClass', newclass),
-            callable = protectClass,
-            args = (newclass, permission)
-            )
-
-        if allowed_attributes:
-            for attr in allowed_attributes:
-                _context.action(
-                    discriminator = ('five:protectName', newclass, attr),
-                    callable = protectName,
-                    args = (newclass, attr, permission)
-                    )
-
-        # Make everything else private
-        allowed = allowed_attributes or []
-        private_attrs = [name for name in dir(newclass)
-                         if (not name.startswith('_')) and
-                            (name not in allowed) and
-                            ismethod(getattr(newclass, name))]
-        for attr in private_attrs:
-            _context.action(
-                discriminator = ('five:protectName', newclass, attr),
-                callable = protectName,
-                args = (newclass, attr, CheckerPrivateId, False)
-                )
-
-        # Protect the class
-        _context.action(
-            discriminator = ('five:initialize:class', newclass),
-            callable = InitializeClass,
-            args = (newclass,)
-            )
 
 _factory_map = {'image':{'prefix':'ImageResource',
                          'count':0,
@@ -448,6 +431,14 @@ def resourceDirectory(_context, name, directory, layer=IDefaultBrowserLayer,
 class ViewMixinForAttributes(BrowserView,
                              zope.browserpage.metaconfigure.simple):
 
+    # XXX: this alternative implementation would support permission checks for
+    #      the attribute instead of the view
+    # def browserDefault(self, request):
+    #     return self, (self.__page_attribute__,)
+    #
+    # def publishTraverse(self, request, name):
+    #     return getattr(self, name)
+
     # For some reason, the 'simple' baseclass doesn't implement this
     # mandatory method (see https://bugs.launchpad.net/zope3/+bug/129296)
     def browserDefault(self, request):

src/Products/Five/browser/tests/pages.txt

@@ -205,11 +205,11 @@ high-level security tests).  Let's manually look up a protected view:
   >>> request = TestRequest()
   >>> view = getMultiAdapter((self.folder.testoid, request), name=u'eagle.txt')
 
-It's protecting the object with the permission, and not the attribute,
-so we get ('',) instead of ('eagle',):
+With the current browserDefault implementation permissions are checked for the
+object, and not for the attribute, but it is more robust to protect both:
 
   >>> view.__ac_permissions__
-  (('View management screens', ('',)),)
+  (('View management screens', ('', 'eagle')),)
 
 The view's __roles__ attribute can be evaluated correctly:
 
@@ -224,6 +224,21 @@ Just Works.)
   >>> aq_acquire(view, '__roles__')
   ('Manager',)
 
+  >>> aq_acquire(view, 'eagle__roles__')
+  ('Manager',)
+
+Other attributes are private:
+
+  >>> from AccessControl import ACCESS_PRIVATE
+  >>> aq_acquire(view, 'mouse__roles__') is ACCESS_PRIVATE
+  True
+
+In zope.browserpage this is just protected with the specified permission. Not
+sure if this has to be private in Zope 2:
+
+  >>> aq_acquire(view, 'publishTraverse__roles__') is ACCESS_PRIVATE
+  True
+
 Check to see if view's context properly acquires its true
 parent
 

src/Products/Five/browser/tests/test_zope3security.py

@@ -78,6 +78,12 @@ def test_allowed_interface():
       ...            xmlns:browser="http://namespaces.zope.org/browser">
       ...   <browser:page
       ...       for="*"
+      ...       name="testpage"
+      ...       permission="zope2.ViewManagementScreens"
+      ...       class="AccessControl.tests.testZCML.Dummy1"
+      ...       allowed_interface="AccessControl.tests.testZCML.IDummy" />
+      ...   <browser:view
+      ...       for="*"
       ...       name="testview"
       ...       permission="zope2.ViewManagementScreens"
       ...       class="AccessControl.tests.testZCML.Dummy1"
@@ -106,7 +112,7 @@ def test_allowed_interface():
       >>> from zope.component import getMultiAdapter
       >>> from zope.publisher.browser import TestRequest
       >>> request = TestRequest()
-      >>> view = getMultiAdapter((dummy1, request), name="testview")
+      >>> view = getMultiAdapter((dummy1, request), name="testpage")
 
     As 'foo' is defined in IDummy, it should have the 'Manager' role.
 
@@ -124,6 +130,16 @@ def test_allowed_interface():
       >>> getRoles(view, 'superMethod', view.superMethod, ('Def',))
       ('Manager',)
 
+   Same tests work using the view directive:
+
+      >>> view = getMultiAdapter((dummy1, request), name="testview")
+      >>> getRoles(view, 'foo', view.foo, ('Def',))
+      ('Manager',)
+      >>> getRoles(view, 'wot', view.wot, ('Def',)) is ACCESS_PRIVATE
+      True
+      >>> getRoles(view, 'superMethod', view.superMethod, ('Def',))
+      ('Manager',)
+
       >>> tearDown()
     """