From 56fc4341fb33984c797b16d1d937a57819731731 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 3870c797f8dcf..9611bdec659d3 100644 --- a/lib/private/Authentication/TwoFactorAuth/Manager.php +++ b/lib/private/Authentication/TwoFactorAuth/Manager.php @@ -385,6 +385,8 @@ public function clearTwoFactorPending(string $userId) { $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 a2655f58649e5..c741ff068ac59 100644 --- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php @@ -715,4 +715,30 @@ public function testNeedsSecondFactorAppPassword() { $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 b39c5d8393fb155a490a2dba1709235dd828d377 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 9611bdec659d3..b066f00901fec 100644 --- a/lib/private/Authentication/TwoFactorAuth/Manager.php +++ b/lib/private/Authentication/TwoFactorAuth/Manager.php @@ -31,6 +31,7 @@ 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; @@ -387,7 +388,10 @@ public function clearTwoFactorPending(string $userId) { 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 c741ff068ac59..23ae5d93fdda3 100644 --- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php @@ -29,6 +29,7 @@ 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; @@ -741,4 +742,35 @@ public function testClearTwoFactorPending() { $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'); + } }