Hoo man has uploaded a new change for review.
https://gerrit.wikimedia.org/r/270255
Change subject: Introduce a "repoUserValidationMethod" client setting
......................................................................
Introduce a "repoUserValidationMethod" client setting
Used in UpdateRepo to ensure the user on client is the same
as the one on repo with the same name.
Supports two modes:
* centralauth: Make use of CentralAuthUser.
* assumeSame: Assume that if a user with that name exists on
repo, it will be the same.
This way we might be able to remove the binding against CentralAuth,
now that the SULF has been finished.
Also needed in order to be able to write tests for
UpdateRepoHookHandlers without having to work with CentralAuth.
Change-Id: I2f9c0c5743a66b1e62f633ee3c64dad407fef424
---
M client/config/WikibaseClient.default.php
M client/includes/Hooks/UpdateRepoHookHandlers.php
M client/includes/UpdateRepo/UpdateRepo.php
M client/includes/UpdateRepo/UpdateRepoOnMove.php
M client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnDeleteTest.php
M client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnMoveTest.php
M docs/options.wiki
M repo/includes/UpdateRepo/UpdateRepoJob.php
8 files changed, 128 insertions(+), 44 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/55/270255/1
diff --git a/client/config/WikibaseClient.default.php
b/client/config/WikibaseClient.default.php
index 7c68e48..cd9a1e4 100644
--- a/client/config/WikibaseClient.default.php
+++ b/client/config/WikibaseClient.default.php
@@ -297,5 +297,9 @@
return $otherProjectsSitesProvider->getOtherProjectsSiteIds(
$settings->getSetting( 'siteLinkGroups' ) );
};
+ $defaults['repoUserValidationMethod'] = function( SettingsArray
$settings ) {
+ return class_exists( 'CentralAuthUser' ) ? 'centralauth' :
'assumeSame';
+ };
+
return $defaults;
} );
diff --git a/client/includes/Hooks/UpdateRepoHookHandlers.php
b/client/includes/Hooks/UpdateRepoHookHandlers.php
index e5e771a..ce330bb 100644
--- a/client/includes/Hooks/UpdateRepoHookHandlers.php
+++ b/client/includes/Hooks/UpdateRepoHookHandlers.php
@@ -59,6 +59,11 @@
private $propagateChangesToRepo;
/**
+ * @var string
+ */
+ private $userValidationMethod;
+
+ /**
* @return UpdateRepoHookHandlers|boolean
*/
private static function newFromGlobalState() {
@@ -83,7 +88,8 @@
$siteLinkLookup,
$settings->getSetting( 'repoDatabase' ),
$settings->getSetting( 'siteGlobalID' ),
- $settings->getSetting( 'propagateChangesToRepo' )
+ $settings->getSetting( 'propagateChangesToRepo' ),
+ $settings->getSetting( 'repoUserValidationMethod' )
);
}
@@ -154,6 +160,7 @@
* @param string $repoDatabase
* @param string $siteGlobalID
* @param bool $propagateChangesToRepo
+ * @param string $userValidationMethod
*/
public function __construct(
NamespaceChecker $namespaceChecker,
@@ -161,7 +168,8 @@
SiteLinkLookup $siteLinkLookup,
$repoDatabase,
$siteGlobalID,
- $propagateChangesToRepo
+ $propagateChangesToRepo,
+ $userValidationMethod
) {
$this->namespaceChecker = $namespaceChecker;
$this->jobQueueGroup = $jobQueueGroup;
@@ -170,6 +178,7 @@
$this->repoDatabase = $repoDatabase;
$this->siteGlobalID = $siteGlobalID;
$this->propagateChangesToRepo = $propagateChangesToRepo;
+ $this->userValidationMethod = $userValidationMethod;
}
/**
@@ -209,7 +218,8 @@
$this->siteLinkLookup,
$user,
$this->siteGlobalID,
- $title
+ $title,
+ $this->userValidationMethod
);
if ( !$this->shouldBePushed( $updateRepo ) ) {
@@ -254,7 +264,8 @@
$user,
$this->siteGlobalID,
$oldTitle,
- $newTitle
+ $newTitle,
+ $this->userValidationMethod
);
if ( !$this->shouldBePushed( $updateRepo ) ) {
diff --git a/client/includes/UpdateRepo/UpdateRepo.php
b/client/includes/UpdateRepo/UpdateRepo.php
index 90e3faf..2c4e30c 100644
--- a/client/includes/UpdateRepo/UpdateRepo.php
+++ b/client/includes/UpdateRepo/UpdateRepo.php
@@ -12,6 +12,7 @@
use Wikibase\DataModel\Entity\EntityId;
use Wikibase\DataModel\SiteLink;
use Wikibase\Lib\Store\SiteLinkLookup;
+use Wikimedia\Assert\Assert;
/**
* Provides logic to update the repo after certain changes have been
@@ -58,24 +59,39 @@
private $entityId = false;
/**
+ * @var string
+ */
+ private $userValidationMethod;
+
+ /**
* @param string $repoDB Database name of the repo
* @param SiteLinkLookup $siteLinkLookup
* @param User $user
* @param string $siteId Global id of the client wiki
* @param Title $title Title in the client that has been changed
+ * @param string $userValidationMethod Value of the
"repoUserValidationMethod" setting
*/
public function __construct(
$repoDB,
SiteLinkLookup $siteLinkLookup,
User $user,
$siteId,
- Title $title
+ Title $title,
+ $userValidationMethod
) {
$this->repoDB = $repoDB;
$this->siteLinkLookup = $siteLinkLookup;
$this->user = $user;
$this->siteId = $siteId;
$this->title = $title;
+
+ Assert::parameter(
+ in_array( $userValidationMethod, array( 'centralauth',
'assumeSame' ) ),
+ '$userValidationMethod',
+ "Unkown value $userValidationMethod"
+ );
+
+ $this->userValidationMethod = $userValidationMethod;
}
/**
@@ -105,11 +121,27 @@
/**
* Find out whether the user also exists on the repo and belongs to the
- * same global account (uses CentralAuth).
+ * same global account.
*
* @return bool
*/
public function userIsValidOnRepo() {
+ if ( $this->userValidationMethod === 'centralauth' ) {
+ return $this->userIsValidOnRepoCentralAuth();
+ } elseif ( $this->userValidationMethod === 'assumeSame' ) {
+ return true;
+ } else {
+ throw new RuntimeException( 'Unexpected
$this->userValidationMethod value: ' . $this->userValidationMethod );
+ }
+ }
+
+ /**
+ * Find out whether the user also exists on the repo and belongs to the
+ * same global account (uses CentralAuth).
+ *
+ * @return bool
+ */
+ public function userIsValidOnRepoCentralAuth() {
if ( !class_exists( 'CentralAuthUser' ) ) {
// We can't do anything without CentralAuth as there's
no way to verify that
// the local user equals the repo one with the same name
diff --git a/client/includes/UpdateRepo/UpdateRepoOnMove.php
b/client/includes/UpdateRepo/UpdateRepoOnMove.php
index 272ca41..ae03bed 100644
--- a/client/includes/UpdateRepo/UpdateRepoOnMove.php
+++ b/client/includes/UpdateRepo/UpdateRepoOnMove.php
@@ -28,15 +28,17 @@
* @param string $siteId Global id of the client wiki
* @param Title $oldTitle
* @param Title $newTitle
+ * @param string $userValidationMethod Value of the
"repoUserValidationMethod" setting
*/
public function __construct(
$repoDB,
SiteLinkLookup $siteLinkLookup,
User $user, $siteId,
Title $oldTitle,
- Title $newTitle
+ Title $newTitle,
+ $userValidationMethod
) {
- parent::__construct( $repoDB, $siteLinkLookup, $user, $siteId,
$oldTitle );
+ parent::__construct( $repoDB, $siteLinkLookup, $user, $siteId,
$oldTitle, $userValidationMethod );
$this->newTitle = $newTitle;
}
diff --git
a/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnDeleteTest.php
b/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnDeleteTest.php
index b2116e4..c2593eb 100644
--- a/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnDeleteTest.php
+++ b/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnDeleteTest.php
@@ -46,23 +46,31 @@
}
/**
+ * @param bool $cache Whether to cache the instance/ use a cached
instance
+ * @param string $userValidationMethod
+ *
* @return UpdateRepoOnDelete
*/
- private function getNewUpdateRepoOnDelete() {
- static $updateRepo = null;
+ private function getNewUpdateRepoOnDelete( $cache = true,
$userValidationMethod = 'assumeSame' ) {
+ static $updateRepoCached = null;
- if ( !$updateRepo ) {
- $data = $this->getFakeData();
+ if ( $updateRepoCached && $cache ) {
+ return $updateRepoCached;
+ }
- $updateRepo = new UpdateRepoOnDelete(
- $data['repoDB'],
- // Nobody knows why we need to clone over here,
but it's not working
- // without... PHP is fun!
- clone $data['siteLinkLookup'],
- $data['user'],
- $data['siteId'],
- $data['title']
- );
+ $data = $this->getFakeData();
+
+ $updateRepo = new UpdateRepoOnDelete(
+ $data['repoDB'],
+ $data['siteLinkLookup'],
+ $data['user'],
+ $data['siteId'],
+ $data['title'],
+ $userValidationMethod
+ );
+
+ if ( $cache ) {
+ $updateRepoCached = $updateRepo;
}
return $updateRepo;
@@ -100,10 +108,20 @@
return $jobQueueGroupMock;
}
- public function testUserIsValidOnRepo() {
- $updateRepo = $this->getNewUpdateRepoOnDelete();
+ /**
+ * @dataProvider userIsValidOnRepoProvider
+ */
+ public function testUserIsValidOnRepo( $expected, $userValidationMethod
) {
+ $updateRepo = $this->getNewUpdateRepoOnDelete( false,
$userValidationMethod );
- $this->assertFalse( $updateRepo->userIsValidOnRepo() );
+ $this->assertSame( $expected, $updateRepo->userIsValidOnRepo()
);
+ }
+
+ public function userIsValidOnRepoProvider() {
+ return array(
+ array( true, 'assumeSame' ),
+ array( false, 'centralauth' )
+ );
}
/**
diff --git a/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnMoveTest.php
b/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnMoveTest.php
index dbb190e..d391d0b 100644
--- a/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnMoveTest.php
+++ b/client/tests/phpunit/includes/UpdateRepo/UpdateRepoOnMoveTest.php
@@ -49,24 +49,32 @@
/**
* Get a new object which thinks we're both the repo and client
*
+ * @param bool $cache Whether to cache the instance/ use a cached
instance
+ * @param string $userValidationMethod
+ *
* @return UpdateRepoOnMove
*/
- private function getNewUpdateRepoOnMove() {
- static $updateRepo = null;
+ private function getNewUpdateRepoOnMove( $cache = true,
$userValidationMethod = 'assumeSame' ) {
+ static $updateRepoCached = null;
- if ( !$updateRepo ) {
- $moveData = $this->getFakeMoveData();
+ if ( $updateRepoCached && $cache ) {
+ return $updateRepoCached;
+ }
- $updateRepo = new UpdateRepoOnMove(
- $moveData['repoDB'],
- // Nobody knows why we need to clone over here,
but it's not working
- // without... PHP is fun!
- clone $moveData['siteLinkLookup'],
- $moveData['user'],
- $moveData['siteId'],
- $moveData['oldTitle'],
- $moveData['newTitle']
- );
+ $moveData = $this->getFakeMoveData();
+
+ $updateRepo = new UpdateRepoOnMove(
+ $moveData['repoDB'],
+ $moveData['siteLinkLookup'],
+ $moveData['user'],
+ $moveData['siteId'],
+ $moveData['oldTitle'],
+ $moveData['newTitle'],
+ $userValidationMethod
+ );
+
+ if ( $cache ) {
+ $updateRepoCached = $updateRepo;
}
return $updateRepo;
@@ -108,10 +116,20 @@
return $jobQueueGroupMock;
}
- public function testUserIsValidOnRepo() {
- $updateRepo = $this->getNewUpdateRepoOnMove();
+ /**
+ * @dataProvider userIsValidOnRepoProvider
+ */
+ public function testUserIsValidOnRepo( $expected, $userValidationMethod
) {
+ $updateRepo = $this->getNewUpdateRepoOnMove( false,
$userValidationMethod );
- $this->assertFalse( $updateRepo->userIsValidOnRepo() );
+ $this->assertSame( $expected, $updateRepo->userIsValidOnRepo()
);
+ }
+
+ public function userIsValidOnRepoProvider() {
+ return array(
+ array( true, 'assumeSame' ),
+ array( false, 'centralauth' )
+ );
}
/**
diff --git a/docs/options.wiki b/docs/options.wiki
index b189136..03f35db 100644
--- a/docs/options.wiki
+++ b/docs/options.wiki
@@ -93,6 +93,7 @@
'wikibase-property' => 'Property'
)
</poem>
+;repoUserValidationMethod: Method used to validate that a user on the repo is
the same as on client. Used when propagating changes to repo (see the
"propagateChangesToRepo" setting below). Possible values: "centralauth" (uses
CentralAuth) and "assumeSame" (just assumes that the users are the same).
Defaults to "centralauth" if CentralAuth is available, "assumeSame" otherwise.
;allowDataTransclusion: switch to enable data transclusion features like the
<nowiki>{{#property}}</nowiki> parser function and the <tt>wikibase</tt>
Scribunto module. Defaults to <tt>true</tt>.
;allowArbitraryDataAccess: switch to allow accessing arbitrary items from the
<tt>wikibase</tt> Scribunto module and the via the parser functions (instead of
just the item which is linked to the current page). Defaults to <tt>true</tt>.
;allowDataAccessInUserLanguage: switch to allow accessing data in the user's
language rather than the content language from the <tt>wikibase</tt> Scribunto
module and the via the parser functions. Useful for multilingual wikis: Allows
users to split the ParserCache by user language. Defaults to <tt>false</tt>.
diff --git a/repo/includes/UpdateRepo/UpdateRepoJob.php
b/repo/includes/UpdateRepo/UpdateRepoJob.php
index 229fa2d..04db32a 100644
--- a/repo/includes/UpdateRepo/UpdateRepoJob.php
+++ b/repo/includes/UpdateRepo/UpdateRepoJob.php
@@ -180,9 +180,7 @@
private function getUser( $name ) {
$user = User::newFromName( $name );
if ( !$user || !$user->isLoggedIn() ) {
- // This should never happen as we check with CentralAuth
- // that the user actually does exist
- wfLogWarning( "User $name doesn't exist while
CentralAuth pretends it does" );
+ wfDebugLog( 'UpdateRepo', "User $name doesn't exist." );
return false;
}
--
To view, visit https://gerrit.wikimedia.org/r/270255
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2f9c0c5743a66b1e62f633ee3c64dad407fef424
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Hoo man <h...@online.de>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
