r52201 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r52200‎ | r52201 | r52202 >
Date:14:37, 20 June 2009
Author:skizzerz
Status:reverted (Comments)
Tags:
Comment:
* Remove "shortcut" in Title::userCanRead, it prevents $wgRevokePermissions and extensions not using the userCan hook from restricting read access on the wiki if anon reading is allowed.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/Title.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Title.php
——@@ -1490,10 +1490,6 @@
14911491 return $result;
14921492 }
14931493
1494 - # Shortcut for public wikis, allows skipping quite a bit of code
1495 - if ( !empty( $wgGroupPermissions['*']['read'] ) )
1496 - return true;
1497 -
14981494 if( $wgUser->isAllowed( 'read' ) ) {
14991495 return true;
15001496 } else {
Index: trunk/phase3/RELEASE-NOTES
——@@ -195,6 +195,8 @@
196196 * (bug 17014) Blocked users can no longer use Special:UserRights unless they
197197 can add/remove *all* groups (have 'userrights' permission).
198198 * (bug 19294) Always show Sp-contributions-footer(-anon)
 199+* Attempts to restrict reading of pages while anonymous viewing is allowed
 200+ via extensions not using the userCan hook and via $wgRevokePermissions now work.
199201
200202 == API changes in 1.16 ==
201203

Follow-up revisions

RevisionCommit summaryAuthorDate
r52301* Revert r52201, but modify the shortcut code so that it properly detects "pu...skizzerz02:03, 23 June 2009

Comments

#Comment by Tbleher (talk | contribs)   19:20, 20 June 2009

I think domas won't like that. See r29725 where it was added, r32324 for a partial revert and r32326 where domas added it again.

#Comment by Skizzerz (talk | contribs)   19:25, 20 June 2009

It breaks a core feature, there is a real reason to remove it now.

Example: $wgGroupPermissions['*']['read'] = true; $wgRevokePermissions['user']['read'] = true;

This should prevent logged in users from viewing the site, but allow anon users. However, because of the shortcut, this allows logged in users anyway. This shortcut also prevents extensions using the UserGetRights hook from revoking reading permissions as well if anons are allowed to read.

#Comment by Tbleher (talk | contribs)   20:20, 20 June 2009

Your example makes no sense to me: Why would you want to restrict users from viewing pages, if they could just log out to view them?

#Comment by Werdna (talk | contribs)   18:40, 21 June 2009

You should add a configuration variable for activating and deactivating this, which could be deactivated by extensions which are broken by it.

We need this hack on Wikimedia to keep from doing a heap of unnecessary processing.

#Comment by Skizzerz (talk | contribs)   22:33, 21 June 2009

I'm not worried too much by extensions, since I only know of about three that would be affected by this, it's the fact that it breaks $wgRevokePermissions that worries me. Perhaps adding it back in with another check would be good? I'm not a fan of having one config variable dependent on another one.

Perhaps using a static in the function to determine whether the 'read' key is set in $wgRevokePermissions will work. If it isn't, then use the shortcut. If it is, then don't use the shortcut.

#Comment by Skizzerz (talk | contribs)   23:14, 21 June 2009

Would this work as expected? The ... is the userCan hook, I just didn't feel like including it :) 

static $useShortcut = null;

# Initialize the $useShortcut boolean, to determine if we can skip quite a bit of code below
if( is_null( $useShortcut ) ) {
    global $wgRevokePermissions;
    $useShortcut = true;
    if( empty( $wgGroupPermissions['*']['read'] ) ) {
        # Not a public wiki, so no shortcut
        $useShortcut = false;
    } elseif( !empty( $wgRevokePermissions ) ) {
        foreach( array_keys( $wgRevokePermissions ) ) {
            if( !empty( $wgRevokePermissions[$group]['read'] ) ) {
                # We might be removing the read right from the user, so no shortcut
                $useShortcut = false;
                break;
            }
        }
    }
}

...

# Shortcut for public wikis, allows skipping quite a bit of code
if ( $useShortcut )
  return true;
#Comment by Skizzerz (talk | contribs)   23:19, 21 June 2009

I meant foreach( array_keys( $wgRevokePermissions ) as $group ) { of course >_>

#Comment by Midom (talk | contribs)   19:10, 21 June 2009

hehe, hehehe. extensions can check if '*' has read permission :-)

#Comment by Skizzerz (talk | contribs)   02:06, 23 June 2009

reverted and fixed "public wiki" detection in r52301

Status & tagging log