I set Workbench Access to base their permissions on menu:

- Menu of My Site
- - AAA
- - - BBB
- - - - CCC
- - - - - DDD
- - - - - - XXX

AAA,BBB,CCC,DDD,XXX - basic pages, their workbench sections are set to themselves,
i.e. DDD is in section DDD.

Now I create user. Now in order to edit "XXX" he has to be set as an editor of: "Menu of My Site", "AAA" or "XXX".
Any middle ones "BBB", "CCC", "DDD" doesn't give him permission of editing "XXX".

Is it messed up with my site? Maybe some inconsistencies in base or maybe this is how it was designed to work?
If I may provide some further details or logs - please let me know.

CommentFileSizeAuthor
#51 1422868-wa-menu-hierarchy-test.patch13.8 KBagentrickard
#48 1422868-wa-menu-hierarchy-test.patch9.84 KBagentrickard
#46 1422868-wa-menu-hierarchy-test.patch9.63 KBagentrickard
#44 1422868-wa-menu-hierarchy-test.patch6.28 KBagentrickard
#42 1422868-42-workbench_access-menu_hierarchy_test-do-not-test.patch5.93 KBjbylsma
#36 1422868-workbench_access-empty_menu_fix-36.patch1.15 KBagentrickard
#35 menu_permission_table.png39.39 KBjbylsma
#33 1422868-workbench_access-empty_menu_fix-29_2.patch1.58 KBjbylsma
#29 1422868-workbench_access-empty_menu_fix-29_1.patch1.28 KBagentrickard
#28 1422868-workbench_access-empty_menu_fix_with_cleanup-28.patch1.8 KBjbylsma
#28 1422868-workbench_access-empty_menu_fix-28.patch1.55 KBjbylsma
#27 Screen Shot 2012-10-01 at 9.27.48 AM.png74.56 KBjbylsma
#22 1422868-wa-hierarchy-followup.patch567 bytesagentrickard
#20 Screen Shot 2012-09-28 at 10.11.35 AM.png13.5 KBagentrickard
#18 Screen Shot 2012-09-27 at 6.27.45 PM.png12.72 KBagentrickard
#14 1422868-wa-menu-hierarchy.patch2.91 KBagentrickard
#8 1422868-8.patch2.88 KBjbylsma
#1 s1.png32.6 KBExceptionHandler
#1 s2.png28.96 KBExceptionHandler

Comments

ExceptionHandler’s picture

ExceptionHandler commented
StatusFileSize
new s2.png28.96 KB
new s1.png32.6 KB

Does my report mean anything? Am I wrong saying that there's something wrong with this? I'm suprised because this feature is quite essential.
I looked into the code and found this dependency:

1st situation (attachment s1.png picture). Then I edit workbench_access.module, I put
print_r($info)
on line 465, so in this case (opening node pointed by CCC) I see output:
Array ( [0] => 389 [1] => 390 [2] => 391 [3] => 437 )
which corresponds perfectly to menu item which user is set to an editor. ( AAA: 389, ..., DDD: 437 )

Now I change to 2nd situation (attachment s2.png picture). Now I see (still at CCC) seeing output:
Array ( [0] => main-menu )
which is not what you would expect considering output in previous case..

Is this how it should be? Maybe this was your purpose to make section descendancy only in case of 1st item in hierarchy or 2nd, and in case of 3rd level (or any higher) it doesn't inherit permission, only applies to single level.

/Sorry for any language inaccuracies, I'm doing my best to describe it./

ExceptionHandler’s picture

ExceptionHandler commented

My temporary solution:

Step 1: Add new function in workbench_access.module:

function ALT_workbench_access_menu_build_tree(&$tree, $link, $sections, $inc=FALSE) {
  $item = (object) $link['link'];
  
  if( $inc || in_array( intval($item->mlid), $sections) ) {
  		$inc = true ;
		$tree[$item->mlid] = array(
			'access_id' => $item->mlid,
			'access_type_id' => $item->menu_name,
			'name' => $item->link_title,
			'description' => isset($item->options['attributes']['title']) ? $item->options['attributes']['title'] : '',
			'weight' => $item->weight,
			'depth' => $item->depth,
			'parent' => ($item->plid == 0) ? $item->menu_name : $item->plid,
		  );
	}
  
  if (!empty($link['below'])) {
    foreach ($link['below'] as $below) {
      ALT_workbench_access_menu_build_tree($tree, $below, $sections, $inc);
    }
  }
}

