nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
26.21k stars 3.95k forks source link

Keep unshared calendars in trashbin to enable restoring them for group shares #45783

Open MPStudyly opened 1 month ago

MPStudyly commented 1 month ago

Steps to reproduce

  1. Create a new calendar and a single event within
  2. Share the calendar with a group
  3. As a user within the group the calendar is shared with, go to the calendar edit menu, cancel the subscription
  4. The calendar will be gone (forever)
  5. As the initial user, stop sharing the calendar with the group
  6. Re-share the calendar with the group
  7. As the user who deleted the calendar, check if it reappeared. It won't have.

Expected behavior

When the user sharing the calendar is canceling the share and then re-shares it with the same group, every user in the group should see the calendar. Even (or especially) those who previously unsubscribed from it. Alternatively, keep unsubscribed shared calendars in users trash bin forever, allowing them to restore as long as the calendar is still shared.

Actual behaviour

When I unsubscribe a calendar by accident and miss the short time-frame to cancel the delete operation, there is no chance for me to get the calendar back. It will effectively be gone forever until the user who shared the calendar with a group explicitly re-shares it with me/my user, nullifying all the advantages that come with "share with group".

Calendar app version

4.7.6

CalDAV-clients used

DAVx5, Gnome Integration

Browser

Firefox 126.0.2 64-Bit

Client operating system

Ubuntu 22.04 64-Bit

Server operating system

Debian 11

Web server

Apache

Database engine version

PostgreSQL

PHP engine version

PHP 8.1

Nextcloud version

29.0.1

Updated from an older installed version or fresh install

Updated from an older version

List of activated apps

No response

Nextcloud configuration

No response

Web server error log

No response

Log file

No response

Browser log

No response

Additional info

No response

miaulalala commented 1 month ago

IIRC correctly from https://github.com/nextcloud/server/pull/43117 this use case should have been covered. Do you have access to your database? If so, can you observe the following four states of the table for the resource?

State 1: After Alice (the calendar owner) has shared the Calendar with the group State 2: Bob (sharee) has removed the calendar from themselves State 3: Alice has removed the group share State 4: Alice has reshared the calendar with the group

MPStudyly commented 1 month ago

IIRC correctly from #43117 this use case should have been covered. Do you have access to your database? If so, can you observe the following four states of the table for the resource?

State 1: After Alice (the calendar owner) has shared the Calendar with the group State 2: Bob (sharee) has removed the calendar from themselves State 3: Alice has removed the group share State 4: Alice has reshared the calendar with the group

Unfortunately I can't provide you with the database states out of the box. I'll ask our admin to perform the check/provide the requested info.

miaulalala commented 1 month ago

SELECT * FROM oc_dav_shares where type='calendar' AND resourceid=<your_resource_id>; is the query

MPStudyly commented 4 weeks ago

Thanks for helping out with the query. Our admin didn't react to my request yet, so I ran it myself. I'd probably need a little bit more help though :sweat_smile:

SELECT * FROM oc_dav_shares where type='calendar' AND resourceid=<your_resource_id>; is the query

Pardon this potentially stupid question, but how do I figure the appropriate resource id? I've ran the query without limiting myself to a certain resource id, which yielded just 41 results (way less than expected), so I was able to look at the whole table. Still I don't grasp what I'd see there besides the "access" column which seems to represent some sort of counter?

EDIT: Had some bad sleep last night, took me a while to grasp the concept of what I'm supposed to give you :facepalm: Here you go:

State 1:

98 | principals/groups/operations          | calendar |      2 |        496 | 

State 2:

98 | principals/groups/operations          | calendar |      2 |        496 | 
99 | principals/users/me                   | calendar |      5 |        496 | 

State 3:

99 | principals/users/me                   | calendar |      5 |        496 | 

State 4:

99 | principals/users/me                   | calendar |      5 |        496 | 
101 | principals/groups/operations         | calendar |      2 |        496 | 

So it seems the removal of the share didn't clean the "stop sharing with me" :/ Is there anything else I can you provide with?

miaulalala commented 4 weeks ago

No that is exactly what I need actually, thanks for that.

miaulalala commented 4 weeks ago

The following issue:

User Alice shares a Calendar with resource id 1 with two groups.

Our state:

1 | principals/groups/groupA          | calendar |      1 |        123456 | 
2 | principals/groups/groupB          | calendar |      1 |        123456 | 

Bob is a member of group A. He unshares the calendar from himself.

Our state:

1 | principals/groups/groupA          | calendar |      1 |        123456 | 
2 | principals/groups/groupB          | calendar |      1 |        123456 | 
3 | principals/users/bob                   | calendar |      5 |        123456 | 

Then Alice unshares the Calendar from group B. Since the resource ID is identical for both shares, the only way we can differentiate the shares is by the principal, so we delete table row 2. For Bob, the sharing code doesn't know which group Bob is a member of, so it keeps his unshare for the resource.

Our state:

1 | principals/groups/groupA          | calendar |      1 |        123456 | 
3 | principals/users/bob                   | calendar |      5 |        123456 | 

Since we don't want Bob to have his unshare removed when we unshare a calendar with Group B (which he is not a member of), the code works as intended.

When Alice reshares the calendar with Group B, the code still doesn't know if Bob is a member of that group, so it doesn't touch the unshare either.

If we switch out Bob's Group membership, i. e. he is part of Group B instead of Group A, we get the exact issue you're facing. But since we don't know Bob's Group membership, we can't do anything about it.

What Alice can do, is share the Calendar again directly with Bob, which is the only way we can restore that calendar without breaking other group (un)shares.

Unfortunately that is the current limitation of not expanding group memberships when managing shares.

