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
Fix the issueCreate a patch- Review the patch
- Commit the patch
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#4 | workflow_fix-isallowed-return-values_2894974_4.patch | 4.44 KB | oadaeh |
Comments
Comment #2
oadaeh CreditAttribution: oadaeh at Hook 42 commentedThe attached patch addresses the three issues raised in this ticket.
Comment #3
oadaeh CreditAttribution: oadaeh at Hook 42 commentedI 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:
I will submit a patch that corrects those and the other instances I find to use $roles = 'ALL'.
Comment #4
oadaeh CreditAttribution: oadaeh at Hook 42 commentedOkay, 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.
Comment #5
johnvThanks 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.
Comment #6
johnvAnd do you have an actual use case that gives a wrong answer? This is a good initiative to have a better API, tough.
Comment #7
oadaeh CreditAttribution: oadaeh at Hook 42 commentedI 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.
Comment #9
johnvI 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.
Comment #10
oadaeh CreditAttribution: oadaeh at Hook 42 commentedJohn, 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.
Comment #11
johnvThanks,