Step 2: now in file menu.workbench_access.inc, add else condition around line 130 to make it look like:

foreach ($menu as $link) {
   // Ensure that we start at the top of the current request.
   if ($mlid > 0 && $link['link']['mlid'] == $mlid) {
	$tree = array();
       	_workbench_access_menu_build_tree($tree, $link);
   } elseif ($mlid == 0) {
    	_workbench_access_menu_build_tree($tree, $link);
   } else {
	// here we go deeper
	ALT_workbench_access_menu_build_tree($tree, $link, array_values($items));
   }
}

I hope some of module maintainers will finally make statement - is it bug/feature ;)

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Certainly looks like a bug. Not sure how to reproduce. Been working on #1101638: Default menu form support instead.

ExceptionHandler’s picture

ExceptionHandler commented

I would be happy to help by providing some details needed to reproduce it. Should be easy though as I recreated it myself on new instance of drupal quickly.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

A nice bullet list of steps would be perfect, thanks.

ExceptionHandler’s picture

ExceptionHandler commented
  1. turn on workbench access based on menu M
  2. create basic pages A, B, C, D, E
  3. put them in order to menu M, root to be A and E as a leaf (child of D)
  4. now they should become sections of workbench (refresh sections otherwise?)
  5. assign A to section A, B to section B, ..., E to section E
  6. - assign regular user U as an editor of section A – now he can edit A,B,C,D,E
  7. - assign regular user U as an editor of section B – now he can edit B,C,D,E
  8. - assign regular user U as an editor of section C – now he can edit only C (permission inheritance not working anymore)
  9. - assign regular user U as an editor of section D – now he can edit only D
  10. - assign regular user U as an editor of section E – now he can edit only E

Code I pasted in one of my previous posts shows that list of editable nodes includes proper nodes only when user has mlid of root or first level children.
/ if it's not quite a list you expected – tell me /

jbylsma’s picture

jbylsma commented

I also ran into this issue. Here's my test menu system (ultra verbose!):

  • 1
    • 2
      • 3
        • 4
          • 5
            • 6

Nothing fancy. Next, a chart of capturing what a user within Workbench Access can edit, depending on the assigned section level.
X axis: User can update
Y axis: Assigned section level

1 2 3 4 5 6
All of Menu X X X X X X
1 X X X X X
2 X X
3 X X
4 X X
5 X X
6 X

The problem comes from the menu_workbench_access_tree and _workbench_access_menu_build_tree. menu_workbench_access_tree is only designed to work with root level (where $mlid == 0) or first level (where $mlid is one of the first level menu items). Items deeper than the first level don't trigger any of the _workbench_access_menu_build_tree code, which allows an initialized tree array to pass back the menu's name (root level) as a valid area. The proper section level (the direct descendent of the section) stills works because of the "Simple equivalence check" at the beginning of workbench_access_in_tree.

Though I didn't see the code in #1422868-2: Menu hierarchy - permissions descendancy before, my implementation was fairly similar. The patch was written against 7.x-dev.

jbylsma’s picture

jbylsma commented
StatusFileSize
new 1422868-8.patch2.88 KB

Hit enter too fast...

jbylsma’s picture

jbylsma commented
Status: Active » Needs review

Why not one more comment. Needs review. I haven't checked against "Allow multiple section assignments."

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Needs work

Needs better documentation of the changes and the new parameter. A test would be ideal, but difficult.

jbylsma’s picture

jbylsma commented

Sorry, I should have remembered to document the parameter in the code. Were you also looking for better documentation of the recursive function in the code or a further explanation here on d.o (or something else)?

Are there are menu tests so far to extend? I quickly grepped the source but didn't see much.

Taxoman’s picture

Taxoman commented
Version: 7.x-1.0 » 7.x-1.x-dev
agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Better inline documentation of the recursive function.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Needs review
StatusFileSize
new 1422868-wa-menu-hierarchy.patch2.91 KB

