New user in group gets old behavior of "automatically accept incoming shares"

[phil-davis]

Steps to reproduce

1. Have an existing system with users and groups, e.g.:
   Group: finance with users fin1 and fin2
   User fin1 shares folder finance-folder with finance group
   User fin2 has logged in and has access to finance-folder

2. As admin, disable "Automatically accept new incoming local user shares"

3. As admin create more users, e.g. fin3 in group finance

4. Login as fin3 - the finance share is automatically accepted and seen (not as per the current system setting above)

Now let some months/years pass, and "everyone" has forgotten about the time when "Automatically accept new incoming local user shares" was enabled (and the time before that when the setting did not even exist)

5. User fin1 creates a new folder other-finance-stuff and shares it with finance group.
6. Users in the finance group get to choose to accept or decline the new share - good.
7. As admin create new user fin4 in group finance
8. Login as fin4
9. fin4 sees finance-folder automatically, without accepting it, but has to accept other-finance-stuff in order to see the files
10. fin4 wonders "why is this inconsistency?"
11. fin4 calls the help desk.
12. the help desk has no idea what is going on, why do some received shares still auto-accept, and others need to be explicitly accepted?

Expected behaviour

For a new user, when they first login, all the shares that they receive behave according to the current setting of "Automatically accept new incoming local user shares"

Actual behaviour

Old shares to groups keep their auto-accept behavior that was in place at the time they were created.
For existing users in the group, this is not really noticeable, because the existing users have likely logged in and already have the shared folder (or decided to decline it...).

But when a new user is added to the group, they get the old auto-accept behavior. If the new user is receiving multiple shares (e.g. they were added to multiple groups, or the group they were added to has multiple folders shared with it...) then some shares will be auto-accepted and some will need an accept/decline decision.

This will happen mostly for existing "old" sites. Their golden-oldy shares of "finance-files" "engineering-drawings"... will continue to behave in the old way as new users join the relevant groups. But new shares of "space-division-files" will behave in the new way.

Server configuration

10.0.9RC1

[phil-davis]

This is not an issue for sharing with individual users - because a newuser cannot have "old shares" that have been sitting around shared with newuser, just waiting for newuser to exist some day.

[ownclouders]

GitMate.io thinks the contributor most likely able to help you is @PVince81.

Possibly related issues are #4435 (Accept incoming shares), #841 (Restrict users/groups to shared folders), #19153 (User Can Accept Shares from Other Users), #31613 ([stable10] Accept user share stable10), and #6167 (Sharing with a group).

[PVince81]

Hmmm I think this is because the group share entry in the database gets the group value "accepted=STATE_ACCEPTED" when the mode is "auto-accept" so it will default to accepted for every current and future members.

New group shares will get "accepted=STATE_PENDING" so every new member will get the value "pending" when "auto-accept" is disabled.

If we want to fix this we'd need to update all group states after toggling the checkbox.

@pmaier1 is this something we want ?

[PVince81]

Some background: when a group share is created, we don't automatically generate sub-entries for every members. The sub-share-entries are only created once a recipient does a local change like renaming/moving the received share or unshare from self. This means that the "accepted" info isn't available in the DB until the user does something. So the value requires a default which is read from the group values.

Now thinking of it, there might be another possible fix: if no sub-share is available, infer the value based on the "auto-accept" flag instead of the group "accept" value. It also means that the group's "accept" value becomes unused. Need to verify that it won't cause side effects.

Tagging as bug for now, unless @pmaier1 has objections

[phil-davis]

That sounds like the right fix - a group share does not need to remember the state of "auto-accept", because actually when a new user appears and has rights to a group share, then the way in which they are offered the share should be whatever is the current setting of "auto-accept".

[PVince81]

Estimate 0.5md for a fix

I'm not sure if this should be a release blocker @pmaier1.
In any case I'd schedule work on this in the current sprint so we're ready for the next release.

@phil-davis can you prepare an acceptance test to enforce this behavior ?

[phil-davis]

Yes, I will make an automated acceptance test scenario - should be easy enough.

And with the suggested way to fix this, if it does not get into 10.0.9 then the behavior will just be a little inconsistent for a few weeks. Then it will "self-repair" when the fix is released.

[PVince81]

And with the suggested way to fix this, if it does not get into 10.0.9 then the behavior will just be a little inconsistent for a few weeks. Then it will "self-repair" when the fix is released.

Yes, indeed

[phil-davis]

PR #31913 has 3 acceptance test scenarios that currently will fail, but should pass.

[phil-davis]

In doing that, I discovered an easier and bit more surprising scenario that fails:

1. Have an existing group grp1 with some users in it.
2. Turn off "auto-accept"
3. Share a folder with the grp1
4. The existing users in the group get a notification about the share - good.
5. Create newuser and add newuser to grp1
6. Login as newuser - there is no notification, and the shared folder is not seen either

Note: if newuser goes to "shared with you", the share is listed and can be accepted.

[phil-davis]

Hint: apps/files_sharing/lib/Hooks.php has
share.afterCreate that does $this->notificationPublisher->sendNotification
share.afterDelete that does $this->notificationPublisher->discardNotification

These manage the notifications that are waiting (or not) for the user(s) receiving the share.

So it probably needs some hooks for when a user is added or removed from a group, with names like group.afterAddMember group.afterRemoveMember, which can be used to trigger adding or removing the relevant notification(s).

