Merge pull request #11390 from nextcloud/feature/11043/apptoken_v3

Apptoken v3: imrpove token handling on external password change

core/Controller/LoginController.php

@@ -320,6 +320,7 @@ class LoginController extends Controller {
 		// requires https://github.com/owncloud/core/pull/24616
 		$this->userSession->completeLogin($loginResult, ['loginName' => $user, 'password' => $password]);
 		$this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password, IToken::REMEMBER);
+		$this->userSession->updateTokens($loginResult->getUID(), $password);
 
 		// User has successfully logged in, now remove the password reset link, when it is available
 		$this->config->deleteUserValue($loginResult->getUID(), 'core', 'lostpassword');

core/Migrations/Version15000Date20180926101451.php

@@ -0,0 +1,53 @@
+<?php
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2018 Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @author Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OC\Core\Migrations;
+
+use Closure;
+use OCP\DB\ISchemaWrapper;
+use OCP\Migration\SimpleMigrationStep;
+use OCP\Migration\IOutput;
+
+class Version15000Date20180926101451 extends SimpleMigrationStep {
+
+	/**
+	 * @param IOutput $output
+	 * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
+	 * @param array $options
+	 * @return null|ISchemaWrapper
+	 */
+	public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) {
+		/** @var ISchemaWrapper $schema */
+		$schema = $schemaClosure();
+
+		$table = $schema->getTable('authtoken');
+		$table->addColumn('password_invalid','boolean', [
+			'notnull' => true,
+			'default' => false,
+		]);
+
+		return $schema;
+	}
+
+}

lib/composer/composer/autoload_classmap.php

@@ -626,6 +626,7 @@ return array(
     'OC\\Core\\Migrations\\Version14000Date20180626223656' => $baseDir . '/core/Migrations/Version14000Date20180626223656.php',
     'OC\\Core\\Migrations\\Version14000Date20180710092004' => $baseDir . '/core/Migrations/Version14000Date20180710092004.php',
     'OC\\Core\\Migrations\\Version14000Date20180712153140' => $baseDir . '/core/Migrations/Version14000Date20180712153140.php',
+    'OC\\Core\\Migrations\\Version15000Date20180926101451' => $baseDir . '/core/Migrations/Version15000Date20180926101451.php',
     'OC\\DB\\Adapter' => $baseDir . '/lib/private/DB/Adapter.php',
     'OC\\DB\\AdapterMySQL' => $baseDir . '/lib/private/DB/AdapterMySQL.php',
     'OC\\DB\\AdapterOCI8' => $baseDir . '/lib/private/DB/AdapterOCI8.php',

lib/composer/composer/autoload_static.php

@@ -656,6 +656,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OC\\Core\\Migrations\\Version14000Date20180626223656' => __DIR__ . '/../../..' . '/core/Migrations/Version14000Date20180626223656.php',
         'OC\\Core\\Migrations\\Version14000Date20180710092004' => __DIR__ . '/../../..' . '/core/Migrations/Version14000Date20180710092004.php',
         'OC\\Core\\Migrations\\Version14000Date20180712153140' => __DIR__ . '/../../..' . '/core/Migrations/Version14000Date20180712153140.php',
+        'OC\\Core\\Migrations\\Version15000Date20180926101451' => __DIR__ . '/../../..' . '/core/Migrations/Version15000Date20180926101451.php',
         'OC\\DB\\Adapter' => __DIR__ . '/../../..' . '/lib/private/DB/Adapter.php',
         'OC\\DB\\AdapterMySQL' => __DIR__ . '/../../..' . '/lib/private/DB/AdapterMySQL.php',
         'OC\\DB\\AdapterOCI8' => __DIR__ . '/../../..' . '/lib/private/DB/AdapterOCI8.php',

lib/private/Authentication/Token/DefaultTokenProvider.php

@@ -338,4 +338,16 @@ class DefaultTokenProvider implements IProvider {
 		}
 	}
 
