From 0b507ae5466d7d1b4a41c17c5f39cb4e4785b6bc Mon Sep 17 00:00:00 2001 From: Michiel de Jong Date: Fri, 2 Dec 2022 10:27:14 +0100 Subject: [PATCH] Distinguish 'done' from 'configuring' in 2FA When there is a token in the session for which the user is still [setting up 2FA](https://raw.githubusercontent.com/nextcloud/twofactor_totp/master/screenshots/settings.png), setting `self::SESSION_UID_DONE` ("two_factor_auth_passed") is a misnomer. AFAICT, everything works fine if you set nothing into the session and just return 'false' from this if-statement, but in case there is some code (now or in the future) that needs to know if the user is configuring 2FA, to play it safe I would suggest storing `self::SESSION_UID_CONFIGURING` ("two_factor_auth_configuring") into the session. Signed-off-by: Michiel de Jong --- lib/private/Authentication/TwoFactorAuth/Manager.php | 3 ++- tests/lib/Authentication/TwoFactorAuth/ManagerTest.php | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/private/Authentication/TwoFactorAuth/Manager.php b/lib/private/Authentication/TwoFactorAuth/Manager.php index ce73238498755..dd0a7b64d0c12 100644 --- a/lib/private/Authentication/TwoFactorAuth/Manager.php +++ b/lib/private/Authentication/TwoFactorAuth/Manager.php @@ -52,6 +52,7 @@ class Manager { public const SESSION_UID_KEY = 'two_factor_auth_uid'; public const SESSION_UID_DONE = 'two_factor_auth_passed'; + public const SESSION_UID_CONFIGURING = 'two_factor_auth_configuring'; public const REMEMBER_LOGIN = 'two_factor_remember_login'; public const BACKUP_CODES_PROVIDER_ID = 'backup_codes'; @@ -360,7 +361,7 @@ public function needsSecondFactor(IUser $user = null): bool { $tokensNeeding2FA = $this->config->getUserKeys($user->getUID(), 'login_token_2fa'); if (!\in_array((string) $tokenId, $tokensNeeding2FA, true)) { - $this->session->set(self::SESSION_UID_DONE, $user->getUID()); + $this->session->set(self::SESSION_UID_CONFIGURING, $user->getUID()); return false; } } catch (InvalidTokenException|SessionNotAvailableException $e) { diff --git a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php index ae6fadc790c57..ae177ef98b2bc 100644 --- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php @@ -648,7 +648,7 @@ public function testNeedsSecondFactorSessionAuth() { $this->assertFalse($this->manager->needsSecondFactor($user)); } - public function testNeedsSecondFactorSessionAuthFailDBPass() { + public function testNeedsSecondFactorWhileConfiguring() { $user = $this->createMock(IUser::class); $user->method('getUID') ->willReturn('user'); @@ -672,10 +672,12 @@ public function testNeedsSecondFactorSessionAuthFailDBPass() { '42', '43', '44' ]); + // the user is still configuring 2FA with token 40 $this->session->expects($this->once()) ->method('set') - ->with(Manager::SESSION_UID_DONE, 'user'); + ->with(Manager::SESSION_UID_CONFIGURING, 'user'); + // 2FA should not be required if configuration is not complete $this->assertFalse($this->manager->needsSecondFactor($user)); }