From c4149c59c2cfe83b5e4cd2b20b8ad4caf2341ca9 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Fri, 17 Jun 2016 12:08:48 +0200 Subject: [PATCH 1/8] use token last_activity instead of session value --- .../Token/DefaultTokenProvider.php | 11 +++++++---- lib/private/Authentication/Token/IProvider.php | 2 +- lib/private/User/Session.php | 18 ++---------------- .../Token/DefaultTokenProviderTest.php | 13 ++++++++++++- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/lib/private/Authentication/Token/DefaultTokenProvider.php b/lib/private/Authentication/Token/DefaultTokenProvider.php index 84effc5f875..03b8bb5da28 100644 --- a/lib/private/Authentication/Token/DefaultTokenProvider.php +++ b/lib/private/Authentication/Token/DefaultTokenProvider.php @@ -97,14 +97,17 @@ class DefaultTokenProvider implements IProvider { * @throws InvalidTokenException * @param IToken $token */ - public function updateToken(IToken $token) { + public function updateTokenActivity(IToken $token) { if (!($token instanceof DefaultToken)) { throw new InvalidTokenException(); } /** @var DefaultToken $token */ - $token->setLastActivity($this->time->getTime()); - - $this->mapper->update($token); + $now = $this->time->getTime(); + if ($token->getLastActivity() < ($now - 60)) { + // Update token only once per minute + $token->setLastActivity($now); + $this->mapper->update($token); + } } /** diff --git a/lib/private/Authentication/Token/IProvider.php b/lib/private/Authentication/Token/IProvider.php index fece7dcb567..e79ba8b30e5 100644 --- a/lib/private/Authentication/Token/IProvider.php +++ b/lib/private/Authentication/Token/IProvider.php @@ -76,7 +76,7 @@ interface IProvider { * * @param IToken $token */ - public function updateToken(IToken $token); + public function updateTokenActivity(IToken $token); /** * Get all token of a user diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 0cebb3e0613..89148dcf8ec 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -237,8 +237,7 @@ class Session implements IUserSession, Emitter { $this->session->set('last_login_check', $now); } - // Session is valid, so the token can be refreshed - $this->updateToken($token); + $this->tokenProvider->updateTokenActivity($token); } /** @@ -541,7 +540,7 @@ class Session implements IUserSession, Emitter { $result = $this->loginWithToken($token->getUID()); if ($result) { // Login success - $this->updateToken($token); + $this->tokenProvider->updateTokenActivity($token); return true; } } @@ -551,19 +550,6 @@ class Session implements IUserSession, Emitter { return false; } - /** - * @param IToken $token - */ - private function updateToken(IToken $token) { - // To save unnecessary DB queries, this is only done once a minute - $lastTokenUpdate = $this->session->get('last_token_update') ? : 0; - $now = $this->timeFacory->getTime(); - if ($lastTokenUpdate < ($now - 60)) { - $this->tokenProvider->updateToken($token); - $this->session->set('last_token_update', $now); - } - } - /** * Tries to login the user with auth token header * diff --git a/tests/lib/Authentication/Token/DefaultTokenProviderTest.php b/tests/lib/Authentication/Token/DefaultTokenProviderTest.php index 98cee208065..86f4842bbc3 100644 --- a/tests/lib/Authentication/Token/DefaultTokenProviderTest.php +++ b/tests/lib/Authentication/Token/DefaultTokenProviderTest.php @@ -97,14 +97,25 @@ class DefaultTokenProviderTest extends TestCase { public function testUpdateToken() { $tk = new DefaultToken(); + $tk->setLastActivity($this->time - 200); $this->mapper->expects($this->once()) ->method('update') ->with($tk); - $this->tokenProvider->updateToken($tk); + $this->tokenProvider->updateTokenActivity($tk); $this->assertEquals($this->time, $tk->getLastActivity()); } + + public function testUpdateTokenDebounce() { + $tk = new DefaultToken(); + $tk->setLastActivity($this->time - 30); + $this->mapper->expects($this->never()) + ->method('update') + ->with($tk); + + $this->tokenProvider->updateTokenActivity($tk); + } public function testGetTokenByUser() { $user = $this->getMock('\OCP\IUser'); From 0c0a216f42bb004380efca1fd665711f938579d9 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Fri, 17 Jun 2016 13:59:15 +0200 Subject: [PATCH 2/8] store last check timestamp in token instead of session --- db_structure.xml | 9 ++ .../Authentication/Token/DefaultToken.php | 23 +++ .../Token/DefaultTokenMapper.php | 4 +- .../Token/DefaultTokenProvider.php | 27 ++-- .../Authentication/Token/IProvider.php | 14 +- lib/private/Authentication/Token/IToken.php | 14 ++ lib/private/User/Session.php | 144 +++++++++++------- version.php | 2 +- 8 files changed, 160 insertions(+), 77 deletions(-) diff --git a/db_structure.xml b/db_structure.xml index b7dacc05d92..6b91c3c4c5d 100644 --- a/db_structure.xml +++ b/db_structure.xml @@ -1120,6 +1120,15 @@ 4 + + last_check + integer + 0 + true + true + 4 + + authtoken_token_index true diff --git a/lib/private/Authentication/Token/DefaultToken.php b/lib/private/Authentication/Token/DefaultToken.php index 299291e34af..79b03eed27f 100644 --- a/lib/private/Authentication/Token/DefaultToken.php +++ b/lib/private/Authentication/Token/DefaultToken.php @@ -74,6 +74,11 @@ class DefaultToken extends Entity implements IToken { */ protected $lastActivity; + /** + * @var int + */ + protected $lastCheck; + public function getId() { return $this->id; } @@ -109,4 +114,22 @@ class DefaultToken extends Entity implements IToken { ]; } + /** + * Get the timestamp of the last password check + * + * @return int + */ + public function getLastCheck() { + return parent::getLastCheck(); + } + + /** + * Get the timestamp of the last password check + * + * @param int $time + */ + public function setLastCheck($time) { + return parent::setLastCheck($time); + } + } diff --git a/lib/private/Authentication/Token/DefaultTokenMapper.php b/lib/private/Authentication/Token/DefaultTokenMapper.php index c56a513b94c..b02220976cf 100644 --- a/lib/private/Authentication/Token/DefaultTokenMapper.php +++ b/lib/private/Authentication/Token/DefaultTokenMapper.php @@ -70,7 +70,7 @@ class DefaultTokenMapper extends Mapper { public function getToken($token) { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); - $result = $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'token', 'last_activity') + $result = $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'token', 'last_activity', 'last_check') ->from('authtoken') ->where($qb->expr()->eq('token', $qb->createParameter('token'))) ->setParameter('token', $token) @@ -95,7 +95,7 @@ class DefaultTokenMapper extends Mapper { public function getTokenByUser(IUser $user) { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); - $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'token', 'last_activity') + $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'token', 'last_activity', 'last_check') ->from('authtoken') ->where($qb->expr()->eq('uid', $qb->createNamedParameter($user->getUID()))) ->setMaxResults(1000); diff --git a/lib/private/Authentication/Token/DefaultTokenProvider.php b/lib/private/Authentication/Token/DefaultTokenProvider.php index 03b8bb5da28..2467b8d836f 100644 --- a/lib/private/Authentication/Token/DefaultTokenProvider.php +++ b/lib/private/Authentication/Token/DefaultTokenProvider.php @@ -91,6 +91,18 @@ class DefaultTokenProvider implements IProvider { return $dbToken; } + /** + * Save the updated token + * + * @param IToken $token + */ + public function updateToken(IToken $token) { + if (!($token instanceof DefaultToken)) { + throw new InvalidTokenException(); + } + $this->mapper->update($token); + } + /** * Update token activity timestamp * @@ -181,21 +193,6 @@ class DefaultTokenProvider implements IProvider { $this->mapper->invalidateOld($olderThan); } - /** - * @param string $token - * @throws InvalidTokenException - * @return DefaultToken user UID - */ - public function validateToken($token) { - try { - $dbToken = $this->mapper->getToken($this->hashToken($token)); - $this->logger->debug('valid default token for ' . $dbToken->getUID()); - return $dbToken; - } catch (DoesNotExistException $ex) { - throw new InvalidTokenException(); - } - } - /** * @param string $token * @return string diff --git a/lib/private/Authentication/Token/IProvider.php b/lib/private/Authentication/Token/IProvider.php index e79ba8b30e5..97f8ababbbe 100644 --- a/lib/private/Authentication/Token/IProvider.php +++ b/lib/private/Authentication/Token/IProvider.php @@ -49,13 +49,6 @@ interface IProvider { */ public function getToken($tokenId) ; - /** - * @param string $token - * @throws InvalidTokenException - * @return IToken - */ - public function validateToken($token); - /** * Invalidate (delete) the given session token * @@ -71,6 +64,13 @@ interface IProvider { */ public function invalidateTokenById(IUser $user, $id); + /** + * Save the updated token + * + * @param IToken $token + */ + public function updateToken(IToken $token); + /** * Update token activity timestamp * diff --git a/lib/private/Authentication/Token/IToken.php b/lib/private/Authentication/Token/IToken.php index a34bdc2c43d..096550fd091 100644 --- a/lib/private/Authentication/Token/IToken.php +++ b/lib/private/Authentication/Token/IToken.php @@ -55,4 +55,18 @@ interface IToken extends JsonSerializable { * @return string */ public function getPassword(); + + /** + * Get the timestamp of the last password check + * + * @return int + */ + public function getLastCheck(); + + /** + * Get the timestamp of the last password check + * + * @param int $time + */ + public function setLastCheck($time); } diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 89148dcf8ec..ccae72ed35a 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -192,52 +192,22 @@ class Session implements IUserSession, Emitter { if (is_null($this->activeUser)) { return null; } - $this->validateSession($this->activeUser); + $this->validateSession(); } return $this->activeUser; } - protected function validateSession(IUser $user) { + protected function validateSession() { try { $sessionId = $this->session->getId(); } catch (SessionNotAvailableException $ex) { return; } - try { - $token = $this->tokenProvider->getToken($sessionId); - } catch (InvalidTokenException $ex) { + + if (!$this->validateToken($sessionId)) { // Session was invalidated $this->logout(); - return; } - - // Check whether login credentials are still valid and the user was not disabled - // This check is performed each 5 minutes - $lastCheck = $this->session->get('last_login_check') ? : 0; - $now = $this->timeFacory->getTime(); - if ($lastCheck < ($now - 60 * 5)) { - try { - $pwd = $this->tokenProvider->getPassword($token, $sessionId); - } catch (InvalidTokenException $ex) { - // An invalid token password was used -> log user out - $this->logout(); - return; - } catch (PasswordlessTokenException $ex) { - // Token has no password, nothing to check - $this->session->set('last_login_check', $now); - return; - } - - if ($this->manager->checkPassword($token->getLoginName(), $pwd) === false - || !$user->isEnabled()) { - // Password has changed or user was disabled -> log user out - $this->logout(); - return; - } - $this->session->set('last_login_check', $now); - } - - $this->tokenProvider->updateTokenActivity($token); } /** @@ -297,20 +267,22 @@ class Session implements IUserSession, Emitter { public function login($uid, $password) { $this->session->regenerateId(); if ($this->validateToken($password)) { - $user = $this->getUser(); - // When logging in with token, the password must be decrypted first before passing to login hook try { $token = $this->tokenProvider->getToken($password); try { - $password = $this->tokenProvider->getPassword($token, $password); - $this->manager->emit('\OC\User', 'preLogin', array($uid, $password)); + $loginPassword = $this->tokenProvider->getPassword($token, $password); + $this->manager->emit('\OC\User', 'preLogin', array($uid, $loginPassword)); } catch (PasswordlessTokenException $ex) { $this->manager->emit('\OC\User', 'preLogin', array($uid, '')); } } catch (InvalidTokenException $ex) { // Invalid token, nothing to do } + + $this->loginWithToken($password); + $user = $this->getUser(); + $this->tokenProvider->updateTokenActivity($token); } else { $this->manager->emit('\OC\User', 'preLogin', array($uid, $password)); $user = $this->manager->checkPassword($uid, $password); @@ -459,8 +431,21 @@ class Session implements IUserSession, Emitter { return false; } - private function loginWithToken($uid) { - // TODO: $this->manager->emit('\OC\User', 'preTokenLogin', array($uid)); + private function loginWithToken($token) { + try { + $dbToken = $this->tokenProvider->getToken($token); + } catch (InvalidTokenException $ex) { + return false; + } + $uid = $dbToken->getUID(); + + try { + $password = $this->tokenProvider->getPassword($dbToken, $token); + $this->manager->emit('\OC\User', 'preLogin', array($uid, $password)); + } catch (PasswordlessTokenException $ex) { + $this->manager->emit('\OC\User', 'preLogin', array($uid, '')); + } + $user = $this->manager->get($uid); if (is_null($user)) { // user does not exist @@ -473,7 +458,9 @@ class Session implements IUserSession, Emitter { //login $this->setUser($user); - // TODO: $this->manager->emit('\OC\User', 'postTokenLogin', array($user)); + $this->tokenProvider->updateTokenActivity($dbToken); + + $this->manager->emit('\OC\User', 'postLogin', array($user, $password)); return true; } @@ -530,24 +517,72 @@ class Session implements IUserSession, Emitter { } /** + * @param IToken $dbToken * @param string $token * @return boolean */ - private function validateToken($token) { + private function checkTokenCredentials(IToken $dbToken, $token) { + // Check whether login credentials are still valid and the user was not disabled + // This check is performed each 5 minutes + $lastCheck = $dbToken->getLastCheck() ? : 0; + $now = $this->timeFacory->getTime(); + if ($lastCheck > ($now - 60 * 5)) { + // Checked performed recently, nothing to do now + return true; + } + try { - $token = $this->tokenProvider->validateToken($token); - if (!is_null($token)) { - $result = $this->loginWithToken($token->getUID()); - if ($result) { - // Login success - $this->tokenProvider->updateTokenActivity($token); - return true; - } + $pwd = $this->tokenProvider->getPassword($dbToken, $token); + } catch (InvalidTokenException $ex) { + // An invalid token password was used -> log user out + $this->logout(); + return false; + } catch (PasswordlessTokenException $ex) { + // Token has no password + + if (!is_null($this->activeUser) && !$this->activeUser->isEnabled()) { + $this->tokenProvider->invalidateToken($token); + $this->logout(); + return false; } + + $dbToken->setLastCheck($now); + $this->tokenProvider->updateToken($dbToken); + return true; + } + + if ($this->manager->checkPassword($dbToken->getLoginName(), $pwd) === false + || (!is_null($this->activeUser) && !$this->activeUser->isEnabled())) { + $this->tokenProvider->invalidateToken($token); + // Password has changed or user was disabled -> log user out + $this->logout(); + return false; + } + $dbToken->setLastCheck($now); + $this->tokenProvider->updateToken($dbToken); + return true; + } + + /** + * Check if the given token exists and performs password/user-enabled checks + * + * Invalidates the token if checks fail + * + * @param string $token + * @return boolean + */ + private function validateToken($token) { + try { + $dbToken = $this->tokenProvider->getToken($token); } catch (InvalidTokenException $ex) { + return false; + } + if (!$this->checkTokenCredentials($dbToken, $token)) { + return false; } - return false; + + return true; } /** @@ -562,10 +597,15 @@ class Session implements IUserSession, Emitter { // No auth header, let's try session id try { $sessionId = $this->session->getId(); - return $this->validateToken($sessionId); } catch (SessionNotAvailableException $ex) { return false; } + + if (!$this->validateToken($sessionId)) { + return false; + } + + return $this->loginWithToken($sessionId); } else { $token = substr($authHeader, 6); return $this->validateToken($token); diff --git a/version.php b/version.php index 698636a2196..3015d976e53 100644 --- a/version.php +++ b/version.php @@ -25,7 +25,7 @@ // We only can count up. The 4. digit is only for the internal patchlevel to trigger DB upgrades // between betas, final and RCs. This is _not_ the public version number. Reset minor/patchlevel // when updating major/minor version number. -$OC_Version = array(9, 1, 0, 8); +$OC_Version = array(9, 1, 0, 9); // The human readable string $OC_VersionString = '9.1.0 beta 2'; From 1889df5c7cac71e9faf42d19686b98bf61b23bf8 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Fri, 17 Jun 2016 15:41:32 +0200 Subject: [PATCH 3/8] dont create a session token for clients, validate the app password instead --- lib/private/User/Session.php | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index ccae72ed35a..cd9e973e306 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -197,14 +197,27 @@ class Session implements IUserSession, Emitter { return $this->activeUser; } + /** + * Validate whether the current session is valid + * + * - For token-authenticated clients, the token validity is checked + * - For browsers, the session token validity is checked + */ protected function validateSession() { - try { - $sessionId = $this->session->getId(); - } catch (SessionNotAvailableException $ex) { - return; + $token = null; + $appPassword = $this->session->get('app_password'); + + if (is_null($appPassword)) { + try { + $token = $this->session->getId(); + } catch (SessionNotAvailableException $ex) { + return; + } + } else { + $token = $appPassword; } - if (!$this->validateToken($sessionId)) { + if (!$this->validateToken($token)) { // Session was invalidated $this->logout(); } @@ -282,7 +295,6 @@ class Session implements IUserSession, Emitter { $this->loginWithToken($password); $user = $this->getUser(); - $this->tokenProvider->updateTokenActivity($token); } else { $this->manager->emit('\OC\User', 'preLogin', array($uid, $password)); $user = $this->manager->checkPassword($uid, $password); @@ -341,7 +353,10 @@ class Session implements IUserSession, Emitter { return false; } - if ($this->supportsCookies($request)) { + if ($isTokenPassword) { + $this->session->set('app_password', $password); + } else if($this->supportsCookies($request)) { + // Password login, but cookies supported -> create (browser) session token $this->createSessionToken($request, $this->getUser()->getUID(), $user, $password); } @@ -458,7 +473,6 @@ class Session implements IUserSession, Emitter { //login $this->setUser($user); - $this->tokenProvider->updateTokenActivity($dbToken); $this->manager->emit('\OC\User', 'postLogin', array($user, $password)); return true; @@ -582,6 +596,8 @@ class Session implements IUserSession, Emitter { return false; } + $this->tokenProvider->updateTokenActivity($dbToken); + return true; } From 8ef5431e7a0db3120223fafd336f273cb3a61513 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 20 Jun 2016 09:10:11 +0200 Subject: [PATCH 4/8] fix user session tests --- tests/lib/User/SessionTest.php | 146 ++++++++++++++++----------------- 1 file changed, 73 insertions(+), 73 deletions(-) diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 28f6b6a5377..974b5d3fd88 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -41,6 +41,7 @@ class SessionTest extends \Test\TestCase { public function testGetUser() { $token = new \OC\Authentication\Token\DefaultToken(); $token->setLoginName('User123'); + $token->setLastCheck(200); $expectedUser = $this->getMock('\OCP\IUser'); $expectedUser->expects($this->any()) @@ -56,41 +57,32 @@ class SessionTest extends \Test\TestCase { $manager = $this->getMockBuilder('\OC\User\Manager') ->disableOriginalConstructor() ->getMock(); + $session->expects($this->at(1)) + ->method('get') + ->with('app_password') + ->will($this->returnValue(null)); // No password set -> browser session $session->expects($this->once()) ->method('getId') ->will($this->returnValue($sessionId)); $this->tokenProvider->expects($this->once()) ->method('getToken') + ->with($sessionId) ->will($this->returnValue($token)); - $session->expects($this->at(2)) - ->method('get') - ->with('last_login_check') - ->will($this->returnValue(null)); // No check has been run yet $this->tokenProvider->expects($this->once()) ->method('getPassword') ->with($token, $sessionId) - ->will($this->returnValue('password123')); + ->will($this->returnValue('passme')); $manager->expects($this->once()) ->method('checkPassword') - ->with('User123', 'password123') + ->with('User123', 'passme') ->will($this->returnValue(true)); $expectedUser->expects($this->once()) ->method('isEnabled') ->will($this->returnValue(true)); - $session->expects($this->at(3)) - ->method('set') - ->with('last_login_check', 10000); - $session->expects($this->at(4)) - ->method('get') - ->with('last_token_update') - ->will($this->returnValue(null)); // No check run so far $this->tokenProvider->expects($this->once()) - ->method('updateToken') + ->method('updateTokenActivity') ->with($token); - $session->expects($this->at(5)) - ->method('set') - ->with('last_token_update', $this->equalTo(10000)); $manager->expects($this->any()) ->method('get') @@ -100,6 +92,7 @@ class SessionTest extends \Test\TestCase { $userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config); $user = $userSession->getUser(); $this->assertSame($expectedUser, $user); + $this->assertSame(10000, $token->getLastCheck()); } public function isLoggedInData() { @@ -155,6 +148,10 @@ class SessionTest extends \Test\TestCase { $session = $this->getMock('\OC\Session\Memory', array(), array('')); $session->expects($this->once()) ->method('regenerateId'); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with('bar') + ->will($this->throwException('\OC\Authentication\Exceptions\InvalidTokenException')); $session->expects($this->exactly(2)) ->method('set') ->with($this->callback(function ($key) { @@ -219,6 +216,10 @@ class SessionTest extends \Test\TestCase { ->method('set'); $session->expects($this->once()) ->method('regenerateId'); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with('bar') + ->will($this->throwException(new \OC\Authentication\Exceptions\InvalidTokenException())); $managerMethods = get_class_methods('\OC\User\Manager'); //keep following methods intact in order to ensure hooks are @@ -252,11 +253,6 @@ class SessionTest extends \Test\TestCase { public function testLoginInvalidPassword() { $session = $this->getMock('\OC\Session\Memory', array(), array('')); - $session->expects($this->never()) - ->method('set'); - $session->expects($this->once()) - ->method('regenerateId'); - $managerMethods = get_class_methods('\OC\User\Manager'); //keep following methods intact in order to ensure hooks are //working @@ -268,10 +264,20 @@ class SessionTest extends \Test\TestCase { } } $manager = $this->getMock('\OC\User\Manager', $managerMethods, array()); - $backend = $this->getMock('\Test\Util\User\Dummy'); + $userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config); $user = $this->getMock('\OC\User\User', array(), array('foo', $backend)); + + $session->expects($this->never()) + ->method('set'); + $session->expects($this->once()) + ->method('regenerateId'); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with('bar') + ->will($this->throwException(new \OC\Authentication\Exceptions\InvalidTokenException())); + $user->expects($this->never()) ->method('isEnabled'); $user->expects($this->never()) @@ -282,27 +288,29 @@ class SessionTest extends \Test\TestCase { ->with('foo', 'bar') ->will($this->returnValue(false)); - $userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config); $userSession->login('foo', 'bar'); } public function testLoginNonExisting() { $session = $this->getMock('\OC\Session\Memory', array(), array('')); + $manager = $this->getMock('\OC\User\Manager'); + $backend = $this->getMock('\Test\Util\User\Dummy'); + $userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config); + $session->expects($this->never()) ->method('set'); $session->expects($this->once()) ->method('regenerateId'); - - $manager = $this->getMock('\OC\User\Manager'); - - $backend = $this->getMock('\Test\Util\User\Dummy'); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with('bar') + ->will($this->throwException(new \OC\Authentication\Exceptions\InvalidTokenException())); $manager->expects($this->once()) ->method('checkPassword') ->with('foo', 'bar') ->will($this->returnValue(false)); - $userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config); $userSession->login('foo', 'bar'); } @@ -351,24 +359,14 @@ class SessionTest extends \Test\TestCase { ->will($this->returnValue(true)); $userSession->expects($this->once()) ->method('login') - ->with('john', 'doe') + ->with('john', 'I-AM-AN-APP-PASSWORD') ->will($this->returnValue(true)); - $userSession->expects($this->once()) - ->method('supportsCookies') - ->with($request) - ->will($this->returnValue(true)); - $userSession->expects($this->once()) - ->method('getUser') - ->will($this->returnValue($user)); - $user->expects($this->once()) - ->method('getUID') - ->will($this->returnValue('user123')); - $userSession->expects($this->once()) - ->method('createSessionToken') - ->with($request, 'user123', 'john', 'doe'); - - $this->assertTrue($userSession->logClientIn('john', 'doe', $request)); + $session->expects($this->once()) + ->method('set') + ->with('app_password', 'I-AM-AN-APP-PASSWORD'); + + $this->assertTrue($userSession->logClientIn('john', 'I-AM-AN-APP-PASSWORD', $request)); } public function testLogClientInNoTokenPasswordNo2fa() { @@ -738,38 +736,40 @@ class SessionTest extends \Test\TestCase { ->getMock(); $user = $this->getMock('\OCP\IUser'); - $token = $this->getMock('\OC\Authentication\Token\IToken'); + $token = new \OC\Authentication\Token\DefaultToken(); + $token->setLoginName('susan'); + $token->setLastCheck(20); $session->expects($this->once()) - ->method('getId') - ->will($this->returnValue('sessionid')); + ->method('get') + ->with('app_password') + ->will($this->returnValue('APP-PASSWORD')); $tokenProvider->expects($this->once()) ->method('getToken') - ->with('sessionid') + ->with('APP-PASSWORD') ->will($this->returnValue($token)); - $session->expects($this->once()) - ->method('get') - ->with('last_login_check') - ->will($this->returnValue(1000)); $timeFactory->expects($this->once()) ->method('getTime') - ->will($this->returnValue(5000)); + ->will($this->returnValue(1000)); // more than 5min since last check $tokenProvider->expects($this->once()) ->method('getPassword') - ->with($token, 'sessionid') + ->with($token, 'APP-PASSWORD') ->will($this->returnValue('123456')); - $token->expects($this->once()) - ->method('getLoginName') - ->will($this->returnValue('User5')); $userManager->expects($this->once()) ->method('checkPassword') - ->with('User5', '123456') + ->with('susan', '123456') ->will($this->returnValue(true)); $user->expects($this->once()) ->method('isEnabled') ->will($this->returnValue(false)); - $userSession->expects($this->once()) + $this->tokenProvider->expects($this->once()) + ->method('invalidateToken') + ->with($token); + $session->expects($this->once()) ->method('logout'); + $tokenProvider->expects($this->once()) + ->method('updateToken') + ->with($token); $this->invokePrivate($userSession, 'validateSession', [$user]); } @@ -785,31 +785,31 @@ class SessionTest extends \Test\TestCase { ->getMock(); $user = $this->getMock('\OCP\IUser'); - $token = $this->getMock('\OC\Authentication\Token\IToken'); + $token = new \OC\Authentication\Token\DefaultToken(); + $token->setLastCheck(20); $session->expects($this->once()) - ->method('getId') - ->will($this->returnValue('sessionid')); + ->method('get') + ->with('app_password') + ->will($this->returnValue('APP-PASSWORD')); $tokenProvider->expects($this->once()) ->method('getToken') - ->with('sessionid') + ->with('APP-PASSWORD') ->will($this->returnValue($token)); - $session->expects($this->once()) - ->method('get') - ->with('last_login_check') - ->will($this->returnValue(1000)); $timeFactory->expects($this->once()) ->method('getTime') - ->will($this->returnValue(5000)); + ->will($this->returnValue(1000)); // more than 5min since last check $tokenProvider->expects($this->once()) ->method('getPassword') - ->with($token, 'sessionid') + ->with($token, 'APP-PASSWORD') ->will($this->throwException(new \OC\Authentication\Exceptions\PasswordlessTokenException())); - $session->expects($this->once()) - ->method('set') - ->with('last_login_check', 5000); + $tokenProvider->expects($this->once()) + ->method('updateToken') + ->with($token); $this->invokePrivate($userSession, 'validateSession', [$user]); + + $this->assertEquals(1000, $token->getLastCheck()); } } From 9d74ff02a4014d11020e21e5f73beb02bc749697 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 20 Jun 2016 09:13:47 +0200 Subject: [PATCH 5/8] fix nitpick --- lib/private/User/Session.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index cd9e973e306..07235c1b42b 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -454,12 +454,13 @@ class Session implements IUserSession, Emitter { } $uid = $dbToken->getUID(); + $password = ''; try { $password = $this->tokenProvider->getPassword($dbToken, $token); - $this->manager->emit('\OC\User', 'preLogin', array($uid, $password)); } catch (PasswordlessTokenException $ex) { - $this->manager->emit('\OC\User', 'preLogin', array($uid, '')); + // Ignore and use empty string instead } + $this->manager->emit('\OC\User', 'preLogin', array($uid, $password)); $user = $this->manager->get($uid); if (is_null($user)) { From 5c680848232439588a536cef02a5cc85aa994819 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 20 Jun 2016 09:17:19 +0200 Subject: [PATCH 6/8] fix default token provider tests --- .../Token/DefaultTokenProviderTest.php | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/tests/lib/Authentication/Token/DefaultTokenProviderTest.php b/tests/lib/Authentication/Token/DefaultTokenProviderTest.php index 86f4842bbc3..d0bb38e6bb1 100644 --- a/tests/lib/Authentication/Token/DefaultTokenProviderTest.php +++ b/tests/lib/Authentication/Token/DefaultTokenProviderTest.php @@ -218,30 +218,4 @@ class DefaultTokenProviderTest extends TestCase { $this->tokenProvider->invalidateOldTokens(); } - public function testValidateToken() { - $token = 'sometoken'; - $dbToken = new DefaultToken(); - $this->mapper->expects($this->once()) - ->method('getToken') - ->with(hash('sha512', $token)) - ->will($this->returnValue($dbToken)); - - $actual = $this->tokenProvider->validateToken($token); - - $this->assertEquals($dbToken, $actual); - } - - /** - * @expectedException \OC\Authentication\Exceptions\InvalidTokenException - */ - public function testValidateInvalidToken() { - $token = 'sometoken'; - $this->mapper->expects($this->once()) - ->method('getToken') - ->with(hash('sha512', $token)) - ->will($this->throwException(new DoesNotExistException(''))); - - $this->tokenProvider->validateToken($token); - } - } From fb36fd495b2b97063a13239f214909011d0b1385 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 20 Jun 2016 09:25:15 +0200 Subject: [PATCH 7/8] fix DefaultTokenMapperTest --- tests/lib/Authentication/Token/DefaultTokenMapperTest.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/lib/Authentication/Token/DefaultTokenMapperTest.php b/tests/lib/Authentication/Token/DefaultTokenMapperTest.php index 5d49f75aaa4..6b73cab5ed0 100644 --- a/tests/lib/Authentication/Token/DefaultTokenMapperTest.php +++ b/tests/lib/Authentication/Token/DefaultTokenMapperTest.php @@ -63,6 +63,7 @@ class DefaultTokenMapperTest extends TestCase { 'token' => $qb->createNamedParameter('9c5a2e661482b65597408a6bb6c4a3d1af36337381872ac56e445a06cdb7fea2b1039db707545c11027a4966919918b19d875a8b774840b18c6cbb7ae56fe206'), 'type' => $qb->createNamedParameter(IToken::TEMPORARY_TOKEN), 'last_activity' => $qb->createNamedParameter($this->time - 120, IQueryBuilder::PARAM_INT), // Two minutes ago + 'last_check' => $this->time - 60 * 10, // 10mins ago ])->execute(); $qb->insert('authtoken')->values([ 'uid' => $qb->createNamedParameter('user2'), @@ -72,6 +73,7 @@ class DefaultTokenMapperTest extends TestCase { 'token' => $qb->createNamedParameter('1504445f1524fc801035448a95681a9378ba2e83930c814546c56e5d6ebde221198792fd900c88ed5ead0555780dad1ebce3370d7e154941cd5de87eb419899b'), 'type' => $qb->createNamedParameter(IToken::TEMPORARY_TOKEN), 'last_activity' => $qb->createNamedParameter($this->time - 60 * 60 * 24 * 3, IQueryBuilder::PARAM_INT), // Three days ago + 'last_check' => $this->time - 10, // 10secs ago ])->execute(); $qb->insert('authtoken')->values([ 'uid' => $qb->createNamedParameter('user1'), @@ -81,6 +83,7 @@ class DefaultTokenMapperTest extends TestCase { 'token' => $qb->createNamedParameter('47af8697ba590fb82579b5f1b3b6e8066773a62100abbe0db09a289a62f5d980dc300fa3d98b01d7228468d1ab05c1aa14c8d14bd5b6eee9cdf1ac14864680c3'), 'type' => $qb->createNamedParameter(IToken::TEMPORARY_TOKEN), 'last_activity' => $qb->createNamedParameter($this->time - 120, IQueryBuilder::PARAM_INT), // Two minutes ago + 'last_check' => $this->time - 60 * 10, // 10mins ago ])->execute(); } @@ -127,6 +130,7 @@ class DefaultTokenMapperTest extends TestCase { $token->setToken('1504445f1524fc801035448a95681a9378ba2e83930c814546c56e5d6ebde221198792fd900c88ed5ead0555780dad1ebce3370d7e154941cd5de87eb419899b'); $token->setType(IToken::TEMPORARY_TOKEN); $token->setLastActivity($this->time - 60 * 60 * 24 * 3); + $token->setLastCheck($this->time - 10); $dbToken = $this->mapper->getToken($token->getToken()); From 56199eba376e044f65e9e02844a9d98e524340b4 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 20 Jun 2016 10:41:23 +0200 Subject: [PATCH 8/8] fix unit test warning/errors --- lib/private/User/Session.php | 20 +++++++++----------- tests/lib/User/SessionTest.php | 31 ++++++++++++++++--------------- 2 files changed, 25 insertions(+), 26 deletions(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 07235c1b42b..aedb308539a 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -550,14 +550,12 @@ class Session implements IUserSession, Emitter { $pwd = $this->tokenProvider->getPassword($dbToken, $token); } catch (InvalidTokenException $ex) { // An invalid token password was used -> log user out - $this->logout(); return false; } catch (PasswordlessTokenException $ex) { // Token has no password if (!is_null($this->activeUser) && !$this->activeUser->isEnabled()) { $this->tokenProvider->invalidateToken($token); - $this->logout(); return false; } @@ -570,7 +568,6 @@ class Session implements IUserSession, Emitter { || (!is_null($this->activeUser) && !$this->activeUser->isEnabled())) { $this->tokenProvider->invalidateToken($token); // Password has changed or user was disabled -> log user out - $this->logout(); return false; } $dbToken->setLastCheck($now); @@ -613,20 +610,21 @@ class Session implements IUserSession, Emitter { if (strpos($authHeader, 'token ') === false) { // No auth header, let's try session id try { - $sessionId = $this->session->getId(); + $token = $this->session->getId(); } catch (SessionNotAvailableException $ex) { return false; } - - if (!$this->validateToken($sessionId)) { - return false; - } - - return $this->loginWithToken($sessionId); } else { $token = substr($authHeader, 6); - return $this->validateToken($token); } + + if (!$this->loginWithToken($token)) { + return false; + } + if(!$this->validateToken($token)) { + return false; + } + return true; } /** diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 974b5d3fd88..6b72cf81bc9 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -151,7 +151,7 @@ class SessionTest extends \Test\TestCase { $this->tokenProvider->expects($this->once()) ->method('getToken') ->with('bar') - ->will($this->throwException('\OC\Authentication\Exceptions\InvalidTokenException')); + ->will($this->throwException(new \OC\Authentication\Exceptions\InvalidTokenException())); $session->expects($this->exactly(2)) ->method('set') ->with($this->callback(function ($key) { @@ -698,9 +698,15 @@ class SessionTest extends \Test\TestCase { ->disableOriginalConstructor() ->getMock(); $session = new Memory(''); - $token = $this->getMock('\OC\Authentication\Token\IToken'); + $token = new \OC\Authentication\Token\DefaultToken(); + $token->setLoginName('fritz'); + $token->setUid('fritz0'); + $token->setLastCheck(100); // Needs check $user = $this->getMock('\OCP\IUser'); - $userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config); + $userSession = $this->getMockBuilder('\OC\User\Session') + ->setMethods(['logout']) + ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config]) + ->getMock(); $request = $this->getMock('\OCP\IRequest'); $request->expects($this->once()) @@ -708,15 +714,12 @@ class SessionTest extends \Test\TestCase { ->with('Authorization') ->will($this->returnValue('token xxxxx')); $this->tokenProvider->expects($this->once()) - ->method('validateToken') + ->method('getToken') ->with('xxxxx') ->will($this->returnValue($token)); - $token->expects($this->once()) - ->method('getUID') - ->will($this->returnValue('user123')); $manager->expects($this->once()) ->method('get') - ->with('user123') + ->with('fritz0') ->will($this->returnValue($user)); $user->expects($this->once()) ->method('isEnabled') @@ -762,16 +765,14 @@ class SessionTest extends \Test\TestCase { $user->expects($this->once()) ->method('isEnabled') ->will($this->returnValue(false)); - $this->tokenProvider->expects($this->once()) + $tokenProvider->expects($this->once()) ->method('invalidateToken') - ->with($token); - $session->expects($this->once()) + ->with('APP-PASSWORD'); + $userSession->expects($this->once()) ->method('logout'); - $tokenProvider->expects($this->once()) - ->method('updateToken') - ->with($token); - $this->invokePrivate($userSession, 'validateSession', [$user]); + $userSession->setUser($user); + $this->invokePrivate($userSession, 'validateSession'); } public function testValidateSessionNoPassword() {