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

Alice can't restore files that Bob moved out of shared folder #24053

Closed
michaelstingl opened this issue Apr 18, 2016 · 64 comments · Fixed by #27042
Closed

Alice can't restore files that Bob moved out of shared folder #24053

michaelstingl opened this issue Apr 18, 2016 · 64 comments · Fixed by #27042

Comments

@michaelstingl
Copy link

michaelstingl commented Apr 18, 2016

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
  1. 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

00005262

@cdamken
Copy link
Contributor

cdamken commented Apr 18, 2016

@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"}

@MTRichards
Copy link
Contributor

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.

@michaelstingl
Copy link
Author

@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.

@MTRichards
Copy link
Contributor

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?

@jancborchardt
Copy link
Member

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.

@MTRichards
Copy link
Contributor

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?

@jancborchardt
Copy link
Member

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.

@MTRichards
Copy link
Contributor

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?

@nickvergessen
Copy link
Contributor

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.

@MTRichards
Copy link
Contributor

I suspect this touches core sharing code, so while a higher priority it needs proper planning and qa.

@nickvergessen
Copy link
Contributor

yes

@MorrisJobke
Copy link
Contributor

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.

@rullzer
Copy link
Contributor

rullzer commented Apr 19, 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.

@e-alfred
Copy link

e-alfred commented Apr 20, 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!

@MTRichards
Copy link
Contributor

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
Copy link
Contributor

Let's make a feature request out of 00005262 with this.

@cdamken
Copy link
Contributor

cdamken commented Apr 20, 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.

@MTRichards
Copy link
Contributor

All good input for a discussion. When we plan 9.2 we will need to normalize them and decide on a course of action.

@cdamken
Copy link
Contributor

cdamken commented Apr 20, 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)

@nickvergessen
Copy link
Contributor

Well sometimes a move is a add, delete and move at the same time. So yes, that is really not that easy.

@MorrisJobke MorrisJobke added this to the 9.2-next milestone Apr 20, 2016
@rullzer
Copy link
Contributor

rullzer commented Apr 20, 2016

@MTRichards design week sounds like a good idea!

@cdamken
Copy link
Contributor

cdamken commented Apr 20, 2016

@nickvergessen probably vectoring the shares 😄

@michaelstingl
Copy link
Author

@MTRichards A solution that could be backported would be much appreciated.

@MTRichards
Copy link
Contributor

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
Copy link
Contributor

@michaelstingl this doesn't seem to represent the "restore a lost share" feature request?
00005262

@PVince81
Copy link
Contributor

PVince81 commented Feb 2, 2017

@cdamken
Copy link
Contributor

cdamken commented Feb 2, 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?

@PVince81
Copy link
Contributor

PVince81 commented Feb 2, 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
Copy link
Contributor

PVince81 commented Feb 2, 2017

The basic POC for rename activity seems to work without big blockers so far owncloud/activity#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.

@hodyroff
Copy link
Contributor

hodyroff commented Feb 9, 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
Copy link
Contributor

hodyroff commented Feb 9, 2017

@jochenwezel Great thought, but should be an app outside of core, IMHO.

@PVince81
Copy link
Contributor

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)

@jochenwezel
Copy link

@PVince81 This sounds difficult and to the same time it sounds as you are on the right way 👍

@PVince81
Copy link
Contributor

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
Copy link
Contributor

After a long and painful struggle with glitches in the architecture, the PR was finally merge: #27042

Enjoy your new (partial) feature: backup is now created.

What's still missing: generating an activity. This is still WIP.

@PVince81 PVince81 reopened this Feb 21, 2017
@cdamken
Copy link
Contributor

cdamken commented Mar 17, 2017

Tested and works in 10.0 Alpha.

@PVince81
Copy link
Contributor

not sure how important the activity is, setting to triage

@PVince81 PVince81 modified the milestones: triage, 10.0 May 15, 2017
@jochenwezel
Copy link

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.

@CorneeldH
Copy link

Any ETA of a fix on this issue?

@PVince81
Copy link
Contributor

PVince81 commented Nov 2, 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.

@CorneeldH
Copy link

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!

@PVince81
Copy link
Contributor

PVince81 commented Nov 6, 2017

Raised #29473 for the remaining tasks as the main thing is done.

@PVince81 PVince81 closed this as completed Nov 6, 2017
@felixboehm felixboehm removed this from the triage milestone Apr 10, 2018
@lock
Copy link

lock bot commented Jul 30, 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 Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.