Merge pull request #16337 from owncloud/versions-expireforowner

Use owner when expiring versions, not the logged in user

apps/files_versions/lib/storage.php

@@ -158,7 +158,7 @@ class Storage {
 			// 1.5 times as large as the current version -> 2.5
 			$neededSpace = $files_view->filesize($filename) * 2.5;
 
-			self::scheduleExpire($filename, $versionsSize, $neededSpace);
+			self::scheduleExpire($uid, $filename, $versionsSize, $neededSpace);
 
 			// store a new version of a file
 			$mtime = $users_view->filemtime('files/' . $filename);
@@ -276,7 +276,7 @@ class Storage {
 
 		// if we moved versions directly for a file, schedule expiration check for that file
 		if (!$rootView->is_dir('/' . $targetOwner . '/files/' . $targetPath)) {
-			self::scheduleExpire($targetPath);
+			self::scheduleExpire($targetOwner, $targetPath);
 		}
 
 	}
@@ -309,7 +309,7 @@ class Storage {
 			// rollback
 			if (self::copyFileContents($users_view, 'files_versions' . $filename . '.v' . $revision, 'files' . $filename)) {
 				$files_view->touch($file, $revision);
-				Storage::scheduleExpire($file);
+				Storage::scheduleExpire($uid, $file);
 				return true;
 			} else if ($versionCreated) {
 				self::deleteVersion($users_view, $version);
@@ -532,12 +532,15 @@ class Storage {
 	}
 
 	/**
-	 * @param string $fileName
-	 * @param int|null $versionsSize
-	 * @param int $neededSpace
+	 * Schedule versions expiration for the given file
+	 *
+	 * @param string $uid owner of the file
+	 * @param string $fileName file/folder for which to schedule expiration
+	 * @param int|null $versionsSize current versions size
+	 * @param int $neededSpace requested versions size
 	 */
-	private static function scheduleExpire($fileName, $versionsSize = null, $neededSpace = 0) {
-		$command = new Expire(\OC::$server->getUserSession()->getUser()->getUID(), $fileName, $versionsSize, $neededSpace);
+	private static function scheduleExpire($uid, $fileName, $versionsSize = null, $neededSpace = 0) {
+		$command = new Expire($uid, $fileName, $versionsSize, $neededSpace);
 		\OC::$server->getCommandBus()->push($command);
 	}
 

apps/files_versions/tests/versions.php

@@ -45,10 +45,6 @@ class Test_Files_Versioning extends \Test\TestCase {
 	public static function setUpBeforeClass() {
 		parent::setUpBeforeClass();
 
-		// clear share hooks
-		\OC_Hook::clear('OCP\\Share');
-		\OC::registerShareHooks();
-		\OCA\Files_Versions\Hooks::connectHooks();
 		$application = new \OCA\Files_Sharing\AppInfo\Application();
 		$application->registerMountProviders();
 		$application->setupPropagation();
@@ -69,6 +65,11 @@ class Test_Files_Versioning extends \Test\TestCase {
 	protected function setUp() {
 		parent::setUp();
 
+		// clear hooks
+		\OC_Hook::clear();
+		\OC::registerShareHooks();
+		\OCA\Files_Versions\Hooks::connectHooks();
+
 		self::loginHelper(self::TEST_VERSIONS_USER);
 		$this->rootView = new \OC\Files\View();
 		if (!$this->rootView->file_exists(self::USERS_VERSIONS_ROOT)) {
@@ -82,6 +83,8 @@ class Test_Files_Versioning extends \Test\TestCase {
 		$this->rootView->deleteAll(self::TEST_VERSIONS_USER . '/files_versions/');
 		$this->rootView->deleteAll(self::TEST_VERSIONS_USER2 . '/files_versions/');
 
+		\OC_Hook::clear();
+
 		parent::tearDown();
 	}
 
@@ -647,6 +650,85 @@ class Test_Files_Versioning extends \Test\TestCase {
 		);
 	}
 
+	/**
+	 * Test whether versions are created when overwriting as owner
+	 */
+	public function testStoreVersionAsOwner() {
+		$this->loginAsUser(self::TEST_VERSIONS_USER);
+
+		$this->createAndCheckVersions(
+			\OC\Files\Filesystem::getView(),
+			'test.txt'
+		);
+	}
+
+	/**
+	 * Test whether versions are created when overwriting as share recipient
+	 */
+	public function testStoreVersionAsRecipient() {
+		$this->loginAsUser(self::TEST_VERSIONS_USER);
+
+		\OC\Files\Filesystem::mkdir('folder');
+		\OC\Files\Filesystem::file_put_contents('folder/test.txt', 'test file');
+		$fileInfo = \OC\Files\Filesystem::getFileInfo('folder');
+
+		\OCP\Share::shareItem(
+			'folder',
+			$fileInfo['fileid'],
+			\OCP\Share::SHARE_TYPE_USER,
+			self::TEST_VERSIONS_USER2,
+			\OCP\Constants::PERMISSION_ALL
+		);
+
+		$this->loginAsUser(self::TEST_VERSIONS_USER2);
+
+		$this->createAndCheckVersions(
+			\OC\Files\Filesystem::getView(),
+			'folder/test.txt'
+		);
+	}
+
+	/**
+	 * Test whether versions are created when overwriting anonymously.
+	 *
+	 * When uploading through a public link or publicwebdav, no user
+	 * is logged in. File modification must still be able to find
+	 * the owner and create versions.
+	 */
+	public function testStoreVersionAsAnonymous() {
+		$this->logout();
+
+		// note: public link upload does this,
+		// needed to make the hooks fire
+		\OC_Util::setupFS(self::TEST_VERSIONS_USER);
+
+		$userView = new \OC\Files\View('/' . self::TEST_VERSIONS_USER . '/files');
+		$this->createAndCheckVersions(
+			$userView,
+			'test.txt'
+		);
+	}
+
+	private function createAndCheckVersions($view, $path) {
+		$view->file_put_contents($path, 'test file');
+		$view->file_put_contents($path, 'version 1');
+		$view->file_put_contents($path, 'version 2');
+
+		$this->loginAsUser(self::TEST_VERSIONS_USER);
+
+		// need to scan for the versions
+		list($rootStorage,) = $this->rootView->resolvePath(self::TEST_VERSIONS_USER . '/files_versions');
+		$rootStorage->getScanner()->scan('files_versions');
+
+		$versions = \OCA\Files_Versions\Storage::getVersions(
+			self::TEST_VERSIONS_USER, '/' . $path
+		);
+
+		// note: we cannot predict how many versions are created due to
+		// test run timing
+		$this->assertGreaterThan(0, count($versions));
+	}
+
 	/**
 	 * @param string $user
 	 * @param bool $create

tests/lib/testcase.php

@@ -182,6 +182,8 @@ abstract class TestCase extends \PHPUnit_Framework_TestCase {
 	static protected function logout() {
 		\OC_Util::tearDownFS();
 		\OC_User::setUserId('');
+		// needed for fully logout
+		\OC::$server->getUserSession()->setUser(null);
 	}
 
 	/**