mbk-lab
/
nextcloud
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Do not leak the login name - fixes #25047

This commit is contained in:
Thomas Müller 2016-06-09 16:44:31 +02:00
parent c13c561219
commit 232d735893
No known key found for this signature in database
GPG Key ID: A943788A3BBEC44C
2 changed files with 43 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
core/Controller/LoginController.php
View File

@ -171,6 +171,7 @@ class LoginController extends Controller {
* @return RedirectResponse * @return RedirectResponse
*/ */
public function tryLogin($user, $password, $redirect_url) { public function tryLogin($user, $password, $redirect_url) {
$originalUser = $user;
// TODO: Add all the insane error handling // TODO: Add all the insane error handling
/* @var $loginResult IUser */ /* @var $loginResult IUser */
$loginResult = $this->userManager->checkPassword($user, $password); $loginResult = $this->userManager->checkPassword($user, $password);
@ -186,8 +187,8 @@ class LoginController extends Controller {
$this->session->set('loginMessages', [ $this->session->set('loginMessages', [
['invalidpassword'] ['invalidpassword']
]); ]);
// Read current user and append if possible // 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' => $user] : []; $args = !is_null($user) ? ['user' => $originalUser] : [];
return new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args)); return new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args));
} }
// TODO: remove password checks from above and let the user session handle failures // TODO: remove password checks from above and let the user session handle failures

48
tests/Core/Controller/LoginControllerTest.php
View File

@ -29,6 +29,7 @@ use OCP\IConfig;
use OCP\IRequest; use OCP\IRequest;
use OCP\ISession; use OCP\ISession;
use OCP\IURLGenerator; use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager; use OCP\IUserManager;
use OCP\IUserSession; use OCP\IUserSession;
use Test\TestCase; use Test\TestCase;
@ -36,19 +37,19 @@ use Test\TestCase;
class LoginControllerTest extends TestCase { class LoginControllerTest extends TestCase {
/** @var LoginController */ /** @var LoginController */
private $loginController; private $loginController;
/** @var IRequest */ /** @var IRequest | \PHPUnit_Framework_MockObject_MockObject */
private $request; private $request;
/** @var IUserManager */ /** @var IUserManager | \PHPUnit_Framework_MockObject_MockObject */
private $userManager; private $userManager;
/** @var IConfig */ /** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */
private $config; private $config;
/** @var ISession */ /** @var ISession | \PHPUnit_Framework_MockObject_MockObject */
private $session; private $session;
/** @var IUserSession */ /** @var IUserSession | \PHPUnit_Framework_MockObject_MockObject */
private $userSession; private $userSession;
/** @var IURLGenerator */ /** @var IURLGenerator | \PHPUnit_Framework_MockObject_MockObject */
private $urlGenerator; private $urlGenerator;
/** @var Manager */ /** @var Manager | \PHPUnit_Framework_MockObject_MockObject */
private $twoFactorManager; private $twoFactorManager;
public function setUp() { public function setUp() {
@ -296,6 +297,7 @@ class LoginControllerTest extends TestCase {
} }
public function testLoginWithValidCredentials() { public function testLoginWithValidCredentials() {
/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */
$user = $this->getMock('\OCP\IUser'); $user = $this->getMock('\OCP\IUser');
$password = 'secret'; $password = 'secret';
$indexPageUrl = 'some url'; $indexPageUrl = 'some url';
@ -323,6 +325,7 @@ class LoginControllerTest extends TestCase {
} }
public function testLoginWithValidCredentialsAndRedirectUrl() { public function testLoginWithValidCredentialsAndRedirectUrl() {
/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */
$user = $this->getMock('\OCP\IUser'); $user = $this->getMock('\OCP\IUser');
$user->expects($this->any()) $user->expects($this->any())
->method('getUID') ->method('getUID')
@ -352,6 +355,7 @@ class LoginControllerTest extends TestCase {
} }
public function testLoginWithTwoFactorEnforced() { public function testLoginWithTwoFactorEnforced() {
/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */
$user = $this->getMock('\OCP\IUser'); $user = $this->getMock('\OCP\IUser');
$user->expects($this->any()) $user->expects($this->any())
->method('getUID') ->method('getUID')
@ -380,8 +384,36 @@ class LoginControllerTest extends TestCase {
->with('core.TwoFactorChallenge.selectChallenge') ->with('core.TwoFactorChallenge.selectChallenge')
->will($this->returnValue($challengeUrl)); ->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)); $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));
}
} }