From 5e74265c92a58d6c1ba6f52b900a23ae56413166 Mon Sep 17 00:00:00 2001 From: Semih Serhat Karakaya Date: Sat, 24 Jun 2017 09:37:52 -0700 Subject: [PATCH 1/5] loginController now emitting preLogin hook signal before failed login attempts --- core/Controller/LoginController.php | 17 ++++----- tests/Core/Controller/LoginControllerTest.php | 36 +++++++++++-------- 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index afd547ab2103..f571140f1627 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -197,17 +197,11 @@ public function showLoginForm($user, $redirect_url, $remember_login) { 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); - if ($loginResult === false) { - $users = $this->userManager->getByEmail($user); - // we only allow login by email if unique - if (count($users) === 1) { - $user = $users[0]->getUID(); - $loginResult = $this->userManager->checkPassword($user, $password); - } + $emailUsers = $this->userManager->getByEmail($user); + if (count($emailUsers) === 1) { + $user = $emailUsers[0]->getUID(); } - if ($loginResult === false) { + if ($this->userSession->login($user, $password) !== true) { $this->session->set('loginMessages', [ ['invalidpassword'], [] ]); @@ -222,9 +216,10 @@ public function tryLogin($user, $password, $redirect_url) { } return new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args)); } + /* @var $loginResult IUser */ + $loginResult = $this->userManager->get($user); // TODO: remove password checks from above and let the user session handle failures // requires https://github.com/owncloud/core/pull/24616 - $this->userSession->login($user, $password); $this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password); // User has successfully logged in, now remove the password reset link, when it is available diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 442b339990d1..b10a8548e20c 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -307,8 +307,8 @@ public function testLoginWithInvalidCredentials() { $password = 'secret'; $loginPageUrl = 'some url'; - $this->userManager->expects($this->once()) - ->method('checkPassword') + $this->userSession->expects($this->once()) + ->method('login') ->will($this->returnValue(false)); $this->urlGenerator->expects($this->once()) ->method('linkToRoute') @@ -328,12 +328,14 @@ public function testLoginWithValidCredentials() { $password = 'secret'; $indexPageUrl = 'some url'; - $this->userManager->expects($this->once()) - ->method('checkPassword') - ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('login') - ->with($user, $password); + ->with($user, $password) + ->will($this->returnValue(true)); + $this->userManager->expects($this->once()) + ->method('get') + ->with($user) + ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('createSessionToken') ->with($this->request, $user->getUID(), $user, $password); @@ -374,9 +376,13 @@ public function testLoginWithValidCredentialsAndRedirectUrl() { $originalUrl = 'another%20url'; $redirectUrl = 'http://localhost/another url'; - $this->userManager->expects($this->once()) - ->method('checkPassword') + $this->userSession->expects($this->once()) + ->method('login') ->with('Jane', $password) + ->will($this->returnValue(true)); + $this->userManager->expects($this->once()) + ->method('get') + ->with('Jane') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('createSessionToken') @@ -403,8 +409,11 @@ public function testLoginWithTwoFactorEnforced() { $password = 'secret'; $challengeUrl = 'challenge/url'; + $this->userSession->expects($this->once()) + ->method('login') + ->will($this->returnValue(true)); $this->userManager->expects($this->once()) - ->method('checkPassword') + ->method('get') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('login') @@ -435,12 +444,9 @@ public function testToNotLeakLoginName() { ->method('getUID') ->will($this->returnValue('john')); - $this->userManager->expects($this->exactly(2)) - ->method('checkPassword') - ->withConsecutive( - ['john@doe.com', 'just wrong'], - ['john', 'just wrong'] - ) + $this->userSession->expects($this->once()) + ->method('login') + ->with('john', 'just wrong') ->willReturn(false); $this->userManager->expects($this->once()) From f8e29401d04024ee4e6f5ae051383c431b6a0d00 Mon Sep 17 00:00:00 2001 From: Semih Serhat Karakaya Date: Wed, 28 Jun 2017 03:52:21 -0700 Subject: [PATCH 2/5] add failedLogin hook to detect failed login attempts --- core/Controller/LoginController.php | 14 ++++++++++---- lib/private/User/Session.php | 4 +++- tests/Core/Controller/LoginControllerTest.php | 8 ++++++-- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index f571140f1627..3a6b82a15c2e 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -3,6 +3,7 @@ * @author Christoph Wurst * @author Joas Schilling * @author Lukas Reschke + * @author Semih Serhat Karakaya * @author Thomas Müller * * @copyright Copyright (c) 2017, ownCloud GmbH @@ -197,11 +198,16 @@ public function showLoginForm($user, $redirect_url, $remember_login) { public function tryLogin($user, $password, $redirect_url) { $originalUser = $user; // TODO: Add all the insane error handling - $emailUsers = $this->userManager->getByEmail($user); - if (count($emailUsers) === 1) { - $user = $emailUsers[0]->getUID(); + $loginResult = $this->userSession->login($user, $password); + if ($loginResult !== true) { + $users = $this->userManager->getByEmail($user); + // we only allow login by email if unique + if (count($users) === 1) { + $user = $users[0]->getUID(); + $loginResult = $this->userSession->login($user, $password); + } } - if ($this->userSession->login($user, $password) !== true) { + if ($loginResult !== true) { $this->session->set('loginMessages', [ ['invalidpassword'], [] ]); diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 2bbc29d33e9c..be3a960daac6 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -9,6 +9,7 @@ * @author Morris Jobke * @author Robin Appelman * @author Robin McCorkell + * @author Semih Serhat Karakaya * @author Thomas Müller * @author Vincent Petry * @@ -70,6 +71,7 @@ * - postCreateUser(\OC\User\User $user) * - preLogin(string $user, string $password) * - postLogin(\OC\User\User $user, string $password) + * - failedLogin(string $user) * - preRememberedLogin(string $uid) * - postRememberedLogin(\OC\User\User $user) * - logout() @@ -464,7 +466,7 @@ private function loginWithPassword($uid, $password) { $this->manager->emit('\OC\User', 'preLogin', [$uid, $password]); $user = $this->manager->checkPassword($uid, $password); if ($user === false) { - // Password check failed + $this->manager->emit('\OC\User', 'failedLogin', [$uid]); return false; } diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index b10a8548e20c..0690c8a14b92 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -1,6 +1,7 @@ + * @author Semih Serhat Karakaya * * @copyright Copyright (c) 2016, ownCloud, Inc. * @license AGPL-3.0 @@ -444,9 +445,12 @@ public function testToNotLeakLoginName() { ->method('getUID') ->will($this->returnValue('john')); - $this->userSession->expects($this->once()) + $this->userSession->expects($this->exactly(2)) ->method('login') - ->with('john', 'just wrong') + ->withConsecutive( + ['john@doe.com', 'just wrong'], + ['john', 'just wrong'] + ) ->willReturn(false); $this->userManager->expects($this->once()) From 134b685871b248050d22954196d96832fdf2ac81 Mon Sep 17 00:00:00 2001 From: Friedrich Weber Date: Thu, 13 Jul 2017 15:27:32 +0200 Subject: [PATCH 3/5] Login: Use userSession to determine the uid This fixes user logins using the user_ldap app. --- core/Controller/LoginController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 3a6b82a15c2e..b4fc29c8dc0f 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -223,7 +223,7 @@ public function tryLogin($user, $password, $redirect_url) { return new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args)); } /* @var $loginResult IUser */ - $loginResult = $this->userManager->get($user); + $loginResult = $this->userSession->getUser(); // TODO: remove password checks from above and let the user session handle failures // requires https://github.com/owncloud/core/pull/24616 $this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password); From 1170cf4f06e4c4a72fec4f2931f84a9e26b3e884 Mon Sep 17 00:00:00 2001 From: Friedrich Weber Date: Thu, 20 Jul 2017 16:45:33 +0200 Subject: [PATCH 4/5] Update tests to work with new tryLogin call structure --- tests/Core/Controller/LoginControllerTest.php | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 0690c8a14b92..74ea7c4ae37b 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -333,9 +333,8 @@ public function testLoginWithValidCredentials() { ->method('login') ->with($user, $password) ->will($this->returnValue(true)); - $this->userManager->expects($this->once()) - ->method('get') - ->with($user) + $this->userSession->expects($this->once()) + ->method('getUser') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('createSessionToken') @@ -381,9 +380,8 @@ public function testLoginWithValidCredentialsAndRedirectUrl() { ->method('login') ->with('Jane', $password) ->will($this->returnValue(true)); - $this->userManager->expects($this->once()) - ->method('get') - ->with('Jane') + $this->userSession->expects($this->once()) + ->method('getUser') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('createSessionToken') @@ -413,8 +411,8 @@ public function testLoginWithTwoFactorEnforced() { $this->userSession->expects($this->once()) ->method('login') ->will($this->returnValue(true)); - $this->userManager->expects($this->once()) - ->method('get') + $this->userSession->expects($this->once()) + ->method('getUser') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('login') From 19101ebd44e5b70453da813332c88e06cb4d8720 Mon Sep 17 00:00:00 2001 From: Friedrich Weber Date: Mon, 24 Jul 2017 20:54:31 +0200 Subject: [PATCH 5/5] Rename second `$loginResult` occurrence --- core/Controller/LoginController.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index b4fc29c8dc0f..4df09ec1564d 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -222,17 +222,17 @@ public function tryLogin($user, $password, $redirect_url) { } return new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args)); } - /* @var $loginResult IUser */ - $loginResult = $this->userSession->getUser(); + /* @var $userObject IUser */ + $userObject = $this->userSession->getUser(); // TODO: remove password checks from above and let the user session handle failures // requires https://github.com/owncloud/core/pull/24616 - $this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password); + $this->userSession->createSessionToken($this->request, $userObject->getUID(), $user, $password); // User has successfully logged in, now remove the password reset link, when it is available - $this->config->deleteUserValue($loginResult->getUID(), 'owncloud', 'lostpassword'); + $this->config->deleteUserValue($userObject->getUID(), 'owncloud', 'lostpassword'); - if ($this->twoFactorManager->isTwoFactorAuthenticated($loginResult)) { - $this->twoFactorManager->prepareTwoFactorLogin($loginResult); + if ($this->twoFactorManager->isTwoFactorAuthenticated($userObject)) { + $this->twoFactorManager->prepareTwoFactorLogin($userObject); if (!is_null($redirect_url)) { return new RedirectResponse($this->urlGenerator->linkToRoute('core.TwoFactorChallenge.selectChallenge', [ 'redirect_url' => $redirect_url