From a7e9f3298b776640b49abee2e2bfd6c06de1e0bc Mon Sep 17 00:00:00 2001 From: Lucas Werkmeister Date: Tue, 30 Nov 2021 11:47:59 +0100 Subject: [PATCH] SECURITY: Replace more block checks permission checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Similarly to Iac86cf63bd, update other places which explicitly checked if the user is blocked, and get all possible kinds of permission errors instead. This also subsumes separate calls to userHasRight(). In NewEntitySchema, we inherit checkPermissions() from SpecialPage, but that method doesn’t seem to do all we need (it boils down to just a userHasRight() check), so we add a new checkPermissionsWithSubpage() method that gets all the permission errors. The browser tests need some slight adjustments, since the blocked error on Special:NewEntitySchema looks a bit different now. Bug: T296578 Change-Id: Id9af124427bcd1e85301d2140a38bf47bbc5622c --- .../Actions/AbstractRestoreAction.php | 16 ++++---- src/MediaWiki/Actions/UndoSubmitAction.php | 15 ++++---- src/MediaWiki/Specials/NewEntitySchema.php | 38 +++++++++++-------- tests/selenium/specs/special/new.js | 8 ++-- 4 files changed, 43 insertions(+), 34 deletions(-) diff --git a/src/MediaWiki/Actions/AbstractRestoreAction.php b/src/MediaWiki/Actions/AbstractRestoreAction.php index 457d88f..a0a6dd2 100644 --- a/src/MediaWiki/Actions/AbstractRestoreAction.php +++ b/src/MediaWiki/Actions/AbstractRestoreAction.php @@ -45,18 +45,20 @@ protected function checkPermissions() { $services = MediaWikiServices::getInstance(); $pm = $services->getPermissionManager(); $checkReplica = !$this->getRequest()->wasPosted(); - if ( $pm->isBlockedFrom( $this->getUser(), $this->getTitle(), $checkReplica ) ) { - // @phan-suppress-next-line PhanTypeMismatchArgumentNullable - throw new UserBlockedError( $this->getUser()->getBlock() ); + + $permissionErrors = $pm->getPermissionErrors( + $this->getRestriction(), + $this->getUser(), + $this->getTitle(), + $checkReplica ? $pm::RIGOR_FULL : $pm::RIGOR_SECURE + ); + if ( $permissionErrors !== [] ) { + throw new PermissionsError( $this->getRestriction(), $permissionErrors ); } if ( $services->getReadOnlyMode()->isReadOnly() ) { throw new ReadOnlyError; } - - if ( !$pm->userHasRight( $this->getUser(), $this->getRestriction() ) ) { - throw new PermissionsError( $this->getRestriction() ); - } } /** diff --git a/src/MediaWiki/Actions/UndoSubmitAction.php b/src/MediaWiki/Actions/UndoSubmitAction.php index a1b7e88..62aba08 100644 --- a/src/MediaWiki/Actions/UndoSubmitAction.php +++ b/src/MediaWiki/Actions/UndoSubmitAction.php @@ -58,19 +58,20 @@ private function checkPermissions(): Status { $services = MediaWikiServices::getInstance(); $pm = $services->getPermissionManager(); - if ( $pm->isBlockedFrom( $this->getUser(), $this->getTitle() ) ) { - // @phan-suppress-next-line PhanTypeMismatchArgumentNullable - throw new UserBlockedError( $this->getUser()->getBlock() ); + + $permissionErrors = $pm->getPermissionErrors( + $this->getRestriction(), + $this->getUser(), + $this->getTitle() + ); + if ( $permissionErrors !== [] ) { + throw new PermissionsError( $this->getRestriction(), $permissionErrors ); } if ( $services->getReadOnlyMode()->isReadOnly() ) { throw new ReadOnlyError; } - if ( !$pm->userHasRight( $this->getUser(), $this->getRestriction() ) ) { - throw new PermissionsError( $this->getRestriction() ); - } - return Status::newGood(); } diff --git a/src/MediaWiki/Specials/NewEntitySchema.php b/src/MediaWiki/Specials/NewEntitySchema.php index 7fbb61d..a2a8f8e 100644 --- a/src/MediaWiki/Specials/NewEntitySchema.php +++ b/src/MediaWiki/Specials/NewEntitySchema.php @@ -12,10 +12,10 @@ use Language; use MediaWiki\MediaWikiServices; use OutputPage; +use PermissionsError; use SpecialPage; use Status; use Title; -use UserBlockedError; /** * Page for creating a new EntitySchema. @@ -44,8 +44,7 @@ public function __construct() { public function execute( $subPage ) { parent::execute( $subPage ); - $this->checkPermissions(); - $this->checkBlocked( $subPage ); + $this->checkPermissionsWithSubpage( $subPage ); $this->checkReadOnly(); $form = HTMLForm::factory( 'ooui', $this->getFormFields(), $this->getContext() ) @@ -225,22 +224,29 @@ private function addJavaScript() { } /** - * Checks if the user is blocked from this page, - * and if they are, throws a {@link UserBlockedError}. + * Checks if the user has permissions to perform this page’s action, + * and throws a {@link PermissionsError} if they don’t. * - * @throws UserBlockedError + * @throws PermissionsError */ - protected function checkBlocked( $subPage ) { + protected function checkPermissionsWithSubpage( $subPage ) { + $pm = MediaWikiServices::getInstance()->getPermissionManager(); $checkReplica = !$this->getRequest()->wasPosted(); - if ( MediaWikiServices::getInstance()->getPermissionManager() - ->isBlockedFrom( - $this->getUser(), - $this->getPageTitle( $subPage ), - $checkReplica - ) - ) { - // @phan-suppress-next-line PhanTypeMismatchArgumentNullable - throw new UserBlockedError( $this->getUser()->getBlock() ); + $permissionErrors = $pm->getPermissionErrors( + $this->getRestriction(), + $this->getUser(), + $this->getPageTitle( $subPage ), + $checkReplica ? $pm::RIGOR_FULL : $pm::RIGOR_SECURE, + [ + 'ns-specialprotected', // ignore “special pages cannot be edited” + ] + ); + if ( $permissionErrors !== [] ) { + // reindex $permissionErrors: + // the ignoreErrors param (ns-specialprotected) may have left holes, + // but PermissionsError expects $errors[0] to exist + $permissionErrors = array_values( $permissionErrors ); + throw new PermissionsError( $this->getRestriction(), $permissionErrors ); } } diff --git a/tests/selenium/specs/special/new.js b/tests/selenium/specs/special/new.js index 71444e3..4cec408 100644 --- a/tests/selenium/specs/special/new.js +++ b/tests/selenium/specs/special/new.js @@ -63,8 +63,8 @@ describe( 'NewEntitySchema:Page', () => { LoginPage.loginAdmin(); NewEntitySchemaPage.open(); - $( '#mw-returnto' ).waitForDisplayed(); - assert.strictEqual( $( '#firstHeading' ).getText(), 'User is blocked' ); + $( '.permissions-errors' ).waitForDisplayed(); + assert.strictEqual( $( '#firstHeading' ).getText(), 'Permission error' ); } ); it( 'cannot submit form', () => { @@ -77,8 +77,8 @@ describe( 'NewEntitySchema:Page', () => { NewEntitySchemaPage.clickSubmit(); - $( '#mw-returnto' ).waitForDisplayed(); - assert.strictEqual( $( '#firstHeading' ).getText(), 'User is blocked' ); + $( '.permissions-errors' ).waitForDisplayed(); + assert.strictEqual( $( '#firstHeading' ).getText(), 'Permission error' ); } ); } ); -- 2.30.2