Introduce own method for calendar unsharing#52046
Merged
Conversation
af9efb7 to
0198174
Compare
kesselb
commented
Apr 11, 2025
| * '{urn:ietf:params:xml:ns:caldav}schedule-calendar-transp': ScheduleCalendarTransp, | ||
| * '{DAV:}displayname': string, | ||
| * '{urn:ietf:params:xml:ns:caldav}calendar-timezone': ?string, | ||
| * '{http://nextcloud.com/ns}owner-displayname': string, |
Contributor
Author
There was a problem hiding this comment.
owner-displayname is only set if we have displayname:
server/apps/dav/lib/CalDAV/CalDavBackend.php
Lines 3522 to 3535 in 2ea3491
But we use it without an isset before and thus, marking it as optional, will trigger some new psalm warnings.
c79d01f to
6867a31
Compare
6867a31 to
a70bc08
Compare
kesselb
commented
Apr 15, 2025
apps/dav/tests/integration/DAV/Sharing/CalDavSharingBackendTest.php
Outdated
Show resolved
Hide resolved
a58aefa to
811af48
Compare
ChristophWurst
approved these changes
Apr 25, 2025
811af48 to
cb72b02
Compare
Member
|
@SebastianKrupinski please test and review this PR. Thanks! |
SebastianKrupinski
approved these changes
May 14, 2025
Contributor
SebastianKrupinski
left a comment
There was a problem hiding this comment.
Tested Works.
- Introduces a `unshare` method in `CalDavBackend` to handle user unshares. - Implements check to determine if unshare entry is needed based on group/circle membership. - Ensures `updateShares` is only used when the calendar owner manages shares. - Resolves issue where unsharing a calendar as owner created an unshare entry in `oc_dav_shares`. Related PRs: - #43117 - #47737 Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
Before: Find all entries in `dav_shares` with `access = 5` for the user's principal, as well as group and circle memberships. After: Find all entries in `dav_shares` with `access = 5` solely for the user's principal. Future support for unsharing group or circle principals could be considered as a feature enhancement. Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
cb72b02 to
023b98c
Compare
st3iny
approved these changes
May 14, 2025
Contributor
Author
|
/backport to stable31 |
2 tasks
2 tasks
Closed
8 tasks
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I'm sorry the PR ended up being much larger than expected. The issue arose because managing calendar shares as an owner and unsharing a calendar as an owner used the same code. This led to a situation where removing an individual calendar share as an owner, while having at least one group share, created an unshare item for the individual user and made the calendar inaccessible.
I reworked the implementation to use a separate method for unsharing a user and reverted the changes for the "manage shares as calendar owner" method. We decided to implement an occ command to unset calendar unshares instead of a migration that drops all unshares. Additionally, there's now a command to list the calendar shares.
The second commit is a hardening of the unsharing logic. Previously, we considered all principal URIs (users, groups, and circles) when looking for unshares. Now, it only considers the user's principal URI, as we only support unshares for individuals but not for groups or teams yet.
Steps to reproduce are available at: #49834
The events are, among other things, used to write the activity, so please keep an eye on whether the activities look correct while testing.
TODO
Checklist