Page MenuHomePhabricator

Title::checkPermissionHooks docs for "return true" with "$result = false" state that user should not be allowed to proceed
Open, MediumPublic

Description

Author: ju.ju2004

Description:
I had some suprising results with 'userCan' hook: user was allowed to edit a page although the $result value returned by the function hooked was false. So I took a look at the code. Title::UserCan calls Title::getUserPermissionErrorsInternal which calls Title::checkPermissionHooks, which runs 'UserCan' hook. Here is a piece of this code :

private function checkPermissionHooks( $action, $user, $errors, $doExpensiveQueries, $short ) {
...
if ( !wfRunHooks( 'userCan', array( &$this, &$user, $action, &$result ) ) ) {

		return $result ? array() : array( array( 'badaccess-group0' ) );

}
...
}

If the function called by hook 'userCan' (or 'getUserPermissionsErrors' or 'getUserPermissionsErrorsExpensive') returns true, the value of $result is never considered. That means that a false "$result" will have no effect on the result of Title::UserCan. Is this conscious ? Should my hook return $result instead of true ?


Version: 1.17.x
Severity: minor
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=34856

Details

Reference
bz30821

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:50 PM
bzimport set Reference to bz30821.
bzimport added a subscriber: Unknown Object (MLST).

Returning true usually indicates that you are passing regular control back to the caller, as if you were never there.

Returning false short-circuits further processing, and in a case like this you have a chance to also return some override value through the $result outparam. You can then return either true or false in there, as I understand.

ju.ju2004 wrote:

Okay, but I expected something like what was described in http://www.mediawiki.org/wiki/Manual:Hooks/userCan#Table_of_combinations :

  • return true:
    • $result = true: User should be allowed to proceed. Later functions can override.
    • $result = false: User should not be allowed to proceed. Later functions can override.
  • return false:
    • $result = true: User should be allowed to proceed. Later functions not consulted.
    • $result = false: User should not be allowed to proceed. Later functions not consulted.

But the real table of combinations seems to be this one :

  • return true:
    • $result = true: User is allowed to proceed. Later functions can override.
    • $result = false: USER IS ALLOWED TO PROCEED. Later functions can override.
  • return false:
    • $result = true: User is allowed to proceed. Later functions not consulted.
    • $result = false: User is not allowed to proceed. Later functions not consulted.

Is there a reason for this ?

resolving invalid since this is just a question, not a bug report.

ju.ju2004 wrote:

(In reply to comment #3)

resolving invalid since this is just a question, not a bug report.

I still think this is a bug! But I'm not sure (and I'm polite): that's why I'm asking...

ju.ju2004 wrote:

Excuse me, but there are no official specifications for MW. The most I can say is: IT SEEMS REALLY TO BE A BUG. Please read...