Last modified: 2011-01-25 00:18:02 UTC
Created attachment 5719 [details] Patch to continue userCan chain through userCanRead function When an extension uses the userCan hook, the code in Title.php treats the result of the hook as the end of the line. This means that if any userCan extension is in use, $wgGroupPermissions is ignored for read permissions. To replicate this issue, put the following in LocalSettings.php: $wgGroupPermissions['*']['read'] = false; $wgGroupPermissions['user']['read'] = true; require_once("extensions/bugreport.php"); The bugreport.php file should simply be: <?php $wgHooks['userCan'][] = 'bugReportUserCan'; function bugReportUserCan( $title, $wgUser, $action, &$result ){ $result = true; return true; } ?> Note that anonymous users now have full access to the wiki. I have attached a patch for Title.php that I've tested in a few configurations but I'm not completely confident in a change to such a critical component. This change continues the userCan hook through the userCanRead() function so that if the hook returns true (to continue), userCanRead() still has the opportunity to override it based on the default security.
Have you tested that test case? In theory, you need to return false, not true, to cause the override, and if false is returned, then this is expected behaviour.
I did -- the code causing the original behavior centers on this: wfRunHooks( 'userCan', array( &$this, &$wgUser, 'read', &$result ) ); if ( $result !== null ) { return $result; } The result is returned as long as there was a result. The state of wfRunHooks is not consulted, so returning true or false doesn't make a difference.
I'm not sure what the documented behaviour is, but it looks like if you don't want to force a specific result, then you shouldn't set $result.
If I understand correctly, the result of wfRunHooks() should be the instruction of whether to continue processing security rules. An extension author can set the $result to true and return false in order to allow the user to read and force their extension to be the last word on the matter, according to the documentation at http://www.mediawiki.org/wiki/Manual:Hooks/userCan. In this case, there is no way for the extension author to declare that it this extension has no problem with the user having read access but also allow the other security code to run. (Even if $result isn't set, an earlier or later userCan hook might change it.) I believe this turns the original MW access code using $wgGroupPermissions off, with the only way to re-enable it being to re-implement it in the extension code.
I don't agree that "There is no way for the extension author to declare that it has no problem with the user having read access, but to also allow the other security code to run". If no extension sets $result, then this is what happens. If one or more extensions sets $result to true, then they have declared that they want the user to be able to read, no matter what. If one or more extensions set $result to false, then they have declined access, regardless of the other code.
Fair enough -- I'll use this as a workaround in code. I think it disagrees with the documentation which states, "User should be allowed to proceed. Later functions can override" but I think you're right that this may be how it's intended to work at this time.
Will leave the bug open for a determination of whether this is something we *want*, or something that needs to be fixed.
If you've set the value to true, there's no way anything in $wgGroupPermissions could override you. Truth table: true || true -> true true || false -> true Skipping evaluation of the righthand term when it can never affect the result is perfectly correct boolean shortcut processing.
