Error when checking-in working copy

serviliusahala commented 8 years ago

My attempt to complete the check in of a working copy (http://pleiades.stoa.org/places/copy_of_589981/?) produced an error. The error was logged as 1470192500.750.866846775154 at about 10:45 PM EDT on 2 August 2016.

paregorios commented 8 years ago

This is confirmed. Traceback:

2016-08-02T22:48:20 ERROR Zope.SiteErrorLog 1470192500.750.866846775154 http://pleiades.stoa.org/places/copy_of_589981/@@content-checkin
Traceback (innermost last):
  Module ZPublisher.Publish, line 138, in publish
  Module ZPublisher.mapply, line 77, in mapply
  Module ZPublisher.Publish, line 48, in call_object
  Module plone.app.iterate.browser.checkin, line 50, in __call__
  Module plone.app.iterate.policy, line 90, in checkin
  Module pleiades.iterate.copier, line 69, in merge
  Module pleiades.iterate.copier, line 94, in _replaceBaseline
  Module OFS.CopySupport, line 116, in manage_cutObjects
CopyError: 
<HTML>
<HEAD>
<TITLE>Not Supported</TITLE>
</HEAD>
<BODY BGCOLOR="#FFFFFF">
<FORM ACTION="manage_main" METHOD="GET" >
<TABLE BORDER="0" WIDTH="100%" CELLPADDING="10">
<TR>
  <TD VALIGN="TOP">
  <BR>
  <CENTER><B><FONT SIZE="+6" COLOR="#77003B">!</FONT></B></CENTER>
  </TD>
  <TD VALIGN="TOP">
  <BR><BR>
  <CENTER>
  The action against the <em>batlas-location</em> object could not be carried out. One of the following constraints caused the problem: <br><br>The object does not support this operation.<br><br>-- OR --<br><br>The currently logged-in user does not have the <b>Copy or Move</b> permission respective to the object.
  </CENTER>
  </TD>
</TR>
<TR>
  <TD VALIGN="TOP">
  </TD>
  <TD VALIGN="TOP">
  <CENTER>
  <INPUT TYPE="SUBMIT" VALUE="   Ok   ">
  </CENTER>
  </TD>
</TR>
</TABLE>
</FORM>
</BODY></HTML>

alecpm commented 8 years ago

It appears that the "Copy or Move" permission is granted to Anonymous (which effectively means all users) at the Zope root, so there's a bit of a mystery here.

alecpm commented 8 years ago

I see what the issue is here. In order to perform the check-in the code does a cut/paste from the baseline to the working copy before checking in. The cut requires delete permissions on all the sub-objects, which the user checking in often doesn't have. I should be able to re-implement the custom check-in code to move those sub-objects without the permission checks.

alecpm commented 8 years ago

Additionally, in order to remove the working copy after checkin, the user needs to have permission to delete the working copy. In this case the working copy is in the "Pending review" state, which makes it not deletable by anyone besides a Site Administrator. Is it expected that people should be allowed to check in content that is in the "Pending review" state? If so, I can override this permission check as well.

alecpm commented 8 years ago

This is fixed on staging. Please test and confirm that current behaviors are acceptable:

1) The working copy is deleted on check-in, even if it is currently in a state that would not ordinarily allow deletion by the current user.

2) The current sub-objects from the baseline content are moved into the working copy before check in, if the working copy already has a sub-object with a matching id, the sub-object copied from the baseline will get an id like "copy_of_sub_object_id".

paregorios commented 8 years ago

@serviliusahala please test check-in of some items to see if we've cracked the nut here. Thanks.

serviliusahala commented 8 years ago

Same behavior as before. I still lack sufficient privileges to check in and publish a working copy. Attempted with this working copy from the pending list - http://pleiades.stoa.org/places/147997/copy_of_cessero - and was told I lacked privileges.

Jeffrey A. Becker, Ph.D. RPA jeffrey.becker@gmail.com; 617-877-4484 Twitter: @servilius_ahala https://twitter.com/servilius_ahala

On Wed, Aug 24, 2016 at 10:39 AM, Tom Elliott notifications@github.com wrote:

Assigned #198 https://github.com/isawnyu/pleiades-gazetteer/issues/198 to @serviliusahala https://github.com/serviliusahala.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/isawnyu/pleiades-gazetteer/issues/198#event-766079958, or mute the thread https://github.com/notifications/unsubscribe-auth/ADUQfWqbtEC46syI8c_Vn4TidjTDDXDgks5qjFe5gaJpZM4JblxB .

paregorios commented 8 years ago

@alecpm should I set up a test user for you to use on staging with group membership matching those that @serviliusahala has?

davisagli commented 8 years ago

From what I see above, it looks like Alec only deployed this to staging but @serviliusahala was testing on production?

paregorios commented 8 years ago

@davisagli aha! yes, that was my mistake in (mis)-guiding @serviliusahala. @alecpm I'm taking this back for further re-review (with apologies).

alecpm commented 8 years ago