+	public function markPasswordInvalid(IToken $token, string $tokenId) {
+		if (!($token instanceof DefaultToken)) {
+			throw new InvalidTokenException();
+		}
+
+		//No need to mark as invalid. We just invalide default tokens
+		$this->invalidateToken($tokenId);
+	}
+
+	public function updatePasswords(string $uid, string $password) {
+		// Nothing to do here
+	}
 }

lib/private/Authentication/Token/IProvider.php

@@ -156,4 +156,20 @@ interface IProvider {
 	 * @return IToken
 	 */
 	public function rotate(IToken $token, string $oldTokenId, string $newTokenId): IToken;
+
+	/**
+	 * Marks a token as having an invalid password.
+	 *
+	 * @param IToken $token
+	 * @param string $tokenId
+	 */
+	public function markPasswordInvalid(IToken $token, string $tokenId);
+
+	/**
+	 * Update all the passwords of $uid if required
+	 *
+	 * @param string $uid
+	 * @param string $password
+	 */
+	public function updatePasswords(string $uid, string $password);
 }

lib/private/Authentication/Token/Manager.php

@@ -227,4 +227,16 @@ class Manager implements IProvider {
 		}
 		throw new InvalidTokenException();
 	}
+
+
+	public function markPasswordInvalid(IToken $token, string $tokenId) {
+		$this->getProvider($token)->markPasswordInvalid($token, $tokenId);
+	}
+
+	public function updatePasswords(string $uid, string $password) {
+		$this->defaultTokenProvider->updatePasswords($uid, $password);
+		$this->publicKeyTokenProvider->updatePasswords($uid, $password);
+	}
+
+
 }

lib/private/Authentication/Token/PublicKeyToken.php

@@ -43,6 +43,7 @@ use OCP\AppFramework\Db\Entity;
  * @method string getPublicKey()
  * @method void setPublicKey(string $key)
  * @method void setVersion(int $version)
+ * @method bool getPasswordInvalid()
  */
 class PublicKeyToken extends Entity implements IToken {
 
@@ -90,6 +91,9 @@ class PublicKeyToken extends Entity implements IToken {
 	/** @var int */
 	protected $version;
 
+	/** @var bool */
+	protected $passwordInvalid;
+
 	public function __construct() {
 		$this->addType('uid', 'string');
 		$this->addType('loginName', 'string');
@@ -105,6 +109,7 @@ class PublicKeyToken extends Entity implements IToken {
 		$this->addType('publicKey', 'string');
 		$this->addType('privateKey', 'string');
 		$this->addType('version', 'int');
+		$this->addType('passwordInvalid', 'bool');
 	}
 
 	public function getId(): int {
@@ -214,4 +219,8 @@ class PublicKeyToken extends Entity implements IToken {
 	public function getExpires() {
 		return parent::getExpires();
 	}
+
+	public function setPasswordInvalid(bool $invalid) {
+		parent::setPasswordInvalid($invalid);
+	}
 }

lib/private/Authentication/Token/PublicKeyTokenMapper.php

@@ -169,4 +169,19 @@ class PublicKeyTokenMapper extends QBMapper {
 
 		$qb->execute();
 	}
+
+	public function hasExpiredTokens(string $uid): bool {
+		$qb = $this->db->getQueryBuilder();
+		$qb->select('*')
+			->from('authtoken')
+			->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid)))
+			->andWhere($qb->expr()->eq('password_invalid', $qb->createNamedParameter(true), IQueryBuilder::PARAM_BOOL))
+			->setMaxResults(1);
+
+		$cursor = $qb->execute();
+		$data = $cursor->fetchAll();
+		$cursor->closeCursor();
+
+		return count($data) === 1;
+	}
 }

lib/private/Authentication/Token/PublicKeyTokenProvider.php

@@ -317,4 +317,30 @@ class PublicKeyTokenProvider implements IProvider {
 
 		return $dbToken;
 	}
