From c7b5c3e02f1d2f9d7dc49ee094cc4420673f4e9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Mon, 28 Oct 2024 10:14:29 +0100 Subject: [PATCH 1/2] fix: Clear pending two factor tokens also from configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise as the tokens were removed from the database but not from the configuration the next time that the tokens were cleared the previous tokens were still got from the configuration, and trying to remove them again from the database ended in a DoesNotExistException being thrown. Signed-off-by: Daniel Calviño Sánchez --- .../Authentication/TwoFactorAuth/Manager.php | 2 ++ .../TwoFactorAuth/ManagerTest.php | 26 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/lib/private/Authentication/TwoFactorAuth/Manager.php b/lib/private/Authentication/TwoFactorAuth/Manager.php index 2585646c998..f4b3b88c50b 100644 --- a/lib/private/Authentication/TwoFactorAuth/Manager.php +++ b/lib/private/Authentication/TwoFactorAuth/Manager.php @@ -366,6 +366,8 @@ class Manager { $tokensNeeding2FA = $this->config->getUserKeys($userId, 'login_token_2fa'); foreach ($tokensNeeding2FA as $tokenId) { + $this->config->deleteUserValue($userId, 'login_token_2fa', $tokenId); + $this->tokenProvider->invalidateTokenById($userId, (int)$tokenId); } } diff --git a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php index a89b07f7716..4f4dbaccaff 100644 --- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php @@ -701,4 +701,30 @@ class ManagerTest extends TestCase { $this->assertFalse($this->manager->needsSecondFactor($user)); } + + public function testClearTwoFactorPending() { + $this->config->method('getUserKeys') + ->with('theUserId', 'login_token_2fa') + ->willReturn([ + '42', '43', '44' + ]); + + $this->config->expects($this->exactly(3)) + ->method('deleteUserValue') + ->withConsecutive( + ['theUserId', 'login_token_2fa', '42'], + ['theUserId', 'login_token_2fa', '43'], + ['theUserId', 'login_token_2fa', '44'], + ); + + $this->tokenProvider->expects($this->exactly(3)) + ->method('invalidateTokenById') + ->withConsecutive( + ['theUserId', 42], + ['theUserId', 43], + ['theUserId', 44], + ); + + $this->manager->clearTwoFactorPending('theUserId'); + } } From fcefd37a5f7e24208a115214660d419cbdeae7f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Mon, 28 Oct 2024 10:15:16 +0100 Subject: [PATCH 2/2] fix: Handle exception when clearing previously removed two factor tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a token was already removed from the database but not from the configuration clearing the tokens will try to remove it again from the database, which caused a DoesNotExistException to be thrown. Signed-off-by: Daniel Calviño Sánchez --- .../Authentication/TwoFactorAuth/Manager.php | 6 +++- .../TwoFactorAuth/ManagerTest.php | 32 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/lib/private/Authentication/TwoFactorAuth/Manager.php b/lib/private/Authentication/TwoFactorAuth/Manager.php index f4b3b88c50b..39d886132dd 100644 --- a/lib/private/Authentication/TwoFactorAuth/Manager.php +++ b/lib/private/Authentication/TwoFactorAuth/Manager.php @@ -12,6 +12,7 @@ use BadMethodCallException; use Exception; use OC\Authentication\Token\IProvider as TokenProvider; use OCP\Activity\IManager; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Authentication\Exceptions\InvalidTokenException; use OCP\Authentication\TwoFactorAuth\IActivatableAtLogin; @@ -368,7 +369,10 @@ class Manager { foreach ($tokensNeeding2FA as $tokenId) { $this->config->deleteUserValue($userId, 'login_token_2fa', $tokenId); - $this->tokenProvider->invalidateTokenById($userId, (int)$tokenId); + try { + $this->tokenProvider->invalidateTokenById($userId, (int)$tokenId); + } catch (DoesNotExistException $e) { + } } } } diff --git a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php index 4f4dbaccaff..a574299642a 100644 --- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php @@ -15,6 +15,7 @@ use OC\Authentication\TwoFactorAuth\MandatoryTwoFactor; use OC\Authentication\TwoFactorAuth\ProviderLoader; use OCP\Activity\IEvent; use OCP\Activity\IManager; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Authentication\TwoFactorAuth\IActivatableAtLogin; use OCP\Authentication\TwoFactorAuth\IProvider; @@ -727,4 +728,35 @@ class ManagerTest extends TestCase { $this->manager->clearTwoFactorPending('theUserId'); } + + public function testClearTwoFactorPendingTokenDoesNotExist() { + $this->config->method('getUserKeys') + ->with('theUserId', 'login_token_2fa') + ->willReturn([ + '42', '43', '44' + ]); + + $this->config->expects($this->exactly(3)) + ->method('deleteUserValue') + ->withConsecutive( + ['theUserId', 'login_token_2fa', '42'], + ['theUserId', 'login_token_2fa', '43'], + ['theUserId', 'login_token_2fa', '44'], + ); + + $this->tokenProvider->expects($this->exactly(3)) + ->method('invalidateTokenById') + ->withConsecutive( + ['theUserId', 42], + ['theUserId', 43], + ['theUserId', 44], + ) + ->willReturnCallback(function ($user, $tokenId) { + if ($tokenId === 43) { + throw new DoesNotExistException('token does not exist'); + } + }); + + $this->manager->clearTwoFactorPending('theUserId'); + } }