Unable to chain in front of some system paths

Applying CMA to every menu item — that's a very interesting stress test, thank you for your feedback!

Your patch in #3 makes perfect sense.

What do you mean in #4 with

Somehow, the core obviously bother to call user_access() if a module has not defined any 'access arguments'.

? Does core grant or deny access if neither 'access callback' nor 'access arguments' is specified?

Re. #4: The normalization

  $item += array('access callback' => 'user_access', 'access arguments' => array());

fails if it sets both items. We should probably do something like

  $item += array(
    'access callback' => (isset($item['access arguments']) ? 'user_access' : TRUE),
    'access arguments' => array(),
  );

instead.

Your work-around in #5 means that you're not chaining items that don't set 'type'. Is that the intended behavior? (I don't know if there are any such menu items.)

Do you know what core does if you specify a different access for a MENU_DEFAULT_LOCAL_TASK than for its parent?

How is that better than just leaving the MENU_DEFAULT_LOCAL_TASKs alone and letting core do the inheriting? Why do you need to chain the default children at all?

Core doesn't just set the access arguments (as your comment seems to say) but also the access callback of the default child, does it not?

So what does core do, if the parent and the default child have different access?

If the parent and default child have different access, then Core will use what ever has been defined. There is a special case in core (it's even referred to as a special case in the core docs) where there is supposed to be a default behavior for MENU_DEFAULT_LOCAL_TASKs to default to the access callback and arguments to those of the parent item if they have not been defined on the child.. I don't know the reasoning for this - I guess it saves module authors a few lines of code here and there.

Very interesting. I'd say it's actually a bug if the access of MENU_DEFAULT_LOCAL_TASKs is specified rather than inherited. Since node/%/view shows the same thing as node/%, its access should also be the same, and the best way to ensure this is to inherit it.

So, I still think it would be best for you to leave the MENU_DEFAULT_LOCAL_TASKs alone and let them default to their parent. Especially if you're a good citizen and keep in mind that some other module might come after yours and expect to find things as they normally are in core. Something like

/**
 * Implements hook_menu_alter().
 */
function path_access_menu_alter(&$items) {
  foreach ($items as $key => $value) {
    if (!isset($value['type']) || $value['type'] != MENU_DEFAULT_LOCAL_TASK) {
      chain_menu_access_chain($items[$key], '_path_access_check_permission');
    }
  }
}

which is close to what you had in #5, except for a slight variation of the condition.

I'm beginning to think that chain_menu_access_chain() should refuse to do anything on MENU_DEFAULT_LOCAL_TASK items and return FALSE.

If 'type' is not specified, it defaults to MENU_NORMAL_ITEM, i.e. the condition in #12 is correct rather than the one in #5. However, you're right, #12 only works if the access of the MENU_DEFAULT_LOCAL_TASK item is fully inherited, and otherwise your module would miss the item.

Core does support having different access on MENU_DEFAULT_LOCAL_TASKs vs. their parents, either different 'access callback' and 'access arguments' or just different 'access callback', even though that doesn't really make sense from the user perspective.

CMA should provide the same support that core does:

1. If we chain a MENU_DEFAULT_LOCAL_TASK child and ...
   
   1. it does not specify its own access, then we need to copy the parent item's access before chaining onto it, otherwise we lose that information.
   2. it specifies only 'access arguments', then we need to copy the parent item's 'access callback' before chaining onto it, otherwise we risk calling the wrong function with the right arguments.
   3. it specifies both 'access callback' and 'access arguments': we can chain normally.
2. We have to worry about the parent items, too! If we chain a parent item and...
   
   1. the MENU_DEFAULT_LOCAL_TASK child does not specify its own access, then the child can continue to inherit its parent's access; all is well, nothing to do.
   2. the MENU_DEFAULT_LOCAL_TASK child specifies only 'access arguments', then we're breaking the child item right there. This means we must look for such a child and copy the parent's 'access callback' to it before chaining the parent.
   3. the MENU_DEFAULT_LOCAL_TASK child specifies both 'access callback' and 'access arguments': we could try to check whether they are the same as the parent's, but it's probably best to treat them as disconnected, just chain the parent, and let the caller worry about the situation.
      However, this means that the caller can easily be surprised, if he expects the situation as it's in core, but a module such as yours has run before him. [Your module should probably try to run last!] I'm not sure we can solve this problem...
3. There's a challenge here: calling menu_get_item() from hook_menu_alter() is probably not a good idea, which means we can't get to the parent/child items. We'll need to have the entire menu array passed to us from the caller.

It shouldn't be the CMA client's responsibility to worry about these things. The reason for having CMA is that we implement this once and for all, so that it just works.

Did I miss something?

So the only thing from a MENU_DEFAULT_LOCAL_TASK definition should be used is the weight and the title. I can see Drupal uses the access control when drawing the tab itself but that's plain wrong because the href points to the parent and so the parent's access control should be used. That smells like a core bug to me.

Nonetheless, the simplest thing to do is to never set access callback on MENU_DEFAULT_LOCAL_TASK and let core handle it. As per the comment in chain_menu_access_chain , it probably breaks on default tabs -- if that's a problem we can add a check into chain_menu_access_chain so that MENU_DEFAULT_LOCAL_TASK is never altered. Now.

function _menu_check_access(&$item, $map) {
  // Determine access callback, which will decide whether or not the current
  // user has access to this path.
  $callback = empty($item['access_callback']) ? 0 : trim($item['access_callback']);
  // Check for a TRUE or FALSE value.
  if (is_numeric($callback)) {
    $item['access'] = (bool) $callback;
  }

vs

function _chain_menu_access_callback_call($access_callback, $access_arguments) {
  if (is_numeric($access_callback)) {
    // It's a number (see hook_menu()).
    return (bool) $access_callback;
  }

that's a clear-cut bug, we need $access_callback = empty($access_callback) ? 0 : trim($access_callback); in here to match core.

The plan is for CMA to refuse to chain MENU_DEFAULT_LOCAL_TASK items. They should never specify their own access and always inherit from their parent item.

@mrfelton:

Your code in #12 wont work, because the path node/%/view will end up not being protected as it's a MENU_DEFAULT_LOCAL_TASK. This is the problem that let me to the solution in #11 in the first place.

Why do you write this? According to your #14, node/%/view will come in without any access specification, and if we keep it that way, it will subsequently (in _menu_router_build()) get assigned the access specification of node/%, which you've protected.

@chx:

Regarding the access check in menu_local_tasks(): I think the items have been passed through _menu_router_build(), which has applied the inheritance by copying the parent's access specification to the default tab item. Thus it should be OK to check the child's access.

Please try this out and let us know how it goes, without chaining the MENU_DEFAULT_LOCAL_TASK items.

salvis' patch is correct , it's mostly copy - paste out of menu.inc (router build and menu check access).

@chx: Thank you for your help with this (also behind the scenes)!

So the only thing from a MENU_DEFAULT_LOCAL_TASK definition should be used is the weight and the title.

Should we try to add this to the hook_menu() documentation?

Thanks, I've committed this to D7 and created BETA3.

@chx: We don't have exceptions in D6. What should we do? Return FALSE?

http://drupal.org/project/usage/chain_menu_access shows that there's little interest in the D6 version, but I'd still like to keep it current.

I'm not sure why chx put the second condition there, but I thought it wouldn't hurt.

@chx: Can we remove it?

(I'm on the road and may only be able to take care of this in 10 days.)

Sure, I removed it (because salvis travels didnt want to hold you up with it).

Thank you chx, I just found (actually working!) Internet access at the place where I'm staying for most of the week.

@mrfelton: Please open a new issue (linking to this one would be helpful) if another problem pops up.

Good to know — I appreciate your help with getting this to work universally.

Fixed and released BETA4 for D7 and D6.

There are still issues with MENU_DEFAULT_LOCAL_TASK with the current code, when the parent menu item uses chain_menu_access_chain() :

If the default task is defined as

$items['foo/bar'] = array(
  'title' => $title,
  'page callback' => 'my_callback',
  'access arguments' => array('administer content types'),
  'type' => MENU_DEFAULT_LOCAL_TASK,
);

- No 'access callback', so _menu_router_build() will use the one from the parent, which has been altered to '_chain_menu_access_callback'.
- But there are explicit 'access arguments', so _menu_router_build() keeps them and doesn't override from the parent 'access arguments' (which have been adapted for _chain_menu_access_callback()).
- So when checking access for 'foo/bar', _menu_check_access() calls _chain_menu_access_callback('administer content types'). Strange stuff ensues, since that's not the kind of arguments that the callback expects.

[edit : using 6.x-1.0-beta4. My use case is "alter all existing menu items" as well]

It is an error to specify access arguments (and page callback for that matter) on the MENU_DEFAULT_LOCAL_TASK.

Look at the example:

// Make "Foo settings" appear on the admin Config page
$items['admin/config/foo'] = array(
  'title' => 'Foo settings',
  'type' => MENU_NORMAL_ITEM,
  // Page callback, etc. need to be added here.
);
// Make "Global settings" the main tab on the "Foo settings" page
$items['admin/config/foo/global'] = array(
  'title' => 'Global settings',
  'type' => MENU_DEFAULT_LOCAL_TASK,
  // Access callback, page callback, and theme callback will be inherited
  // from 'admin/config/foo', if not specified here to override.
);
// Make an additional tab called "Node settings" on "Foo settings"
$items['admin/config/foo/node'] = array(
  'title' => 'Node settings',
  'type' => MENU_LOCAL_TASK,
  // Page callback and theme callback will be inherited from
  // 'admin/config/foo', if not specified here to override.
  // Need to add access callback or access arguments.
);

It says "if not specified here to override" but overriding any of the functional keys does not make sense for the MENU_DEFAULT_LOCAL_TASK, even if the core code explicitly allows overriding the access arguments and inheriting the access callback. This is a conceptual error in core, IMO.

So, please ask the creator of the menu item to fix it.

Yup, I ended up with something like that in my hook_menu_alter() :

foreach ($items as &$item) {
  if (!isset($item['type']) || $item['type'] != MENU_DEFAULT_LOCAL_TASK) {
    chain_menu_access_chain($item, ...);
  }
  elseif ($item['type'] == MENU_DEFAULT_LOCAL_TASK) {
    // Both will be inherited from the parent.
    unset($item['access arguments'], $item['access callback']);
  }
}

Maybe something like the above could be integrated in chain_menu_access_chain() itself when called on a MENU_DEFAULT_LOCAL_TASK, instead of a no-op (D6) or an exception (D7) ? Just remove the access params, and rely on the parent's - that's assuming the parent item will get passed to chain_menu_access_chain() too, but that's only reasonable IMO, since the parent path is the one users will actually visit, so it's very likely to be the one you focus on when altering in access rules.

Specifyng access params (moreover partial ones) on a MENU_DEFAULT_LOCAL_TASK might be a silly idea, but declaring it an error is more like an afterthought from chx, and it's currently definitely and explicitly supported by the menu API, and lots of contrib module out there do it - at least in D6.

Other than that, BTW, the module works great in D6, and IMO is ready for an 1.0 release :-)

You mean silently unset the MENU_DEFAULT_LOCAL_TASK's access parameters when chain_menu_access_chain() is called on it, rather than complaining or ignoring? But what do we do if clients do NOT call cmac() for the MENU_DEFAULT_LOCAL_TASKs, as we've been teaching them so far?

It does make sense (and allows us to shift the burden of handling misbehaved third-party modules to the client module), but it's a complete reversal of our strategy. The alternative would be to go out and search for any dependent MENU_DEFAULT_LOCAL_TASK and clean it, but that's a bit of a drag...

@chx: What do you say?

Other than that, BTW, the module works great in D6, and IMO is ready for an 1.0 release :-)

Thanks! Where are you using CMA for D6? That version has seen very little usage so far...

what do we do if clients do NOT call cmac() for the MENU_DEFAULT_LOCAL_TASKs, as we've been teaching them so far?

If they don't call cmac() on MDLTs then, as currently, then the kind of incorrect access checks on the MDLT, pointed in #35, can happen.
If we go as proposed in #37, the cmac() phpdoc should state something like "if cmac() is applied to a MDLT, it should be applied to the parent item as well, and reversedly".
But we're talking about allowing something that was forbidden so far, not the opposite, so there's no BC break. Less headaches on the calling side ("Am I allowed to call cmac() on this menu item ?"), and removes some buggy cases, all benefit IMO.

Where are you using CMA for D6? That version has seen very little usage so far...

Yeah, not sure why, the module fills a need that's not specific to D7.
Anyway. My use case is a setup serving two separate sites through Domain Access. The sites share some content but are fairly different so most menu callbacks available on one site should be hidden on the other. So I stick an additional menu access callback on top of every menu item, and filter whitelists / blacklists depending on the domain. Gives 403s instead of 404s, but we can live with that.

Access control on default local tasks are weird, can't imagine a valid use case. We can be broken in that case: garbage in, garbage out. That said, we can be broken in any way we want. If you CMA the parent callback and not the default task, little we can do. So yched's idea is a solid one and I wouldn't worry about #38. I said, little we can do -- but we can check in the access callback whether count($args[0]) == 5 and if not THEN throw the exception.

So, how about this (for D6)?

Nitpicks :

+++ b/chain_menu_access.moduleundefined
@@ -15,6 +15,14 @@
+ * access but instead clear it, so that sane behavior (i.e. inheriting from

"clear it" is not too clear ;-). "... but instead remove its access properties, so that..."

+++ b/chain_menu_access.moduleundefined
@@ -33,12 +41,14 @@
+ *   TRUE if the chaining succeeded, FALSE if a MENU_DEFAULT_LOCAL_TASK item
+ *   was passed (see the note above). Be sure to chain the parent item in the

I'm not sure the return value is needed at all, but that's probably not really important.

As I see this, the idea is "you can call cmac() indifferently on any menu item. However, be sure apply the same cmac() call to a MENU_DEFAULT_LOCAL_TASK and its parent item".

+++ b/chain_menu_access.moduleundefined
@@ -33,12 +41,14 @@
+    // Inherit the parent's access! See #1186208 for details.

I think the common practice is to provide full http://d.o urls

+++ b/chain_menu_access.moduleundefined
@@ -78,7 +88,13 @@ function _chain_menu_access_callback() {
+    // Something's wrong, probably failed to clear this MENU_DEFAULT_LOCAL_TASK
+    // item (see chain_menu_access_chain()). Deny access!

I think chx proposed an exception here. Might make sense, because silently denying access might cause a fair amount of headscratching ("why is my tab not displaying" ?)

Thank you for your comments.

"... but instead remove its access properties, so that..."

ok

I'm not sure the return value is needed at all

We don't want to change the API. "TRUE if the chaining succeeded" still holds, and calling cmac() on an MENU_DEFAULT_LOCAL_TASK only doesn't have the (presumably) desired effect, so we cannot claim it succeeded.

I think the common practice is to provide full http://d.o urls

ok

I think chx proposed an exception here.

Yes, we'll do that for D7/D8, but D6 doesn't have exceptions. That's why we didn't have it in D6 before.

Here's the updated patch with the two changes. Can you try this for your usage (that's why I started with D6)?

Right, agreed on your replies.
Patch works for me :-)

I found that I had previously committed a line of code at the top of the wrong function in D6. Here's the fixed version for D6 as well as a version for D7/D8.

Pushed to the D6/D7/D8 -dev versions.