From 2994cbc586449caaa07d0a5a1934848da0a3ffa5 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 30 Nov 2016 14:59:59 +0100 Subject: [PATCH 1/8] fix login controller tests Signed-off-by: Arthur Schiwon --- core/Controller/LoginController.php | 3 +++ tests/Core/Controller/LoginControllerTest.php | 14 ++++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index abb1df4bcd..09b0845d67 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -207,6 +207,9 @@ class LoginController extends Controller { * @return RedirectResponse */ public function tryLogin($user, $password, $redirect_url, $remember_login = false, $timezone = '', $timezone_offset = '') { + if(!is_string($user)) { + throw new \InvalidArgumentException('Username must be string'); + } $currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress(), 'login'); $this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login'); diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 72f921724a..51592c2c43 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -334,6 +334,7 @@ class LoginControllerTest extends TestCase { $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('uid')); + $loginName = 'loginli'; $user->expects($this->any()) ->method('getLastLogin') ->willReturn(123456); @@ -362,10 +363,10 @@ class LoginControllerTest extends TestCase { ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('login') - ->with($user, $password); + ->with($loginName, $password); $this->userSession->expects($this->once()) ->method('createSessionToken') - ->with($this->request, $user->getUID(), $user, $password, false); + ->with($this->request, $user->getUID(), $loginName, $password, false); $this->twoFactorManager->expects($this->once()) ->method('isTwoFactorAuthenticated') ->with($user) @@ -387,7 +388,7 @@ class LoginControllerTest extends TestCase { ); $expected = new \OCP\AppFramework\Http\RedirectResponse($indexPageUrl); - $this->assertEquals($expected, $this->loginController->tryLogin($user, $password, null, false, 'Europe/Berlin', '1')); + $this->assertEquals($expected, $this->loginController->tryLogin($loginName, $password, null, false, 'Europe/Berlin', '1')); } public function testLoginWithValidCredentialsAndRememberMe() { @@ -396,6 +397,7 @@ class LoginControllerTest extends TestCase { $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('uid')); + $loginName = 'loginli'; $password = 'secret'; $indexPageUrl = \OC_Util::getDefaultPageUrl(); @@ -421,10 +423,10 @@ class LoginControllerTest extends TestCase { ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('login') - ->with($user, $password); + ->with($loginName, $password); $this->userSession->expects($this->once()) ->method('createSessionToken') - ->with($this->request, $user->getUID(), $user, $password, true); + ->with($this->request, $user->getUID(), $loginName, $password, true); $this->twoFactorManager->expects($this->once()) ->method('isTwoFactorAuthenticated') ->with($user) @@ -437,7 +439,7 @@ class LoginControllerTest extends TestCase { ->with($user); $expected = new \OCP\AppFramework\Http\RedirectResponse($indexPageUrl); - $this->assertEquals($expected, $this->loginController->tryLogin($user, $password, null, true)); + $this->assertEquals($expected, $this->loginController->tryLogin($loginName, $password, null, true)); } public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn() { From 7b3fdfeeaac1ae8e7277ccb0118fd62611c0e302 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 30 Nov 2016 13:28:36 +0100 Subject: [PATCH 2/8] do login routine only once when done via LoginController Signed-off-by: Arthur Schiwon --- core/Controller/LoginController.php | 2 +- lib/private/User/Session.php | 83 +++++++++---------- tests/Core/Controller/LoginControllerTest.php | 16 ++-- 3 files changed, 49 insertions(+), 52 deletions(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 09b0845d67..68acbbd43f 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -250,7 +250,7 @@ class LoginController extends Controller { } // 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->completeLogin($loginResult, ['loginName' => $user, 'password' => $password]); $this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password, (int)$remember_login); // User has successfully logged in, now remove the password reset link, when it is available diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 73a8196cec..05b24c8ccf 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -41,6 +41,7 @@ use OC\Authentication\Exceptions\PasswordLoginForbiddenException; use OC\Authentication\Token\IProvider; use OC\Authentication\Token\IToken; use OC\Hooks\Emitter; +use OC\Hooks\PublicEmitter; use OC_User; use OC_Util; use OCA\DAV\Connector\Sabre\Auth; @@ -78,7 +79,7 @@ use Symfony\Component\EventDispatcher\GenericEvent; */ class Session implements IUserSession, Emitter { - /** @var IUserManager $manager */ + /** @var IUserManager|PublicEmitter $manager */ private $manager; /** @var ISession $session */ @@ -156,7 +157,7 @@ class Session implements IUserSession, Emitter { /** * get the manager object * - * @return Manager + * @return Manager|PublicEmitter */ public function getManager() { return $this->manager; @@ -324,6 +325,41 @@ class Session implements IUserSession, Emitter { return $this->loginWithPassword($uid, $password); } + /** + * @param IUser $user + * @param array $loginDetails + * @return bool + * @throws LoginException + */ + public function completeLogin(IUser $user, array $loginDetails) { + if (!$user->isEnabled()) { + // disabled users can not log in + // injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory + $message = \OC::$server->getL10N('lib')->t('User disabled'); + throw new LoginException($message); + } + + $this->setUser($user); + $this->setLoginName($loginDetails['loginName']); + + if(isset($loginDetails['token']) && $loginDetails['token'] instanceof IToken) { + $this->setToken($loginDetails['token']->getId()); + \OC::$server->getLockdownManager()->setToken($loginDetails['token']); + $firstTimeLogin = false; + } else { + $this->setToken(null); + $firstTimeLogin = $user->updateLastLoginTimestamp(); + } + $this->manager->emit('\OC\User', 'postLogin', [$user, $loginDetails['password']]); + if($this->isLoggedIn()) { + $this->prepareUserLogin($firstTimeLogin); + return true; + } else { + $message = \OC::$server->getL10N('lib')->t('Login canceled by app'); + throw new LoginException($message); + } + } + /** * Tries to log in a client * @@ -498,25 +534,7 @@ class Session implements IUserSession, Emitter { return false; } - if ($user->isEnabled()) { - $this->setUser($user); - $this->setLoginName($uid); - $this->setToken(null); - $firstTimeLogin = $user->updateLastLoginTimestamp(); - $this->manager->emit('\OC\User', 'postLogin', [$user, $password]); - if ($this->isLoggedIn()) { - $this->prepareUserLogin($firstTimeLogin); - return true; - } else { - // injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory - $message = \OC::$server->getL10N('lib')->t('Login canceled by app'); - throw new LoginException($message); - } - } else { - // injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory - $message = \OC::$server->getL10N('lib')->t('User disabled'); - throw new LoginException($message); - } + return $this->completeLogin($user, ['loginName' => $uid, 'password' => $password]); } /** @@ -547,29 +565,8 @@ class Session implements IUserSession, Emitter { // user does not exist return false; } - if (!$user->isEnabled()) { - // disabled users can not log in - // injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory - $message = \OC::$server->getL10N('lib')->t('User disabled'); - throw new LoginException($message); - } - //login - $this->setUser($user); - $this->setLoginName($dbToken->getLoginName()); - $this->setToken($dbToken->getId()); - $this->lockdownManager->setToken($dbToken); - $this->manager->emit('\OC\User', 'postLogin', array($user, $password)); - - if ($this->isLoggedIn()) { - $this->prepareUserLogin(false); // token login cant be the first - } else { - // injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory - $message = \OC::$server->getL10N('lib')->t('Login canceled by app'); - throw new LoginException($message); - } - - return true; + return $this->completeLogin($user, ['loginName' => $uid, 'password' => $password, 'token' => $dbToken]); } /** diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 51592c2c43..aa6ebe4938 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -362,8 +362,8 @@ class LoginControllerTest extends TestCase { ->method('checkPassword') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) - ->method('login') - ->with($loginName, $password); + ->method('completeLogin') + ->with($user, ['loginName' => $loginName, 'password' => $password]); $this->userSession->expects($this->once()) ->method('createSessionToken') ->with($this->request, $user->getUID(), $loginName, $password, false); @@ -422,8 +422,8 @@ class LoginControllerTest extends TestCase { ->method('checkPassword') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) - ->method('login') - ->with($loginName, $password); + ->method('completeLogin') + ->with($user, ['loginName' => $loginName, 'password' => $password]); $this->userSession->expects($this->once()) ->method('createSessionToken') ->with($this->request, $user->getUID(), $loginName, $password, true); @@ -606,8 +606,8 @@ class LoginControllerTest extends TestCase { ->method('checkPassword') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) - ->method('login') - ->with('john@doe.com', $password); + ->method('completeLogin') + ->with($user, ['loginName' => 'john@doe.com', 'password' => $password]); $this->userSession->expects($this->once()) ->method('createSessionToken') ->with($this->request, $user->getUID(), 'john@doe.com', $password, false); @@ -673,8 +673,8 @@ class LoginControllerTest extends TestCase { ->method('checkPassword') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) - ->method('login') - ->with('john@doe.com', $password); + ->method('completeLogin') + ->with($user, ['loginName' => 'john@doe.com', 'password' => $password]); $this->userSession->expects($this->once()) ->method('createSessionToken') ->with($this->request, $user->getUID(), 'john@doe.com', $password, false); From 50844e8c47f64196a22b1428e0cae509f9f47856 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 1 Dec 2016 14:06:22 +0100 Subject: [PATCH 3/8] regenerate session id on successful login, fixes integration test Signed-off-by: Arthur Schiwon --- lib/private/User/Session.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 05b24c8ccf..dca5ff394a 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -339,6 +339,8 @@ class Session implements IUserSession, Emitter { throw new LoginException($message); } + $this->session->regenerateId(); + $this->setUser($user); $this->setLoginName($loginDetails['loginName']); @@ -560,6 +562,8 @@ class Session implements IUserSession, Emitter { // Ignore and use empty string instead } + $this->manager->emit('\OC\User', 'preLogin', array($uid, $password)); + $user = $this->manager->get($uid); if (is_null($user)) { // user does not exist From daf9d235478ceccd484fbb66e445e19a3b2cd09b Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 2 Dec 2016 11:16:33 +0100 Subject: [PATCH 4/8] don't regenerate Session ID twice, also fixes tests Signed-off-by: Arthur Schiwon --- lib/private/User/Session.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index dca5ff394a..9bbf62b527 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -328,10 +328,11 @@ class Session implements IUserSession, Emitter { /** * @param IUser $user * @param array $loginDetails + * @param bool $regenerateSessionId * @return bool * @throws LoginException */ - public function completeLogin(IUser $user, array $loginDetails) { + public function completeLogin(IUser $user, array $loginDetails, $regenerateSessionId = true) { if (!$user->isEnabled()) { // disabled users can not log in // injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory @@ -339,7 +340,9 @@ class Session implements IUserSession, Emitter { throw new LoginException($message); } - $this->session->regenerateId(); + if($regenerateSessionId) { + $this->session->regenerateId(); + } $this->setUser($user); $this->setLoginName($loginDetails['loginName']); @@ -536,7 +539,7 @@ class Session implements IUserSession, Emitter { return false; } - return $this->completeLogin($user, ['loginName' => $uid, 'password' => $password]); + return $this->completeLogin($user, ['loginName' => $uid, 'password' => $password], false); } /** @@ -570,7 +573,7 @@ class Session implements IUserSession, Emitter { return false; } - return $this->completeLogin($user, ['loginName' => $uid, 'password' => $password, 'token' => $dbToken]); + return $this->completeLogin($user, ['loginName' => $uid, 'password' => $password, 'token' => $dbToken], false); } /** From 4eddb95fc446fc9325ca934293d19050d9528664 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Tue, 4 Apr 2017 16:19:39 -0500 Subject: [PATCH 5/8] Add method to $methodsWithSensitiveParameters Signed-off-by: Morris Jobke --- lib/private/Log.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/Log.php b/lib/private/Log.php index bcaa788603..a87aff0b95 100644 --- a/lib/private/Log.php +++ b/lib/private/Log.php @@ -63,6 +63,7 @@ class Log implements ILogger { protected $methodsWithSensitiveParameters = [ // Session/User + 'completeLogin', 'login', 'checkPassword', 'loginWithPassword', From 0a463e55ae72fc0b5d4fd5394e1f39b6bbafe03e Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 6 Apr 2017 15:04:00 +0200 Subject: [PATCH 6/8] Save correct login name Signed-off-by: Arthur Schiwon --- lib/private/User/Session.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 9bbf62b527..b1e6b1650c 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -573,7 +573,14 @@ class Session implements IUserSession, Emitter { return false; } - return $this->completeLogin($user, ['loginName' => $uid, 'password' => $password, 'token' => $dbToken], false); + return $this->completeLogin( + $user, + [ + 'loginName' => $dbToken->getLoginName(), + 'password' => $password, + 'token' => $dbToken + ], + false); } /** From fbadb37b9bf2b7b8e270f25b82bea969d9b59609 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 6 Apr 2017 15:27:30 +0200 Subject: [PATCH 7/8] use known LockdownManager Signed-off-by: Arthur Schiwon --- lib/private/User/Session.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index b1e6b1650c..5d36cfe21a 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -349,7 +349,7 @@ class Session implements IUserSession, Emitter { if(isset($loginDetails['token']) && $loginDetails['token'] instanceof IToken) { $this->setToken($loginDetails['token']->getId()); - \OC::$server->getLockdownManager()->setToken($loginDetails['token']); + $this->lockdownManager->setToken($loginDetails['token']); $firstTimeLogin = false; } else { $this->setToken(null); From ac05d6dd6777f564ec18dbc2ca530daa4bfaa99d Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Thu, 13 Apr 2017 12:16:12 -0500 Subject: [PATCH 8/8] Improve PHPDoc Signed-off-by: Morris Jobke --- lib/private/User/Session.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 5d36cfe21a..efa11348ef 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -329,7 +329,7 @@ class Session implements IUserSession, Emitter { * @param IUser $user * @param array $loginDetails * @param bool $regenerateSessionId - * @return bool + * @return true returns true if login successful or an exception otherwise * @throws LoginException */ public function completeLogin(IUser $user, array $loginDetails, $regenerateSessionId = true) {