+
+	public function markPasswordInvalid(IToken $token, string $tokenId) {
+		if (!($token instanceof PublicKeyToken)) {
+			throw new InvalidTokenException();
+		}
+
+		$token->setPasswordInvalid(true);
+		$this->mapper->update($token);
+	}
+
+	public function updatePasswords(string $uid, string $password) {
+		if (!$this->mapper->hasExpiredTokens($uid)) {
+			// Nothing to do here
+			return;
+		}
+
+		// Update the password for all tokens
+		$tokens = $this->mapper->getTokenByUser($uid);
+		foreach ($tokens as $t) {
+			$publicKey = $t->getPublicKey();
+			$t->setPassword($this->encryptPassword($password, $publicKey));
+			$this->updateToken($t);
+		}
+	}
+
+
 }

lib/private/User/Session.php

@@ -694,12 +694,19 @@ class Session implements IUserSession, Emitter {
 			return true;
 		}
 
-		if ($this->manager->checkPassword($dbToken->getLoginName(), $pwd) === false
-			|| (!is_null($this->activeUser) && !$this->activeUser->isEnabled())) {
+		// Invalidate token if the user is no longer active
+		if (!is_null($this->activeUser) && !$this->activeUser->isEnabled()) {
 			$this->tokenProvider->invalidateToken($token);
-			// Password has changed or user was disabled -> log user out
 			return false;
 		}
+
+		// If the token password is no longer valid mark it as such
+		if ($this->manager->checkPassword($dbToken->getLoginName(), $pwd) === false) {
+			$this->tokenProvider->markPasswordInvalid($dbToken, $token);
+			// User is logged out
+			return false;
+		}
+
 		$dbToken->setLastCheck($now);
 		return true;
 	}
@@ -943,5 +950,9 @@ class Session implements IUserSession, Emitter {
 		}
 	}
 
+	public function updateTokens(string $uid, string $password) {
+		$this->tokenProvider->updatePasswords($uid, $password);
+	}
+
 
 }

tests/lib/Authentication/Token/DefaultTokenProviderTest.php

@@ -28,6 +28,7 @@ use OC\Authentication\Token\DefaultTokenMapper;
 use OC\Authentication\Token\DefaultTokenProvider;
 use OC\Authentication\Token\ExpiredTokenException;
 use OC\Authentication\Token\IToken;
+use OC\Authentication\Token\PublicKeyToken;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IConfig;
@@ -532,4 +533,22 @@ class DefaultTokenProviderTest extends TestCase {
 
 		$this->tokenProvider->rotate($token, 'oldtoken', 'newtoken');
 	}
+
+	public function testMarkPasswordInvalidInvalidToken() {
+		$token = $this->createMock(PublicKeyToken::class);
+
+		$this->expectException(InvalidTokenException::class);
+
+		$this->tokenProvider->markPasswordInvalid($token, 'tokenId');
+	}
+
+	public function testMarkPasswordInvalid() {
+		$token = $this->createMock(DefaultToken::class);
+
+		$this->mapper->expects($this->once())
+			->method('invalidate')
+			->with('0c7db0098fe8ddba6032b22719ec18867c69a1820fa36d71c28bf96d52843bdc44a112bd24093b049be5bb54769bcb72d67190a4a9690e51aac263cba38186fb');
+
+		$this->tokenProvider->markPasswordInvalid($token, 'tokenId');
+	}
 }

tests/lib/Authentication/Token/ManagerTest.php

@@ -448,4 +448,45 @@ class ManagerTest extends TestCase {
 
 		$this->assertSame($newToken, $this->manager->rotate($oldToken, 'oldId', 'newId'));
 	}
