Merge pull request #4660 from nextcloud/fix/hide-2fa-backup-codes

Hide 2FA backup codes if no other 2FA providers are enabled

3 files changed, 111 insertions, 8 deletions

diff --git a/apps/twofactor_backupcodes/lib/Provider/BackupCodesProvider.php b/apps/twofactor_backupcodes/lib/Provider/BackupCodesProvider.php
index 902f7c783d5..5c5500862e6 100644
--- a/apps/twofactor_backupcodes/lib/Provider/BackupCodesProvider.php
+++ b/apps/twofactor_backupcodes/lib/Provider/BackupCodesProvider.php
@@ -22,6 +22,7 @@
 
 namespace OCA\TwoFactorBackupCodes\Provider;
 
+use OC\App\AppManager;
 use OCA\TwoFactorBackupCodes\Service\BackupCodeStorage;
 use OCP\Authentication\TwoFactorAuth\IProvider;
 use OCP\IL10N;
@@ -30,15 +31,29 @@ use OCP\Template;
 
 class BackupCodesProvider implements IProvider {
 
+	/** @var string */
+	private $appName;
+
 	/** @var BackupCodeStorage */
 	private $storage;
 
 	/** @var IL10N */
 	private $l10n;
 
-	public function __construct(BackupCodeStorage $storage, IL10N $l10n) {
+	/** @var AppManager */
+	private $appManager;
+
+	/**
+	 * @param string $appName
+	 * @param BackupCodeStorage $storage
+	 * @param IL10N $l10n
+	 * @param AppManager $appManager
+	 */
+	public function __construct($appName, BackupCodeStorage $storage, IL10N $l10n, AppManager $appManager) {
+		$this->appName = $appName;
 		$this->l10n = $l10n;
 		$this->storage = $storage;
+		$this->appManager = $appManager;
 	}
 
 	/**
@@ -99,4 +114,28 @@ class BackupCodesProvider implements IProvider {
 		return $this->storage->hasBackupCodes($user);
 	}
 
+	/**
+	 * Determine whether backup codes should be active or not
+	 *
+	 * Backup codes only make sense if at least one 2FA provider is active,
+	 * hence this method checks all enabled apps on whether they provide 2FA
+	 * functionality or not. If there's at least one app, backup codes are
+	 * enabled on the personal settings page.
+	 *
+	 * @param IUser $user
+	 * @return boolean
+	 */
+	public function isActive(IUser $user) {
+		$appIds = array_filter($this->appManager->getEnabledAppsForUser($user), function($appId) {
+			return $appId !== $this->appName;
+		});
+		foreach ($appIds as $appId) {
+			$info = $this->appManager->getAppInfo($appId);
+			if (isset($info['two-factor-providers']) && count($info['two-factor-providers']) > 0) {
+				return true;
+			}
+		}
+		return false;
+	}
+
 }
diff --git a/apps/twofactor_backupcodes/settings/personal.php b/apps/twofactor_backupcodes/settings/personal.php
index 0a018c0ff28..48c84a3355e 100644
--- a/apps/twofactor_backupcodes/settings/personal.php
+++ b/apps/twofactor_backupcodes/settings/personal.php
@@ -1,6 +1,19 @@
 <?php
+
+use OCA\TwoFactorBackupCodes\Provider\BackupCodesProvider;
+use OCP\Template;
+
 // @codeCoverageIgnoreStart
-$tmpl = new \OCP\Template('twofactor_backupcodes', 'personal');
 
-return $tmpl->fetchPage();
+/* @var $provider BackupCodesProvider */
+$provider = OC::$server->query(BackupCodesProvider::class);
+$user = OC::$server->getUserSession()->getUser();
+
+if ($provider->isActive($user)) {
+	$tmpl = new Template('twofactor_backupcodes', 'personal');
+	return $tmpl->fetchPage();
+} else {
+	return "";
+}
+
 // @codeCoverageIgnoreEnd
diff --git a/apps/twofactor_backupcodes/tests/Unit/Provider/BackupCodesProviderTest.php b/apps/twofactor_backupcodes/tests/Unit/Provider/BackupCodesProviderTest.php
index 5a99cfadd41..cec5b7b2160 100644
--- a/apps/twofactor_backupcodes/tests/Unit/Provider/BackupCodesProviderTest.php
+++ b/apps/twofactor_backupcodes/tests/Unit/Provider/BackupCodesProviderTest.php
@@ -22,32 +22,41 @@
 
 namespace OCA\TwoFactorBackupCodes\Tests\Unit\Provider;
 
+use OC\App\AppManager;
 use OCA\TwoFactorBackupCodes\Provider\BackupCodesProvider;
 use OCA\TwoFactorBackupCodes\Service\BackupCodeStorage;
 use OCP\IL10N;
 use OCP\IUser;
 use OCP\Template;
+use PHPUnit_Framework_MockObject_MockObject;
 use Test\TestCase;
 
 class BackupCodesProviderTest extends TestCase {
 
+	/** @var string */
+	private $appName;
+
 	/** @var BackupCodeStorage|PHPUnit_Framework_MockObject_MockObject */
 	private $storage;
 
 	/** @var IL10N|PHPUnit_Framework_MockObject_MockObject */
 	private $l10n;
 
+	/** @var AppManager|PHPUnit_Framework_MockObject_MockObject */
+	private $appManager;
+
 	/** @var BackupCodesProvider */
 	private $provider;
 
 	protected function setUp() {
 		parent::setUp();
 
-		$this->storage = $this->getMockBuilder(BackupCodeStorage::class)
-			->disableOriginalConstructor()
-			->getMock();
-		$this->l10n = $this->getMockBuilder(IL10N::class)->getMock();
-		$this->provider = new BackupCodesProvider($this->storage, $this->l10n);
+		$this->appName = "twofactor_backupcodes";
+		$this->storage = $this->createMock(BackupCodeStorage::class);
+		$this->l10n = $this->createMock(IL10N::class);
+		$this->appManager = $this->createMock(AppManager::class);
+
+		$this->provider = new BackupCodesProvider($this->appName, $this->storage, $this->l10n, $this->appManager);
 	}
 
 	public function testGetId() {
@@ -100,4 +109,46 @@ class BackupCodesProviderTest extends TestCase {
 		$this->assertTrue($this->provider->isTwoFactorAuthEnabledForUser($user));
 	}
 
+	public function testIsActiveNoProviders() {
+		$user = $this->getMockBuilder(IUser::class)->getMock();
+
+		$this->appManager->expects($this->once())
+			->method('getEnabledAppsForUser')
+			->with($user)
+			->willReturn([
+				'twofactor_backupcodes',
+				'mail',
+		]);
+		$this->appManager->expects($this->once())
+			->method('getAppInfo')
+			->with('mail')
+			->willReturn([
+				'two-factor-providers' => [],
+		]);
+
+		$this->assertFalse($this->provider->isActive($user));
+	}
+
+	public function testIsActiveWithProviders() {
+		$user = $this->getMockBuilder(IUser::class)->getMock();
+
+		$this->appManager->expects($this->once())
+			->method('getEnabledAppsForUser')
+			->with($user)
+			->willReturn([
+				'twofactor_backupcodes',
+				'twofactor_u2f',
+		]);
+		$this->appManager->expects($this->once())
+			->method('getAppInfo')
+			->with('twofactor_u2f')
+			->willReturn([
+				'two-factor-providers' => [
+					'OCA\TwoFactorU2F\Provider\U2FProvider',
+				],
+		]);
+
+		$this->assertTrue($this->provider->isActive($user));
+	}
+
 }