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

diff --git a/includes/Preferences.php b/includes/Preferences.php
index 327d19a..ad25fa8 100644 (file)
--- 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 74ce053..7a90527 100644 (file)
--- 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 3b5b9c9..49ab6d5 100644 (file)
--- 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 1de4008..c6d215d 100644 (file)
--- 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 51154ae..fff05c7 100644 (file)
--- 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' ) );