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 0837ec339a..0ee10ac0ef 100644
--- a/lib/private/Authentication/TwoFactorAuth/Manager.php
+++ b/lib/private/Authentication/TwoFactorAuth/Manager.php
@@ -104,7 +104,9 @@ class Manager {
 		}
 
 		$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 e54e435340..34ce340049 100644
--- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
+++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
@@ -108,7 +108,6 @@ class ManagerTest extends TestCase {
 
 		$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 @@ class ManagerTest extends TestCase {
 			]);
 	}
 
-	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 @@ class ManagerTest extends TestCase {
 			->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')