Skip to content

Commit 681fae0

Browse files
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 <michiel@unhosted.org>
1 parent 3892c3e commit 681fae0

2 files changed

Lines changed: 6 additions & 3 deletions

File tree

lib/private/Authentication/TwoFactorAuth/Manager.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
class Manager {
5353
public const SESSION_UID_KEY = 'two_factor_auth_uid';
5454
public const SESSION_UID_DONE = 'two_factor_auth_passed';
55+
public const SESSION_UID_CONFIGURING = 'two_factor_auth_configuring';
5556
public const REMEMBER_LOGIN = 'two_factor_remember_login';
5657
public const BACKUP_CODES_PROVIDER_ID = 'backup_codes';
5758

@@ -360,7 +361,7 @@ public function needsSecondFactor(IUser $user = null): bool {
360361
$tokensNeeding2FA = $this->config->getUserKeys($user->getUID(), 'login_token_2fa');
361362

362363
if (!\in_array((string) $tokenId, $tokensNeeding2FA, true)) {
363-
$this->session->set(self::SESSION_UID_DONE, $user->getUID());
364+
$this->session->set(self::SESSION_UID_CONFIGURING, $user->getUID());
364365
return false;
365366
}
366367
} catch (InvalidTokenException|SessionNotAvailableException $e) {

tests/lib/Authentication/TwoFactorAuth/ManagerTest.php

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -648,7 +648,7 @@ public function testNeedsSecondFactorSessionAuth() {
648648
$this->assertFalse($this->manager->needsSecondFactor($user));
649649
}
650650

651-
public function testNeedsSecondFactorSessionAuthFailDBPass() {
651+
public function testNeedsSecondFactorWhileConfiguring() {
652652
$user = $this->createMock(IUser::class);
653653
$user->method('getUID')
654654
->willReturn('user');
@@ -672,10 +672,12 @@ public function testNeedsSecondFactorSessionAuthFailDBPass() {
672672
'42', '43', '44'
673673
]);
674674

675+
// the user is still configuring 2FA with token 40
675676
$this->session->expects($this->once())
676677
->method('set')
677-
->with(Manager::SESSION_UID_DONE, 'user');
678+
->with(Manager::SESSION_UID_CONFIGURING, 'user');
678679

680+
// 2FA should not be required if configuration is not complete
679681
$this->assertFalse($this->manager->needsSecondFactor($user));
680682
}
681683

0 commit comments

Comments
 (0)