fix(session): Replace remember-me tokens in transaction

Signed-off-by: Christoph Wurst <[email protected]>

lib/private/AllConfig.php

@@ -345,19 +345,12 @@ public function getUserKeys($userId, $appName) {
 		}
 	}
 
-	/**
-	 * 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 @@ public function deleteUserValue($userId, $appName, $key) {
 		if (isset($this->userCache[$userId][$appName])) {
 			unset($this->userCache[$userId][$appName][$key]);
 		}
+
+		return $rowsAffected > 0;
 	}
 
 	/**

lib/private/Server.php

@@ -548,6 +548,7 @@ public function __construct($webRoot, \OC\Config $config) {
 				$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 */

lib/private/User/Session.php

@@ -48,13 +48,15 @@
 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;
 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\UserFirstTimeLoggedInEvent;
 use OCP\Util;
 use Psr\Log\LoggerInterface;
+use function in_array;
 
 /**
  * Class Session
@@ -91,6 +94,9 @@
  * @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 @@ public function __construct(Manager $manager,
 		ISecureRandom $random,
 		ILockdownManager $lockdownManager,
 		LoggerInterface $logger,
+		IDBConnection $dbConnection,
 		IEventDispatcher $dispatcher
 	) {
 		$this->manager = $manager;
@@ -137,6 +145,7 @@ public function __construct(Manager $manager,
 		$this->random = $random;
 		$this->lockdownManager = $lockdownManager;
 		$this->logger = $logger;
+		$this->dbConnection = $dbConnection;
 		$this->dispatcher = $dispatcher;
 	}
 
@@ -905,24 +914,49 @@ public function loginWithCookie($uid, $currentToken, $oldSessionId) {
 			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();

lib/public/IConfig.php

@@ -241,9 +241,10 @@ public function getAllUserValues(string $userId): array;
 	 * @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