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 2585646c9984f..f4b3b88c50b3c 100644 --- a/lib/private/Authentication/TwoFactorAuth/Manager.php +++ b/lib/private/Authentication/TwoFactorAuth/Manager.php @@ -366,6 +366,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 a89b07f7716e1..4f4dbaccaff04 100644 --- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php @@ -701,4 +701,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 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 f4b3b88c50b3c..39d886132dd61 100644 --- a/lib/private/Authentication/TwoFactorAuth/Manager.php +++ b/lib/private/Authentication/TwoFactorAuth/Manager.php @@ -12,6 +12,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; @@ -368,7 +369,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 4f4dbaccaff04..a574299642aeb 100644 --- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php @@ -15,6 +15,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; @@ -727,4 +728,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'); + } }