From ffeeee69f77f1950a98b950476d11914fd875dc1 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Fri, 13 Sep 2019 13:39:50 -0700 Subject: [PATCH] Make WatchedItemQueryService depend on PermissionManager Bug: T220191 Change-Id: I714d2b33b83dab230a8168765d5523cb14971371 --- includes/ServiceWiring.php | 3 +- .../watcheditem/WatchedItemQueryService.php | 24 +++++--- .../WatchedItemQueryServiceUnitTest.php | 58 ++++++++++--------- 3 files changed, 50 insertions(+), 35 deletions(-) diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 4cd3d85174..8807d0417e 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -784,7 +784,8 @@ return [ $services->getDBLoadBalancer(), $services->getCommentStore(), $services->getActorMigration(), - $services->getWatchedItemStore() + $services->getWatchedItemStore(), + $services->getPermissionManager() ); }, diff --git a/includes/watcheditem/WatchedItemQueryService.php b/includes/watcheditem/WatchedItemQueryService.php index 13f2b52406..d8e49852aa 100644 --- a/includes/watcheditem/WatchedItemQueryService.php +++ b/includes/watcheditem/WatchedItemQueryService.php @@ -1,6 +1,7 @@ loadBalancer = $loadBalancer; $this->commentStore = $commentStore; $this->actorMigration = $actorMigration; $this->watchedItemStore = $watchedItemStore; + $this->permissionManager = $permissionManager; } /** @@ -549,7 +555,7 @@ class WatchedItemQueryService { return $conds; } - private function getUserRelatedConds( IDatabase $db, User $user, array $options ) { + private function getUserRelatedConds( IDatabase $db, UserIdentity $user, array $options ) { if ( !array_key_exists( 'onlyByUser', $options ) && !array_key_exists( 'notByUser', $options ) ) { return []; } @@ -566,9 +572,11 @@ class WatchedItemQueryService { // Avoid brute force searches (T19342) $bitmask = 0; - if ( !$user->isAllowed( 'deletedhistory' ) ) { + if ( !$this->permissionManager->userHasRight( $user, 'deletedhistory' ) ) { $bitmask = RevisionRecord::DELETED_USER; - } elseif ( !$user->isAllowedAny( 'suppressrevision', 'viewsuppressed' ) ) { + } elseif ( !$this->permissionManager + ->userHasAnyRight( $user, 'suppressrevision', 'viewsuppressed' ) + ) { $bitmask = RevisionRecord::DELETED_USER | RevisionRecord::DELETED_RESTRICTED; } if ( $bitmask ) { @@ -578,13 +586,15 @@ class WatchedItemQueryService { return $conds; } - private function getExtraDeletedPageLogEntryRelatedCond( IDatabase $db, User $user ) { + private function getExtraDeletedPageLogEntryRelatedCond( IDatabase $db, UserIdentity $user ) { // LogPage::DELETED_ACTION hides the affected page, too. So hide those // entirely from the watchlist, or someone could guess the title. $bitmask = 0; - if ( !$user->isAllowed( 'deletedhistory' ) ) { + if ( !$this->permissionManager->userHasRight( $user, 'deletedhistory' ) ) { $bitmask = LogPage::DELETED_ACTION; - } elseif ( !$user->isAllowedAny( 'suppressrevision', 'viewsuppressed' ) ) { + } elseif ( !$this->permissionManager + ->userHasAnyRight( $user, 'suppressrevision', 'viewsuppressed' ) + ) { $bitmask = LogPage::DELETED_ACTION | LogPage::DELETED_RESTRICTED; } if ( $bitmask ) { diff --git a/tests/phpunit/includes/watcheditem/WatchedItemQueryServiceUnitTest.php b/tests/phpunit/includes/watcheditem/WatchedItemQueryServiceUnitTest.php index 0b1d0132fb..45fa1430d7 100644 --- a/tests/phpunit/includes/watcheditem/WatchedItemQueryServiceUnitTest.php +++ b/tests/phpunit/includes/watcheditem/WatchedItemQueryServiceUnitTest.php @@ -1,5 +1,6 @@ getMockLoadBalancer( $mockDb ), $this->getMockCommentStore(), $this->getMockActorMigration(), - $this->getMockWatchedItemStore() + $this->getMockWatchedItemStore(), + $mockPM ?: $this->getMockPermissionManager() ); } @@ -153,6 +156,25 @@ class WatchedItemQueryServiceUnitTest extends MediaWikiTestCase { return $mock; } + /** + * @param string $notAllowedAction + * @return PHPUnit_Framework_MockObject_MockObject|PermissionManager + */ + private function getMockPermissionManager( $notAllowedAction = null ) { + $mock = $this->getMockBuilder( PermissionManager::class ) + ->disableOriginalConstructor() + ->getMock(); + $mock->method( 'userHasRight' ) + ->will( $this->returnCallback( function ( $user, $action ) use ( $notAllowedAction ) { + return $action !== $notAllowedAction; + } ) ); + $mock->method( 'userHasAnyRight' ) + ->will( $this->returnCallback( function ( $user, ...$actions ) use ( $notAllowedAction ) { + return !in_array( $notAllowedAction, $actions ); + } ) ); + return $mock; + } + /** * @param int $id * @param string[] $extraMethods Extra methods that are expected might be called @@ -177,9 +199,7 @@ class WatchedItemQueryServiceUnitTest extends MediaWikiTestCase { */ private function getMockUnrestrictedNonAnonUserWithId( $id, array $extraMethods = [] ) { $mock = $this->getMockNonAnonUserWithId( $id, - array_merge( [ 'isAllowed', 'isAllowedAny', 'useRCPatrol' ], $extraMethods ) ); - $mock->method( 'isAllowed' )->willReturn( true ); - $mock->method( 'isAllowedAny' )->willReturn( true ); + array_merge( [ 'useRCPatrol' ], $extraMethods ) ); $mock->method( 'useRCPatrol' )->willReturn( true ); return $mock; } @@ -189,18 +209,9 @@ class WatchedItemQueryServiceUnitTest extends MediaWikiTestCase { * @param string $notAllowedAction * @return PHPUnit_Framework_MockObject_MockObject|User */ - private function getMockNonAnonUserWithIdAndRestrictedPermissions( $id, $notAllowedAction ) { + private function getMockNonAnonUserWithIdAndRestrictedPermissions( $id ) { $mock = $this->getMockNonAnonUserWithId( $id, - [ 'isAllowed', 'isAllowedAny', 'useRCPatrol', 'useNPPatrol' ] ); - - $mock->method( 'isAllowed' ) - ->will( $this->returnCallback( function ( $action ) use ( $notAllowedAction ) { - return $action !== $notAllowedAction; - } ) ); - $mock->method( 'isAllowedAny' ) - ->will( $this->returnCallback( function ( ...$actions ) use ( $notAllowedAction ) { - return !in_array( $notAllowedAction, $actions ); - } ) ); + [ 'useRCPatrol', 'useNPPatrol' ] ); $mock->method( 'useRCPatrol' )->willReturn( false ); $mock->method( 'useNPPatrol' )->willReturn( false ); @@ -213,15 +224,7 @@ class WatchedItemQueryServiceUnitTest extends MediaWikiTestCase { */ private function getMockNonAnonUserWithIdAndNoPatrolRights( $id ) { $mock = $this->getMockNonAnonUserWithId( $id, - [ 'isAllowed', 'isAllowedAny', 'useRCPatrol', 'useNPPatrol' ] ); - - $mock->expects( $this->any() ) - ->method( 'isAllowed' ) - ->will( $this->returnValue( true ) ); - $mock->expects( $this->any() ) - ->method( 'isAllowedAny' ) - ->will( $this->returnValue( true ) ); - + [ 'useRCPatrol', 'useNPPatrol' ] ); $mock->expects( $this->any() ) ->method( 'useRCPatrol' ) ->will( $this->returnValue( false ) ); @@ -1132,9 +1135,10 @@ class WatchedItemQueryServiceUnitTest extends MediaWikiTestCase { ) ->will( $this->returnValue( [] ) ); - $user = $this->getMockNonAnonUserWithIdAndRestrictedPermissions( 1, $notAllowedAction ); + $permissionManager = $this->getMockPermissionManager( $notAllowedAction ); + $user = $this->getMockNonAnonUserWithIdAndRestrictedPermissions( 1 ); - $queryService = $this->newService( $mockDb ); + $queryService = $this->newService( $mockDb, $permissionManager ); $items = $queryService->getWatchedItemsWithRecentChangeInfo( $user, $options ); $this->assertEmpty( $items ); -- 2.20.1