Take a user without permissions to view or edit some content type.

Lets say that content type nid is 45.

The said user go to the url and trigger a lock on content they are not allowed to edit or even view: http://example/node/45/edit

I can login as a user who _is_ authorized to edit and if the above user did what was mentioned, the "this document was locked by.." does in fact show up!

Additional Potentially Relevant Modules Installed: ACL, Content Access.

CommentFileSizeAuthor
#4 422870-checkout-access.patch1.34 KBsmk-ka
#3 checkout.module-check_access.patch1.2 KBthekevinday
#2 checkout.module-check_access.patch1.19 KBsmk-ka
#1 checkout-6.x-2.x-lock_access_fix-1.patch1.17 KBthekevinday
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thekevinday’s picture

thekevinday CreditAttribution: thekevinday commented
FileSize
checkout-6.x-2.x-lock_access_fix-1.patch1.17 KB
smk-ka’s picture

smk-ka CreditAttribution: smk-ka commented
Status: Active » Needs review
FileSize
checkout.module-check_access.patch1.19 KB

Nice catch! I've just fixed on place: node_access() requires a node object, not a nid. Otherwise this looks good to me.

thekevinday’s picture

thekevinday CreditAttribution: thekevinday commented
FileSize
checkout.module-check_access.patch1.2 KB

Thanks for the correction, i rushed the patch out without much testing at all...

Something still went wrong during my testing, I looked at http://api.drupal.org/api/function/node_access/6

Apparently, it should not be 'edit' but instead be 'update'.

smk-ka’s picture

smk-ka CreditAttribution: smk-ka commented
FileSize
422870-checkout-access.patch1.34 KB

Found a better solution using menu_get_item(): the hardwired node_access('update', ...) only checks for node edit permissions, which might be sufficient for node/%/edit, but not any other path that can be locked. For example, on node/%/outline we need to check for book.module's "administer outline" permission. This can be done by directly querying the menu system and letting it figure out the access permissions.

Leeteq’s picture

Leeteq CreditAttribution: Leeteq commented

Subscribing.

thekevinday’s picture

thekevinday CreditAttribution: thekevinday commented

I have had the #4 patch applied for a little while now without any related troubles popup.

cels’s picture

cels CreditAttribution: cels commented

Has anyone tested if there are problems with
· Taxonomy Access Control
· Domain Access
· Domain Access pach: multiple_node_access patch (allows you to use AND logic instead of OR logic: ALL of the access' modules must allow you to view/edit the node, not only one)

I try to reproduce the scenary and test it.

itsnotme’s picture

itsnotme CreditAttribution: itsnotme commented

I didn't check the patches yet, but I just found out that at least some of the "unauthorized" locking was caused by users opening the revisions tab of a content.

Whoa. That is an unexpected behavior. The message "...is locked" appears, but I wonder why the lock happens at all, because revisions should only be displayed at that point.

stacysimpson’s picture

stacysimpson CreditAttribution: stacysimpson commented
Status: Needs review » Needs work

+1. We are also getting hit with the inadvertent locking of content when navigating to the revisions tab.

stacysimpson’s picture

stacysimpson CreditAttribution: stacysimpson commented

We've confirmed that the patch from #4 does not work with 'Taxonomy Access Control Lite', 'Module Grants' and 'Workflow Access'. The patch from #3 does work though.

We are still interested in a fix for inadvertent locking when navigating to the 'revisions' tab.

stacysimpson’s picture

stacysimpson CreditAttribution: stacysimpson commented

Got time to look through the code, the following line allows people to override the locking behavior using a variable:
$patterns = variable_get('checkout_edit_paths', "edit\nrevisions\nrevisions/*\noutline\nclassify");

We added only 'edit' to the settings.php file and it seems to work fine for us. Still believe the default setting shouldn't lock when navigating to the revisions tab.

itsnotme’s picture

itsnotme CreditAttribution: itsnotme commented

In settings.php? Why did you do anything in that one? *confused*

I tried using the line
$patterns = variable_get('checkout_edit_paths', "edit");

but then nothing gets locked properly, despite the message being shown. Help would still be appreciated.

thekevinday’s picture

thekevinday CreditAttribution: thekevinday commented

perhaps something like $op should be passed to the function in question so that we know if the operation is treated as an update and can handle permissions?

stacysimpson’s picture

stacysimpson CreditAttribution: stacysimpson commented

@itsnotme. The 'variable_get' function will use the provided variable's value ('checkout_edit_paths' in this instance) or the provided default value. As opposed to changing the source code, we choose to set the variable's value in settings.php. 

$conf = array(
#   'site_name' => 'My Drupal site',
#   'theme_default' => 'minnelli',
#   'anonymous' => 'Visitor',
   'checkout_edit_paths' => 'edit',
);

Looks like it appropriately locks the node (on edit only) to me.

EugenMayer’s picture

EugenMayer CreditAttribution: EugenMayer commented

You might want check http://drupal.org/project/content_lock . Its a fork(and partially rewrite) of checkout and activly maintained. It does not seem that checkout is maintained anymore (not officially confirmed though)