Merge "Pass the user and request into BlockManager::getUserBlock"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Wed, 11 Sep 2019 18:58:32 +0000 (18:58 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Wed, 11 Sep 2019 18:58:32 +0000 (18:58 +0000)
1  2 
includes/ServiceWiring.php
includes/user/User.php
tests/phpunit/includes/user/UserTest.php

@@@ -77,7 -77,9 +77,7 @@@ use Wikimedia\ObjectFactory
  
  return [
        'ActorMigration' => function ( MediaWikiServices $services ) : ActorMigration {
 -              return new ActorMigration(
 -                      $services->getMainConfig()->get( 'ActorTableSchemaMigrationStage' )
 -              );
 +              return new ActorMigration( SCHEMA_COMPAT_NEW );
        },
  
        'BadFileLookup' => function ( MediaWikiServices $services ) : BadFileLookup {
        },
  
        'BlockManager' => function ( MediaWikiServices $services ) : BlockManager {
-               $context = RequestContext::getMain();
                return new BlockManager(
                        new ServiceOptions(
                                BlockManager::$constructorOptions, $services->getMainConfig()
                        ),
-                       $context->getUser(),
-                       $context->getRequest(),
                        $services->getPermissionManager()
                );
        },
diff --combined includes/user/User.php
@@@ -64,7 -64,7 +64,7 @@@ class User implements IDBAccessObject, 
         * Version number to tag cached versions of serialized User objects. Should be increased when
         * {@link $mCacheVars} or one of it's members changes.
         */
 -      const VERSION = 13;
 +      const VERSION = 14;
  
        /**
         * Exclude user options that are set to their default value.
                        case 'defaults':
                                $this->loadDefaults();
                                break;
 -                      case 'name':
 -                              // Make sure this thread sees its own changes
 -                              $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
 -                              if ( $lb->hasOrMadeRecentMasterChanges() ) {
 -                                      $flags |= self::READ_LATEST;
 -                                      $this->queryFlagsUsed = $flags;
 -                              }
 -
 -                              $this->mId = self::idFromName( $this->mName, $flags );
 -                              if ( !$this->mId ) {
 -                                      // Nonexistent user placeholder object
 -                                      $this->loadDefaults( $this->mName );
 -                              } else {
 -                                      $this->loadFromId( $flags );
 -                              }
 -                              break;
                        case 'id':
                                // Make sure this thread sees its own changes, if the ID isn't 0
                                if ( $this->mId != 0 ) {
                                $this->loadFromId( $flags );
                                break;
                        case 'actor':
 +                      case 'name':
                                // Make sure this thread sees its own changes
                                $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
                                if ( $lb->hasOrMadeRecentMasterChanges() ) {
                                list( $index, $options ) = DBAccessObjectUtils::getDBOptions( $flags );
                                $row = wfGetDB( $index )->selectRow(
                                        'actor',
 -                                      [ 'actor_user', 'actor_name' ],
 -                                      [ 'actor_id' => $this->mActorId ],
 +                                      [ 'actor_id', 'actor_user', 'actor_name' ],
 +                                      $this->mFrom === 'name' ? [ 'actor_name' => $this->mName ] : [ 'actor_id' => $this->mActorId ],
                                        __METHOD__,
                                        $options
                                );
  
                                if ( !$row ) {
                                        // Ugh.
 -                                      $this->loadDefaults();
 +                                      $this->loadDefaults( $this->mFrom === 'name' ? $this->mName : false );
                                } elseif ( $row->actor_user ) {
                                        $this->mId = $row->actor_user;
                                        $this->loadFromId( $flags );
                                } else {
 -                                      $this->loadDefaults( $row->actor_name );
 +                                      $this->loadDefaults( $row->actor_name, $row->actor_id );
                                }
                                break;
                        case 'session':
         * @return User The corresponding User object
         */
        public static function newFromActorId( $id ) {
 -              global $wgActorTableSchemaMigrationStage;
 -
 -              // Technically we shouldn't allow this without SCHEMA_COMPAT_READ_NEW,
 -              // but it does little harm and might be needed for write callers loading a User.
 -              if ( !( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_NEW ) ) {
 -                      throw new BadMethodCallException(
 -                              'Cannot use ' . __METHOD__
 -                                      . ' when $wgActorTableSchemaMigrationStage lacks SCHEMA_COMPAT_NEW'
 -                      );
 -              }
 -
                $u = new User;
                $u->mActorId = $id;
                $u->mFrom = 'actor';
         * @return User
         */
        public static function newFromAnyId( $userId, $userName, $actorId, $dbDomain = 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.
                $user = new User;
                $user->mFrom = 'defaults';
  
 -              // Technically we shouldn't allow this without SCHEMA_COMPAT_READ_NEW,
 -              // but it does little harm and might be needed for write callers loading a User.
 -              if ( ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_NEW ) && $actorId !== null ) {
 +              if ( $actorId !== null ) {
                        $user->mActorId = (int)$actorId;
                        if ( $user->mActorId !== 0 ) {
                                $user->mFrom = 'actor';
         *       the constructor does that instead.
         *
         * @param string|bool $name
 +       * @param int|null $actorId
         */
 -      public function loadDefaults( $name = false ) {
 +      public function loadDefaults( $name = false, $actorId = null ) {
                $this->mId = 0;
                $this->mName = $name;
 -              $this->mActorId = null;
 +              $this->mActorId = $actorId;
                $this->mRealName = '';
                $this->mEmail = '';
                $this->mOptionOverrides = null;
         *  user_properties   Array with properties out of the user_properties table
         */
        protected function loadFromRow( $row, $data = null ) {
 -              global $wgActorTableSchemaMigrationStage;
 -
                if ( !is_object( $row ) ) {
                        throw new InvalidArgumentException( '$row must be an object' );
                }
  
                $this->mGroupMemberships = null; // deferred
  
 -              // Technically we shouldn't allow this without SCHEMA_COMPAT_READ_NEW,
 -              // but it does little harm and might be needed for write callers loading a User.
 -              if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_NEW ) {
 -                      if ( isset( $row->actor_id ) ) {
 -                              $this->mActorId = (int)$row->actor_id;
 -                              if ( $this->mActorId !== 0 ) {
 -                                      $this->mFrom = 'actor';
 -                              }
 -                              $this->setItemLoaded( 'actor' );
 -                      } else {
 -                              $all = false;
 +              if ( isset( $row->actor_id ) ) {
 +                      $this->mActorId = (int)$row->actor_id;
 +                      if ( $this->mActorId !== 0 ) {
 +                              $this->mFrom = 'actor';
                        }
 +                      $this->setItemLoaded( 'actor' );
 +              } else {
 +                      $all = false;
                }
  
                if ( isset( $row->user_name ) && $row->user_name !== '' ) {
                // overwriting mBlockedby, surely?
                $this->load();
  
+               // TODO: Block checking shouldn't really be done from the User object. Block
+               // checking can involve checking for IP blocks, cookie blocks, and/or XFF blocks,
+               // which need more knowledge of the request context than the User should have.
+               // Since we do currently check blocks from the User, we have to do the following
+               // here:
+               // - Check if this is the user associated with the main request
+               // - If so, pass the relevant request information to the block manager
+               $request = null;
+               // The session user is set up towards the end of Setup.php. Until then,
+               // assume it's a logged-out user.
+               $sessionUser = RequestContext::getMain()->getUser();
+               $globalUserName = $sessionUser->isSafeToLoad()
+                       ? $sessionUser->getName()
+                       : IP::sanitizeIP( $sessionUser->getRequest()->getIP() );
+               if ( $this->getName() === $globalUserName ) {
+                       // This is the global user, so we need to pass the request
+                       $request = $this->getRequest();
+               }
                // @phan-suppress-next-line PhanAccessMethodInternal It's the only allowed use
                $block = MediaWikiServices::getInstance()->getBlockManager()->getUserBlock(
                        $this,
+                       $request,
                        $fromReplica
                );
  
                // Avoid PHP 7.1 warning of passing $this by reference
                $thisUser = $this;
                // Extensions
 -              Hooks::run( 'GetBlockedStatus', [ &$thisUser ] );
 +              Hooks::run( 'GetBlockedStatus', [ &$thisUser ], '1.34' );
        }
  
        /**
                if ( !$this->mHideName ) {
                        // Reset for hook
                        $this->mHideName = false;
 -                      Hooks::run( 'UserIsHidden', [ $this, &$this->mHideName ] );
 +                      Hooks::run( 'UserIsHidden', [ $this, &$this->mHideName ], '1.34' );
                }
                return (bool)$this->mHideName;
        }
         * @return int The user's ID; 0 if the user is anonymous or nonexistent
         */
        public function getId() {
 -              if ( $this->mId === null && $this->mName !== null && self::isIP( $this->mName ) ) {
 +              if ( $this->mId === null && $this->mName !== null &&
 +                      ( self::isIP( $this->mName ) || ExternalUserNames::isExternal( $this->mName ) )
 +              ) {
                        // Special case, we know the user is anonymous
                        return 0;
                }
         * @return int The actor's ID, or 0 if no actor ID exists and $dbw was null
         */
        public function getActorId( IDatabase $dbw = null ) {
 -              global $wgActorTableSchemaMigrationStage;
 -
 -              // Technically we should always return 0 without SCHEMA_COMPAT_READ_NEW,
 -              // but it does little harm and might be needed for write callers loading a User.
 -              if ( !( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_NEW ) ) {
 -                      return 0;
 -              }
 -
                if ( !$this->isItemLoaded( 'actor' ) ) {
                        $this->load();
                }
  
 -              // Currently $this->mActorId might be null if $this was loaded from a
 -              // cache entry that was written when $wgActorTableSchemaMigrationStage
 -              // was SCHEMA_COMPAT_OLD. Once that is no longer a possibility (i.e. when
 -              // User::VERSION is incremented after $wgActorTableSchemaMigrationStage
 -              // has been removed), that condition may be removed.
 -              if ( $this->mActorId === null || !$this->mActorId && $dbw ) {
 +              if ( !$this->mActorId && $dbw ) {
                        $q = [
                                'actor_user' => $this->getId() ?: null,
                                'actor_name' => (string)$this->getName(),
                        ];
 -                      if ( $dbw ) {
 -                              if ( $q['actor_user'] === null && self::isUsableName( $q['actor_name'] ) ) {
 +                      if ( $q['actor_user'] === null && self::isUsableName( $q['actor_name'] ) ) {
 +                              throw new CannotCreateActorException(
 +                                      'Cannot create an actor for a usable name that is not an existing user'
 +                              );
 +                      }
 +                      if ( $q['actor_name'] === '' ) {
 +                              throw new CannotCreateActorException( 'Cannot create an actor for a user with no name' );
 +                      }
 +                      $dbw->insert( 'actor', $q, __METHOD__, [ 'IGNORE' ] );
 +                      if ( $dbw->affectedRows() ) {
 +                              $this->mActorId = (int)$dbw->insertId();
 +                      } else {
 +                              // Outdated cache?
 +                              // Use LOCK IN SHARE MODE to bypass any MySQL REPEATABLE-READ snapshot.
 +                              $this->mActorId = (int)$dbw->selectField(
 +                                      'actor',
 +                                      'actor_id',
 +                                      $q,
 +                                      __METHOD__,
 +                                      [ 'LOCK IN SHARE MODE' ]
 +                              );
 +                              if ( !$this->mActorId ) {
                                        throw new CannotCreateActorException(
 -                                              'Cannot create an actor for a usable name that is not an existing user'
 -                                      );
 -                              }
 -                              if ( $q['actor_name'] === '' ) {
 -                                      throw new CannotCreateActorException( 'Cannot create an actor for a user with no name' );
 -                              }
 -                              $dbw->insert( 'actor', $q, __METHOD__, [ 'IGNORE' ] );
 -                              if ( $dbw->affectedRows() ) {
 -                                      $this->mActorId = (int)$dbw->insertId();
 -                              } else {
 -                                      // Outdated cache?
 -                                      // Use LOCK IN SHARE MODE to bypass any MySQL REPEATABLE-READ snapshot.
 -                                      $this->mActorId = (int)$dbw->selectField(
 -                                              'actor',
 -                                              'actor_id',
 -                                              $q,
 -                                              __METHOD__,
 -                                              [ 'LOCK IN SHARE MODE' ]
 +                                              "Cannot create actor ID for user_id={$this->getId()} user_name={$this->getName()}"
                                        );
 -                                      if ( !$this->mActorId ) {
 -                                              throw new CannotCreateActorException(
 -                                                      "Cannot create actor ID for user_id={$this->getId()} user_name={$this->getName()}"
 -                                              );
 -                                      }
                                }
 -                              $this->invalidateCache();
 -                      } else {
 -                              list( $index, $options ) = DBAccessObjectUtils::getDBOptions( $this->queryFlagsUsed );
 -                              $db = wfGetDB( $index );
 -                              $this->mActorId = (int)$db->selectField( 'actor', 'actor_id', $q, __METHOD__, $options );
                        }
 +                      $this->invalidateCache();
                        $this->setItemLoaded( 'actor' );
                }
  
  
                                // If there is a new, unseen, revision, use its timestamp
                                $nextid = $oldid
 -                                      ? $title->getNextRevisionID( $oldid, Title::GAID_FOR_UPDATE )
 +                                      ? $title->getNextRevisionID( $oldid, Title::READ_LATEST )
                                        : null;
                                if ( $nextid ) {
                                        $this->setNewtalk( true, Revision::newFromId( $nextid ) );
  
                $dbw = wfGetDB( DB_MASTER );
                $dbw->doAtomicSection( __METHOD__, function ( IDatabase $dbw, $fname ) use ( $newTouched ) {
 -                      global $wgActorTableSchemaMigrationStage;
 -
                        $dbw->update( 'user',
                                [ /* SET */
                                        'user_name' => $this->mName,
                                );
                        }
  
 -                      if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_NEW ) {
 -                              $dbw->update(
 -                                      'actor',
 -                                      [ 'actor_name' => $this->mName ],
 -                                      [ 'actor_user' => $this->mId ],
 -                                      $fname
 -                              );
 -                      }
 +                      $dbw->update(
 +                              'actor',
 +                              [ 'actor_name' => $this->mName ],
 +                              [ 'actor_user' => $this->mId ],
 +                              $fname
 +                      );
                } );
  
                $this->mTouched = $newTouched;
         * @param IDatabase $dbw Writable database handle
         */
        private function updateActorId( IDatabase $dbw ) {
 -              global $wgActorTableSchemaMigrationStage;
 -
 -              if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_NEW ) {
 -                      $dbw->insert(
 -                              'actor',
 -                              [ 'actor_user' => $this->mId, 'actor_name' => $this->mName ],
 -                              __METHOD__
 -                      );
 -                      $this->mActorId = (int)$dbw->insertId();
 -              }
 +              $dbw->insert(
 +                      'actor',
 +                      [ 'actor_user' => $this->mId, 'actor_name' => $this->mName ],
 +                      __METHOD__
 +              );
 +              $this->mActorId = (int)$dbw->insertId();
        }
  
        /**
         *   - joins: (array) to include in the `$join_conds` to `IDatabase->select()`
         */
        public static function getQueryInfo() {
 -              global $wgActorTableSchemaMigrationStage;
 -
                $ret = [
 -                      'tables' => [ 'user' ],
 +                      'tables' => [ 'user', 'user_actor' => 'actor' ],
                        'fields' => [
                                'user_id',
                                'user_name',
                                'user_email_token_expires',
                                'user_registration',
                                'user_editcount',
 +                              'user_actor.actor_id',
 +                      ],
 +                      'joins' => [
 +                              'user_actor' => [ 'JOIN', 'user_actor.actor_user = user_id' ],
                        ],
 -                      'joins' => [],
                ];
  
 -              // Technically we shouldn't allow this without SCHEMA_COMPAT_READ_NEW,
 -              // but it does little harm and might be needed for write callers loading a User.
 -              if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_NEW ) {
 -                      $ret['tables']['user_actor'] = 'actor';
 -                      $ret['fields'][] = 'user_actor.actor_id';
 -                      $ret['joins']['user_actor'] = [
 -                              ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_READ_NEW ) ? 'JOIN' : 'LEFT JOIN',
 -                              [ 'user_actor.actor_user = user_id' ]
 -                      ];
 -              }
 -
                return $ret;
        }
  
@@@ -31,6 -31,7 +31,6 @@@ class UserTest extends MediaWikiTestCas
                $this->setMwGlobals( [
                        'wgGroupPermissions' => [],
                        'wgRevokePermissions' => [],
 -                      'wgActorTableSchemaMigrationStage' => SCHEMA_COMPAT_NEW,
                ] );
  
                $this->setUpPermissionGlobals();
  
                $user = User::newFromName( $ip, false );
                $this->assertTrue( $user->getActorId() > 0, 'Actor ID can be loaded for an anonymous user' );
 -              $user2 = User::newFromActorId( $user->getActorId() );
 -              $this->assertEquals( $user->getName(), $user2->getName(),
 -                      'User::newFromActorId works for an anonymous user' );
 -      }
 -
 -      /**
 -       * Actor tests with SCHEMA_COMPAT_READ_OLD
 -       *
 -       * The only thing different from testActorId() is the behavior if the actor
 -       * row doesn't exist in the DB, since with SCHEMA_COMPAT_READ_NEW that
 -       * situation can't happen. But we copy all the other tests too just for good measure.
 -       *
 -       * @covers User::newFromActorId
 -       */
 -      public function testActorId_old() {
 -              $this->setMwGlobals( [
 -                      'wgActorTableSchemaMigrationStage' => SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD,
 -              ] );
 -
 -              $domain = MediaWikiServices::getInstance()->getDBLoadBalancer()->getLocalDomainID();
 -              $this->hideDeprecated( 'User::selectFields' );
 -
 -              // Newly-created user has an actor ID
 -              $user = User::createNew( 'UserTestActorIdOld1' );
 -              $id = $user->getId();
 -              $this->assertTrue( $user->getActorId() > 0, 'User::createNew sets an actor ID' );
 -
 -              $user = User::newFromName( 'UserTestActorIdOld2' );
 -              $user->addToDatabase();
 -              $this->assertTrue( $user->getActorId() > 0, 'User::addToDatabase sets an actor ID' );
 -
 -              $user = User::newFromName( 'UserTestActorIdOld1' );
 -              $this->assertTrue( $user->getActorId() > 0, 'Actor ID can be retrieved for user loaded by name' );
 -
 -              $user = User::newFromId( $id );
 -              $this->assertTrue( $user->getActorId() > 0, 'Actor ID can be retrieved for user loaded by ID' );
 -
 -              $user2 = User::newFromActorId( $user->getActorId() );
 -              $this->assertEquals( $user->getId(), $user2->getId(),
 -                      'User::newFromActorId works for an existing user' );
 -
 -              $row = $this->db->selectRow( 'user', User::selectFields(), [ 'user_id' => $id ], __METHOD__ );
 -              $user = User::newFromRow( $row );
 -              $this->assertTrue( $user->getActorId() > 0,
 -                      'Actor ID can be retrieved for user loaded with User::selectFields()' );
 -
 -              $this->db->delete( 'actor', [ 'actor_user' => $id ], __METHOD__ );
 -              User::purge( $domain, $id );
 -              // Because WANObjectCache->delete() stupidly doesn't delete from the process cache.
 -
 -              MediaWikiServices::getInstance()->getMainWANObjectCache()->clearProcessCache();
 -
 -              $user = User::newFromId( $id );
 -              $this->assertFalse( $user->getActorId() > 0, 'No Actor ID by default if none in database' );
 -              $this->assertTrue( $user->getActorId( $this->db ) > 0, 'Actor ID can be created if none in db' );
 -
 -              $user->setName( 'UserTestActorIdOld4-renamed' );
 -              $user->saveSettings();
 -              $this->assertEquals(
 -                      $user->getName(),
 -                      $this->db->selectField(
 -                              'actor', 'actor_name', [ 'actor_id' => $user->getActorId() ], __METHOD__
 -                      ),
 -                      'User::saveSettings updates actor table for name change'
 -              );
 -
 -              // For sanity
 -              $ip = '192.168.12.34';
 -              $this->db->delete( 'actor', [ 'actor_name' => $ip ], __METHOD__ );
 -
 -              $user = User::newFromName( $ip, false );
 -              $this->assertFalse( $user->getActorId() > 0, 'Anonymous user has no actor ID by default' );
 -              $this->assertTrue( $user->getActorId( $this->db ) > 0,
 -                      'Actor ID can be created for an anonymous user' );
 -
 -              $user = User::newFromName( $ip, false );
 -              $this->assertTrue( $user->getActorId() > 0, 'Actor ID can be loaded for an anonymous user' );
                $user2 = User::newFromActorId( $user->getActorId() );
                $this->assertEquals( $user->getName(), $user2->getName(),
                        'User::newFromActorId works for an anonymous user' );
                // logged in users should be inmune to cookie block of type ip/range
                $this->assertNull( $user->getBlock() );
  
-               // cookie is being cleared
-               $cookies = $request->response()->getCookies();
-               $this->assertEquals( '', $cookies['wikiBlockID']['value'] );
                // clean up
                $block->delete();
        }