Merge pull request #38 from delib/evilbungle-comment-acquisition

Fixing incorrect behaviour with acquired permissions

plone/app/discussion/conversation.py

@@ -50,8 +50,14 @@ from plone.app.discussion.comment import Comment
 
 from AccessControl.SpecialUsers import nobody as user_nobody
 
+from ComputedAttribute import ComputedAttribute
+
 ANNOTATION_KEY = 'plone.app.discussion:conversation'
 
+def computed_attribute_decorator(level=0):
+    def computed_attribute_wrapper(func):
+        return ComputedAttribute(func, level)
+    return computed_attribute_wrapper
 
 class Conversation(Traversable, Persistent, Explicit):
     """A conversation is a container for all comments on a content object.
@@ -87,21 +93,21 @@ class Conversation(Traversable, Persistent, Explicit):
         parent = aq_inner(self.__parent__)
         return parent.restrictedTraverse('@@conversation_view').enabled()
 
-    @property
+    @computed_attribute_decorator(level=1)
     def total_comments(self):
         public_comments = [
-            x for x in self._comments.values()
+            x for x in self.values()
             if user_nobody.has_permission('View', x)
         ]
         return len(public_comments)
 
-    @property
+    @computed_attribute_decorator(level=1)
     def last_comment_date(self):
         # self._comments is an Instance of a btree. The keys
         # are always ordered
         comment_keys = self._comments.keys()
         for comment_key in reversed(comment_keys):
-            comment = self._comments[comment_key]
+            comment = self[comment_key]
             if user_nobody.has_permission('View', comment):
                 return comment.creation_date
         return None
@@ -110,10 +116,10 @@ class Conversation(Traversable, Persistent, Explicit):
     def commentators(self):
         return self._commentators
 
-    @property
+    @computed_attribute_decorator(level=1)
     def public_commentators(self):
         retval = set()
-        for comment in self._comments.values():
+        for comment in self.values():
             if not user_nobody.has_permission('View', comment):
                 continue
             retval.add(comment.author_username)

plone/app/discussion/testing.py

@@ -41,10 +41,14 @@ class PloneAppDiscussion(PloneSandboxLayer):
         xmlconfig.file('configure.zcml',
                        plone.app.discussion,
                        context=configurationContext)
+        xmlconfig.file('configure.zcml',
+                       plone.app.discussion.tests,
+                       context=configurationContext)
 
     def setUpPloneSite(self, portal):
         # Install into Plone site using portal_setup
         applyProfile(portal, 'plone.app.discussion:default')
+        applyProfile(portal, 'plone.app.discussion.tests:testing')
 
         # Creates some users
         acl_users = getToolByName(portal, 'acl_users')

plone/app/discussion/tests/configure.zcml

@@ -0,0 +1,16 @@
+<configure
+  xmlns="http://namespaces.zope.org/zope"
+  xmlns:genericsetup="http://namespaces.zope.org/genericsetup"
+  i18n_domain="freitag.discussion">
+
+  <include package="plone.app.dexterity" />
+
+  <genericsetup:registerProfile
+    name="testing"
+    title="plone.app.discussion.testing"
+    directory="profile"
+    description="Testing profile for plone.app.discussion"
+    provides="Products.GenericSetup.interfaces.EXTENSION"
+    />
+
+</configure>

plone/app/discussion/tests/profile/types.xml

@@ -0,0 +1,4 @@
+<?xml version="1.0"?>
+<object name="portal_types" meta_type="Plone Types Tool">
+  <object name="sample_content_type" meta_type="Dexterity FTI" />
+</object>

plone/app/discussion/tests/profile/types/sample_content_type.xml

@@ -0,0 +1,47 @@
+<?xml version="1.0"?>
+<object name="sample_content_type"
+   meta_type="Dexterity FTI"
+   i18n:domain="freitag.discussion" xmlns:i18n="http://xml.zope.org/namespaces/i18n">
+
+  <!-- Basic metadata -->
+  <property name="title" i18n:translate="">sample_content_type</property>
+  <property name="description"
+    i18n:translate="">Sample Content</property>
+  <property name="content_icon">document_icon.png</property>
+  <property name="global_allow">True</property>
+  <property name="filter_content_types">True</property>
+  <property name="allowed_content_types">
+  </property>
+  <property name="allow_discussion">True</property>
+
+  <property name="klass">plone.dexterity.content.Item</property>
+
+  <property name="add_permission">cmf.AddPortalContent</property>
+  <property name="behaviors">
+    <element value="plone.app.content.interfaces.INameFromTitle" />
+  </property>
+
+  <!-- View information -->
+  <property name="default_view">view</property>
+  <property name="default_view_fallback">False</property>
+  <property name="view_methods">
+    <element value="view" />
+  </property>
+
+  <!-- Method aliases -->
+  <alias from="(Default)" to="(selected layout)" />
+  <alias from="edit" to="@@edit" />
+  <alias from="sharing" to="@@sharing" />
+  <alias from="view" to="@@view" />
+
+  <!-- Actions -->
+  <action title="View" action_id="view" category="object" condition_expr=""
+    url_expr="string:${object_url}/" visible="True">
+    <permission value="View" />
+  </action>
+
+  <action title="Edit" action_id="edit" category="object" condition_expr=""
+    url_expr="string:${object_url}/edit" visible="True">
+    <permission value="Modify portal content" />
+  </action>
+</object>

plone/app/discussion/tests/profile/workflows.xml

@@ -0,0 +1,4 @@
+<?xml version="1.0"?>
+<object name="portal_workflow" meta_type="CMF Workflow Tool">
+ <object name="comment_workflow_acquired_view" meta_type="Workflow"/>
+</object>

plone/app/discussion/tests/profile/workflows/comment_workflow_acquired_view/definition.xml

@@ -0,0 +1,75 @@
+<?xml version="1.0"?>
+<dc-workflow
+    workflow_id="comment_workflow_acquired_view"
+    title="Single State Workflow"
+    description="- Essentially a workflow with no transitions, but has a Published state, so portlets and applications that expect that state will continue to work."
+    state_variable="review_state"
+    initial_state="published"
+    manager_bypass="False">
+ <permission>Access contents information</permission>
+ <permission>Change portal events</permission>
+ <permission>Modify portal content</permission>
+ <permission>View</permission>
+ <state state_id="published" title="Published">
+  <description>Visible to everyone, editable by the owner.</description>
+  <permission-map name="Access contents information" acquired="False">
+   <permission-role>Anonymous</permission-role>
+  </permission-map>
+  <permission-map name="Change portal events" acquired="False">
+   <permission-role>Editor</permission-role>
+   <permission-role>Manager</permission-role>
+   <permission-role>Owner</permission-role>
+   <permission-role>Site Administrator</permission-role>
+  </permission-map>
+  <permission-map name="Modify portal content" acquired="False">
+   <permission-role>Editor</permission-role>
+   <permission-role>Manager</permission-role>
+   <permission-role>Owner</permission-role>
+   <permission-role>Site Administrator</permission-role>
+  </permission-map>
+  <permission-map name="View" acquired="True">
+  </permission-map>
+ </state>
+ <variable variable_id="action" for_catalog="False" for_status="True" update_always="True">
+  <description>Previous transition</description>
+  <default>
+   <expression>transition/getId|nothing</expression>
+  </default>
+  <guard>
+  </guard>
+ </variable>
+ <variable variable_id="actor" for_catalog="False" for_status="True" update_always="True">
+  <description>The ID of the user who performed the previous transition</description>
+  <default>
+   <expression>user/getId</expression>
+  </default>
+  <guard>
+  </guard>
+ </variable>
+ <variable variable_id="comments" for_catalog="False" for_status="True" update_always="True">
+  <description>Comment about the last transition</description>
+  <default>
+   <expression>python:state_change.kwargs.get('comment', '')</expression>
+  </default>
+  <guard>
+  </guard>
+ </variable>
+ <variable variable_id="review_history" for_catalog="False" for_status="False" update_always="False">
+  <description>Provides access to workflow history</description>
+  <default>
+   <expression>state_change/getHistory</expression>
+  </default>
+  <guard>
+   <guard-permission>Request review</guard-permission>
+   <guard-permission>Review portal content</guard-permission>
+  </guard>
+ </variable>
+ <variable variable_id="time" for_catalog="False" for_status="True" update_always="True">
+  <description>When the previous transition was performed</description>
+  <default>
+   <expression>state_change/getDateTime</expression>
+  </default>
+  <guard>
+  </guard>
+ </variable>
+</dc-workflow>

plone/app/discussion/tests/test_acquisition.py

@@ -0,0 +1,243 @@
+# -*- coding: utf-8 -*-
+from AccessControl.User import User  # before SpecialUsers
+from AccessControl.SpecialUsers import nobody as user_nobody
+from AccessControl.PermissionRole import rolesForPermissionOn
+from Acquisition import aq_chain, aq_base
+from plone.app.discussion.testing import \
+    PLONE_APP_DISCUSSION_INTEGRATION_TESTING
+from plone.app.discussion.interfaces import IConversation
+from plone.app.testing import TEST_USER_ID, setRoles
+from Products.CMFCore.utils import getToolByName
+from zope.component import createObject
+
+import unittest2 as unittest
+
+
+dexterity_type_name = 'sample_content_type'
+dexterity_object_id = 'instance-of-dexterity-type'
+archetypes_object_id = 'instance-of-archetypes-type'
+one_state_workflow = 'one_state_workflow'
+comment_workflow_acquired_view = 'comment_workflow_acquired_view'
+
+class AcquisitionTest(unittest.TestCase):
+    """ Define the expected behaviour of wrapped and unwrapped comments. """
+
+    layer = PLONE_APP_DISCUSSION_INTEGRATION_TESTING
+
+    def setUp(self):
+        self.portal = self.layer['portal']
+        self.request = self.layer['request']
+        setRoles(self.portal, TEST_USER_ID, ['Manager'])
+        self.wftool = getToolByName(self.portal, 'portal_workflow')
+
+        # Use customized workflow for comments.
+        self.wftool.setChainForPortalTypes(
+            ['Discussion Item'],
+            (comment_workflow_acquired_view,),
+        )
+
+        # Use one_state_workflow for Document and sample_content_type,
+        # so they're always published.
+        self.wftool.setChainForPortalTypes(
+            ['Document', dexterity_type_name],
+            (one_state_workflow,),
+        )
+
+        # Create a dexterity item and add a comment.
+        self.portal.invokeFactory(
+            id=dexterity_object_id,
+            title='Instance Of Dexterity Type',
+            type_name=dexterity_type_name,
+        )
+        
+        self.dexterity_object = self.portal.get(dexterity_object_id)
+        dx_conversation = IConversation(self.dexterity_object)
+        self.dexterity_conversation = dx_conversation
+        dx_comment = createObject('plone.Comment')
+        dx_conversation.addComment(dx_comment)
+        self.unwrapped_dexterity_comment = dx_comment
+        self.wrapped_dexterity_comment = dx_conversation[dx_comment.id]
+
+        # Create an Archetypes item and add a comment.
+        self.portal.invokeFactory(
+            id=archetypes_object_id,
+            title='Instance Of Archetypes Type',
+            type_name='Document',
+        )
+        
+        self.archetypes_object = self.portal.get(archetypes_object_id)
+        at_conversation = IConversation(self.archetypes_object)
+        self.archetypes_conversation = at_conversation
+        at_comment = createObject('plone.Comment')
+        at_conversation.addComment(at_comment)
+        self.unwrapped_archetypes_comment = at_comment
+        self.wrapped_archetypes_comment = at_conversation[at_comment.id]
+        
+
+    def test_workflows_installed(self):
+        """Check that the new comment workflow has been installed properly.
+        (Just a test to check our test setup.)
+        """
+        workflows = self.wftool.objectIds()
+        self.assertTrue('comment_workflow_acquired_view' in workflows)
+
+    def test_workflows_applied(self):
+        """Check that all objects have the workflow that we expect.
+        (Just a test to check our test setup.)"""
+        self.assertEqual(
+            self.wftool.getChainFor(self.archetypes_object),
+            (one_state_workflow,)
+        )
+        self.assertEqual(
+            self.wftool.getChainFor(self.dexterity_object),
+            (one_state_workflow,)
+        )
+        self.assertEqual(
+            self.wftool.getChainFor(self.unwrapped_archetypes_comment),
+            (comment_workflow_acquired_view,)
+        )
+        self.assertEqual(
+            self.wftool.getChainFor(self.unwrapped_dexterity_comment),
+            (comment_workflow_acquired_view,)
+        )
+
+    def test_comment_acquisition_chain(self):
+        """ Test that the acquisition chains for wrapped and unwrapped
+            comments are as expected. """
+        
+        # Unwrapped comments rely on __parent__ attributes to determine 
+        # parentage.  Frustratingly there is no guarantee that __parent__
+        # is always set, so the computed acquisition chain may be short.
+        # In this case the unwrapped AT and DX objects stored as the 
+        # conversation parents don't have a __parent__, preventing the portal
+        # from being included in the chain.
+        self.assertNotIn(self.portal,
+                         aq_chain(self.unwrapped_archetypes_comment))
+        self.assertNotIn(self.portal,
+                         aq_chain(self.unwrapped_dexterity_comment))
+
+        # Wrapped comments however have a complete chain and thus can find the
+        # portal object reliably.
+        self.assertIn(self.portal,aq_chain(self.wrapped_archetypes_comment))
+        self.assertIn(self.portal,aq_chain(self.wrapped_dexterity_comment))
+
+    
+    def test_acquiring_comment_permissions(self):
+        """ Unwrapped comments should not be able to acquire permissions
+            controlled by unreachable objects """
+        
+        # We use the "Allow sendto" permission as by default it is
+        # controlled by the portal, which is unreachable via __parent__
+        # attributes on the comments.
+        permission = "Allow sendto"
+
+        # Unwrapped comments can't find the portal so just return manager
+        self.assertNotIn("Anonymous",
+             rolesForPermissionOn(permission,
+                                  self.unwrapped_archetypes_comment))
+        self.assertNotIn("Anonymous",
+             rolesForPermissionOn(permission,
+                                  self.unwrapped_dexterity_comment))
+        
+        # Wrapped objects can find the portal and correctly return the
+        # anonymous role.
+        self.assertIn("Anonymous",
+             rolesForPermissionOn(permission,
+                                  self.wrapped_archetypes_comment))
+        self.assertIn("Anonymous",
+             rolesForPermissionOn(permission,
+                                  self.wrapped_dexterity_comment))
+    
+    def test_acquiring_comment_permissions_via_user_nobody(self):
+        """ The current implementation uses user_nobody.has_permission to
+            check whether anonymous can view comments.  This confirms it also
+            works. """        
+        
+        # Again we want to use a permission that's not managed by any of our
+        # content objects so it must be acquired from the portal.
+        permission = "Allow sendto"
+    
+        self.assertFalse(
+             user_nobody.has_permission(permission,
+                                        self.unwrapped_archetypes_comment))
+        
+        self.assertFalse(
+             user_nobody.has_permission(permission,
+                                        self.unwrapped_dexterity_comment))
+
+        self.assertTrue(
+             user_nobody.has_permission(permission,
+                                        self.wrapped_archetypes_comment))
+
+        self.assertTrue(
+             user_nobody.has_permission(permission,
+                                        self.wrapped_dexterity_comment))
+
+class AcquiredPermissionTest(unittest.TestCase):
+    """ Test methods of a conversation which rely on acquired permissions """
+
+    layer = PLONE_APP_DISCUSSION_INTEGRATION_TESTING
+
+    def setUp(self):
+        self.portal = self.layer['portal']
+        self.request = self.layer['request']
+        setRoles(self.portal, TEST_USER_ID, ['Manager'])
+        self.wftool = getToolByName(self.portal, 'portal_workflow')
+
+        # Disable workflow for comments and content.
+        self.wftool.setChainForPortalTypes(["Discussion Item"],[])
+        self.wftool.setChainForPortalTypes([dexterity_type_name],[])
+
+        # Create a dexterity item.
+        self.portal.invokeFactory(
+            id=dexterity_object_id,
+            title='Instance Of Dexterity Type',
+            type_name=dexterity_type_name,
+        )
+        
+        self.content = self.portal.get(dexterity_object_id)
+
+        # Absolutely make sure that we're replicating the case of an 
+        # incomplete chain correctly.
+        aq_base(self.content).__parent__ = None
+        
+        self.conversation = IConversation(self.content)
+        
+        # Add a comment
+        comment = createObject('plone.Comment')
+        self.conversation.addComment(comment)
+        self.comment = comment
+    
+    def test_view_permission_is_only_available_on_portal(self):
+        """ Check that the test setup is correct """
+        
+        content_roles = rolesForPermissionOn("View",aq_base(self.content))
+        self.assertNotIn("Anonymous",content_roles)
+        
+        comment_roles = rolesForPermissionOn("View",aq_base(self.comment))
+        self.assertNotIn("Anonymous",comment_roles)
+        
+        # This actually acquires view from the app root, but we don't really
+        # care, we just need to confirm that something above our content
+        # object will give us View.
+        portal_roles = rolesForPermissionOn("View",self.portal)
+        self.assertIn("Anonymous",portal_roles)
+
+    # The following tests fail when the conversation uses unwrapped comment
+    # objects to determine whether an anonymous user has the view permission.
+        
+    def test_total_comments(self):
+        self.assertEqual(self.conversation.total_comments,1)
+    
+    def test_last_comment_date(self):
+        self.assertEqual(self.conversation.last_comment_date,
+                         self.comment.creation_date)
+    
+    def test_public_commentators(self):
+        self.assertEqual(self.conversation.public_commentators,
+                         (self.comment.author_username,))
+
+
+
+def test_suite():
+    return unittest.defaultTestLoader.loadTestsFromName(__name__)