Anomie has uploaded a new change for review.
https://gerrit.wikimedia.org/r/286926
Change subject: Use master CentralAuthUser instances when writing
......................................................................
Use master CentralAuthUser instances when writing
If we use a slave instance, then after the write is saved to the DB
invalidateCache() will reload from the slave and things will get all
screwed up.
This also applies to methods that only set dirty state, since that state
will not be sanely saveable later.
This also adds some logging for calls to "write" methods on a
slave-loaded instance, so we can see if I missed anything important. One
potential issue is that getAuthToken() can sometimes call
resetAuthToken() (which is fully a write method), but we probably don't
want to make everything using getAuthToken() have to use a master
instance.
Bug: T134246
Change-Id: I3279dc1ca1178aa3683bc35e165381d4e33a45c6
---
M includes/CentralAuthHooks.php
M includes/CentralAuthPlugin.php
M includes/CentralAuthUser.php
M includes/session/CentralAuthSessionProvider.php
M tests/phpunit/CentralAuthUserUsingDatabaseTest.php
5 files changed, 48 insertions(+), 17 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CentralAuth
refs/changes/26/286926/1
diff --git a/includes/CentralAuthHooks.php b/includes/CentralAuthHooks.php
index 6e8d9d0..58364d4 100644
--- a/includes/CentralAuthHooks.php
+++ b/includes/CentralAuthHooks.php
@@ -231,7 +231,7 @@
* @return bool
*/
static function onAddNewAccount( $user, $byEmail ) {
- $central = CentralAuthUser::getInstance( $user );
+ $central = CentralAuthUser::getMasterInstance( $user );
$central->addLocalName( wfWikiID() );
return true;
}
@@ -678,7 +678,7 @@
// Use local sessions only.
return true;
}
- $centralUser = CentralAuthUser::getInstance( $user );
+ $centralUser = CentralAuthUser::getMasterInstance( $user );
if ( $centralUser->exists() ) {
DeferredUpdates::addCallableUpdate( function() use (
$centralUser ) {
@@ -854,7 +854,7 @@
* @return bool
*/
static function onUserInvalidateEmailComplete( $user ) {
- $ca = CentralAuthUser::getInstance( $user );
+ $ca = CentralAuthUser::getMasterInstance( $user );
if ( $ca->isAttached() ) {
$ca->setEmail( '' );
$ca->setEmailAuthenticationTimestamp( null );
@@ -869,7 +869,7 @@
* @return bool
*/
static function onUserSetEmail( $user, &$email ) {
- $ca = CentralAuthUser::getInstance( $user );
+ $ca = CentralAuthUser::getMasterInstance( $user );
if ( $ca->isAttached() ) {
$ca->setEmail( $email );
}
@@ -881,7 +881,7 @@
* @return bool
*/
static function onUserSaveSettings( $user ) {
- $ca = CentralAuthUser::getInstance( $user );
+ $ca = CentralAuthUser::getMasterInstance( $user );
if ( $ca->isAttached() ) {
$ca->saveSettings();
}
@@ -894,7 +894,7 @@
* @return bool
*/
static function onUserSetEmailAuthenticationTimestamp( $user,
&$timestamp ) {
- $ca = CentralAuthUser::getInstance( $user );
+ $ca = CentralAuthUser::getMasterInstance( $user );
if ( $ca->isAttached() ) {
$ca->setEmailAuthenticationTimestamp( $timestamp );
$ca->saveSettings();
@@ -1487,7 +1487,7 @@
* @return bool
*/
public static function onDeleteAccount( User &$user ) {
- $caUser = CentralAuthUser::getInstance( $user );
+ $caUser = CentralAuthUser::getMasterInstance( $user );
if ( $caUser->isAttached() ) {
// Clean up localuser table.
diff --git a/includes/CentralAuthPlugin.php b/includes/CentralAuthPlugin.php
index e8a959f..3bb4766 100644
--- a/includes/CentralAuthPlugin.php
+++ b/includes/CentralAuthPlugin.php
@@ -211,7 +211,7 @@
$user->sulRenamed = true;
}
- $central = CentralAuthUser::getInstance( $user );
+ $central = CentralAuthUser::getMasterInstance( $user );
if ( $central->exists() && $central->isAttached() &&
$central->getEmail() != $user->getEmail() )
{
@@ -253,7 +253,7 @@
*/
public function setPassword( $user, $password ) {
// Fixme: password changes should happen through central
interface.
- $central = CentralAuthUser::getInstance( $user );
+ $central = CentralAuthUser::getMasterInstance( $user );
if ( $central->isAttached() ) {
return $central->setPassword( $password );
} else {
@@ -299,7 +299,7 @@
public function addUser( $user, $password, $email = '', $realname = ''
) {
global $wgCentralAuthAutoNew;
if ( $wgCentralAuthAutoNew ) {
- $central = CentralAuthUser::getInstance( $user );
+ $central = CentralAuthUser::getMasterInstance( $user );
if ( !$central->exists() && !$central->listUnattached()
) {
// Username is unused; set up as a global
account
// @fixme is this even vaguely reliable? pah
@@ -345,7 +345,7 @@
*/
public function initUser( &$user, $autocreate = false ) {
if ( $autocreate ) {
- $central = CentralAuthUser::getInstance( $user );
+ $central = CentralAuthUser::getMasterInstance( $user );
if ( $central->exists() ) {
$central->attach( wfWikiID(), 'login' );
$central->addLocalName( wfWikiID() );
diff --git a/includes/CentralAuthUser.php b/includes/CentralAuthUser.php
index fe7cbd3..b5db903 100644
--- a/includes/CentralAuthUser.php
+++ b/includes/CentralAuthUser.php
@@ -209,6 +209,15 @@
}
/**
+ * Test if this is a write-mode instance, and log if not.
+ */
+ private function checkWriteMode() {
+ if ( !$this->mFromMaster ) {
+ wfDebugLog( 'CentralAuth', "Setter called on a slave
instance: " . wfGetAllCallers( 10 ) );
+ }
+ }
+
+ /**
* @return DatabaseBase Master or slave based on shouldUseMasterDB()
* @throws CentralAuthReadOnlyError
*/
@@ -531,8 +540,8 @@
* Save cachable data to memcached.
*/
protected function saveToCache() {
- $obj = $this->getCacheObject();
- wfDebugLog( 'CentralAuthVerbose', "Saving user {$this->mName}
to cache." );
+ $obj = $this->getCacheObject();
+ wfDebugLog( 'CentralAuthVerbose', "Saving user {$this->mName}
to cache." );
$opts = array( 'since' =>
CentralAuthUtils::getCentralSlaveDB()->trxTimestamp() );
ObjectCache::getMainWANInstance()->set( $this->getCacheKey(),
$obj, 86400, $opts );
}
@@ -740,6 +749,7 @@
* @return bool
*/
function register( $password, $email ) {
+ $this->checkWriteMode();
$dbw = CentralAuthUtils::getCentralDB();
list( $salt, $hash ) = $this->saltedPassword( $password );
$dbw->insert(
@@ -1024,6 +1034,8 @@
* @return Status object
*/
function migrationDryRun( $passwords, &$home, &$attached, &$unattached,
&$methods ) {
+ $this->checkWriteMode(); // Because it messes with
$this->mEmail and so on
+
$home = false;
$attached = array();
$unattached = array();
@@ -1113,6 +1125,8 @@
* @param array[] $wikisToAttach
*/
private function chooseEmail( array $wikisToAttach ) {
+ $this->checkWriteMode();
+
if ( $this->mEmail ) {
return;
}
@@ -1140,6 +1154,7 @@
* @return bool Whether full automatic migration completed successfully.
*/
protected function attemptAutoMigration( $passwords = array(),
$sendToRC = true, $safe = false, $checkHome = false ) {
+ $this->checkWriteMode();
$migrationSet = $this->queryUnattached();
if ( empty( $migrationSet ) ) {
wfDebugLog( 'CentralAuth', 'no accounts to merge,
failed migration' );
@@ -1318,6 +1333,8 @@
* @return Status
*/
public function adminUnattach( $list ) {
+ $this->checkWriteMode();
+
if ( !count( $list ) ) {
return Status::newFatal(
'centralauth-admin-none-selected' );
}
@@ -1380,6 +1397,7 @@
* @return Status
*/
function adminDelete( $reason ) {
+ $this->checkWriteMode();
wfDebugLog( 'CentralAuth', "Deleting global account for user
{$this->mName}" );
$centralDB = CentralAuthUtils::getCentralDB();
@@ -1438,6 +1456,7 @@
* @return Status
*/
function adminLock() {
+ $this->checkWriteMode();
$dbw = CentralAuthUtils::getCentralDB();
$dbw->update( 'globaluser', array( 'gu_locked' => 1 ),
array( 'gu_name' => $this->mName ), __METHOD__ );
@@ -1456,6 +1475,7 @@
* @return Status
*/
function adminUnlock() {
+ $this->checkWriteMode();
$dbw = CentralAuthUtils::getCentralDB();
$dbw->update( 'globaluser', array( 'gu_locked' => 0 ),
array( 'gu_name' => $this->mName ), __METHOD__ );
@@ -1475,6 +1495,7 @@
* @return Status
*/
function adminSetHidden( $level ) {
+ $this->checkWriteMode();
$dbw = CentralAuthUtils::getCentralDB();
$dbw->update( 'globaluser', array( 'gu_hidden' => $level ),
array( 'gu_name' => $this->mName ), __METHOD__ );
@@ -1742,6 +1763,7 @@
* - completed migration state
*/
public function attach( $wikiID, $method = 'new', $sendToRC = true ) {
+ $this->checkWriteMode();
$dbw = CentralAuthUtils::getCentralDB();
$dbw->begin( __METHOD__ );
$dbw->insert( 'localuser',
@@ -2374,6 +2396,7 @@
* @return void
*/
function setEmail( $email ) {
+ $this->checkWriteMode();
$this->loadState();
if ( $this->mEmail !== $email ) {
$this->mEmail = $email;
@@ -2386,6 +2409,7 @@
* @return void
*/
function setEmailAuthenticationTimestamp( $ts ) {
+ $this->checkWriteMode();
$this->loadState();
if ( $this->mAuthenticationTimestamp !== $ts ) {
$this->mAuthenticationTimestamp = $ts;
@@ -2414,6 +2438,7 @@
* @return Bool true
*/
function setPassword( $password, $resetAuthToken = true ) {
+ $this->checkWriteMode();
list( $salt, $hash ) = $this->saltedPassword( $password );
$this->mPassword = $hash;
@@ -2495,6 +2520,8 @@
* not randomly log users out (so on logout, as is done currently, is a
good time).
*/
function resetAuthToken() {
+ $this->checkWriteMode();
+
// Load state, since its hard to reset the token without it
$this->loadState();
@@ -2507,6 +2534,8 @@
}
function saveSettings() {
+ $this->checkWriteMode();
+
if ( !$this->mStateDirty ) {
return;
}
@@ -2601,6 +2630,7 @@
* @return void
*/
function removeFromGlobalGroups( $groups ) {
+ $this->checkWriteMode();
$dbw = CentralAuthUtils::getCentralDB();
# Delete from the DB
@@ -2616,6 +2646,7 @@
* @return void
*/
function addToGlobalGroups( $groups ) {
+ $this->checkWriteMode();
$dbw = CentralAuthUtils::getCentralDB();
if ( !is_array( $groups ) ) {
diff --git a/includes/session/CentralAuthSessionProvider.php
b/includes/session/CentralAuthSessionProvider.php
index b313690..6d706fd 100644
--- a/includes/session/CentralAuthSessionProvider.php
+++ b/includes/session/CentralAuthSessionProvider.php
@@ -383,7 +383,7 @@
return;
}
- $centralUser = CentralAuthUser::getInstanceByName( $username );
+ $centralUser = CentralAuthUser::getMasterInstanceByName(
$username );
if ( !$centralUser->exists() ) {
return;
}
diff --git a/tests/phpunit/CentralAuthUserUsingDatabaseTest.php
b/tests/phpunit/CentralAuthUserUsingDatabaseTest.php
index dc3338c..11a3f88 100644
--- a/tests/phpunit/CentralAuthUserUsingDatabaseTest.php
+++ b/tests/phpunit/CentralAuthUserUsingDatabaseTest.php
@@ -96,7 +96,7 @@
* @covers CentralAuthUser::register
*/
public function testRegister() {
- $caUserNew = new CentralAuthUser( 'RegTest' );
+ $caUserNew = new CentralAuthUser( 'RegTest',
CentralAuthUser::READ_LATEST );
$ok = $caUserNew->register( "R3gT3stP@ssword", "user@localhost"
);
$this->assertSame( true, $ok );
@@ -157,7 +157,7 @@
* @covers CentralAuthUser::adminSetHidden
*/
public function testAdminLockAndHide() {
- $caUser = new CentralAuthUser( 'GlobalUser' );
+ $caUser = new CentralAuthUser( 'GlobalUser',
CentralAuthUser::READ_LATEST );
$this->assertSame( true, $caUser->exists() ); #sanity
$this->assertSame( false, $caUser->isHidden() ); #sanity
$this->assertSame( false, $caUser->isLocked() ); #sanity
@@ -193,7 +193,7 @@
* @covers CentralAuthUser::attach
*/
public function testAttach() {
- $caUser = new CentralAuthUser( 'GlobalUser' );
+ $caUser = new CentralAuthUser( 'GlobalUser',
CentralAuthUser::READ_LATEST );
$caUser->attach( 'anotherwiki', 'admin', false );
$this->assertSame( true, $caUser->exists() );
$this->assertSame( true, in_array( 'anotherwiki',
$caUser->listAttached() ) );
--
To view, visit https://gerrit.wikimedia.org/r/286926
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3279dc1ca1178aa3683bc35e165381d4e33a45c6
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/CentralAuth
Gerrit-Branch: master
Gerrit-Owner: Anomie <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
