Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Copy file to owner's trash when recipient moves out of share #27042

Merged
merged 2 commits into from
Feb 21, 2017
Merged

Copy file to owner's trash when recipient moves out of share #27042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d70aa8
Copy file to owner's trash when recipient moves out of share
Jan 27, 2017
641d3dc
Register encryption storage wrapper in trashbin storage test
Feb 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions apps/files_trashbin/lib/Storage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,30 @@ public static function preRenameHook($params) {
// in cross-storage cases, a rename is a copy + unlink,
// that last unlink must not go to trash
self::$disableTrash = true;

$path1 = $params[Filesystem::signal_param_oldpath];
$path2 = $params[Filesystem::signal_param_newpath];

$view = Filesystem::getView();
$absolutePath1 = Filesystem::normalizePath($view->getAbsolutePath($path1));

$mount1 = $view->getMount($path1);
$mount2 = $view->getMount($path2);
$sourceStorage = $mount1->getStorage();
$targetStorage = $mount2->getStorage();
$sourceInternalPath = $mount1->getInternalPath($absolutePath1);
// check whether this is a cross-storage move from a *local* shared storage
if ($sourceInternalPath !== '' && $sourceStorage !== $targetStorage && $sourceStorage->instanceOfStorage('OCA\Files_Sharing\SharedStorage')) {
$ownerPath = $sourceStorage->getSourcePath($sourceInternalPath);
$owner = $sourceStorage->getOwner($sourceInternalPath);
if ($owner !== null && $owner !== '' && $ownerPath !== null && substr($ownerPath, 0, 6) === 'files/') {
// ownerPath is in the format "files/path/to/file.txt", strip "files"
$ownerPath = substr($ownerPath, 6);

// make a backup copy for the owner
\OCA\Files_Trashbin\Trashbin::copyBackupForOwner($ownerPath, $owner, time());
}
}
}

/**
Expand Down
54 changes: 43 additions & 11 deletions apps/files_trashbin/lib/Trashbin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,14 +183,42 @@ private static function copyFilesToUser($sourcePath, $owner, $targetPath, $user,
$target = $user . '/files_trashbin/files/' . $targetFilename . '.d' . $timestamp;
$source = $owner . '/files_trashbin/files/' . $sourceFilename . '.d' . $timestamp;
self::copy_recursive($source, $target, $view);
}

/**
* Make a backup of a file into the trashbin for the owner
*
* @param string $ownerPath path relative to the owner's home folder and containing "files"
* @param string $owner user id of the owner
* @param int $timestamp deletion timestamp
*/
public static function copyBackupForOwner($ownerPath, $owner, $timestamp) {
self::setUpTrash($owner);

$targetFilename = basename($ownerPath);
$targetLocation = dirname($ownerPath);
$source = $owner . '/files/' . ltrim($ownerPath, '/');
$target = $owner . '/files_trashbin/files/' . $targetFilename . '.d' . $timestamp;

$view = new View('/');
self::copy_recursive($source, $target, $view);

self::retainVersions($targetFilename, $owner, $ownerPath, $timestamp, true);

if ($view->file_exists($target)) {
$query = \OC_DB::prepare("INSERT INTO `*PREFIX*files_trash` (`id`,`timestamp`,`location`,`user`) VALUES (?,?,?,?)");
$result = $query->execute([$targetFilename, $timestamp, $targetLocation, $user]);
if (!$result) {
\OCP\Util::writeLog('files_trashbin', 'trash bin database couldn\'t be updated for the files owner', \OCP\Util::ERROR);
}
self::insertTrashEntry($owner, $targetFilename, $targetLocation, $timestamp);
self::scheduleExpire($owner);
}
}

/**
*
*/
public static function insertTrashEntry($user, $targetFilename, $targetLocation, $timestamp) {
$query = \OC_DB::prepare("INSERT INTO `*PREFIX*files_trash` (`id`,`timestamp`,`location`,`user`) VALUES (?,?,?,?)");
$result = $query->execute([$targetFilename, $timestamp, $targetLocation, $user]);
if (!$result) {
\OCP\Util::writeLog('files_trashbin', 'trash bin database couldn\'t be updated for the files owner', \OCP\Util::ERROR);
}
}

