From b684db06d43fcb53c0a1d0bb76fcce9988ab13fb Mon Sep 17 00:00:00 2001 From: Dreamy Jazz Date: Wed, 24 Jan 2024 18:40:20 +0000 Subject: [PATCH] [SECURITY] Check for deleted status in temp account REST APIs Why: * The temporary account reveal REST APIs provided by CheckUser do not inspect the deleted status of the performer of edits and log actions, as well as not seeing if the user is blocked with hideuser enabled. * In these cases CheckUser should hide the IP information when the user making the request does not have the ability to see the performer of the edit/log action. What: * Check for a block on the temporary account specified in the request, and if a block exists with 'hideuser' set then pretend that the user does not exist if the user making the request does not have the right to see suppressed information. * Filter out revision IDs in TemporaryAccountRevisionHandler that have the performer hidden from the user making the REST API request. * Filter out log IDs in TemporaryAccountLogHandler that have the performer hidden from the user making the REST API request. * Add and update tests for this new code. Bug: T355434 Change-Id: I4f786711ea9f36f5a64e11c44966455e5203f683 --- extension.json | 10 +++- .../AbstractTemporaryAccountHandler.php | 21 ++++++- .../Handler/TemporaryAccountLogHandler.php | 23 ++++++++ .../TemporaryAccountRevisionHandler.php | 55 +++++++++++++++++++ .../Handler/TemporaryAccountHandlerTest.php | 50 +++++++++++++++++ .../TemporaryAccountLogHandlerTest.php | 52 ++++++++++++++++++ .../TemporaryAccountRevisionHandlerTest.php | 35 ++++++++++++ 7 files changed, 242 insertions(+), 4 deletions(-) diff --git a/extension.json b/extension.json index bb8b0984..78494f33 100644 --- a/extension.json +++ b/extension.json @@ -203,7 +203,8 @@ "UserOptionsLookup", "UserNameUtils", "DBLoadBalancerFactory", - "ActorStore" + "ActorStore", + "BlockManager" ] }, { @@ -217,7 +218,9 @@ "UserOptionsLookup", "UserNameUtils", "DBLoadBalancerFactory", - "ActorStore" + "ActorStore", + "BlockManager", + "RevisionLookup" ] }, { @@ -231,7 +234,8 @@ "UserOptionsLookup", "UserNameUtils", "DBLoadBalancerFactory", - "ActorStore" + "ActorStore", + "BlockManager" ] }, { diff --git a/src/Api/Rest/Handler/AbstractTemporaryAccountHandler.php b/src/Api/Rest/Handler/AbstractTemporaryAccountHandler.php index bb6c3d5d..f79f8bec 100644 --- a/src/Api/Rest/Handler/AbstractTemporaryAccountHandler.php +++ b/src/Api/Rest/Handler/AbstractTemporaryAccountHandler.php @@ -4,6 +4,7 @@ namespace MediaWiki\CheckUser\Api\Rest\Handler; use JobQueueGroup; use JobSpecification; +use MediaWiki\Block\BlockManager; use MediaWiki\Config\Config; use MediaWiki\Permissions\PermissionManager; use MediaWiki\Rest\LocalizedHttpException; @@ -25,6 +26,7 @@ abstract class AbstractTemporaryAccountHandler extends SimpleHandler { protected UserNameUtils $userNameUtils; protected IConnectionProvider $dbProvider; protected ActorStore $actorStore; + protected BlockManager $blockManager; /** * @param Config $config @@ -34,6 +36,7 @@ abstract class AbstractTemporaryAccountHandler extends SimpleHandler { * @param UserNameUtils $userNameUtils * @param IConnectionProvider $dbProvider * @param ActorStore $actorStore + * @param BlockManager $blockManager */ public function __construct( Config $config, @@ -42,7 +45,8 @@ abstract class AbstractTemporaryAccountHandler extends SimpleHandler { UserOptionsLookup $userOptionsLookup, UserNameUtils $userNameUtils, IConnectionProvider $dbProvider, - ActorStore $actorStore + ActorStore $actorStore, + BlockManager $blockManager ) { $this->config = $config; $this->jobQueueGroup = $jobQueueGroup; @@ -51,6 +55,7 @@ abstract class AbstractTemporaryAccountHandler extends SimpleHandler { $this->userNameUtils = $userNameUtils; $this->dbProvider = $dbProvider; $this->actorStore = $actorStore; + $this->blockManager = $blockManager; } /** @@ -103,6 +108,20 @@ abstract class AbstractTemporaryAccountHandler extends SimpleHandler { ); } + $blockOnTempAccount = $this->blockManager->getBlock( $this->actorStore->getUserIdentityByName( $name ), null ); + if ( + $blockOnTempAccount && + $blockOnTempAccount->getHideName() && + !$this->permissionManager->userHasRight( $this->getAuthority()->getUser(), 'viewsuppressed' ) + ) { + // Pretend the username does not exist if the temporary account is hidden and the user does not have the + // rights to see suppressed information. + throw new LocalizedHttpException( + new MessageValue( 'rest-nonexistent-user', [ $name ] ), + 404 + ); + } + $data = $this->getData( $actorId, $dbr ); $this->jobQueueGroup->push( diff --git a/src/Api/Rest/Handler/TemporaryAccountLogHandler.php b/src/Api/Rest/Handler/TemporaryAccountLogHandler.php index 0212a9ce..2f950fc9 100644 --- a/src/Api/Rest/Handler/TemporaryAccountLogHandler.php +++ b/src/Api/Rest/Handler/TemporaryAccountLogHandler.php @@ -2,6 +2,9 @@ namespace MediaWiki\CheckUser\Api\Rest\Handler; +use DatabaseLogEntry; +use LogEventsList; +use LogPage; use MediaWiki\Rest\LocalizedHttpException; use Wikimedia\Message\DataMessageValue; use Wikimedia\Message\MessageValue; @@ -38,6 +41,26 @@ class TemporaryAccountLogHandler extends AbstractTemporaryAccountHandler { ] ); } + + // Filter out logs where the user doesn't have permission to view the performer. + $ids = array_filter( $ids, function ( $id ) { + $logEntry = DatabaseLogEntry::newFromId( $id, $this->dbProvider->getReplicaDatabase() ); + if ( $logEntry ) { + return LogEventsList::userCanBitfield( + $logEntry->getDeleted(), + LogPage::DELETED_USER, + $this->getAuthority() + ); + } + return true; + } ); + + if ( !count( $ids ) ) { + // If all logs were filtered out, return a results list with no IPs + // which is what happens when there is no CU data for the log events. + return [ 'ips' => [] ]; + } + $conds = [ 'cule_actor' => $actorId, 'cule_log_id' => $ids, diff --git a/src/Api/Rest/Handler/TemporaryAccountRevisionHandler.php b/src/Api/Rest/Handler/TemporaryAccountRevisionHandler.php index 679606f5..9287c71d 100644 --- a/src/Api/Rest/Handler/TemporaryAccountRevisionHandler.php +++ b/src/Api/Rest/Handler/TemporaryAccountRevisionHandler.php @@ -2,12 +2,47 @@ namespace MediaWiki\CheckUser\Api\Rest\Handler; +use JobQueueGroup; +use MediaWiki\Block\BlockManager; +use MediaWiki\Config\Config; +use MediaWiki\Permissions\PermissionManager; use MediaWiki\Rest\LocalizedHttpException; +use MediaWiki\Revision\ArchivedRevisionLookup; +use MediaWiki\Revision\RevisionLookup; +use MediaWiki\Revision\RevisionRecord; +use MediaWiki\User\ActorStore; +use MediaWiki\User\Options\UserOptionsLookup; +use MediaWiki\User\UserNameUtils; use Wikimedia\Message\DataMessageValue; use Wikimedia\ParamValidator\ParamValidator; +use Wikimedia\Rdbms\IConnectionProvider; use Wikimedia\Rdbms\IDatabase; class TemporaryAccountRevisionHandler extends AbstractTemporaryAccountHandler { + + private RevisionLookup $revisionLookup; + private ArchivedRevisionLookup $archivedRevisionLookup; + + public function __construct( + Config $config, + JobQueueGroup $jobQueueGroup, + PermissionManager $permissionManager, + UserOptionsLookup $userOptionsLookup, + UserNameUtils $userNameUtils, + IConnectionProvider $dbProvider, + ActorStore $actorStore, + BlockManager $blockManager, + RevisionLookup $revisionLookup, + ArchivedRevisionLookup $archivedRevisionLookup + ) { + parent::__construct( + $config, $jobQueueGroup, $permissionManager, $userOptionsLookup, $userNameUtils, $dbProvider, $actorStore, + $blockManager + ); + $this->revisionLookup = $revisionLookup; + $this->archivedRevisionLookup = $archivedRevisionLookup; + } + /** * @inheritDoc */ @@ -27,6 +62,26 @@ class TemporaryAccountRevisionHandler extends AbstractTemporaryAccountHandler { ] ); } + + // Filter out revisions where the user doesn't have permission to view the performer. + $ids = array_filter( $ids, function ( $id ) { + $revisionRecord = $this->revisionLookup->getRevisionById( $id ); + if ( !$revisionRecord ) { + // If the revision lookup fails, try the archived revision lookup. + $revisionRecord = $this->archivedRevisionLookup->getArchivedRevisionRecord( null, $id ); + } + if ( $revisionRecord ) { + return $revisionRecord->userCan( RevisionRecord::DELETED_USER, $this->getAuthority() ); + } + return true; + } ); + + if ( !count( $ids ) ) { + // If all revisions were filtered out, return a results list with no IPs + // which is what happens when there is no CU data for the revisions. + return [ 'ips' => [] ]; + } + $conds = [ 'cuc_actor' => $actorId, 'cuc_this_oldid' => $ids, diff --git a/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountHandlerTest.php b/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountHandlerTest.php index 2e2c1bdb..a1ab1fa6 100644 --- a/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountHandlerTest.php +++ b/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountHandlerTest.php @@ -3,7 +3,9 @@ namespace MediaWiki\CheckUser\Tests\Integration\Api\Rest\Handler; use JobQueueGroup; +use MediaWiki\Block\AbstractBlock; use MediaWiki\Block\Block; +use MediaWiki\Block\BlockManager; use MediaWiki\CheckUser\Api\Rest\Handler\TemporaryAccountHandler; use MediaWiki\MediaWikiServices; use MediaWiki\Permissions\Authority; @@ -52,6 +54,8 @@ class TemporaryAccountHandlerTest extends MediaWikiIntegrationTestCase { $actorStore = $this->createMock( ActorStore::class ); $actorStore->method( 'findActorIdByName' ) ->willReturn( 1234 ); + $actorStore->method( 'getUserIdentityByName' ) + ->willReturn( new UserIdentityValue( 1234, '*Unregistered 1' ) ); return new TemporaryAccountHandler( ...array_values( array_merge( [ @@ -62,6 +66,7 @@ class TemporaryAccountHandlerTest extends MediaWikiIntegrationTestCase { 'userNameUtils' => $userNameUtils, 'dbProvider' => MediaWikiServices::getInstance()->getDBLoadBalancerFactory(), 'actorStore' => $actorStore, + 'blockManager' => $this->getServiceContainer()->getBlockManager(), ], $options ) ) ); @@ -354,6 +359,51 @@ class TemporaryAccountHandlerTest extends MediaWikiIntegrationTestCase { ); } + public function testExecutePermissionErrorsSuppressedUser() { + $mockBlock = $this->createMock( AbstractBlock::class ); + $mockBlock->method( 'getHideName' ) + ->willReturn( true ); + $mockBlockManager = $this->createMock( BlockManager::class ); + $mockBlockManager->method( 'getBlock' ) + ->willReturn( $mockBlock ); + $mockPermissionManager = $this->createMock( PermissionManager::class ); + $mockPermissionManager->method( 'userHasRight' ) + ->willReturnCallback( static function ( $_, $permission ) { + if ( $permission === 'viewsuppressed' ) { + return false; + } + return true; + } ); + $handler = $this->getTemporaryAccountHandler( [ + 'blockManager' => $mockBlockManager, + 'permissionManager' => $mockPermissionManager, + ] ); + + $authority = $this->createMock( Authority::class ); + $authority->method( 'isNamed' ) + ->willReturn( true ); + + $this->expectExceptionObject( + new LocalizedHttpException( + new MessageValue( + 'rest-nonexistent-user' + ), + 404 + ) + ); + + // Can't use executeHandlerAndGetHttpException, since it doesn't take an Authority + $this->executeHandler( + $handler, + $this->getRequestData( [ 'name' => '*Unregistered 1' ] ), + [], + [], + [], + [], + $authority + ); + } + public function addDBData() { $testData = [ [ diff --git a/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountLogHandlerTest.php b/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountLogHandlerTest.php index eccde41a..7d899b2e 100644 --- a/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountLogHandlerTest.php +++ b/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountLogHandlerTest.php @@ -3,12 +3,17 @@ namespace MediaWiki\CheckUser\Tests\Integration\Api\Rest\Handler; use JobQueueGroup; +use LogPage; +use ManualLogEntry; use MediaWiki\CheckUser\Api\Rest\Handler\TemporaryAccountLogHandler; +use MediaWiki\CheckUser\Hooks; use MediaWiki\MediaWikiServices; use MediaWiki\Permissions\Authority; use MediaWiki\Permissions\PermissionManager; use MediaWiki\Rest\RequestData; use MediaWiki\Tests\Rest\Handler\HandlerTestTrait; +use MediaWiki\Tests\Unit\Permissions\MockAuthorityTrait; +use MediaWiki\Tests\User\TempUser\TempUserTestTrait; use MediaWiki\User\ActorStore; use MediaWiki\User\Options\UserOptionsLookup; use MediaWiki\User\UserIdentityValue; @@ -23,6 +28,8 @@ use Wikimedia\IPUtils; */ class TemporaryAccountLogHandlerTest extends MediaWikiIntegrationTestCase { + use TempUserTestTrait; + use MockAuthorityTrait; use HandlerTestTrait; /** @@ -48,6 +55,8 @@ class TemporaryAccountLogHandlerTest extends MediaWikiIntegrationTestCase { $actorStore = $this->createMock( ActorStore::class ); $actorStore->method( 'findActorIdByName' ) ->willReturn( 1234 ); + $actorStore->method( 'getUserIdentityByName' ) + ->willReturn( new UserIdentityValue( 1234, '*Unregistered 1' ) ); return new TemporaryAccountLogHandler( ...array_values( array_merge( [ @@ -58,6 +67,7 @@ class TemporaryAccountLogHandlerTest extends MediaWikiIntegrationTestCase { 'userNameUtils' => $userNameUtils, 'dbProvider' => MediaWikiServices::getInstance()->getDBLoadBalancerFactory(), 'actorStore' => $actorStore, + 'blockManager' => $this->getServiceContainer()->getBlockManager(), ], $options ) ) ); @@ -161,6 +171,48 @@ class TemporaryAccountLogHandlerTest extends MediaWikiIntegrationTestCase { ); } + public function testWhenLogPerformerIsSuppressed() { + $this->enableAutoCreateTempUser(); + $this->getServiceContainer()->getTempUserCreator()->create( '*Unregistered 1' ); + $this->setMwGlobals( 'wgCheckUserEventTablesMigrationStage', SCHEMA_COMPAT_NEW ); + // Create a log entry for the *Unregistered 1 user which has the performer suppressed + // and publish it so that it gets added to the CheckUser tables. + $logEntry = new ManualLogEntry( 'move', 'move' ); + $logEntry->setPerformer( new UserIdentityValue( 1234, '*Unregistered 1' ) ); + $logEntry->setDeleted( LogPage::DELETED_USER | LogPage::DELETED_RESTRICTED ); + $logEntry->setTarget( $this->getExistingTestPage() ); + $logEntry->setParameters( [ + '4::target' => wfRandomString(), + '5::noredir' => '0' + ] ); + $logId = $logEntry->insert(); + // Send this log to CheckUser, so a row gets inserted. + Hooks::updateCheckUserData( $logEntry->getRecentChange( $logId ) ); + // Set up a mock actor store that gets the real actor ID for the test temp user. + $actorStore = $this->createMock( ActorStore::class ); + $actorStore->method( 'findActorIdByName' ) + ->willReturn( $this->getServiceContainer()->getActorStore() + ->findActorId( new UserIdentityValue( 1234, '*Unregistered 1' ), $this->getDb() ) + ); + $actorStore->method( 'getUserIdentityByName' ) + ->willReturn( new UserIdentityValue( 1234, '*Unregistered 1' ) ); + $data = $this->executeHandlerAndGetBodyData( + $this->getTemporaryAccountLogHandler( [ + 'actorStore' => $actorStore, + ] ), + $this->getRequestData( [ + 'name' => '*Unregistered 1', + 'ids' => $logId, + ] ), + [], + [], + [], + [], + $this->mockRegisteredAuthorityWithPermissions( [ 'checkuser-temporary-account' ] ) + ); + $this->assertArrayEquals( [], $data['ips'] ); + } + public function testErrorOnWrongMigrationStage() { $this->setMwGlobals( 'wgCheckUserEventTablesMigrationStage', SCHEMA_COMPAT_OLD ); $this->expectExceptionCode( 404 ); diff --git a/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountRevisionHandlerTest.php b/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountRevisionHandlerTest.php index a50742d2..4a82b6c0 100644 --- a/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountRevisionHandlerTest.php +++ b/tests/phpunit/integration/Api/Rest/Handler/TemporaryAccountRevisionHandlerTest.php @@ -8,6 +8,8 @@ use MediaWiki\MediaWikiServices; use MediaWiki\Permissions\Authority; use MediaWiki\Permissions\PermissionManager; use MediaWiki\Rest\RequestData; +use MediaWiki\Revision\RevisionLookup; +use MediaWiki\Revision\RevisionRecord; use MediaWiki\Tests\Rest\Handler\HandlerTestTrait; use MediaWiki\User\ActorStore; use MediaWiki\User\Options\UserOptionsLookup; @@ -48,6 +50,8 @@ class TemporaryAccountRevisionHandlerTest extends MediaWikiIntegrationTestCase { $actorStore = $this->createMock( ActorStore::class ); $actorStore->method( 'findActorIdByName' ) ->willReturn( 1234 ); + $actorStore->method( 'getUserIdentityByName' ) + ->willReturn( new UserIdentityValue( 1234, '*Unregistered 1' ) ); return new TemporaryAccountRevisionHandler( ...array_values( array_merge( [ @@ -58,6 +62,9 @@ class TemporaryAccountRevisionHandlerTest extends MediaWikiIntegrationTestCase { 'userNameUtils' => $userNameUtils, 'dbProvider' => MediaWikiServices::getInstance()->getDBLoadBalancerFactory(), 'actorStore' => $actorStore, + 'blockManager' => $this->getServiceContainer()->getBlockManager(), + 'revisionLookup' => $this->getServiceContainer()->getRevisionLookup(), + 'archivedRevisionLookup' => $this->getServiceContainer()->getArchivedRevisionLookup(), ], $options ) ) ); @@ -159,6 +166,34 @@ class TemporaryAccountRevisionHandlerTest extends MediaWikiIntegrationTestCase { ); } + public function testWhenRevisionPerformersAreSuppressed() { + $mockRevision = $this->createMock( RevisionRecord::class ); + $mockRevision->method( 'userCan' ) + ->willReturn( false ); + $mockRevisionLookup = $this->createMock( RevisionLookup::class ); + $mockRevisionLookup->method( 'getRevisionById' ) + ->willReturn( $mockRevision ); + $data = $this->executeHandlerAndGetBodyData( + $this->getTemporaryAccountRevisionHandler( [ + 'revisionLookup' => $mockRevisionLookup + ] ), + $this->getRequestData( [ + 'name' => '*Unregistered 1', + 'ids' => 10, + ] ), + [], + [], + [], + [], + $this->getAuthorityForSuccess() + ); + $this->assertArrayEquals( + [], + $data['ips'], + true + ); + } + public function addDBData() { $testData = [ [ -- 2.34.1