From e7a550e8a4bb259a151fc6db1647789330b1d4b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Mon, 5 Feb 2018 12:31:48 +0100 Subject: [PATCH] When verifying the request authentication mechanisms we need to loop over all modules and allow each of them to verify the headers and in case something is terribly wrong the user will be logged out and the session will be destroyed. The BasicAuthModule now handles app passwords as well - this was missing before and caused users being locked out of ldap. Solves OAuth token expiry issue - fixes owncloud/oauth2#103 --- lib/private/User/BasicAuthModule.php | 11 +++-- lib/private/User/Session.php | 55 +++++++++++++-------- lib/private/User/TokenAuthModule.php | 67 ++++++++++++++++++++------ tests/lib/User/BasicAuthModuleTest.php | 18 ++++--- tests/lib/User/SessionTest.php | 54 +++++++++++++++++++++ tests/lib/User/TokenAuthModuleTest.php | 32 +++++++++--- 6 files changed, 185 insertions(+), 52 deletions(-) diff --git a/lib/private/User/BasicAuthModule.php b/lib/private/User/BasicAuthModule.php index d8c7fce67a0c..3c5a2457f1bf 100644 --- a/lib/private/User/BasicAuthModule.php +++ b/lib/private/User/BasicAuthModule.php @@ -41,7 +41,7 @@ public function __construct(IUserManager $manager) { * @inheritdoc */ public function auth(IRequest $request) { - if (empty($request->server['PHP_AUTH_USER']) || empty($request->server['PHP_AUTH_PW'])) { + if ($request->server['PHP_AUTH_USER'] === '' || $request->server['PHP_AUTH_PW'] === '') { return null; } @@ -52,10 +52,13 @@ public function auth(IRequest $request) { } // check email and password $users = $this->manager->getByEmail($request->server['PHP_AUTH_USER']); - if (count($users) !== 1) { - return null; + if (count($users) === 1) { + $user = $this->manager->checkPassword($users[0]->getUID(), $request->server['PHP_AUTH_PW']); + } + if ($user instanceof IUser) { + return $user; } - return $this->manager->checkPassword($users[0]->getUID(), $request->server['PHP_AUTH_PW']); + throw new \Exception('Invalid credentials'); } /** diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 08a0231bf3ae..34bf2c7dacec 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -283,15 +283,15 @@ public function setLoginName($loginName) { public function getLoginName() { if ($this->activeUser) { return $this->session->get('loginname'); - } else { - $uid = $this->session->get('user_id'); - if ($uid) { - $this->activeUser = $this->manager->get($uid); - return $this->session->get('loginname'); - } else { - return null; - } } + + $uid = $this->session->get('user_id'); + if ($uid) { + $this->activeUser = $this->manager->get($uid); + return $this->session->get('loginname'); + } + + return null; } /** @@ -320,6 +320,7 @@ public function login($uid, $password) { * @param string $user * @param string $password * @param IRequest $request + * @throws \InvalidArgumentException * @throws LoginException * @throws PasswordLoginForbiddenException * @return boolean @@ -988,21 +989,33 @@ public function updateSessionTokenPassword($password) { } public function verifyAuthHeaders($request) { - foreach ($this->getAuthModules(true) as $module) { - $user = $module->auth($request); - if ($user !== null) { - if ($this->isLoggedIn() && $this->getUser()->getUID() !== $user->getUID()) { - // the session is bad -> kill it - $this->logout(); - return false; + $shallLogout = false; + try { + $lastUser = null; + foreach ($this->getAuthModules(true) as $module) { + $user = $module->auth($request); + if ($user !== null) { + if ($this->isLoggedIn() && $this->getUser()->getUID() !== $user->getUID()) { + $shallLogout = true; + break; + } + if ($lastUser !== null && $user->getUID() !== $lastUser->getUID()) { + $shallLogout = true; + break; + } + $lastUser = $user; } - return true; } - } - // the session is bad -> kill it - $this->logout(); - return false; + } catch (Exception $ex) { + $shallLogout = true; + } + if ($shallLogout) { + // the session is bad -> kill it + $this->logout(); + return false; + } + return true; } /** @@ -1010,7 +1023,7 @@ public function verifyAuthHeaders($request) { * @return \Generator | IAuthModule[] * @throws Exception */ - private function getAuthModules($includeBuiltIn) { + protected function getAuthModules($includeBuiltIn) { if ($includeBuiltIn) { yield new TokenAuthModule($this->session, $this->tokenProvider, $this->manager); } diff --git a/lib/private/User/TokenAuthModule.php b/lib/private/User/TokenAuthModule.php index eda10efd3854..7e1cf7dd543c 100644 --- a/lib/private/User/TokenAuthModule.php +++ b/lib/private/User/TokenAuthModule.php @@ -26,9 +26,11 @@ use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Exceptions\PasswordlessTokenException; use OC\Authentication\Token\IProvider; +use OC\Authentication\Token\IToken; use OCP\Authentication\IAuthModule; use OCP\IRequest; use OCP\ISession; +use OCP\IUser; use OCP\IUserManager; use OCP\Session\Exceptions\SessionNotAvailableException; @@ -56,24 +58,13 @@ public function __construct(ISession $session, IProvider $tokenProvider, IUserMa * @inheritdoc */ public function auth(IRequest $request) { - $authHeader = $request->getHeader('Authorization'); - if ($authHeader === null || strpos($authHeader, 'token ') === false) { - // No auth header, let's try session id - try { - $token = $this->session->getId(); - } catch (SessionNotAvailableException $ex) { - return null; - } - } else { - $token = substr($authHeader, 6); + $dbToken = $this->getTokenForAppPassword($request, $token); + if ($dbToken === null) { + $dbToken = $this->getToken($request, $token); } - - try { - $dbToken = $this->tokenProvider->getToken($token); - } catch (InvalidTokenException $ex) { + if ($dbToken === null) { return null; } - $uid = $dbToken->getUID(); // When logging in with token, the password must be decrypted first before passing to login hook try { @@ -82,6 +73,7 @@ public function auth(IRequest $request) { // Ignore and use empty string instead } + $uid = $dbToken->getUID(); return $this->manager->get($uid); } @@ -91,4 +83,49 @@ public function auth(IRequest $request) { public function getUserPassword(IRequest $request) { return $this->password; } + + /** + * @param IRequest $request + * @return null|IToken + */ + private function getTokenForAppPassword(IRequest $request, &$token) { + if ($request->server['PHP_AUTH_USER'] === '' || $request->server['PHP_AUTH_PW'] === '') { + return null; + } + try { + $token = $request->server['PHP_AUTH_PW']; + return $this->tokenProvider->getToken($token); + } catch (InvalidTokenException $ex) { + // invalid app passwords do NOT throw an exception because basic + // auth headers can be evaluated properly in the basic auth module + $token = null; + return null; + } + } + + /** + * @param IRequest $request + * @return null|IToken + * @throws \Exception + */ + private function getToken(IRequest $request, &$token) { + $authHeader = $request->getHeader('Authorization'); + if ($authHeader === null || strpos($authHeader, 'token ') === false) { + // No auth header, let's try session id + try { + $token = $this->session->getId(); + } catch (SessionNotAvailableException $ex) { + return null; + } + } else { + $token = substr($authHeader, 6); + } + + try { + return $this->tokenProvider->getToken($token); + } catch (InvalidTokenException $ex) { + $token = null; + return null; + } + } } diff --git a/tests/lib/User/BasicAuthModuleTest.php b/tests/lib/User/BasicAuthModuleTest.php index 41b67f2bdb47..083a97cfb9c0 100644 --- a/tests/lib/User/BasicAuthModuleTest.php +++ b/tests/lib/User/BasicAuthModuleTest.php @@ -42,8 +42,8 @@ public function setUp() { parent::setUp(); $this->manager = $this->createMock(IUserManager::class); $this->request = $this->createMock(IRequest::class); - $this->user = $this->createMock(IUser::class); + $this->user = $this->createMock(IUser::class); $this->user->expects($this->any())->method('getUID')->willReturn('user1'); $this->manager->expects($this->any())->method('checkPassword') @@ -65,16 +65,20 @@ public function setUp() { /** * @dataProvider providesCredentials - * @param bool $expectsUser + * @param mixed $expectedResult * @param string $userId */ - public function testAuth($expectsUser, $userId) { + public function testAuth($expectedResult, $userId) { $module = new BasicAuthModule($this->manager); $this->request->server = [ 'PHP_AUTH_USER' => $userId, 'PHP_AUTH_PW' => '123456', ]; - $this->assertEquals($expectsUser ? $this->user : null, $module->auth($this->request)); + if ($expectedResult instanceof \Exception) { + $this->expectException(get_class($expectedResult)); + $this->expectExceptionMessage($expectedResult->getMessage()); + } + $this->assertEquals($expectedResult ? $this->user : null, $module->auth($this->request)); } public function testGetUserPassword() { @@ -90,12 +94,14 @@ public function testGetUserPassword() { } public function providesCredentials() { + return [ + 'no user is' => [false, ''], 'user1 can login' => [true, 'user1'], 'user1 can login with email' => [true, 'user@example.com'], 'unique email can login' => [true, 'unique@example.com'], - 'not unique email can not login' => [false, 'not-unique@example.com'], - 'user2 is not known' => [false, 'user2'], + 'not unique email can not login' => [new \Exception('Invalid credentials'), 'not-unique@example.com'], + 'user2 is not known' => [new \Exception('Invalid credentials'), 'user2'], ]; } } diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 355d12e0a613..a1d18406c8f0 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -22,6 +22,7 @@ use OC\User\Session; use OCP\App\IServiceLoader; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\Authentication\IAuthModule; use OCP\IConfig; use OCP\IRequest; use OCP\ISession; @@ -1051,5 +1052,58 @@ public function testApacheLogin() { $this->assertFalse($loginVal); } + public function providesModules() { + $nullModule = $this->createMock(IAuthModule::class); + $nullModule->expects($this->any())->method('auth')->willReturn(null); + $throwingModule = $this->createMock(IAuthModule::class); + $throwingModule->expects($this->any())->method('auth')->willThrowException(new \Exception('Invalid token')); + $user1 = $this->createMock(IUser::class); + $user1->expects($this->any())->method('getUID')->willReturn('user1'); + $user1Module = $this->createMock(IAuthModule::class); + $user1Module->expects($this->any())->method('auth')->willReturn($user1); + $user2 = $this->createMock(IUser::class); + $user2->expects($this->any())->method('getUID')->willReturn('user2'); + $user2Module = $this->createMock(IAuthModule::class); + $user2Module->expects($this->any())->method('auth')->willReturn($user2); + return [ + 'no modules' => [true, []], + 'module returning null' => [true, [$nullModule]], + 'module returning a user' => [true, [$user1Module]], + 'module throwing an exception' => [false, [$throwingModule]], + 'modules returning different user' => [false, [$user1Module, $user2Module]], + 'logged in user does not match' => [false, [$user2Module], $user1], + ]; + } + + /** + * @dataProvider providesModules + * @param $expectedReturn + * @param array $modules + */ + public function testVerifyAuthHeaders($expectedReturn, array $modules, $loggedInUser = null) { + /** @var IRequest | \PHPUnit_Framework_MockObject_MockObject $request */ + $request = $this->createMock(IRequest::class); + /** @var IUserManager | \PHPUnit_Framework_MockObject_MockObject $userManager */ + $userManager = $this->createMock(IUserManager::class); + /** @var ISession | \PHPUnit_Framework_MockObject_MockObject $session */ + $session = $this->createMock(ISession::class); + /** @var ITimeFactory | \PHPUnit_Framework_MockObject_MockObject $timeFactory */ + $timeFactory = $this->createMock(ITimeFactory::class); + /** @var IProvider | \PHPUnit_Framework_MockObject_MockObject $tokenProvider */ + $tokenProvider = $this->createMock(IProvider::class); + + /** @var Session | \PHPUnit_Framework_MockObject_MockObject $session */ + $session = $this->getMockBuilder(Session::class) + ->setConstructorArgs([$userManager, $session, $timeFactory, $tokenProvider, $this->config, $this->serviceLoader, $this->userSyncService]) + ->setMethods(['getAuthModules', 'logout', 'isLoggedIn', 'getUser']) + ->getMock(); + $session->expects($this->any())->method('getAuthModules')->willReturn($modules); + + $session->expects($this->any())->method('isLoggedIn')->willReturn($loggedInUser !== null); + $session->expects($this->any())->method('getUser')->willReturn($loggedInUser); + + $session->expects($expectedReturn ? $this->never() : $this->once())->method('logout'); + $this->assertEquals( $expectedReturn, $session->verifyAuthHeaders($request)); + } } diff --git a/tests/lib/User/TokenAuthModuleTest.php b/tests/lib/User/TokenAuthModuleTest.php index 0bcdb8f0dc14..0222f7395aa3 100644 --- a/tests/lib/User/TokenAuthModuleTest.php +++ b/tests/lib/User/TokenAuthModuleTest.php @@ -31,6 +31,7 @@ use OCP\ISession; use OCP\IUser; use OCP\IUserManager; +use OCP\Session\Exceptions\SessionNotAvailableException; use Test\TestCase; class TokenAuthModuleTest extends TestCase { @@ -54,6 +55,7 @@ public function setUp() { $this->request = $this->createMock(IRequest::class); $this->user = $this->createMock(IUser::class); + $this->session->expects($this->any())->method('getId')->willThrowException(new SessionNotAvailableException()); $this->user->expects($this->any())->method('getUID')->willReturn('user1'); $this->tokenProvider->expects($this->any())->method('getToken') @@ -82,16 +84,33 @@ public function setUp() { /** * @dataProvider providesCredentials - * @param bool $expectsUser + * @param mixed $expectedResult * @param string $authHeader * @param string $password */ - public function testModule($expectsUser, $authHeader, $password = '') { + public function testModule($expectedResult, $authHeader, $password = '') { $module = new TokenAuthModule($this->session, $this->tokenProvider, $this->manager); - $this->request->expects($this->any())->method('getHeader')->willReturn($authHeader); + if ($authHeader === 'basic') { + $this->request->server = [ + 'PHP_AUTH_USER' => 'user1', + 'PHP_AUTH_PW' => 'valid-token', + ]; + } else { + $this->request->server = [ + 'PHP_AUTH_USER' => '', + 'PHP_AUTH_PW' => '', + ]; + $this->request->expects($this->any())->method('getHeader')->willReturn($authHeader); + } - $this->assertEquals($expectsUser ? $this->user : null, $module->auth($this->request)); - $this->assertEquals($password, $module->getUserPassword($this->request)); + if ($expectedResult instanceof \Exception) { + $this->expectException(get_class($expectedResult)); + $this->expectExceptionMessage($expectedResult->getMessage()); + $module->auth($this->request); + } else { + $this->assertEquals($expectedResult ? $this->user : null, $module->auth($this->request)); + $this->assertEquals($password, $module->getUserPassword($this->request)); + } } public function providesCredentials() { @@ -99,7 +118,8 @@ public function providesCredentials() { 'no auth header' => [false, ''], 'not valid token' => [false, 'token whateverbutnothingvalid'], 'valid token' => [true, 'token valid-token'], - 'valid token with password' => [true, 'token valid-token-with-password', 'supersecret'] + 'valid token with password' => [true, 'token valid-token-with-password', 'supersecret'], + 'valid app password' => [true, 'basic'], ]; } }