Merge pull request #22791 from owncloud/enc_master_key_improvements

Enc master key improvements

apps/encryption/appinfo/application.php

@@ -66,6 +66,11 @@ class Application extends \OCP\AppFramework\App {
 			$session = $this->getContainer()->query('Session');
 			$session->setStatus(Session::RUN_MIGRATION);
 		}
+		if ($this->encryptionManager->isEnabled() && $encryptionSystemReady) {
+			/** @var Setup $setup */
+			$setup = $this->getContainer()->query('UserSetup');
+			$setup->setupSystem();
+		}
 	}
 
 	/**

apps/encryption/hooks/userhooks.php

@@ -118,22 +118,29 @@ class UserHooks implements IHook {
 	public function addHooks() {
 		OCUtil::connectHook('OC_User', 'post_login', $this, 'login');
 		OCUtil::connectHook('OC_User', 'logout', $this, 'logout');
-		OCUtil::connectHook('OC_User',
-			'post_setPassword',
-			$this,
-			'setPassphrase');
-		OCUtil::connectHook('OC_User',
-			'pre_setPassword',
-			$this,
-			'preSetPassphrase');
-		OCUtil::connectHook('OC_User',
-			'post_createUser',
-			$this,
-			'postCreateUser');
-		OCUtil::connectHook('OC_User',
-			'post_deleteUser',
-			$this,
-			'postDeleteUser');
+
+		// this hooks only make sense if no master key is used
+		if ($this->util->isMasterKeyEnabled() === false) {
+			OCUtil::connectHook('OC_User',
+				'post_setPassword',
+				$this,
+				'setPassphrase');
+
+			OCUtil::connectHook('OC_User',
+				'pre_setPassword',
+				$this,
+				'preSetPassphrase');
+
+			OCUtil::connectHook('OC_User',
+				'post_createUser',
+				$this,
+				'postCreateUser');
+
+			OCUtil::connectHook('OC_User',
+				'post_deleteUser',
+				$this,
+				'postDeleteUser');
+		}
 	}
 
 
@@ -152,12 +159,10 @@ class UserHooks implements IHook {
 
 		// ensure filesystem is loaded
 		if (!\OC\Files\Filesystem::$loaded) {
-			\OC_Util::setupFS($params['uid']);
+			$this->setupFS($params['uid']);
 		}
-
-		// setup user, if user not ready force relogin
-		if (!$this->userSetup->setupUser($params['uid'], $params['password'])) {
-			return false;
+		if ($this->util->isMasterKeyEnabled() === false) {
+			$this->userSetup->setupUser($params['uid'], $params['password']);
 		}
 
 		$this->keyManager->init($params['uid'], $params['password']);
@@ -302,7 +307,16 @@ class UserHooks implements IHook {
 	public function postPasswordReset($params) {
 		$password = $params['password'];
 
-		$this->keyManager->replaceUserKeys($params['uid']);
-		$this->userSetup->setupServerSide($params['uid'], $password);
+		$this->keyManager->deleteUserKeys($params['uid']);
+		$this->userSetup->setupUser($params['uid'], $password);
+	}
+
+	/**
+	 * setup file system for user
+	 *
+	 * @param string $uid user id
+	 */
+	protected function setupFS($uid) {
+		\OC_Util::setupFS($uid);
 	}
 }

apps/encryption/lib/keymanager.php

