Merge pull request #1169 from nextcloud/2fa-single-provider

redirect to 2fa provider if there's only one active for the user

2 files changed, 93 insertions, 5 deletions

diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php
index 67e1e215289..b686b34b2ce 100644
--- a/core/Controller/LoginController.php
+++ b/core/Controller/LoginController.php
@@ -242,12 +242,26 @@ class LoginController extends Controller {
 
 		if ($this->twoFactorManager->isTwoFactorAuthenticated($loginResult)) {
 			$this->twoFactorManager->prepareTwoFactorLogin($loginResult);
+
+			$providers = $this->twoFactorManager->getProviders($loginResult);
+			if (count($providers) === 1) {
+				// Single provider, hence we can redirect to that provider's challenge page directly
+				/* @var $provider IProvider */
+				$provider = array_pop($providers);
+				$url = 'core.TwoFactorChallenge.showChallenge';
+				$urlParams = [
+					'challengeProviderId' => $provider->getId(),
+				];
+			} else {
+				$url = 'core.TwoFactorChallenge.selectChallenge';
+				$urlParams = [];
+			}
+
 			if (!is_null($redirect_url)) {
-				return new RedirectResponse($this->urlGenerator->linkToRoute('core.TwoFactorChallenge.selectChallenge', [
-					'redirect_url' => $redirect_url
-				]));
+				$urlParams['redirect_url'] = $redirect_url;
 			}
-			return new RedirectResponse($this->urlGenerator->linkToRoute('core.TwoFactorChallenge.selectChallenge'));
+
+			return new RedirectResponse($this->urlGenerator->linkToRoute($url, $urlParams));
 		}
 
 		return $this->generateRedirect($redirect_url);
diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php
index 417a60a9e5f..ff50ac98fbd 100644
--- a/tests/Core/Controller/LoginControllerTest.php
+++ b/tests/Core/Controller/LoginControllerTest.php
@@ -505,7 +505,7 @@ class LoginControllerTest extends TestCase {
 		$this->assertEquals($expected, $this->loginController->tryLogin('Jane', $password, $originalUrl));
 	}
 	
-	public function testLoginWithTwoFactorEnforced() {
+	public function testLoginWithOneTwoFactorProvider() {
 		/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->getMockBuilder('\OCP\IUser')->getMock();
 		$user->expects($this->any())
@@ -513,6 +513,7 @@ class LoginControllerTest extends TestCase {
 			->will($this->returnValue('john'));
 		$password = 'secret';
 		$challengeUrl = 'challenge/url';
+		$provider = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock();
 
 		$this->request
 			->expects($this->exactly(2))
@@ -547,6 +548,79 @@ class LoginControllerTest extends TestCase {
 		$this->twoFactorManager->expects($this->once())
 			->method('prepareTwoFactorLogin')
 			->with($user);
+		$this->twoFactorManager->expects($this->once())
+			->method('getProviders')
+			->with($user)
+			->will($this->returnValue([$provider]));
+		$provider->expects($this->once())
+			->method('getId')
+			->will($this->returnValue('u2f'));
+		$this->urlGenerator->expects($this->once())
+			->method('linkToRoute')
+			->with('core.TwoFactorChallenge.showChallenge', [
+				'challengeProviderId' => 'u2f',
+			])
+			->will($this->returnValue($challengeUrl));
+		$this->config->expects($this->once())
+			->method('deleteUserValue')
+			->with('john', 'core', 'lostpassword');
+
+		$expected = new RedirectResponse($challengeUrl);
+		$this->assertEquals($expected, $this->loginController->tryLogin('john@doe.com', $password, null));
+	}
+
+	public function testLoginWithMultpleTwoFactorProviders() {
+		/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */
+		$user = $this->getMockBuilder('\OCP\IUser')->getMock();
+		$user->expects($this->any())
+			->method('getUID')
+			->will($this->returnValue('john'));
+		$password = 'secret';
+		$challengeUrl = 'challenge/url';
+		$provider1 = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock();
+		$provider2 = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')->getMock();
+
+		$this->request
+			->expects($this->exactly(2))
+			->method('getRemoteAddress')
+			->willReturn('192.168.0.1');
+		$this->request
+			->expects($this->once())
+			->method('passesCSRFCheck')
+			->willReturn(true);
+		$this->throttler
+			->expects($this->once())
+			->method('sleepDelay')
+			->with('192.168.0.1');
+		$this->throttler
+			->expects($this->once())
+			->method('getDelay')
+			->with('192.168.0.1')
+			->willReturn(200);
+		$this->userManager->expects($this->once())
+			->method('checkPassword')
+			->will($this->returnValue($user));
+		$this->userSession->expects($this->once())
+			->method('login')
+			->with('john@doe.com', $password);
+		$this->userSession->expects($this->once())
+			->method('createSessionToken')
+			->with($this->request, $user->getUID(), 'john@doe.com', $password);
+		$this->twoFactorManager->expects($this->once())
+			->method('isTwoFactorAuthenticated')
+			->with($user)
+			->will($this->returnValue(true));
+		$this->twoFactorManager->expects($this->once())
+			->method('prepareTwoFactorLogin')
+			->with($user);
+		$this->twoFactorManager->expects($this->once())
+			->method('getProviders')
+			->with($user)
+			->will($this->returnValue([$provider1, $provider2]));
+		$provider1->expects($this->never())
+			->method('getId');
+		$provider2->expects($this->never())
+			->method('getId');
 		$this->urlGenerator->expects($this->once())
 			->method('linkToRoute')
 			->with('core.TwoFactorChallenge.selectChallenge')