From b3b70624c9ca8ba73bd8b3448a7d7fb30a6f0e5b Mon Sep 17 00:00:00 2001 From: daniel Date: Wed, 10 Mar 2021 20:40:33 +0100 Subject: [PATCH] Authority: expose user block info Expose info about user blocks from Authority. This allows calling code to provide more detailed information to the user about why they are denied some action on the wiki. Bug: T271494 Change-Id: Ia84e469888866d72752aad355292666c31e12bad --- RELEASE-NOTES-1.37 | 6 + includes/Permissions/Authority.php | 25 ++++ includes/Permissions/PermissionManager.php | 29 ++++- includes/Permissions/PermissionStatus.php | 28 +++++ includes/Permissions/SimpleAuthority.php | 12 ++ includes/Permissions/UltimateAuthority.php | 11 ++ includes/Permissions/UserAuthority.php | 46 ++++++- includes/ServiceWiring.php | 3 +- includes/api/ApiBase.php | 34 +++--- includes/api/ApiUndelete.php | 6 +- includes/api/ApiUserrights.php | 3 +- includes/block/BlockPermissionChecker.php | 14 +-- .../block/BlockPermissionCheckerFactory.php | 10 +- includes/block/DatabaseBlockStore.php | 8 +- includes/installer/WebInstallerOptions.php | 7 -- includes/user/User.php | 45 +++++-- includes/user/UserGroupManager.php | 2 +- tests/phpunit/includes/user/UserTest.php | 40 ++++++- .../mocks/permissions/MockAuthorityTrait.php | 33 ++++- .../Permissions/PermissionStatusTest.php | 12 ++ .../Permissions/SimpleAuthorityTest.php | 7 ++ .../Permissions/UltimateAuthorityTest.php | 7 ++ .../Permissions/UserAuthorityTest.php | 113 ++++++++++++++++-- .../block/BlockPermissionCheckerTest.php | 59 +++------ 24 files changed, 435 insertions(+), 125 deletions(-) diff --git a/RELEASE-NOTES-1.37 b/RELEASE-NOTES-1.37 index 09c2e6d34e8..e683c5b3d3f 100644 --- a/RELEASE-NOTES-1.37 +++ b/RELEASE-NOTES-1.37 @@ -345,6 +345,12 @@ because of Phabricator reports. the method it wraps, User->isRegistered(), instead. * FileBackendGroup::singleton() and ::destroySingletons(), deprecated since 1.35, now emit deprecation warnings. +* The first parameter of User::getBlock should now be an integer using the + Authority::FOR_XXX constants. Providing a boolean is deprecated. +* ApiBase::addBlockInfoToStatus() is deprecated for use by extensions. It is now + marked as @internal and may be deleted in the future. + It should not be necessary to call this method, Authority should be providing + all relevant information via a PermissionStatus object. * JobQueueGroup::singleton was deprecated - use MediaWikiServices::getJobQueueGroup instead. * JobQueueGroup::destroySingletons was deprecated. JobQueueGroups are now diff --git a/includes/Permissions/Authority.php b/includes/Permissions/Authority.php index 324b01fa364..8e9d63b12b8 100644 --- a/includes/Permissions/Authority.php +++ b/includes/Permissions/Authority.php @@ -20,6 +20,8 @@ namespace MediaWiki\Permissions; +use IDBAccessObject; +use MediaWiki\Block\Block; use MediaWiki\Page\PageIdentity; use MediaWiki\User\UserIdentity; @@ -34,6 +36,18 @@ use MediaWiki\User\UserIdentity; */ interface Authority { + /** + * @var int Fetch information quickly, slightly stale data is acceptable. + * @see IDBAccessObject::READ_NORMAL + */ + public const READ_NORMAL = IDBAccessObject::READ_NORMAL; + + /** + * @var int Fetch information reliably, stale data is not acceptable. + * @see IDBAccessObject::READ_LATEST + */ + public const READ_LATEST = IDBAccessObject::READ_LATEST; + /** * Returns the performer of the actions associated with this authority. * @@ -44,6 +58,17 @@ interface Authority { */ public function getUser(): UserIdentity; + /** + * Returns any user block affecting the Authority. + * + * @param int $freshness Indicates whether slightly stale data is acceptable in, + * exchange for a fast response. + * + * @return ?Block + * @since 1.37 + */ + public function getBlock( int $freshness = self::READ_NORMAL ): ?Block; + /** * Checks whether this authority has the given permission in general. * For some permissions, exceptions may exist, both positive and negative, on a per-target basis. diff --git a/includes/Permissions/PermissionManager.php b/includes/Permissions/PermissionManager.php index d6fb48051ca..3783d219f37 100644 --- a/includes/Permissions/PermissionManager.php +++ b/includes/Permissions/PermissionManager.php @@ -28,6 +28,7 @@ use MediaWiki\Config\ServiceOptions; use MediaWiki\HookContainer\HookContainer; use MediaWiki\HookContainer\HookRunner; use MediaWiki\Linker\LinkTarget; +use MediaWiki\Page\PageIdentity; use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Session\SessionManager; @@ -329,19 +330,22 @@ class PermissionManager { * have a block, this will return false. * * @param User $user - * @param LinkTarget $page Title to check + * @param PageIdentity|LinkTarget $page Title to check * @param bool $fromReplica Whether to check the replica DB instead of the master * * @return bool */ - public function isBlockedFrom( User $user, LinkTarget $page, $fromReplica = false ) { + public function isBlockedFrom( User $user, $page, $fromReplica = false ) { $block = $user->getBlock( $fromReplica ); if ( !$block ) { return false; } - // TODO: remove upon further migration to LinkTarget - $title = Title::newFromLinkTarget( $page ); + if ( $page instanceof PageIdentity ) { + $title = Title::castFromPageIdentity( $page ); + } else { + $title = Title::castFromLinkTarget( $page ); + } $blocked = $user->isHidden(); if ( !$blocked ) { @@ -721,8 +725,21 @@ class PermissionManager { $errors[] = [ 'confirmedittext' ]; } - $useReplica = ( $rigor !== self::RIGOR_SECURE ); - $block = $user->getBlock( $useReplica ); + switch ( $rigor ) { + case self::RIGOR_SECURE: + $blockInfoFreshness = Authority::READ_LATEST; + $useReplica = false; + break; + case self::RIGOR_FULL: + $blockInfoFreshness = Authority::READ_NORMAL; + $useReplica = true; + break; + default: + $useReplica = true; + $blockInfoFreshness = Authority::READ_NORMAL; + } + + $block = $user->getBlock( $blockInfoFreshness ); if ( $action === 'createaccount' ) { $applicableBlock = null; diff --git a/includes/Permissions/PermissionStatus.php b/includes/Permissions/PermissionStatus.php index 38dc3845c97..129a108e104 100644 --- a/includes/Permissions/PermissionStatus.php +++ b/includes/Permissions/PermissionStatus.php @@ -20,6 +20,7 @@ namespace MediaWiki\Permissions; +use MediaWiki\Block\Block; use StatusValue; /** @@ -33,6 +34,33 @@ use StatusValue; */ class PermissionStatus extends StatusValue { + /** @var ?Block */ + private $block = null; + + /** + * Returns the user block that contributed to permissions being denied, + * if such a block was provided via setBlock(). + * + * This is intended to be used to provide additional information to the user that + * allows them to determine the reason for them being denied an action. + * + * @since 1.37 + * + * @return ?Block + */ + public function getBlock(): ?Block { + return $this->block; + } + + /** + * @since 1.37 + * @internal + * @param Block $block + */ + public function setBlock( Block $block ): void { + $this->block = $block; + } + /** * @return static */ diff --git a/includes/Permissions/SimpleAuthority.php b/includes/Permissions/SimpleAuthority.php index 54b063b1722..51da09fdcc5 100644 --- a/includes/Permissions/SimpleAuthority.php +++ b/includes/Permissions/SimpleAuthority.php @@ -21,6 +21,7 @@ namespace MediaWiki\Permissions; use InvalidArgumentException; +use MediaWiki\Block\Block; use MediaWiki\Page\PageIdentity; use MediaWiki\User\UserIdentity; @@ -60,6 +61,16 @@ class SimpleAuthority implements Authority { return $this->actor; } + /** + * @param int $freshness + * + * @return ?Block always null + * @since 1.37 + */ + public function getBlock( int $freshness = self::READ_NORMAL ): ?Block { + return null; + } + /** * @inheritDoc * @@ -191,4 +202,5 @@ class SimpleAuthority implements Authority { ): bool { return $this->checkPermission( $action, $status ); } + } diff --git a/includes/Permissions/UltimateAuthority.php b/includes/Permissions/UltimateAuthority.php index 8d405eb526f..01a4c99bd34 100644 --- a/includes/Permissions/UltimateAuthority.php +++ b/includes/Permissions/UltimateAuthority.php @@ -21,6 +21,7 @@ namespace MediaWiki\Permissions; use InvalidArgumentException; +use MediaWiki\Block\Block; use MediaWiki\Page\PageIdentity; use MediaWiki\User\UserIdentity; @@ -53,6 +54,16 @@ class UltimateAuthority implements Authority { return $this->actor; } + /** + * @param int $freshness + * + * @return ?Block always null + * @since 1.37 + */ + public function getBlock( int $freshness = self::READ_NORMAL ): ?Block { + return null; + } + /** * @inheritDoc * diff --git a/includes/Permissions/UserAuthority.php b/includes/Permissions/UserAuthority.php index 0b8f92c046f..57e221040c1 100644 --- a/includes/Permissions/UserAuthority.php +++ b/includes/Permissions/UserAuthority.php @@ -21,6 +21,7 @@ namespace MediaWiki\Permissions; use InvalidArgumentException; +use MediaWiki\Block\Block; use MediaWiki\Linker\LinkTarget; use MediaWiki\Page\PageIdentity; use MediaWiki\User\UserIdentity; @@ -49,10 +50,17 @@ class UserAuthority implements Authority { private $permissionManager; /** - * @var UserIdentity + * @var User */ private $actor; + /** + * Local cache for user block information. False is used to indicate that there is no block, + * while null indicates that we don't know and have to check. + * @var Block|false|null + */ + private $userBlock = null; + /** * @param User $user * @param PermissionManager $permissionManager @@ -254,8 +262,22 @@ class UserAuthority implements Authority { $rigor ); + $blockError = false; foreach ( $errors as $err ) { $status->fatal( ...$err ); + + // HACK: Detect whether the permission was denied because the user is blocked. + // A similar hack exists in ApiBase::$blockMsgMap. + // When permission checking logic is moved out of PermissionManager, + // we can record the block info directly when first checking the block, + // rather than doing that here. + if ( strpos( $err[0], 'blockedtext' ) !== false ) { + $block = $this->getBlock(); + + if ( $block ) { + $status->setBlock( $block ); + } + } } return $status->isOK(); @@ -270,4 +292,26 @@ class UserAuthority implements Authority { } } + /** + * Returns any user block affecting the Authority. + * + * @param int $freshness + * + * @return ?Block + * @since 1.37 + * + */ + public function getBlock( int $freshness = self::READ_NORMAL ): ?Block { + // Cache block info, so we don't have to fetch it again unnecessarily. + if ( $this->userBlock === null || $freshness === self::READ_LATEST ) { + $this->userBlock = $this->actor->getBlock( $freshness ); + + // if we got null back, remember this as "false" + $this->userBlock = $this->userBlock ?: false; + } + + // if we remembered "false", return null + return $this->userBlock ?: null; + } + } diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 586704dce64..243d2ce70f2 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -245,8 +245,7 @@ return [ BlockPermissionCheckerFactory::CONSTRUCTOR_OPTIONS, $services->getMainConfig() ), - $services->getBlockUtils(), - $services->getUserFactory() + $services->getBlockUtils() ); }, diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index bcf8a1fe214..06d4188eed3 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -22,13 +22,13 @@ use MediaWiki\Api\ApiHookRunner; use MediaWiki\Api\Validator\SubmoduleDef; -use MediaWiki\Block\AbstractBlock; -use MediaWiki\Block\DatabaseBlock; +use MediaWiki\Block\Block; use MediaWiki\HookContainer\HookContainer; use MediaWiki\Linker\LinkTarget; use MediaWiki\MediaWikiServices; use MediaWiki\Page\PageIdentity; use MediaWiki\ParamValidator\TypeDef\NamespaceDef; +use MediaWiki\Permissions\Authority; use MediaWiki\Permissions\GroupPermissionsLookup; use MediaWiki\Permissions\PermissionManager; use MediaWiki\Permissions\PermissionStatus; @@ -1216,19 +1216,25 @@ abstract class ApiBase extends ContextSource { /** * Add block info to block messages in a Status * @since 1.33 + * @internal since 1.37, should become protected in the future. * @param StatusValue $status - * @param User|null $user + * @param Authority|null $user */ - public function addBlockInfoToStatus( StatusValue $status, User $user = null ) { - if ( $user === null ) { - $user = $this->getUser(); + public function addBlockInfoToStatus( StatusValue $status, Authority $user = null ) { + if ( $status instanceof PermissionStatus ) { + $block = $status->getBlock(); + } else { + $user = $user ?: $this->getAuthority(); + $block = $user->getBlock(); } - foreach ( self::$blockMsgMap as $msg => list( $apiMsg, $code ) ) { - if ( $status->hasMessage( $msg ) && $user->getBlock() ) { - $status->replaceMessage( $msg, ApiMessage::create( $apiMsg, $code, - [ 'blockinfo' => $this->getBlockDetails( $user->getBlock() ) ] - ) ); + if ( $block ) { + foreach ( self::$blockMsgMap as $msg => list( $apiMsg, $code ) ) { + if ( $status->hasMessage( $msg ) ) { + $status->replaceMessage( $msg, ApiMessage::create( $apiMsg, $code, + [ 'blockinfo' => $this->getBlockDetails( $block ) ] + ) ); + } } } } @@ -1403,12 +1409,12 @@ abstract class ApiBase extends ContextSource { * error handler and die with an error message including block info. * * @since 1.27 - * @param AbstractBlock $block The block used to generate the ApiUsageException + * @param Block $block The block used to generate the ApiUsageException * @throws ApiUsageException always */ - public function dieBlocked( AbstractBlock $block ) { + public function dieBlocked( Block $block ) { // Die using the appropriate message depending on block type - if ( $block->getType() == DatabaseBlock::TYPE_AUTO ) { + if ( $block->getType() == Block::TYPE_AUTO ) { $this->dieWithError( 'apierror-autoblocked', 'autoblocked', diff --git a/includes/api/ApiUndelete.php b/includes/api/ApiUndelete.php index b59f1f5999a..1a8162c9805 100644 --- a/includes/api/ApiUndelete.php +++ b/includes/api/ApiUndelete.php @@ -20,6 +20,8 @@ * @file */ +use MediaWiki\Permissions\Authority; + /** * @ingroup API */ @@ -40,9 +42,9 @@ class ApiUndelete extends ApiBase { $params = $this->extractRequestParams(); $user = $this->getUser(); - $block = $user->getBlock(); + $block = $user->getBlock( Authority::READ_LATEST ); if ( $block && $block->isSitewide() ) { - $this->dieBlocked( $user->getBlock() ); + $this->dieBlocked( $block ); } $titleObj = Title::newFromText( $params['title'] ); diff --git a/includes/api/ApiUserrights.php b/includes/api/ApiUserrights.php index 0791c818d74..cf4de40dd09 100644 --- a/includes/api/ApiUserrights.php +++ b/includes/api/ApiUserrights.php @@ -24,6 +24,7 @@ */ use MediaWiki\ParamValidator\TypeDef\UserDef; +use MediaWiki\Permissions\Authority; /** * @ingroup API @@ -54,7 +55,7 @@ class ApiUserrights extends ApiBase { // Deny if the user is blocked and doesn't have the full 'userrights' permission. // This matches what Special:UserRights does for the web UI. if ( !$this->getAuthority()->isAllowed( 'userrights' ) ) { - $block = $pUser->getBlock(); + $block = $pUser->getBlock( Authority::READ_LATEST ); if ( $block && $block->isSitewide() ) { $this->dieBlocked( $block ); } diff --git a/includes/block/BlockPermissionChecker.php b/includes/block/BlockPermissionChecker.php index 9bd91540a61..8c65bef9ceb 100644 --- a/includes/block/BlockPermissionChecker.php +++ b/includes/block/BlockPermissionChecker.php @@ -23,7 +23,6 @@ namespace MediaWiki\Block; use MediaWiki\Config\ServiceOptions; use MediaWiki\Permissions\Authority; -use MediaWiki\User\UserFactory; use MediaWiki\User\UserIdentity; /** @@ -62,26 +61,20 @@ class BlockPermissionChecker { /** @var ServiceOptions */ private $options; - /** @var UserFactory */ - private $userFactory; - /** * @param ServiceOptions $options * @param BlockUtils $blockUtils - * @param UserFactory $userFactory * @param UserIdentity|string|null $target * @param Authority $performer */ public function __construct( ServiceOptions $options, BlockUtils $blockUtils, - UserFactory $userFactory, $target, Authority $performer ) { $options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS ); $this->options = $options; - $this->userFactory = $userFactory; list( $this->target, $this->targetType ) = $blockUtils->parseBlockTarget( $target ); $this->performer = $performer; } @@ -120,10 +113,7 @@ class BlockPermissionChecker { * @return bool|string True when checks passed, message code for failures */ public function checkBlockPermissions() { - $performerIdentity = $this->performer->getUser(); - $legacyUser = $this->userFactory->newFromUserIdentity( $performerIdentity ); - - $block = $legacyUser->getBlock(); + $block = $this->performer->getBlock(); // TODO: pass disposition parameter if ( !$block ) { // User is not blocked, process as normal return true; @@ -134,6 +124,8 @@ class BlockPermissionChecker { return true; } + $performerIdentity = $this->performer->getUser(); + if ( $this->target instanceof UserIdentity && $this->target->getId() === $performerIdentity->getId() diff --git a/includes/block/BlockPermissionCheckerFactory.php b/includes/block/BlockPermissionCheckerFactory.php index ec647288e0c..2e53c40d986 100644 --- a/includes/block/BlockPermissionCheckerFactory.php +++ b/includes/block/BlockPermissionCheckerFactory.php @@ -23,7 +23,6 @@ namespace MediaWiki\Block; use MediaWiki\Config\ServiceOptions; use MediaWiki\Permissions\Authority; -use MediaWiki\User\UserFactory; use MediaWiki\User\UserIdentity; /** @@ -44,23 +43,17 @@ class BlockPermissionCheckerFactory { /** @var BlockUtils */ private $blockUtils; - /** @var UserFactory */ - private $userFactory; - /** * @param ServiceOptions $options * @param BlockUtils $blockUtils - * @param UserFactory $userFactory */ public function __construct( ServiceOptions $options, - BlockUtils $blockUtils, - UserFactory $userFactory + BlockUtils $blockUtils ) { $options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS ); $this->options = $options; $this->blockUtils = $blockUtils; - $this->userFactory = $userFactory; } /** @@ -76,7 +69,6 @@ class BlockPermissionCheckerFactory { return new BlockPermissionChecker( $this->options, $this->blockUtils, - $this->userFactory, $target, $performer ); diff --git a/includes/block/DatabaseBlockStore.php b/includes/block/DatabaseBlockStore.php index 043e7d4da98..7aa4c3adea0 100644 --- a/includes/block/DatabaseBlockStore.php +++ b/includes/block/DatabaseBlockStore.php @@ -517,10 +517,12 @@ class DatabaseBlockStore { 'LIMIT' => 1, ]; - $actor = $this->actorStoreFactory->getActorNormalization()->findActorId( - $block->getTargetUserIdentity(), + $targetUser = $block->getTargetUserIdentity(); + $actor = $targetUser ? $this->actorStoreFactory->getActorNormalization()->findActorId( + $targetUser, $dbr - ); + ) : null; + if ( !$actor ) { $this->logger->debug( 'No actor found to retroactively autoblock' ); return []; diff --git a/includes/installer/WebInstallerOptions.php b/includes/installer/WebInstallerOptions.php index dde2531761b..47292845285 100644 --- a/includes/installer/WebInstallerOptions.php +++ b/includes/installer/WebInstallerOptions.php @@ -114,7 +114,6 @@ class WebInstallerOptions extends WebInstallerPage { $chosenSkinName = $this->getVar( 'wgDefaultSkin', $this->parent->getDefaultSkin( $skinNames ) ); if ( $skins ) { - // @phan-suppress-next-line SecurityCheck-DoubleEscaped skin names are valid $radioButtons = $this->parent->getRadioElements( [ 'var' => 'wgDefaultSkin', 'itemLabels' => array_fill_keys( $skinNames, 'config-skins-use-as-default' ), @@ -124,10 +123,8 @@ class WebInstallerOptions extends WebInstallerPage { foreach ( $skins as $skin => $info ) { if ( isset( $info['screenshots'] ) ) { - // @phan-suppress-next-line SecurityCheck-DoubleEscaped skin names are valid $screenshotText = $this->makeScreenshotsLink( $skin, $info['screenshots'] ); } else { - // @phan-suppress-next-line SecurityCheck-DoubleEscaped False positive $screenshotText = htmlspecialchars( $skin ); } $skinHtml .= @@ -179,7 +176,6 @@ class WebInstallerOptions extends WebInstallerPage { foreach ( $extByType[$type] as $ext => $info ) { $urlText = ''; if ( isset( $info['url'] ) ) { - // @phan-suppress-next-line SecurityCheck-DoubleEscaped False positive $urlText = ' ' . Html::element( 'a', [ 'href' => $info['url'] ], '(more information)' ); } $attribs = [ @@ -202,7 +198,6 @@ class WebInstallerOptions extends WebInstallerPage { // extension/skin that is required if ( isset( $dependencyMap[$ext]['extensions'] ) ) { foreach ( $dependencyMap[$ext]['extensions'] as $name ) { - // @phan-suppress-next-line SecurityCheck-DoubleEscaped False positive $links[] = Html::element( 'a', [ 'href' => "#config_ext-$name" ], @@ -212,7 +207,6 @@ class WebInstallerOptions extends WebInstallerPage { } if ( isset( $dependencyMap[$ext]['skins'] ) ) { foreach ( $dependencyMap[$ext]['skins'] as $name ) { - // @phan-suppress-next-line SecurityCheck-DoubleEscaped False positive $links[] = Html::element( 'a', [ 'href' => "#config_skin-$name" ], @@ -243,7 +237,6 @@ class WebInstallerOptions extends WebInstallerPage { $this->addHTML( $extHtml ); // Push the dependency map to the client side $this->addHTML( Html::inlineScript( - // @phan-suppress-next-line SecurityCheck-DoubleEscaped False positive 'var extDependencyMap = ' . Xml::encodeJsVar( $dependencyMap ) ) ); } diff --git a/includes/user/User.php b/includes/user/User.php index 011777c147f..92d5691ad98 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -23,6 +23,7 @@ use MediaWiki\Auth\AuthenticationRequest; use MediaWiki\Auth\AuthManager; use MediaWiki\Block\AbstractBlock; +use MediaWiki\Block\Block; use MediaWiki\Block\DatabaseBlock; use MediaWiki\Block\SystemBlock; use MediaWiki\DAO\WikiAwareEntityTrait; @@ -64,10 +65,22 @@ use Wikimedia\ScopedCallback; * * @newable in 1.35 only, the constructor is @internal since 1.36 */ -class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact { +class User implements Authority, UserIdentity, UserEmailContact { use ProtectedHookAccessorTrait; use WikiAwareEntityTrait; + /** + * @var int + * @see IDBAccessObject::READ_EXCLUSIVE + */ + public const READ_EXCLUSIVE = IDBAccessObject::READ_EXCLUSIVE; + + /** + * @var int + * @see IDBAccessObject::READ_LOCKING + */ + public const READ_LOCKING = IDBAccessObject::READ_LOCKING; + /** * Number of characters required for the user_token field. */ @@ -1897,26 +1910,40 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact * Check if user is blocked * * @deprecated since 1.34, use User::getBlock() or - * PermissionManager::isBlockedFrom() or - * PermissionManager::userCan() instead. + * Authority:getBlock() or Authority:definitlyCan() or + * Authority:authorizeRead() or Authority:authorizeWrite() or + * PermissionManager::isBlockedFrom(), as appropriate. * * @param bool $fromReplica Whether to check the replica DB instead of * the master. Hacked from false due to horrible probs on site. * @return bool True if blocked, false otherwise */ public function isBlocked( $fromReplica = true ) { - return $this->getBlock( $fromReplica ) instanceof AbstractBlock; + return $this->getBlock( $fromReplica ) !== null; } /** * Get the block affecting the user, or null if the user is not blocked * - * @param bool $fromReplica Whether to check the replica DB instead of the master + * @param int|bool $freshness One of the Authority::READ_XXX constants. + * For backwards compatibility, a boolean is also accepted, + * with true meaning READ_NORMAL and false meaning + * READ_LATEST. * @param bool $disableIpBlockExemptChecking This is used internally to prevent * a infinite recursion with autopromote. See T270145. - * @return AbstractBlock|null + * + * @return ?AbstractBlock */ - public function getBlock( $fromReplica = true, $disableIpBlockExemptChecking = false ) { + public function getBlock( + $freshness = self::READ_NORMAL, + $disableIpBlockExemptChecking = false + ): ?Block { + if ( is_bool( $freshness ) ) { + $fromReplica = $freshness; + } else { + $fromReplica = ( $freshness !== self::READ_LATEST ); + } + $this->getBlockedStatus( $fromReplica, $disableIpBlockExemptChecking ); return $this->mBlock instanceof AbstractBlock ? $this->mBlock : null; } @@ -3487,7 +3514,7 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact [ 'LOCK IN SHARE MODE' ] ); $loaded = false; - if ( $this->mId && $this->loadFromDatabase( self::READ_LOCKING ) ) { + if ( $this->mId && $this->loadFromDatabase( IDBAccessObject::READ_LOCKING ) ) { $loaded = true; } if ( !$loaded ) { @@ -4198,7 +4225,7 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact } $user = self::newFromId( $this->getId() ); - if ( !$user->loadFromId( self::READ_EXCLUSIVE ) ) { + if ( !$user->loadFromId( IDBAccessObject::READ_EXCLUSIVE ) ) { return null; } diff --git a/includes/user/UserGroupManager.php b/includes/user/UserGroupManager.php index bcc27336232..9773f2e663c 100644 --- a/includes/user/UserGroupManager.php +++ b/includes/user/UserGroupManager.php @@ -554,7 +554,7 @@ class UserGroupManager implements IDBAccessObject { // we stop checking for ipblock-exempt via here. We do this by setting the second // param to true. // See T270145. - $block = $user->getBlock( false, true ); + $block = $user->getBlock( Authority::READ_LATEST, true ); return $block && $block->isSitewide(); case APCOND_ISBOT: return in_array( 'bot', $this->groupPermissionsLookup diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php index 033faf98df6..31f6309dc34 100644 --- a/tests/phpunit/includes/user/UserTest.php +++ b/tests/phpunit/includes/user/UserTest.php @@ -1363,19 +1363,21 @@ class UserTest extends MediaWikiIntegrationTestCase { * @covers User::getBlockedStatus */ public function testCompositeBlocks() { + $this->tablesUsed[] = 'ipblocks'; + $user = $this->getMutableTestUser()->getUser(); $request = $user->getRequest(); $this->setSessionUser( $user, $request ); $blockStore = MediaWikiServices::getInstance()->getDatabaseBlockStore(); - $ipBlock = new Block( [ + $ipBlock = new DatabaseBlock( [ 'address' => $user->getRequest()->getIP(), 'by' => $this->getTestSysop()->getUser(), 'createAccount' => true, ] ); $blockStore->insertBlock( $ipBlock ); - $userBlock = new Block( [ + $userBlock = new DatabaseBlock( [ 'address' => $user, 'by' => $this->getTestSysop()->getUser(), 'createAccount' => false, @@ -1389,6 +1391,30 @@ class UserTest extends MediaWikiIntegrationTestCase { $this->assertTrue( $block->appliesToNamespace( NS_MAIN ) ); } + /** + * @covers User::getBlock + */ + public function testUserBlock() { + $this->tablesUsed[] = 'ipblocks'; + + $user = $this->getMutableTestUser()->getUser(); + $request = $user->getRequest(); + $this->setSessionUser( $user, $request ); + + $blockStore = MediaWikiServices::getInstance()->getDatabaseBlockStore(); + $ipBlock = new DatabaseBlock( [ + 'address' => $user, + 'by' => $this->getTestSysop()->getUser(), + 'createAccount' => true, + ] ); + $blockStore->insertBlock( $ipBlock ); + + $block = $user->getBlock(); + $this->assertNotNull( $block, 'getuserBlock' ); + $this->assertNotNull( $block->getTargetUserIdentity(), 'getTargetUserIdentity()' ); + $this->assertSame( $user->getName(), $block->getTargetUserIdentity()->getName() ); + } + /** * @covers User::isBlockedFrom * @dataProvider provideIsBlockedFrom @@ -1404,7 +1430,7 @@ class UserTest extends MediaWikiIntegrationTestCase { 'wgBlockAllowsUTEdit' => $options['blockAllowsUTEdit'] ?? true, ] ); - $user = $this->user; + $user = $this->getMutableTestUser()->getUser(); if ( $title === self::USER_TALK_PAGE ) { $title = $user->getTalkPage(); @@ -1528,7 +1554,7 @@ class UserTest extends MediaWikiIntegrationTestCase { * @param bool $blockFromAccountCreation Whether to block account creation. */ public function testIsBlockedFromAction( $blockFromEmail, $blockFromAccountCreation ) { - $user = $this->getTestUser( 'accountcreator' )->getUser(); + $user = $this->getMutableTestUser( 'accountcreator' )->getUser(); $block = new DatabaseBlock( [ 'expiry' => wfTimestamp( TS_MW, wfTimestamp() + ( 40 * 60 * 60 ) ), @@ -1565,17 +1591,19 @@ class UserTest extends MediaWikiIntegrationTestCase { * @param bool $expected Whether the user is expected to be blocked from uploads. */ public function testIsBlockedFromUpload( $sitewide, $expected ) { + $user = $this->getMutableTestUser()->getUser(); + $block = new DatabaseBlock( [ 'expiry' => wfTimestamp( TS_MW, wfTimestamp() + ( 40 * 60 * 60 ) ), 'sitewide' => $sitewide, ] ); - $block->setTarget( $this->user ); + $block->setTarget( $user ); $block->setBlocker( $this->getTestSysop()->getUser() ); $blockStore = MediaWikiServices::getInstance()->getDatabaseBlockStore(); $blockStore->insertBlock( $block ); try { - $this->assertSame( $expected, $this->user->isBlockedFromUpload() ); + $this->assertSame( $expected, $user->isBlockedFromUpload() ); } finally { $blockStore->deleteBlock( $block ); } diff --git a/tests/phpunit/mocks/permissions/MockAuthorityTrait.php b/tests/phpunit/mocks/permissions/MockAuthorityTrait.php index e20ce44845e..91ddb8a9a8d 100644 --- a/tests/phpunit/mocks/permissions/MockAuthorityTrait.php +++ b/tests/phpunit/mocks/permissions/MockAuthorityTrait.php @@ -2,6 +2,7 @@ namespace MediaWiki\Tests\Unit\Permissions; +use MediaWiki\Block\Block; use MediaWiki\Permissions\Authority; use MediaWiki\Permissions\SimpleAuthority; use MediaWiki\Permissions\UltimateAuthority; @@ -84,6 +85,29 @@ trait MockAuthorityTrait { return new SimpleAuthority( $user, $permissions ); } + /** + * Create a mock Authority for $user with $block and $permissions. + * + * @param UserIdentity $user + * @param Block $block + * @param array $permissions + * + * @return Authority + */ + private function mockUserAuthorityWithBlock( + UserIdentity $user, + Block $block, + array $permissions = [] + ): Authority { + return $this->mockAuthority( + $user, + static function ( $permission ) use ( $permissions ) { + return in_array( $permission, $permissions ); + }, + $block + ); + } + /** * Create a mock Authority for an anon user with all but $permissions * @param array $permissions @@ -157,9 +181,15 @@ trait MockAuthorityTrait { * * @param UserIdentity $user * @param callable $permissionCallback ( string $permission, ?PageIdentity $page ) + * @param Block|null $block + * * @return Authority */ - private function mockAuthority( UserIdentity $user, callable $permissionCallback ): Authority { + private function mockAuthority( + UserIdentity $user, + callable $permissionCallback, + Block $block = null + ): Authority { $mock = $this->createMock( Authority::class ); $mock->method( 'getUser' )->willReturn( $user ); $methods = [ 'isAllowed', 'probablyCan', 'definitelyCan', 'authorizeRead', 'authorizeWrite' ]; @@ -184,6 +214,7 @@ trait MockAuthorityTrait { } return true; } ); + $mock->method( 'getBlock' )->willReturn( $block ); return $mock; } } diff --git a/tests/phpunit/unit/includes/Permissions/PermissionStatusTest.php b/tests/phpunit/unit/includes/Permissions/PermissionStatusTest.php index eb595979f7c..40f0b1174e5 100644 --- a/tests/phpunit/unit/includes/Permissions/PermissionStatusTest.php +++ b/tests/phpunit/unit/includes/Permissions/PermissionStatusTest.php @@ -20,6 +20,7 @@ namespace MediaWiki\Tests\Unit\Permissions; +use MediaWiki\Block\Block; use MediaWiki\Permissions\PermissionStatus; use MediaWikiUnitTestCase; @@ -36,4 +37,15 @@ class PermissionStatusTest extends MediaWikiUnitTestCase { $this->assertEmpty( $status->getErrors() ); } + public function testBlock() { + $status = PermissionStatus::newEmpty(); + + $this->assertNull( $status->getBlock() ); + + $block = $this->createMock( Block::class ); + $status->setBlock( $block ); + + $this->assertSame( $block, $status->getBlock() ); + } + } diff --git a/tests/phpunit/unit/includes/Permissions/SimpleAuthorityTest.php b/tests/phpunit/unit/includes/Permissions/SimpleAuthorityTest.php index ebeae5e0dbf..bd45b83be6f 100644 --- a/tests/phpunit/unit/includes/Permissions/SimpleAuthorityTest.php +++ b/tests/phpunit/unit/includes/Permissions/SimpleAuthorityTest.php @@ -139,4 +139,11 @@ class SimpleAuthorityTest extends MediaWikiUnitTestCase { $authority->isAllowedAll(); } + public function testGetBlock() { + $actor = new UserIdentityValue( 12, 'Test' ); + $authority = new SimpleAuthority( $actor, [] ); + + $this->assertNull( $authority->getBlock() ); + } + } diff --git a/tests/phpunit/unit/includes/Permissions/UltimateAuthorityTest.php b/tests/phpunit/unit/includes/Permissions/UltimateAuthorityTest.php index 0765282e745..7973f0cd5d7 100644 --- a/tests/phpunit/unit/includes/Permissions/UltimateAuthorityTest.php +++ b/tests/phpunit/unit/includes/Permissions/UltimateAuthorityTest.php @@ -119,4 +119,11 @@ class UltimateAuthorityTest extends MediaWikiUnitTestCase { $authority->isAllowedAll(); } + public function testGetBlock() { + $actor = new UserIdentityValue( 12, 'Test' ); + $authority = new UltimateAuthority( $actor ); + + $this->assertNull( $authority->getBlock() ); + } + } diff --git a/tests/phpunit/unit/includes/Permissions/UserAuthorityTest.php b/tests/phpunit/unit/includes/Permissions/UserAuthorityTest.php index d6ab3b40652..9d84e4e22f1 100644 --- a/tests/phpunit/unit/includes/Permissions/UserAuthorityTest.php +++ b/tests/phpunit/unit/includes/Permissions/UserAuthorityTest.php @@ -21,12 +21,15 @@ namespace MediaWiki\Tests\Unit\Permissions; use InvalidArgumentException; +use MediaWiki\Block\Block; use MediaWiki\Page\PageIdentity; use MediaWiki\Page\PageIdentityValue; +use MediaWiki\Permissions\Authority; use MediaWiki\Permissions\PermissionManager; use MediaWiki\Permissions\PermissionStatus; use MediaWiki\Permissions\UserAuthority; use MediaWikiUnitTestCase; +use PHPUnit\Framework\MockObject\MockObject; use User; /** @@ -36,13 +39,13 @@ class UserAuthorityTest extends MediaWikiUnitTestCase { /** * @param string[] $permissions - * @param User|null $actor The user to use, null to create a new mock - * @return UserAuthority + * @return PermissionManager */ - private function newAuthority( array $permissions, User $actor = null ) : UserAuthority { + private function newPermissionsManager( array $permissions ) : PermissionManager { + /** @var PermissionManager|MockObject $permissionManager */ $permissionManager = $this->createNoOpMock( PermissionManager::class, - [ 'userHasRight', 'userCan', 'getPermissionErrors' ] + [ 'userHasRight', 'userCan', 'getPermissionErrors', 'isBlockedFrom' ] ); $permissionManager->method( 'userHasRight' )->willReturnCallback( @@ -64,21 +67,97 @@ class UserAuthorityTest extends MediaWikiUnitTestCase { $errors[] = [ 'permissionserrors' ]; } + if ( $user->getBlock() && $permission !== 'read' ) { + $errors[] = [ 'blockedtext-partial' ]; + } + return $errors; } ); + $permissionManager->method( 'isBlockedFrom' )->willReturnCallback( + static function ( User $user, $page ) { + return $page->getDBkey() === 'Forbidden'; + } + ); + + return $permissionManager; + } + + private function newUser(): User { + /** @var User|MockObject $actor */ + $actor = $this->createNoOpMock( User::class, [ 'getBlock' ] ); + $actor->method( 'getBlock' )->willReturn( null ); + return $actor; + } + + private function newAuthority( array $permissions, User $actor = null ): Authority { + /** @var UserAuthority|MockObject $permissionManager */ + $permissionManager = $this->newPermissionsManager( $permissions ); return new UserAuthority( - $actor ?? $this->createNoOpMock( User::class ), + $actor ?? $this->newUser(), $permissionManager ); } - public function testGetAuthor() { - $actor = $this->createNoOpMock( User::class ); - $authority = $this->newAuthority( [], $actor ); + public function testGetUser() { + $user = $this->newUser(); + $authority = $this->newAuthority( [], $user ); - $this->assertSame( $actor, $authority->getUser() ); + $this->assertSame( $user, $authority->getUser() ); + } + + public function testGetUserBlockNotBlocked() { + $authority = $this->newAuthority( [] ); + $this->assertNull( $authority->getBlock() ); + } + + /** + * @param Block|null $block + * + * @return MockObject|User + */ + private function getBlockedUser( $block = null ) { + $block = $block ?: $this->createNoOpMock( Block::class ); + + $user = $this->createNoOpMock( User::class, [ 'getBlock' ] ); + $user->method( 'getBlock' ) + ->willReturn( $block ); + + return $user; + } + + public function testGetUserBlockWasBlocked() { + $block = $this->createNoOpMock( Block::class ); + $user = $this->getBlockedUser( $block ); + + $authority = $this->newAuthority( [], $user ); + $this->assertSame( $block, $authority->getBlock() ); + } + + public function testBlockedUserCanRead() { + $block = $this->createNoOpMock( Block::class ); + $user = $this->getBlockedUser( $block ); + + $authority = $this->newAuthority( [ 'read', 'edit' ], $user ); + + $status = PermissionStatus::newEmpty(); + $target = new PageIdentityValue( 321, NS_MAIN, __METHOD__, PageIdentity::LOCAL ); + $this->assertTrue( $authority->authorizeRead( 'read', $target, $status ) ); + $this->assertTrue( $status->isOK() ); + } + + public function testBlockedUserCanNotWrite() { + $block = $this->createNoOpMock( Block::class ); + $user = $this->getBlockedUser( $block ); + + $authority = $this->newAuthority( [ 'read', 'edit' ], $user ); + + $status = PermissionStatus::newEmpty(); + $target = new PageIdentityValue( 321, NS_MAIN, __METHOD__, PageIdentity::LOCAL ); + $this->assertFalse( $authority->authorizeRead( 'edit', $target, $status ) ); + $this->assertFalse( $status->isOK() ); + $this->assertSame( $block, $status->getBlock() ); } public function testPermissions() { @@ -173,4 +252,20 @@ class UserAuthorityTest extends MediaWikiUnitTestCase { $authority->isAllowedAll(); } + public function testGetBlock_none() { + $actor = $this->newUser(); + + $authority = $this->newAuthority( [ 'foo', 'bar' ], $actor ); + + $this->assertNull( $authority->getBlock() ); + } + + public function testGetBlock_blocked() { + $block = $this->createNoOpMock( Block::class ); + $actor = $this->getBlockedUser( $block ); + + $authority = $this->newAuthority( [ 'foo', 'bar' ], $actor ); + + $this->assertSame( $block, $authority->getBlock() ); + } } diff --git a/tests/phpunit/unit/includes/block/BlockPermissionCheckerTest.php b/tests/phpunit/unit/includes/block/BlockPermissionCheckerTest.php index a5ec1b4901e..d8dd98b5f82 100644 --- a/tests/phpunit/unit/includes/block/BlockPermissionCheckerTest.php +++ b/tests/phpunit/unit/includes/block/BlockPermissionCheckerTest.php @@ -5,8 +5,8 @@ use MediaWiki\Block\BlockPermissionChecker; use MediaWiki\Block\BlockUtils; use MediaWiki\Block\DatabaseBlock; use MediaWiki\Config\ServiceOptions; +use MediaWiki\Permissions\Authority; use MediaWiki\Tests\Unit\Permissions\MockAuthorityTrait; -use MediaWiki\User\UserFactory; use MediaWiki\User\UserIdentity; use MediaWiki\User\UserIdentityValue; use PHPUnit\Framework\MockObject\MockObject; @@ -22,30 +22,19 @@ class BlockPermissionCheckerTest extends MediaWikiUnitTestCase { /** * @param bool $enableUserEmail * @param array $targetAndType - * @param UserIdentity|MockObject $user - * @param string[] $rights + * @param Authority $performer * @return BlockPermissionChecker */ private function getBlockPermissionChecker( bool $enableUserEmail, array $targetAndType, - $user, - array $rights + Authority $performer ) { $options = new ServiceOptions( BlockPermissionChecker::CONSTRUCTOR_OPTIONS, [ 'EnableUserEmail' => $enableUserEmail ] ); - // Make things simple: in the test cases when the UserFactory is needed, - // we expect to have the mock authority objects be based on mocks of full - // User objects instead of just UserIdentity, so performer->getUser() is a - // full user object, and have the factory just return that object - $userFactory = $this->createMock( UserFactory::class ); - $userFactory->method( 'newFromUserIdentity' ) - ->with( $this->isInstanceOf( User::class ) ) - ->will( $this->returnArgument( 0 ) ); - // We don't care about how BlockUtils::parseBlockTarget actually works, just // override with whatever. Only used for a single call in the constructor // for getting target and type @@ -54,12 +43,9 @@ class BlockPermissionCheckerTest extends MediaWikiUnitTestCase { ->method( 'parseBlockTarget' ) ->willReturn( $targetAndType ); - $performer = $this->mockUserAuthorityWithPermissions( $user, $rights ); - return new BlockPermissionChecker( $options, $blockUtils, - $userFactory, 'foo', // input to BlockUtils::parseBlockTarget, not used $performer ); @@ -94,12 +80,11 @@ class BlockPermissionCheckerTest extends MediaWikiUnitTestCase { * @dataProvider provideCheckBasePermissions */ public function testCheckBasePermissions( $rights, $checkHideuser, $expect ) { - $user = new UserIdentityValue( 4, 'admin' ); + $performer = $this->mockRegisteredAuthorityWithPermissions( $rights ); $blockPermissionChecker = $this->getBlockPermissionChecker( true, // $enableUserEmail, irrelevant [ null, null ], // $targetAndType, irrelevant - $user, - $rights + $performer ); $this->assertSame( $expect, @@ -112,14 +97,10 @@ class BlockPermissionCheckerTest extends MediaWikiUnitTestCase { */ public function testNotBlockedPerformer() { // checkBlockPermissions has an early return true if the performer has no block - $user = $this->createMock( User::class ); - $user->method( 'getBlock' )->willReturn( null ); - $blockPermissionChecker = $this->getBlockPermissionChecker( true, // $enableUserEmail, irrelevant [ null, null ], // $targetAndType, irrelevant - $user, - [] // $rights, irrelevant + $this->mockRegisteredAuthorityWithoutPermissions( [] ) ); $this->assertTrue( $blockPermissionChecker->checkBlockPermissions() @@ -131,16 +112,13 @@ class BlockPermissionCheckerTest extends MediaWikiUnitTestCase { */ public function testPartialBlockedPerformer() { // checkBlockPermissions has an early return true if the performer is not sitewide blocked - $user = $this->createMock( User::class ); - $user->method( 'getBlock' )->willReturn( - $this->getBlock( false, '' ) - ); + $blocker = new UserIdentityValue( 1, 'blocker', UserIdentity::LOCAL ); + $performer = $this->mockUserAuthorityWithBlock( $blocker, $this->getBlock( false, '' ) ); $blockPermissionChecker = $this->getBlockPermissionChecker( true, // $enableUserEmail, irrelevant [ null, null ], // $targetAndType, irrelevant - $user, - [] // $rights, irrelevant + $performer ); $this->assertTrue( $blockPermissionChecker->checkBlockPermissions() @@ -177,21 +155,17 @@ class BlockPermissionCheckerTest extends MediaWikiUnitTestCase { true, // sitewide $blockedBy ); - $user = $this->createMock( User::class ); - $user->method( 'getBlock' )->willReturn( $block ); - $user->method( 'getId' )->willReturn( 1 ); - $user->method( 'getName' )->willReturn( 'blocker' ); - $user->method( 'equals' )->willReturnCallback( static function ( $user ) { - return $user->getName() === 'blocker'; - } ); + $rights = $unblockSelf ? [ 'unblockself' ] : []; + + $blocker = new UserIdentityValue( 1, 'blocker', UserIdentity::LOCAL ); + $performer = $this->mockUserAuthorityWithBlock( $blocker, $block, $rights ); $target = new UserIdentityValue( $targetUserId, $targetUserName ); $blockPermissionChecker = $this->getBlockPermissionChecker( true, // $enableUserEmail, irrelevant [ $target, AbstractBlock::TYPE_USER ], // $targetAndType, irrelevant - $user, - $unblockSelf ? [ 'unblockself' ] : [] + $performer ); $this->assertSame( $expect, @@ -212,12 +186,11 @@ class BlockPermissionCheckerTest extends MediaWikiUnitTestCase { * @dataProvider provideCheckEmailPermissions */ public function testCheckEmailPermissionOkay( $enableEmail, $rights, $expect ) { - $user = new UserIdentityValue( 4, 'admin' ); + $performer = $this->mockRegisteredAuthorityWithPermissions( $rights ); $blockPermissionChecker = $this->getBlockPermissionChecker( $enableEmail, [ null, null ], // $targetAndType, irrelevant - $user, - $rights + $performer ); $this->assertSame( $expect, $blockPermissionChecker->checkEmailPermissions() ); }