Revert "Tombstone the old session on SessionBackend::resetId()"wmf/1.39.0-wmf.12

This reverts commit 95a2f2a1304a6467ce80449bb5b5c401397b2cf7.

Reason for revert: Wrong branch.

Change-Id: I02875048022a1688863c87ef7785a64cf9d1623d

4 files changed, 14 insertions, 84 deletions

diff --git a/includes/session/SessionBackend.php b/includes/session/SessionBackend.php
index e6c32cde4e2e..6b0c81e43b01 100644
--- a/includes/session/SessionBackend.php
+++ b/includes/session/SessionBackend.php
@@ -32,7 +32,6 @@ use Psr\Log\LoggerInterface;
 use User;
 use WebRequest;
 use Wikimedia\AtEase\AtEase;
-use Wikimedia\LightweightObjectStore\ExpirationAwareness;
 
 /**
  * This is the actual workhorse for Session.
@@ -53,10 +52,6 @@ use Wikimedia\LightweightObjectStore\ExpirationAwareness;
  * @since 1.27
  */
 final class SessionBackend {
-
-	/** Expiration time of tombstone records in seconds */
-	private const TOMBSTONE_EXPIRY = ExpirationAwareness::TTL_MINUTE;
-
 	/** @var SessionId */
 	private $id;
 
@@ -127,7 +122,7 @@ final class SessionBackend {
 	/** @var array|null provider-specified metadata */
 	private $providerMetadata = null;
 
-	/** @var int UNIX timestamp at which the session will expire */
+	/** @var int */
 	private $expires = 0;
 
 	/** @var int */
@@ -301,7 +296,8 @@ final class SessionBackend {
 
 			$this->autosave();
 
-			$this->tombstone( $oldId );
+			// Delete the data for the old session ID now
+			$this->store->delete( $this->store->makeKey( 'MWSession', $oldId ) );
 		}
 
 		return $this->id;
@@ -635,39 +631,6 @@ final class SessionBackend {
 	}
 
 	/**
-	 * Put a tombstone in the session store for the given ID, to mark it as not used anymore.
-	 * For most purposes this is the same as deleting the object with that ID.
-	 *
-	 * @param string $id
-	 * @return bool Success.
-	 */
-	private function tombstone( $id ) {
-		// Match the data format from save(). The 'tombstoned' flag makes SessionManager
-		// abort any loading attempts, but use data for an anonymous session just in case.
-		return $this->store->set(
-			$this->store->makeKey( 'MWSession', $id ),
-			[
-				'data' => [],
-				'metadata' => [
-					'provider' => (string)$this->provider,
-					'providerMetadata' => null,
-					'userId' => 0,
-					'userName' => null,
-					'userToken' => null,
-					'remember' => false,
-					'forceHTTPS' => $this->forceHTTPS,
-					'expires' => time() + self::TOMBSTONE_EXPIRY,
-					'loggedOut' => 0,
-					'persisted' => false,
-					'tombstoned' => true,
-				],
-			],
-			self::TOMBSTONE_EXPIRY,
-			CachedBagOStuff::WRITE_SYNC
-		);
-	}
-
-	/**
 	 * Delay automatic saving while multiple updates are being made
 	 *
 	 * Calls to save() will not be delayed.
@@ -806,7 +769,6 @@ final class SessionBackend {
 			'expires' => time() + $this->lifetime,
 			'loggedOut' => $this->loggedOut,
 			'persisted' => $this->persist,
-			'tombstoned' => false,
 		];
 
 		$this->hookRunner->onSessionMetadata( $this, $metadata, $this->requests );
diff --git a/includes/session/SessionManager.php b/includes/session/SessionManager.php
index 2e036ac819ea..673609a014d9 100644
--- a/includes/session/SessionManager.php
+++ b/includes/session/SessionManager.php
@@ -365,7 +365,7 @@ class SessionManager implements SessionManagerInterface {
 		}
 
 		// @phan-suppress-next-line PhanTypePossiblyInvalidDimOffset False positive
-		return $this->getSessionFromInfo( $infos[0], $request, true );
+		return $this->getSessionFromInfo( $infos[0], $request );
 	}
 
 	public function invalidateSessionsForUser( User $user ) {
@@ -542,9 +542,8 @@ class SessionManager implements SessionManagerInterface {
 		usort( $infos, [ SessionInfo::class, 'compare' ] );
 		$retInfos = [];
 		while ( $infos ) {
-			$tombstoned = false;
 			$info = array_pop( $infos );
-			if ( $this->loadSessionInfoFromStore( $info, $request, $tombstoned ) ) {
+			if ( $this->loadSessionInfoFromStore( $info, $request ) ) {
 				$retInfos[] = $info;
 				while ( $infos ) {
 					/** @var SessionInfo $info */
@@ -563,10 +562,8 @@ class SessionManager implements SessionManagerInterface {
 						$info->getProvider()->unpersistSession( $request );
 					}
 				}
-			} elseif ( !$tombstoned ) {
-				// Session load failed, so unpersist it from this request; but don't unpersist
-				// tombstoned sessions to avoid unpersisting the user's newer, valid session
-				// via a race condition (T299193).
+			} else {
+				// Session load failed, so unpersist it from this request
 				$this->logUnpersist( $info, $request );
 				$info->getProvider()->unpersistSession( $request );
 			}
