Merge pull request #13204 from owncloud/enc_fix_key_lost

Encryption fix key lost if group share gets renamed

apps/files_encryption/lib/hooks.php

@@ -428,8 +428,10 @@ class Hooks {
 
 		// we only need to rename the keys if the rename happens on the same mountpoint
 		// otherwise we perform a stream copy, so we get a new set of keys
-		$mp1 = $view->getMountPoint('/' . $user . '/files/' . $params['oldpath']);
+		$oldPath = \OC\Files\Filesystem::normalizePath('/' . $user . '/files/' . $params['oldpath']);
-		$mp2 = $view->getMountPoint('/' . $user . '/files/' . $params['newpath']);
+		$newPath = \OC\Files\Filesystem::normalizePath('/' . $user . '/files/' . $params['newpath']);
+		$mp1 = $view->getMountPoint($oldPath);
+		$mp2 = $view->getMountPoint($newPath);
 
 		$oldKeysPath = Keymanager::getKeyPath($view, $util, $params['oldpath']);
 
@@ -438,7 +440,7 @@ class Hooks {
 				'operation' => $operation,
 				'oldKeysPath' => $oldKeysPath,
 				);
-		} else {
+		} elseif ($mp1 !== $oldPath . '/') {
 			self::$renamedFiles[$params['oldpath']] = array(
 				'operation' => 'cleanup',
 				'oldKeysPath' => $oldKeysPath,

apps/files_encryption/tests/share.php

@@ -1032,7 +1032,7 @@ class Share extends TestCase {
 
 
 	/**
-	 * test moving a shared file out of the Shared folder
+	 * test rename a shared file mount point
 	 */
 	function testRename() {
 
@@ -1055,7 +1055,10 @@ class Share extends TestCase {
 		// share the file
 		\OCP\Share::shareItem('file', $fileInfo['fileid'], \OCP\Share::SHARE_TYPE_USER, self::TEST_ENCRYPTION_SHARE_USER2, \OCP\Constants::PERMISSION_ALL);
 
-		// check if share key for user2 exists
+		// check if share key for user1 and user2 exists
+		$this->assertTrue($this->view->file_exists(
+				'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files_encryption/keys/'
+				. $this->filename . '/' . self::TEST_ENCRYPTION_SHARE_USER1 . '.shareKey'));
 		$this->assertTrue($this->view->file_exists(
 			'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files_encryption/keys/'
 			. $this->filename . '/' . self::TEST_ENCRYPTION_SHARE_USER2 . '.shareKey'));
@@ -1073,9 +1076,10 @@ class Share extends TestCase {
 		// check if data is the same as we previously written
 		$this->assertEquals($this->dataShort, $retrievedCryptedFile);
 
+		\OC\Files\Filesystem::mkdir($this->folder1);
+
 		// move the file to a subfolder
-		$this->view->rename('/' . self::TEST_ENCRYPTION_SHARE_USER2 . '/files/' . $this->filename,
+		\OC\Files\Filesystem::rename($this->filename, $this->folder1 . $this->filename);
-			'/' . self::TEST_ENCRYPTION_SHARE_USER2 . '/files/' . $this->folder1 . $this->filename);
 
 		// check if we can read the moved file
 		$retrievedRenamedFile = $this->view->file_get_contents(
@@ -1084,11 +1088,89 @@ class Share extends TestCase {
 		// check if data is the same as we previously written
 		$this->assertEquals($this->dataShort, $retrievedRenamedFile);
 
+		// check if share key for user2 and user1 still exists
+		$this->assertTrue($this->view->file_exists(
+				'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files_encryption/keys/'
+				. $this->filename . '/' . self::TEST_ENCRYPTION_SHARE_USER1 . '.shareKey'));
+		$this->assertTrue($this->view->file_exists(
+				'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files_encryption/keys/'
+				. $this->filename . '/' . self::TEST_ENCRYPTION_SHARE_USER2 . '.shareKey'));
+
 		// cleanup
 		self::loginHelper(self::TEST_ENCRYPTION_SHARE_USER1);
 		$this->view->unlink('/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files/' . $this->filename);
 	}
 
+	function testRenameGroupShare() {
+		// login as admin
+		self::loginHelper(self::TEST_ENCRYPTION_SHARE_USER1);
+
+		// save file with content
+		$cryptedFile = file_put_contents('crypt:///' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files/' . $this->filename, $this->dataShort);
+
+		// test that data was successfully written
+		$this->assertTrue(is_int($cryptedFile));
+
+		// get the file info from previous created file
+		$fileInfo = $this->view->getFileInfo(
+			'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files/' . $this->filename);
+
+		// check if we have a valid file info
+		$this->assertTrue($fileInfo instanceof \OC\Files\FileInfo);
+
+		// share the file
+		\OCP\Share::shareItem('file', $fileInfo['fileid'], \OCP\Share::SHARE_TYPE_GROUP, self::TEST_ENCRYPTION_SHARE_GROUP1, \OCP\Constants::PERMISSION_ALL);
+
+		// check if share key for user1, user3 and user4 exists
+		$this->assertTrue($this->view->file_exists(
+				'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files_encryption/keys/'
+				. $this->filename . '/' . self::TEST_ENCRYPTION_SHARE_USER1 . '.shareKey'));
+		$this->assertTrue($this->view->file_exists(
+				'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files_encryption/keys/'
+				. $this->filename . '/' . self::TEST_ENCRYPTION_SHARE_USER3 . '.shareKey'));
+		$this->assertTrue($this->view->file_exists(
+				'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files_encryption/keys/'
+				. $this->filename . '/' . self::TEST_ENCRYPTION_SHARE_USER4 . '.shareKey'));
+
+
+		// login as user2
+		self::loginHelper(self::TEST_ENCRYPTION_SHARE_USER3);
+
+		$this->assertTrue(\OC\Files\Filesystem::file_exists($this->filename));
+
+		// get file contents
+		$retrievedCryptedFile = \OC\Files\Filesystem::file_get_contents($this->filename);
+
+		// check if data is the same as we previously written
+		$this->assertEquals($this->dataShort, $retrievedCryptedFile);
+
+		\OC\Files\Filesystem::mkdir($this->folder1);
+
+		// move the file to a subfolder
+		\OC\Files\Filesystem::rename($this->filename, $this->folder1 . $this->filename);
+
+		// check if we can read the moved file
+		$retrievedRenamedFile = \OC\Files\Filesystem::file_get_contents($this->folder1 . $this->filename);
+
+		// check if data is the same as we previously written
+		$this->assertEquals($this->dataShort, $retrievedRenamedFile);
+
+		// check if share key for user1, user3 and user4 still exists
+		$this->assertTrue($this->view->file_exists(
+				'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files_encryption/keys/'
+				. $this->filename . '/' . self::TEST_ENCRYPTION_SHARE_USER1 . '.shareKey'));
+		$this->assertTrue($this->view->file_exists(
+				'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files_encryption/keys/'
+				. $this->filename . '/' . self::TEST_ENCRYPTION_SHARE_USER3 . '.shareKey'));
+		$this->assertTrue($this->view->file_exists(
+				'/' . self::TEST_ENCRYPTION_SHARE_USER1 . '/files_encryption/keys/'
+				. $this->filename . '/' . self::TEST_ENCRYPTION_SHARE_USER4 . '.shareKey'));
+
+		// cleanup
+		self::loginHelper(self::TEST_ENCRYPTION_SHARE_USER1);
+		\OC\Files\Filesystem::unlink($this->filename);
+	}
+
 	/**
 	 * test if additional share keys are added if we move a folder to a shared parent
 	 * @medium