From d248a0bd1e66cffcaf354204bdfb7e803e2d4f49 Mon Sep 17 00:00:00 2001
From: Christoph Wurst <christoph@winzerhof-wurst.at>
Date: Wed, 8 Aug 2018 06:57:32 +0200
Subject: [PATCH] Fix 2FA provider registry population on login

If the 2FA provider registry has not been populated yet, we have to make
sure all available providers are loaded and queried on login. Otherwise
previously active 2FA providers aren't detected as enabled.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
---
 .../Authentication/TwoFactorAuth/Manager.php  |  4 +-
 .../TwoFactorAuth/ManagerTest.php             | 82 ++++++++++++++++++-
 2 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/lib/private/Authentication/TwoFactorAuth/Manager.php b/lib/private/Authentication/TwoFactorAuth/Manager.php
index 0837ec339a5e3..0ee10ac0effde 100644
--- a/lib/private/Authentication/TwoFactorAuth/Manager.php
+++ b/lib/private/Authentication/TwoFactorAuth/Manager.php
@@ -104,7 +104,9 @@ public function isTwoFactorAuthenticated(IUser $user): bool {
 		}
 
 		$providerStates = $this->providerRegistry->getProviderStates($user);
-		$enabled = array_filter($providerStates);
+		$providers = $this->providerLoader->getProviders($user);
+		$fixedStates = $this->fixMissingProviderStates($providerStates, $providers, $user);
+		$enabled = array_filter($fixedStates);
 
 		return $twoFactorEnabled && !empty($enabled);
 	}
diff --git a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
index e54e4353404e9..34ce340049a1d 100644
--- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
+++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
@@ -108,7 +108,6 @@ protected function setUp() {
 
 		$this->fakeProvider = $this->createMock(IProvider::class);
 		$this->fakeProvider->method('getId')->willReturn('email');
-		$this->fakeProvider->method('isTwoFactorAuthEnabledForUser')->willReturn(true);
 
 		$this->backupProvider = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock();
 		$this->backupProvider->method('getId')->willReturn('backup_codes');
@@ -143,7 +142,25 @@ private function prepareProvidersWitBackupProvider() {
 			]);
 	}
 
-	public function testIsTwoFactorAuthenticated() {
+	public function testIsTwoFactorAuthenticatedNoProviders() {
+		$this->user->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('user123'));
+		$this->config->expects($this->once())
+			->method('getUserValue')
+			->with('user123', 'core', 'two_factor_auth_disabled', 0)
+			->willReturn(0);
+		$this->providerRegistry->expects($this->once())
+			->method('getProviderStates')
+			->willReturn([]); // No providers registered
+		$this->providerLoader->expects($this->once())
+			->method('getProviders')
+			->willReturn([]); // No providers loadable
+
+		$this->assertFalse($this->manager->isTwoFactorAuthenticated($this->user));
+	}
+
+	public function testIsTwoFactorAuthenticatedFailingProviders() {
 		$this->user->expects($this->once())
 			->method('getUID')
 			->will($this->returnValue('user123'));
@@ -156,11 +173,70 @@ public function testIsTwoFactorAuthenticated() {
 			->willReturn([
 				'twofactor_totp' => true,
 				'twofactor_u2f' => false,
-			]);
+			]); // Two providers registered, but …
+		$this->providerLoader->expects($this->once())
+			->method('getProviders')
+			->willReturn([]); // … none of them is able to load, however …
 
+		// … 2FA is still enforced
 		$this->assertTrue($this->manager->isTwoFactorAuthenticated($this->user));
 	}
 
+	public function providerStatesFixData(): array {
+		return [
+			[false, false],
+			[true, true],
+		];
+	}
+
+	/**
+	 * If the 2FA registry has not been populated when a user logs in,
+	 * the 2FA manager has to first fix the state before it checks for
+	 * enabled providers.
+	 *
+	 * If any of these providers is active, 2FA is enabled
+	 *
+	 * @dataProvider providerStatesFixData
+	 */
+	public function testIsTwoFactorAuthenticatedFixesProviderStates(bool $providerEnabled, bool $expected) {
+		$this->user->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('user123'));
+		$this->config->expects($this->once())
+			->method('getUserValue')
+			->with('user123', 'core', 'two_factor_auth_disabled', 0)
+			->willReturn(0);
+		$this->providerRegistry->expects($this->once())
+			->method('getProviderStates')
+			->willReturn([]); // Nothing registered yet
+		$this->providerLoader->expects($this->once())
+			->method('getProviders')
+			->willReturn([
+				$this->fakeProvider
+			]);
+		$this->fakeProvider->expects($this->once())
+			->method('isTwoFactorAuthEnabledForUser')
+			->with($this->user)
+			->willReturn($providerEnabled);
+		if ($providerEnabled) {
+			$this->providerRegistry->expects($this->once())
+				->method('enableProviderFor')
+				->with(
+					$this->fakeProvider,
+					$this->user
+				);
+		} else {
+			$this->providerRegistry->expects($this->once())
+				->method('disableProviderFor')
+				->with(
+					$this->fakeProvider,
+					$this->user
+				);
+		}
+
+		$this->assertEquals($expected, $this->manager->isTwoFactorAuthenticated($this->user));
+	}
+
 	public function testGetProvider() {
 		$this->providerRegistry->expects($this->once())
 			->method('getProviderStates')