Merge "objectcache: Add missing @covers to unit tests"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Tue, 23 Aug 2016 07:00:55 +0000 (07:00 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Tue, 23 Aug 2016 07:00:55 +0000 (07:00 +0000)
14 files changed:
RELEASE-NOTES-1.28
autoload.php
includes/DefaultSettings.php
includes/OutputPage.php
includes/Pingback.php
includes/db/Database.php
includes/db/DatabaseMysqlBase.php
includes/db/loadbalancer/LoadBalancer.php
includes/deferred/DataUpdate.php
includes/deferred/EnqueueableDataUpdate.php [new file with mode: 0644]
includes/jobqueue/JobRunner.php
includes/objectcache/SqlBagOStuff.php
includes/user/User.php
tests/phpunit/includes/user/UserTest.php

index 5d88fbf..865e300 100644 (file)
@@ -52,6 +52,16 @@ production.
 ==== Removed and replaced external libraries ====
 
 === Bug fixes in 1.28 ===
+* (T137264) SECURITY: XSS in unclosed internal links
+* (T133147) SECURITY: Escape '<' and ']]>' in inline <style> blocks
+* (T133147) SECURITY: Require login to preview user CSS pages
+* (T132926) SECURITY: Do not allow undeleting a revision deleted file if it is
+  the top file
+* (T129738) SECURITY: Make $wgBlockDisablesLogin also restrict logged in
+  permissions
+* (T129738) SECURITY: Make blocks log users out if $wgBlockDisablesLogin is true
+* (T139670) Move 'UserGetRights' call before application of
+  Session::getAllowedUserRights()
 
 === Action API changes in 1.28 ===
 * Added 'maxarticlesize' property to action=query&meta=siteinfo which contains
@@ -72,6 +82,8 @@ production.
 === Action API internal changes in 1.28 ===
 * Added a new hook, 'ApiMakeParserOptions', to allow extensions to better
   interact with ApiParse and ApiExpandTemplates.
+* (T139565) SECURITY: API: Generate head items in the context of the given title
+* (T115333) SECURITY: Check read permission when loading page content in ApiParse
 
 === Languages updated in 1.28 ===
 
index 5457d2a..0d19056 100644 (file)
@@ -408,7 +408,7 @@ $wgAutoloadLocalClasses = [
        'EnhancedChangesList' => __DIR__ . '/includes/changes/EnhancedChangesList.php',
        'EnotifNotifyJob' => __DIR__ . '/includes/jobqueue/jobs/EnotifNotifyJob.php',
        'EnqueueJob' => __DIR__ . '/includes/jobqueue/jobs/EnqueueJob.php',
-       'EnqueueableDataUpdate' => __DIR__ . '/includes/deferred/DataUpdate.php',
+       'EnqueueableDataUpdate' => __DIR__ . '/includes/deferred/EnqueueableDataUpdate.php',
        'EraseArchivedFile' => __DIR__ . '/maintenance/eraseArchivedFile.php',
        'ErrorPageError' => __DIR__ . '/includes/exception/ErrorPageError.php',
        'EventRelayer' => __DIR__ . '/includes/libs/eventrelayer/EventRelayer.php',
index 87003f0..cbf98a3 100644 (file)
@@ -8342,7 +8342,7 @@ $wgEventRelayerConfig = [
  * PHP version, and chosen database backend. The Wikimedia Foundation shares this data with
  * MediaWiki developers to help guide future development efforts.
  *
- * For details about what data is sent, see: https://www.mediawiki.org/wiki/Pingback
+ * For details about what data is sent, see: https://www.mediawiki.org/wiki/Manual:$wgPingback
  *
  * @var bool
  * @since 1.28
index 8fb3bc2..852a4ed 100644 (file)
@@ -2859,7 +2859,6 @@ class OutputPage extends ContextSource {
 
        private function isUserCssPreview() {
                return $this->getConfig()->get( 'AllowUserCss' )
-                       && $this->getUser()->isLoggedIn()
                        && $this->getTitle()
                        && $this->getTitle()->isCssSubpage()
                        && $this->userCanPreview();
index dd68102..0039d2b 100644 (file)
@@ -117,6 +117,9 @@ class Pingback {
         *
         * This is public so we can display it in the installer
         *
+        * Developers: If you're adding a new piece of data to this, please ensure
+        * that you update https://www.mediawiki.org/wiki/Manual:$wgPingback
+        *
         * @return array
         */
        public function getSystemInfo() {
index 186a87b..6ddc9f7 100644 (file)
 abstract class DatabaseBase implements IDatabase {
        /** Number of times to re-try an operation in case of deadlock */
        const DEADLOCK_TRIES = 4;
-
        /** Minimum time to wait before retry, in microseconds */
        const DEADLOCK_DELAY_MIN = 500000;
-
        /** Maximum time to wait before retry */
        const DEADLOCK_DELAY_MAX = 1500000;
 
+       /** How long before it is worth doing a dummy query to test the connection */
+       const PING_TTL = 1.0;
+
+       /** @var string SQL query */
        protected $mLastQuery = '';
+       /** @var bool */
        protected $mDoneWrites = false;
+       /** @var string|bool */
        protected $mPHPError = false;
-
-       protected $mServer, $mUser, $mPassword, $mDBname;
+       /** @var string */
+       protected $mServer;
+       /** @var string */
+       protected $mUser;
+       /** @var string */
+       protected $mPassword;
+       /** @var string */
+       protected $mDBname;
 
        /** @var BagOStuff APC cache */
        protected $srvCache;
 
        /** @var resource Database connection */
        protected $mConn = null;
+       /** @var bool */
        protected $mOpened = false;
 
        /** @var array[] List of (callable, method name) */
@@ -61,20 +72,27 @@ abstract class DatabaseBase implements IDatabase {
        /** @var bool Whether to suppress triggering of post-commit callbacks */
        protected $suppressPostCommitCallbacks = false;
 
+       /** @var string */
        protected $mTablePrefix;
+       /** @var string */
        protected $mSchema;
+       /** @var integer */
        protected $mFlags;
+       /** @var bool */
        protected $mForeign;
+       /** @var array */
        protected $mLBInfo = [];
+       /** @var bool|null */
        protected $mDefaultBigSelects = null;
+       /** @var array|bool */
        protected $mSchemaVars = false;
        /** @var array */
        protected $mSessionVars = [];
-
+       /** @var array|null */
        protected $preparedArgs;
-
+       /** @var string|bool|null Stashed value of html_errors INI setting */
        protected $htmlErrors;
-
+       /** @var string */
        protected $delimiter = ';';
 
        /**
@@ -177,6 +195,9 @@ abstract class DatabaseBase implements IDatabase {
         */
        protected $allViews = null;
 
+       /** @var float UNIX timestamp */
+       protected $lastPing = 0.0;
+
        /** @var TransactionProfiler */
        protected $trxProfiler;
 
@@ -786,8 +807,8 @@ abstract class DatabaseBase implements IDatabase {
                $priorWritesPending = $this->writesOrCallbacksPending();
                $this->mLastQuery = $sql;
 
-               $isWriteQuery = $this->isWriteQuery( $sql );
-               if ( $isWriteQuery ) {
+               $isWrite = $this->isWriteQuery( $sql );
+               if ( $isWrite ) {
                        $reason = $this->getReadOnlyReason();
                        if ( $reason !== false ) {
                                throw new DBReadOnlyError( $this, "Database is read-only: $reason" );
@@ -820,31 +841,12 @@ abstract class DatabaseBase implements IDatabase {
                }
 
                # Keep track of whether the transaction has write queries pending
-               if ( $this->mTrxLevel && !$this->mTrxDoneWrites && $isWriteQuery ) {
+               if ( $this->mTrxLevel && !$this->mTrxDoneWrites && $isWrite ) {
                        $this->mTrxDoneWrites = true;
                        $this->getTransactionProfiler()->transactionWritingIn(
                                $this->mServer, $this->mDBname, $this->mTrxShortId );
                }
 
-               $isMaster = !is_null( $this->getLBInfo( 'master' ) );
-               # generalizeSQL will probably cut down the query to reasonable
-               # logging size most of the time. The substr is really just a sanity check.
-               if ( $isMaster ) {
-                       $queryProf = 'query-m: ' . substr( DatabaseBase::generalizeSQL( $sql ), 0, 255 );
-                       $totalProf = 'DatabaseBase::query-master';
-               } else {
-                       $queryProf = 'query: ' . substr( DatabaseBase::generalizeSQL( $sql ), 0, 255 );
-                       $totalProf = 'DatabaseBase::query';
-               }
-               # Include query transaction state
-               $queryProf .= $this->mTrxShortId ? " [TRX#{$this->mTrxShortId}]" : "";
-
-               $profiler = Profiler::instance();
-               if ( !$profiler instanceof ProfilerStub ) {
-                       $totalProfSection = $profiler->scopedProfileIn( $totalProf );
-                       $queryProfSection = $profiler->scopedProfileIn( $queryProf );
-               }
-
                if ( $this->debug() ) {
                        wfDebugLog( 'queries', sprintf( "%s: %s", $this->mDBname, $commentedSql ) );
                }
@@ -852,15 +854,8 @@ abstract class DatabaseBase implements IDatabase {
                # Avoid fatals if close() was called
                $this->assertOpen();
 
-               # Do the query and handle errors
-               $startTime = microtime( true );
-               $ret = $this->doQuery( $commentedSql );
-               $queryRuntime = microtime( true ) - $startTime;
-               # Log the query time and feed it into the DB trx profiler
-               $this->getTransactionProfiler()->recordQueryCompletion(
-                       $queryProf, $startTime, $isWriteQuery, $this->affectedRows() );
-
-               MWDebug::query( $sql, $fname, $isMaster, $queryRuntime );
+               # Send the query to the server
+               $ret = $this->doProfiledQuery( $sql, $commentedSql, $isWrite, $fname );
 
                # Try reconnecting if the connection was lost
                if ( false === $ret && $this->wasErrorReissuable() ) {
@@ -881,12 +876,7 @@ abstract class DatabaseBase implements IDatabase {
                                        $this->reportQueryError( $lastError, $lastErrno, $sql, $fname );
                                } else {
                                        # Should be safe to silently retry the query
-                                       $startTime = microtime( true );
-                                       $ret = $this->doQuery( $commentedSql );
-                                       $queryRuntime = microtime( true ) - $startTime;
-                                       # Log the query time and feed it into the DB trx profiler
-                                       $this->getTransactionProfiler()->recordQueryCompletion(
-                                               $queryProf, $startTime, $isWriteQuery, $this->affectedRows() );
+                                       $ret = $this->doProfiledQuery( $sql, $commentedSql, $isWrite, $fname );
                                }
                        } else {
                                wfDebug( "Failed\n" );
@@ -911,16 +901,47 @@ abstract class DatabaseBase implements IDatabase {
 
                $res = $this->resultObject( $ret );
 
-               // Destroy profile sections in the opposite order to their creation
-               ScopedCallback::consume( $queryProfSection );
-               ScopedCallback::consume( $totalProfSection );
+               return $res;
+       }
 
-               if ( $isWriteQuery && $this->mTrxLevel ) {
-                       $this->mTrxWriteDuration += $queryRuntime;
-                       $this->mTrxWriteCallers[] = $fname;
+       private function doProfiledQuery( $sql, $commentedSql, $isWrite, $fname ) {
+               $isMaster = !is_null( $this->getLBInfo( 'master' ) );
+               # generalizeSQL() will probably cut down the query to reasonable
+               # logging size most of the time. The substr is really just a sanity check.
+               if ( $isMaster ) {
+                       $queryProf = 'query-m: ' . substr( DatabaseBase::generalizeSQL( $sql ), 0, 255 );
+               } else {
+                       $queryProf = 'query: ' . substr( DatabaseBase::generalizeSQL( $sql ), 0, 255 );
                }
 
-               return $res;
+               # Include query transaction state
+               $queryProf .= $this->mTrxShortId ? " [TRX#{$this->mTrxShortId}]" : "";
+
+               $profiler = Profiler::instance();
+               if ( !( $profiler instanceof ProfilerStub ) ) {
+                       $queryProfSection = $profiler->scopedProfileIn( $queryProf );
+               }
+
+               $startTime = microtime( true );
+               $ret = $this->doQuery( $commentedSql );
+               $queryRuntime = microtime( true ) - $startTime;
+
+               unset( $queryProfSection ); // profile out (if set)
+
+               if ( $ret !== false ) {
+                       $this->lastPing = $startTime;
+                       if ( $isWrite && $this->mTrxLevel ) {
+                               $this->mTrxWriteDuration += $queryRuntime;
+                               $this->mTrxWriteCallers[] = $fname;
+                       }
+               }
+
+               $this->getTransactionProfiler()->recordQueryCompletion(
+                       $queryProf, $startTime, $isWrite, $this->affectedRows()
+               );
+               MWDebug::query( $sql, $fname, $isMaster, $queryRuntime );
+
+               return $ret;
        }
 
        private function canRecoverFromDisconnect( $sql, $priorWritesPending ) {
@@ -2926,6 +2947,9 @@ abstract class DatabaseBase implements IDatabase {
        }
 
        public function ping() {
+               if ( $this->isOpen() && ( microtime( true ) - $this->lastPing ) < self::PING_TTL ) {
+                       return true;
+               }
                try {
                        // This will reconnect if possible, or error out if not
                        $this->query( "SELECT 1 AS ping", __METHOD__ );
@@ -2939,8 +2963,18 @@ abstract class DatabaseBase implements IDatabase {
         * @return bool
         */
        protected function reconnect() {
-               # Stub. Not essential to override.
-               return true;
+               $this->closeConnection();
+               $this->mOpened = false;
+               $this->mConn = false;
+               try {
+                       $this->open( $this->mServer, $this->mUser, $this->mPassword, $this->mDBname );
+                       $this->lastPing = microtime( true );
+                       $ok = true;
+               } catch ( DBConnectionError $e ) {
+                       $ok = false;
+               }
+
+               return $ok;
        }
 
        public function getSessionLagStatus() {
index d1ebe62..fa3756c 100644 (file)
@@ -599,15 +599,6 @@ abstract class DatabaseMysqlBase extends Database {
                return strlen( $name ) && $name[0] == '`' && substr( $name, -1, 1 ) == '`';
        }
 
-       function reconnect() {
-               $this->closeConnection();
-               $this->mOpened = false;
-               $this->mConn = false;
-               $this->open( $this->mServer, $this->mUser, $this->mPassword, $this->mDBname );
-
-               return true;
-       }
-
        function getLag() {
                if ( $this->getLagDetectionMethod() === 'pt-heartbeat' ) {
                        return $this->getLagFromPtHeartbeat();
index f045acf..13a0879 100644 (file)
@@ -1090,7 +1090,7 @@ class LoadBalancer {
        public function approveMasterChanges( array $options ) {
                $limit = isset( $options['maxWriteDuration'] ) ? $options['maxWriteDuration'] : 0;
                $this->forEachOpenMasterConnection( function ( DatabaseBase $conn ) use ( $limit ) {
-                       // If atomic section or explicit transactions are still open, some caller must have
+                       // If atomic sections or explicit transactions are still open, some caller must have
                        // caught an exception but failed to properly rollback any changes. Detect that and
                        // throw and error (causing rollback).
                        if ( $conn->explicitTrxActive() ) {
@@ -1108,6 +1108,14 @@ class LoadBalancer {
                                        wfMessage( 'transaction-duration-limit-exceeded', $time, $limit )->text()
                                );
                        }
+                       // If a connection sits idle while slow queries execute on another, that connection
+                       // may end up dropped before the commit round is reached. Ping servers to detect this.
+                       if ( $conn->writesOrCallbacksPending() && !$conn->ping() ) {
+                               throw new DBTransactionError(
+                                       $conn,
+                                       "A connection to the {$conn->getDBname()} database was lost before commit."
+                               );
+                       }
                } );
        }
 
index 5b84ca9..281ac24 100644 (file)
@@ -155,18 +155,3 @@ abstract class DataUpdate implements DeferrableUpdate {
                return $remaining;
        }
 }
-
-/**
- * Interface that marks a DataUpdate as enqueuable via the JobQueue
- *
- * Such updates must be representable using IJobSpecification, so that
- * they can be serialized into jobs and enqueued for later execution
- *
- * @since 1.27
- */
-interface EnqueueableDataUpdate {
-       /**
-        * @return array (wiki => wiki ID, job => IJobSpecification)
-        */
-       public function getAsJobSpecification();
-}
diff --git a/includes/deferred/EnqueueableDataUpdate.php b/includes/deferred/EnqueueableDataUpdate.php
new file mode 100644 (file)
index 0000000..ffeb740
--- /dev/null
@@ -0,0 +1,15 @@
+<?php
+/**
+ * Interface that marks a DataUpdate as enqueuable via the JobQueue
+ *
+ * Such updates must be representable using IJobSpecification, so that
+ * they can be serialized into jobs and enqueued for later execution
+ *
+ * @since 1.27
+ */
+interface EnqueueableDataUpdate {
+       /**
+        * @return array (wiki => wiki ID, job => IJobSpecification)
+        */
+       public function getAsJobSpecification();
+}
index 4b906a7..ad0abf2 100644 (file)
@@ -528,17 +528,6 @@ class JobRunner implements LoggerAwareInterface {
                        $lb->waitForAll( $pos );
                }
 
-               $fname = __METHOD__;
-               // Re-ping all masters with transactions. This throws DBError if some
-               // connection died while waiting on locks/slaves, triggering a rollback.
-               wfGetLBFactory()->forEachLB( function( LoadBalancer $lb ) use ( $fname ) {
-                       $lb->forEachOpenConnection( function( IDatabase $conn ) use ( $fname ) {
-                               if ( $conn->writesOrCallbacksPending() ) {
-                                       $conn->ping();
-                               }
-                       } );
-               } );
-
                // Actually commit the DB master changes
                wfGetLBFactory()->commitMasterChanges( __METHOD__ );
 
index 84936a4..9ab28aa 100644 (file)
@@ -21,6 +21,8 @@
  * @ingroup Cache
  */
 
+use \MediaWiki\MediaWikiServices;
+
 /**
  * Class to store objects in the database
  *
@@ -276,6 +278,7 @@ class SqlBagOStuff extends BagOStuff {
                        if ( isset( $dataRows[$key] ) ) { // HIT?
                                $row = $dataRows[$key];
                                $this->debug( "get: retrieved data; expiry time is " . $row->exptime );
+                               $db = null;
                                try {
                                        $db = $this->getDB( $row->serverIndex );
                                        if ( $this->isExpired( $db, $row->exptime ) ) { // MISS
@@ -284,7 +287,7 @@ class SqlBagOStuff extends BagOStuff {
                                                $values[$key] = $this->unserialize( $db->decodeBlob( $row->value ) );
                                        }
                                } catch ( DBQueryError $e ) {
-                                       $this->handleWriteError( $e, $row->serverIndex );
+                                       $this->handleWriteError( $e, $db, $row->serverIndex );
                                }
                        } else { // MISS
                                $this->debug( 'get: no matching rows' );
@@ -306,10 +309,11 @@ class SqlBagOStuff extends BagOStuff {
                $result = true;
                $exptime = (int)$expiry;
                foreach ( $keysByTable as $serverIndex => $serverKeys ) {
+                       $db = null;
                        try {
                                $db = $this->getDB( $serverIndex );
                        } catch ( DBError $e ) {
-                               $this->handleWriteError( $e, $serverIndex );
+                               $this->handleWriteError( $e, $db, $serverIndex );
                                $result = false;
                                continue;
                        }
@@ -342,7 +346,7 @@ class SqlBagOStuff extends BagOStuff {
                                                __METHOD__
                                        );
                                } catch ( DBError $e ) {
-                                       $this->handleWriteError( $e, $serverIndex );
+                                       $this->handleWriteError( $e, $db, $serverIndex );
                                        $result = false;
                                }
 
@@ -364,6 +368,7 @@ class SqlBagOStuff extends BagOStuff {
 
        protected function cas( $casToken, $key, $value, $exptime = 0 ) {
                list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
+               $db = null;
                try {
                        $db = $this->getDB( $serverIndex );
                        $exptime = intval( $exptime );
@@ -394,7 +399,7 @@ class SqlBagOStuff extends BagOStuff {
                                __METHOD__
                        );
                } catch ( DBQueryError $e ) {
-                       $this->handleWriteError( $e, $serverIndex );
+                       $this->handleWriteError( $e, $db, $serverIndex );
 
                        return false;
                }
@@ -404,6 +409,7 @@ class SqlBagOStuff extends BagOStuff {
 
        public function delete( $key ) {
                list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
+               $db = null;
                try {
                        $db = $this->getDB( $serverIndex );
                        $db->delete(
@@ -411,7 +417,7 @@ class SqlBagOStuff extends BagOStuff {
                                [ 'keyname' => $key ],
                                __METHOD__ );
                } catch ( DBError $e ) {
-                       $this->handleWriteError( $e, $serverIndex );
+                       $this->handleWriteError( $e, $db, $serverIndex );
                        return false;
                }
 
@@ -420,6 +426,7 @@ class SqlBagOStuff extends BagOStuff {
 
        public function incr( $key, $step = 1 ) {
                list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
+               $db = null;
                try {
                        $db = $this->getDB( $serverIndex );
                        $step = intval( $step );
@@ -455,7 +462,7 @@ class SqlBagOStuff extends BagOStuff {
                                $newValue = null;
                        }
                } catch ( DBError $e ) {
-                       $this->handleWriteError( $e, $serverIndex );
+                       $this->handleWriteError( $e, $db, $serverIndex );
                        return null;
                }
 
@@ -473,6 +480,7 @@ class SqlBagOStuff extends BagOStuff {
 
        public function changeTTL( $key, $expiry = 0 ) {
                list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
+               $db = null;
                try {
                        $db = $this->getDB( $serverIndex );
                        $db->update(
@@ -485,7 +493,7 @@ class SqlBagOStuff extends BagOStuff {
                                return false;
                        }
                } catch ( DBError $e ) {
-                       $this->handleWriteError( $e, $serverIndex );
+                       $this->handleWriteError( $e, $db, $serverIndex );
                        return false;
                }
 
@@ -542,6 +550,7 @@ class SqlBagOStuff extends BagOStuff {
         */
        public function deleteObjectsExpiringBefore( $timestamp, $progressCallback = false ) {
                for ( $serverIndex = 0; $serverIndex < $this->numServers; $serverIndex++ ) {
+                       $db = null;
                        try {
                                $db = $this->getDB( $serverIndex );
                                $dbTimestamp = $db->timestamp( $timestamp );
@@ -604,7 +613,7 @@ class SqlBagOStuff extends BagOStuff {
                                        }
                                }
                        } catch ( DBError $e ) {
-                               $this->handleWriteError( $e, $serverIndex );
+                               $this->handleWriteError( $e, $db, $serverIndex );
                                return false;
                        }
                }
@@ -618,13 +627,14 @@ class SqlBagOStuff extends BagOStuff {
         */
        public function deleteAll() {
                for ( $serverIndex = 0; $serverIndex < $this->numServers; $serverIndex++ ) {
+                       $db = null;
                        try {
                                $db = $this->getDB( $serverIndex );
                                for ( $i = 0; $i < $this->shards; $i++ ) {
                                        $db->delete( $this->getTableNameByShard( $i ), '*', __METHOD__ );
                                }
                        } catch ( DBError $e ) {
-                               $this->handleWriteError( $e, $serverIndex );
+                               $this->handleWriteError( $e, $db, $serverIndex );
                                return false;
                        }
                }
@@ -694,26 +704,19 @@ class SqlBagOStuff extends BagOStuff {
         * Handle a DBQueryError which occurred during a write operation.
         *
         * @param DBError $exception
+        * @param IDatabase|null $db DB handle or null if connection failed
         * @param int $serverIndex
         * @throws Exception
         */
-       protected function handleWriteError( DBError $exception, $serverIndex ) {
-               if ( $exception instanceof DBConnectionError ) {
+       protected function handleWriteError( DBError $exception, IDatabase $db = null, $serverIndex ) {
+               if ( !$db ) {
                        $this->markServerDown( $exception, $serverIndex );
-               }
-               if ( $exception->db && $exception->db->wasReadOnlyError() ) {
-                       if ( $exception->db->trxLevel() ) {
-                               if ( !$this->serverInfos ) {
-                                       // Errors like deadlocks and connection drops already cause rollback.
-                                       // For consistency, we have no choice but to throw an error and trigger
-                                       // complete rollback if the main DB is also being used as the cache DB.
-                                       throw $exception;
-                               }
-
-                               try {
-                                       $exception->db->rollback( __METHOD__ );
-                               } catch ( DBError $e ) {
-                               }
+               } elseif ( $db->wasReadOnlyError() ) {
+                       if ( $db->trxLevel() && $this->usesMainDB() ) {
+                               // Errors like deadlocks and connection drops already cause rollback.
+                               // For consistency, we have no choice but to throw an error and trigger
+                               // complete rollback if the main DB is also being used as the cache DB.
+                               throw $exception;
                        }
                }
 
@@ -733,7 +736,7 @@ class SqlBagOStuff extends BagOStuff {
         * @param DBError $exception
         * @param int $serverIndex
         */
-       protected function markServerDown( $exception, $serverIndex ) {
+       protected function markServerDown( DBError $exception, $serverIndex ) {
                unset( $this->conns[$serverIndex] ); // bug T103435
 
                if ( isset( $this->connFailureTimes[$serverIndex] ) ) {
@@ -770,11 +773,19 @@ class SqlBagOStuff extends BagOStuff {
                }
        }
 
+       /**
+        * @return bool Whether the main DB is used, e.g. wfGetDB( DB_MASTER )
+        */
+       protected function usesMainDB() {
+               return !$this->serverInfos;
+       }
+
        protected function waitForSlaves() {
-               if ( !$this->serverInfos ) {
+               if ( $this->usesMainDB() ) {
                        // Main LB is used; wait for any slaves to catch up
                        try {
-                               wfGetLBFactory()->waitForReplication( [ 'wiki' => wfWikiID() ] );
+                               $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
+                               $lbFactory->waitForReplication( [ 'wiki' => wfWikiID() ] );
                                return true;
                        } catch ( DBReplicationWaitError $e ) {
                                return false;
index 8859609..a9ccc4e 100644 (file)
@@ -3134,6 +3134,7 @@ class User implements IDBAccessObject {
        public function getRights() {
                if ( is_null( $this->mRights ) ) {
                        $this->mRights = self::getGroupPermissions( $this->getEffectiveGroups() );
+                       Hooks::run( 'UserGetRights', [ $this, &$this->mRights ] );
 
                        // Deny any rights denied by the user's session, unless this
                        // endpoint has no sessions.
@@ -3144,7 +3145,6 @@ class User implements IDBAccessObject {
                                }
                        }
 
-                       Hooks::run( 'UserGetRights', [ $this, &$this->mRights ] );
                        // Force reindexation of rights when a hook has unset one of them
                        $this->mRights = array_values( array_unique( $this->mRights ) );
 
@@ -3977,7 +3977,6 @@ class User implements IDBAccessObject {
                $noPass = PasswordFactory::newInvalidPassword()->toString();
 
                $dbw = wfGetDB( DB_MASTER );
-               $inWrite = $dbw->writesOrCallbacksPending();
                $seqVal = $dbw->nextSequenceValue( 'user_user_id_seq' );
                $dbw->insert( 'user',
                        [
@@ -3996,25 +3995,17 @@ class User implements IDBAccessObject {
                        [ 'IGNORE' ]
                );
                if ( !$dbw->affectedRows() ) {
-                       // The queries below cannot happen in the same REPEATABLE-READ snapshot.
-                       // Handle this by COMMIT, if possible, or by LOCK IN SHARE MODE otherwise.
-                       if ( $inWrite ) {
-                               // Can't commit due to pending writes that may need atomicity.
-                               // This may cause some lock contention unlike the case below.
-                               $options = [ 'LOCK IN SHARE MODE' ];
-                               $flags = self::READ_LOCKING;
-                       } else {
-                               // Often, this case happens early in views before any writes when
-                               // using CentralAuth. It's should be OK to commit and break the snapshot.
-                               $dbw->commit( __METHOD__, 'flush' );
-                               $options = [];
-                               $flags = self::READ_LATEST;
-                       }
-                       $this->mId = $dbw->selectField( 'user', 'user_id',
-                               [ 'user_name' => $this->mName ], __METHOD__, $options );
+                       // Use locking reads to bypass any REPEATABLE-READ snapshot.
+                       $this->mId = $dbw->selectField(
+                               'user',
+                               'user_id',
+                               [ 'user_name' => $this->mName ],
+                               __METHOD__,
+                               [ 'LOCK IN SHARE MODE' ]
+                       );
                        $loaded = false;
                        if ( $this->mId ) {
-                               if ( $this->loadFromDatabase( $flags ) ) {
+                               if ( $this->loadFromDatabase( self::READ_LOCKING ) ) {
                                        $loaded = true;
                                }
                        }
index bd076ba..985554b 100644 (file)
@@ -92,6 +92,57 @@ class UserTest extends MediaWikiTestCase {
                $this->assertNotContains( 'nukeworld', $rights );
        }
 
+       /**
+        * @covers User::getRights
+        */
+       public function testUserGetRightsHooks() {
+               $user = new User;
+               $user->addGroup( 'unittesters' );
+               $user->addGroup( 'testwriters' );
+               $userWrapper = TestingAccessWrapper::newFromObject( $user );
+
+               $rights = $user->getRights();
+               $this->assertContains( 'test', $rights, 'sanity check' );
+               $this->assertContains( 'runtest', $rights, 'sanity check' );
+               $this->assertContains( 'writetest', $rights, 'sanity check' );
+               $this->assertNotContains( 'nukeworld', $rights, 'sanity check' );
+
+               // Add a hook manipluating the rights
+               $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'UserGetRights' => [ function ( $user, &$rights ) {
+                       $rights[] = 'nukeworld';
+                       $rights = array_diff( $rights, [ 'writetest' ] );
+               } ] ] );
+
+               $userWrapper->mRights = null;
+               $rights = $user->getRights();
+               $this->assertContains( 'test', $rights );
+               $this->assertContains( 'runtest', $rights );
+               $this->assertNotContains( 'writetest', $rights );
+               $this->assertContains( 'nukeworld', $rights );
+
+               // Add a Session that limits rights
+               $mock = $this->getMockBuilder( stdclass::class )
+                       ->setMethods( [ 'getAllowedUserRights', 'deregisterSession', 'getSessionId' ] )
+                       ->getMock();
+               $mock->method( 'getAllowedUserRights' )->willReturn( [ 'test', 'writetest' ] );
+               $mock->method( 'getSessionId' )->willReturn(
+                       new MediaWiki\Session\SessionId( str_repeat( 'X', 32 ) )
+               );
+               $session = MediaWiki\Session\TestUtils::getDummySession( $mock );
+               $mockRequest = $this->getMockBuilder( FauxRequest::class )
+                       ->setMethods( [ 'getSession' ] )
+                       ->getMock();
+               $mockRequest->method( 'getSession' )->willReturn( $session );
+               $userWrapper->mRequest = $mockRequest;
+
+               $userWrapper->mRights = null;
+               $rights = $user->getRights();
+               $this->assertContains( 'test', $rights );
+               $this->assertNotContains( 'runtest', $rights );
+               $this->assertNotContains( 'writetest', $rights );
+               $this->assertNotContains( 'nukeworld', $rights );
+       }
+
        /**
         * @dataProvider provideGetGroupsWithPermission
         * @covers User::getGroupsWithPermission