Core: Alice can't restore files that Bob moved out of shared folder
Steps to reproduce
1) Alice shared the following files with Bob:
.
└── SharedwithBob
├── folder1
│ ├── file1.txt
│ ├── file2.txt
│ └── file3.txt
└── folder2
├── file4.txt
├── file5.txt
└── file6.txt
2) Bob moves folder2 out of the shared folder. (files app in web UI or desktop file explorer)
Expected behaviour
Alice expects a way to restore.
Actual behaviour
- No way to restore the files
- No entry in the activity list to check what happened with Alice files
- No entry in the Deleted files to restore the files
Server configuration
ownCloud version: (see ownCloud admin page)
9.0.1
Updated from an older ownCloud or fresh install:
fresh install
Signing status (ownCloud 9.0 and above):
No errors have been found.
Client configuration
Browser:
Chrome 50
Operating system:
Mac OS X 10.11.4
@MorrisJobke
michaelstingl
All 64 comments
@MTRichards Could you please tell us if it works as the design?
The only log I got in the server (Tested in 8.2.2 too):
{"reqId":"BSZDHMSH5I8UTvRK9bHA","remoteAddr":"10.1.10.100","app":"webdav","message":"Exception: {"Message":"HTTP/1.1 404 File with name Test-Hacker-Songs/Teil-Share2/mc_plus_plus-alice_and_bob.mp3 could not be located","Exception":"Sabre\DAV\Exception\NotFound","Code":0,"Trace":"
#0 /var/www/html/owncloud/3rdparty/sabre/dav/lib/DAV/Tree.php(178): OC\Connector\Sabre\ObjectTree->getNodeForPath('Test-Hacker-Son...')
#1 /var/www/html/owncloud/3rdparty/sabre/dav/lib/DAV/CorePlugin.php(287): Sabre\DAV\Tree->delete('Test-Hacker-Son...')
#2 [internal function]: Sabre\DAV\CorePlugin->httpDelete(Object(Sabre\HTTP\Request), Object(Sabre\HTTP\Response))
#3 /var/www/html/owncloud/3rdparty/sabre/event/lib/EventEmitterTrait.php(105): call_user_func_array(Array, Array)
#4 /var/www/html\/owncloud/3rdparty/sabre/dav/lib/DAV/Server.php(469): Sabre\Event\EventEmitter->emit('method:DELETE', Array)
#5 /var/www/html/owncloud/3rdparty/sabre/dav/lib/DAV/Server.php(254):Sabre\DAV\Server->invokeMethod(Object(Sabre\HTTP\Request), Object(Sabre\HTTP\Response))
#6 /var/www/html/owncloud/apps/files/appinfo/remote.php(56): Sabre\DAV\Server->exec()
#7 /var\/www/html/owncloud/remote.php(137): require_once('/var/www/html/o...')
#8 {main}","File":"/var/www/html/owncloud/lib/private/connector/sabre/objecttree.php","Line":159}","level":0,"time":"2016-04-18T11:01:40+02:00","method":"DELETE","url":"/remote.php/webdav/Test-Hacker-Songs/Teil-Share2/mc_plus_plus-alice_and_bob.mp3"}
cdamken
on 18 Apr 2016
What should happen is that Bob moves the folder or renames it, but Alice's files remain the same. This allows bob to organize the shared files wherever he wants, while Alice keeps the view she wants.
What actually happens to Alice's files? Does Bob still have access? Can Bob edit the files?
@janborchardt for ux to make certain I got that correct.
MTRichards
on 18 Apr 2016
@MTRichards Bob has edit permission of the share and moves only the subfolder, not the share itself. If Bob moved this subfolder outside the share (to account root for example), they are no longer available for Alice. Bob now can access those files. They are somewhere in his account, but not accessible for Alice.
michaelstingl
on 18 Apr 2016
Ouch, ok. So Bob basically takes over those files and Alice loses them. But they were Alice's files...this is not ideal. This should not happen, or be allowed, or if allowed the link to Alice's files should be kept as we do with root shares.
@jancborchardt your ux on what should happen here?
MTRichards
on 18 Apr 2016
Basically we should treat it similar to a deletion. Moving the files out should show as a activity and should be reversible for anyone having write access to the share. Most of the time it will probably have a reason (like a deletion), but sometimes it might be an error.
jancborchardt
on 18 Apr 2016
While I understand, wouldn't a user dragging files around and having them deleted be confusing? Since that is what happened here, perhaps I am overthinking it. If we add the undo, how do we know? Is it just an undo in activity, or somewhere else?
MTRichards
on 18 Apr 2016
From the original sharers point of view, there is no difference between moving and deletion. So from their POV it should probably be in their trash for them to restore. And yes, a change action in Activity with the ability to restore.
The other route is that we flat out restrict moving, but that would not make any sense since the sharee can also delete the files. It would be completely arbitrary.
jancborchardt
on 18 Apr 2016
Ok, so a feature request to enhance the activity stream with an undo, and to put the files into the owners trash can as well as the person that moved it? Or all trash cans?
MTRichards
on 18 Apr 2016
The feature is way before activities or anything.
When a file is moved out of your "vision" of the filesystem, it is like a delete, but up to now doesn't end up in your trashbin.
nickvergessen
on 18 Apr 2016
I suspect this touches core sharing code, so while a higher priority it needs proper planning and qa.
MTRichards
on 18 Apr 2016
yes
nickvergessen
on 18 Apr 2016
When a file is moved out of your "vision" of the filesystem, it is like a delete, but up to now doesn't end up in your trashbin.
It's a cross storage move. That means a deletion in the one storage and a creation in another storage. cc @icewind1991 for the storage details. Maybe we could go into that direction.
MorrisJobke
on 19 Apr 2016
Nice use case.
The sharing code actually does what we ask from it. A file is moved from storage A to storage B. Shares don't have the concept of versions or trashbin. That is stuff we add later.
We would need to keep track of moves in the trashbin. And if we move from a sharedStorage to also add the files in the trash of the owner.
But we should really think and discuss how to do this. As this could get messy really quick.For example what happens if the recipient moves the files back? How do we handle versions? and all that.
rullzer
on 19 Apr 2016
This should also apply if Bob is deleted by the administrator. It would make life much easier because it wouldn't make sharing a hell (analogous to the dependency hell) for the admin if he has to remove a user with loads of shared files. Also Alice wouldn't be in for a bad and nasty surprise if the shared files are suddenly gone. Restoring single files is extremely difficult if not outright impossible if encryption without recovery key is turned on and one has to rely on methods used to restore files encrypted by a cryptolocker to get them back. This shouldn't be something that a professional business grade software should do at any time!
e-alfred
on 20 Apr 2016
I think we should tee this up for design week, because it is unlikely to be fixed prior to 9.2 given the amount of thinking and impact on core.
MTRichards
on 20 Apr 2016
Let's make a feature request out of 00005262 with this.
MTRichards
on 20 Apr 2016
@MTRichards (another person) asked some similar as a feature request in 00005283.
IMO I think that folders under the share should be kept as part of the share.
The owner (Bob in this case) should be informed that a folder tried to be moved or removed and he has the possibility to recover it.
And/or the other shared user (Alice in this case) that the folder can be moved only in inside the share or copied as a fork in another directory.
cdamken
on 20 Apr 2016
All good input for a discussion. When we plan 9.2 we will need to normalize them and decide on a course of action.
MTRichards
on 20 Apr 2016
I'm not sure about how hard can it be the implementation:
Any share (main folder (SharedwithBob) in this case) can be moved to different folder and can even be renamed.
Could be the same behavior (moved and renamed) be followed for the sub-folders from the share? (Folder1 and Folder2)
cdamken
on 20 Apr 2016
Well sometimes a move is a add, delete and move at the same time. So yes, that is really not that easy.
nickvergessen
on 20 Apr 2016
@MTRichards design week sounds like a good idea!
rullzer
on 20 Apr 2016
@nickvergessen probably vectoring the shares :smile:
cdamken
on 20 Apr 2016
@MTRichards A solution that could be backported would be much appreciated.
michaelstingl
on 23 May 2016
First question, backported to what version(s)?
The best way to restore this is probably the share overview, that allows someone to restore a share. The best I can do is 9.2 at this time, and I can't guarantee a backport is possible since it may require a new set of tables and changes to core as it stands.
The other item is the "accept a share" for normal shares. This is quite a popular request, and may also provide a solution here.
Still, earliest for both is 9.2.
MTRichards
on 23 May 2016
@michaelstingl this doesn't seem to represent the "restore a lost share" feature request?
00005262
MTRichards
on 23 May 2016
I agree that there most likely is not a simple backportable solution.
rullzer
on 24 May 2016
As I understood, you will investigate this in next design week. If there are different possible solutions to solve this, perhaps one of them could be easier to backport than another one, which would be my choice. But I think I also understood the complexity.
michaelstingl
on 24 May 2016
@DeepDiver1975 @dragotin Plz discuss this for 9.2 planning and adjust Milestone if necessary.
michaelstingl
on 20 Jul 2016
As a share is an storage, this storage also needs to have an trashbin, so that would be a shared trashbin.
felixboehm
on 25 Jul 2016
@felixboehm @dragotin @DeepDiver1975 What is the status here?
michaelstingl
on 11 Aug 2016
@felixboehm currently there is no concept of per-storage trashbin.
This was discussed before here but would need to rethink/rewrite the trashbin logic to work differently:
- trashbin methods for storages: https://github.com/owncloud/core/issues/12274
- trashbin on ext storage: https://github.com/owncloud/core/issues/4389
- https://github.com/owncloud/core/issues/18771#issuecomment-142465905
Then come questions about how the UI should behave, whether the "Deleted files" section displays files deleted from all storages combined or need to have subdirectories for every storage, etc.
PVince81
on 13 Sep 2016
Hmm, and having a "received file that is in the trashbin of another user but visible because it was shared with me" adds yet another level of complexity, because as recipient you don't get access to the full trashbin of the other user but only a subset of files/folders.
PVince81
on 13 Sep 2016
In general, I would expect this behaviour for data protection reasons:
- if Alice shared the folder with Bob and Bob deleted some files (or moved it to another location outside of Alice's work area), Alice should be possible to restore the files again
- if Alice shared the folder with Bob and Alice deleted some files (or moved it to another location outside of Alice's work area), Bob should NOT be able to restore the files again
jochenwezel
on 27 Oct 2016
It should be technically possible (without overhauling the whole concept) to at least detect whenever a recipient moves a file out of a shared folder (through hooks) and then create a copy of that file or folder in the owner's trashbin. This would cover case 1. from @jochenwezel.
Case 2. should already be covered by the current code.
PVince81
on 21 Nov 2016
Downgrading to high, I don't think this is critical / showstopper. @michaelstingl @cdamken
We need an agreement on how to proceed with the fix and the expected behavior.
I think what @jochenwezel said here https://github.com/owncloud/core/issues/24053#issuecomment-256608820 is what makes most sense.
PVince81
on 6 Dec 2016
This is the behaviour in 9.1.2:
| | Alice's Activity | Bob's Activity | Alice's Trashbin | Bob's Trashbin |
|--------------------------------------------|:---------------------:|:-----------------------:|:----------------:|:--------------:|
| Alice deletes file1.txt in Alice's share | You deleted file1.txt | Alice deleted file1.txt | file1.txt | - |
| Bob deletes file2.txt in Alice's share | Bob deleted file2.txt | You deleted file2.txt | file2.txt | file2.txt |
| Alice moves file3.txt out of Alice's share | - | - | - | - |
| Bob moves file4.txt out of Alice's share | - | - | - | - |
This is what I would expect:
| | Alice's Activity | Bob's Activity | Alice's Trashbin | Bob's Trashbin |
|--------------------------------------------|:--------------------------------:|:-----------------------:|:----------------:|:--------------:|
| Alice deletes file1.txt in Alice's share | You deleted file1.txt | Alice deleted file1.txt | file1.txt | - |
| Bob deletes file2.txt in Alice's share | Bob deleted file2.txt | You deleted file2.txt | file2.txt | file2.txt |
| Alice moves file3.txt out of Alice's share | - | - | - | - |
| Bob moves file4.txt out of Alice's share | Bob removed file4.txt from share | - | file4.txt | - |
(regular move operations in the web UI don't show up in the Activity too. Should this get improved in general? A raised this question here too)
@SergioBertolinSG How do we test common file operations? Could you add Behat tests to cover the 4th case?
michaelstingl
on 19 Dec 2016
Nice. Some BDD going on here.
felixboehm
on 19 Dec 2016
@michaelstingl yes, I'll add it. We have sharing cases here: https://github.com/owncloud/core/blob/master/build/integration/features/sharing-v1.feature.
SergioBertolinSG
on 20 Dec 2016
@michaelstingl @SergioBertolinSG Thinking some steps forward, What happens (or which are the expectations) when on the scenario https://github.com/owncloud/core/issues/24053#issuecomment-268046713 the file2.txt is recovered from any of the two parties (for example: deleted by error and try to recover it) or file4.txt is recovered by Alice because Bob erased it?
cdamken
on 11 Jan 2017
Making a copy into the owner's trash will induce a metadata loss. And copying metadata to the trashed file is dubious in cases where both files are restored (moved back into the share and also restored by owner), see https://github.com/owncloud/core/issues/22594#issuecomment-275613586
PVince81
on 27 Jan 2017
WIP PR here: https://github.com/owncloud/core/pull/27042
PVince81
on 27 Jan 2017
Update after some internal discussions:
This is what I would expect as a quick solution for the 'restore bug':
| | Alice's Activity | Bob's Activity | Alice's Trashbin | Bob's Trashbin |
|--------------------------------------------|:--------------------------------:|:-----------------------:|:----------------:|:--------------:|
| Alice deletes file1.txt in Alice's share | You deleted file1.txt | Alice deleted file1.txt | file1.txt | - |
| Bob deletes file2.txt in Alice's share | Bob deleted file2.txt | You deleted file2.txt | file2.txt | file2.txt |
| Alice moves file3.txt out of Alice's share | You moved file3.txt from x to y | Alice deleted file3.txt | - | - |
| Bob moves file4.txt out of Alice's share | Bob deleted file4.txt | You moved file4.txt from x to y | file4.txt | - |
Concerns
- We treat a move as a delete which appears quite inconsistent to me and does not deliver excellent UX
- File4.txt: The proposed handling enables a file ownership transfer (of data, not shares!) and might produce duplicate files in case of a restore by Alice. This behavior/expectation needs to be discussed.
Further issues
- Move/rename activity not implemented yet
- System-wide external storages which never have an owner: If someone moved a file out of that storage into their local home, the file also disappeares for everybody else.
- File2.txt: If Bob restores -> File metadata lost because of changed FileID https://github.com/owncloud/core/issues/22594
=> Decided to make a copy of the file and replicate metadata for copy in the owner's trashbin and keep the metadata with the file that is moved out of the share - When both Alice and Bob restore the file this can lead to duplicates (Bob moved the file out of the share, Alice has the file in her trashbin and restores => Bob now has two identical versions of the file, one in the share, one in his personal space)
=> @PVince81 suggested a "file linking" info in the database to make it possible to re-delete that trashed file if the owner moves the file back into the share
=> would increase complexity a lot and could cause unwanted interactions elsewhere
=> any other ideas on solving this? - Other internal issues that need to be solved
- file is locked when moved out so can't copy to trash
- make sure encryption keys work properly in this scenario for the trashed file and the file can be restored
pmaier1
on 27 Jan 2017
Not sure if this is an aspect of this issue or if it should be another issue:
For data protection rules, it might be required in some cases to securely delete folders/files without trash bin option (real physical deletion). (from my point of view, it would be enough to provide this option through web-interface, a little bit more hidden so that it's not the default click for a user if he wants to delete a file)
Might it make sense to extend this use-case to the table above?
jochenwezel
on 27 Jan 2017
@jochenwezel slightly related, shredding: https://github.com/owncloud/core/issues/26427
PVince81
on 27 Jan 2017
I assume that we'll also want a copy of the versions in the owner's trashbin with the backup. There is already code to keep versions in the regular trashbin delete code, so I'll reuse that.
PVince81
on 30 Jan 2017
Interesting fact: this behavior already works for federated shares. If a recipient of a federated share moves a file out of a share, the local OC instance will issue a DELETE statement to the owner's ownCloud instance, which will simply move the file to trash on that instance.
PVince81
on 2 Feb 2017
- [x] owner backup PR for the backup feature is ready for review: https://github.com/owncloud/core/pull/27042
- [ ] activity for renames: https://github.com/owncloud/activity/issues/375
- [ ] adjust activity to also be raised in the specific "owner backup" scenario
- [x] documentation ticket for the behavior: https://github.com/owncloud/documentation/issues/3501
PVince81
on 2 Feb 2017
Interesting fact: this behavior already works for federated shares. If a recipient of a federated share moves a file out of a share, the local OC instance will issue a DELETE statement to the owner's ownCloud instance, which will simply move the file to trash on that instance.
does it also appears in the activities?
cdamken
on 2 Feb 2017
does it also appears in the activities?
for federated shares, the activity will say "the file X was deleted" because the federated server doesn't know it was moved out since it received a DELETE command... Might need more special handling, passing hints to that server so it can detect it.
PVince81
on 2 Feb 2017
The basic POC for rename activity seems to work without big blockers so far https://github.com/owncloud/activity/pull/558. However I'm struggling with the formatter and there are many additional conditions that need to be checked against. I'm not sure there is enough time to finish this for 10.0.
Additionally we'll need some special handling for the trashbin. Basically: if trashbin is enabled, don't say "moved out" but say "moved out and look at your trash the file is backed up there". Doing this without big hacks might be tough.
PVince81
on 2 Feb 2017
Don't think there is a need for this special message. "moved out and look at your trash the file is backed up there" when a delete happens its no different. And yes, when somebody moves the file back and its restored we have it twice I think users will be able to handle this.
hodyroff
on 9 Feb 2017
@jochenwezel Great thought, but should be an app outside of core, IMHO.
hodyroff
on 9 Feb 2017
Still struggling with the PR. Writing a unit test for this case revealed a few potential bugs hidden in the platform code. It's the kind of bug that is not easy to fix because some other code might be relying on it returning the wrong value. (the "two bugs cancel each other" kind)
PVince81
on 16 Feb 2017
@PVince81 This sounds difficult and to the same time it sounds as you are on the right way 👍
jochenwezel
on 16 Feb 2017
Yeah. Sometimes work takes much longer than originally expected due to these kind of findings. I hope I can iron all this out for 10.0
PVince81
on 16 Feb 2017
After a long and painful struggle with glitches in the architecture, the PR was finally merge: https://github.com/owncloud/core/pull/27042
Enjoy your new (partial) feature: backup is now created.
What's still missing: generating an activity. This is still WIP.
PVince81
on 21 Feb 2017
Tested and works in 10.0 Alpha.
cdamken
on 17 Mar 2017
not sure how important the activity is, setting to triage
PVince81
on 15 May 2017
not sure how important the activity is, setting to triage
I see that there might be some more high priorities for 10.0 and to deliver 10.0 soon.
So, for me it's okay to wait for another minor release to see this improvement coming into reality.
jochenwezel
on 15 May 2017
Any ETA of a fix on this issue?
CorneeldH
on 2 Nov 2017
@CorneelDragon which part of the issue ?
The main part is already fixed in 10.0.3.
The missing bit is about creating an activity entry.
PVince81
on 2 Nov 2017
okay, just saw it was still open en scrolled down, my bad. I will verify it on our test 10.0.x-environment.
Thanks for the quick response!
CorneeldH
on 2 Nov 2017
Raised https://github.com/owncloud/core/issues/29473 for the remaining tasks as the main thing is done.
PVince81
on 6 Nov 2017
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[bot] picture](https://avatars.githubusercontent.com/in/6672?v=4&s=40)
lock[bot]
on 30 Jul 2019
Related issues
rehoehle
·
4Comments
tommis
·
5Comments
gxgani
·
5Comments
jvillafanez
·
4Comments
dpeger
·
3Comments
Most helpful comment
After a long and painful struggle with glitches in the architecture, the PR was finally merge: https://github.com/owncloud/core/pull/27042
Enjoy your new (partial) feature: backup is now created.
What's still missing: generating an activity. This is still WIP.