@@ -174,6 +174,11 @@ class KeyManager {
 	 * check if a key pair for the master key exists, if not we create one
 	 */
 	public function validateMasterKey() {
+
+		if ($this->util->isMasterKeyEnabled() === false) {
+			return;
+		}
+
 		$masterKey = $this->getPublicMasterKey();
 		if (empty($masterKey)) {
 			$keyPair = $this->crypt->createKeyPair();
@@ -334,7 +339,7 @@ class KeyManager {
 	/**
 	 * Decrypt private key and store it
 	 *
-	 * @param string $uid userid
+	 * @param string $uid user id
 	 * @param string $passPhrase users password
 	 * @return boolean
 	 */
@@ -342,7 +347,6 @@ class KeyManager {
 
 		$this->session->setStatus(Session::INIT_EXECUTED);
 
-
 		try {
 			if($this->util->isMasterKeyEnabled()) {
 				$uid = $this->getMasterKeyId();
@@ -554,9 +558,11 @@ class KeyManager {
 	}
 
 	/**
+	 * creat a backup of the users private and public key and then  delete it
+	 *
 	 * @param string $uid
 	 */
-	public function replaceUserKeys($uid) {
+	public function deleteUserKeys($uid) {
 		$this->backupAllKeys('password_reset');
 		$this->deletePublicKey($uid);
 		$this->deletePrivateKey($uid);

apps/encryption/lib/users/setup.php

@@ -66,29 +66,23 @@ class Setup {
  	}
 
 	/**
-	 * @param string $uid userid
+	 * @param string $uid user id
 	 * @param string $password user password
 	 * @return bool
 	 */
 	public function setupUser($uid, $password) {
-		return $this->setupServerSide($uid, $password);
-	}
-
-	/**
-	 * check if user has a key pair, if not we create one
-	 *
-	 * @param string $uid userid
-	 * @param string $password user password
-	 * @return bool
-	 */
-	public function setupServerSide($uid, $password) {
-		$this->keyManager->validateShareKey();
-		$this->keyManager->validateMasterKey();
-		// Check if user already has keys
 		if (!$this->keyManager->userHasKeys($uid)) {
 			return $this->keyManager->storeKeyPair($uid, $password,
 				$this->crypt->createKeyPair());
 		}
 		return true;
 	}
+
+	/**
+	 * make sure that all system keys exists
+	 */
+	public function setupSystem() {
+		$this->keyManager->validateShareKey();
+		$this->keyManager->validateMasterKey();
+	}
 }

apps/encryption/tests/hooks/UserHooksTest.php

@@ -77,7 +77,7 @@ class UserHooksTest extends TestCase {
 	private $params = ['uid' => 'testUser', 'password' => 'password'];
 
 	public function testLogin() {
-		$this->userSetupMock->expects($this->exactly(2))
+		$this->userSetupMock->expects($this->once())
 			->method('setupUser')
 			->willReturnOnConsecutiveCalls(true, false);
 
@@ -86,7 +86,6 @@ class UserHooksTest extends TestCase {
 			->with('testUser', 'password');
 
 		$this->assertNull($this->instance->login($this->params));
-		$this->assertFalse($this->instance->login($this->params));
 	}
 
 	public function testLogout() {
@@ -279,11 +278,11 @@ class UserHooksTest extends TestCase {
 
 	public function testPostPasswordReset() {
 		$this->keyManagerMock->expects($this->once())
-			->method('replaceUserKeys')
+			->method('deleteUserKeys')
 			->with('testUser');
 
 		$this->userSetupMock->expects($this->once())
-			->method('setupServerSide')
+			->method('setupUser')
 			->with('testUser', 'password');
 
 		$this->assertNull($this->instance->postPasswordReset($this->params));
@@ -339,16 +338,22 @@ class UserHooksTest extends TestCase {
 		$this->sessionMock = $sessionMock;
 		$this->recoveryMock = $recoveryMock;
 		$this->utilMock = $utilMock;
-		$this->instance = new UserHooks($this->keyManagerMock,
-			$this->userManagerMock,
-			$this->loggerMock,
-			$this->userSetupMock,
-			$this->userSessionMock,
-			$this->utilMock,
-			$this->sessionMock,
-			$this->cryptMock,
-			$this->recoveryMock
-		);
+		$this->utilMock->expects($this->any())->method('isMasterKeyEnabled')->willReturn(false);
+
+		$this->instance = $this->getMockBuilder('OCA\Encryption\Hooks\UserHooks')
+			->setConstructorArgs(
+				[
+					$this->keyManagerMock,
+					$this->userManagerMock,
+					$this->loggerMock,
+					$this->userSetupMock,
+					$this->userSessionMock,
+					$this->utilMock,
+					$this->sessionMock,
+					$this->cryptMock,
+					$this->recoveryMock
+				]
+			)->setMethods(['setupFS'])->getMock();
 
 	}
 

apps/encryption/tests/lib/users/SetupTest.php

@@ -41,26 +41,6 @@ class SetupTest extends TestCase {
 	 */
 	private $instance;
 
-	public function testSetupServerSide() {
-		$this->keyManagerMock->expects($this->exactly(2))->method('validateShareKey');
-		$this->keyManagerMock->expects($this->exactly(2))->method('validateMasterKey');
-		$this->keyManagerMock->expects($this->exactly(2))
-			->method('userHasKeys')
-			->with('admin')
-			->willReturnOnConsecutiveCalls(true, false);
-
-		$this->assertTrue($this->instance->setupServerSide('admin',
-			'password'));
-
-		$this->keyManagerMock->expects($this->once())
-			->method('storeKeyPair')
-			->with('admin', 'password')
-			->willReturn(false);
-
-		$this->assertFalse($this->instance->setupServerSide('admin',
-			'password'));
-	}
-
 	protected function setUp() {
 		parent::setUp();
 		$logMock = $this->getMock('OCP\ILogger');
@@ -81,4 +61,43 @@ class SetupTest extends TestCase {
 			$this->keyManagerMock);
 	}
 
+
+	public function testSetupSystem() {
+		$this->keyManagerMock->expects($this->once())->method('validateShareKey');
+		$this->keyManagerMock->expects($this->once())->method('validateMasterKey');
+
+		$this->instance->setupSystem();
+	}
+
+	/**
+	 * @dataProvider dataTestSetupUser
+	 *
+	 * @param bool $hasKeys
+	 * @param bool $expected
+	 */
+	public function testSetupUser($hasKeys, $expected) {
+
+		$this->keyManagerMock->expects($this->once())->method('userHasKeys')
+			->with('uid')->willReturn($hasKeys);
+
+		if ($hasKeys) {
+			$this->keyManagerMock->expects($this->never())->method('storeKeyPair');
+		} else {
+			$this->cryptMock->expects($this->once())->method('createKeyPair')->willReturn('keyPair');
+			$this->keyManagerMock->expects($this->once())->method('storeKeyPair')
+				->with('uid', 'password', 'keyPair')->willReturn(true);
+		}
+
+		$this->assertSame($expected,
+			$this->instance->setupUser('uid', 'password')
+		);
+	}
+
+	public function dataTestSetupUser() {
+		return [
+			[true, true],
+			[false, true]
+		];
+	}
+
 }

tests/lib/traits/encryptiontrait.php

@@ -63,7 +63,7 @@ trait EncryptionTrait {
 		$keyManager = $container->query('KeyManager');
 		/** @var Setup $userSetup */
 		$userSetup = $container->query('UserSetup');
-		$userSetup->setupServerSide($name, $password);
+		$userSetup->setupUser($name, $password);
 		$keyManager->init($name, $password);
 	}