r93246 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r93245‎ | r93246 | r93247 >
Date:20:54, 26 July 2011
Author:happy-melon
Status:reverted (Comments)
Tags:
Comment:
(bug 15641) prevent blocked administrators from accessing deleted revisions.
Modified paths:
  • /trunk/phase3/includes/LogEventsList.php (modified) (history)
  • /trunk/phase3/includes/Skin.php (modified) (history)
  • /trunk/phase3/includes/SkinTemplate.php (modified) (history)
  • /trunk/phase3/includes/api/ApiQueryDeletedrevs.php (modified) (history)
  • /trunk/phase3/includes/api/ApiQueryFilearchive.php (modified) (history)
  • /trunk/phase3/includes/diff/DifferenceEngine.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialContributions.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialDeletedContributions.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialRevisiondelete.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialUndelete.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialUpload.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/diff/DifferenceEngine.php
——@@ -147,7 +147,7 @@
148148 */
149149 function deletedLink( $id ) {
150150 global $wgUser;
151 - if ( $wgUser->isAllowed( 'deletedhistory' ) ) {
 151+ if ( $wgUser->isAllowed( 'deletedhistory' ) && !$wgUser->isBlocked() ) {
152152 $dbr = wfGetDB( DB_SLAVE );
153153 $row = $dbr->selectRow('archive', '*',
154154 array( 'ar_rev_id' => $id ),
Index: trunk/phase3/includes/LogEventsList.php
——@@ -543,7 +543,7 @@
544544 }
545545 $del = '';
546546 // Don't show useless link to people who cannot hide revisions
547 - if( $wgUser->isAllowed( 'deletedhistory' ) ) {
 547+ if( $wgUser->isAllowed( 'deletedhistory' ) && !$wgUser->isBlocked() ) {
548548 if( $row->log_deleted || $wgUser->isAllowed( 'deleterevision' ) ) {
549549 $canHide = $wgUser->isAllowed( 'deleterevision' );
550550 // If event was hidden from sysops
——@@ -891,9 +891,9 @@
892892 global $wgUser;
893893 $this->mConds['log_user'] = $userid;
894894 // Paranoia: avoid brute force searches (bug 17342)
895 - if( !$wgUser->isAllowed( 'deletedhistory' ) ) {
 895+ if( !$wgUser->isAllowed( 'deletedhistory' ) || $wgUser->isBlocked() ) {
896896 $this->mConds[] = $this->mDb->bitAnd('log_deleted', LogPage::DELETED_USER) . ' = 0';
897 - } elseif( !$wgUser->isAllowed( 'suppressrevision' ) ) {
 897+ } elseif( !$wgUser->isAllowed( 'suppressrevision' ) || $wgUser->isBlocked() ) {
898898 $this->mConds[] = $this->mDb->bitAnd('log_deleted', LogPage::SUPPRESSED_USER) .
899899 ' != ' . LogPage::SUPPRESSED_USER;
900900 }
——@@ -940,9 +940,9 @@
941941 $this->mConds['log_title'] = $title->getDBkey();
942942 }
943943 // Paranoia: avoid brute force searches (bug 17342)
944 - if( !$wgUser->isAllowed( 'deletedhistory' ) ) {
 944+ if( !$wgUser->isAllowed( 'deletedhistory' ) || $wgUser->isBlocked() ) {
945945 $this->mConds[] = $db->bitAnd('log_deleted', LogPage::DELETED_ACTION) . ' = 0';
946 - } elseif( !$wgUser->isAllowed( 'suppressrevision' ) ) {
 946+ } elseif( !$wgUser->isAllowed( 'suppressrevision' ) || $wgUser->isBlocked() ) {
947947 $this->mConds[] = $db->bitAnd('log_deleted', LogPage::SUPPRESSED_ACTION) .
948948 ' != ' . LogPage::SUPPRESSED_ACTION;
949949 }
Index: trunk/phase3/includes/api/ApiQueryDeletedrevs.php
——@@ -43,7 +43,7 @@
4444 public function execute() {
4545 global $wgUser;
4646 // Before doing anything at all, let's check permissions
47 - if ( !$wgUser->isAllowed( 'deletedhistory' ) ) {
 47+ if ( !$wgUser->isAllowed( 'deletedhistory' ) || $wgUser->isBlocked() ) {
4848 $this->dieUsage( 'You don\'t have permission to view deleted revision information', 'permissiondenied' );
4949 }
5050
Index: trunk/phase3/includes/api/ApiQueryFilearchive.php
——@@ -45,7 +45,7 @@
4646 public function execute() {
4747 global $wgUser;
4848 // Before doing anything at all, let's check permissions
49 - if ( !$wgUser->isAllowed( 'deletedhistory' ) ) {
 49+ if ( !$wgUser->isAllowed( 'deletedhistory' ) || $wgUser->isBlocked() ) {
5050 $this->dieUsage( 'You don\'t have permission to view deleted file information', 'permissiondenied' );
5151 }
5252
Index: trunk/phase3/includes/SkinTemplate.php
——@@ -924,7 +924,7 @@
925925 }
926926 } else {
927927 // article doesn't exist or is deleted
928 - if ( $wgUser->isAllowed( 'deletedhistory' ) ) {
 928+ if ( $wgUser->isAllowed( 'deletedhistory' ) && !$wgUser->isBlocked() ) {
929929 $includeSuppressed = $wgUser->isAllowed( 'suppressrevision' );
930930 $n = $title->isDeleted( $includeSuppressed );
931931 if( $n ) {
Index: trunk/phase3/includes/specials/SpecialUndelete.php
——@@ -643,6 +643,11 @@
644644 $this->displayRestrictionError();
645645 return;
646646 }
 647+
 648+ if ( $this->getUser()->isBlocked() ) {
 649+ throw new UserBlockedError( $this->getUser()->getBlock() );
 650+ }
 651+
647652 $this->outputHeader();
648653
649654 $this->loadRequest();
Index: trunk/phase3/includes/specials/SpecialRevisiondelete.php
——@@ -113,13 +113,15 @@
114114 public function execute( $par ) {
115115 $output = $this->getOutput();
116116 $user = $this->getUser();
 117+
117118 if( !$user->isAllowed( 'deletedhistory' ) ) {
118 - $output->permissionRequired( 'deletedhistory' );
119 - return;
 119+ throw new PermissionsError( 'deletedhistory' );
120120 } elseif( wfReadOnly() ) {
121 - $output->readOnlyPage();
122 - return;
 121+ throw new ReadOnlyError;
 122+ } elseif( $user->isBlocked() ) {
 123+ throw new UserBlockedError( $user->getBlock() );
123124 }
 125+
124126 $this->mIsAllowed = $user->isAllowed('deleterevision'); // for changes
125127 $this->setHeaders();
126128 $this->outputHeader();
Index: trunk/phase3/includes/specials/SpecialContributions.php
——@@ -317,7 +317,7 @@
318318 );
319319
320320 # Add link to deleted user contributions for priviledged users
321 - if( $subject->isAllowed( 'deletedhistory' ) ) {
 321+ if( $subject->isAllowed( 'deletedhistory' ) && !$subject->isBlocked() ) {
322322 $tools[] = $sk->linkKnown(
323323 SpecialPage::getTitleFor( 'DeletedContributions', $username ),
324324 wfMsgHtml( 'sp-contributions-deleted' )
——@@ -486,7 +486,7 @@
487487
488488 $conds = array_merge( $userCond, $this->getNamespaceCond() );
489489 // Paranoia: avoid brute force searches (bug 17342)
490 - if( !$wgUser->isAllowed( 'deletedhistory' ) ) {
 490+ if( !$wgUser->isAllowed( 'deletedhistory' ) || $wgUser->isBlocked() ) {
491491 $conds[] = $this->mDb->bitAnd('rev_deleted',Revision::DELETED_USER) . ' = 0';
492492 } elseif( !$wgUser->isAllowed( 'suppressrevision' ) ) {
493493 $conds[] = $this->mDb->bitAnd('rev_deleted',Revision::SUPPRESSED_USER) .
Index: trunk/phase3/includes/specials/SpecialUpload.php
——@@ -313,21 +313,19 @@
314314
315315 $title = Title::makeTitleSafe( NS_FILE, $this->mDesiredDestName );
316316 // Show a subtitle link to deleted revisions (to sysops et al only)
317 - if( $title instanceof Title ) {
318 - if ( $wgUser->isAllowed( 'deletedhistory' ) ) {
319 - $canViewSuppress = $wgUser->isAllowed( 'suppressrevision' );
320 - $count = $title->isDeleted( $canViewSuppress );
321 - if ( $count > 0 ) {
322 - $link = wfMsgExt(
323 - $wgUser->isAllowed( 'delete' ) ? 'thisisdeleted' : 'viewdeleted',
324 - array( 'parse', 'replaceafter' ),
325 - $this->getSkin()->linkKnown(
326 - SpecialPage::getTitleFor( 'Undelete', $title->getPrefixedText() ),
327 - wfMsgExt( 'restorelink', array( 'parsemag', 'escape' ), $count )
328 - )
329 - );
330 - $wgOut->addHTML( "<div id=\"contentSub2\">{$link}</div>" );
331 - }
 317+ if( $title instanceof Title && $wgUser->isAllowed( 'deletedhistory' ) && !$wgUser->isBlocked() ) {
 318+ $canViewSuppress = $wgUser->isAllowed( 'suppressrevision' );
 319+ $count = $title->isDeleted( $canViewSuppress );
 320+ if ( $count > 0 ) {
 321+ $link = wfMsgExt(
 322+ $wgUser->isAllowed( 'delete' ) ? 'thisisdeleted' : 'viewdeleted',
 323+ array( 'parse', 'replaceafter' ),
 324+ $this->getSkin()->linkKnown(
 325+ SpecialPage::getTitleFor( 'Undelete', $title->getPrefixedText() ),
 326+ wfMsgExt( 'restorelink', array( 'parsemag', 'escape' ), $count )
 327+ )
 328+ );
 329+ $wgOut->addHTML( "<div id=\"contentSub2\">{$link}</div>" );
332330 }
333331 }
334332 }
Index: trunk/phase3/includes/specials/SpecialDeletedContributions.php
——@@ -281,6 +281,10 @@
282282 return;
283283 }
284284
 285+ if( $wgUser->isBlocked() ){
 286+ throw new UserBlockedError( $wgUser->getBlock() );
 287+ }
 288+
285289 global $wgOut, $wgRequest;
286290
287291 $wgOut->setPageTitle( wfMsgExt( 'deletedcontributions-title', array( 'parsemag' ) ) );
Index: trunk/phase3/includes/Skin.php
——@@ -631,7 +631,7 @@
632632 function getUndeleteLink() {
633633 $action = $this->getRequest()->getVal( 'action', 'view' );
634634
635 - if ( $this->getUser()->isAllowed( 'deletedhistory' ) &&
 635+ if ( $this->getUser()->isAllowed( 'deletedhistory' ) && !$this->getUser()->isBlocked() &&
636636 ( $this->getTitle()->getArticleId() == 0 || $action == 'history' ) ) {
637637
638638 $includeSuppressed = $this->getUser()->isAllowed( 'suppressrevision' );

Follow-up revisions

RevisionCommit summaryAuthorDate
r99211Revert r93246: besides the problems pointed out at CR, it also causes bug 314...maxsem13:58, 7 October 2011
r99323* Only spread blocks on page edit/move attempts via spreadAnyEditBlock(). We ...aaron20:22, 8 October 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r85005(bug 15641) tweak Title::checkUserBlock() so that Title::getUserPermissionsEr...happy-melon12:53, 30 March 2011

Comments

#Comment by Siebrand (talk | contribs)   22:18, 26 July 2011

This fix looks pretty crazy. Adding tens of similar checks to solve a relatively small issue - at least from the description. Should access checks maybe be more centralised somehow?

#Comment by Aaron Schulz (talk | contribs)   22:19, 26 July 2011

Recommend revert.

#Comment by Siebrand (talk | contribs)   22:21, 26 July 2011

Reason?

#Comment by Nikerabbit (talk | contribs)   08:40, 21 September 2011

The access checking should be done only once. Otherwise it is bound to get out of sync somewhere.

#Comment by Happy-melon (talk | contribs)   21:28, 24 September 2011

So instead you want to do what? Hack User::isAllowed() to specialcase deletedhistory and thereby pollute one of the few functions in User.php that actually does what it says on the tin?

The best way to implement this and various other block-related permissions issues would probably be to make being blocked add users to an implicit group, then we can use the existing $wgRevokePermissions infrastructure to do this nicely (and within the context of what User->isAllowed() is correctly testing). But that's a big job that shouldn't prejudice this specific fix.

#Comment by Bryan (talk | contribs)   19:46, 2 October 2011

I agree. This "fix" is really a hack, but that is inherent to our blocking system. I would leave it for now like this.

Status & tagging log