WMDE-leszek has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/365259 )
Change subject: [DNM][EXPERIMENT] Require *-term permissions to edit entity
terms using wbeditentity
......................................................................
[DNM][EXPERIMENT] Require *-term permissions to edit entity terms using
wbeditentity
This is a rough experiment to see what it takes to make wbeditentity
be more accurate about permission its changes require.
T170673 suggests other way to solve this issue.
This makes EditEntity get information required to decide on what permissions
are required from EntityChangeOpProvider, which in turn gets this from
entity-level ChangeOpDeserializers (using PermissionAwareChangeOpDeserializer
interface).
Possible advantage compared to what T170673 suggests: as the issue
seems to be only relevant to wbeditentity, ChangeOp interface is not
"polluted" with permission-related stuff, and stays simple.
On the flip side: solution here is a bit messy.
Also adds permission-related tests for creating an entity using
setX API actions which have not been there before. Also adjusts
WikiPageEntityStorePermissionChecker to handle the case of creating
the entity when peforming ACTION_EDIT_TERMS.
TODO:
- resolve TODOs (e.g. what about wbeditentity's "clear" param?)
- add tests for missing new bits
- this is probably too much change for a single patch!
Change-Id: I294f7bcfbd0e10b6a6ad82c80ddd8a4ed5eb637e
---
M repo/includes/Api/EditEntity.php
M repo/includes/Api/ModifyEntity.php
M repo/includes/Api/ModifyTerm.php
M repo/includes/Api/SetAliases.php
M repo/includes/ChangeOp/Deserialization/ItemChangeOpDeserializer.php
A repo/includes/ChangeOp/Deserialization/PermissionAwareChangeOpDeserializer.php
M repo/includes/ChangeOp/Deserialization/PropertyChangeOpDeserializer.php
M repo/includes/ChangeOp/EntityChangeOpProvider.php
M repo/includes/Store/EntityPermissionChecker.php
M repo/includes/Store/WikiPageEntityStorePermissionChecker.php
M repo/tests/phpunit/includes/Api/EditEntityTest.php
M repo/tests/phpunit/includes/Api/SetAliasesTest.php
M repo/tests/phpunit/includes/Api/SetDescriptionTest.php
M repo/tests/phpunit/includes/Api/SetLabelTest.php
M repo/tests/phpunit/includes/Api/SetSiteLinkTest.php
M repo/tests/phpunit/includes/Store/WikiPageEntityStorePermissionCheckerTest.php
16 files changed, 432 insertions(+), 39 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/59/365259/1
diff --git a/repo/includes/Api/EditEntity.php b/repo/includes/Api/EditEntity.php
index 63a2dcd..e4404f6 100644
--- a/repo/includes/Api/EditEntity.php
+++ b/repo/includes/Api/EditEntity.php
@@ -25,6 +25,7 @@
use Wikibase\Lib\Store\EntityRevisionLookup;
use Wikibase\Repo\ChangeOp\Deserialization\ChangeOpDeserializationException;
use Wikibase\Repo\ChangeOp\EntityChangeOpProvider;
+use Wikibase\Repo\Store\EntityPermissionChecker;
use Wikibase\Summary;
/**
@@ -150,6 +151,24 @@
}
/**
+ * @param EntityDocument $entity
+ *
+ * @throws InvalidArgumentException
+ * @return string[] A list of permissions
+ */
+ protected function getRequiredPermissions( EntityDocument $entity,
array $params ) {
+ $data = json_decode( $params['data'], true );
+ if (
$this->entityChangeOpProvider->includesChangesToEntityTerms(
$entity->getType(), $data ) ) {
+ return [ EntityPermissionChecker::ACTION_EDIT_TERMS ];
+ }
+ // TODO: if "clear" parameter provided, always assume $entity
might have some terms that will be removed?
+ // TODO: or do it 100% right and check if there are any terms
on $entity that would change? That'd be
+ // becoming a bit of an overkill...
+
+ return [ EntityPermissionChecker::ACTION_EDIT ];
+ }
+
+ /**
* @param EntityId $entityId
*
* @return bool
@@ -190,6 +209,9 @@
'param-illegal'
);
}
+
+ $this->validateDataParameter( $params );
+ $this->checkValidJson( json_decode( $params['data'], true ) );
}
/**
@@ -202,7 +224,6 @@
* @return Summary
*/
protected function modifyEntity( EntityDocument &$entity, array
$params, $baseRevId ) {
- $this->validateDataParameter( $params );
$data = json_decode( $params['data'], true );
$this->validateDataProperties( $data, $entity, $baseRevId );
@@ -341,7 +362,6 @@
$entityId = $entity->getId();
$title = $entityId === null ? null :
$this->getTitleLookup()->getTitleForId( $entityId );
- $this->checkValidJson( $data );
$this->checkEntityId( $data, $entityId );
$this->checkEntityType( $data, $entity );
$this->checkPageIdProp( $data, $title );
diff --git a/repo/includes/Api/ModifyEntity.php
b/repo/includes/Api/ModifyEntity.php
index 7bb82bd..9859460 100644
--- a/repo/includes/Api/ModifyEntity.php
+++ b/repo/includes/Api/ModifyEntity.php
@@ -232,7 +232,7 @@
}
// At this point only change/edit rights should be checked
- $status = $this->checkPermissions( $entity, $user );
+ $status = $this->checkPermissions( $entity, $user, $params );
if ( !$status->isOK() ) {
// Was …->dieError( 'You do not have sufficient
permissions', … ) before T150512.
@@ -266,11 +266,12 @@
*
* @param EntityDocument $entity the entity to check
* @param User $user User doing the action
+ * @param array $params
*
* @return Status the check's result
*/
- private function checkPermissions( EntityDocument $entity, User $user )
{
- $permissions = $this->getRequiredPermissions( $entity );
+ private function checkPermissions( EntityDocument $entity, User $user,
array $params ) {
+ $permissions = $this->getRequiredPermissions( $entity, $params
);
$status = Status::newGood();
foreach ( array_unique( $permissions ) as $perm ) {
@@ -283,10 +284,11 @@
/**
* @param EntityDocument $entity
+ * @param array $params
*
* @return string[]
*/
- protected function getRequiredPermissions( EntityDocument $entity ) {
+ protected function getRequiredPermissions( EntityDocument $entity,
array $params ) {
return [ EntityPermissionChecker::ACTION_EDIT ];
}
diff --git a/repo/includes/Api/ModifyTerm.php b/repo/includes/Api/ModifyTerm.php
index c6b744a..c6e82ba 100644
--- a/repo/includes/Api/ModifyTerm.php
+++ b/repo/includes/Api/ModifyTerm.php
@@ -39,11 +39,12 @@
/**
* @param EntityDocument $entity
+ * @param array $params
*
* @throws InvalidArgumentException
* @return string[] A list of permissions
*/
- protected function getRequiredPermissions( EntityDocument $entity ) {
+ protected function getRequiredPermissions( EntityDocument $entity,
array $params ) {
return [ EntityPermissionChecker::ACTION_EDIT_TERMS ];
}
diff --git a/repo/includes/Api/SetAliases.php b/repo/includes/Api/SetAliases.php
index 39f068c..b627ff5 100644
--- a/repo/includes/Api/SetAliases.php
+++ b/repo/includes/Api/SetAliases.php
@@ -62,11 +62,12 @@
/**
* @param EntityDocument $entity
+ * @param array $params
*
* @throws InvalidArgumentException
* @return string[] A list of permissions
*/
- protected function getRequiredPermissions( EntityDocument $entity ) {
+ protected function getRequiredPermissions( EntityDocument $entity,
array $params ) {
return [ EntityPermissionChecker::ACTION_EDIT_TERMS ];
}
diff --git
a/repo/includes/ChangeOp/Deserialization/ItemChangeOpDeserializer.php
b/repo/includes/ChangeOp/Deserialization/ItemChangeOpDeserializer.php
index 217329c..29f70a4 100644
--- a/repo/includes/ChangeOp/Deserialization/ItemChangeOpDeserializer.php
+++ b/repo/includes/ChangeOp/Deserialization/ItemChangeOpDeserializer.php
@@ -4,14 +4,13 @@
use Wikibase\Repo\ChangeOp\ChangeOp;
use Wikibase\Repo\ChangeOp\ChangeOps;
-use Wikibase\Repo\ChangeOp\ChangeOpDeserializer;
/**
* Constructs ChangeOps for item change requests
*
* @license GPL-2.0+
*/
-class ItemChangeOpDeserializer implements ChangeOpDeserializer {
+class ItemChangeOpDeserializer implements PermissionAwareChangeOpDeserializer {
/**
* @var ChangeOpDeserializerFactory
@@ -77,4 +76,9 @@
return $changeOps;
}
+ public function includesChangesToEntityTerms( array $changeRequest ) {
+ return array_key_exists( 'labels', $changeRequest ) ||
array_key_exists( 'descriptions', $changeRequest ) ||
+ array_key_exists( 'aliases', $changeRequest );
+ }
+
}
diff --git
a/repo/includes/ChangeOp/Deserialization/PermissionAwareChangeOpDeserializer.php
b/repo/includes/ChangeOp/Deserialization/PermissionAwareChangeOpDeserializer.php
new file mode 100644
index 0000000..353d3de
--- /dev/null
+++
b/repo/includes/ChangeOp/Deserialization/PermissionAwareChangeOpDeserializer.php
@@ -0,0 +1,19 @@
+<?php
+
+namespace Wikibase\Repo\ChangeOp\Deserialization;
+
+use Wikibase\Repo\ChangeOp\ChangeOpDeserializer;
+
+/**
+ * TODO: better name?
+ * TODO: does it make sense to have this extend ChangeOpDeserializer interface?
+ */
+interface PermissionAwareChangeOpDeserializer extends ChangeOpDeserializer {
+
+ /**
+ * @param array $changeRequest
+ * @return bool
+ */
+ public function includesChangesToEntityTerms( array $changeRequest );
+
+}
diff --git
a/repo/includes/ChangeOp/Deserialization/PropertyChangeOpDeserializer.php
b/repo/includes/ChangeOp/Deserialization/PropertyChangeOpDeserializer.php
index e718b6f..3cb8aaf 100644
--- a/repo/includes/ChangeOp/Deserialization/PropertyChangeOpDeserializer.php
+++ b/repo/includes/ChangeOp/Deserialization/PropertyChangeOpDeserializer.php
@@ -4,14 +4,13 @@
use Wikibase\Repo\ChangeOp\ChangeOp;
use Wikibase\Repo\ChangeOp\ChangeOps;
-use Wikibase\Repo\ChangeOp\ChangeOpDeserializer;
/**
* Constructs ChangeOp objects for property change requests
*
* @license GPL-2.0+
*/
-class PropertyChangeOpDeserializer implements ChangeOpDeserializer {
+class PropertyChangeOpDeserializer implements
PermissionAwareChangeOpDeserializer {
/**
* @var ChangeOpDeserializerFactory
@@ -65,4 +64,9 @@
return $changeOps;
}
+ public function includesChangesToEntityTerms( array $changeRequest ) {
+ return array_key_exists( 'labels', $changeRequest ) ||
array_key_exists( 'descriptions', $changeRequest ) ||
+ array_key_exists( 'aliases', $changeRequest );
+ }
+
}
diff --git a/repo/includes/ChangeOp/EntityChangeOpProvider.php
b/repo/includes/ChangeOp/EntityChangeOpProvider.php
index d271e9d..283d51f 100644
--- a/repo/includes/ChangeOp/EntityChangeOpProvider.php
+++ b/repo/includes/ChangeOp/EntityChangeOpProvider.php
@@ -4,6 +4,7 @@
use Wikibase\Repo\ChangeOp\ChangeOp;
use Wikibase\Repo\ChangeOp\Deserialization\ChangeOpDeserializationException;
+use Wikibase\Repo\ChangeOp\Deserialization\PermissionAwareChangeOpDeserializer;
use Wikimedia\Assert\Assert;
/**
@@ -53,6 +54,17 @@
return $deserializer->createEntityChangeOp( $changeRequest );
}
+ public function includesChangesToEntityTerms( $entityType, array
$changeRequest ) {
+ $deserializer = $this->getDeserializerForEntityType(
$entityType );
+
+ // TODO: make newDeserializerForEntityType assert that
$deserializer is
+ // a PermissionAwareChangeOpDeserializer?
+ if ( ! $deserializer instanceof
PermissionAwareChangeOpDeserializer ) {
+ return false;
+ }
+ return $deserializer->includesChangesToEntityTerms(
$changeRequest );
+ }
+
/**
* @param string $type
*
diff --git a/repo/includes/Store/EntityPermissionChecker.php
b/repo/includes/Store/EntityPermissionChecker.php
index 60de8e3..14212f4 100644
--- a/repo/includes/Store/EntityPermissionChecker.php
+++ b/repo/includes/Store/EntityPermissionChecker.php
@@ -20,6 +20,10 @@
const ACTION_EDIT = 'edit';
+ // TODO: should this be a separate action? No class asks explicitly for
"create-only" permissions. And
+ // There is no way to just create an entity, some of its properties
must set, so it is at least some
+ // kind of an "edit" action too. Maybe implementation should take care
of specifics of "create" case
+ // and this would be irrelevant here?
const ACTION_CREATE = 'create';
const ACTION_EDIT_TERMS = 'term';
diff --git a/repo/includes/Store/WikiPageEntityStorePermissionChecker.php
b/repo/includes/Store/WikiPageEntityStorePermissionChecker.php
index 0831a7f..6cda6b3 100644
--- a/repo/includes/Store/WikiPageEntityStorePermissionChecker.php
+++ b/repo/includes/Store/WikiPageEntityStorePermissionChecker.php
@@ -66,11 +66,6 @@
if ( $id === null ) {
$entityType = $entity->getType();
- if ( $action === EntityPermissionChecker::ACTION_EDIT )
{
- // for editing a non-existing page, check the
create permission
- return $this->getPermissionStatusForEntityType(
$user, EntityPermissionChecker::ACTION_CREATE, $entityType, $quick );
- }
-
return $this->getPermissionStatusForEntityType( $user,
$action, $entityType, $quick );
}
@@ -93,15 +88,6 @@
$title = $this->titleLookup->getTitleForId( $entityId );
if ( $title === null || !$title->exists() ) {
- if ( $action === EntityPermissionChecker::ACTION_EDIT )
{
- return $this->getPermissionStatusForEntityType(
- $user,
- EntityPermissionChecker::ACTION_CREATE,
- $entityId->getEntityType(),
- $quick
- );
- }
-
return $this->getPermissionStatusForEntityType(
$user,
$action,
@@ -110,7 +96,7 @@
);
}
- return $this->checkPermissionsForAction( $user, $action,
$title, $entityId->getEntityType(), $quick );
+ return $this->checkPermissionsForActions( $user, [ $action ],
$title, $entityId->getEntityType(), $quick );
}
/**
@@ -128,18 +114,27 @@
public function getPermissionStatusForEntityType( User $user, $action,
$type, $quick = '' ) {
$title = $this->getPageTitleInEntityNamespace( $type );
- if ( $action === EntityPermissionChecker::ACTION_EDIT ) {
+ if ( $this->isEditAction( $action ) ) {
// Note: No entity ID given, assuming creating new
entity, i.e. create permissions will be checked
- return $this->checkPermissionsForAction(
+ return $this->checkPermissionsForActions(
$user,
- EntityPermissionChecker::ACTION_CREATE,
+ [ $action,
EntityPermissionChecker::ACTION_CREATE ],
$title,
$type,
$quick
);
}
- return $this->checkPermissionsForAction( $user, $action,
$title, $type, $quick );
+ return $this->checkPermissionsForActions( $user, [ $action ],
$title, $type, $quick );
+ }
+
+ /**
+ * @param string $action
+ *
+ * @return bool
+ */
+ private function isEditAction( $action ) {
+ return $action === EntityPermissionChecker::ACTION_EDIT ||
$action === EntityPermissionChecker::ACTION_EDIT_TERMS;
}
/**
@@ -153,10 +148,19 @@
return Title::makeTitle( $namespace, '/' );
}
- private function checkPermissionsForAction( User $user, $action, Title
$title, $entityType, $quick ='' ) {
+ private function checkPermissionsForActions( User $user, array
$actions, Title $title, $entityType, $quick ='' ) {
$status = Status::newGood();
- $mediaWikiPermissions = $this->getMediaWikiPermissionsToCheck(
$action, $entityType );
+ $mediaWikiPermissions = [];
+
+ foreach ( $actions as $action ) {
+ $mediaWikiPermissions = array_merge(
+ $mediaWikiPermissions,
+ $this->getMediaWikiPermissionsToCheck( $action,
$entityType )
+ );
+ }
+
+ $mediaWikiPermissions = array_unique( $mediaWikiPermissions );
foreach ( $mediaWikiPermissions as $mwPermission ) {
$partialStatus = $this->getPermissionStatus( $user,
$mwPermission, $title, $quick );
@@ -171,9 +175,11 @@
$entityTypeSpecificCreatePermission = $entityType .
'-create';
$permissions = [ 'read', 'edit', 'createpage' ];
+
if ( $this->mediawikiPermissionExists(
$entityTypeSpecificCreatePermission ) ) {
$permissions[] =
$entityTypeSpecificCreatePermission;
}
+
return $permissions;
}
diff --git a/repo/tests/phpunit/includes/Api/EditEntityTest.php
b/repo/tests/phpunit/includes/Api/EditEntityTest.php
index a097a8b..c2df8dd 100644
--- a/repo/tests/phpunit/includes/Api/EditEntityTest.php
+++ b/repo/tests/phpunit/includes/Api/EditEntityTest.php
@@ -423,7 +423,7 @@
$userWithAllPermissions = $this->createUserWithGroup(
'all-permission' );
$this->setMwGlobals( 'wgGroupPermissions', [
- 'all-permission' => [ 'read' => true, 'edit' => true,
'createpage' => true ],
+ 'all-permission' => [ 'read' => true, 'edit' => true,
'item-term' => true, 'createpage' => true ],
'*' => [ 'read' => true, 'edit' => false, 'writeapi' =>
true ]
] );
@@ -437,7 +437,7 @@
$this->setMwGlobals( 'wgGroupPermissions', [
'no-permission' => [ 'read' => true, 'edit' => false ],
- 'all-permission' => [ 'read' => true, 'edit' => true,
'createpage' => true ],
+ 'all-permission' => [ 'read' => true, 'edit' => true,
'item-term' => true, 'createpage' => true ],
'*' => [ 'read' => true, 'edit' => false, 'writeapi' =>
true ]
] );
@@ -458,14 +458,12 @@
}
public function testEditingLabelRequiresEntityTermEditPermissions() {
- $this->markTestSkipped( 'Api\EditEntity currently does not
check term edit permissions when editing terms!' );
-
$userWithInsufficientPermissions = $this->createUserWithGroup(
'no-permission' );
$userWithAllPermissions = $this->createUserWithGroup(
'all-permission' );
$this->setMwGlobals( 'wgGroupPermissions', [
'no-permission' => [ 'read' => true, 'edit' => true,
'item-term' => false, ],
- 'all-permission' => [ 'read' => true, 'edit' => true,
'createpage' => true ],
+ 'all-permission' => [ 'read' => true, 'edit' => true,
'item-term' => true, 'createpage' => true ],
'*' => [ 'read' => true, 'edit' => false, 'writeapi' =>
true ]
] );
diff --git a/repo/tests/phpunit/includes/Api/SetAliasesTest.php
b/repo/tests/phpunit/includes/Api/SetAliasesTest.php
index c64df6a..15b2d15 100644
--- a/repo/tests/phpunit/includes/Api/SetAliasesTest.php
+++ b/repo/tests/phpunit/includes/Api/SetAliasesTest.php
@@ -303,6 +303,45 @@
);
}
+ public function
testUserCanCreateItemWithAliasWhenTheyHaveSufficientPermissions() {
+ $userWithAllPermissions = $this->createUserWithGroup(
'all-permission' );
+
+ $this->setMwGlobals( 'wgGroupPermissions', [
+ 'all-permission' => [ 'item-term' => true, 'createpage'
=> true ],
+ '*' => [ 'read' => true, 'edit' => true, 'writeapi' =>
true ]
+ ] );
+
+ list ( $result, ) = $this->doApiRequestWithToken(
+ $this->getCreateItemAndSetAliasRequestParams(),
+ null,
+ $userWithAllPermissions
+ );
+
+ $this->assertEquals( 1, $result['success'] );
+ $this->assertSame( 'an alias',
$result['entity']['aliases']['en'][0]['value'] );
+ }
+
+ public function testUserCannotCreateItemWhenTheyLackPermission() {
+ $userWithInsufficientPermissions = $this->createUserWithGroup(
'no-permission' );
+
+ $this->setMwGlobals( 'wgGroupPermissions', [
+ 'no-permission' => [ 'createpage' => false ],
+ '*' => [ 'read' => true, 'edit' => true, 'item-term' =>
true, 'writeapi' => true ]
+ ] );
+
+ // Then the request is denied
+ $expected = [
+ 'type' => ApiUsageException::class,
+ 'code' => 'permissiondenied'
+ ];
+
+ $this->doTestQueryExceptions(
+ $this->getCreateItemAndSetAliasRequestParams(),
+ $expected,
+ $userWithInsufficientPermissions
+ );
+ }
+
/**
* @param User $user
* @return Item
@@ -330,4 +369,13 @@
];
}
+ private function getCreateItemAndSetAliasRequestParams() {
+ return [
+ 'action' => 'wbsetaliases',
+ 'new' => 'item',
+ 'language' => 'en',
+ 'add' => 'an alias',
+ ];
+ }
+
}
diff --git a/repo/tests/phpunit/includes/Api/SetDescriptionTest.php
b/repo/tests/phpunit/includes/Api/SetDescriptionTest.php
index 99e8998..1dd35dd 100644
--- a/repo/tests/phpunit/includes/Api/SetDescriptionTest.php
+++ b/repo/tests/phpunit/includes/Api/SetDescriptionTest.php
@@ -98,6 +98,45 @@
);
}
+ public function
testUserCanCreateItemWithDescriptionWhenTheyHaveSufficientPermissions() {
+ $userWithAllPermissions = $this->createUserWithGroup(
'all-permission' );
+
+ $this->setMwGlobals( 'wgGroupPermissions', [
+ 'all-permission' => [ 'item-term' => true, 'createpage'
=> true ],
+ '*' => [ 'read' => true, 'edit' => true, 'writeapi' =>
true ]
+ ] );
+
+ list ( $result, ) = $this->doApiRequestWithToken(
+ $this->getCreateItemAndSetDescriptionRequestParams(),
+ null,
+ $userWithAllPermissions
+ );
+
+ $this->assertEquals( 1, $result['success'] );
+ $this->assertSame( 'some description',
$result['entity']['descriptions']['en']['value'] );
+ }
+
+ public function testUserCannotCreateItemWhenTheyLackPermission() {
+ $userWithInsufficientPermissions = $this->createUserWithGroup(
'no-permission' );
+
+ $this->setMwGlobals( 'wgGroupPermissions', [
+ 'no-permission' => [ 'createpage' => false ],
+ '*' => [ 'read' => true, 'edit' => true, 'item-term' =>
true, 'writeapi' => true ]
+ ] );
+
+ // Then the request is denied
+ $expected = [
+ 'type' => ApiUsageException::class,
+ 'code' => 'permissiondenied'
+ ];
+
+ $this->doTestQueryExceptions(
+ $this->getCreateItemAndSetDescriptionRequestParams(),
+ $expected,
+ $userWithInsufficientPermissions
+ );
+ }
+
/**
* @param User $user
* @return Item
@@ -125,4 +164,13 @@
];
}
+ private function getCreateItemAndSetDescriptionRequestParams() {
+ return [
+ 'action' => 'wbsetdescription',
+ 'new' => 'item',
+ 'language' => 'en',
+ 'value' => 'some description',
+ ];
+ }
+
}
diff --git a/repo/tests/phpunit/includes/Api/SetLabelTest.php
b/repo/tests/phpunit/includes/Api/SetLabelTest.php
index 4875845..1dff140 100644
--- a/repo/tests/phpunit/includes/Api/SetLabelTest.php
+++ b/repo/tests/phpunit/includes/Api/SetLabelTest.php
@@ -98,6 +98,45 @@
);
}
+ public function
testUserCanCreateItemWithLabelWhenTheyHaveSufficientPermissions() {
+ $userWithAllPermissions = $this->createUserWithGroup(
'all-permission' );
+
+ $this->setMwGlobals( 'wgGroupPermissions', [
+ 'all-permission' => [ 'item-term' => true, 'createpage'
=> true ],
+ '*' => [ 'read' => true, 'edit' => true, 'writeapi' =>
true ]
+ ] );
+
+ list ( $result, ) = $this->doApiRequestWithToken(
+ $this->getCreateItemAndSetLabelRequestParams(),
+ null,
+ $userWithAllPermissions
+ );
+
+ $this->assertEquals( 1, $result['success'] );
+ $this->assertSame( 'a label',
$result['entity']['labels']['en']['value'] );
+ }
+
+ public function testUserCannotCreateItemWhenTheyLackPermission() {
+ $userWithInsufficientPermissions = $this->createUserWithGroup(
'no-permission' );
+
+ $this->setMwGlobals( 'wgGroupPermissions', [
+ 'no-permission' => [ 'createpage' => false ],
+ '*' => [ 'read' => true, 'edit' => true, 'item-term' =>
true, 'writeapi' => true ]
+ ] );
+
+ // Then the request is denied
+ $expected = [
+ 'type' => ApiUsageException::class,
+ 'code' => 'permissiondenied'
+ ];
+
+ $this->doTestQueryExceptions(
+ $this->getCreateItemAndSetLabelRequestParams(),
+ $expected,
+ $userWithInsufficientPermissions
+ );
+ }
+
/**
* @param User $user
* @return Item
@@ -116,6 +155,15 @@
}
+ private function getCreateItemAndSetLabelRequestParams() {
+ return [
+ 'action' => 'wbsetlabel',
+ 'new' => 'item',
+ 'language' => 'en',
+ 'value' => 'a label',
+ ];
+ }
+
private function getSetLabelRequestParams( ItemId $id ) {
return [
'action' => 'wbsetlabel',
diff --git a/repo/tests/phpunit/includes/Api/SetSiteLinkTest.php
b/repo/tests/phpunit/includes/Api/SetSiteLinkTest.php
index 4ae4ecd..e93990d 100644
--- a/repo/tests/phpunit/includes/Api/SetSiteLinkTest.php
+++ b/repo/tests/phpunit/includes/Api/SetSiteLinkTest.php
@@ -610,6 +610,45 @@
);
}
+ public function
testUserCanCreateItemWithSiteLinkWhenTheyHaveSufficientPermissions() {
+ $userWithAllPermissions = $this->createUserWithGroup(
'all-permission' );
+
+ $this->setMwGlobals( 'wgGroupPermissions', [
+ 'all-permission' => [ 'edit' => true, 'createpage' =>
true ],
+ '*' => [ 'read' => true, 'writeapi' => true ]
+ ] );
+
+ list ( $result, ) = $this->doApiRequestWithToken(
+ $this->getCreateItemAndSetSiteLinkRequestParams(),
+ null,
+ $userWithAllPermissions
+ );
+
+ $this->assertEquals( 1, $result['success'] );
+ $this->assertSame( 'Another Cool Page',
$result['entity']['sitelinks']['enwiki']['title'] );
+ }
+
+ public function testUserCannotCreateItemWhenTheyLackPermission() {
+ $userWithInsufficientPermissions = $this->createUserWithGroup(
'no-permission' );
+
+ $this->setMwGlobals( 'wgGroupPermissions', [
+ 'no-permission' => [ 'createpage' => false ],
+ '*' => [ 'read' => true, 'edit' => true, 'writeapi' =>
true ]
+ ] );
+
+ // Then the request is denied
+ $expected = [
+ 'type' => ApiUsageException::class,
+ 'code' => 'permissiondenied'
+ ];
+
+ $this->doTestQueryExceptions(
+ $this->getCreateItemAndSetSiteLinkRequestParams(),
+ $expected,
+ $userWithInsufficientPermissions
+ );
+ }
+
/**
* @param User $user
* @return Item
@@ -633,7 +672,16 @@
'action' => 'wbsetsitelink',
'id' => $id->getSerialization(),
'linksite' => 'enwiki',
- 'linktitle' => 'Come Cool Page',
+ 'linktitle' => 'Some Cool Page',
+ ];
+ }
+
+ private function getCreateItemAndSetSiteLinkRequestParams() {
+ return [
+ 'action' => 'wbsetsitelink',
+ 'new' => 'item',
+ 'linksite' => 'enwiki',
+ 'linktitle' => 'Another Cool Page',
];
}
diff --git
a/repo/tests/phpunit/includes/Store/WikiPageEntityStorePermissionCheckerTest.php
b/repo/tests/phpunit/includes/Store/WikiPageEntityStorePermissionCheckerTest.php
index c8cfbc3..a8d3551 100644
---
a/repo/tests/phpunit/includes/Store/WikiPageEntityStorePermissionCheckerTest.php
+++
b/repo/tests/phpunit/includes/Store/WikiPageEntityStorePermissionCheckerTest.php
@@ -519,6 +519,136 @@
];
}
+ /**
+ * @dataProvider
provideNonExistingEntitiesAndPermissionsThatAllowEditingTerms
+ */
+ public function
testAllRequiredPermissionsAreNeededToEditTermsOfNonExistingEntity(
+ EntityDocument $nonExistingEntity,
+ array $groupPermissions
+ ) {
+ $this->anyUserHasPermissions( $groupPermissions );
+
+ $this->assertUserIsAllowedTo(
EntityPermissionChecker::ACTION_EDIT_TERMS, $nonExistingEntity );
+ }
+
+ public function
provideNonExistingEntitiesAndPermissionsThatAllowEditingTerms() {
+ return [
+ 'non-existing item, createpage permission' => [
+ $this->getNonExistingItem(),
+ [ 'createpage' => true ]
+ ],
+ 'non-existing item (null ID), createpage permission' =>
[
+ $this->getNonExistingItemWithNullId(),
+ [ 'createpage' => true ]
+ ],
+ 'non-existing property, createpage and property-create
permission' => [
+ $this->getNonExistingProperty(),
+ [ 'createpage' => true, 'property-create' =>
true, ]
+ ],
+ 'non-existing property (null ID), createpage and
property-create permission' => [
+ $this->getNonExistingPropertyWithNullId(),
+ [ 'createpage' => true, 'property-create' =>
true, ]
+ ],
+ ];
+ }
+
+ /**
+ * @dataProvider
provideNonExistingEntitiesAndPermissionsThatDisallowEditingTerms
+ */
+ public function
testAllRequiredPermissionsAreNeededToEditTermsOfNonExistingEntity_failures(
+ EntityDocument $nonExistingEntity,
+ array $groupPermissions
+ ) {
+ $this->anyUserHasPermissions( $groupPermissions );
+
+ $this->assertItIsForbiddenForUserTo(
EntityPermissionChecker::ACTION_EDIT_TERMS, $nonExistingEntity );
+ }
+
+ public function
provideNonExistingEntitiesAndPermissionsThatDisallowEditingTerms() {
+ return [
+ 'non-existing item, no createpage permission' => [
+ $this->getNonExistingItem(),
+ [ 'createpage' => false ]
+ ],
+ 'non-existing item, no item-term permission' => [
+ $this->getNonExistingItem(),
+ [ 'item-term' => false, 'createpage' => true ]
+ ],
+ 'non-existing item, no edit permission' => [
+ $this->getNonExistingItem(),
+ [ 'edit' => false, 'item-term' => true,
'createpage' => true ]
+ ],
+ 'non-existing item, no read permission' => [
+ $this->getNonExistingItem(),
+ [ 'read' => false, 'edit' => true, 'item-term'
=> true, 'createpage' => true ]
+ ],
+ 'non-existing item (null ID), no createpage permission'
=> [
+ $this->getNonExistingItemWithNullId(),
+ [ 'createpage' => false ]
+ ],
+ 'non-existing item (null ID), no item-term permission'
=> [
+ $this->getNonExistingItemWithNullId(),
+ [ 'item-term' => false, 'createpage' => true ]
+ ],
+ 'non-existing item (null ID), no edit permission' => [
+ $this->getNonExistingItemWithNullId(),
+ [ 'edit' => false, 'item-term' => true,
'createpage' => true ]
+ ],
+ 'non-existing item (null ID), no read permission' => [
+ $this->getNonExistingItemWithNullId(),
+ [ 'read' => false, 'edit' => true, 'item-term'
=> true, 'createpage' => true ]
+ ],
+ 'non-existing property, no property-create permission'
=> [
+ $this->getNonExistingProperty(),
+ [ 'createpage' => true, 'property-create' =>
false, ]
+ ],
+ 'non-existing property, no createpage permission' => [
+ $this->getNonExistingProperty(),
+ [ 'createpage' => false, 'property-create' =>
true, ]
+ ],
+ 'non-existing property, no createpage nor
property-create permission' => [
+ $this->getNonExistingProperty(),
+ [ 'createpage' => false, 'property-create' =>
false, ]
+ ],
+ 'non-existing property, no property-term permission' =>
[
+ $this->getNonExistingProperty(),
+ [ 'property-term' => false, 'createpage' =>
true, 'property-create' => true, ]
+ ],
+ 'non-existing property, no edit permission' => [
+ $this->getNonExistingProperty(),
+ [ 'edit' => false, 'property-term' => true,
'createpage' => true, 'property-create' => true, ]
+ ],
+ 'non-existing property, no read permission' => [
+ $this->getNonExistingProperty(),
+ [ 'read' => false, 'edit' => true,
'property-term' => true, 'createpage' => true, 'property-create' => true, ]
+ ],
+ 'non-existing property (null ID), no property-create
permission' => [
+ $this->getNonExistingPropertyWithNullId(),
+ [ 'createpage' => true, 'property-create' =>
false, ]
+ ],
+ 'non-existing property (null ID), no createpage
permission' => [
+ $this->getNonExistingPropertyWithNullId(),
+ [ 'createpage' => false, 'property-create' =>
true, ]
+ ],
+ 'non-existing property (null ID), no createpage nor
property-create permission' => [
+ $this->getNonExistingPropertyWithNullId(),
+ [ 'createpage' => false, 'property-create' =>
false, ]
+ ],
+ 'non-existing property (null ID), no property-term
permission' => [
+ $this->getNonExistingPropertyWithNullId(),
+ [ 'property-term' => false, 'createpage' =>
true, 'property-create' => true, ]
+ ],
+ 'non-existing property (null ID), no edit permission'
=> [
+ $this->getNonExistingPropertyWithNullId(),
+ [ 'edit' => false, 'property-term' => true,
'createpage' => true, 'property-create' => true, ]
+ ],
+ 'non-existing property (null ID), no read permission'
=> [
+ $this->getNonExistingPropertyWithNullId(),
+ [ 'read' => false, 'edit' => true,
'property-term' => true, 'createpage' => true, 'property-create' => true, ]
+ ],
+ ];
+ }
+
public function
testGivenUnknownPermission_getPermissionStatusForEntityThrowsException() {
$checker = $this->getPermissionChecker();
--
To view, visit https://gerrit.wikimedia.org/r/365259
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I294f7bcfbd0e10b6a6ad82c80ddd8a4ed5eb637e
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: WMDE-leszek <leszek.mani...@wikimedia.de>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