Cleaned up version of the patch. Seems to work.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Fixed

Committed.

   4c4cc38..137fb94  7.x-1.x -> 7.x-1.x
fearlsgroove’s picture

fearlsgroove commented
Status: Fixed » Needs work

This change appears to be hiding top level menu items from being available as editorial sections. Reversing just this patch and clearing cache resolves the issue.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

/me gets irrational when no one tests patches before they get committed.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Postponed (maintainer needs more info)
StatusFileSize
new Screen Shot 2012-09-27 at 6.27.45 PM.png12.72 KB

I cannot replicate that issue using the Workbench Access form element. If you are using the native menu form, of course you cannot assign something to the top-level, because the Menu system doesn't allow it.

fearlsgroove’s picture

fearlsgroove commented
Status: Postponed (maintainer needs more info) » Needs work

It's not on the section element on the form, it's when deciding which menu entries are accessible as sections at admin/config/workbench/access/sections

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Postponed (maintainer needs more info)
StatusFileSize
new Screen Shot 2012-09-28 at 10.11.35 AM.png13.5 KB

Um...

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

When you say top-level, do you mean the Menu itself or the 1st-level children? Because I am missing 1st-level children.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new 1422868-wa-hierarchy-followup.patch567 bytes

Here's a patch.

fearlsgroove’s picture

fearlsgroove commented
Status: Needs review » Needs work

That's right first level children are missing. Sorry for the unclear description.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Needs review

Cross-post!

Note that screenshots are really helpful with this issue.

fearlsgroove’s picture

fearlsgroove commented
Status: Needs review » Reviewed & tested by the community

Cross post ... That seems to fix it. Looks good to me

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Reviewed & tested by the community » Fixed

Committed!

137fb94..b16f52c  7.x-1.x -> 7.x-1.x
jbylsma’s picture

jbylsma commented
Status: Fixed » Active
StatusFileSize
new Screen Shot 2012-10-01 at 9.27.48 AM.png74.56 KB

After disappearing and now returning, I found an issue where empty menus don't return the "All of {menu}" option. Mostly unnecessary screenshot attached. I'll dig in this morning.

jbylsma’s picture

jbylsma commented
Status: Active » Needs review
StatusFileSize
new 1422868-workbench_access-empty_menu_fix-28.patch1.55 KB
new 1422868-workbench_access-empty_menu_fix_with_cleanup-28.patch1.8 KB

The problem stems from having the initial $tree population within the menu_tree_all_data() $menu loop. If the menu is empty, menu_tree_all_data returns an empty array.

In an attempt to make life easier, I've attached two patches:

1422868-workbench_access-empty_menu_fix_with_cleanup-28.patch
Includes cleanup on the WA selected menus, as well removing some of the incorrectly labeled "mlids" text and vars
1422868-workbench_access-empty_menu_fix-28.patch
Just the menu fix
agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
StatusFileSize
new 1422868-workbench_access-empty_menu_fix-29_1.patch1.28 KB

I went with the easier patch. There is some strange logic that needs re-thinking here, but the fact is that we still need the array loop and, if I recall correctly, the check for mlid == 0. These are provided in the case that we have multiple top-level menu items (e.g. two access control menus) and in case the top-level menu item is not, for some reason, configured for access control.

jbylsma’s picture

jbylsma commented

That's cool. My test case was a couple of variations on a skeleton D7 install, so while it seemed right, I figured it'd be smarter to make two patches. Functionality seems to be working as expected with the current patch, but I won't muck with the status.

btw, thanks for your work and attentiveness on this module!

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Fixed

Well, we keep committing and then fixing this issue, so one more won't hurt.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented 
   efcf3ea..5cb859a  7.x-1.x -> 7.x-1.x
jbylsma’s picture

jbylsma commented
Status: Fixed » Needs review
StatusFileSize
new 1422868-workbench_access-empty_menu_fix-29_2.patch1.58 KB

All second level or lower menu items were matching, which is fixed. Switched $mlid to TRUE if an menu item matches and added a conditional to type check for that. Added a little documentation.