+
+	public function testMarkPasswordInvalidDefault() {
+		$token = $this->createMock(DefaultToken::class);
+
+		$this->defaultTokenProvider->expects($this->once())
+			->method('markPasswordInvalid')
+			->with($token, 'tokenId');
+		$this->publicKeyTokenProvider->expects($this->never())
+			->method($this->anything());
+
+		$this->manager->markPasswordInvalid($token, 'tokenId');
+	}
+
+	public function testMarkPasswordInvalidPublicKey() {
+		$token = $this->createMock(PublicKeyToken::class);
+
+		$this->publicKeyTokenProvider->expects($this->once())
+			->method('markPasswordInvalid')
+			->with($token, 'tokenId');
+		$this->defaultTokenProvider->expects($this->never())
+			->method($this->anything());
+
+		$this->manager->markPasswordInvalid($token, 'tokenId');
+	}
+
+	public function testMarkPasswordInvalidInvalidToken() {
+		$this->expectException(InvalidTokenException::class);
+
+		$this->manager->markPasswordInvalid($this->createMock(IToken::class), 'tokenId');
+	}
+
+	public function testUpdatePasswords() {
+		$this->defaultTokenProvider->expects($this->once())
+			->method('updatePasswords')
+			->with('uid', 'pass');
+		$this->publicKeyTokenProvider->expects($this->once())
+			->method('updatePasswords')
+			->with('uid', 'pass');
+
+		$this->manager->updatePasswords('uid', 'pass');
+	}
 }

tests/lib/Authentication/Token/PublicKeyTokenMapperTest.php

@@ -99,6 +99,20 @@ class PublicKeyTokenMapperTest extends TestCase {
 			'private_key' => $qb->createNamedParameter('private key'),
 			'version' => $qb->createNamedParameter(2),
 		])->execute();
