menu_tree_check_access filters it's query by status = 1. This is unexpected behavior when people have the 'administer nodes' permission, and is particularly problematic when dealing with unpublished book nodes, as the nodes don't show up in the parent item select list.

This patch does a simple check to see if the current user has the administer nodes permission, and omits the status = 1 condition if they do. Going to throw it against the wall and see if simpletest likes it, I'll follow up with test fixes if not.

CommentFileSizeAuthor
#21 drupal-menu_tree_check_access-520786-16.patch1.25 KBVikas.Kumar
#15 drupal-menu_tree_check_access-520786-15.patch1.25 KBrupertj
#14 drupal-menu_tree_check_access_for_admins-520786-11.patch663 bytesrupertj
#11 drupal-menu_tree_check_access_for_admins-520786-11.patch663 bytesrupertj
#6 drupal-menu_tree_check_access-520786-6.patch1.01 KBrooby
menu_tree_check_access.patch882 bytesbeeradb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

This is probably reasonable (and maybe even a 6.x bugfix) since there is no meaningful reason to limit administrators in this way.

beeradb’s picture

beeradb CreditAttribution: beeradb commented

Testbot likes it, pwolanin likes it - anyone else want to chime in? I'd like to get this backported for a d6 project I'm on which it might help with.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Looks sensible to me.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Reviewed & tested by the community

Makes sense.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Reviewed & tested by the community » Needs work

There is a now a permission for user_access('view own unpublished content'). just went in recently. see node_access() for example.

rooby’s picture

rooby CreditAttribution: rooby commented
Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
drupal-menu_tree_check_access-520786-6.patch1.01 KB

Here is the patch for D8 with added support for the 'view own unpublished content' permission.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Code looks fine, does this deserve a test case?

mdupont’s picture

mdupont
Primary language French
Location Switzerland
CreditAttribution: mdupont commented

Somewhat related to #50680: "Printer-friendly version" of unpublished book pages is blank issue I worked on. Looks like this patch would also fix the issue related there.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
CreditAttribution: kscheirer commented

#6: drupal-menu_tree_check_access-520786-6.patch queued for re-testing.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
CreditAttribution: kscheirer commented

Retesting against latest HEAD since it has been over a year.

rupertj’s picture

rupertj CreditAttribution: rupertj commented
FileSize
drupal-menu_tree_check_access_for_admins-520786-11.patch663 bytes

Here's a re-roll of beeradb's original patch against current 7.x HEAD. (I ran into the same issue in prod too...)

Status: Needs review » Needs work

The last submitted patch, drupal-menu_tree_check_access_for_admins-520786-11.patch, failed testing.

rupertj’s picture

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

#6: drupal-menu_tree_check_access-520786-6.patch queued for re-testing.

rupertj’s picture

rupertj CreditAttribution: rupertj commented
FileSize
drupal-menu_tree_check_access_for_admins-520786-11.patch663 bytes

This is a re-roll of rooby's patch from #6 against current D8 head. I've also swopped instances of user_access for $account->hasPermission() and got rid of the global $user from the previous patch.

rupertj’s picture

rupertj CreditAttribution: rupertj commented
FileSize
drupal-menu_tree_check_access-520786-15.patch1.25 KB

Uploaded the wrong patch. This is the right one.

Haza’s picture

Haza
Primary language French
CreditAttribution: Haza commented
Issue summary: View changes

Not sure of the status of this issue. Also, the menu_tree_check_access() function doesn't exists anymore in current D8 HEAD.

Maybe this could still apply on D7 ?

Haza’s picture

Haza
Primary language French
CreditAttribution: Haza commented
Status: Needs review » Needs work
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Closed (fixed)

This is outdated compared to HEAD. This code was removed.

rupertj’s picture

rupertj CreditAttribution: rupertj commented
Status: Closed (fixed) » Needs review

Can this issue not still be kept open to fix the issue in D7?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Version: 8.x-dev » 7.x-dev
Status: Needs review » Needs work

not sure this APi/security change will be accepted for 7.x, but you can try.

Vikas.Kumar’s picture

Vikas.Kumar CreditAttribution: Vikas.Kumar commented
Issue tags: +SprintWeekend2015
FileSize
drupal-menu_tree_check_access-520786-16.patch1.25 KB
mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: drupal-menu_tree_check_access-520786-16.patch, failed testing.

osopolar’s picture

osopolar
Primary language German
Location 🇩🇪 GER 🌐
CreditAttribution: osopolar commented

Drupal 7 uses global $user instead of $account = \Drupal::currentUser();

rooby’s picture

rooby CreditAttribution: rooby commented

In addition, you can call user_access() without having to pass in the global $user object. See previous D7 patches in this issue as an example.

Göran’s picture

Göran CreditAttribution: Göran commented

Please please, can anyone create a working patch for D7?
Above patches failed!

I have a critical problem - Authors with 'view own unpublished content'-permission can not find own unpublished pages in there own books!
- Authors only possibility to find own unpublished page is to writ page-address in URL (views-list not work or the core # /admin/content
The only permission who can see the pages in list or menus is the ''Bypass content access control'!

Göran’s picture

Göran CreditAttribution: Göran commented

Compromise solution found!
This issue describe a logical permission problem related to Book and core permission incompatibility. But there is very good module, view_unpublished, who solve some problem and make unpublished nodes visible to the author who did create them if you set up a Views and let a Views Display show them.

Its also good that you not need to set the permission: 'View any unpublished content' for the content type to be viewed! Views makes is possible to view the nodes through the view_unpublished module. Views is able to view all own nodes if the core permission 'view own unpublished content' is active fore the role.