Merge pull request #59806 from nextcloud/backport/59804/stable22

[stable22] fix: Reduce the mixups between apptokens and session ids

Author: sorbaugh

lib/private/User/Session.php

@@ -45,6 +45,7 @@
 use OC\Authentication\Exceptions\PasswordLoginForbiddenException;
 use OC\Authentication\Token\IProvider;
 use OC\Authentication\Token\IToken;
+use OC\Authentication\Token\PublicKeyToken;
 use OC\Hooks\Emitter;
 use OC\Hooks\PublicEmitter;
 use OC\Security\Bruteforce\Throttler;
@@ -446,7 +447,14 @@ public function logClientIn($user,
 		}
 
 		try {
-			$isTokenPassword = $this->isTokenPassword($password);
+			$dbToken = $this->getTokenFromPassword($password);
+			$isTokenPassword = $dbToken !== null;
+			if (($dbToken instanceof PublicKeyToken)
+				&& ($dbToken->getType() !== IToken::PERMANENT_TOKEN)
+			) {
+				// Refuse session tokens here, only app tokens are handled
+				return false;
+			}
 		} catch (ExpiredTokenException $e) {
 			// Just return on an expired token no need to check further or record a failed login
 			return false;
@@ -550,6 +558,24 @@ public function isTokenPassword($password) {
 		}
 	}
 
+	/**
+	 * Check if the given 'password' is actually a device token
+	 *
+	 * @throws ExpiredTokenException
+	 */
+	private function getTokenFromPassword(string $password): ?IToken {
+		try {
+			return $this->tokenProvider->getToken($password);
+		} catch (ExpiredTokenException $e) {
+			throw $e;
+		} catch (InvalidTokenException $ex) {
+			$this->logger->debug('Token is not valid: ' . $ex->getMessage(), [
+				'exception' => $ex,
+			]);
+			return null;
+		}
+	}
+
 	protected function prepareUserLogin($firstTimeLogin, $refreshCsrfToken = true) {
 		if ($refreshCsrfToken) {
 			// TODO: mock/inject/use non-static
@@ -831,29 +857,36 @@ private function validateToken($token, $user = null) {
 	 */
 	public function tryTokenLogin(IRequest $request) {
 		$authHeader = $request->getHeader('Authorization');
+		$tokenFromCookie = false;
 		if (strpos($authHeader, 'Bearer ') === 0) {
 			$token = substr($authHeader, 7);
 		} else {
 			// No auth header, let's try session id
 			try {
 				$token = $this->session->getId();
+				$tokenFromCookie = true;
 			} catch (SessionNotAvailableException $ex) {
 				return false;
 			}
 		}
 
-		if (!$this->loginWithToken($token)) {
+		try {
+			$dbToken = $this->tokenProvider->getToken($token);
+		} catch (InvalidTokenException $e) {
+			// Can't really happen but better safe than sorry
 			return false;
 		}
-		if (!$this->validateToken($token)) {
+
+		if ($dbToken instanceof PublicKeyToken && $dbToken->getType() === IToken::TEMPORARY_TOKEN && !$tokenFromCookie) {
+			// Session token but from Bearer header, not allowed
 			return false;
 		}
 
-		try {
-			$dbToken = $this->tokenProvider->getToken($token);
-		} catch (InvalidTokenException $e) {
-			// Can't really happen but better save than sorry
-			return true;
+		if (!$this->loginWithToken($token)) {
+			return false;
+		}
+		if (!$this->validateToken($token)) {
+			return false;
 		}
 
 		// Remember me tokens are not app_passwords

tests/lib/User/SessionTest.php

@@ -1,4 +1,5 @@
 <?php
+
 /**
  * Copyright (c) 2013 Robin Appelman <icewind@owncloud.com>
  * This file is licensed under the Affero General Public License version 3 or
@@ -10,6 +11,7 @@
 
 use OC\AppFramework\Http\Request;
 use OC\Authentication\Events\LoginFailed;
+use OC\Authentication\Exceptions\InvalidTokenException;
 use OC\Authentication\Token\DefaultTokenMapper;
 use OC\Authentication\Token\DefaultTokenProvider;
 use OC\Authentication\Token\IProvider;
@@ -489,16 +491,18 @@ public function testLogClientInWithTokenPassword() {
 		$manager = $this->createMock(Manager::class);
 		$session = $this->createMock(ISession::class);
 		$request = $this->createMock(IRequest::class);
+		$token = $this->createMock(IToken::class);
 
 		/** @var \OC\User\Session $userSession */
 		$userSession = $this->getMockBuilder(Session::class)
 			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher])
-			->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser'])
+			->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser'])
 			->getMock();
 
-		$userSession->expects($this->once())
-			->method('isTokenPassword')
-			->willReturn(true);
+		$this->tokenProvider->expects($this->once())
+			->method('getToken')
+			->with('I-AM-AN-APP-PASSWORD')
+			->willReturn($token);
 		$userSession->expects($this->once())
 			->method('login')
 			->with('john', 'I-AM-AN-APP-PASSWORD')
@@ -1032,7 +1036,7 @@ public function testTryTokenLoginWithDisabledUser() {
 			->method('getHeader')
 			->with('Authorization')
 			->willReturn('Bearer xxxxx');
-		$this->tokenProvider->expects($this->once())
+		$this->tokenProvider->expects($this->atLeastOnce())
 			->method('getToken')
 			->with('xxxxx')
 			->willReturn($token);
@@ -1478,16 +1482,18 @@ public function testLogClientInThrottlerUsername() {
 		$manager = $this->createMock(Manager::class);
 		$session = $this->createMock(ISession::class);
 		$request = $this->createMock(IRequest::class);
+		$token = $this->createMock(IToken::class);
 
 		/** @var Session $userSession */
 		$userSession = $this->getMockBuilder(Session::class)
 			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher])
-			->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser'])
+			->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser'])
 			->getMock();
 
-		$userSession->expects($this->once())
-			->method('isTokenPassword')
-			->willReturn(true);
+		$this->tokenProvider->expects($this->once())
+			->method('getToken')
+			->with('I-AM-AN-PASSWORD')
+			->willReturn($token);
 		$userSession->expects($this->once())
 			->method('login')
 			->with('john', 'I-AM-AN-PASSWORD')
@@ -1528,12 +1534,13 @@ public function testLogClientInThrottlerEmail() {
 		/** @var Session $userSession */
 		$userSession = $this->getMockBuilder(Session::class)
 			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher])
-			->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser'])
+			->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser'])
 			->getMock();
 
-		$userSession->expects($this->once())
-			->method('isTokenPassword')
-			->willReturn(true);
+		$this->tokenProvider->expects($this->once())
+			->method('getToken')
+			->with('I-AM-AN-PASSWORD')
+			->willThrowException(new InvalidTokenException());
 		$userSession->expects($this->once())
 			->method('login')
 			->with('john@foo.bar', 'I-AM-AN-PASSWORD')