From a88f1d6b58613cbfa5c24c97887197a41ce66908 Mon Sep 17 00:00:00 2001 From: =?utf8?q?M=C3=A1t=C3=A9=20Szab=C3=B3?= Date: Sat, 18 May 2019 18:58:24 +0200 Subject: [PATCH] API: Migrate Title::userCan() calls to PermissionManager T208768 introduced the PermissionManager service that can now be used for page specific permission checks. This change replaces calls to Title::userCan() with the new service in API classes. Bug: T220191 Change-Id: I768d07a520ca6473a4eefb88c9f587657bc74357 --- includes/api/ApiBase.php | 27 +++++++++++++++---- includes/api/ApiFeedContributions.php | 4 ++- includes/api/ApiQuery.php | 3 +-- includes/api/ApiQueryInfo.php | 14 +++++++--- includes/api/ApiQueryRevisions.php | 5 ++-- includes/api/ApiUndelete.php | 2 +- .../includes/api/query/ApiQueryTest.php | 24 +++++++++++++++++ 7 files changed, 65 insertions(+), 14 deletions(-) diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index dd1a6db08a..5687f0f7d9 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -22,7 +22,9 @@ use MediaWiki\Block\AbstractBlock; use MediaWiki\Block\DatabaseBlock; +use MediaWiki\Linker\LinkTarget; use MediaWiki\MediaWikiServices; +use MediaWiki\Permissions\PermissionManager; use Wikimedia\Rdbms\IDatabase; /** @@ -698,6 +700,16 @@ abstract class ApiBase extends ContextSource { $this->getMain()->setContinuationManager( $manager ); } + /** + * Obtain a PermissionManager instance that subclasses may use in their authorization checks. + * + * @since 1.34 + * @return PermissionManager + */ + protected function getPermissionManager(): PermissionManager { + return MediaWikiServices::getInstance()->getPermissionManager(); + } + /**@}*/ /************************************************************************//** @@ -2120,17 +2132,19 @@ abstract class ApiBase extends ContextSource { /** * Helper function for permission-denied errors - * @since 1.29 - * @since 1.33 Changed the third parameter from $user to $options. - * @param Title $title + * + * @param LinkTarget $linkTarget * @param string|string[] $actions * @param array $options Additional options * - user: (User) User to use rather than $this->getUser() * - autoblock: (bool, default false) Whether to spread autoblocks * For compatibility, passing a User object is treated as the value for the 'user' option. * @throws ApiUsageException if the user doesn't have all of the rights. + * + * @since 1.29 + * @since 1.33 Changed the third parameter from $user to $options. */ - public function checkTitleUserPermissions( Title $title, $actions, $options = [] ) { + public function checkTitleUserPermissions( LinkTarget $linkTarget, $actions, $options = [] ) { if ( !is_array( $options ) ) { wfDeprecated( '$user as the third parameter to ' . __METHOD__, '1.33' ); $options = [ 'user' => $options ]; @@ -2139,7 +2153,10 @@ abstract class ApiBase extends ContextSource { $errors = []; foreach ( (array)$actions as $action ) { - $errors = array_merge( $errors, $title->getUserPermissionsErrors( $action, $user ) ); + $errors = array_merge( + $errors, + $this->getPermissionManager()->getPermissionErrors( $action, $user, $linkTarget ) + ); } if ( $errors ) { diff --git a/includes/api/ApiFeedContributions.php b/includes/api/ApiFeedContributions.php index ed3b7b8268..08be8e029c 100644 --- a/includes/api/ApiFeedContributions.php +++ b/includes/api/ApiFeedContributions.php @@ -138,7 +138,9 @@ class ApiFeedContributions extends ApiBase { // Hook completed and did not return a valid feed item $title = Title::makeTitle( (int)$row->page_namespace, $row->page_title ); - if ( $title && $title->userCan( 'read', $this->getUser() ) ) { + $user = $this->getUser(); + + if ( $title && $this->getPermissionManager()->userCan( 'read', $user, $title ) ) { $date = $row->rev_timestamp; $comments = $title->getTalkPage()->getFullURL(); $revision = $this->revisionStore->newRevisionFromRow( $row ); diff --git a/includes/api/ApiQuery.php b/includes/api/ApiQuery.php index ae6b1a1dbc..7cbd3ef602 100644 --- a/includes/api/ApiQuery.php +++ b/includes/api/ApiQuery.php @@ -429,10 +429,9 @@ class ApiQuery extends ApiBase { $exportTitles = []; $titles = $pageSet->getGoodTitles(); if ( count( $titles ) ) { - $user = $this->getUser(); /** @var Title $title */ foreach ( $titles as $title ) { - if ( $title->userCan( 'read', $user ) ) { + if ( $this->getPermissionManager()->userCan( 'read', $this->getUser(), $title ) ) { $exportTitles[] = $title; } } diff --git a/includes/api/ApiQueryInfo.php b/includes/api/ApiQueryInfo.php index 276aafbff0..90f1340eb5 100644 --- a/includes/api/ApiQueryInfo.php +++ b/includes/api/ApiQueryInfo.php @@ -409,6 +409,8 @@ class ApiQueryInfo extends ApiQueryBase { $pageInfo['pagelanguagehtmlcode'] = $pageLanguage->getHtmlCode(); $pageInfo['pagelanguagedir'] = $pageLanguage->getDir(); + $user = $this->getUser(); + if ( $titleExists ) { $pageInfo['touched'] = wfTimestamp( TS_ISO_8601, $this->pageTouched[$pageid] ); $pageInfo['lastrevid'] = (int)$this->pageLatest[$pageid]; @@ -493,7 +495,9 @@ class ApiQueryInfo extends ApiQueryBase { $pageInfo['canonicalurl'] = wfExpandUrl( $title->getFullURL(), PROTO_CANONICAL ); } if ( $this->fld_readable ) { - $pageInfo['readable'] = $title->userCan( 'read', $this->getUser() ); + $pageInfo['readable'] = $this->getPermissionManager()->userCan( + 'read', $user, $title + ); } if ( $this->fld_preload ) { @@ -539,10 +543,14 @@ class ApiQueryInfo extends ApiQueryBase { $this->countTestedActions++; if ( $detailLevel === 'boolean' ) { - $pageInfo['actions'][$action] = $title->userCan( $action, $user ); + $pageInfo['actions'][$action] = $this->getPermissionManager()->userCan( + $action, $user, $title + ); } else { $pageInfo['actions'][$action] = $errorFormatter->arrayFromStatus( $this->errorArrayToStatus( - $title->getUserPermissionsErrors( $action, $user, $rigor ), + $this->getPermissionManager()->getPermissionErrors( + $action, $user, $title, $rigor + ), $user ) ); } diff --git a/includes/api/ApiQueryRevisions.php b/includes/api/ApiQueryRevisions.php index ee6a264281..fe3ae87d52 100644 --- a/includes/api/ApiQueryRevisions.php +++ b/includes/api/ApiQueryRevisions.php @@ -201,11 +201,12 @@ class ApiQueryRevisions extends ApiQueryRevisionsBase { if ( $resultPageSet === null && $this->fetchContent ) { // For each page we will request, the user must have read rights for that page - $user = $this->getUser(); $status = Status::newGood(); + $user = $this->getUser(); + /** @var Title $title */ foreach ( $pageSet->getGoodTitles() as $title ) { - if ( !$title->userCan( 'read', $user ) ) { + if ( !$this->getPermissionManager()->userCan( 'read', $user, $title ) ) { $status->fatal( ApiMessage::create( [ 'apierror-cannotviewtitle', wfEscapeWikiText( $title->getPrefixedText() ) ], 'accessdenied' diff --git a/includes/api/ApiUndelete.php b/includes/api/ApiUndelete.php index 5f9b97c457..ba9be81bea 100644 --- a/includes/api/ApiUndelete.php +++ b/includes/api/ApiUndelete.php @@ -41,7 +41,7 @@ class ApiUndelete extends ApiBase { $this->dieWithError( [ 'apierror-invalidtitle', wfEscapeWikiText( $params['title'] ) ] ); } - if ( !$titleObj->userCan( 'undelete', $user, 'secure' ) ) { + if ( !$this->getPermissionManager()->userCan( 'undelete', $this->getUser(), $titleObj ) ) { $this->dieWithError( 'permdenied-undelete' ); } diff --git a/tests/phpunit/includes/api/query/ApiQueryTest.php b/tests/phpunit/includes/api/query/ApiQueryTest.php index de8d8156f3..20bd8557d0 100644 --- a/tests/phpunit/includes/api/query/ApiQueryTest.php +++ b/tests/phpunit/includes/api/query/ApiQueryTest.php @@ -148,4 +148,28 @@ class ApiQueryTest extends ApiTestCase { ); } } + + public function testShouldNotExportPagesThatUserCanNotRead() { + $title = Title::makeTitle( NS_MAIN, 'Test article' ); + $this->insertPage( $title ); + + $this->setTemporaryHook( 'getUserPermissionsErrors', + function ( Title $page, &$user, $action, &$result ) use ( $title ) { + if ( $page->equals( $title ) && $action === 'read' ) { + $result = false; + return false; + } + } ); + + $data = $this->doApiRequest( [ + 'action' => 'query', + 'titles' => $title->getPrefixedText(), + 'export' => 1, + ] ); + + $this->assertArrayHasKey( 'query', $data[0] ); + $this->assertArrayHasKey( 'export', $data[0]['query'] ); + // This response field contains an XML document even if no pages were exported + $this->assertNotContains( $title->getPrefixedText(), $data[0]['query']['export'] ); + } } -- 2.20.1