r64228 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r64227‎ | r64228 | r64229 >
Date:22:02, 26 March 2010
Author:happy-melon
Status:ok (Comments)
Tags:
Comment:
(bug 15810) stop blocked admins from unblocking themselves or others.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/specials/SpecialBlockip.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialIpblocklist.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesQqq.php (modified) (history)
  • /trunk/phase3/maintenance/language/messages.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/language/messages.inc
@@ -1078,6 +1078,7 @@
10791079 'right-override-export-depth',
10801080 'right-versiondetail',
10811081 'right-sendemail',
 1082+ 'right-unblockself',
10821083 ),
10831084 'rightslog' => array(
10841085 'rightslog',
@@ -2013,7 +2014,9 @@
20142015 'sorbsreason',
20152016 'sorbs_create_account_reason',
20162017 'cant-block-while-blocked',
2017 - 'cant-see-hidden-user'
 2018+ 'cant-see-hidden-user',
 2019+ 'ipbblocked',
 2020+ 'ipbnounblockself',
20182021 ),
20192022 'developertools' => array(
20202023 'lockdb',
Index: trunk/phase3/includes/specials/SpecialIpblocklist.php
@@ -19,7 +19,7 @@
2020
2121 $ipu = new IPUnblockForm( $ip, $id, $reason );
2222
23 - if( $action == 'unblock' ) {
 23+ if( $action == 'unblock' || $action == 'submit' && $wgRequest->wasPosted() ) {
2424 # Check permissions
2525 if( !$wgUser->isAllowed( 'block' ) ) {
2626 $wgOut->permissionRequired( 'block' );
@@ -30,22 +30,40 @@
3131 $wgOut->readOnlyPage();
3232 return;
3333 }
34 - # Show unblock form
35 - $ipu->showForm( '' );
36 - } elseif( $action == 'submit' && $wgRequest->wasPosted()
37 - && $wgUser->matchEditToken( $wgRequest->getVal( 'wpEditToken' ) ) ) {
38 - # Check permissions
39 - if( !$wgUser->isAllowed( 'block' ) ) {
40 - $wgOut->permissionRequired( 'block' );
41 - return;
 34+
 35+ # bug 15810: blocked admins should have limited access here
 36+ if( $wgUser->isBlocked() ){
 37+ if( $id ){
 38+ # This doesn't pick up on autoblocks, but admins
 39+ # should have the ipblock-exempt permission anyway
 40+ $block = Block::newFromID( $id );
 41+ $user = User::newFromName( $block->mAddress );
 42+ } else {
 43+ $user = User::newFromName( $ip );
 44+ }
 45+ if( $user instanceof User
 46+ && $user->getId() == $wgUser->getId() )
 47+ {
 48+ # User is trying to unblock themselves
 49+ if( !$wgUser->isAllowed( 'unblockself' ) ){
 50+ throw new ErrorPageError( 'badaccess', 'ipbnounblockself' );
 51+ }
 52+ } else {
 53+ # User is trying to block/unblock someone else
 54+ throw new ErrorPageError( 'badaccess', 'ipbblocked' );
 55+ }
4256 }
43 - # Check for database lock
44 - if( wfReadOnly() ) {
45 - $wgOut->readOnlyPage();
46 - return;
 57+ if( $action == 'unblock' ){
 58+ # Show unblock form
 59+ $ipu->showForm( '' );
 60+ } elseif( $action == 'submit'
 61+ && $wgRequest->wasPosted()
 62+ && $wgUser->matchEditToken( $wgRequest->getVal( 'wpEditToken' ) ) )
 63+ {
 64+ # Remove blocks and redirect user to success page
 65+ $ipu->doSubmit();
4766 }
48 - # Remove blocks and redirect user to success page
49 - $ipu->doSubmit();
 67+
5068 } elseif( $action == 'success' ) {
5169 # Inform the user of a successful unblock
5270 # (No need to check permissions or locks here,
Index: trunk/phase3/includes/specials/SpecialBlockip.php
@@ -24,6 +24,22 @@
2525 }
2626
2727 $ipb = new IPBlockForm( $par );
 28+
 29+ # bug 15810: blocked admins should have limited access here
 30+ if( $wgUser->isBlocked() ){
 31+ $user = User::newFromName( $ipb->BlockAddress );
 32+ if( $user instanceof User
 33+ && $user->getId() == $wgUser->getId() )
 34+ {
 35+ # User is trying to unblock themselves
 36+ if( !$wgUser->isAllowed( 'unblockself' ) ){
 37+ throw new ErrorPageError( 'badaccess', 'ipbnounblockself' );
 38+ }
 39+ } else {
 40+ # User is trying to block/unblock someone else
 41+ throw new ErrorPageError( 'badaccess', 'ipbblocked' );
 42+ }
 43+ }
2844
2945 $action = $wgRequest->getVal( 'action' );
3046 if( 'success' == $action ) {
Index: trunk/phase3/languages/messages/MessagesQqq.php
@@ -2495,6 +2495,8 @@
24962496 'blockme' => 'The page title of [[Special:Blockme]], a feature which is disabled by default.',
24972497 'sorbs' => '{{optional}}',
24982498 'cant-see-hidden-user' => 'Used as (red) error message on Special:Block when you try to change (as sysop w/o the hideuser right) the block of a hidden user.',
 2499+'ipbblocked' => 'Error message shown when a user tries to alter block settings when they are themselves blocked.',
 2500+'ipbnounblockself' => 'Error message shown when a user without the <tt>unblockself</tt> right tries to unblock themselves.',
24992501
25002502 # Developer tools
25012503 'lockdb' => 'The title of the special page [[Special:LockDB]].
Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -1904,6 +1904,7 @@
19051905 'right-override-export-depth' => 'Export pages including linked pages up to a depth of 5',
19061906 'right-versiondetail' => 'Show the extended software version information',
19071907 'right-sendemail' => 'Send e-mail to other users',
 1908+'right-unblockself' => 'Unblock themselves',
19081909
19091910 # User rights log
19101911 'rightslog' => 'User rights log',
@@ -2999,6 +3000,8 @@
30003001 'cant-block-while-blocked' => 'You cannot block other users while you are blocked.',
30013002 'cant-see-hidden-user' => "The user you are trying to block has already been blocked and hidden.
30023003 Since you do not have the hideuser right, you cannot see or edit the user's block.",
 3004+'ipbblocked' => 'You cannot block or unblock other users, because you are yourself blocked',
 3005+'ipbnounblockself' => 'You are not allowed to unblock yourself',
30033006
30043007 # Developer tools
30053008 'lockdb' => 'Lock database',
Index: trunk/phase3/RELEASE-NOTES
@@ -58,6 +58,8 @@
5959 * (bug 22903) Revdelete log entries now show in the user preferred language.
6060 * (bug 22905) Correctly handle <abbr> followed by ISBN
6161 * (bug 22940) Namespace aliases pointing to main namespace don't work
 62+* (bug 15810) blocked admins can no longer block/unblock other users, nor
 63+ themselves unless they are given the 'unblockself' permission.
6264
6365 == API changes in 1.17 ==
6466 * (bug 22738) Allow filtering by action type on query=logevent

Follow-up revisions

RevisionCommit summaryAuthorDate
r64230Followup to r64228 - apply restrictions in API.happy-melon23:02, 26 March 2010
r64231Follow-up r64228, per requests at bug 15810: grant admins 'unblockself' by de...happy-melon23:11, 26 March 2010
r64256Per r64228 CR: make the check a static method in IPBlockForm to reduce duplic...happy-melon15:05, 27 March 2010
r64494Better RELEASE-NOTES for r64228...simetrical18:22, 1 April 2010
r64496Better better RELEASE-NOTES for r64228...simetrical19:43, 1 April 2010
r89293'unblockself' right was never added to User::$mCoreRights...reedy21:44, 1 June 2011

Comments

#Comment by Bryan (talk | contribs)   22:31, 26 March 2010

Still circumventable via API. Permission checks should be done in IPBlockForm::doBlock itself.

#Comment by Happy-melon (talk | contribs)   23:03, 26 March 2010

Fixed in r64230.

#Comment by Bryan (talk | contribs)   10:22, 27 March 2010

Code duplication is considered bad in programming because it it doubles (or in this case quadruples) maintenance effort. There should be a static IpBlockForm::checkUnblockSelf that is called in the 4 places that you use it.

#Comment by Happy-melon (talk | contribs)   15:07, 27 March 2010

Now that makes sense. Done in r64256.

#Comment by X! (talk | contribs)   20:13, 4 May 2010

What if an admin accidentally blocks themselves? It's happened quite a bit.

#Comment by Happy-melon (talk | contribs)   20:22, 4 May 2010

They wait for someone to unblock them, just like everyone else who gets blocked accidentally. The only problem is if there're a limited number of admins on a site; especially if one goes rogue and blocks the others. Which is why it should be a configurable setting which can be on for small wikis and turned off for larger ones when that ceases to be a concern.

#Comment by FloNight (talk | contribs)   20:36, 4 May 2010

As long as it can be either turned off or on to individualize it for the needs of a wiki, I think it is a good change indeed!

#Comment by Werdna (talk | contribs)   03:52, 9 December 2010

I think this proposed feature needs more discussion, preferably in a public forum rather than a developer back channel. A change like this definitely needs community consultation.

#Comment by Bryan (talk | contribs)   07:49, 9 December 2010

Assigning the unblockself right to sysops by default would be an acceptable solution pending community consultation I think.

#Comment by Reedy (talk | contribs)   21:50, 1 June 2011

Never added to core user rights list, added in r89293

Status & tagging log