Let users confirm their TOTP setup before enforcing the provider

Users might have trouble get their TOTP app to work. By checking
whether their basic setup is configured correctly, we lower the
risk of users being locked out unexpectedly.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

2 files changed, 59 insertions, 16 deletions

diff --git a/tests/Acceptance/TOTPAcceptanceTest.php b/tests/Acceptance/TOTPAcceptanceTest.php
index 61397c2..9d10074 100644
--- a/tests/Acceptance/TOTPAcceptanceTest.php
+++ b/tests/Acceptance/TOTPAcceptanceTest.php
@@ -49,11 +49,16 @@ class TOTPAcceptenceTest extends AcceptanceTest {
 
 		$this->user = OC::$server->getUserManager()->get('admin');
 		$this->secretMapper = new TotpSecretMapper(OC::$server->getDatabaseConnection());
+		$this->cleanUp();
 	}
 
 	protected function tearDown() {
 		parent::tearDown();
 
+		$this->cleanUp();
+	}
+
+	private function cleanUp() {
 		// Always delete secret again
 		try {
 			$secret = $this->secretMapper->getSecret($this->user);
@@ -91,10 +96,9 @@ class TOTPAcceptenceTest extends AcceptanceTest {
 			} catch (ElementNotSelectableException $ex) {
 				return false;
 			}
-			return true;
 		});
 		$this->webDriver->executeScript('arguments[0].click(); console.log(arguments[0]);', [
-		    $this->webDriver->findElement(WebDriverBy::id('totp-enabled')),
+			$this->webDriver->findElement(WebDriverBy::id('totp-enabled')),
 		]);
 		$this->webDriver->wait(20, 1000)->until(WebDriverExpectedCondition::elementTextContains(WebDriverBy::id('twofactor-totp-settings'), 'This is your new TOTP secret:'));
 	}
@@ -119,9 +123,19 @@ class TOTPAcceptenceTest extends AcceptanceTest {
 		$this->webDriver->findElement(WebDriverBy::id('password'))->sendKeys('admin');
 		$this->webDriver->findElement(WebDriverBy::cssSelector('form[name=login] input[type=submit]'))->click();
 
+		$this->webDriver->wait(20, 1000)->until(function(WebDriver $driver) {
+			try {
+				return $driver->findElements(WebDriverBy::className('totp-form'));
+			} catch (ElementNotSelectableException $ex) {
+				return false;
+			}
+		});
+
 		// Enter a wrong OTP
-		$this->webDriver->findElement(WebDriverBy::name('challenge'))->sendKeys('000');
-		$this->webDriver->findElement(WebDriverBy::cssSelector('button[type="submit"]'))->click();
+		$this->webDriver->findElement(WebDriverBy::name('challenge'))->sendKeys('000000');
+		$this->webDriver->findElement(WebDriverBy::cssSelector('button[type="submit"]'))->submit();
+
+		$this->webDriver->wait(20, 1000)->until(WebDriverExpectedCondition::elementTextContains(WebDriverBy::className('warning'), 'Error while validating your second factor'));
 
 		$this->assertEquals('http://localhost:8080/index.php/login/challenge/totp', $this->webDriver->getCurrentURL());
 	}
diff --git a/tests/Unit/Controller/SettingsControllerTest.php b/tests/Unit/Controller/SettingsControllerTest.php
index f011c5d..c82e052 100644
--- a/tests/Unit/Controller/SettingsControllerTest.php
+++ b/tests/Unit/Controller/SettingsControllerTest.php
@@ -23,15 +23,17 @@
 namespace OCA\TwoFactorTOTP\Unit\Controller;
 
 use Endroid\QrCode\QrCode;
+use InvalidArgumentException;
 use OCA\TwoFactorTOTP\Controller\SettingsController;
+use OCA\TwoFactorTOTP\Service\ITotp;
 use OCA\TwoFactorTOTP\Service\Totp;
 use OCP\Defaults;
 use OCP\IRequest;
 use OCP\IUser;
 use OCP\IUserSession;
-use Test\TestCase;
+use PHPUnit_Framework_TestCase;
 
-class SettingsControllerTest extends TestCase {
+class SettingsControllerTest extends PHPUnit_Framework_TestCase {
 
 	private $request;
 	private $userSession;
@@ -52,7 +54,7 @@ class SettingsControllerTest extends TestCase {
 		$this->controller = new SettingsController('twofactor_totp', $this->request, $this->userSession, $this->totp, $this->defaults);
 	}
 
-	public function testNothing() {
+	public function testDisabledState() {
 		$user = $this->createMock(IUser::class);
 		$this->userSession->expects($this->once())
 			->method('getUser')
@@ -60,16 +62,16 @@ class SettingsControllerTest extends TestCase {
 		$this->totp->expects($this->once())
 			->method('hasSecret')
 			->with($user)
-			->will($this->returnValue(true));
+			->will($this->returnValue(false));
 
 		$expected = [
-		    'enabled' => true,
+			'state' => false,
 		];
 
 		$this->assertEquals($expected, $this->controller->state());
 	}
 
-	public function testEnable() {
+	public function testCreateSecret() {
 		$user = $this->createMock(IUser::class);
 		$this->userSession->expects($this->exactly(2))
 			->method('getUser')
@@ -89,15 +91,32 @@ class SettingsControllerTest extends TestCase {
 			->getDataUri();
 
 		$expected = [
-		    'enabled' => true,
-		    'secret' => 'newsecret',
-		    'qr' => $qr,
+			'state' => ITotp::STATE_CREATED,
+			'secret' => 'newsecret',
+			'qr' => $qr,
 		];
 
 		$this->assertEquals($expected, $this->controller->enable(true));
 	}
 
-	public function testEnableDisable() {
+	public function testEnableSecret() {
+		$user = $this->createMock(IUser::class);
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+		$this->totp->expects($this->once())
+			->method('enable')
+			->with($user, '123456')
+			->willReturn(true);
+
+		$expected = [
+			'state' => ITotp::STATE_ENABLED,
+		];
+
+		$this->assertEquals($expected, $this->controller->enable(ITotp::STATE_ENABLED, '123456'));
+	}
+
+	public function testDisableSecret() {
 		$user = $this->createMock(IUser::class);
 		$this->userSession->expects($this->once())
 			->method('getUser')
@@ -106,10 +125,20 @@ class SettingsControllerTest extends TestCase {
 			->method('deleteSecret');
 
 		$expected = [
-		    'enabled' => false,
+			'state' => ITotp::STATE_DISABLED,
 		];
 
-		$this->assertEquals($expected, $this->controller->enable(false));
+		$this->assertEquals($expected, $this->controller->enable(ITotp::STATE_DISABLED));
+	}
+
+	public function testEnableInvalidState() {
+		$user = $this->createMock(IUser::class);
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+
+		$this->expectException(InvalidArgumentException::class);
+		$this->controller->enable(17);
 	}
 
 }