Lost share when transferring files to recipient

[PVince81]

Steps

1. Create two users "user1" and "user2"
2. Login as "user1"
3. Create folder "foo/test"
4. Share "foo" with "user2"
5. Login as "user2"
6. Share the received "foo/test" with link
7. Log out
8. On the CLI: occ files:transfer-ownership user1 user2
9. Login as "user2"
10. Find the "test" folder in the "transferred from" folder
11. Check link share

Expected result

Link share preserved

Actual result

Link share completely lost, even removed from oc_share table.

Version

10.0.8 RC1

Discovered while testing #30161

[ownclouders]

GitMate.io thinks possibly related issues are #13671 (Versions lost when moving file into shared folder as recipient), #22594 (Meta data lost when recipient restores a file), #13307 (Move shared file into shared folder as recipient, moved share info lost), #16225 (Files_texteditor gets frozen when a user tries to open a shared file as recipient which was deleted), and #15468 (Files stop being shared after a recipient restores them to previous versions).

[PVince81]

setting to p2 as data loss is bad

[PVince81]

talked with @felixboehm and we're going to revert the original fix as the effect is less critical

then move ticket to 10.0.9 for a proper full fix

[PVince81]

Revert PR is here for stable10: #31164

[PVince81]

The revert doesn't solve the issue.

@sharidas will try to find a solution. I expect it to be non-intrusive and should not affect QA testing efforts. If a solution is found in time we can consider putting it in RC3 if a RC3 is deemed necessary.

@pmaier1 @felixboehm FYI

[PVince81]

Investigated with @sharidas: the public link has a "parent" field pointing to the regular share.
When we delete the regular share, the link share is deleted as well as the parent is gone.
The reason we delete the regular share from "user1" to "user2" is because "user2" becomes the owner, so the share in question isn't necessary any more.

@sharidas is investigating if there is a good way to deparent the reshare so it can continue to exist, with a solution within the boundaries of the current sharing API.

[PVince81]

the parent relationship is not exposed over API

Another idea is to add a new method ShareManager::transferShare($share, $newOwner) and move all the transfer logic there and add the part about reshare parents. This is a better approach because normally the transfer ownership command should not know anything about shares and rely on the share manager/provider to execute the operation. Remember, share manager/provider is an abstraction and third parties can provide their own which would also need to implement a method about transferring their shares.

[PVince81]

to be rescheduled

[PVince81]

I misunderstood, this was close to completion and just got finished. Closing

[sharidas]

In the above scenario, the deletion of share at https://github.com/owncloud/core/blob/stable10/lib/private/Share20/Manager.php#L730.
Which internally calls deletion of children at https://github.com/owncloud/core/blob/stable10/lib/private/Share20/Manager.php#L978.

I was having a thought of processing public links first. But this code path suggests that it doesn't give any upper edge if we process public links first and the remaining shares at the last.

[sharidas]

Manual testing of scenario #31150 (comment) still fails when the git HEAD is pointed to commit ( git reset ) representing this pr #31176

[sharidas]

Would it be a bad solution if the deleteShare() (here -> https://github.com/owncloud/core/blob/stable10/lib/private/Share20/Manager.php#L962 ) is allowed to exclude some of its children? May be pass the types of children that we want to exclude. And the same argument to be passed to deleteChildren() https://github.com/owncloud/core/blob/stable10/lib/private/Share20/Manager.php#L913
And then in the deleteChildren loop just ignore the type and continue.

If the exclude list is empty the behaviour of deleteShare() or deleteChildren() would be same as we have now.

[PVince81]

Not sure if it would be a good idea, because it means you'd have orphaned entries in the DB where the "parent" value points nowhere. If we had foreign key contraints it would be stricly forbidden by the DB.

How about gathering the list of children before doing any deletion ? Then transfer the children. Once the transfer is done, proceed with deletion.

[PVince81]

the latter would require two iteration over the user's share but I don't expect a single user to have 10000+ shares so it should be ok

[sharidas]

Not sure if it would be a good idea, because it means you'd have orphaned entries in the DB where the "parent" value points nowhere. If we had foreign key contraints it would be stricly forbidden by the DB.

Yes the orphaned entries in the DB was pulling me back from this idea.

How about gathering the list of children before doing any deletion ? Then transfer the children. Once the transfer is done, proceed with deletion.

didn't quite get by 'transfer the children'

[sharidas]

How about gathering the list of children before doing any deletion ? Then transfer the children. Once the transfer is done, proceed with deletion.

Ok so if I understood correctly for 'transfer the children' -> Create new share from the child share. And then proceed with the deletion.

[PVince81]

or just update the child share to not have a parent value any more ? whatever you already did in the current code, but do it earlier ?

[sharidas]

Created a WIP PR -> #31924.

[lock]

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.