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() ); }