jenkins-bot has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/361889 )
Change subject: Use WikiPageEntityStorePermissionChecker as WikibaseRepo permission checker ...................................................................... Use WikiPageEntityStorePermissionChecker as WikibaseRepo permission checker Instead of the permission checker implemented by EntityContentFactory. Also makes EntityContentFactory no longer an implementation of the EntityPermissionChecker interface (as it is now redundant). Bug: T166586 Change-Id: I9481c95ae0b359564a14bc6411102709970aedbe --- M repo/includes/Content/EntityContentFactory.php M repo/includes/Store/WikiPageEntityStorePermissionChecker.php M repo/includes/WikibaseRepo.php M repo/tests/phpunit/includes/Content/EntityContentFactoryTest.php M repo/tests/phpunit/includes/Specials/SpecialSetSiteLinkTest.php 5 files changed, 26 insertions(+), 229 deletions(-) Approvals: Daniel Kinzler: Looks good to me, approved jenkins-bot: Verified diff --git a/repo/includes/Content/EntityContentFactory.php b/repo/includes/Content/EntityContentFactory.php index 30de48d..2f6ef05 100644 --- a/repo/includes/Content/EntityContentFactory.php +++ b/repo/includes/Content/EntityContentFactory.php @@ -17,7 +17,6 @@ use Wikibase\EntityContent; use Wikibase\Repo\Store\EntityTitleStoreLookup; use Wikibase\Lib\Store\StorageException; -use Wikibase\Repo\Store\EntityPermissionChecker; use Wikibase\Store\EntityIdLookup; use Wikimedia\Assert\Assert; @@ -29,7 +28,7 @@ * @author Daniel Kinzler * @author Bene* < [email protected] > */ -class EntityContentFactory implements EntityTitleStoreLookup, EntityIdLookup, EntityPermissionChecker { +class EntityContentFactory implements EntityTitleStoreLookup, EntityIdLookup { /** * @var string[] Entity type ID to content model ID mapping. @@ -278,113 +277,6 @@ public function newFromRedirect( EntityRedirect $redirect ) { $handler = $this->getContentHandlerForType( $redirect->getEntityId()->getEntityType() ); return $handler->makeEntityRedirectContent( $redirect ); - } - - /** - * @param User $user - * @param string $permission - * @param Title $entityPage - * @param string $quick - * - * //XXX: would be nice to be able to pass the $short flag too, - * as used by getUserPermissionsErrorsInternal. But Title doesn't expose that. - * @todo Move to a separate service (merge into WikiPageEntityStore?) - * - * @return Status a status object representing the check's result. - */ - protected function getPermissionStatus( User $user, $permission, Title $entityPage, $quick = '' ) { - $errors = $entityPage->getUserPermissionsErrors( $permission, $user, $quick !== 'quick' ); - return $this->getStatusForPermissionErrors( $errors ); - } - - /** - * @param string[] $errors - * - * @return Status - */ - protected function getStatusForPermissionErrors( array $errors ) { - $status = Status::newGood(); - - foreach ( $errors as $error ) { - call_user_func_array( [ $status, 'fatal' ], $error ); - $status->setResult( false ); - } - - return $status; - } - - /** - * @see EntityPermissionChecker::getPermissionStatusForEntityId - * - * @param User $user - * @param string $permission - * @param EntityId $entityId - * @param string $quick - * - * @return Status a status object representing the check's result. - * - * @todo Move to a separate service (merge into WikiPageEntityStore?) - */ - public function getPermissionStatusForEntityId( User $user, $permission, EntityId $entityId, $quick = '' ) { - $title = $this->getTitleForId( $entityId ); - return $this->getPermissionStatus( $user, $permission, $title, $quick ); - } - - /** - * @see EntityPermissionChecker::getPermissionStatusForEntityType - * - * @param User $user - * @param string $permission - * @param string $entityType - * @param string $quick - * - * @return Status a status object representing the check's result. - * - * @todo Move to a separate service (merge into WikiPageEntityStore?) - */ - public function getPermissionStatusForEntityType( User $user, $permission, $entityType, $quick = '' ) { - $ns = $this->getNamespaceForType( $entityType ); - $dummyTitle = Title::makeTitle( $ns, '/' ); - - return $this->getPermissionStatus( $user, $permission, $dummyTitle, $quick ); - } - - /** - * @see EntityPermissionChecker::getPermissionStatusForEntity - * - * @note When checking for the 'edit' permission, this will check the 'createpage' - * permission first in case the entity does not yet exist (i.e. if $entity->getId() - * returns null). - * - * @param User $user - * @param string $permission - * @param EntityDocument $entity - * @param string $quick - * - * @return Status a status object representing the check's result. - * - * @todo Move to a separate service (merge into WikiPageEntityStore?) - */ - public function getPermissionStatusForEntity( User $user, $permission, EntityDocument $entity, $quick = '' ) { - $id = $entity->getId(); - $status = null; - - if ( !$id ) { - $entityType = $entity->getType(); - - if ( $permission === 'edit' ) { - // for editing a non-existing page, check the createpage permission - $status = $this->getPermissionStatusForEntityType( $user, 'createpage', $entityType, $quick ); - } - - if ( !$status || $status->isOK() ) { - $status = $this->getPermissionStatusForEntityType( $user, $permission, $entityType, $quick ); - } - } else { - $status = $this->getPermissionStatusForEntityId( $user, $permission, $id, $quick ); - } - - return $status; } } diff --git a/repo/includes/Store/WikiPageEntityStorePermissionChecker.php b/repo/includes/Store/WikiPageEntityStorePermissionChecker.php index 1487ab6..8df144f 100644 --- a/repo/includes/Store/WikiPageEntityStorePermissionChecker.php +++ b/repo/includes/Store/WikiPageEntityStorePermissionChecker.php @@ -167,7 +167,10 @@ } private function getMediaWikiPermissionsToCheck( $action, $entityType ) { - if ( $action === EntityPermissionChecker::ACTION_CREATE ) { + if ( $action === EntityPermissionChecker::ACTION_CREATE || + // TODO: temporarily handle MW create permissions here, until all users are adjusted + $action === 'createpage' || $action === 'property-create' + ) { $entityTypeSpecificCreatePermission = $entityType . '-create'; $permissions = [ 'read', 'edit', 'createpage' ]; @@ -177,7 +180,10 @@ return $permissions; } - if ( $action === EntityPermissionChecker::ACTION_EDIT_TERMS ) { + if ( $action === EntityPermissionChecker::ACTION_EDIT_TERMS || + // TODO: temporarily handle MW permissions here, until all users are adjusted + $action === 'item-term' || $action === 'property-term' + ) { $entityTypeSpecificEditTermsPermission = $entityType . '-term'; $permissions = [ 'read', 'edit' ]; @@ -187,7 +193,10 @@ return $permissions; } - if ( $action === EntityPermissionChecker::ACTION_MERGE ) { + if ( $action === EntityPermissionChecker::ACTION_MERGE || + // TODO: temporarily handle MW permissions here, until all users are adjusted + $action === 'item-merge' + ) { $entityTypeSpecificMergePermission = $entityType . '-merge'; $permissions = [ 'read', 'edit' ]; @@ -197,7 +206,10 @@ return $permissions; } - if ( $action === EntityPermissionChecker::ACTION_REDIRECT ) { + if ( $action === EntityPermissionChecker::ACTION_REDIRECT || + // TODO: temporarily handle MW permissions here, until all users are adjusted + $action === 'item-redirect' + ) { $entityTypeSpecificRedirectPermission = $entityType . '-redirect'; $permissions = [ 'read', 'edit' ]; diff --git a/repo/includes/WikibaseRepo.php b/repo/includes/WikibaseRepo.php index 03b3ff9..74c7122 100644 --- a/repo/includes/WikibaseRepo.php +++ b/repo/includes/WikibaseRepo.php @@ -131,6 +131,7 @@ use Wikibase\Repo\ParserOutput\DispatchingEntityViewFactory; use Wikibase\Repo\ParserOutput\EntityParserOutputGeneratorFactory; use Wikibase\Repo\Store\EntityPermissionChecker; +use Wikibase\Repo\Store\WikiPageEntityStorePermissionChecker; use Wikibase\Repo\Validators\EntityConstraintProvider; use Wikibase\Repo\Validators\SnakValidator; use Wikibase\Repo\Validators\TermValidatorFactory; @@ -1200,7 +1201,13 @@ * @return EntityPermissionChecker */ public function getEntityPermissionChecker() { - return $this->getEntityContentFactory(); + global $wgAvailableRights; + + return new WikiPageEntityStorePermissionChecker( + $this->getEntityNamespaceLookup(), + $this->getEntityTitleLookup(), + $wgAvailableRights + ); } /** diff --git a/repo/tests/phpunit/includes/Content/EntityContentFactoryTest.php b/repo/tests/phpunit/includes/Content/EntityContentFactoryTest.php index 0652bbe..173f23f 100644 --- a/repo/tests/phpunit/includes/Content/EntityContentFactoryTest.php +++ b/repo/tests/phpunit/includes/Content/EntityContentFactoryTest.php @@ -15,7 +15,6 @@ use Wikibase\DataModel\Entity\PropertyId; use Wikibase\Repo\Content\EntityContentFactory; use Wikibase\Repo\WikibaseRepo; -use Wikibase\Repo\Tests\PermissionsHelper; /** * @covers Wikibase\Repo\Content\EntityContentFactory @@ -204,119 +203,6 @@ $this->setExpectedException( OutOfBoundsException::class ); $factory->getEntityHandlerForContentModel( 'foo' ); - } - - public function provideGetPermissionStatusForEntity() { - return [ - 'read allowed for non-existing entity' => [ - 'read', - [ 'read' => true ], - null, - [ - 'getPermissionStatusForEntity' => true, - 'getPermissionStatusForEntityType' => true, - ], - ], - 'edit and createpage allowed for new entity' => [ - 'edit', - [ 'read' => true, 'edit' => true, 'createpage' => true ], - null, - [ - 'getPermissionStatusForEntity' => true, - 'getPermissionStatusForEntityType' => true, - ], - ], - 'implicit createpage not allowed for new entity' => [ - 'edit', - [ 'read' => true, 'edit' => true, 'createpage' => false ], - null, - [ - 'getPermissionStatusForEntity' => false, // "createpage" is implicitly needed - 'getPermissionStatusForEntityType' => true, // "edit" is allowed for type - ], - ], - 'createpage not allowed' => [ - 'createpage', - [ 'read' => true, 'edit' => true, 'createpage' => false ], - null, - [ - 'getPermissionStatusForEntity' => false, // "createpage" is implicitly needed - 'getPermissionStatusForEntityType' => false, // "createpage" is not allowed - ], - ], - 'edit allowed for existing item' => [ - 'edit', - [ 'read' => true, 'edit' => true, 'createpage' => false ], - 'Q23', - [ - 'getPermissionStatusForEntity' => true, - 'getPermissionStatusForEntityType' => true, - 'getPermissionStatusForEntityId' => true, - ], - ], - 'edit not allowed' => [ - 'edit', - [ 'read' => true, 'edit' => false ], - 'Q23', - [ - 'getPermissionStatusForEntity' => false, - 'getPermissionStatusForEntityType' => false, - 'getPermissionStatusForEntityId' => false, - ], - ], - 'delete not allowed' => [ - 'delete', - [ 'read' => true, 'delete' => false ], - null, - [ - 'getPermissionStatusForEntity' => false, - 'getPermissionStatusForEntityType' => false, - ], - ], - ]; - } - - /** - * @dataProvider provideGetPermissionStatusForEntity - */ - public function testGetPermissionStatusForEntity( $action, array $permissions, $id, array $expectations ) { - global $wgUser; - - $entity = new Item(); - - if ( $id ) { - // "exists" - $entity->setId( new ItemId( $id ) ); - } - - $this->stashMwGlobals( 'wgUser' ); - $this->stashMwGlobals( 'wgGroupPermissions' ); - - PermissionsHelper::applyPermissions( - // set permissions for implicit groups - [ '*' => $permissions, - 'user' => $permissions, - 'autoconfirmed' => $permissions, - 'emailconfirmed' => $permissions ], - [] // remove all groups not implied - ); - - $factory = $this->newFactory(); - - if ( isset( $expectations['getPermissionStatusForEntity'] ) ) { - $status = $factory->getPermissionStatusForEntity( $wgUser, $action, $entity ); - $this->assertEquals( $expectations['getPermissionStatusForEntity'], $status->isOK() ); - } - - if ( isset( $expectations['getPermissionStatusForEntityType'] ) ) { - $status = $factory->getPermissionStatusForEntityType( $wgUser, $action, $entity->getType() ); - $this->assertEquals( $expectations['getPermissionStatusForEntityType'], $status->isOK() ); - } - - if ( isset( $expectations['getPermissionStatusForEntityId'] ) ) { - $status = $factory->getPermissionStatusForEntityId( $wgUser, $action, $entity->getId() ); - $this->assertEquals( $expectations['getPermissionStatusForEntityId'], $status->isOK() ); - } } public function newFromEntityProvider() { diff --git a/repo/tests/phpunit/includes/Specials/SpecialSetSiteLinkTest.php b/repo/tests/phpunit/includes/Specials/SpecialSetSiteLinkTest.php index b34e3df..8268cbb 100644 --- a/repo/tests/phpunit/includes/Specials/SpecialSetSiteLinkTest.php +++ b/repo/tests/phpunit/includes/Specials/SpecialSetSiteLinkTest.php @@ -93,7 +93,7 @@ protected function setUp() { parent::setUp(); - $this->setMwGlobals( 'wgGroupPermissions', [ '*' => [ 'edit' => true ] ] ); + $this->setMwGlobals( 'wgGroupPermissions', [ '*' => [ 'read' => true, 'edit' => true ] ] ); if ( !self::$badgeId ) { self::$matchers = self::createMatchers(); -- To view, visit https://gerrit.wikimedia.org/r/361889 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I9481c95ae0b359564a14bc6411102709970aedbe Gerrit-PatchSet: 12 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: WMDE-leszek <[email protected]> Gerrit-Reviewer: Aleksey Bekh-Ivanov (WMDE) <[email protected]> Gerrit-Reviewer: Daniel Kinzler <[email protected]> Gerrit-Reviewer: Thiemo Mättig (WMDE) <[email protected]> Gerrit-Reviewer: WMDE-leszek <[email protected]> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
