Skip to content

Conversation

@kesselb
Copy link
Contributor

@kesselb kesselb commented Apr 8, 2025

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

  • Review
  • Check CardDav
  • CI
  • Backports

Checklist

@kesselb kesselb added bug 2. developing Work in progress labels Apr 8, 2025
@kesselb kesselb added this to the Nextcloud 32 milestone Apr 8, 2025
@kesselb kesselb self-assigned this Apr 8, 2025
@kesselb kesselb force-pushed the bug/49834/calendar-unsharing branch 5 times, most recently from af9efb7 to 0198174 Compare April 11, 2025 12:39
* '{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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

owner-displayname is only set if we have displayname:

private function addOwnerPrincipalToCalendar(array $calendarInfo): array {
$ownerPrincipalKey = '{' . \OCA\DAV\DAV\Sharing\Plugin::NS_OWNCLOUD . '}owner-principal';
$displaynameKey = '{' . \OCA\DAV\DAV\Sharing\Plugin::NS_NEXTCLOUD . '}owner-displayname';
if (isset($calendarInfo[$ownerPrincipalKey])) {
$uri = $calendarInfo[$ownerPrincipalKey];
} else {
$uri = $calendarInfo['principaluri'];
}
$principalInformation = $this->principalBackend->getPrincipalByPath($uri);
if (isset($principalInformation['{DAV:}displayname'])) {
$calendarInfo[$displaynameKey] = $principalInformation['{DAV:}displayname'];
}
return $calendarInfo;

But we use it without an isset before and thus, marking it as optional, will trigger some new psalm warnings.

@kesselb kesselb force-pushed the bug/49834/calendar-unsharing branch from c79d01f to 6867a31 Compare April 11, 2025 13:20
@kesselb kesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 11, 2025
@kesselb kesselb marked this pull request as ready for review April 11, 2025 13:33
@kesselb kesselb requested review from Altahrim, ChristophWurst, icewind1991 and nfebe and removed request for a team April 11, 2025 13:33
@kesselb kesselb force-pushed the bug/49834/calendar-unsharing branch from 6867a31 to a70bc08 Compare April 14, 2025 16:22
@kesselb kesselb force-pushed the bug/49834/calendar-unsharing branch 3 times, most recently from a58aefa to 811af48 Compare April 15, 2025 16:09
@kesselb kesselb force-pushed the bug/49834/calendar-unsharing branch from 811af48 to cb72b02 Compare May 9, 2025 12:24
@ChristophWurst
Copy link
Member

@SebastianKrupinski please test and review this PR. Thanks!

Copy link
Contributor

@SebastianKrupinski SebastianKrupinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested Works.

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 14, 2025
kesselb added 2 commits May 14, 2025 09:03
- 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 <[email protected]>
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 <[email protected]>
@kesselb kesselb force-pushed the bug/49834/calendar-unsharing branch from cb72b02 to 023b98c Compare May 14, 2025 07:03
@kesselb kesselb enabled auto-merge May 14, 2025 07:03
@kesselb kesselb merged commit 498c57f into master May 14, 2025
194 of 198 checks passed
@kesselb kesselb deleted the bug/49834/calendar-unsharing branch May 14, 2025 09:00
@kesselb kesselb added the pending documentation This pull request needs an associated documentation update label May 14, 2025
@kesselb
Copy link
Contributor Author

kesselb commented May 14, 2025

/backport to stable31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Individual calendar share removal leads to group unshare [Bug]: (CalDav|CardDav) unsharing calendar as owner leads to wrong state

6 participants