Run session token renewals in a database transaction

The session token renewal does
1) Read the old token
2) Write a new token
3) Delete the old token

If two processes succeed to read the old token there can be two new tokens because
the queries were not run in a transaction. This is particularly problematic on
clustered DBs where 1) would go to a read node and 2) and 3) go to a write node.

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

diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php
index 511aad76211b8b79b0174adecf9176e463424b3f..c7e295683831df962a079b14ad9d0ca51e000f20 100644 (file)
--- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php
+++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php
@@ -34,14 +34,18 @@ use OC\Authentication\Exceptions\InvalidTokenException;
 use OC\Authentication\Exceptions\TokenPasswordExpiredException;
 use OC\Authentication\Exceptions\PasswordlessTokenException;
 use OC\Authentication\Exceptions\WipeTokenException;
+use OCP\AppFramework\Db\TTransactional;
 use OCP\Cache\CappedMemoryCache;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IConfig;
+use OCP\IDBConnection;
 use OCP\Security\ICrypto;
 use Psr\Log\LoggerInterface;
 
 class PublicKeyTokenProvider implements IProvider {
+       use TTransactional;
+
        /** @var PublicKeyTokenMapper */
        private $mapper;
 
@@ -51,6 +55,8 @@ class PublicKeyTokenProvider implements IProvider {
        /** @var IConfig */
        private $config;
 
+       private IDBConnection $db;
+
        /** @var LoggerInterface */
        private $logger;
 
@@ -63,11 +69,13 @@ class PublicKeyTokenProvider implements IProvider {
        public function __construct(PublicKeyTokenMapper $mapper,
                                                                ICrypto $crypto,
                                                                IConfig $config,
+                                                               IDBConnection $db,
                                                                LoggerInterface $logger,
                                                                ITimeFactory $time) {
                $this->mapper = $mapper;
                $this->crypto = $crypto;
                $this->config = $config;
+               $this->db = $db;
                $this->logger = $logger;
                $this->time = $time;
 
@@ -164,31 +172,32 @@ class PublicKeyTokenProvider implements IProvider {
        public function renewSessionToken(string $oldSessionId, string $sessionId): IToken {
                $this->cache->clear();
 
-               $token = $this->getToken($oldSessionId);
-
-               if (!($token instanceof PublicKeyToken)) {
-                       throw new InvalidTokenException("Invalid token type");
-               }
+               return $this->atomic(function () use ($oldSessionId, $sessionId) {
+                       $token = $this->getToken($oldSessionId);
 
-               $password = null;
-               if (!is_null($token->getPassword())) {
-                       $privateKey = $this->decrypt($token->getPrivateKey(), $oldSessionId);
-                       $password = $this->decryptPassword($token->getPassword(), $privateKey);
-               }
-
-               $newToken = $this->generateToken(
-                       $sessionId,
-                       $token->getUID(),
-                       $token->getLoginName(),
-                       $password,
-                       $token->getName(),
-                       IToken::TEMPORARY_TOKEN,
-                       $token->getRemember()
-               );
-
-               $this->mapper->delete($token);
+                       if (!($token instanceof PublicKeyToken)) {
+                               throw new InvalidTokenException("Invalid token type");
+                       }
 
-               return $newToken;
+                       $password = null;
+                       if (!is_null($token->getPassword())) {
+                               $privateKey = $this->decrypt($token->getPrivateKey(), $oldSessionId);
+                               $password = $this->decryptPassword($token->getPassword(), $privateKey);
+                       }
+                       $newToken = $this->generateToken(
+                               $sessionId,
+                               $token->getUID(),
+                               $token->getLoginName(),
+                               $password,
+                               $token->getName(),
+                               IToken::TEMPORARY_TOKEN,
+                               $token->getRemember()
+                       );
+
+                       $this->mapper->delete($token);
+
+                       return $newToken;
+               }, $this->db);
        }
 
        public function invalidateToken(string $token) {

diff --git a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
index ad0a13937ae179a357b4b1b154c5629a4a57393d..ce739a74bb88fff360c6f9fe59c053ae902b624b 100644 (file)
--- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
+++ b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
@@ -1,4 +1,7 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * @copyright Copyright (c) 2018 Roeland Jago Douma <roeland@famdouma.nl>
  *
@@ -34,6 +37,7 @@ use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IConfig;
 use OCP\Security\ICrypto;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
@@ -46,6 +50,8 @@ class PublicKeyTokenProviderTest extends TestCase {
        private $crypto;
        /** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
        private $config;
+       /** @var IDBConnection|IDBConnection|MockObject */
+       private IDBConnection $db;
        /** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
        private $logger;
        /** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */
@@ -66,14 +72,24 @@ class PublicKeyTokenProviderTest extends TestCase {
                                ['secret', '', '1f4h9s'],
                                ['openssl', [], []],
                        ]);
+               $this->db = $this->createMock(IDBConnection::class);
+               $this->db->method('atomic')->willReturnCallback(function ($cb) {
+                       return $cb();
+               });
                $this->logger = $this->createMock(LoggerInterface::class);
                $this->timeFactory = $this->createMock(ITimeFactory::class);
                $this->time = 1313131;
                $this->timeFactory->method('getTime')
                        ->willReturn($this->time);
 
-               $this->tokenProvider = new PublicKeyTokenProvider($this->mapper, $this->crypto, $this->config, $this->logger,
-                       $this->timeFactory);
+               $this->tokenProvider = new PublicKeyTokenProvider(
+                       $this->mapper,
+                       $this->crypto,
+                       $this->config,
+                       $this->db,
+                       $this->logger,
+                       $this->timeFactory,
+               );
        }
 
        public function testGenerateToken() {