Better Access Control in Custom Triggers

Note: when referring to hook_info above, I mean "hook_hook_info"

Note: This can be worked around by using "hook_menu_alter".

Looking into patching this, it is more complicated that originally expected. Due to the structure of the array that "hook_hook_info" provides, more than just the menu item will need to be changed. I will still try to create a patch but it will take me some time.

I am not sure what scared me so much before, but this was a pretty easy fix. I just allowed for two new options in the hook_hook_info() (this is a bad name, by the way). These are names as the same items in the menu array.

trigger.module in trigger_menu():

  // We want contributed modules to be able to describe
  // their hooks and have actions assignable to them.
  $hooks = module_invoke_all('hook_info');
  foreach ($hooks as $module => $hook) {
    // We've already done these.
    if (in_array($module, array('node', 'comment', 'user', 'system', 'taxonomy'))) {
      continue;
    }
    $info = db_result(db_query("SELECT info FROM {system} WHERE name = '%s'", $module));
    $info = unserialize($info);
    $nice_name = $info['name'];
    $items["admin/build/trigger/$module"] = array(
      'title' => $nice_name,
      'page callback' => 'trigger_assign',
      'page arguments' => array($module),
      'access arguments' => isset($hook['access arguments']) ? $hook['access arguments'] : array($module),
      'access callback' => isset($hook['access callback']) ? $hook['access callback'] : 'user_access',
      'type' => MENU_LOCAL_TASK,
    );
  }

Also changed documentation in trigger.api.php. An example hook_hook_info() implementation for the node module if it needed to use these:

function hook_hook_info() {
  return array(
    'node' => array(
      'node' => array(
        'presave' => array(
          'runs when' => t('When either saving a new post or updating an existing post'),
        ),
        'insert' => array(
          'runs when' => t('After saving a new post'),
        ),
        'update' => array(
          'runs when' => t('After saving an updated post'),
        ),
        'delete' => array(
          'runs when' => t('After deleting a post')
        ),
        'view' => array(
          'runs when' => t('When content is viewed by an authenticated user')
        ),
      ),
      'access callback' => 'user_access',
      'access arguments' => 'administer actions',
    ),
  );
}

I just had an idea. The default access callback for any modules that implement this hook, should be traigger_access() (I believe that is it, not looking at code). As I imagine this is why the default access argument is the module name. I can't believe I am just seeing this.

I'll make another patch.

It was the trigger_access_check() function. New patch. Also, now that it finally clicked why the default access argument would be the module name, I definitely think this is a bug and putting it back to such.

I created a new patch with proper tests. In doing this, I came across a few new things:

* Bug: #551002: Bad Query in Menu Creation in Trigger
* in trigger_forms, we need to make a check for the new array indexes.

#296322: Tests for abort of actions firing in a loop + trigger module API is completely broken + fix changed around the trigger module. Got rid of trigger_access_check() so I put the default callback for hook implemented triggers as user_access(). Uploading new patch that accounts for changes.

Ugh! Replacing the default access callback to user_access() basically recreated the original issue. So, now, the default call back is user_access() but the default access argument is 'administer actions'. Should pass tests now.

back to needs review.

updated. re-rolled.

I had put this on the back burner because it is a bug and I heard there was some major work going into this module, so I was just going to wait until after the slush to redo the patch. There is already tests in the patch, fyi, but yes the last patch is way dated. Will pick back up soon.

This no longer a bug. The current version of the trigger module just uses "administer actions" for all access to trigger pages instead of the broken module name. Though I do not think this is a good way to handle it, it is adequate and I have never really run into anyone that needs the functionality of providing menu access with custom triggers (the menu alter hooks are a get around anyway). Also, sine is now a feature request, it will not get into D7. Putting as D8 and postponing.

@moritzz, as you have an ccount on drupal.org, you can edit handbook pages instead of just leaving a comment.