And (hopefully) they would also get called when a user is created and added to groups "all at once", and similarly when deleting a user, the user gets removed from eaach group they are in, and pending notifications get removed...

[pmaier1]

I agree with the expected behavior but don't see a huge issue with this. Yes, it can confuse users but everything will continue to work as normal. p3 is the right priority here. Fix would be nice but not a blocker from my POV.

[PVince81]

@phil-davis I don't think users added to a group should get a notification. If 100+ things were shared with this group since the beginning of time, that would be too many. Maybe one idea would be to receive only a single notification telling the user to go to "Shared with you" to accept many shares. Let's raise this one as a separate ticket for discussion.

[jvillafanez]

Now thinking of it, there might be another possible fix: if no sub-share is available, infer the value based on the "auto-accept" flag instead of the group "accept" value. It also means that the group's "accept" value becomes unused. Need to verify that it won't cause side effects.

I have something based on this. The relevant changes are below:

@@ -951,11 +958,13 @@ class DefaultShareProvider implements IShareProvider {
                $shareTime = new \DateTime();
                $shareTime->setTimestamp((int)$data['stime']);
                $share->setShareTime($shareTime);
+               $share->setState((int)$data['accepted']);
 
                if ($share->getShareType() === \OCP\Share::SHARE_TYPE_USER) {
                        $share->setSharedWith($data['share_with']);
                } elseif ($share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) {
                        $share->setSharedWith($data['share_with']);
+                       $share->setState($this->getDefaultShareState());
                } elseif ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK) {
                        $share->setPassword($data['share_with']);
                        $share->setToken($data['token']);
@@ -967,7 +976,6 @@ class DefaultShareProvider implements IShareProvider {
                $share->setNodeId((int)$data['file_source']);
                $share->setNodeType($data['item_type']);
                $share->setName($data['share_name']);
-               $share->setState((int)$data['accepted']);
 
                if ($data['expiration'] !== null) {
                        $expiration = \DateTime::createFromFormat('Y-m-d H:i:s', $data['expiration']);

While this fixes the main issue described above, the problem is that it there isn't anything that create a usergroup share on auto accept. This means that, when the admin switches from auto-accept to manually accept, all the shares that were auto accepted aren't accepted any longer.
You have to explicitly accept the share (first reject and then accept) in order to create the usergroup share entry so the share behaves properly for that user.

If we still want to go with this solution we'll need to solve this scenario too.

[PVince81]

This means that, when the admin switches from auto-accept to manually accept, all the shares that were auto accepted aren't accepted any longer.

Right. At this point I don't think we want to automatically create the entries as it could become a lot for big groups.

Seems in this one case the UX would become worse.

@jvillafanez any other ideas ?

[jvillafanez]

I don't have simple ideas that require minor changes. I'd prefer to keep things as simple as possible, so I'm not proposing crazy ideas that can be easily become a huge mess to implement, understand and maintain.

Current options are:

• leave it as it is right now, which means that the default group share state will be set when the share is created. This default state won't be changed
• "overwrite" the default group share state with whatever the option is configured (auto-accept or not auto-accept). Changing the option will also change the default state of the group share.

In both cases, unless the user has explicitly accepted or rejected the group share, the default group share state will be used for each user.

Taking into account that we don't expect changes in the configuration once it is set, I think we can use the second option because the UX is more consistent, and we can document the drawback with a big warning.

[PVince81]

The second proposed solution looks close to #31870 (comment), and will likely have the same effect: users having to reject and then re-accept ? I think this is a worse UX than with proposal one (leaving as it is now).

The third alternative is to agree to change our minds about automatically creating sub-shares. But the benefit of this is too low compared to its complexity and increased migration time (when setting/unsetting the option it could take several minutes for updating)

[PVince81]

We could provide an occ migration tool to reset or auto-accept shares for everyone...

I'm thinking of encryption: we already state that one should pick an encryption type at setup time and stick with it. Any change to it requires decrypting and reencrypting on the CLI.

At this point I think I'd rather vote for "won't fix"

[jvillafanez]

The second proposed solution looks close to #31870 (comment), and will likely have the same effect: users having to reject and then re-accept ? I think this is a worse UX than with proposal one (leaving as it is now).

Yes, users that haven't taken any explicit option (and as a consecuence they don't have their own share entry for the group) will have the share state changed.

I'm not sure if the UX will be worse, but the behaviour for the scenario described in the issue feels more consistent with the new approach. In any case, both solutions will require documentation about this scenario on way or another to explain the behaviour.

The third alternative is to agree to change our minds about automatically creating sub-shares. But the benefit of this is too low compared to its complexity and increased migration time (when setting/unsetting the option it could take several minutes for updating)

I haven't checked the changes we'd need to do, but It shouldn't be too complex, and there shouldn't be any migration time (the change is made on the fly over the group share but not on any user of the group). The problem are big groups because these new entries should be created at the same time the group share is created. We might need to create 1000-10000 new entries, which will affect performance without any doubt.

[phil-davis]

At the moment, if auto-accept is off, when someone shares an item with a big group then the backend code makes ("sends") a notification for every user in the group. I guess that already will take some time.

[PVince81]

The sending part will be moved to a background job when the group is big, scheduled as part of https://github.com/owncloud/enterprise/issues/2616

[PVince81]

moving this to "development" as more discussion and evaluation needed

[PVince81]

Discussed with @pmaier1, current behavior is acceptable. Won't fix.