fix: Reduce the mixups between apptokens and session ids

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

Author: come-nc

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_User;
@@ -435,8 +436,15 @@ public function logClientIn($user,
 		}
 
 		try {
-			$isTokenPassword = $this->isTokenPassword($password);
-		} catch (ExpiredTokenException $e) {
+			$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) {
 			// Just return on an expired token no need to check further or record a failed login
 			return false;
 		}
@@ -458,7 +466,6 @@ public function logClientIn($user,
 			}
 
 			if ($isTokenPassword) {
-				$dbToken = $this->tokenProvider->getToken($password);
 				$userFromToken = $this->manager->get($dbToken->getUID());
 				$isValidEmailLogin = $userFromToken->getEMailAddress() === $user;
 			} else {
@@ -547,6 +554,24 @@ public function isTokenPassword($password) {
 		}
 	}
 
+	/**
+	 * Check if the given 'password' is actually a device token
+	 *
+	 * @throws ExpiredTokenException
+	 */
+	private function getTokenFromPassword(string $password): ?\OCP\Authentication\Token\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
@@ -828,29 +853,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

@@ -403,16 +403,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 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')
@@ -1063,16 +1065,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')
@@ -1113,12 +1117,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(false);
+		$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')