From b046b93d93439c425454d49ffa086686422d665b Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sat, 12 Dec 2015 20:35:22 -0800 Subject: [PATCH] Avoid "CAS updated failed" errors on Special:Preferences double post * This does the same thing ApiOptions does to avoid these errors. A new getInstanceForUpdate() method is now in the User class to simplify this pattern. * Avoid overriding $user in ApiOptions for code readability. * Fixed IDEA errors around Preferences::getFormObject() return type. Bug: T95839 Change-Id: If2385b7486c043bd70d7031ff35e37dfb079a4d2 --- includes/Preferences.php | 2 +- includes/api/ApiOptions.php | 18 ++++------- includes/specials/SpecialPreferences.php | 5 ++- includes/user/User.php | 22 +++++++++++++ tests/phpunit/includes/api/ApiOptionsTest.php | 32 +++++++++++-------- 5 files changed, 51 insertions(+), 28 deletions(-) diff --git a/includes/Preferences.php b/includes/Preferences.php index 327d19ae37..ad25fa8d91 100644 --- a/includes/Preferences.php +++ b/includes/Preferences.php @@ -1273,7 +1273,7 @@ class Preferences { * @param IContextSource $context * @param string $formClass * @param array $remove Array of items to remove - * @return HtmlForm + * @return PreferencesForm|HtmlForm */ static function getFormObject( $user, diff --git a/includes/api/ApiOptions.php b/includes/api/ApiOptions.php index 74ce0539d5..7a905275ec 100644 --- a/includes/api/ApiOptions.php +++ b/includes/api/ApiOptions.php @@ -35,14 +35,10 @@ class ApiOptions extends ApiBase { * Changes preferences of the current user. */ public function execute() { - $user = $this->getUser(); - - if ( $user->isAnon() ) { + if ( $this->getUser()->isAnon() ) { $this->dieUsage( 'Anonymous users cannot change preferences', 'notloggedin' ); - } - - if ( !$user->isAllowed( 'editmyoptions' ) ) { - $this->dieUsage( 'You don\'t have permission to edit your options', 'permissiondenied' ); + } elseif ( !$this->getUser()->isAllowed( 'editmyoptions' ) ) { + $this->dieUsage( "You don't have permission to edit your options", 'permissiondenied' ); } $params = $this->extractRequestParams(); @@ -53,11 +49,9 @@ class ApiOptions extends ApiBase { } // Load the user from the master to reduce CAS errors on double post (T95839) - if ( wfGetLB()->getServerCount() > 1 ) { - $user = User::newFromId( $user->getId() ); - if ( !$user->loadFromId( User::READ_EXCLUSIVE ) ) { - $this->dieUsage( 'Anonymous users cannot change preferences', 'notloggedin' ); - } + $user = $this->getUser()->getInstanceForUpdate(); + if ( !$user ) { + $this->dieUsage( 'Anonymous users cannot change preferences', 'notloggedin' ); } if ( $params['reset'] ) { diff --git a/includes/specials/SpecialPreferences.php b/includes/specials/SpecialPreferences.php index 3b5b9c90ef..49ab6d5e98 100644 --- a/includes/specials/SpecialPreferences.php +++ b/includes/specials/SpecialPreferences.php @@ -65,7 +65,10 @@ class SpecialPreferences extends SpecialPage { $this->addHelpLink( 'Help:Preferences' ); - $htmlForm = Preferences::getFormObject( $this->getUser(), $this->getContext() ); + // Load the user from the master to reduce CAS errors on double post (T95839) + $user = $this->getUser()->getInstanceForUpdate() ?: $this->getUser(); + + $htmlForm = Preferences::getFormObject( $user, $this->getContext() ); $htmlForm->setSubmitCallback( array( 'Preferences', 'tryUISubmit' ) ); $sectionTitles = $htmlForm->getPreferenceSections(); diff --git a/includes/user/User.php b/includes/user/User.php index 1de4008b99..c6d215d9ad 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -5321,6 +5321,28 @@ class User implements IDBAccessObject { } } + /** + * Get a new instance of this user that was loaded from the master via a locking read + * + * Use this instead of the main context User when updating that user. This avoids races + * where that user was loaded from a slave or even the master but without proper locks. + * + * @return User|null Returns null if the user was not found in the DB + * @since 1.27 + */ + public function getInstanceForUpdate() { + if ( !$this->getId() ) { + return null; // anon + } + + $user = self::newFromId( $this->getId() ); + if ( !$user->loadFromId( self::READ_EXCLUSIVE ) ) { + return null; + } + + return $user; + } + /** * Checks if two user objects point to the same user. * diff --git a/tests/phpunit/includes/api/ApiOptionsTest.php b/tests/phpunit/includes/api/ApiOptionsTest.php index 51154ae398..fff05c76bf 100644 --- a/tests/phpunit/includes/api/ApiOptionsTest.php +++ b/tests/phpunit/includes/api/ApiOptionsTest.php @@ -36,6 +36,10 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->any() ) ->method( 'getOptionKinds' )->will( $this->returnCallback( array( $this, 'getOptionKinds' ) ) ); + // No actual DB data + $this->mUserMock->expects( $this->any() ) + ->method( 'getInstanceForUpdate' )->will( $this->returnValue( $this->mUserMock ) ); + // Create a new context $this->mContext = new DerivativeContext( new RequestContext() ); $this->mContext->getContext()->setTitle( Title::newFromText( 'Test' ) ); @@ -283,21 +287,21 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->at( 2 ) ) ->method( 'getOptions' ); - $this->mUserMock->expects( $this->at( 4 ) ) + $this->mUserMock->expects( $this->at( 5 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'willBeNull' ), $this->identicalTo( null ) ); - $this->mUserMock->expects( $this->at( 5 ) ) + $this->mUserMock->expects( $this->at( 6 ) ) ->method( 'getOptions' ); - $this->mUserMock->expects( $this->at( 6 ) ) + $this->mUserMock->expects( $this->at( 7 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'willBeEmpty' ), $this->equalTo( '' ) ); - $this->mUserMock->expects( $this->at( 7 ) ) + $this->mUserMock->expects( $this->at( 8 ) ) ->method( 'getOptions' ); - $this->mUserMock->expects( $this->at( 8 ) ) + $this->mUserMock->expects( $this->at( 9 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'willBeHappy' ), $this->equalTo( 'Happy' ) ); @@ -317,17 +321,17 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->once() ) ->method( 'resetOptions' ); - $this->mUserMock->expects( $this->at( 4 ) ) + $this->mUserMock->expects( $this->at( 5 ) ) ->method( 'getOptions' ); - $this->mUserMock->expects( $this->at( 5 ) ) + $this->mUserMock->expects( $this->at( 6 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'willBeHappy' ), $this->equalTo( 'Happy' ) ); - $this->mUserMock->expects( $this->at( 6 ) ) + $this->mUserMock->expects( $this->at( 7 ) ) ->method( 'getOptions' ); - $this->mUserMock->expects( $this->at( 7 ) ) + $this->mUserMock->expects( $this->at( 8 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'name' ), $this->equalTo( 'value' ) ); @@ -350,19 +354,19 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->never() ) ->method( 'resetOptions' ); - $this->mUserMock->expects( $this->at( 3 ) ) + $this->mUserMock->expects( $this->at( 4 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'testmultiselect-opt1' ), $this->identicalTo( true ) ); - $this->mUserMock->expects( $this->at( 4 ) ) + $this->mUserMock->expects( $this->at( 5 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'testmultiselect-opt2' ), $this->identicalTo( null ) ); - $this->mUserMock->expects( $this->at( 5 ) ) + $this->mUserMock->expects( $this->at( 6 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'testmultiselect-opt3' ), $this->identicalTo( false ) ); - $this->mUserMock->expects( $this->at( 6 ) ) + $this->mUserMock->expects( $this->at( 7 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'testmultiselect-opt4' ), $this->identicalTo( false ) ); @@ -429,7 +433,7 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->never() ) ->method( 'resetOptions' ); - $this->mUserMock->expects( $this->at( 3 ) ) + $this->mUserMock->expects( $this->once() ) ->method( 'setOption' ) ->with( $this->equalTo( 'userjs-option' ), $this->equalTo( '1' ) ); -- 2.20.1