diff --git a/lib/private/AllConfig.php b/lib/private/AllConfig.php index 2a0e8f53b1494..5fe902e398ed7 100644 --- a/lib/private/AllConfig.php +++ b/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; } /** diff --git a/lib/private/Server.php b/lib/private/Server.php index 7ab57478f44ed..86182bd493a43 100644 --- a/lib/private/Server.php +++ b/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) { diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 82887f8d02967..c4b3a874aa290 100644 --- a/lib/private/User/Session.php +++ b/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; } diff --git a/lib/public/IConfig.php b/lib/public/IConfig.php index 0e7a752321874..f7b3e810785a9 100644 --- a/lib/public/IConfig.php +++ b/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