Merge "Force user id and actor id to 0 when loading from remote wikis"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Mon, 6 May 2019 15:19:38 +0000 (15:19 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Mon, 6 May 2019 15:19:38 +0000 (15:19 +0000)
1  2 
includes/Revision/RevisionStore.php
includes/user/User.php
tests/phpunit/includes/user/UserTest.php

@@@ -278,13 -278,12 +278,13 @@@ class RevisionStor
  
        /**
         * @param int $mode DB_MASTER or DB_REPLICA
 +       * @param array $groups
         *
         * @return IDatabase
         */
 -      private function getDBConnection( $mode ) {
 +      private function getDBConnection( $mode, $groups = [] ) {
                $lb = $this->getDBLoadBalancer();
 -              return $lb->getConnection( $mode, [], $this->wikiId );
 +              return $lb->getConnection( $mode, $groups, $this->wikiId );
        }
  
        /**
                        $user = User::newFromAnyId(
                                $row->ar_user ?? null,
                                $row->ar_user_text ?? null,
-                               $row->ar_actor ?? null
+                               $row->ar_actor ?? null,
+                               $this->wikiId
                        );
                } catch ( InvalidArgumentException $ex ) {
                        wfWarn( __METHOD__ . ': ' . $title->getPrefixedDBkey() . ': ' . $ex->getMessage() );
                        $user = User::newFromAnyId(
                                $row->rev_user ?? null,
                                $row->rev_user_text ?? null,
-                               $row->rev_actor ?? null
+                               $row->rev_actor ?? null,
+                               $this->wikiId
                        );
                } catch ( InvalidArgumentException $ex ) {
                        wfWarn( __METHOD__ . ': ' . $title->getPrefixedDBkey() . ': ' . $ex->getMessage() );
                /** @var UserIdentity $user */
                $user = null;
  
-               if ( isset( $fields['user'] ) && ( $fields['user'] instanceof UserIdentity ) ) {
+               // If a user is passed in, use it if possible. We cannot use a user from a
+               // remote wiki with unsuppressed ids, due to issues described in T222212.
+               if ( isset( $fields['user'] ) &&
+                       ( $fields['user'] instanceof UserIdentity ) &&
+                       ( $this->wikiId === false ||
+                               ( !$fields['user']->getId() && !$fields['user']->getActorId() ) )
+               ) {
                        $user = $fields['user'];
                } else {
                        try {
                                $user = User::newFromAnyId(
                                        $fields['user'] ?? null,
                                        $fields['user_text'] ?? null,
-                                       $fields['actor'] ?? null
+                                       $fields['actor'] ?? null,
+                                       $this->wikiId
                                );
                        } catch ( InvalidArgumentException $ex ) {
                                $user = null;
        }
  
        /**
 -       * Get the revision before $rev in the page's history, if any.
 -       * Will return null for the first revision but also for deleted or unsaved revisions.
 -       *
 -       * MCR migration note: this replaces Revision::getPrevious
 -       *
 -       * @see Title::getPreviousRevisionID
 -       * @see PageArchive::getPreviousRevision
 +       * Implementation of getPreviousRevision and getNextRevision.
         *
         * @param RevisionRecord $rev
 -       * @param Title|null $title if known (optional)
 -       *
 +       * @param int $flags
 +       * @param string $dir 'next' or 'prev'
         * @return RevisionRecord|null
         */
 -      public function getPreviousRevision( RevisionRecord $rev, Title $title = null ) {
 +      private function getRelativeRevision( RevisionRecord $rev, $flags, $dir ) {
 +              $op = $dir === 'next' ? '>' : '<';
 +              $sort = $dir === 'next' ? 'ASC' : 'DESC';
 +
                if ( !$rev->getId() || !$rev->getPageId() ) {
                        // revision is unsaved or otherwise incomplete
                        return null;
                        return null;
                }
  
 -              if ( $title === null ) {
 -                      // this would fail for deleted revisions
 -                      $title = $this->getTitle( $rev->getPageId(), $rev->getId() );
 +              list( $dbType, ) = DBAccessObjectUtils::getDBOptions( $flags );
 +              $db = $this->getDBConnection( $dbType, [ 'contributions' ] );
 +
 +              $ts = $this->getTimestampFromId( $rev->getId(), $flags );
 +              if ( $ts === false ) {
 +                      // XXX Should this be moved into getTimestampFromId?
 +                      $ts = $db->selectField( 'archive', 'ar_timestamp',
 +                              [ 'ar_rev_id' => $rev->getId() ], __METHOD__ );
 +                      if ( $ts === false ) {
 +                              // XXX Is this reachable? How can we have a page id but no timestamp?
 +                              return null;
 +                      }
                }
 +              $ts = $db->addQuotes( $db->timestamp( $ts ) );
 +
 +              $revId = $db->selectField( 'revision', 'rev_id',
 +                      [
 +                              'rev_page' => $rev->getPageId(),
 +                              "rev_timestamp $op $ts OR (rev_timestamp = $ts AND rev_id $op {$rev->getId()})"
 +                      ],
 +                      __METHOD__,
 +                      [
 +                              'ORDER BY' => "rev_timestamp $sort, rev_id $sort",
 +                              'IGNORE INDEX' => 'rev_timestamp', // Probably needed for T159319
 +                      ]
 +              );
  
 -              $prev = $title->getPreviousRevisionID( $rev->getId() );
 -              if ( !$prev ) {
 +              if ( $revId === false ) {
                        return null;
                }
  
 -              return $this->getRevisionByTitle( $title, $prev );
 +              return $this->getRevisionById( intval( $revId ) );
        }
  
        /**
 -       * Get the revision after $rev in the page's history, if any.
 -       * Will return null for the latest revision but also for deleted or unsaved revisions.
 +       * Get the revision before $rev in the page's history, if any.
 +       * Will return null for the first revision but also for deleted or unsaved revisions.
         *
 -       * MCR migration note: this replaces Revision::getNext
 +       * MCR migration note: this replaces Revision::getPrevious
         *
 -       * @see Title::getNextRevisionID
 +       * @see Title::getPreviousRevisionID
 +       * @see PageArchive::getPreviousRevision
         *
         * @param RevisionRecord $rev
 -       * @param Title|null $title if known (optional)
 +       * @param int $flags (optional) $flags include:
 +       *      IDBAccessObject::READ_LATEST: Select the data from the master
         *
         * @return RevisionRecord|null
         */
 -      public function getNextRevision( RevisionRecord $rev, Title $title = null ) {
 -              if ( !$rev->getId() || !$rev->getPageId() ) {
 -                      // revision is unsaved or otherwise incomplete
 -                      return null;
 -              }
 -
 -              if ( $rev instanceof RevisionArchiveRecord ) {
 -                      // revision is deleted, so it's not part of the page history
 -                      return null;
 +      public function getPreviousRevision( RevisionRecord $rev, $flags = 0 ) {
 +              if ( $flags instanceof Title ) {
 +                      // Old calling convention, we don't use Title here anymore
 +                      wfDeprecated( __METHOD__ . ' with Title', '1.34' );
 +                      $flags = 0;
                }
  
 -              if ( $title === null ) {
 -                      // this would fail for deleted revisions
 -                      $title = $this->getTitle( $rev->getPageId(), $rev->getId() );
 -              }
 +              return $this->getRelativeRevision( $rev, $flags, 'prev' );
 +      }
  
 -              $next = $title->getNextRevisionID( $rev->getId() );
 -              if ( !$next ) {
 -                      return null;
 +      /**
 +       * Get the revision after $rev in the page's history, if any.
 +       * Will return null for the latest revision but also for deleted or unsaved revisions.
 +       *
 +       * MCR migration note: this replaces Revision::getNext
 +       *
 +       * @see Title::getNextRevisionID
 +       *
 +       * @param RevisionRecord $rev
 +       * @param int $flags (optional) $flags include:
 +       *      IDBAccessObject::READ_LATEST: Select the data from the master
 +       * @return RevisionRecord|null
 +       */
 +      public function getNextRevision( RevisionRecord $rev, $flags = 0 ) {
 +              if ( $flags instanceof Title ) {
 +                      // Old calling convention, we don't use Title here anymore
 +                      wfDeprecated( __METHOD__ . ' with Title', '1.34' );
 +                      $flags = 0;
                }
  
 -              return $this->getRevisionByTitle( $title, $next );
 +              return $this->getRelativeRevision( $rev, $flags, 'next' );
        }
  
        /**
        }
  
        /**
 -       * Get rev_timestamp from rev_id, without loading the rest of the row
 +       * Get rev_timestamp from rev_id, without loading the rest of the row.
 +       *
 +       * Historically, there was an extra Title parameter that was passed before $id. This is no
 +       * longer needed and is deprecated in 1.34.
         *
         * MCR migration note: this replaces Revision::getTimestampFromId
         *
 -       * @param Title $title
         * @param int $id
         * @param int $flags
         * @return string|bool False if not found
         */
 -      public function getTimestampFromId( $title, $id, $flags = 0 ) {
 +      public function getTimestampFromId( $id, $flags = 0 ) {
 +              if ( $id instanceof Title ) {
 +                      // Old deprecated calling convention supported for backwards compatibility
 +                      $id = $flags;
 +                      $flags = func_num_args() > 2 ? func_get_arg( 2 ) : 0;
 +              }
                $db = $this->getDBConnectionRefForQueryFlags( $flags );
  
 -              $conds = [ 'rev_id' => $id ];
 -              $conds['rev_page'] = $title->getArticleID();
 -              $timestamp = $db->selectField( 'revision', 'rev_timestamp', $conds, __METHOD__ );
 +              $timestamp =
 +                      $db->selectField( 'revision', 'rev_timestamp', [ 'rev_id' => $id ], __METHOD__ );
  
                return ( $timestamp !== false ) ? wfTimestamp( TS_MW, $timestamp ) : false;
        }
diff --combined includes/user/User.php
@@@ -673,11 -673,20 +673,20 @@@ class User implements IDBAccessObject, 
         * @param int|null $userId User ID, if known
         * @param string|null $userName User name, if known
         * @param int|null $actorId Actor ID, if known
+        * @param bool|string $wikiId remote wiki to which the User/Actor ID applies, or false if none
         * @return User
         */
-       public static function newFromAnyId( $userId, $userName, $actorId ) {
+       public static function newFromAnyId( $userId, $userName, $actorId, $wikiId = false ) {
                global $wgActorTableSchemaMigrationStage;
  
+               // Stop-gap solution for the problem described in T222212.
+               // Force the User ID and Actor ID to zero for users loaded from the database
+               // of another wiki, to prevent subtle data corruption and confusing failure modes.
+               if ( $wikiId !== false ) {
+                       $userId = 0;
+                       $actorId = 0;
+               }
                $user = new User;
                $user->mFrom = 'defaults';
  
                return true;
        }
  
 +      /**
 +       * Alias of isLoggedIn() with a name that describes its actual functionality. UserIdentity has
 +       * only this new name and not the old isLoggedIn() variant.
 +       *
 +       * @return bool True if user is registered on this wiki, i.e., has a user ID. False if user is
 +       *   anonymous or has no local account (which can happen when importing). This is equivalent to
 +       *   getId() != 0 and is provided for code readability.
 +       * @since 1.34
 +       */
 +      public function isRegistered() {
 +              return $this->getId() != 0;
 +      }
 +
        /**
         * Get whether the user is logged in
         * @return bool
         */
        public function isLoggedIn() {
 -              return $this->getId() != 0;
 +              return $this->isRegistered();
        }
  
        /**
         * @return bool
         */
        public function isAnon() {
 -              return !$this->isLoggedIn();
 +              return !$this->isRegistered();
        }
  
        /**
@@@ -504,24 -504,20 +504,24 @@@ class UserTest extends MediaWikiTestCas
        }
  
        /**
 +       * @covers User::isRegistered
         * @covers User::isLoggedIn
         * @covers User::isAnon
         */
        public function testLoggedIn() {
                $user = $this->getMutableTestUser()->getUser();
 +              $this->assertTrue( $user->isRegistered() );
                $this->assertTrue( $user->isLoggedIn() );
                $this->assertFalse( $user->isAnon() );
  
                // Non-existent users are perceived as anonymous
                $user = User::newFromName( 'UTNonexistent' );
 +              $this->assertFalse( $user->isRegistered() );
                $this->assertFalse( $user->isLoggedIn() );
                $this->assertTrue( $user->isAnon() );
  
                $user = new User;
 +              $this->assertFalse( $user->isRegistered() );
                $this->assertFalse( $user->isLoggedIn() );
                $this->assertTrue( $user->isAnon() );
        }
                $this->assertSame( 'Bogus', $test->getName() );
                $this->assertSame( 654321, $test->getActorId() );
  
+               // Loading remote user by name from remote wiki should succeed
+               $test = User::newFromAnyId( null, 'Bogus', null, 'foo' );
+               $this->assertSame( 0, $test->getId() );
+               $this->assertSame( 'Bogus', $test->getName() );
+               $this->assertSame( 0, $test->getActorId() );
+               $test = User::newFromAnyId( 123456, 'Bogus', 654321, 'foo' );
+               $this->assertSame( 0, $test->getId() );
+               $this->assertSame( 0, $test->getActorId() );
                // Exceptional cases
                try {
                        User::newFromAnyId( null, null, null );
                        $this->fail( 'Expected exception not thrown' );
                } catch ( InvalidArgumentException $ex ) {
                }
+               // Loading remote user by id from remote wiki should fail
+               try {
+                       User::newFromAnyId( 123456, null, 654321, 'foo' );
+                       $this->fail( 'Expected exception not thrown' );
+               } catch ( InvalidArgumentException $ex ) {
+               }
        }
  
        /**