From d8b3fe460e8fc12d7611f0e2f3f469e791f4c090 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Tue, 2 May 2017 14:40:44 +0200 Subject: [PATCH] Hide 2FA backup codes if no other 2FA providers are enabled Signed-off-by: Christoph Wurst --- .../lib/Provider/BackupCodesProvider.php | 41 ++++++++++++- .../settings/personal.php | 19 +++++- .../Unit/Provider/BackupCodesProviderTest.php | 61 +++++++++++++++++-- 3 files changed, 112 insertions(+), 9 deletions(-) diff --git a/apps/twofactor_backupcodes/lib/Provider/BackupCodesProvider.php b/apps/twofactor_backupcodes/lib/Provider/BackupCodesProvider.php index 902f7c783d..5c5500862e 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 0a018c0ff2..48c84a3355 100644 --- a/apps/twofactor_backupcodes/settings/personal.php +++ b/apps/twofactor_backupcodes/settings/personal.php @@ -1,6 +1,19 @@ fetchPage(); +use OCA\TwoFactorBackupCodes\Provider\BackupCodesProvider; +use OCP\Template; + +// @codeCoverageIgnoreStart + +/* @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 5a99cfadd4..cec5b7b216 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)); + } + }