Problem/Motivation

I do not believe the WorkflowConfigTransition::isAllowed() method always returns the value it should.

Here is the method as it exists in the 2.x-dev branch (and in at least 2.10) at the time of this writing:

  /**
   * Verifies if the given transition is allowed.
   *
   * - In settings;
   * - In permissions;
   * - By permission hooks, implemented by other modules.
   *
   * @return bool
   *   TRUE if OK, else FALSE.
   */
  public function isAllowed($user_roles) {
    if ($user_roles == 'ALL') {
      // Superuser.
      return TRUE;
    }
    elseif ($user_roles) {
      return array_intersect($user_roles, $this->roles) == TRUE;
    }
    return TRUE;
  }

What I see here is that it is first checking if ALL user roles are allowed, and if they are, it returning TRUE. Then, if user roles are provided, it is checking if the specific roles provided in the user roles are allowed (based on the value og $this->roles), and again, returning TRUE, if they do. Then the method is just returning TRUE.

I think the last return statement should be FALSE. Otherwise, if the method is called with several possible options, like an empty string or array (like if there was an error filling the roles array with data right before calling the method), it will be the same as calling it with 'ALL'.

I tested this by inserting the following lines to Workflow.php at line 584 (for 2.10 and line 588 for 2.x-dev, immediately after the line that reads foreach ($this->transitions as &$config_transition) {):

dpm($config_transition->isAllowed('ALL'), '$config_transition->isAllowed(ALL)');
dpm($config_transition->isAllowed($roles), '$config_transition->isAllowed($roles)');
dpm($config_transition->isAllowed(''), '$config_transition->isAllowed("")');
dpm($config_transition->isAllowed(array()), '$config_transition->isAllowed(array())');
dpm($config_transition->isAllowed('testing'), '$config_transition->isAllowed("testing")');
dpm($config_transition->isAllowed(array('testing')), '$config_transition->isAllowed(array("testing"))');
dpm($config_transition->isAllowed(NULL), '$config_transition->isAllowed(NULL)');
dpm($config_transition->isAllowed(TRUE), '$config_transition->isAllowed(TRUE)');
dpm($config_transition->isAllowed(FALSE), '$config_transition->isAllowed(FALSE)');
dpm($config_transition->isAllowed(0), '$config_transition->isAllowed(0)');
dpm($config_transition->isAllowed(1), '$config_transition->isAllowed(1)');
dpm($config_transition->isAllowed(2), '$config_transition->isAllowed(2)');

And then in a test module, I executed the following code:

    $user = user_load($some_user_id);
    $workflows = workflow_load_multiple();
    foreach ($workflows as $workflow) {
      $workflow->getTransitions(FALSE, array('roles' => array_keys($user->roles)));
    }

For all transitions, I received the following values for the various calls to isAllowed() listed above:

  • As uid 1: TRUE, TRUE, TRUE, TRUE, FALSE, FALSE, TRUE, TRUE, TRUE, TRUE, FALSE, FALSE
  • As a privileged user: TRUE, Mixed, TRUE, TRUE, FALSE, FALSE, TRUE, TRUE, TRUE, TRUE, FALSE, FALSE
  • As a normal, unprivileged user: TRUE, FALSE, TRUE, TRUE, FALSE, FALSE, TRUE, TRUE, TRUE, TRUE, FALSE, FALSE

Also, there were PHP notices, where appropriate, for the incorrect value types being passed to array_intersect() similar to this:

Warning: array_intersect(): Argument #1 is not an array in WorkflowConfigTransition->isAllowed() (line 113 of /path/to/drupal/sites/all/modules/contrib/workflow/includes/Entity/WorkflowConfigTransition.php).)

So, when checking $user_roles, a type check should be made, too, because I think that is causing successes where there should be failures.

Also, it seems that $user_roles == 'ALL' allows false positives that $user_roles === 'ALL' does not.

It is possible that the same situation is true for WorkflowTransition::isAllowed(), but the code is fundamentally different, and I did not test it.

Proposed resolution

Change the function to both validate the $user_roles variable before using it in array_intersect(), and to return FALSE at the end.

Remaining tasks

  1. Fix the issue
  2. Create a patch
  3. Review the patch
  4. Commit the patch

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

oadaeh created an issue. See original summary.

oadaeh’s picture

Assigned: oadaeh » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
1.09 KB

The attached patch addresses the three issues raised in this ticket.

oadaeh’s picture

Assigned: Unassigned » oadaeh
Status: Needs review » Needs work

I have discovered that there are several places in the module that actually make use of the fact that the function behaves the way it does, including, but not limited to:

  • public function getTransitionsByTid($tid, $roles = '', $reset = FALSE) {}
  • public function getTransitionsBySid($sid, $roles = '', $reset = FALSE) {}
  • public function getTransitionsByTargetSid($target_sid, $roles = '', $reset = FALSE) {}

I will submit a patch that corrects those and the other instances I find to use $roles = 'ALL'.

oadaeh’s picture

Assigned: oadaeh » Unassigned
Status: Needs work » Needs review
FileSize
4.44 KB

Okay, the attached patch fixes the calls to getTransitions() (which uses the roles values for its call to isAllowed()) to use 'ALL' in all the places where they are set to '' or array(). I also tweaked a couple of other related things and updated some of the commenting.

johnv’s picture

Thanks for your work. But remember:
-. ALL is used to skip all checks. Let us not make that the default.
-. IIRC the defaults are set after adding the third parameter. We better check for use of '', then use of All.

johnv’s picture

And do you have an actual use case that gives a wrong answer? This is a good initiative to have a better API, tough.

oadaeh’s picture

I do not have a use case. I was actually in there because $this->roles was a string for me (not an array) and causing me problems. When I read through the code, it occurred to me that the only time it would return FALSE was if the roles did not match. Because the 'ALL' option exists for an override, I made an assumption that the default should be FALSE. However, if that is not the case, and if the default is to be TRUE, then the 'ALL' option is unnecessary, and all that is needed is to verify that $user_roles is an array and not empty, and everything else can just fall out the end of the function.

  • johnv committed 65cf689 on 7.x-2.x authored by oadaeh
    Issue #2894974 by oadaeh: Add documentation
    
johnv’s picture

I am sorry. This module is very old, and has lots of installations. I am reluctant to add these changes and possibly change its behaviour. All code is taken from the 1.2 version, and translated to the 2.x OO-code, leaving as much code as possible backwards compatible.
In this late phase of the D7-life cycle, I do not wish to change this and spend time in testing the results.

Thanks for your effort, though. I have committed the documentation improvements.

oadaeh’s picture

John, thank you for your consideration. I understand and appreciate your position, and I might make the same decision, if I were you. I think this can be marked Fixed, for that which was added, unless you feel otherwise.

johnv’s picture

Version: 7.x-2.x-dev » 7.x-2.10
Status: Needs review » Fixed

Thanks,

Status: Fixed » Closed (fixed)

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