Skip to content

Commit

Permalink
Distinguish 'done' from 'configuring' in 2FA
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
michielbdejong committed Jan 9, 2023
1 parent 3892c3e commit 681fae0
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 3 deletions.
3 changes: 2 additions & 1 deletion lib/private/Authentication/TwoFactorAuth/Manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 4 additions & 2 deletions tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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));
}

Expand Down

0 comments on commit 681fae0

Please sign in to comment.