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.

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

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

Merged
merged 2 commits into from
Feb 21, 2017

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Jan 27, 2017

Description

Whenever a share recipient moves files or folders out of the share,
make a backup copy in the owner's trash just in case.

The metadata stays on the recipient's copy that was moved out.

Related Issue

Fixes #24053 (without activity entry)

Motivation and Context

Solves the case where the recipient is not reachable any more so the owner still has a chance to restore the file.

How Has This Been Tested?

  1. Login as "user0" (owner)
  2. Create a folder "shared"
  3. Share "shared" with "user1" (recipient)
  4. Login as "user1"
  5. Create a file "shared/test.txt" and overwrite several times to create versions
  6. Create a folder "shared/sub"
  7. Create a file "shared/sub/testsub.txt" and overwrite several times to create versions
  8. Rename "shared" to "shared_renamed" (this covers the path matching logic)
  9. Move "sub" and "test.txt" out of the "shared_renamed" folder.
  10. Login as "owner"
  11. Check that trashbin contains both "sub/testsub.txt" and "test.txt"
  12. Restore both
  13. Check that both "test.txt" and "sub/testsub.txt" are restored in the "shared" folder (from the owner's perspective)
  14. Check that both entries still have their versions attached

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

TODOs:

  • find a way to bypass the locking issue or find another place / hook to do this operation...

@mention-bot
Copy link

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @stevenbuehner and @bartv2 to be potential reviewers.

@PVince81
Copy link
Contributor Author

PVince81 commented Jan 27, 2017

Currently when moving out "test/deletedk.txt" into the root as recipient:

{"reqId":"gpWkNw+rulBC4FrPhDt5","remoteAddr":"127.0.0.1","app":"webdav","message":"Exception: {\"Message\":\"HTTP\\\/1.1 423 \\\"test\\\/deletedk.txt\\\" is locked\",\"Exception\":\"OCA\\\\DAV\\\\Connector\\\\Sabre\\\\Exception\\\\FileLocked\",\"Code\":0,\"Trace\":\"
#0 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/CorePlugin.php(642): OCA\\\\DAV\\\\Connector\\\\Sabre\\\\ObjectTree->move('test\\\/deletedk.t...', 'deletedk.txt')\\n
#1 [internal function]: Sabre\\\\DAV\\\\CorePlugin->httpMove(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n
#2 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/event\\\/lib\\\/EventEmitterTrait.php(105): call_user_func_array(Array, Array)\\n
#3 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(479): Sabre\\\\Event\\\\EventEmitter->emit('method:MOVE', Array)\\n
#4 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(254): Sabre\\\\DAV\\\\Server->invokeMethod(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n
#5 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/appinfo\\\/v1\\\/webdav.php(58): Sabre\\\\DAV\\\\Server->exec()\\n
#6 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/remote.php(164): require_once('\\\/srv\\\/www\\\/htdocs...')\\n
#7 {main}\",\"File\":\"\\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/Connector\\\/Sabre\\\/ObjectTree.php\",\"Line\":271,\"User\":\"user1\"}","level":4,"time":"2017-01-27T11:38:59+00:00","method":"MOVE","url":"\/owncloud\/remote.php\/webdav\/test\/deletedk.txt","user":"user1"}

@PVince81
Copy link
Contributor Author

It looks like I won't be able to bypass the locking issue from this point of the code. The problem is that the View class already sets an exclusive lock earlier and it wouldn't be suitable to change that: https://github.com/owncloud/core/blob/v9.1.3/lib/private/Files/View.php#L767

So the only solution is to do the copy in question from within the pre rename hook, in which only a shared lock is applied.

@PVince81
Copy link
Contributor Author

PVince81 commented Jan 30, 2017

  • TODO: find out who has the versions in a "recipient deletes" scenario

@codecov-io
Copy link

codecov-io commented Jan 30, 2017

Codecov Report

❗ No coverage uploaded for pull request head (sharing-trashfileforownerwhenmovedout@47bdc5d).


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1e31f0...47bdc5d. Read the comment docs.

@PVince81 PVince81 force-pushed the sharing-trashfileforownerwhenmovedout branch from 327483c to 47bdc5d Compare January 31, 2017 18:18
@PVince81 PVince81 mentioned this pull request Feb 1, 2017
9 tasks
@PVince81
Copy link
Contributor Author

PVince81 commented Feb 1, 2017

@PVince81 PVince81 force-pushed the sharing-trashfileforownerwhenmovedout branch from 47bdc5d to 270c30d Compare February 1, 2017 11:08
@PVince81
Copy link
Contributor Author

PVince81 commented Feb 1, 2017

  • make sure fed sharing doesn't break in this scenario due to "getOwner"

@PVince81
Copy link
Contributor Author

PVince81 commented Feb 1, 2017

  • fix versions case ($ownerPath is wrong)
  • add unit tests + verify if versions were stored as well
  • integration test with versions ? (bootstrap missing...)

@PVince81
Copy link
Contributor Author

PVince81 commented Feb 2, 2017

  • test where the shared folder name is different, in case of potential bugs

@PVince81 PVince81 changed the title [WIP] Copy file to owner's trash when recipient moves out of share Copy file to owner's trash when recipient moves out of share Feb 2, 2017
@PVince81
Copy link
Contributor Author

PVince81 commented Feb 2, 2017

  • TODO: documentation ticket for 10.0

@PVince81
Copy link
Contributor Author

PVince81 commented Feb 2, 2017

It's looking good and also works already.

  • The integration test covers only detecting if the file is in trashbin.
  • Some existing tests from Add integration test for trashbin #27069 ensure that the favorite info is kept, which confirms file id preservation for the recipient. The backup gets a new fileid but no metadata except versions.
  • Versions are copied to owner's trash backup just like for delete. This is not covered by integration tests (we lack version code there...) but is covered by the unit tests

@PVince81 PVince81 force-pushed the sharing-trashfileforownerwhenmovedout branch 2 times, most recently from 0eea85f to 5c1238e Compare February 2, 2017 10:25
@PVince81
Copy link
Contributor Author

PVince81 commented Feb 2, 2017

This is now ready for review @jvillafanez @VicDeo @DeepDiver1975 @IljaN

@PVince81
Copy link
Contributor Author

PVince81 commented Feb 2, 2017

Tests pass for me locally 😢

@PVince81 PVince81 force-pushed the sharing-trashfileforownerwhenmovedout branch from 5c1238e to f3f1e3d Compare February 2, 2017 16:00
@PVince81
Copy link
Contributor Author

PVince81 commented Feb 2, 2017

Rebasing as a tentative of luck

@PVince81
Copy link
Contributor Author

Let's see... maybe I can just register an encryption wrapper in the tests... at least this will bring all tests closer to reality.

@PVince81
Copy link
Contributor Author

Here we go, the encryption wrapper fixes the test: e22d634

@jvillafanez
Copy link
Member

I don't see any other change that should be done, so as long as it's working 👍

@PVince81
Copy link
Contributor Author

So... now StorageTests pass but TrashbinTest fail again 😢

@PVince81
Copy link
Contributor Author

Failure on pgsql now, so much fun:

19:35:56 1) OCA\Files_Trashbin\Tests\StorageTest::testOwnerBackupWhenMovingFileOutOfShare
19:35:56 Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'SELECT "path", "fileid" FROM "oc_filecache" WHERE "storage" = ? AND "path" LIKE ?' with params [false, "sub\/%"]:
19:35:56 
19:35:56 SQLSTATE[22P02]: Invalid text representation: 7 ERROR:  invalid input syntax for integer: ""
19:35:56 
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractPostgreSQLDriver.php:91
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:128
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:836
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/DB/Connection.php:192
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/Cache/Cache.php:514
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/Cache/Updater.php:197
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/View.php:325
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/View.php:802
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/Node/Node.php:414
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/apps/files_trashbin/tests/StorageTest.php:496
19:35:56 
19:35:56 Caused by
19:35:56 Doctrine\DBAL\Driver\PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR:  invalid input syntax for integer: ""
19:35:56 
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php:93
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:830
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/DB/Connection.php:192
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/Cache/Cache.php:514
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/Cache/Updater.php:197
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/View.php:325
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/View.php:802
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/Node/Node.php:414
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/apps/files_trashbin/tests/StorageTest.php:496
19:35:56 
19:35:56 Caused by
19:35:56 PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR:  invalid input syntax for integer: ""
19:35:56 
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php:91
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:830
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/DB/Connection.php:192
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/Cache/Cache.php:514
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/Cache/Updater.php:197
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/View.php:325
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/View.php:802
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/lib/private/Files/Node/Node.php:414
19:35:56 /var/lib/jenkins/workspace/owncloud-core_core_PR-27042-D3EHZDNEIUXO3TN26UHXFNK7YRY4G5SCHC33PRCY6GB2OV74TYEA/apps/files_trashbin/tests/StorageTest.php:496
19:35:56 

@PVince81
Copy link
Contributor Author

Looks like the source storage id is (bool)0 before the rename in the test.
Somehow SQLite and MySQL don't mind about this...

Need to make sure that the source storage exists in DB before doing this.

@PVince81
Copy link
Contributor Author

Arghhh, seems there is yet another bug in the FS code.

The SharedCache doesn't seem to forward all calls to the underlying cache, so it cannot get its numeric id.

@PVince81
Copy link
Contributor Author

This seems to fix it, but not sure how dangerous...

diff --git a/apps/files_sharing/lib/Cache.php b/apps/files_sharing/lib/Cache.php
index f09267cecd..b56f25d8fd 100644
--- a/apps/files_sharing/lib/Cache.php
+++ b/apps/files_sharing/lib/Cache.php
@@ -74,11 +74,7 @@ class Cache extends CacheJail {
        }
 
        public function getNumericStorageId() {
-               if (isset($this->numericId)) {
-                       return $this->numericId;
-               } else {
-                       return false;
-               }
+               return $this->sourceCache->getNumericStorageId();
        }
 
        protected function formatCacheEntry($entry) {

I wonder why this wasn't found before... Probably hidden by another bug that cancels it out ?

@PVince81
Copy link
Contributor Author

Looks like this code was completely wrong. There isn't even a numericId attribute neither in this class nor in the parent classes.

But so far no unit test failed because of this...

@PVince81
Copy link
Contributor Author

I made a separate PR for this fix with an extra unit test: #27172

@PVince81 PVince81 force-pushed the sharing-trashfileforownerwhenmovedout branch from e22d634 to 47acac7 Compare February 16, 2017 11:11
@PVince81
Copy link
Contributor Author

I have now cherry-picked the fix from #27172 to this PR here.

Let's see if the tests pass first and then make a decision whether to keep the extra fix or not.

@PVince81
Copy link
Contributor Author

The extra fix has evolved a bit.

Waiting for #27172 to be merged then rebase.

Vincent Petry added 2 commits February 21, 2017 11:23
Whenever a share recipient moves files or folders out of the share,
make a backup copy in the owner's trash just in case.

The metadata stays on the recipient's copy that was moved out.
Because the encryption wrapper does some twisted stuff with methods like
moveFromStorage, we need to apply it here to make sure the behavior is
close to reality.
@PVince81 PVince81 force-pushed the sharing-trashfileforownerwhenmovedout branch from 47acac7 to 641d3dc Compare February 21, 2017 10:23
@PVince81
Copy link
Contributor Author

Rebased to include the final fix from #27172

@PVince81
Copy link
Contributor Author

GREEN ! FINALLY GREEN ! After all these struggles.

I hope you'll enjoy this enhancement, guys...

@PVince81 PVince81 merged commit 28c345d into master Feb 21, 2017
@PVince81 PVince81 deleted the sharing-trashfileforownerwhenmovedout branch February 21, 2017 20:28
@PVince81
Copy link
Contributor Author

Added to new features of 10.0: https://github.com/owncloud/core/wiki/ownCloud-10.0-Features

@PVince81
Copy link
Contributor Author

Regression: #27698

@lock
Copy link

lock bot commented Aug 3, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alice can't restore files that Bob moved out of shared folder
4 participants