Expand Down Expand Up @@ -231,7 +259,6 @@ public static function move2trash($file_path) {
$location = $path_parts['dirname'];
$timestamp = time();

// disable proxy to prevent recursive calls
$trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp;

/** @var \OC\Files\Storage\Storage $trashStorage */
Expand Down Expand Up @@ -297,25 +324,30 @@ public static function move2trash($file_path) {
* @param string $owner owner user id
* @param string $ownerPath path relative to the owner's home storage
* @param integer $timestamp when the file was deleted
* @param bool $forceCopy true to only make a copy of the versions into the trashbin
*/
private static function retainVersions($filename, $owner, $ownerPath, $timestamp) {
private static function retainVersions($filename, $owner, $ownerPath, $timestamp, $forceCopy = false) {
if (\OCP\App::isEnabled('files_versions') && !empty($ownerPath)) {

$user = User::getUser();
$rootView = new View('/');

if ($rootView->is_dir($owner . '/files_versions/' . $ownerPath)) {
if ($owner !== $user) {
if ($owner !== $user || $forceCopy) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move these checks to another function? It might remove the duplication here and also simplify this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to not touch too much here and just inject what I need.

This would be a good task for future refactoring/rewrite of this app with proper tests.

self::copy_recursive($owner . '/files_versions/' . $ownerPath, $owner . '/files_trashbin/versions/' . basename($ownerPath) . '.d' . $timestamp, $rootView);
}
self::move($rootView, $owner . '/files_versions/' . $ownerPath, $user . '/files_trashbin/versions/' . $filename . '.d' . $timestamp);
if (!$forceCopy) {
self::move($rootView, $owner . '/files_versions/' . $ownerPath, $user . '/files_trashbin/versions/' . $filename . '.d' . $timestamp);
}
} else if ($versions = \OCA\Files_Versions\Storage::getVersions($owner, $ownerPath)) {

foreach ($versions as $v) {
if ($owner !== $user) {
if ($owner !== $user || $forceCopy) {
self::copy($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $owner . '/files_trashbin/versions/' . $v['name'] . '.v' . $v['version'] . '.d' . $timestamp);
}
self::move($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $user . '/files_trashbin/versions/' . $filename . '.v' . $v['version'] . '.d' . $timestamp);
if (!$forceCopy) {
self::move($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $user . '/files_trashbin/versions/' . $filename . '.v' . $v['version'] . '.d' . $timestamp);
}
}
}
}
Expand Down
80 changes: 80 additions & 0 deletions apps/files_trashbin/tests/StorageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,14 @@ protected function setUp() {
\OC_Hook::clear();
\OCA\Files_Trashbin\Trashbin::registerHooks();

// the encryption wrapper does some twisted stuff with moveFromStorage...
// we need to register it here so that the tested behavior is closer to reality
\OC::$server->getEncryptionManager()->setupStorage();

$this->user = $this->getUniqueId('user');
\OC::$server->getUserManager()->createUser($this->user, $this->user);


// this will setup the FS
$this->loginAsUser($this->user);

Expand All @@ -82,6 +87,7 @@ protected function tearDown() {
$user = \OC::$server->getUserManager()->get($this->user);
if ($user !== null) { $user->delete(); }
\OC_Hook::clear();
\OC\Files\Filesystem::getLoader()->removeStorageWrapper('oc_encryption');
parent::tearDown();
}

Expand Down Expand Up @@ -446,6 +452,80 @@ public function testKeepFileAndVersionsWhenMovingFolderBetweenStorages() {
$this->assertEquals(0, count($results));
}

/**
* Test that the owner receives a backup of the file that was moved
* out of the shared folder
*/
public function testOwnerBackupWhenMovingFileOutOfShare() {
\OCA\Files_Versions\Hooks::connectHooks();

$this->userView->mkdir('share');
$this->userView->mkdir('share/sub');
// trigger a version (multiple would not work because of the expire logic)
$this->userView->file_put_contents('share/test.txt', 'v1');
$this->userView->file_put_contents('share/test.txt', 'v2');

$this->userView->file_put_contents('share/sub/testsub.txt', 'v1');
$this->userView->file_put_contents('share/sub/testsub.txt', 'v2');

$results = $this->rootView->getDirectoryContent($this->user . '/files_versions/share/');
$this->assertEquals(2, count($results));
$results = $this->rootView->getDirectoryContent($this->user . '/files_versions/share/sub');
$this->assertEquals(1, count($results));

$recipientUser = $this->getUniqueId('recipient_');
$user2 = \OC::$server->getUserManager()->createUser($recipientUser, $recipientUser);

$node = \OC::$server->getUserFolder($this->user)->get('share');
$share = \OC::$server->getShareManager()->newShare();
$share->setNode($node)
->setShareType(\OCP\Share::SHARE_TYPE_USER)
->setSharedBy($this->user)
->setSharedWith($recipientUser)
->setPermissions(\OCP\Constants::PERMISSION_ALL);
\OC::$server->getShareManager()->createShare($share);

$this->loginAsUser($recipientUser);

// delete as recipient
$recipientHome = \OC::$server->getUserFolder($recipientUser);

// rename received share folder to catch potential issues if using the wrong name in the code
$recipientHome->get('share')->move($recipientHome->getPath() . '/share_renamed');
$recipientHome->get('share_renamed/test.txt')->move($recipientHome->getPath() . '/test.txt');
$recipientHome->get('share_renamed/sub')->move($recipientHome->getPath() . '/sub');

$this->assertTrue($recipientHome->nodeExists('test.txt'));
$this->assertTrue($recipientHome->nodeExists('sub'));

// check if file and versions are in trashbin for owner
$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files');
$this->assertEquals(2, count($results), 'Files in owner\'s trashbin');
// grab subdir name
$subDirName = $results[0]->getName();
$name = $results[1]->getName();
$this->assertEquals('test.txt.d', substr($name, 0, strlen('test.txt.d')));

$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions');
$this->assertEquals(2, count($results), 'Versions in owner\'s trashbin');
// note: entry 0 is the "sub" entry for versions
$name = $results[1]->getName();
$this->assertEquals('test.txt.v', substr($name, 0, strlen('test.txt.v')));

// check if sub-file and versions are in trashbin for owner
$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/' . $subDirName);
$this->assertEquals(1, count($results), 'Subfile in owner\'s trashbin');
$this->assertEquals('testsub.txt', $results[0]->getName());

$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions/' . $subDirName);
$this->assertEquals(1, count($results), 'Versions in owner\'s trashbin');
$name = $results[0]->getName();
$this->assertEquals('testsub.txt.v', substr($name, 0, strlen('testsub.txt.v')));

$this->logout();
$user2->delete();
}

/**
* Delete should fail if the source file can't be deleted.
*/
Expand Down
28 changes: 28 additions & 0 deletions build/integration/features/sharing-v1.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1133,3 +1133,31 @@ Feature: sharing
Then as "user1" the file "/shared/shared_file.txt" exists
And as "user0" the file "/shared/shared_file.txt" exists

Scenario: moving file out of a share as recipient creates a backup for the owner
Given As an "admin"
And user "user0" exists
And user "user1" exists
And user "user0" created a folder "/shared"
And User "user0" moved file "/textfile0.txt" to "/shared/shared_file.txt"
And file "/shared" of user "user0" is shared with user "user1"
And User "user1" moved folder "/shared" to "/shared_renamed"
When User "user1" moved file "/shared_renamed/shared_file.txt" to "/taken_out.txt"
Then as "user1" the file "/taken_out.txt" exists
And as "user0" the file "/shared/shared_file.txt" does not exist
And as "user0" the file "/shared_file.txt" exists in trash

Scenario: moving folder out of a share as recipient creates a backup for the owner
Given As an "admin"
And user "user0" exists
And user "user1" exists
And user "user0" created a folder "/shared"
And user "user0" created a folder "/shared/sub"
And User "user0" moved file "/textfile0.txt" to "/shared/sub/shared_file.txt"
And file "/shared" of user "user0" is shared with user "user1"
And User "user1" moved folder "/shared" to "/shared_renamed"
When User "user1" moved folder "/shared_renamed/sub" to "/taken_out"
Then as "user1" the file "/taken_out" exists
And as "user0" the folder "/shared/sub" does not exist
And as "user0" the folder "/sub" exists in trash
And as "user0" the file "/sub/shared_file.txt" exists in trash