If I have the bandwidth to write menu tests, I will, but I doubt that is going to happen anytime soon.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Can you give a clear example of the error this fixes? A screen shot might help. In the original patch re-roll, I removed the TRUE check logic because it seemed unnecessary and inelegant.

jbylsma’s picture

jbylsma commented
StatusFileSize
new menu_permission_table.png39.39 KB

Sure thing. I copied out the administer menu page on a test site and diagrammed the issue with new columns.

During the child item check (!empty($link['below'])), if the child element's mlid is passed, then the built tree will detect the match and return that item, along with any item below it. That's why A1 and A1a are editable in the screenshot.

The $mlid TRUE functionality being set during a match specifies that all child elements should match. I'm OK refactoring it to be something nicer, though I'm not sure what you'd prefer.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
StatusFileSize
new 1422868-workbench_access-empty_menu_fix-36.patch1.15 KB

We were Doing It Wrong. We should only check 'below' if the parent item is valid.

jbylsma’s picture

jbylsma commented

This patch only works if the section is on the root level; the code will not check descendents for a match if the root level doesn't match. It does work as expected if the user is assigned "All of menu" or first level item.

Using the previous screenshot, only B1 gets "Actual update/delete," and that's because the simple check in workbench_access_in_tree matches.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

I can't replicate that in my tests. The patch seems to be working fine.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Priority: Normal » Critical

Anyone? Reviews?

azarzag’s picture

azarzag commented

The patch on #36 seems to be working fine for me too

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

We're working on tests for this now -- see #1878338: Test for Menu access control. I think the blocker is that it fails if you don't have all menu items used as active editing sections.

jbylsma’s picture

jbylsma commented
StatusFileSize
new 1422868-42-workbench_access-menu_hierarchy_test-do-not-test.patch5.93 KB

I've extended the menu test to check update abilities throughout the menu hierarchy. I've created a regular site that mirrors what the tests are doing and it appears as if the tests are properly returning (with a fair amount of failures). Also included is the renaming of the menu test nodes, which made it a little easier to diagnose!

Both #1422868-33: Menu hierarchy - permissions descendancy and #1422868-36: Menu hierarchy - permissions descendancy do not allow a user to update the assigned section. I wrote the test expecting a user to be able to update the assigned section's node. What is the expected update behavior for an assigned section's node?

#36 also has errors on the second level access tests, #33 does not.

Patch is not being tested because of the failing tests.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

OK, I committed the form issue, clearing the way for fixing up these fails.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
StatusFileSize
new 1422868-wa-menu-hierarchy-test.patch6.28 KB

Here's a re-roll that merges 36 with 42, and lets allow the tests to fail here for now.

Status: Needs review » Needs work

The last submitted patch, 1422868-wa-menu-hierarchy-test.patch, failed testing.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Needs review
StatusFileSize
new 1422868-wa-menu-hierarchy-test.patch9.63 KB

Alright, thanks to @jbylsma, I think I cracked the problem here. menu_tree_all_data() returns a nested array, so if we don't start at the top level, we never get into the proper loop.

This patch fixes up the latest and takes part of the approach from #33. Tests are green. So the question is whether they are _sufficient_.

Status: Needs review » Needs work

The last submitted patch, 1422868-wa-menu-hierarchy-test.patch, failed testing.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Needs review
StatusFileSize
new 1422868-wa-menu-hierarchy-test.patch9.84 KB

Hm. That tests green on my system. Minor re-roll to prevent an empty $account.

Status: Needs review » Needs work

The last submitted patch, 1422868-wa-menu-hierarchy-test.patch, failed testing.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Sigh. These are related to using random array items.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Needs review
StatusFileSize
new 1422868-wa-menu-hierarchy-test.patch13.8 KB

One more time.

jbylsma’s picture

jbylsma commented

Reviewed in on a vanilla D7 install and (with three level hierarchy) and a production site clone. Both appear to be working without issue :)

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Going once? Going twice?

I'm going to commit this tomorrow unless someone objects.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Fixed

Alright. Committed.

Big hat tip to @jbylsma for the test coverage.

   cd9ae04..f8ecaeb  7.x-1.x -> 7.x-1.x

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

gramma fix