Yes, the change is currently deployed for testing on staging only, but I can deploy to production if it's not practical to test fully on staging.

paregorios commented 8 years ago

@alecpm on staging I gave a user the same group memberships as @serviliusahala's user. I went to the item above and tried to check in the working copy. I got the same insufficient privileges error. Something's still wonky here, I'm afraid.

alecpm commented 8 years ago

@davisagli pointed out an oversight I made in deploying this one. I hadn't installed the collective.deletepermission add-on that this fix depended on in the control panel. After having installed the add-on, the check in appears to be working as a non-admin.

paregorios commented 8 years ago

I have tested this fix with appropriately-permissioned fake user accounts on staging and it works as expected. Please deploy to production so we can have @serviliusahala test in a real-world situation.

davisagli commented 8 years ago

@paregorios if I understood Alec correctly, the only thing left to deploy was the installation of collective.deletepermission which I did on production for #18.

paregorios commented 8 years ago

@serviliusahala please give this a test on production at your earliest convenience and let us know how it goes!

serviliusahala commented 8 years ago

I tried to do the check-in on one of the original name resources that spawned this error (https://pleiades.stoa.org/places/147997/copy_of_cessero) and I received the same 'insufficient privileges' notification when I attempted to check in the working copy.

JAB

Jeffrey A. Becker, Ph.D. RPA Department of Classical and Near Eastern Studies Binghamton University - SUNY jeffrey.becker@gmail.com; 617-877-4484 Twitter: @servilius_ahala https://twitter.com/servilius_ahala

On Wed, Nov 2, 2016 at 11:25 AM, Tom Elliott notifications@github.com wrote:

Assigned #198 https://github.com/isawnyu/pleiades-gazetteer/issues/198 to @serviliusahala https://github.com/serviliusahala.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/isawnyu/pleiades-gazetteer/issues/198#event-845075878, or mute the thread https://github.com/notifications/unsubscribe-auth/ADUQfV9nXtZ0dgjkALTSA9-nu52bWBb2ks5q6KtVgaJpZM4JblxB .

paregorios commented 8 years ago

@davisagli and @alecpm: this is still not working on production. Steps to reproduce:

Case 1

Login as a user that is a member of the "Editors" group (contact me for test user credentials)
Navigate to https://pleiades.stoa.org/places/147997/copy_of_cessero
Select "actions" -> "check in"
On the "Check in Cessero" page, enter a commit message and select the "check in" button
Get the "Insufficient Privileges" page

Case 2

Login as a user that is a member only of the "Members" group (contact me for test user credentials)
Navigate to https://pleiades.stoa.org/places/381896576
Select (sub "Names") the "Kenan Tepe" link (i.e., click through to https://pleiades.stoa.org/places/381896576/kenan-tepe)
Select "actions" -> "check out"
On the "check out kenan tepe" page, select the "check out" button
Edit and save the working copy, adding some text to the summary (description) field
Submit for review
Log out
Login as a user that is a member of the "Editors" group (contact me for test user credentials)
Navigate to the pending working copy you created and edited
Select "actions" -> "check in"
On the "Check in Kenan Tepe" page, enter a commit message and select the "check in" button
Get the "Insufficient Privileges" page

alecpm commented 8 years ago

It looks like this is working for places (which use pleiades.iterate's customizations), but not for place sub-content (which uses stock iterate methods).

Right now editors do not have permission to delete other owners content in any state. However, the standard iterate checkin requires permission to delete the working copy (regardless of state). We can workaround this by attempting to patch the check-in so that it doesn't check delete permissions (similar to what we do for places), or by modifying the workflow permissions to allow Editors to delete content more liberally (this would require migrating all content to update the permissions).

paregorios commented 8 years ago

@alecpm where do the two options rank on the following scales: difficulty, expense, maintainability?

alecpm commented 8 years ago

Expense and difficulty is comparable. As I see it the pros and cons are:

1) Changing Workflow Permissions

Pro: The most standard solution to these issues, no potential issues with future upgrades of packages.
Con: Requires granting delete permission more widely to editors than the original workflow definitions envisaged. Would allows editors to delete any content within a Place (editors would need to be able to delete any place sub-content in any workflow state that content might be in when checked in). Requires updating stored permissions for all content in the site via a migration script (we already have a script that would probably handle this with some minor tweaks).

2) Patching or overriding the code that replaces the baseline object to not check delete permission:

Pro: Allows you to keep current restrictive permissions on deleting content, and provides flexibility going forward in granting permissions that are only indirectly or confusingly related to the check-out check-in process. Does not require updating permissions for any existing content objects or any other migration of persistent settings.
Con: Requires patching core add-on code to override the default deletion behavior. Such a patch may require adjustments to work with future updates to plone.app.iterate.

paregorios commented 5 years ago

I suspect this is unchanged. I will test to verify that assumption and then move forward with this issue.

isawnyu / pleiades-gazetteer

Error when checking-in working copy #198

Case 1

Case 2