Additionally, sharing a calendar with a user that is a member of both groups, for example is also an issue - what do we do in case Bob is a member of both group A and B, unshares the calendar, and Alice then unshares the calendar for one group but not the other? Do we keep his unshare entry or delete it?

I hope this makes sense, sorry I don't have better news!

MPStudyly commented 3 weeks ago

I hope this makes sense, sorry I don't have better news!

Big oof... :( As far as I understand the calendar might not be the only app affected by this limitation? Regarding the calendar: Could my alternative solution be viable as an intermediate solution? Also, could you provide me with a query to clear any custom unshared calendars, so they are available again? Directly sharing with a person unfortunately is not an option for us :see_no_evil:

Additionally, sharing a calendar with a user that is a member of both groups, for example is also an issue - what do we do in case Bob is a member of both group A and B, unshares the calendar, and Alice then unshares the calendar for one group but not the other? Do we keep his unshare entry or delete it?

I guess deleting the unshare entry is the more viable approach, as unsharing and resharing probably won't happen often without a good reason. Alternatively, if unshared calendars remain in the trash bin forever, keeping the entry would be fine as well. The only requirement is really just to keep it clear to the user I'd say.

miaulalala commented 3 weeks ago

If you simply want all unshares to go away so people can start fresh you can do the following query:

DELETE FROM oc_dav_shares where type = 'calendar' AND access = 5;

if unshared calendars remain in the trash bin forever, keeping the entry would be fine as well. The only requirement is really just to keep it clear to the user I'd say.

Now there's an option I haven't thought about! Let me think this through if it is a viable option.

MPStudyly commented 2 weeks ago

If you simply want all unshares to go away so people can start fresh you can do the following query:

DELETE FROM oc_dav_shares where type = 'calendar' AND access = 5;

Thx for the query! I've noticed an additional detail though: After performing the query and unsharing the calendar (as sharing user) with me explicitly besides the group, a new entry with access = 5 was created. As we were looking at this issue in the perspective of the receiving user, I wasn't expecting the sharing user to be able to "block" implicit sharing (as part of a group) of a certain resource forever.

if unshared calendars remain in the trash bin forever, keeping the entry would be fine as well. The only requirement is really just to keep it clear to the user I'd say.

Now there's an option I haven't thought about! Let me think this through if it is a viable option.

Great to hear you consider this solution :D Is there anything I can help you with?

miaulalala commented 1 week ago

If you simply want all unshares to go away so people can start fresh you can do the following query: DELETE FROM oc_dav_shares where type = 'calendar' AND access = 5;

Thx for the query! I've noticed an additional detail though: After performing the query and unsharing the calendar (as sharing user) with me explicitly besides the group, a new entry with access = 5 was created. As we were looking at this issue in the perspective of the receiving user, I wasn't expecting the sharing user to be able to "block" implicit sharing (as part of a group) of a certain resource forever.

if unshared calendars remain in the trash bin forever, keeping the entry would be fine as well. The only requirement is really just to keep it clear to the user I'd say.

Now there's an option I haven't thought about! Let me think this through if it is a viable option.

Great to hear you consider this solution :D Is there anything I can help you with?

Not really unless you want to develop this feature :wink: ? I will bring this to our project management to see if I can get some time to work on this. Can't make any promises unfortunately!

MPStudyly commented 4 days ago

If you simply want all unshares to go away so people can start fresh you can do the following query: DELETE FROM oc_dav_shares where type = 'calendar' AND access = 5;

Thx for the query! I've noticed an additional detail though: After performing the query and unsharing the calendar (as sharing user) with me explicitly besides the group, a new entry with access = 5 was created. As we were looking at this issue in the perspective of the receiving user, I wasn't expecting the sharing user to be able to "block" implicit sharing (as part of a group) of a certain resource forever.

if unshared calendars remain in the trash bin forever, keeping the entry would be fine as well. The only requirement is really just to keep it clear to the user I'd say.

Now there's an option I haven't thought about! Let me think this through if it is a viable option.

Great to hear you consider this solution :D Is there anything I can help you with?

Not really unless you want to develop this feature 😉 ? I will bring this to our project management to see if I can get some time to work on this. Can't make any promises unfortunately!

If my employer would care enough about this (especially as we now have a workaround :facepalm:) and allow me to have a deeper look, I could try, but I highly doubt I'll get some time approved :(

No worries, I know how this works! I'm already glad to hear this is considered :) You can hide this post, as this doesn't really bring anything new to the table :sweat_smile:

miaulalala commented 4 days ago

If you simply want all unshares to go away so people can start fresh you can do the following query: DELETE FROM oc_dav_shares where type = 'calendar' AND access = 5;

Thx for the query! I've noticed an additional detail though: After performing the query and unsharing the calendar (as sharing user) with me explicitly besides the group, a new entry with access = 5 was created. As we were looking at this issue in the perspective of the receiving user, I wasn't expecting the sharing user to be able to "block" implicit sharing (as part of a group) of a certain resource forever.

if unshared calendars remain in the trash bin forever, keeping the entry would be fine as well. The only requirement is really just to keep it clear to the user I'd say.

Now there's an option I haven't thought about! Let me think this through if it is a viable option.

Great to hear you consider this solution :D Is there anything I can help you with?

Not really unless you want to develop this feature 😉 ? I will bring this to our project management to see if I can get some time to work on this. Can't make any promises unfortunately!

If my employer would care enough about this (especially as we now have a workaround 🤦) and allow me to have a deeper look, I could try, but I highly doubt I'll get some time approved :(

No worries, I know how this works! I'm already glad to hear this is considered :) You can hide this post, as this doesn't really bring anything new to the table 😅

It's been added to the board! :partying_face: