fix(session): Replace remember-me tokens in transactionfix/session/transactional-remember-me-renewal

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

4 files changed, 56 insertions, 25 deletions

diff --git a/lib/private/AllConfig.php b/lib/private/AllConfig.php
index ab4359c798f..967da8417ab 100644
--- a/lib/private/AllConfig.php
+++ b/lib/private/AllConfig.php
@@ -345,19 +345,12 @@ class AllConfig implements IConfig {
 		}
 	}
 
-	/**
-	 * Delete a user value
-	 *
-	 * @param string $userId the userId of the user that we want to store the value under
-	 * @param string $appName the appName that we stored the value under
-	 * @param string $key the key under which the value is being stored
-	 */
-	public function deleteUserValue($userId, $appName, $key) {
+	public function deleteUserValue($userId, $appName, $key): bool {
 		// TODO - FIXME
 		$this->fixDIInit();
 
 		$qb = $this->connection->getQueryBuilder();
-		$qb->delete('preferences')
+		$rowsAffected = $qb->delete('preferences')
 			->where($qb->expr()->eq('userid', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
 			->andWhere($qb->expr()->eq('appid', $qb->createNamedParameter($appName, IQueryBuilder::PARAM_STR)))
 			->andWhere($qb->expr()->eq('configkey', $qb->createNamedParameter($key, IQueryBuilder::PARAM_STR)))
@@ -366,6 +359,8 @@ class AllConfig implements IConfig {
 		if (isset($this->userCache[$userId][$appName])) {
 			unset($this->userCache[$userId][$appName][$key]);
 		}
+
+		return $rowsAffected > 0;
 	}
 
 	/**
diff --git a/lib/private/Server.php b/lib/private/Server.php
index 76c73383b2e..830005b71c8 100644
--- a/lib/private/Server.php
+++ b/lib/private/Server.php
@@ -548,6 +548,7 @@ class Server extends ServerContainer implements IServerContainer {
 				$c->get(ISecureRandom::class),
 				$c->getLockdownManager(),
 				$c->get(LoggerInterface::class),
+				$c->get(IDBConnection::class),
 				$c->get(IEventDispatcher::class)
 			);
 			/** @deprecated 21.0.0 use BeforeUserCreatedEvent event with the IEventDispatcher instead */
diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php
index ab6ec9f38d3..27cb3ea0eff 100644
--- a/lib/private/User/Session.php
+++ b/lib/private/User/Session.php
@@ -48,6 +48,7 @@ use OC\Hooks\PublicEmitter;
 use OC_User;
 use OC_Util;
 use OCA\DAV\Connector\Sabre\Auth;
+use OCP\AppFramework\Db\TTransactional;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Authentication\Exceptions\ExpiredTokenException;
 use OCP\Authentication\Exceptions\InvalidTokenException;
@@ -55,6 +56,7 @@ use OCP\EventDispatcher\GenericEvent;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\NotPermittedException;
 use OCP\IConfig;
+use OCP\IDBConnection;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\IUser;
@@ -67,6 +69,7 @@ use OCP\User\Events\PostLoginEvent;
 use OCP\User\Events\UserFirstTimeLoggedInEvent;
 use OCP\Util;
 use Psr\Log\LoggerInterface;
+use function in_array;
 
 /**
  * Class Session
@@ -91,6 +94,9 @@ use Psr\Log\LoggerInterface;
  * @package OC\User
  */
 class Session implements IUserSession, Emitter {
+
+	use TTransactional;
+
 	/** @var Manager $manager */
 	private $manager;
 
@@ -116,6 +122,7 @@ class Session implements IUserSession, Emitter {
 	private $lockdownManager;
 
 	private LoggerInterface $logger;
+	private IDBConnection $dbConnection;
 	/** @var IEventDispatcher */
 	private $dispatcher;
 
@@ -127,6 +134,7 @@ class Session implements IUserSession, Emitter {
 		ISecureRandom $random,
 		ILockdownManager $lockdownManager,
 		LoggerInterface $logger,
+		IDBConnection $dbConnection,
 		IEventDispatcher $dispatcher
 	) {
 		$this->manager = $manager;
@@ -137,6 +145,7 @@ class Session implements IUserSession, Emitter {
 		$this->random = $random;
 		$this->lockdownManager = $lockdownManager;
 		$this->logger = $logger;
+		$this->dbConnection = $dbConnection;
 		$this->dispatcher = $dispatcher;
 	}
 
@@ -905,24 +914,49 @@ class Session implements IUserSession, Emitter {
 			return false;
 		}
 
-		// get stored tokens
-		$tokens = $this->config->getUserKeys($uid, 'login_token');
-		// test cookies token against stored tokens
-		if (!in_array($currentToken, $tokens, true)) {
-			$this->logger->info('Tried to log in but could not verify token', [
+		/*
+		 * Run token lookup and replacement in a transaction
+		 *
+		 * The READ COMMITTED isolation level causes the database to serialize
+		 * the DELETE query, making it possible to detect if two concurrent
+		 * processes try to replace the same login token.
+		 * Replacing more than once doesn't work because the app token behind
+		 * the session can only be replaced once.
+		 */
+		$newToken = $this->atomic(function () use ($uid, $currentToken): ?string {
+			// get stored tokens
+			$tokens = $this->config->getUserKeys($uid, 'login_token');
+			// test cookies token against stored tokens
+			if (!in_array($currentToken, $tokens, true)) {
+				$this->logger->error('Tried to log in {uid} but could not find token {token} in database', [
+					'app' => 'core',
+					'token' => $currentToken,
+					'uid' => $uid,
+					'user' => $uid,
+				]);
+				return false;
+			}
+			// replace successfully used token with a new one
+			if (!$this->config->deleteUserValue($uid, 'login_token', $currentToken)) {
+				$this->logger->error('Tried to log in {uid} but ran into concurrent session revival', [
+					'app' => 'core',
+					'token' => $currentToken,
+					'uid' => $uid,
+					'user' => $uid,
+				]);
+				return null;
+			}
+			$newToken = $this->random->generate(32);
+			$this->config->setUserValue($uid, 'login_token', $newToken, (string)$this->timeFactory->getTime());
+			$this->logger->debug('Remember-me token {token} for {uid} replaced by {newToken}', [
 				'app' => 'core',
+				'token' => $currentToken,
+				'newToken' => $newToken,
+				'uid' => $uid,
 				'user' => $uid,
 			]);
-			return false;
-		}
-		// replace successfully used token with a new one
-		$this->config->deleteUserValue($uid, 'login_token', $currentToken);
-		$newToken = $this->random->generate(32);
-		$this->config->setUserValue($uid, 'login_token', $newToken, (string)$this->timeFactory->getTime());
-		$this->logger->debug('Remember-me token replaced', [
-			'app' => 'core',
-			'user' => $uid,
-		]);
+			return $newToken;
+		}, $this->dbConnection);
 
 		try {
 			$sessionId = $this->session->getId();
diff --git a/lib/public/IConfig.php b/lib/public/IConfig.php
index 706e4776221..025d2b2de73 100644
--- a/lib/public/IConfig.php
+++ b/lib/public/IConfig.php
@@ -241,9 +241,10 @@ interface IConfig {
 	 * @param string $userId the userId of the user that we want to store the value under
 	 * @param string $appName the appName that we stored the value under
 	 * @param string $key the key under which the value is being stored
+	 * @return bool whether a value has been deleted
 	 * @since 8.0.0
 	 */
-	public function deleteUserValue($userId, $appName, $key);
+	public function deleteUserValue($userId, $appName, $key): bool;
 
 	/**
 	 * Delete all user values