From 6dd64b7b9b602e4cfe5360d8d9997070cd890fe5 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Tue, 20 Aug 2019 22:28:47 -0700 Subject: [PATCH] Convert PermissionManager constructor to use ServiceOptions. Change-Id: I36a3a2f338506ef14cc5d65b8bee2961a92d60da --- includes/Permissions/PermissionManager.php | 118 ++++++++---------- includes/ServiceWiring.php | 11 +- .../Permissions/PermissionManagerTest.php | 23 ++-- .../MWBasicRequestAuthorizerTest.php | 31 ++--- 4 files changed, 81 insertions(+), 102 deletions(-) diff --git a/includes/Permissions/PermissionManager.php b/includes/Permissions/PermissionManager.php index 248ba14dc6..3c0a6e28be 100644 --- a/includes/Permissions/PermissionManager.php +++ b/includes/Permissions/PermissionManager.php @@ -22,6 +22,7 @@ namespace MediaWiki\Permissions; use Action; use Exception; use Hooks; +use MediaWiki\Config\ServiceOptions; use MediaWiki\Linker\LinkTarget; use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\RevisionRecord; @@ -54,36 +55,34 @@ class PermissionManager { /** @var string Does cheap and expensive checks, using the master as needed */ const RIGOR_SECURE = 'secure'; + /** + * TODO Make this const when HHVM support is dropped (T192166) + * + * @since 1.34 + * @var array + */ + public static $constructorOptions = [ + 'WhitelistRead', + 'WhitelistReadRegexp', + 'EmailConfirmToEdit', + 'BlockDisablesLogin', + 'GroupPermissions', + 'RevokePermissions', + 'AvailableRights' + ]; + + /** @var ServiceOptions */ + private $options; + /** @var SpecialPageFactory */ private $specialPageFactory; /** @var RevisionLookup */ private $revisionLookup; - /** @var string[] List of pages names anonymous user may see */ - private $whitelistRead; - - /** @var string[] Whitelists publicly readable titles with regular expressions */ - private $whitelistReadRegexp; - - /** @var bool Require users to confirm email address before they can edit */ - private $emailConfirmToEdit; - - /** @var bool If set to true, blocked users will no longer be allowed to log in */ - private $blockDisablesLogin; - /** @var NamespaceInfo */ private $nsInfo; - /** @var string[][] Access rights for groups and users in these groups */ - private $groupPermissions; - - /** @var string[][] Permission keys revoked from users in each group */ - private $revokePermissions; - - /** @var string[] A list of available rights, in addition to the ones defined by the core */ - private $availableRights; - /** @var string[] Cached results of getAllRights() */ private $allRights = false; @@ -189,38 +188,21 @@ class PermissionManager { ]; /** + * @param ServiceOptions $options * @param SpecialPageFactory $specialPageFactory * @param RevisionLookup $revisionLookup - * @param string[] $whitelistRead - * @param string[] $whitelistReadRegexp - * @param bool $emailConfirmToEdit - * @param bool $blockDisablesLogin - * @param string[][] $groupPermissions - * @param string[][] $revokePermissions - * @param string[] $availableRights * @param NamespaceInfo $nsInfo */ public function __construct( + ServiceOptions $options, SpecialPageFactory $specialPageFactory, RevisionLookup $revisionLookup, - $whitelistRead, - $whitelistReadRegexp, - $emailConfirmToEdit, - $blockDisablesLogin, - $groupPermissions, - $revokePermissions, - $availableRights, NamespaceInfo $nsInfo ) { + $options->assertRequiredOptions( self::$constructorOptions ); + $this->options = $options; $this->specialPageFactory = $specialPageFactory; $this->revisionLookup = $revisionLookup; - $this->whitelistRead = $whitelistRead; - $this->whitelistReadRegexp = $whitelistReadRegexp; - $this->emailConfirmToEdit = $emailConfirmToEdit; - $this->blockDisablesLogin = $blockDisablesLogin; - $this->groupPermissions = $groupPermissions; - $this->revokePermissions = $revokePermissions; - $this->availableRights = $availableRights; $this->nsInfo = $nsInfo; } @@ -500,6 +482,7 @@ class PermissionManager { // TODO: remove when LinkTarget usage will expand further $title = Title::newFromLinkTarget( $page ); + $whiteListRead = $this->options->get( 'WhitelistRead' ); $whitelisted = false; if ( $this->isEveryoneAllowed( 'read' ) ) { # Shortcut for public wikis, allows skipping quite a bit of code @@ -514,20 +497,20 @@ class PermissionManager { # Always grant access to the login page. # Even anons need to be able to log in. $whitelisted = true; - } elseif ( is_array( $this->whitelistRead ) && count( $this->whitelistRead ) ) { + } elseif ( is_array( $whiteListRead ) && count( $whiteListRead ) ) { # Time to check the whitelist # Only do these checks is there's something to check against $name = $title->getPrefixedText(); $dbName = $title->getPrefixedDBkey(); // Check for explicit whitelisting with and without underscores - if ( in_array( $name, $this->whitelistRead, true ) - || in_array( $dbName, $this->whitelistRead, true ) ) { + if ( in_array( $name, $whiteListRead, true ) + || in_array( $dbName, $whiteListRead, true ) ) { $whitelisted = true; } elseif ( $title->getNamespace() == NS_MAIN ) { # Old settings might have the title prefixed with # a colon for main-namespace pages - if ( in_array( ':' . $name, $this->whitelistRead ) ) { + if ( in_array( ':' . $name, $whiteListRead ) ) { $whitelisted = true; } } elseif ( $title->isSpecialPage() ) { @@ -537,18 +520,19 @@ class PermissionManager { $this->specialPageFactory->resolveAlias( $name ); if ( $name ) { $pure = SpecialPage::getTitleFor( $name )->getPrefixedText(); - if ( in_array( $pure, $this->whitelistRead, true ) ) { + if ( in_array( $pure, $whiteListRead, true ) ) { $whitelisted = true; } } } } - if ( !$whitelisted && is_array( $this->whitelistReadRegexp ) - && !empty( $this->whitelistReadRegexp ) ) { + $whitelistReadRegexp = $this->options->get( 'WhitelistReadRegexp' ); + if ( !$whitelisted && is_array( $whitelistReadRegexp ) + && !empty( $whitelistReadRegexp ) ) { $name = $title->getPrefixedText(); // Check for regex whitelisting - foreach ( $this->whitelistReadRegexp as $listItem ) { + foreach ( $whitelistReadRegexp as $listItem ) { if ( preg_match( $listItem, $name ) ) { $whitelisted = true; break; @@ -636,11 +620,11 @@ class PermissionManager { } // Optimize for a very common case - if ( $action === 'read' && !$this->blockDisablesLogin ) { + if ( $action === 'read' && !$this->options->get( 'BlockDisablesLogin' ) ) { return $errors; } - if ( $this->emailConfirmToEdit + if ( $this->options->get( 'EmailConfirmToEdit' ) && !$user->isEmailConfirmed() && $action === 'edit' ) { @@ -1246,7 +1230,7 @@ class PermissionManager { if ( $user->isLoggedIn() && - $this->blockDisablesLogin && + $this->options->get( 'BlockDisablesLogin' ) && $user->getBlock() ) { $anon = new User; @@ -1296,10 +1280,10 @@ class PermissionManager { * @return bool */ public function groupHasPermission( $group, $role ) { - return isset( $this->groupPermissions[$group][$role] ) && - $this->groupPermissions[$group][$role] && - !( isset( $this->revokePermissions[$group][$role] ) && - $this->revokePermissions[$group][$role] ); + $groupPermissions = $this->options->get( 'GroupPermissions' ); + $revokePermissions = $this->options->get( 'RevokePermissions' ); + return isset( $groupPermissions[$group][$role] ) && $groupPermissions[$group][$role] && + !( isset( $revokePermissions[$group][$role] ) && $revokePermissions[$group][$role] ); } /** @@ -1314,17 +1298,17 @@ class PermissionManager { $rights = []; // grant every granted permission first foreach ( $groups as $group ) { - if ( isset( $this->groupPermissions[$group] ) ) { + if ( isset( $this->options->get( 'GroupPermissions' )[$group] ) ) { $rights = array_merge( $rights, // array_filter removes empty items - array_keys( array_filter( $this->groupPermissions[$group] ) ) ); + array_keys( array_filter( $this->options->get( 'GroupPermissions' )[$group] ) ) ); } } // now revoke the revoked permissions foreach ( $groups as $group ) { - if ( isset( $this->revokePermissions[$group] ) ) { + if ( isset( $this->options->get( 'RevokePermissions' )[$group] ) ) { $rights = array_diff( $rights, - array_keys( array_filter( $this->revokePermissions[$group] ) ) ); + array_keys( array_filter( $this->options->get( 'RevokePermissions' )[$group] ) ) ); } } return array_unique( $rights ); @@ -1340,7 +1324,7 @@ class PermissionManager { */ public function getGroupsWithPermission( $role ) { $allowedGroups = []; - foreach ( array_keys( $this->groupPermissions ) as $group ) { + foreach ( array_keys( $this->options->get( 'GroupPermissions' ) ) as $group ) { if ( $this->groupHasPermission( $group, $role ) ) { $allowedGroups[] = $group; } @@ -1370,14 +1354,14 @@ class PermissionManager { return $this->cachedRights[$right]; } - if ( !isset( $this->groupPermissions['*'][$right] ) - || !$this->groupPermissions['*'][$right] ) { + if ( !isset( $this->options->get( 'GroupPermissions' )['*'][$right] ) + || !$this->options->get( 'GroupPermissions' )['*'][$right] ) { $this->cachedRights[$right] = false; return false; } // If it's revoked anywhere, then everyone doesn't have it - foreach ( $this->revokePermissions as $rights ) { + foreach ( $this->options->get( 'RevokePermissions' ) as $rights ) { if ( isset( $rights[$right] ) && $rights[$right] ) { $this->cachedRights[$right] = false; return false; @@ -1415,10 +1399,10 @@ class PermissionManager { */ public function getAllPermissions() { if ( $this->allRights === false ) { - if ( count( $this->availableRights ) ) { + if ( count( $this->options->get( 'AvailableRights' ) ) ) { $this->allRights = array_unique( array_merge( $this->coreRights, - $this->availableRights + $this->options->get( 'AvailableRights' ) ) ); } else { $this->allRights = $this->coreRights; diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 663f3710ce..7c13ac8847 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -497,17 +497,12 @@ return [ }, 'PermissionManager' => function ( MediaWikiServices $services ) : PermissionManager { - $config = $services->getMainConfig(); return new PermissionManager( + new ServiceOptions( + PermissionManager::$constructorOptions, $services->getMainConfig() + ), $services->getSpecialPageFactory(), $services->getRevisionLookup(), - $config->get( 'WhitelistRead' ), - $config->get( 'WhitelistReadRegexp' ), - $config->get( 'EmailConfirmToEdit' ), - $config->get( 'BlockDisablesLogin' ), - $config->get( 'GroupPermissions' ), - $config->get( 'RevokePermissions' ), - $config->get( 'AvailableRights' ), $services->getNamespaceInfo() ); }, diff --git a/tests/phpunit/includes/Permissions/PermissionManagerTest.php b/tests/phpunit/includes/Permissions/PermissionManagerTest.php index 86942fd8d1..3c5f43bda8 100644 --- a/tests/phpunit/includes/Permissions/PermissionManagerTest.php +++ b/tests/phpunit/includes/Permissions/PermissionManagerTest.php @@ -5,6 +5,7 @@ namespace MediaWiki\Tests\Permissions; use Action; use ContentHandler; use FauxRequest; +use LoggedServiceOptions; use MediaWiki\Block\DatabaseBlock; use MediaWiki\Block\Restriction\NamespaceRestriction; use MediaWiki\Block\Restriction\PageRestriction; @@ -14,6 +15,7 @@ use MediaWiki\MediaWikiServices; use MediaWiki\Permissions\PermissionManager; use MediaWiki\Revision\MutableRevisionRecord; use MediaWiki\Revision\RevisionLookup; +use TestAllServiceOptionsUsed; use Wikimedia\ScopedCallback; use MediaWiki\Session\SessionId; use MediaWiki\Session\TestUtils; @@ -30,6 +32,7 @@ use Wikimedia\TestingAccessWrapper; * @covers \MediaWiki\Permissions\PermissionManager */ class PermissionManagerTest extends MediaWikiLangTestCase { + use TestAllServiceOptionsUsed; /** * @var string @@ -725,15 +728,21 @@ class PermissionManagerTest extends MediaWikiLangTestCase { } } ); $permissionManager = new PermissionManager( + new LoggedServiceOptions( + self::$serviceOptionsAccessLog, + PermissionManager::$constructorOptions, + [ + 'WhitelistRead' => [], + 'WhitelistReadRegexp' => [], + 'EmailConfirmToEdit' => false, + 'BlockDisablesLogin' => false, + 'GroupPermissions' => [], + 'RevokePermissions' => [], + 'AvailableRights' => [] + ] + ), $services->getSpecialPageFactory(), $revisionLookup, - [], - [], - false, - false, - [], - [], - [], MediaWikiServices::getInstance()->getNamespaceInfo() ); $this->setService( 'PermissionManager', $permissionManager ); diff --git a/tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php b/tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php index 076ff3643b..49dcf07798 100644 --- a/tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php +++ b/tests/phpunit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php @@ -3,13 +3,12 @@ namespace MediaWiki\Tests\Rest\BasicAccess; use GuzzleHttp\Psr7\Uri; -use MediaWiki\Permissions\PermissionManager; +use MediaWiki\MediaWikiServices; use MediaWiki\Rest\BasicAccess\MWBasicAuthorizer; use MediaWiki\Rest\Handler; use MediaWiki\Rest\RequestData; use MediaWiki\Rest\ResponseFactory; use MediaWiki\Rest\Router; -use MediaWiki\User\UserIdentity; use MediaWikiTestCase; use User; @@ -24,23 +23,15 @@ use User; class MWBasicRequestAuthorizerTest extends MediaWikiTestCase { private function createRouter( $userRights ) { $user = User::newFromName( 'Test user' ); - - $pm = new class( $user, $userRights ) extends PermissionManager { - private $testUser; - private $testUserRights; - - public function __construct( $user, $userRights ) { - $this->testUser = $user; - $this->testUserRights = $userRights; - } - - public function userHasRight( UserIdentity $user, $action = '' ) { - if ( $user === $this->testUser ) { - return $this->testUserRights[$action] ?? false; - } - return parent::userHasRight( $user, $action ); - } - }; + // Don't allow the rights to everybody so that user rights kick in. + $this->mergeMwGlobalArrayValue( 'wgGroupPermissions', [ '*' => $userRights ] ); + $this->resetServices(); + $this->overrideUserPermissions( + $user, + array_keys( array_filter( $userRights ), function ( $value ) { + return $value === true; + } ) + ); global $IP; @@ -50,7 +41,7 @@ class MWBasicRequestAuthorizerTest extends MediaWikiTestCase { '/rest', new \EmptyBagOStuff(), new ResponseFactory(), - new MWBasicAuthorizer( $user, $pm ) ); + new MWBasicAuthorizer( $user, MediaWikiServices::getInstance()->getPermissionManager() ) ); } public function testReadDenied() { -- 2.20.1