Applications cannot detect "unshare from self"

[paulijar]

When user A shares a file with user B and then unshares it, a signal ('OCP\Share', 'post_unshare') is emitted. If, instead, user B unshares the file shared by A, then this or any other signal is not emitted.

Looking at the source codes, I see that the core has two libraries related to sharing: Share and Share20. I assume that these are an old and a new version of the library, and it seems to me that file sharing is using the latter. The library Share seems have a distinct signal 'post_unshareFromSelf' for the described case, but library Share20 has no corresponding signal.

The missing signal is the root cause for the issue owncloud/music#567.

[PVince81]

Indeed, it's missing.

I checked and the code in

core/lib/private/Share/Share.php

Line 1101 in 72cc00d

\OC_Hook::emit('OCP\Share', 'post_unshareFromSelf',

isn't called with the new sharing version.

This should be added in ShareManager::deleteFromSelf.

Do you have time preparing a PR for this ?

[paulijar]

@PVince81 Yes, I can do that but I'd like to get some background information: I'm wondering what is the rationale of there being a distinct signal with different parameter structure for the unshare-from-self in the old Share library as compared to unshare-by-owner. Do we still need to do it like that for Share20, or could there be the same signals 'pre_unshare' and 'post_unshare' in both cases?

As an application developer, I'm not really interested about who initiated the unshare. The important information is that the file got unshared and I would prefer to define only single hook to handle this.

[PVince81]

@paulijar to be honest I don't really know what the rationale for this separate hook and the original committer has left the project. The purpose of the PR here (short term) would just be to bring back the missing functionality in the share manager without changing or improving its semantic.

[paulijar]

@PVince81 IMHO, we shouldn't migrate the separation post_unshare/post_unshareFromSelf if there is no good reason for it. It just complicates the code both in the Share20 library and in the applications hooking with the signals. Also, there's no reason to rush with this as the function ShareManager::deleteFromSelf seems to have been there without the signal already for 18 months.

Grepping through the source codes of core and default apps, I couldn't find anyone actually connecting the hook post_unshareFromSelf. Everyone seems to assume that pre_unshare and post_unshare signals get emitted when a file is unshared, and I think this is a reasonable expectation. For example, the Activity app connects just the hook pre_unshare. I also tested it, and certainly the Activity app does not show "unshares from self". But when I added the emitting of pre_unshare to ShareManager::deleteFromSelf, then the Activity app could show those unshares quite sanely for the both participants of the share.

[PVince81]

We do need to migrate post_unshareFromSelf because not having it would qualify as "API breakage" since it would break with apps relying on it. So this should be done as a bugfix and backported.

Now for having a more meaningful behavior we should introduce new hooks / events for the one signal that is triggered on "unshare" for any user even the current user. This way existing apps aren't surprised to suddenly receive events there where they didn't in past versions.

On the other hand, with the new marketplace app devs need to modify they apps anyway so if your proposed change would happen in OC 10+ it might not be too bad...

@DeepDiver1975 thoughts ?

[paulijar]

@PVince81 Okay, fair enough, I see your point.

I'm still a bit confused with the parameter data structure of these unshare signals. For both post_unshare and post_unshareFromSelf, the data structure contains an array of deleted shares. Has the logic been in the past such that removing an original share removes also all the reshares, but this has changed at some point? So nowadays the array should ever contain only one item?

As a related note, the function Manager::deleteShare calls Manager::deleteChildren but Manager::deleteFromSelf does not. But it is commented here

core/lib/private/Share20/Manager.php

Line 746 in 6a5af02

* FIXME: remove once https://github.com/owncloud/core/pull/21660 is in

that the whole function should be removed once #21660 is in and that PR was merged 18 months ago.

[PVince81]

I'm not deeply familiar with the hooks themselves, but one thing I know is that since 9.0 we have flattened the reshares. Basically in OC < 8.2 you could create a long chain of reshares. Since OC 9.0 all reshares are reattached to the share owner, so the share owner always has a list of all reshares now.

I guess this could affect the result list as you observed for the hooks.

[paulijar]

@PVince81 What is the relation of this flattening of reshares, and the migration from Share to Share 2.0? Is Share 2.0 always using the flattened structure, or is there releases where Share 2.0 is used but the reshare structure is still hierarchical?

[PVince81]

Good question. I forgot to mention that the flattened behavior was introduced through the move to Share 2.0. Bascially Share 2.0 is a reimplementation of sharing with much cleaner code and we took this opportunity to flatten the reshares.

[paulijar]

@PVince81 Could we at least add some new keys to the parameter array sent with post_unshareFromSelf? That shouldn't break the old applications, right?

I ask because I just realized that the signal is pretty much useless in the form in which it is sent in the old Share library: The main problem is that the signal parameters do not include the node ID of the unshared file/folder. There is only the ID of the share and the target path. But as this is a post hook, the share no longer exists when the hook is executed, and it is impossible to obtain the node or the node ID with the available data.

[PVince81]

@paulijar adding new keys is fine

[PVince81]

PR is here: #28401

Thanks a lot!

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