From e54b33acd9475d5eaa66ce03b6fc790b52c44aed Mon Sep 17 00:00:00 2001 From: Ross Patterson Date: Thu, 21 Feb 2019 11:52:41 -0800 Subject: [PATCH 1/6] Fix flake8 warning --- plone/app/iterate/browser/info.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plone/app/iterate/browser/info.py b/plone/app/iterate/browser/info.py index 8a6316e..2c5ee00 100644 --- a/plone/app/iterate/browser/info.py +++ b/plone/app/iterate/browser/info.py @@ -92,7 +92,7 @@ def properties(self): return {} def _getReference(self): - raise NotImplemented + raise NotImplementedError() class BaselineInfoViewlet(BaseInfoViewlet): From 2f002a28358390e795a0b11adaeab154513cbb11 Mon Sep 17 00:00:00 2001 From: Ross Patterson Date: Thu, 21 Feb 2019 11:55:17 -0800 Subject: [PATCH 2/6] Use the Iterate permissions instead of CMF core permissions #67 Also #59 --- news/67.bugfix | 2 ++ plone/app/iterate/browser/configure.zcml | 2 +- plone/app/iterate/browser/control.py | 15 ++++++----- plone/app/iterate/browser/info.py | 14 +++++++--- plone/app/iterate/configure.zcml | 20 +++++++------- plone/app/iterate/copier.py | 4 ++- plone/app/iterate/permissions.py | 7 ++--- plone/app/iterate/testing.py | 6 +++++ plone/app/iterate/tests/browser.rst | 34 ++++++++++++++---------- 9 files changed, 66 insertions(+), 38 deletions(-) create mode 100644 news/67.bugfix diff --git a/news/67.bugfix b/news/67.bugfix new file mode 100644 index 0000000..acd58c4 --- /dev/null +++ b/news/67.bugfix @@ -0,0 +1,2 @@ +Use the Iterate permissions instead of CMF core permissions to support +customizing the policy. [rpatterson] \ No newline at end of file diff --git a/plone/app/iterate/browser/configure.zcml b/plone/app/iterate/browser/configure.zcml index ffcb1f4..79d6a7e 100644 --- a/plone/app/iterate/browser/configure.zcml +++ b/plone/app/iterate/browser/configure.zcml @@ -27,7 +27,7 @@ name="content-checkin" class=".checkin.Checkin" template="checkin.pt" - permission="cmf.ModifyPortalContent" + permission="plone.app.iterate.CheckInContent" /> + + + + @@ -85,16 +95,6 @@ handler=".event.handleDeletion" /> - - - - diff --git a/plone/app/iterate/copier.py b/plone/app/iterate/copier.py index 32b77ed..d43e806 100644 --- a/plone/app/iterate/copier.py +++ b/plone/app/iterate/copier.py @@ -116,7 +116,9 @@ def _replaceBaseline(self, baseline): self.context._v_is_cp = 0 wc_id = self.context.getId() - wc_container.manage_delObjects([wc_id]) + # Bypass AT security check, + # checking `iterate : Check in content` should be sufficient + wc_container._delObject(wc_id) # move the working copy back to the baseline container working_copy = aq_base(self.context) diff --git a/plone/app/iterate/permissions.py b/plone/app/iterate/permissions.py index 7978780..2a71db2 100644 --- a/plone/app/iterate/permissions.py +++ b/plone/app/iterate/permissions.py @@ -26,6 +26,7 @@ CheckinPermission = 'iterate : Check in content' CheckoutPermission = 'iterate : Check out content' -DEFAULT_ROLES = ('Manager', 'Owner', 'Site Administrator', 'Editor') -addPermission(CheckinPermission, default_roles=DEFAULT_ROLES) -addPermission(CheckoutPermission, default_roles=DEFAULT_ROLES) +addPermission(CheckinPermission, default_roles=( + 'Manager', 'Site Administrator', 'Reviewer')) +addPermission(CheckoutPermission, default_roles=( + 'Manager', 'Owner', 'Site Administrator', 'Editor')) diff --git a/plone/app/iterate/testing.py b/plone/app/iterate/testing.py index d645ced..f525dee 100644 --- a/plone/app/iterate/testing.py +++ b/plone/app/iterate/testing.py @@ -34,10 +34,16 @@ 'password': 'secret', 'roles': ['Contributor'], } +REVIEWER = { + 'id': 'reviewer', + 'password': 'secret', + 'roles': ['Reviewer'], +} USERS_TO_BE_ADDED = ( ADMIN, EDITOR, CONTRIBUTOR, + REVIEWER, ) USERS_WITH_MEMBER_FOLDER = ( EDITOR, diff --git a/plone/app/iterate/tests/browser.rst b/plone/app/iterate/tests/browser.rst index 95098c5..b2daa44 100644 --- a/plone/app/iterate/tests/browser.rst +++ b/plone/app/iterate/tests/browser.rst @@ -92,6 +92,7 @@ check out published pages:: True >>> browser = z2.Browser(app) + >>> browser.handleErrors = False >>> browser.addHeader('Authorization', 'Basic editor:secret') >>> browser.open(portal.absolute_url() + '/hello-world') @@ -113,19 +114,12 @@ therefore our Editor lacks permissions to modify the original:: ... LinkNotFoundError -The Editor could, however, ask someone to retract the original so he -gains permissions again and check in (and then possibly request for -review):: +The Reviewer can check the working copy back in to update the original +published item:: >>> browser = z2.Browser(app) - >>> browser.addHeader('Authorization', - ... 'Basic %s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD)) - >>> browser.open(portal.absolute_url() + '/hello-world') - >>> browser.getLink("Published").click() - >>> browser.getControl("Retract").click() - >>> browser.getControl("Save").click() - >>> browser = z2.Browser(app) - >>> browser.addHeader('Authorization', 'Basic editor:secret') + >>> browser.handleErrors = False + >>> browser.addHeader('Authorization', 'Basic reviewer:secret') >>> browser.open(portal.absolute_url() + '/hello-world') >>> browser.getLink("working copy").click() >>> browser.getLink("Check in").click() @@ -261,8 +255,14 @@ Create a new page to test workflows with:: Checkout:: + >>> import transaction + >>> from plone import api + >>> api.user.grant_roles(username='editor', roles=['Contributor']) + >>> transaction.commit() + >>> browser = z2.Browser(app) - >>> browser.addHeader('Authorization', 'Basic contributor:secret') + >>> browser.handleErrors = False + >>> browser.addHeader('Authorization', 'Basic editor:secret') >>> browser.open(workflow_test_url) >>> browser.getLink(id='plone-contentmenu-actions-iterate_checkout').click() >>> browser.contents @@ -271,16 +271,19 @@ Checkout:: >>> checkout_form.getControl('Parent folder').selected = True >>> checkout_form.getControl('Check out').click() >>> browser.contents - '...This is a working copy of...My workflow test..., made by...contributor...' + '...This is a working copy of...My workflow test..., made by...editor...' >>> browser.contents '...state-draft-copy...' >>> workflow_checkout_url = browser.url + >>> api.user.revoke_roles(username='editor', roles=['Contributor']) + >>> transaction.commit() + Check get info message on original:: >>> browser.open(workflow_test_url) >>> browser.contents - '...This item is being edited by...contributor...a working copy...' + '...This item is being edited by...editor...a working copy...' We're going to manually give the contributor user the CheckoutPermission to check it's used when displaying the info messages. In our workflow @@ -288,6 +291,7 @@ once the checked out item is submitted the contributor no longer has permission to modify it but we still want them to see the info messages:: >>> browser = z2.Browser(app) + >>> browser.handleErrors = False >>> browser.addHeader('Authorization', ... 'Basic %s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD)) @@ -297,6 +301,7 @@ permission to modify it but we still want them to see the info messages:: >>> browser.getControl('Save Changes').click() >>> browser = z2.Browser(app) + >>> browser.handleErrors = False >>> browser.addHeader('Authorization', 'Basic contributor:secret') >>> browser.open(workflow_checkout_url) >>> browser.getLink(id='workflow-transition-submit-copy-for-publication')\ @@ -314,6 +319,7 @@ move permissions in our workflow so this should not appear in the action menu. http://code.google.com/p/dexterity/issues/detail?id=258 :: >>> browser = z2.Browser(app) + >>> browser.handleErrors = False >>> browser.addHeader('Authorization', 'Basic editor:secret') >>> browser.open(workflow_checkout_url) >>> browser.getLink(id='plone-contentmenu-actions-copy') From e4d5009cc65acee626da7a0ac29e17f22ce96db3 Mon Sep 17 00:00:00 2001 From: Ross Patterson Date: Mon, 25 Feb 2019 14:32:58 -0800 Subject: [PATCH 3/6] Fix error when checking in from a memebers home folder --- plone/app/iterate/base.py | 12 +++++++++--- plone/app/iterate/copier.py | 7 ++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/plone/app/iterate/base.py b/plone/app/iterate/base.py index d22c6b7..6104969 100644 --- a/plone/app/iterate/base.py +++ b/plone/app/iterate/base.py @@ -28,6 +28,8 @@ from Acquisition import aq_inner from Acquisition import aq_parent +from AccessControl import SpecialUsers + from plone.app.iterate import interfaces from plone.app.iterate.event import BeforeCheckoutEvent from plone.app.iterate.event import CancelCheckoutEvent @@ -35,6 +37,8 @@ from plone.app.iterate.interfaces import ICheckinCheckoutPolicy from plone.app.iterate.interfaces import IObjectCopier from plone.app.iterate.util import get_storage +from plone import api + from Products.CMFCore import interfaces as cmf_ifaces from Products.CMFCore.utils import getToolByName from zope import component @@ -151,10 +155,12 @@ def _removeDuplicateReferences(self, item, backrefs=False): def _copyBaseline(self, container): # copy the context from source to the target container source_container = aq_parent(aq_inner(self.context)) - clipboard = source_container.manage_copyObjects([self.context.getId()]) - result = container.manage_pasteObjects(clipboard) + with api.env._adopt_user(SpecialUsers.system): + clipboard = source_container.manage_copyObjects( + [self.context.getId()]) + result = container.manage_pasteObjects(clipboard) # get a reference to the working copy target_id = result[0]['new_id'] target = container._getOb(target_id) - return target \ No newline at end of file + return target diff --git a/plone/app/iterate/copier.py b/plone/app/iterate/copier.py index d43e806..c1e5812 100644 --- a/plone/app/iterate/copier.py +++ b/plone/app/iterate/copier.py @@ -28,9 +28,13 @@ from Acquisition import aq_base from Acquisition import aq_inner from Acquisition import aq_parent +from AccessControl import SpecialUsers + from plone.app.iterate import interfaces from plone.app.iterate.base import BaseContentCopier from plone.app.iterate.interfaces import CheckinException +from plone import api + from Products.Archetypes.Referenceable import Referenceable from Products.CMFCore.utils import getToolByName from Products.DCWorkflow.DCWorkflow import DCWorkflowDefinition @@ -118,7 +122,8 @@ def _replaceBaseline(self, baseline): wc_id = self.context.getId() # Bypass AT security check, # checking `iterate : Check in content` should be sufficient - wc_container._delObject(wc_id) + with api.env._adopt_user(SpecialUsers.system): + wc_container.manage_delObjects([wc_id]) # move the working copy back to the baseline container working_copy = aq_base(self.context) From 50d9d0f4e1553b6ae0656db033c241f38bfa7b63 Mon Sep 17 00:00:00 2001 From: Ross Patterson Date: Mon, 25 Feb 2019 14:33:55 -0800 Subject: [PATCH 4/6] Better permission check --- plone/app/iterate/browser/info.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plone/app/iterate/browser/info.py b/plone/app/iterate/browser/info.py index 88d6d09..4c17ddf 100644 --- a/plone/app/iterate/browser/info.py +++ b/plone/app/iterate/browser/info.py @@ -109,7 +109,7 @@ def render(self): sm.checkPermission( permissions.CheckoutPermission, self.context) or sm.checkPermission( - permissions.CheckinPermission, self.context) or + permissions.CheckinPermission, working_copy) or sm.checkPermission(ModifyPortalContent, working_copy)): return self.index() else: From a9ae6b8fda2e183b5bbe6ca39d9c0a241f5d51fd Mon Sep 17 00:00:00 2001 From: Ross Patterson Date: Tue, 5 Mar 2019 13:49:10 -0800 Subject: [PATCH 5/6] Avoid circular dependencies, remove plone.api dependency --- plone/app/iterate/base.py | 7 ++++--- plone/app/iterate/copier.py | 8 ++++---- plone/app/iterate/util.py | 26 ++++++++++++++++++++++++-- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/plone/app/iterate/base.py b/plone/app/iterate/base.py index 6104969..9420180 100644 --- a/plone/app/iterate/base.py +++ b/plone/app/iterate/base.py @@ -28,7 +28,6 @@ from Acquisition import aq_inner from Acquisition import aq_parent -from AccessControl import SpecialUsers from plone.app.iterate import interfaces from plone.app.iterate.event import BeforeCheckoutEvent @@ -37,10 +36,11 @@ from plone.app.iterate.interfaces import ICheckinCheckoutPolicy from plone.app.iterate.interfaces import IObjectCopier from plone.app.iterate.util import get_storage -from plone import api +from plone.app.iterate import util from Products.CMFCore import interfaces as cmf_ifaces from Products.CMFCore.utils import getToolByName + from zope import component from zope.component import queryAdapter from zope.event import notify @@ -155,7 +155,8 @@ def _removeDuplicateReferences(self, item, backrefs=False): def _copyBaseline(self, container): # copy the context from source to the target container source_container = aq_parent(aq_inner(self.context)) - with api.env._adopt_user(SpecialUsers.system): + + with util.adopt_system(): clipboard = source_container.manage_copyObjects( [self.context.getId()]) result = container.manage_pasteObjects(clipboard) diff --git a/plone/app/iterate/copier.py b/plone/app/iterate/copier.py index c1e5812..bf918fe 100644 --- a/plone/app/iterate/copier.py +++ b/plone/app/iterate/copier.py @@ -28,17 +28,14 @@ from Acquisition import aq_base from Acquisition import aq_inner from Acquisition import aq_parent -from AccessControl import SpecialUsers from plone.app.iterate import interfaces from plone.app.iterate.base import BaseContentCopier from plone.app.iterate.interfaces import CheckinException -from plone import api from Products.Archetypes.Referenceable import Referenceable from Products.CMFCore.utils import getToolByName from Products.DCWorkflow.DCWorkflow import DCWorkflowDefinition -from .relation import WorkingCopyRelation from ZODB.PersistentMapping import PersistentMapping from zope import component from zope import interface @@ -46,6 +43,9 @@ from zope.event import notify from zope.lifecycleevent import ObjectMovedEvent +from . import util +from .relation import WorkingCopyRelation + @interface.implementer(interfaces.IObjectCopier) @component.adapter(interfaces.IIterateAware) @@ -122,7 +122,7 @@ def _replaceBaseline(self, baseline): wc_id = self.context.getId() # Bypass AT security check, # checking `iterate : Check in content` should be sufficient - with api.env._adopt_user(SpecialUsers.system): + with util.adopt_system(): wc_container.manage_delObjects([wc_id]) # move the working copy back to the baseline container diff --git a/plone/app/iterate/util.py b/plone/app/iterate/util.py index 744b94a..17b5fec 100644 --- a/plone/app/iterate/util.py +++ b/plone/app/iterate/util.py @@ -21,10 +21,19 @@ # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA ################################################################## -from .interfaces import annotation_key +import contextlib + from persistent.dict import PersistentDict -from Products.CMFPlone.utils import get_installer + from zope.annotation import IAnnotations +from zope import globalrequest + +from AccessControl import SpecialUsers +from AccessControl import SecurityManagement + +from Products.CMFPlone.utils import get_installer + +from .interfaces import annotation_key def get_storage(context, default=None): @@ -40,3 +49,16 @@ def upgrade_by_reinstall(context): qi = get_installer(context) qi.uninstall_product('plone.app.iterate') qi.install_product('plone.app.iterate') + + +@contextlib.contextmanager +def adopt_system(user=SpecialUsers.system): + """ + Execute this block of code as the system user. + """ + old_security_manager = SecurityManagement.getSecurityManager() + SecurityManagement.newSecurityManager(globalrequest.getRequest(), user) + + yield + + SecurityManagement.setSecurityManager(old_security_manager) From c90e2c38e9c6e5cdfe8926ca5c53a3802926f5f4 Mon Sep 17 00:00:00 2001 From: Ross Patterson Date: Tue, 5 Mar 2019 14:22:52 -0800 Subject: [PATCH 6/6] Allow controlling access based on who initiated the check out --- news/67.bugfix | 5 ++-- plone/app/iterate/base.py | 7 ++++++ plone/app/iterate/browser/control.py | 2 +- plone/app/iterate/tests/test_iterate.py | 32 +++++++++++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/news/67.bugfix b/news/67.bugfix index acd58c4..e12de1f 100644 --- a/news/67.bugfix +++ b/news/67.bugfix @@ -1,2 +1,3 @@ -Use the Iterate permissions instead of CMF core permissions to support -customizing the policy. [rpatterson] \ No newline at end of file +Add support for customizing the policy of checking out and in using Zope's ACL +security. Use the Iterate permissions instead of CMF core permissions and +allow controlling access based on who initiated the check out. [rpatterson] \ No newline at end of file diff --git a/plone/app/iterate/base.py b/plone/app/iterate/base.py index 9420180..647d52b 100644 --- a/plone/app/iterate/base.py +++ b/plone/app/iterate/base.py @@ -28,6 +28,7 @@ from Acquisition import aq_inner from Acquisition import aq_parent +from AccessControl import SecurityManagement from plone.app.iterate import interfaces from plone.app.iterate.event import BeforeCheckoutEvent @@ -164,4 +165,10 @@ def _copyBaseline(self, container): # get a reference to the working copy target_id = result[0]['new_id'] target = container._getOb(target_id) + + security_manager = SecurityManagement.getSecurityManager() + target.manage_addLocalRoles( + security_manager.getUser().getId(), + ('iterate: Check out initiator', )) + return target diff --git a/plone/app/iterate/browser/control.py b/plone/app/iterate/browser/control.py index 3392cad..a99b1cd 100644 --- a/plone/app/iterate/browser/control.py +++ b/plone/app/iterate/browser/control.py @@ -62,7 +62,7 @@ def checkin_allowed(self): if original is None: return False - can_check_in = checkPermission(permissions.CheckinPermission, original) + can_check_in = checkPermission(permissions.CheckinPermission, context) if not can_check_in: return False diff --git a/plone/app/iterate/tests/test_iterate.py b/plone/app/iterate/tests/test_iterate.py index 3c5d0d7..f7a9f27 100644 --- a/plone/app/iterate/tests/test_iterate.py +++ b/plone/app/iterate/tests/test_iterate.py @@ -22,9 +22,12 @@ ################################################################## from AccessControl import getSecurityManager + from plone.app.iterate.browser.control import Control from plone.app.iterate.interfaces import ICheckinCheckoutPolicy +from plone.app.iterate import testing from plone.app.iterate.testing import PLONEAPPITERATEDEX_INTEGRATION_TESTING + from plone.app.testing import login from plone.app.testing import setRoles from plone.app.testing import TEST_USER_ID @@ -207,3 +210,32 @@ def test_control_cancel_on_original_does_not_delete_original(self): from plone.app.iterate.interfaces import CheckoutException with self.assertRaises(CheckoutException): cancel() + + def test_local_role(self): + """ + A local role is assigned for the user making the checkout. + """ + self.portal.portal_membership.addMember( + testing.EDITOR['id'], testing.EDITOR['password'], + testing.EDITOR['roles'], []) + login(self.portal, testing.EDITOR['id']) + + baseline = self.portal.docs.doc1 + policy = ICheckinCheckoutPolicy(baseline, None) + working_copy = policy.checkout(self.portal.workarea) + + # Verify assumptions + self.assertEqual( + baseline.get_local_roles_for_userid(TEST_USER_ID), ('Owner', ), + 'Different Owner local role than this test assumes') + self.assertEqual( + working_copy.get_local_roles_for_userid(TEST_USER_ID), ('Owner', ), + 'Different Owner local role than this test assumes') + self.assertEqual( + baseline.get_local_roles_for_userid('editor'), (), + 'Different check out user local role than this test assumes') + + self.assertEqual( + working_copy.get_local_roles_for_userid('editor'), + ('iterate: Check out initiator', ), + 'Wrong check out initiator local role')