Daniel Kinzler has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/397899 )
Change subject: Make wbsetclaim and friends check entity permissions.
......................................................................
Make wbsetclaim and friends check entity permissions.
wbsetclaim, wbsetclaimvalue, wbsetqualifier, and wbsetreference did not
check user permissions against the actions required by the respective ChangeOp.
Since we currently do not have special permissions defined for modifying
Statements,
this was not a problem in practice. BUt it's an incosistency that may lead to
nasty surprises down the road.
Change-Id: Ie0d83165508723f89b3a6e9bac6c4704237c7d5a
---
M repo/includes/Api/CreateClaim.php
M repo/includes/Api/SetClaim.php
M repo/includes/Api/SetClaimValue.php
M repo/includes/Api/SetQualifier.php
M repo/includes/Api/SetReference.php
M repo/includes/Api/StatementModificationHelper.php
M repo/tests/phpunit/includes/Api/StatementModificationHelperTest.php
7 files changed, 105 insertions(+), 5 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/99/397899/1
diff --git a/repo/includes/Api/CreateClaim.php
b/repo/includes/Api/CreateClaim.php
index 1224620..4ced0b3 100644
--- a/repo/includes/Api/CreateClaim.php
+++ b/repo/includes/Api/CreateClaim.php
@@ -95,6 +95,7 @@
/* @var ChangeOpMainSnak $changeOp */
$changeOp = $this->statementChangeOpFactory->newSetMainSnakOp(
'', $snak );
+ $this->modificationHelper->checkPermissions( $entity,
$this->getUser(), $changeOp );
$this->modificationHelper->applyChangeOp( $changeOp, $entity,
$summary );
$statement =
$entity->getStatements()->getFirstStatementWithGuid(
$changeOp->getStatementGuid() );
diff --git a/repo/includes/Api/SetClaim.php b/repo/includes/Api/SetClaim.php
index 7f0a164..37cc663 100644
--- a/repo/includes/Api/SetClaim.php
+++ b/repo/includes/Api/SetClaim.php
@@ -132,6 +132,7 @@
$index = isset( $params['index'] ) ? $params['index'] : null;
$changeop = $this->statementChangeOpFactory->newSetStatementOp(
$statement, $index );
+ $this->modificationHelper->checkPermissions( $entity,
$this->getUser(), $changeop );
$this->modificationHelper->applyChangeOp( $changeop, $entity,
$summary );
$status = $this->entitySavingHelper->attemptSaveEntity(
$entity, $summary );
diff --git a/repo/includes/Api/SetClaimValue.php
b/repo/includes/Api/SetClaimValue.php
index ed76bab..e3d00ed 100644
--- a/repo/includes/Api/SetClaimValue.php
+++ b/repo/includes/Api/SetClaimValue.php
@@ -96,6 +96,7 @@
$changeOp = $this->statementChangeOpFactory->newSetMainSnakOp(
$guid, $snak );
+ $this->modificationHelper->checkPermissions( $entity,
$this->getUser(), $changeOp );
$this->modificationHelper->applyChangeOp( $changeOp, $entity,
$summary );
$status = $this->entitySavingHelper->attemptSaveEntity(
$entity, $summary );
diff --git a/repo/includes/Api/SetQualifier.php
b/repo/includes/Api/SetQualifier.php
index 34be6a2..07dc96e 100644
--- a/repo/includes/Api/SetQualifier.php
+++ b/repo/includes/Api/SetQualifier.php
@@ -102,6 +102,7 @@
}
$changeOp = $this->getChangeOp();
+ $this->modificationHelper->checkPermissions( $entity,
$this->getUser(), $changeOp );
$this->modificationHelper->applyChangeOp( $changeOp, $entity,
$summary );
$status = $this->entitySavingHelper->attemptSaveEntity(
$entity, $summary );
diff --git a/repo/includes/Api/SetReference.php
b/repo/includes/Api/SetReference.php
index 8f4d060..5626a44 100644
--- a/repo/includes/Api/SetReference.php
+++ b/repo/includes/Api/SetReference.php
@@ -129,6 +129,7 @@
$newReference = new Reference( $snakList );
$changeOp = $this->getChangeOp( $newReference );
+ $this->modificationHelper->checkPermissions( $entity,
$this->getUser(), $changeOp );
$this->modificationHelper->applyChangeOp( $changeOp, $entity,
$summary );
$status = $this->entitySavingHelper->attemptSaveEntity(
$entity, $summary );
diff --git a/repo/includes/Api/StatementModificationHelper.php
b/repo/includes/Api/StatementModificationHelper.php
index 78c729c..39bc1ee 100644
--- a/repo/includes/Api/StatementModificationHelper.php
+++ b/repo/includes/Api/StatementModificationHelper.php
@@ -7,6 +7,8 @@
use LogicException;
use OutOfBoundsException;
use ApiUsageException;
+use Status;
+use User;
use Wikibase\Repo\ChangeOp\ChangeOp;
use Wikibase\Repo\ChangeOp\ChangeOpException;
use Wikibase\Repo\ChangeOp\ChangeOpValidationException;
@@ -21,6 +23,7 @@
use Wikibase\DataModel\Statement\Statement;
use Wikibase\DataModel\Statement\StatementListProvider;
use Wikibase\Repo\SnakFactory;
+use Wikibase\Repo\Store\EntityPermissionChecker;
use Wikibase\Summary;
/**
@@ -46,23 +49,31 @@
private $guidValidator;
/**
- * @var ApiErrorReporter
- *
+ * @var EntityPermissionChecker
+ */
+ private $permissionChecker;
+
+ /**
* @param SnakFactory $snakFactory
* @param EntityIdParser $entityIdParser
* @param StatementGuidValidator $guidValidator
* @param ApiErrorReporter $errorReporter
+ * @param EntityPermissionChecker $permissionChecker
+ * @internal param ApiErrorReporter $
+ *
*/
public function __construct(
SnakFactory $snakFactory,
EntityIdParser $entityIdParser,
StatementGuidValidator $guidValidator,
- ApiErrorReporter $errorReporter
+ ApiErrorReporter $errorReporter,
+ EntityPermissionChecker $permissionChecker
) {
$this->snakFactory = $snakFactory;
$this->entityIdParser = $entityIdParser;
$this->guidValidator = $guidValidator;
$this->errorReporter = $errorReporter;
+ $this->permissionChecker = $permissionChecker;
}
/**
@@ -165,6 +176,32 @@
}
/**
+ * Check permission to apply the ChangeOp. If permission checks fail,
+ * this calls ApiErrorReporter::dieStatus() which causes the an
exception
+ * that lets the API request fail.
+ *
+ * @param EntityDocument $entity the entity to check
+ * @param User $user User doing the action
+ * @param ChangeOp $changeOp
+ */
+ public function checkPermissions( EntityDocument $entity, User $user,
ChangeOp $changeOp ) {
+ $status = Status::newGood();
+
+ foreach ( $changeOp->getActions() as $perm ) {
+ $permStatus =
$this->permissionChecker->getPermissionStatusForEntity(
+ $user,
+ $perm,
+ $entity
+ );
+ $status->merge( $permStatus );
+ }
+
+ if ( !$status->isOK() ) {
+ $this->errorReporter->dieStatus( $status,
'permissiondenied' );
+ }
+ }
+
+ /**
* Applies the given ChangeOp to the given Entity.
* Any ChangeOpException is converted into an ApiUsageException with
the code 'modification-failed'.
*
diff --git
a/repo/tests/phpunit/includes/Api/StatementModificationHelperTest.php
b/repo/tests/phpunit/includes/Api/StatementModificationHelperTest.php
index 3241209..0c468c7 100644
--- a/repo/tests/phpunit/includes/Api/StatementModificationHelperTest.php
+++ b/repo/tests/phpunit/includes/Api/StatementModificationHelperTest.php
@@ -5,6 +5,8 @@
use ApiMain;
use DataValues\StringValue;
use RuntimeException;
+use Status;
+use User;
use ValueValidators\Result;
use Wikibase\Repo\ChangeOp\ChangeOp;
use Wikibase\Repo\ChangeOp\ChangeOpException;
@@ -19,6 +21,7 @@
use Wikibase\Repo\Api\CreateClaim;
use Wikibase\Repo\Api\StatementModificationHelper;
use Wikibase\Repo\SnakFactory;
+use Wikibase\Repo\Store\EntityPermissionChecker;
use Wikibase\Repo\WikibaseRepo;
/**
@@ -81,7 +84,8 @@
$wikibaseRepo->getSnakFactory(),
$wikibaseRepo->getEntityIdParser(),
$wikibaseRepo->getStatementGuidValidator(),
- $apiHelperFactory->getErrorReporter( $apiMain )
+ $apiHelperFactory->getErrorReporter( $apiMain ),
+ $this->newPermissionChecker()
);
return new CreateClaim(
@@ -129,6 +133,32 @@
$this->setExpectedException( RuntimeException::class,
'no-such-claim' );
$helper->getStatementFromEntity( 'unknown', $entity );
+ }
+
+ public function testCheckPermission_passesWhenPermissionGranted() {
+ $helper = $this->getNewInstance();
+
+ $changeOp = $this->getMock( ChangeOp::class );
+ $changeOp->method( 'getActions' )
+ ->will( $this->returnValue( [ 'test' ] ) );
+
+ $user = $this->getTestUser()->getUser();
+ $helper->checkPermissions( new Item(), $user, $changeOp );
+
+ $this->assertTrue( true ); // dummy, we just have to get here
+ }
+
+ public function testCheckPermission_reportsErrorWhenPermissionDenied() {
+ $helper = $this->getNewInstance();
+
+ $changeOp = $this->getMock( ChangeOp::class );
+ $changeOp->method( 'getActions' )
+ ->will( $this->returnValue( [ 'bad' ] ) );
+
+ $this->setExpectedException( RuntimeException::class,
'permissiondenied' );
+
+ $user = $this->getTestUser()->getUser();
+ $helper->checkPermissions( new Item(), $user, $changeOp );
}
public function testApplyChangeOp_validatesAndAppliesChangeOp() {
@@ -189,7 +219,8 @@
$this->getMockBuilder( SnakFactory::class
)->disableOriginalConstructor()->getMock(),
$entityIdParser,
new StatementGuidValidator( $entityIdParser ),
- $errorReporter ?: $this->newApiErrorReporter()
+ $errorReporter ?: $this->newApiErrorReporter(),
+ $this->newPermissionChecker()
);
}
@@ -211,7 +242,34 @@
throw new RuntimeException( $message );
} ) );
+ $errorReporter->method( 'dieStatus' )
+ ->will( $this->returnCallback( function ( Status
$status, $description ) {
+ throw new RuntimeException( $description . ': '
. $status->getWikiText() );
+ } ) );
+
return $errorReporter;
}
+ /**
+ * @return EntityPermissionChecker
+ */
+ private function newPermissionChecker() {
+ $permissionChecker = $this->getMockBuilder(
EntityPermissionChecker::class )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ $permissionChecker->method( 'getPermissionStatusForEntity' )
+ ->will( $this->returnCallback(
+ function ( User $user, $action, EntityDocument
$entity, $quick = '' ) {
+ if ( $action === 'bad' ) {
+ return Status::newFatal( 'bad'
);
+ } else {
+ return Status::newGood();
+ }
+ }
+ ) );
+
+ return $permissionChecker;
+ }
+
}
--
To view, visit https://gerrit.wikimedia.org/r/397899
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie0d83165508723f89b3a6e9bac6c4704237c7d5a
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <daniel.kinz...@wikimedia.de>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
