From 232d7358934ab8e1fa5e871c37f0997e5f394e86 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?= <thomas.mueller@tmit.eu>
Date: Thu, 9 Jun 2016 16:44:31 +0200
Subject: [PATCH] Do not leak the login name - fixes #25047

---
 core/Controller/LoginController.php           |  5 +-
 tests/Core/Controller/LoginControllerTest.php | 48 +++++++++++++++----
 2 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php
index c64f58ae2c..7806e1de90 100644
--- a/core/Controller/LoginController.php
+++ b/core/Controller/LoginController.php
@@ -171,6 +171,7 @@ class LoginController extends Controller {
 	 * @return RedirectResponse
 	 */
 	public function tryLogin($user, $password, $redirect_url) {
+		$originalUser = $user;
 		// TODO: Add all the insane error handling
 		/* @var $loginResult IUser */
 		$loginResult = $this->userManager->checkPassword($user, $password);
@@ -186,8 +187,8 @@ class LoginController extends Controller {
 			$this->session->set('loginMessages', [
 				['invalidpassword']
 			]);
-			// Read current user and append if possible
-			$args = !is_null($user) ? ['user' => $user] : [];
+			// Read current user and append if possible - we need to return the unmodified user otherwise we will leak the login name
+			$args = !is_null($user) ? ['user' => $originalUser] : [];
 			return new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args));
 		}
 		// TODO: remove password checks from above and let the user session handle failures
diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php
index ea9d6a4414..d6fa772d38 100644
--- a/tests/Core/Controller/LoginControllerTest.php
+++ b/tests/Core/Controller/LoginControllerTest.php
@@ -29,6 +29,7 @@ use OCP\IConfig;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\IURLGenerator;
+use OCP\IUser;
 use OCP\IUserManager;
 use OCP\IUserSession;
 use Test\TestCase;
@@ -36,19 +37,19 @@ use Test\TestCase;
 class LoginControllerTest extends TestCase {
 	/** @var LoginController */
 	private $loginController;
-	/** @var IRequest */
+	/** @var IRequest | \PHPUnit_Framework_MockObject_MockObject */
 	private $request;
-	/** @var IUserManager */
+	/** @var IUserManager | \PHPUnit_Framework_MockObject_MockObject */
 	private $userManager;
-	/** @var IConfig */
+	/** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */
 	private $config;
-	/** @var ISession */
+	/** @var ISession | \PHPUnit_Framework_MockObject_MockObject */
 	private $session;
-	/** @var IUserSession */
+	/** @var IUserSession | \PHPUnit_Framework_MockObject_MockObject */
 	private $userSession;
-	/** @var IURLGenerator */
+	/** @var IURLGenerator | \PHPUnit_Framework_MockObject_MockObject */
 	private $urlGenerator;
-	/** @var Manager */
+	/** @var Manager | \PHPUnit_Framework_MockObject_MockObject */
 	private $twoFactorManager;
 
 	public function setUp() {
@@ -296,6 +297,7 @@ class LoginControllerTest extends TestCase {
 	}
 
 	public function testLoginWithValidCredentials() {
+		/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->getMock('\OCP\IUser');
 		$password = 'secret';
 		$indexPageUrl = 'some url';
@@ -323,6 +325,7 @@ class LoginControllerTest extends TestCase {
 	}
 
 	public function testLoginWithValidCredentialsAndRedirectUrl() {
+		/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->getMock('\OCP\IUser');
 		$user->expects($this->any())
 			->method('getUID')
@@ -352,6 +355,7 @@ class LoginControllerTest extends TestCase {
 	}
 	
 	public function testLoginWithTwoFactorEnforced() {
+		/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->getMock('\OCP\IUser');
 		$user->expects($this->any())
 			->method('getUID')
@@ -380,8 +384,36 @@ class LoginControllerTest extends TestCase {
 			->with('core.TwoFactorChallenge.selectChallenge')
 			->will($this->returnValue($challengeUrl));
 
-		$expected = new \OCP\AppFramework\Http\RedirectResponse($challengeUrl);
+		$expected = new RedirectResponse($challengeUrl);
 		$this->assertEquals($expected, $this->loginController->tryLogin('john@doe.com', $password, null));
 	}
 
+	public function testToNotLeakLoginName() {
+		/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */
+		$user = $this->getMock('\OCP\IUser');
+		$user->expects($this->any())
+			->method('getUID')
+			->will($this->returnValue('john'));
+
+		$this->userManager->expects($this->exactly(2))
+			->method('checkPassword')
+			->withConsecutive(
+				['john@doe.com', 'just wrong'],
+				['john', 'just wrong']
+				)
+			->willReturn(false);
+		
+		$this->userManager->expects($this->once())
+			->method('getByEmail')
+			->with('john@doe.com')
+			->willReturn([$user]);
+
+		$this->urlGenerator->expects($this->once())
+			->method('linkToRoute')
+			->with('core.login.showLoginForm', ['user' => 'john@doe.com'])
+			->will($this->returnValue(''));
+
+		$expected = new RedirectResponse('');
+		$this->assertEquals($expected, $this->loginController->tryLogin('john@doe.com', 'just wrong', null));
+	}
 }