From 6d1e858aa4cf5c35e0396f23144caea68797f42d Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 7 Oct 2016 16:49:57 +0200 Subject: [PATCH 1/2] Fix logClientIn for non-existing users (#26292) The check for two factor enforcement would return true for non-existing users. This fix makes it return false in order to be able to perform the regular login which will then fail and return false. This prevents throwing PasswordLoginForbidden for non-existing users. --- lib/private/User/Session.php | 3 +++ tests/lib/User/SessionTest.php | 26 ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 4b56609ccfc..a213ee48c2a 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -362,6 +362,9 @@ class Session implements IUserSession, Emitter { $user = $this->manager->get($username); if (is_null($user)) { $users = $this->manager->getByEmail($username); + if (empty($users)) { + return false; + } if (count($users) !== 1) { return true; } diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 21ac1b655b9..614ed3d015a 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -401,6 +401,32 @@ class SessionTest extends \Test\TestCase { $userSession->logClientIn('john', 'doe', $request, $this->throttler); } + public function testLogClientInUnexist() { + $manager = $this->getMockBuilder('\OC\User\Manager') + ->disableOriginalConstructor() + ->getMock(); + $session = $this->createMock('\OCP\ISession'); + $request = $this->createMock('\OCP\IRequest'); + $user = $this->createMock('\OCP\IUser'); + + /** @var \OC\User\Session $userSession */ + $userSession = $this->getMockBuilder('\OC\User\Session') + ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config]) + ->setMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser']) + ->getMock(); + + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with('doe') + ->will($this->throwException(new \OC\Authentication\Exceptions\InvalidTokenException())); + $this->config->expects($this->once()) + ->method('getSystemValue') + ->with('token_auth_enforced', false) + ->will($this->returnValue(false)); + + $this->assertFalse($userSession->logClientIn('unexist', 'doe', $request)); + } + public function testLogClientInWithTokenPassword() { $manager = $this->getMockBuilder('\OC\User\Manager') ->disableOriginalConstructor() From 593d52fe913b4a3c29e857432317c73d24b952e3 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 25 Oct 2016 09:34:11 +0200 Subject: [PATCH 2/2] Fix and cleanup SessionTest Signed-off-by: Roeland Jago Douma --- tests/lib/User/SessionTest.php | 69 +++++++++++++--------------------- 1 file changed, 26 insertions(+), 43 deletions(-) diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 614ed3d015a..1b3d5cc4601 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -33,11 +33,11 @@ use OCP\Security\ISecureRandom; * @package Test\User */ class SessionTest extends \Test\TestCase { - /** @var \OCP\AppFramework\Utility\ITimeFactory */ + /** @var ITimeFactory|\PHPUnit_Framework_MockObject_MockObject */ private $timeFactory; - /** @var \OC\Authentication\Token\DefaultTokenProvider */ + /** @var DefaultTokenProvider|\PHPUnit_Framework_MockObject_MockObject */ protected $tokenProvider; - /** @var \OCP\IConfig */ + /** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */ private $config; /** @var Throttler */ private $throttler; @@ -124,11 +124,9 @@ class SessionTest extends \Test\TestCase { public function testIsLoggedIn($isLoggedIn) { $session = $this->getMockBuilder(Memory::class)->setConstructorArgs([''])->getMock(); - $manager = $this->getMockBuilder('\OC\User\Manager') - ->disableOriginalConstructor() - ->getMock(); + $manager = $this->createMock(Manager::class); - $userSession = $this->getMockBuilder('\OC\User\Session') + $userSession = $this->getMockBuilder(Session::class) ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config]) ->setMethods([ 'getUser' @@ -182,7 +180,7 @@ class SessionTest extends \Test\TestCase { } }, 'foo')); - $managerMethods = get_class_methods('\OC\User\Manager'); + $managerMethods = get_class_methods(Manager::class); //keep following methods intact in order to ensure hooks are //working $doNotMock = array('__construct', 'emit', 'listen'); @@ -211,7 +209,7 @@ class SessionTest extends \Test\TestCase { ->with('foo', 'bar') ->will($this->returnValue($user)); - $userSession = $this->getMockBuilder('\OC\User\Session') + $userSession = $this->getMockBuilder(Session::class) ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config]) ->setMethods([ 'prepareUserLogin' @@ -310,7 +308,6 @@ class SessionTest extends \Test\TestCase { public function testLoginNonExisting() { $session = $this->getMockBuilder(Memory::class)->setConstructorArgs([''])->getMock(); $manager = $this->createMock(Manager::class); - $backend = $this->createMock(\Test\Util\User\Dummy::class); $userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config); $session->expects($this->never()) @@ -337,7 +334,6 @@ class SessionTest extends \Test\TestCase { public function testLoginWithDifferentTokenLoginName() { $session = $this->getMockBuilder(Memory::class)->setConstructorArgs([''])->getMock(); $manager = $this->createMock(Manager::class); - $backend = $this->createMock(\Test\Util\User\Dummy::class); $userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config); $username = 'user123'; $token = new \OC\Authentication\Token\DefaultToken(); @@ -364,14 +360,12 @@ class SessionTest extends \Test\TestCase { * @expectedException \OC\Authentication\Exceptions\PasswordLoginForbiddenException */ public function testLogClientInNoTokenPasswordWith2fa() { - $manager = $this->getMockBuilder('\OC\User\Manager') - ->disableOriginalConstructor() - ->getMock(); + $manager = $this->createMock(Manager::class); $session = $this->createMock(ISession::class); $request = $this->createMock(IRequest::class); /** @var \OC\User\Session $userSession */ - $userSession = $this->getMockBuilder('\OC\User\Session') + $userSession = $this->getMockBuilder(Session::class) ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config]) ->setMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser']) ->getMock(); @@ -402,15 +396,12 @@ class SessionTest extends \Test\TestCase { } public function testLogClientInUnexist() { - $manager = $this->getMockBuilder('\OC\User\Manager') - ->disableOriginalConstructor() - ->getMock(); - $session = $this->createMock('\OCP\ISession'); - $request = $this->createMock('\OCP\IRequest'); - $user = $this->createMock('\OCP\IUser'); + $manager = $this->createMock(Manager::class); + $session = $this->createMock(ISession::class); + $request = $this->createMock(IRequest::class); - /** @var \OC\User\Session $userSession */ - $userSession = $this->getMockBuilder('\OC\User\Session') + /** @var Session $userSession */ + $userSession = $this->getMockBuilder(Session::class) ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config]) ->setMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser']) ->getMock(); @@ -424,18 +415,16 @@ class SessionTest extends \Test\TestCase { ->with('token_auth_enforced', false) ->will($this->returnValue(false)); - $this->assertFalse($userSession->logClientIn('unexist', 'doe', $request)); + $this->assertFalse($userSession->logClientIn('unexist', 'doe', $request, $this->throttler)); } public function testLogClientInWithTokenPassword() { - $manager = $this->getMockBuilder('\OC\User\Manager') - ->disableOriginalConstructor() - ->getMock(); + $manager = $this->createMock(Manager::class); $session = $this->createMock(ISession::class); $request = $this->createMock(IRequest::class); /** @var \OC\User\Session $userSession */ - $userSession = $this->getMockBuilder('\OC\User\Session') + $userSession = $this->getMockBuilder(Session::class) ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config]) ->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser']) ->getMock(); @@ -472,14 +461,12 @@ class SessionTest extends \Test\TestCase { * @expectedException \OC\Authentication\Exceptions\PasswordLoginForbiddenException */ public function testLogClientInNoTokenPasswordNo2fa() { - $manager = $this->getMockBuilder('\OC\User\Manager') - ->disableOriginalConstructor() - ->getMock(); + $manager = $this->createMock(Manager::class); $session = $this->createMock(ISession::class); $request = $this->createMock(IRequest::class); /** @var \OC\User\Session $userSession */ - $userSession = $this->getMockBuilder('\OC\User\Session') + $userSession = $this->getMockBuilder(Session::class) ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config]) ->setMethods(['login', 'isTwoFactorEnforced']) ->getMock(); @@ -530,7 +517,7 @@ class SessionTest extends \Test\TestCase { $session->expects($this->once()) ->method('regenerateId'); - $managerMethods = get_class_methods('\OC\User\Manager'); + $managerMethods = get_class_methods(Manager::class); //keep following methods intact in order to ensure hooks are //working $doNotMock = array('__construct', 'emit', 'listen'); @@ -992,9 +979,7 @@ class SessionTest extends \Test\TestCase { } public function testUpdateAuthTokenLastCheck() { - $manager = $this->getMockBuilder('\OC\User\Manager') - ->disableOriginalConstructor() - ->getMock(); + $manager = $this->createMock(Manager::class); $session = $this->createMock(ISession::class); $request = $this->createMock(IRequest::class); @@ -1007,8 +992,8 @@ class SessionTest extends \Test\TestCase { $mapper = $this->getMockBuilder(DefaultTokenMapper::class) ->disableOriginalConstructor() ->getMock(); - $crypto = $this->getMock(ICrypto::class); - $logger = $this->getMock(ILogger::class); + $crypto = $this->createMock(ICrypto::class); + $logger = $this->createMock(ILogger::class); $tokenProvider = new DefaultTokenProvider($mapper, $crypto, $this->config, $logger, $this->timeFactory); /** @var \OC\User\Session $userSession */ @@ -1044,9 +1029,7 @@ class SessionTest extends \Test\TestCase { } public function testNoUpdateAuthTokenLastCheckRecent() { - $manager = $this->getMockBuilder('\OC\User\Manager') - ->disableOriginalConstructor() - ->getMock(); + $manager = $this->createMock(Manager::class); $session = $this->createMock(ISession::class); $request = $this->createMock(IRequest::class); @@ -1059,8 +1042,8 @@ class SessionTest extends \Test\TestCase { $mapper = $this->getMockBuilder(DefaultTokenMapper::class) ->disableOriginalConstructor() ->getMock(); - $crypto = $this->getMock(ICrypto::class); - $logger = $this->getMock(ILogger::class); + $crypto = $this->createMock(ICrypto::class); + $logger = $this->createMock(ILogger::class); $tokenProvider = new DefaultTokenProvider($mapper, $crypto, $this->config, $logger, $this->timeFactory); /** @var \OC\User\Session $userSession */