@@ -587,16 +584,9 @@ class SessionManager implements SessionManagerInterface {
 	 *
 	 * @param SessionInfo &$info Will likely be replaced with an updated SessionInfo instance
 	 * @param WebRequest $request
-	 * @param bool &$tombstoned Output parameter telling whether the session was tombstoned.
-	 *   Tombstoned sessions should be treated as nonexistent, but care should be taken not to
-	 *   overwrite another valid session the user might have.
 	 * @return bool Whether the session info matches the stored data (if any)
 	 */
-	private function loadSessionInfoFromStore(
-		SessionInfo &$info,
-		WebRequest $request,
-		bool &$tombstoned = false
-	) {
+	private function loadSessionInfoFromStore( SessionInfo &$info, WebRequest $request ) {
 		$key = $this->store->makeKey( 'MWSession', $info->getId() );
 		$blob = $this->store->get( $key );
 
@@ -705,10 +695,7 @@ class SessionManager implements SessionManagerInterface {
 
 			// Next, load the user from metadata, or validate it against the metadata.
 			$userInfo = $info->getUserInfo();
-			if ( $metadata['tombstoned'] ?? false ) {
-				$tombstoned = true;
-				return $failHandler();
-			} elseif ( !$userInfo ) {
+			if ( !$userInfo ) {
 				// For loading, id is preferred to name.
 				try {
 					if ( $metadata['userId'] ) {
@@ -887,11 +874,9 @@ class SessionManager implements SessionManagerInterface {
 	 *  own Session. Most session providers won't need this.
 	 * @param SessionInfo $info
 	 * @param WebRequest $request
-	 * @param bool $empty True when we are creating an empty session (ie. this method was called
-	 *   from getEmptySession()).
 	 * @return Session
 	 */
-	public function getSessionFromInfo( SessionInfo $info, WebRequest $request, $empty = false ) {
+	public function getSessionFromInfo( SessionInfo $info, WebRequest $request ) {
 		// @codeCoverageIgnoreStart
 		if ( defined( 'MW_NO_SESSION' ) ) {
 			$ep = defined( 'MW_ENTRY_POINT' ) ? MW_ENTRY_POINT : 'this';
@@ -922,11 +907,7 @@ class SessionManager implements SessionManagerInterface {
 				$this->config->get( MainConfigNames::ObjectCacheSessionExpiry )
 			);
 			$this->allSessionBackends[$id] = $backend;
-			// Do not save the session when we are creating an empty session. It's pointless
-			// and might result in unpersisting cookies, which is a problem when tombstoned.
-			if ( !$empty ) {
-				$delay = $backend->delaySave();
-			}
+			$delay = $backend->delaySave();
 		} else {
 			$backend = $this->allSessionBackends[$id];
 			$delay = $backend->delaySave();
diff --git a/tests/phpunit/includes/auth/AuthManagerTest.php b/tests/phpunit/includes/auth/AuthManagerTest.php
index 11681ce00c70..0437cf475e7b 100644
--- a/tests/phpunit/includes/auth/AuthManagerTest.php
+++ b/tests/phpunit/includes/auth/AuthManagerTest.php
@@ -13,7 +13,6 @@ use MediaWiki\HookContainer\HookContainer;
 use MediaWiki\HookContainer\StaticHookRegistry;
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Session\SessionInfo;
-use MediaWiki\Session\SessionProvider;
 use MediaWiki\Session\UserInfo;
 use MediaWiki\User\BotPasswordStore;
 use MediaWiki\User\UserFactory;
@@ -393,19 +392,17 @@ class AuthManagerTest extends \MediaWikiIntegrationTestCase {
 	public function testSecuritySensitiveOperationStatus( $mutableSession ) {
 		$this->logger = new \Psr\Log\NullLogger();
 		$user = \User::newFromName( 'UTSysop' );
-		$sessionId = str_pad( '', 32, 'a' );
 		$provideUser = null;
 		$reauth = $mutableSession ? AuthManager::SEC_REAUTH : AuthManager::SEC_FAIL;
 
-		/** @var SessionProvider $provider */
 		list( $provider, $reset ) = $this->getMockSessionProvider(
 			$mutableSession, [ 'provideSessionInfo' ]
 		);
 		$provider->method( 'provideSessionInfo' )
-			->will( $this->returnCallback( static function () use ( $provider, &$sessionId, &$provideUser ) {
+			->will( $this->returnCallback( static function () use ( $provider, &$provideUser ) {
 				return new SessionInfo( SessionInfo::MIN_PRIORITY, [
 					'provider' => $provider,
-					'id' => $sessionId,
+					'id' => \DummySessionProvider::ID,
 					'persisted' => true,
 					'userInfo' => UserInfo::newFromUser( $provideUser, true )
 				] );
@@ -423,8 +420,6 @@ class AuthManagerTest extends \MediaWikiIntegrationTestCase {
 		$session->set( 'AuthManager:lastAuthTimestamp', time() - 5 );
 		$this->assertSame( $reauth, $this->manager->securitySensitiveOperationStatus( 'foo' ) );
 
-		// reset mock ID to avoid tombstone
-		$sessionId = str_pad( '', 32, 'b' );
 		$provideUser = $user;
 		$session = $provider->getManager()->getSessionForRequest( $this->request );
 		$this->assertSame( $user->getId(), $session->getUser()->getId() );
diff --git a/tests/phpunit/includes/session/SessionBackendTest.php b/tests/phpunit/includes/session/SessionBackendTest.php
index f38237a89729..2b19ea7c88a9 100644
--- a/tests/phpunit/includes/session/SessionBackendTest.php
+++ b/tests/phpunit/includes/session/SessionBackendTest.php
@@ -307,15 +307,7 @@ class SessionBackendTest extends MediaWikiIntegrationTestCase {
 		$this->assertNotEquals( self::SESSIONID, $backend->getId() );
 		$this->assertSame( $backend->getId(), $sessionId->getId() );
 		$this->assertIsArray( $this->store->getSession( $backend->getId() ) );
-		$oldSession = $this->store->getSession( self::SESSIONID );
-		$oldSessionFromBackend = $this->store->getSessionFromBackend( self::SESSIONID );
-		$this->assertIsArray( $oldSession );
-		foreach ( [ $oldSession, $oldSessionFromBackend ] as $sessionStore ) {
-			$this->assertTrue( $sessionStore['metadata']['tombstoned'] );
-			$this->assertSame( 0, $sessionStore['metadata']['userId'] );
-			$this->assertSame( null, $sessionStore['metadata']['userName'] );
-			$this->assertSame( null, $sessionStore['metadata']['userToken'] );
-		}
+		$this->assertFalse( $this->store->getSession( self::SESSIONID ) );
 		$this->assertSame( $id, session_id() );
 		$this->assertArrayNotHasKey( self::SESSIONID, $manager->allSessionBackends );
 		$this->assertArrayHasKey( $backend->getId(), $manager->allSessionBackends );