From f2d0453f9dce404e3d9a6635a9b7aafad8592252 Mon Sep 17 00:00:00 2001 From: Alexandre Emsenhuber Date: Wed, 2 Nov 2011 15:30:55 +0000 Subject: [PATCH] * Made PermissionsError exception accept an optional second parameter for the description of the errors (as returned by Title::getUserPermissionsErrors()) * PermissionsError now calls OutputPage::showPermissionsErrorPage() to display the error (this is needed to make the item above work correctly) * Removed the override of the HTML title in OutputPage::showPermissionsErrorPage() so that it shows "Permission errors - Sitename" instead of simply "Permission errors" for consistency with the other things * Pass the error array returned by Title::getUserPermissionsErrors() to PermissionsError where available * Converted direct calls to OutputPage::showPermissionsErrorPage() to throw an PermissionsError error instead * Added 'action-rollback' message that will be displayed when accessing action=rollback without sufficient rights * Changed getRestriction() in subclasses of Action to return null when they previously returned 'read' so that user rights can be check with Title::getUserPermissionsErrors() * Reordered checks to do first user rights, then block (if needed) and finally read only (also if needed) so that users don't think the error is temporary when they both don't have right and the database is locked --- includes/Action.php | 19 +++++++---- includes/Article.php | 31 ++++++++---------- includes/Exception.php | 41 ++++++++++++------------ includes/FileDeleteForm.php | 10 +++--- includes/OutputPage.php | 2 +- includes/actions/HistoryAction.php | 2 +- includes/actions/InfoAction.php | 2 +- includes/actions/MarkpatrolledAction.php | 7 ++-- includes/actions/RawAction.php | 2 +- includes/actions/RevertAction.php | 2 +- includes/actions/RollbackAction.php | 4 +-- includes/actions/WatchAction.php | 2 +- includes/specials/SpecialImport.php | 13 ++++---- includes/specials/SpecialMovepage.php | 3 +- includes/specials/SpecialUserlogin.php | 10 +++--- includes/specials/SpecialUserrights.php | 13 +++----- languages/messages/MessagesEn.php | 1 + languages/messages/MessagesQqq.php | 1 + maintenance/language/messages.inc | 1 + 19 files changed, 82 insertions(+), 84 deletions(-) diff --git a/includes/Action.php b/includes/Action.php index 9f362d121f..22879410a5 100644 --- a/includes/Action.php +++ b/includes/Action.php @@ -200,18 +200,25 @@ abstract class Action { * @throws ErrorPageError */ protected function checkCanExecute( User $user ) { - if ( $this->requiresWrite() && wfReadOnly() ) { - throw new ReadOnlyError(); - } - - if ( $this->getRestriction() !== null && !$user->isAllowed( $this->getRestriction() ) ) { - throw new PermissionsError( $this->getRestriction() ); + $right = $this->getRestriction(); + if ( $right !== null ) { + $errors = $this->getTitle()->getUserPermissionsErrors( $right, $user ); + if ( count( $errors ) ) { + throw new PermissionsError( $right, $errors ); + } } if ( $this->requiresUnblock() && $user->isBlocked() ) { $block = $user->mBlock; throw new UserBlockedError( $block ); } + + // This should be checked at the end so that the user won't think the + // error is only temporary when he also don't have the rights to execute + // this action + if ( $this->requiresWrite() && wfReadOnly() ) { + throw new ReadOnlyError(); + } } /** diff --git a/includes/Article.php b/includes/Article.php index 19e74a4b43..d7b0eee3bf 100644 --- a/includes/Article.php +++ b/includes/Article.php @@ -1302,6 +1302,19 @@ class Article extends Page { public function delete() { global $wgOut, $wgRequest; + # This code desperately needs to be totally rewritten + + # Check permissions + $permission_errors = $this->mTitle->getUserPermissionsErrors( 'delete', $this->getContext()->getUser() ); + if ( count( $permission_errors ) ) { + throw new PermissionsError( 'delete', $permission_errors ); + } + + # Read-only check... + if ( wfReadOnly() ) { + throw new ReadOnlyError; + } + $confirm = $wgRequest->wasPosted() && $this->getContext()->getUser()->matchEditToken( $wgRequest->getVal( 'wpEditToken' ) ); @@ -1320,24 +1333,6 @@ class Article extends Page { # Flag to hide all contents of the archived revisions $suppress = $wgRequest->getVal( 'wpSuppress' ) && $this->getContext()->getUser()->isAllowed( 'suppressrevision' ); - # This code desperately needs to be totally rewritten - - # Read-only check... - if ( wfReadOnly() ) { - $wgOut->readOnlyPage(); - - return; - } - - # Check permissions - $permission_errors = $this->getTitle()->getUserPermissionsErrors( 'delete', $this->getContext()->getUser() ); - - if ( count( $permission_errors ) > 0 ) { - $wgOut->showPermissionsErrorPage( $permission_errors ); - - return; - } - $wgOut->setPageTitle( wfMessage( 'delete-confirm', $this->getTitle()->getPrefixedText() ) ); # Better double-check that it hasn't been deleted yet! diff --git a/includes/Exception.php b/includes/Exception.php index c5bb056a60..8c4e0a754f 100644 --- a/includes/Exception.php +++ b/includes/Exception.php @@ -276,33 +276,34 @@ class ErrorPageError extends MWException { * @ingroup Exception */ class PermissionsError extends ErrorPageError { - public $permission; + public $permission, $errors; - function __construct( $permission ) { + function __construct( $permission, $errors = array() ) { global $wgLang; $this->permission = $permission; - $groups = array_map( - array( 'User', 'makeGroupLinkWiki' ), - User::getGroupsWithPermission( $this->permission ) - ); - - if( $groups ) { - parent::__construct( - 'badaccess', - 'badaccess-groups', - array( - $wgLang->commaList( $groups ), - count( $groups ) - ) - ); - } else { - parent::__construct( - 'badaccess', - 'badaccess-group0' + if ( !count( $errors ) ) { + $groups = array_map( + array( 'User', 'makeGroupLinkWiki' ), + User::getGroupsWithPermission( $this->permission ) ); + + if ( $groups ) { + $errors[] = array( 'badaccess-groups', $wgLang->commaList( $groups ), count( $groups ) ); + } else { + $errors[] = array( 'badaccess-group0' ); + } } + + $this->errors = $errors; + } + + function report() { + global $wgOut; + + $wgOut->showPermissionsErrorPage( $this->errors, $this->permission ); + $wgOut->output(); } } diff --git a/includes/FileDeleteForm.php b/includes/FileDeleteForm.php index 0c5e232246..2e806638d3 100644 --- a/includes/FileDeleteForm.php +++ b/includes/FileDeleteForm.php @@ -41,18 +41,18 @@ class FileDeleteForm { */ public function execute() { global $wgOut, $wgRequest, $wgUser; - $this->setHeaders(); - $permission_errors = $this->title->getUserPermissionsErrors('delete', $wgUser); - if ( count( $permission_errors ) > 0 ) { - $wgOut->showPermissionsErrorPage( $permission_errors ); - return; + $permissionErrors = $this->title->getUserPermissionsErrors( 'delete', $wgUser ); + if ( count( $permissionErrors ) ) { + throw new PermissionsError( 'delete', $permissionErrors ); } if ( wfReadOnly() ) { throw new ReadOnlyError; } + $this->setHeaders(); + $this->oldimage = $wgRequest->getText( 'oldimage', false ); $token = $wgRequest->getText( 'wpEditToken' ); # Flag to hide all contents of the archived revisions diff --git a/includes/OutputPage.php b/includes/OutputPage.php index 83faccc806..af2f2649be 100644 --- a/includes/OutputPage.php +++ b/includes/OutputPage.php @@ -1979,7 +1979,7 @@ class OutputPage extends ContextSource { * @param $action String: action that was denied or null if unknown */ public function showPermissionsErrorPage( $errors, $action = null ) { - $this->prepareErrorPage( $this->msg( 'permissionserrors' ), $this->msg( 'permissionserrors' ) ); + $this->prepareErrorPage( $this->msg( 'permissionserrors' ) ); $this->addWikiText( $this->formatPermissionsErrorMessage( $errors, $action ) ); } diff --git a/includes/actions/HistoryAction.php b/includes/actions/HistoryAction.php index 779b4db722..9efe953882 100644 --- a/includes/actions/HistoryAction.php +++ b/includes/actions/HistoryAction.php @@ -24,7 +24,7 @@ class HistoryAction extends FormlessAction { } public function getRestriction() { - return 'read'; + return null; } public function requiresWrite() { diff --git a/includes/actions/InfoAction.php b/includes/actions/InfoAction.php index b0b5f2594a..4854d305cd 100644 --- a/includes/actions/InfoAction.php +++ b/includes/actions/InfoAction.php @@ -30,7 +30,7 @@ class InfoAction extends FormlessAction { } public function getRestriction() { - return 'read'; + return null; } protected function getDescription() { diff --git a/includes/actions/MarkpatrolledAction.php b/includes/actions/MarkpatrolledAction.php index f9b760ac68..e52d0c45ea 100644 --- a/includes/actions/MarkpatrolledAction.php +++ b/includes/actions/MarkpatrolledAction.php @@ -29,7 +29,7 @@ class MarkpatrolledAction extends FormlessAction { } public function getRestriction() { - return 'read'; + return null; } protected function getDescription() { @@ -73,9 +73,8 @@ class MarkpatrolledAction extends FormlessAction { return; } - if ( !empty( $errors ) ) { - $this->getOutput()->showPermissionsErrorPage( $errors ); - return; + if ( count( $errors ) ) { + throw new PermissionsErrorPage( 'patrol', $errors ); } # Inform the user diff --git a/includes/actions/RawAction.php b/includes/actions/RawAction.php index 89952f4a2a..67d31172f7 100644 --- a/includes/actions/RawAction.php +++ b/includes/actions/RawAction.php @@ -25,7 +25,7 @@ class RawAction extends FormlessAction { } public function getRestriction() { - return 'read'; + return null; } public function requiresWrite() { diff --git a/includes/actions/RevertAction.php b/includes/actions/RevertAction.php index bcb8cd8ba9..61403aa947 100644 --- a/includes/actions/RevertAction.php +++ b/includes/actions/RevertAction.php @@ -35,7 +35,7 @@ class RevertAction extends Action { } public function getRestriction() { - return 'read'; + return null; } public function show() { diff --git a/includes/actions/RollbackAction.php b/includes/actions/RollbackAction.php index e7a6521e36..1cc64b368b 100644 --- a/includes/actions/RollbackAction.php +++ b/includes/actions/RollbackAction.php @@ -83,9 +83,7 @@ class RollbackAction extends FormlessAction { $out [] = $error; } } - $this->getOutput()->showPermissionsErrorPage( $out ); - - return; + throw new PermissionsError( 'rollback', $out ); } if ( $result == array( array( 'readonlytext' ) ) ) { diff --git a/includes/actions/WatchAction.php b/includes/actions/WatchAction.php index 52e6675430..4d539f4785 100644 --- a/includes/actions/WatchAction.php +++ b/includes/actions/WatchAction.php @@ -27,7 +27,7 @@ class WatchAction extends FormAction { } public function getRestriction() { - return 'read'; + return null; } public function requiresUnblock() { diff --git a/includes/specials/SpecialImport.php b/includes/specials/SpecialImport.php index d346c0ed92..575c94e719 100644 --- a/includes/specials/SpecialImport.php +++ b/includes/specials/SpecialImport.php @@ -60,10 +60,6 @@ class SpecialImport extends SpecialPage { throw new PermissionsError( 'import' ); } - if ( wfReadOnly() ) { - throw new ReadOnlyError; - } - # @todo Allow Title::getUserPermissionsErrors() to take an array # @todo FIXME: Title::checkSpecialsAndNSPermissions() has a very wierd expectation of what # getUserPermissionsErrors() might actually be used for, hence the 'ns-specialprotected' @@ -78,9 +74,12 @@ class SpecialImport extends SpecialPage { ) ); - if( $errors ){ - $this->getOutput()->showPermissionsErrorPage( $errors ); - return; + if ( $errors ) { + throw new PermissionsError( 'import', $errors ); + } + + if ( wfReadOnly() ) { + throw new ReadOnlyError; } $request = $this->getRequest(); diff --git a/includes/specials/SpecialMovepage.php b/includes/specials/SpecialMovepage.php index 4f66414420..38b25afd2e 100644 --- a/includes/specials/SpecialMovepage.php +++ b/includes/specials/SpecialMovepage.php @@ -74,8 +74,7 @@ class MovePageForm extends UnlistedSpecialPage { if( !empty( $permErrors ) ) { // Auto-block user's IP if the account was "hard" blocked $user->spreadAnyEditBlock(); - $this->getOutput()->showPermissionsErrorPage( $permErrors ); - return; + throw new PermissionsError( 'move', $permErrors ); } $def = !$request->wasPosted(); diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php index 8c779fae1b..8892d9778a 100644 --- a/includes/specials/SpecialUserlogin.php +++ b/includes/specials/SpecialUserlogin.php @@ -953,14 +953,14 @@ class LoginForm extends SpecialPage { // Block signup here if in readonly. Keeps user from // going through the process (filling out data, etc) // and being informed later. - if ( wfReadOnly() ) { - throw new ReadOnlyError; + $permErrors = $titleObj->getUserPermissionsErrors( 'createaccount', $user, true ); + if ( count( $permErrors ) ) { + throw new PermissionsError( 'createaccount', $permErrors ); } elseif ( $user->isBlockedFromCreateAccount() ) { $this->userBlockedMessage( $user->isBlockedFromCreateAccount() ); return; - } elseif ( count( $permErrors = $titleObj->getUserPermissionsErrors( 'createaccount', $user, true ) )>0 ) { - $this->getOutput()->showPermissionsErrorPage( $permErrors, 'createaccount' ); - return; + } elseif ( wfReadOnly() ) { + throw new ReadOnlyError; } } diff --git a/includes/specials/SpecialUserrights.php b/includes/specials/SpecialUserrights.php index aeee554610..048ed6c8d3 100644 --- a/includes/specials/SpecialUserrights.php +++ b/includes/specials/SpecialUserrights.php @@ -99,24 +99,21 @@ class UserrightsPage extends SpecialPage { $this->isself = true; } - $out = $this->getOutput(); - if( !$this->userCanChangeRights( $user, true ) ) { // @todo FIXME: There may be intermediate groups we can mention. - $out->showPermissionsErrorPage( array( array( - $user->isAnon() - ? 'userrights-nologin' - : 'userrights-notallowed' ) ) ); - return; + $msg = $user->isAnon() ? 'userrights-nologin' : 'userrights-notallowed'; + throw new PermissionsError( null, array( array( $msg ) ) ); } if ( wfReadOnly() ) { throw new ReadOnlyError; } + $this->setHeaders(); $this->outputHeader(); + + $out = $this->getOutput(); $out->addModuleStyles( 'mediawiki.special' ); - $this->setHeaders(); // show the general form if ( count( $available['add'] ) || count( $available['remove'] ) ) { diff --git a/languages/messages/MessagesEn.php b/languages/messages/MessagesEn.php index a3b47ee877..b150420f7f 100644 --- a/languages/messages/MessagesEn.php +++ b/languages/messages/MessagesEn.php @@ -2016,6 +2016,7 @@ Your e-mail address is not revealed when other users contact you.', 'action-suppressionlog' => 'view this private log', 'action-block' => 'block this user from editing', 'action-protect' => 'change protection levels for this page', +'action-rollback' => 'quickly rollback the edits of the last user who edited a particular page', 'action-import' => 'import this page from another wiki', 'action-importupload' => 'import this page from a file upload', 'action-patrol' => "mark others' edit as patrolled", diff --git a/languages/messages/MessagesQqq.php b/languages/messages/MessagesQqq.php index c5a6ad86b7..40752be8f1 100644 --- a/languages/messages/MessagesQqq.php +++ b/languages/messages/MessagesQqq.php @@ -1684,6 +1684,7 @@ API is an abbreviation for [http://en.wikipedia.org/wiki/API application program 'action-suppressionlog' => '{{Doc-action|suppressionlog}}', 'action-block' => '{{Doc-action|block}}', 'action-protect' => '{{Doc-action|protect}}', +'action-rollback' => '{{Doc-action|rollback}}', 'action-import' => '{{Doc-action|import}}', 'action-importupload' => '{{Doc-action|importupload}}', 'action-patrol' => '{{Doc-action|patrol}}', diff --git a/maintenance/language/messages.inc b/maintenance/language/messages.inc index a478683f0c..a4cb2e6e5b 100644 --- a/maintenance/language/messages.inc +++ b/maintenance/language/messages.inc @@ -1181,6 +1181,7 @@ $wgMessageStructure = array( 'action-suppressionlog', 'action-block', 'action-protect', + 'action-rollback', 'action-import', 'action-importupload', 'action-patrol', -- 2.20.1