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

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

lib/private/AllConfig.php

@@ -339,19 +339,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)))
@@ -360,6 +353,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

@@ -530,7 +530,8 @@ public function __construct($webRoot, \OC\Config $config) {
 				$c->get(ISecureRandom::class),
 				$c->getLockdownManager(),
 				$c->get(LoggerInterface::class),
-				$c->get(IEventDispatcher::class)
+				$c->get(IEventDispatcher::class),
+				$c->get(IDBConnection::class),
 			);
 			/** @deprecated 21.0.0 use BeforeUserCreatedEvent event with the IEventDispatcher instead */
 			$userSession->listen('\OC\User', 'preCreateUser', function ($uid, $password) {

lib/private/User/Session.php

@@ -50,11 +50,13 @@
 use OC_User;
 use OC_Util;
 use OCA\DAV\Connector\Sabre\Auth;
+use OCP\AppFramework\Db\TTransactional;
 use OCP\AppFramework\Utility\ITimeFactory;
 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;
@@ -91,6 +93,8 @@
  * @package OC\User
  */
 class Session implements IUserSession, Emitter {
+	use TTransactional;
+
 	/** @var Manager $manager */
 	private $manager;
 
@@ -118,6 +122,7 @@ class Session implements IUserSession, Emitter {
 	private LoggerInterface $logger;
 	/** @var IEventDispatcher */
 	private $dispatcher;
+	private IDBConnection $connection;
 
 	public function __construct(Manager $manager,
 								ISession $session,
@@ -127,7 +132,8 @@ public function __construct(Manager $manager,
 								ISecureRandom $random,
 								ILockdownManager $lockdownManager,
 								LoggerInterface $logger,
-								IEventDispatcher $dispatcher
+								IEventDispatcher $dispatcher,
+								IDBConnection $connection,
 	) {
 		$this->manager = $manager;
 		$this->session = $session;
@@ -138,6 +144,7 @@ public function __construct(Manager $manager,
 		$this->lockdownManager = $lockdownManager;
 		$this->logger = $logger;
 		$this->dispatcher = $dispatcher;
+		$this->connection = $connection;
 	}
 
 	/**
@@ -871,32 +878,63 @@ 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 {uid} but could not verify token', [
-				'app' => 'core',
-				'uid' => $uid,
-			]);
+		/*
+		 * 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 null;
+			}
+			// 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());
+			return $newToken;
+		}, $this->connection);
+
+		// Token verification or replacement failed. Session can't be revived.
+		if ($newToken === null) {
 			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());
 
 		try {
 			$sessionId = $this->session->getId();
 			$token = $this->tokenProvider->renewSessionToken($oldSessionId, $sessionId);
 		} catch (SessionNotAvailableException $ex) {
-			$this->logger->warning('Could not renew session token for {uid} because the session is unavailable', [
+			$this->logger->critical('Could not renew session token for {uid} because the session is unavailable', [
 				'app' => 'core',
 				'uid' => $uid,
+				'user' => $uid,
 			]);
 			return false;
 		} catch (InvalidTokenException $ex) {
-			$this->logger->warning('Renewing session token failed', ['app' => 'core']);
+			$this->logger->error('Renewing session token failed', [
+				'app' => 'core',
+				'user' => $uid,
+			]);
 			return false;
 		}
 

lib/public/IConfig.php

@@ -235,9 +235,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