+		$qb->insert('authtoken')->values([
+			'uid' => $qb->createNamedParameter('user3'),
+			'login_name' => $qb->createNamedParameter('User3'),
+			'password' => $qb->createNamedParameter('063de945d6f6b26862d9b6f40652f2d5|DZ/z520tfdXPtd0T|395f6b89be8d9d605e409e20b9d9abe477fde1be38a3223f9e508f979bf906e50d9eaa4dca983ca4fb22a241eb696c3f98654e7775f78c4caf13108f98642b53'),
+			'name' => $qb->createNamedParameter('Iceweasel on Linux'),
+			'token' => $qb->createNamedParameter('6d9a290d239d09f2cc33a03cc54cccd46f7dc71630dcc27d39214824bd3e093f1feb4e2b55eb159d204caa15dee9556c202a5aa0b9d67806c3f4ec2cde11af67'),
+			'type' => $qb->createNamedParameter(IToken::TEMPORARY_TOKEN),
+			'last_activity' => $qb->createNamedParameter($this->time - 120, IQueryBuilder::PARAM_INT), // Two minutes ago
+			'last_check' => $this->time - 60 * 10, // 10mins ago
+			'public_key' => $qb->createNamedParameter('public key'),
+			'private_key' => $qb->createNamedParameter('private key'),
+			'version' => $qb->createNamedParameter(2),
+			'password_invalid' => $qb->createNamedParameter(1),
+		])->execute();
 	}
 
 	private function getNumberOfTokens() {
@@ -115,7 +129,7 @@ class PublicKeyTokenMapperTest extends TestCase {
 
 		$this->mapper->invalidate($token);
 
-		$this->assertSame(2, $this->getNumberOfTokens());
+		$this->assertSame(3, $this->getNumberOfTokens());
 	}
 
 	public function testInvalidateInvalid() {
@@ -123,7 +137,7 @@ class PublicKeyTokenMapperTest extends TestCase {
 
 		$this->mapper->invalidate($token);
 
-		$this->assertSame(3, $this->getNumberOfTokens());
+		$this->assertSame(4, $this->getNumberOfTokens());
 	}
 
 	public function testInvalidateOld() {
@@ -131,7 +145,7 @@ class PublicKeyTokenMapperTest extends TestCase {
 
 		$this->mapper->invalidateOld($olderThan);
 
-		$this->assertSame(2, $this->getNumberOfTokens());
+		$this->assertSame(3, $this->getNumberOfTokens());
 	}
 
 	public function testGetToken() {
@@ -224,7 +238,7 @@ class PublicKeyTokenMapperTest extends TestCase {
 		$id = $result->fetch()['id'];
 
 		$this->mapper->deleteById('user1', (int)$id);
-		$this->assertEquals(2, $this->getNumberOfTokens());
+		$this->assertEquals(3, $this->getNumberOfTokens());
 	}
 
 	public function testDeleteByIdWrongUser() {
@@ -233,7 +247,7 @@ class PublicKeyTokenMapperTest extends TestCase {
 		$id = 33;
 
 		$this->mapper->deleteById('user1000', $id);
-		$this->assertEquals(3, $this->getNumberOfTokens());
+		$this->assertEquals(4, $this->getNumberOfTokens());
 	}
 
 	public function testDeleteByName() {
@@ -244,7 +258,12 @@ class PublicKeyTokenMapperTest extends TestCase {
 		$result = $qb->execute();
 		$name = $result->fetch()['name'];
 		$this->mapper->deleteByName($name);
-		$this->assertEquals(2, $this->getNumberOfTokens());
+		$this->assertEquals(3, $this->getNumberOfTokens());
+	}
+
+	public function testHasExpiredTokens() {
+		$this->assertFalse($this->mapper->hasExpiredTokens('user1'));
+		$this->assertTrue($this->mapper->hasExpiredTokens('user3'));
 	}
 
 }

tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php

@@ -504,4 +504,76 @@ class PublicKeyTokenProviderTest extends TestCase {
 		$this->assertSame(IToken::REMEMBER, $newToken->getRemember());
 		$this->assertSame(IToken::PERMANENT_TOKEN, $newToken->getType());
 	}
+
+	public function testMarkPasswordInvalidInvalidToken() {
+		$token = $this->createMock(DefaultToken::class);
+
+		$this->expectException(InvalidTokenException::class);
+
+		$this->tokenProvider->markPasswordInvalid($token, 'tokenId');
+	}
+
+	public function testMarkPasswordInvalid() {
+		$token = $this->createMock(PublicKeyToken::class);
+
+		$token->expects($this->once())
+			->method('setPasswordInvalid')
+			->with(true);
+		$this->mapper->expects($this->once())
+			->method('update')
+			->with($token);
+
+		$this->tokenProvider->markPasswordInvalid($token, 'tokenId');
+	}
+
+	public function testUpdatePasswords() {
+		$uid = 'myUID';
+		$token1 = $this->tokenProvider->generateToken(
+			'foo',
+			$uid,
+			$uid,
+			'bar',
+			'random1',
+			IToken::PERMANENT_TOKEN,
+			IToken::REMEMBER);
+		$token2 = $this->tokenProvider->generateToken(
+			'foobar',
+			$uid,
+			$uid,
+			'bar',
+			'random2',
+			IToken::PERMANENT_TOKEN,
+			IToken::REMEMBER);
+
+		$this->mapper->expects($this->once())
+			->method('hasExpiredTokens')
+			->with($uid)
+			->willReturn(true);
+		$this->mapper->expects($this->once())
+			->method('getTokenByUser')
+			->with($uid)
+			->willReturn([$token1, $token2]);
+		$this->mapper->expects($this->exactly(2))
+			->method('update')
+			->with($this->callback(function (PublicKeyToken $t) use ($token1, $token2) {
+				return $t === $token1 || $t === $token2;
+			}));
+
+		$this->tokenProvider->updatePasswords($uid, 'bar2');
+	}
+
+	public function testUpdatePasswordsNotRequired() {
+		$uid = 'myUID';
+
+		$this->mapper->expects($this->once())
+			->method('hasExpiredTokens')
+			->with($uid)
+			->willReturn(false);
+		$this->mapper->expects($this->never())
+			->method('getTokenByUser');
+		$this->mapper->expects($this->never())
+			->method('update');
+
+		$this->tokenProvider->updatePasswords($uid, 'bar2');
+	}
 }

tests/lib/User/SessionTest.php

@@ -1017,10 +1017,8 @@ class SessionTest extends \Test\TestCase {
 			->method('getPassword')
 			->with($token, 'APP-PASSWORD')
 			->will($this->returnValue('123456'));
-		$userManager->expects($this->once())
-			->method('checkPassword')
-			->with('susan', '123456')
-			->will($this->returnValue(true));
+		$userManager->expects($this->never())
+			->method('checkPassword');
 		$user->expects($this->once())
 			->method('isEnabled')
 			->will($this->returnValue(false));
@@ -1069,6 +1067,55 @@ class SessionTest extends \Test\TestCase {
 		$this->assertEquals(1000, $token->getLastCheck());
 	}
 
+	public function testValidateSessionInvalidPassword() {
+		$userManager = $this->createMock(Manager::class);
+		$session = $this->createMock(ISession::class);
+		$timeFactory = $this->createMock(ITimeFactory::class);
+		$tokenProvider = $this->createMock(IProvider::class);
+		$userSession = $this->getMockBuilder(Session::class)
+			->setConstructorArgs([$userManager, $session, $timeFactory, $tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger])
+			->setMethods(['logout'])
+			->getMock();
+
+		$user = $this->createMock(IUser::class);
+		$token = new \OC\Authentication\Token\DefaultToken();
+		$token->setLoginName('susan');
+		$token->setLastCheck(20);
+
+		$session->expects($this->once())
+			->method('get')
+			->with('app_password')
+			->will($this->returnValue('APP-PASSWORD'));
+		$tokenProvider->expects($this->once())
+			->method('getToken')
+			->with('APP-PASSWORD')
+			->will($this->returnValue($token));
+		$timeFactory->expects($this->once())
+			->method('getTime')
+			->will($this->returnValue(1000)); // more than 5min since last check
+		$tokenProvider->expects($this->once())
+			->method('getPassword')
+			->with($token, 'APP-PASSWORD')
+			->will($this->returnValue('123456'));
+		$userManager->expects($this->once())
+			->method('checkPassword')
+			->with('susan', '123456')
+			->willReturn(false);
+		$user->expects($this->once())
+			->method('isEnabled')
+			->will($this->returnValue(true));
+		$tokenProvider->expects($this->never())
+			->method('invalidateToken');
+		$tokenProvider->expects($this->once())
+			->method('markPasswordInvalid')
+			->with($token, 'APP-PASSWORD');
+		$userSession->expects($this->once())
+			->method('logout');
+
+		$userSession->setUser($user);
+		$this->invokePrivate($userSession, 'validateSession');
+	}
+
 	public function testUpdateSessionTokenPassword() {
 		$userManager = $this->createMock(Manager::class);
 		$session = $this->createMock(ISession::class);
@@ -1364,4 +1411,12 @@ class SessionTest extends \Test\TestCase {
 
 		$this->assertFalse($userSession->tryBasicAuthLogin($request, $this->throttler));
 	}
+
+	public function testUpdateTokens() {
+		$this->tokenProvider->expects($this->once())
+			->method('updatePasswords')
+			->with('uid', 'pass');
+
+		$this->userSession->updateTokens('uid', 'pass');
+	}
 }

version.php

@@ -29,7 +29,7 @@
 // between betas, final and RCs. This is _not_ the public version number. Reset minor/patchlevel
 // when updating major/minor version number.
 
-$OC_Version = array(15, 0, 0, 0);
+$OC_Version = array(15, 0, 0, 1);
 
 // The human readable string
 $OC_VersionString = '15.0.0 alpha';