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
owncloud · Feb 13, 2018 · b1fc460 · b1fc460
1 parent d5be225
commit b1fc460
Show file tree

Hide file tree

Showing 6 changed files with 198 additions and 54 deletions.
diff --git a/lib/private/User/BasicAuthModule.php b/lib/private/User/BasicAuthModule.php
@@ -41,21 +41,29 @@ 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 (!isset($request->server['PHP_AUTH_USER'], $request->server['PHP_AUTH_PW'])) {
+			return null;
+		}
+		$authUser = $request->server['PHP_AUTH_USER'];
+		$authPass = $request->server['PHP_AUTH_PW'];
+		if ($authUser === '' || $authPass === '') {
 			return null;
 		}
 
 		// check uid and password
-		$user = $this->manager->checkPassword($request->server['PHP_AUTH_USER'], $request->server['PHP_AUTH_PW']);
+		$user = $this->manager->checkPassword($authUser, $authPass);
 		if ($user instanceof IUser) {
 			return $user;
 		}
 		// check email and password
-		$users = $this->manager->getByEmail($request->server['PHP_AUTH_USER']);
-		if (count($users) !== 1) {
-			return null;
+		$users = $this->manager->getByEmail($authUser);
+		if (count($users) === 1) {
+			$user = $this->manager->checkPassword($users[0]->getUID(), $authPass);
+		}
+		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
@@ -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,29 +989,41 @@ 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;
 	}
 
 	/**
 	 * @param $includeBuiltIn
 	 * @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
@@ -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,55 @@ 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 (!isset($request->server['PHP_AUTH_USER'], $request->server['PHP_AUTH_PW'])) {
+			return null;
+		}
+
+		try {
+			$token = $request->server['PHP_AUTH_PW'];
+			$dbToken = $this->tokenProvider->getToken($token);
+			if ($dbToken->getUID() !== $request->server['PHP_AUTH_USER']) {
+				throw new \Exception('Invalid credentials');
+			}
+
+			return $dbToken;
+		} 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
@@ -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
@@ -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));
+	}
 
 }