Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CalDavBackend, implement SharingSupport interface. #36766

Conversation

rotdrop
Copy link
Contributor

@rotdrop rotdrop commented Feb 17, 2023

Summary

Implement the \Sabre\CalDAV\Backend\SharingSupport in \OCA\DAV\CalDAV\CalDavBackend. The starting comment of the linked issue #26668 suggests that this might be the first step in supporting invitations for events in shared writeable calendars. However, after coding #36756 I have now the impression that the CalDAV sharing interface is orthogonal to sending invitations for events in shared calendars. It is nice to have and may help the calendar frontend (see nextcloud/calendar#4983). However, implementing the "SharingSupport" interface in the CalDAV backend does not bring us closer to sending invitations.

TODO

  • testing

Checklist

$principalUri = $calendarInfo['principaluri'];
$calendarUri = $calendarInfo['uri'];
$shareable = new Calendar($this, $calendarInfo, \OCP\Util::getL10N('dav'), $this->config, $this->logger);
$this->updateShares($shareable, $additions, $removals);

Check notice

Code scanning / Psalm

ArgumentTypeCoercion

Argument 2 of OCA\DAV\CalDAV\CalDavBackend::updateShares expects list<array{commonName: string, href: string, readOnly: bool}>, but parent type list<array{commonName: ""|mixed, href: string, readOnly: bool}> provided
@rotdrop rotdrop force-pushed the feature/implement-caldav-sharing-support branch from 428e3d4 to 1c6f5fc Compare February 21, 2023 14:23
Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

First of all, thanks for the effort. As #26668 hints, this might be valuable in the long term.

In the short term, though, it shouldn't bring much: the exposed API is very similar to what we currently have with the oc-resource-sharing custom plugin, which was based on Sabre's experiments at the time, themselves based on CalendarServer custom's spec.

Since all of @evert's drafts have long expired and Apple has abandoned CalendarServer, it's not sure if it's worth it to pursue in this direction until we have hints of the ecosystem going into the same direction (it's been 7 years since the drafts publication, and 4 years since the last mention of this in the CalConnect/IETF discussions that I can find).

Another thing is that SharingSupport is very linked to NotificationSupport (as seen in the Sharing plugin calling sharingReply (as derived from the CS origin).
The notification plugin even says CalDAV-sharing requires it. So that's a whole other thing, even if it would bring nice to have features, such as macOS/iOS calendar clients eventual compatibility.

@@ -436,6 +441,9 @@ public function getCalendarsForUser($principalUri) {
$calendar = $this->addOwnerPrincipalToCalendar($calendar);
$calendar = $this->addResourceTypeToCalendar($row, $calendar);

$calendar['{DAV:}share-resource-uri'] = '/ns/share/' . $calendar['id'];
$calendar['{DAV:}share-access'] = $row['access'];
Copy link
Member

Choose a reason for hiding this comment

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

issue (blocking): The access values we use are (see ACCESS_PUBLIC in this file and ACCESS_* in OCA\DAV\DAV\Sharing\Backend) different from the ones declared Sabre\DAV\Sharing\Plugin. You need to convert them.

@@ -362,6 +364,9 @@ public function getCalendarsForUser($principalUri) {
$calendar = $this->addOwnerPrincipalToCalendar($calendar);
$calendar = $this->addResourceTypeToCalendar($row, $calendar);

$calendar['{DAV:}share-resource-uri'] = '/ns/share/' . $calendar['id'];
Copy link
Member

Choose a reason for hiding this comment

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

thought: Not a fan of this. The PDO backend does the exact same, but is this form even a valid URI?

Comment on lines +2931 to +2932
// updateShares() needs a IShareable, i.e. a Calendar object. This is
// really hacky now ... and inefficient ...
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: This restriction and the whole current implementation of updateShares & getShares is oriented to match the current OCA\DAV\DAV\Sharing\Plugin way of handling shares.

Sharing addressbooks would be the only reason to keep it if we have this instead, so if we manage to overcome that, we could refactor the whole thing.

$result[] = new Sharee([
'href' => $share['href'],
'access' => $share['readOnly'] ? \Sabre\DAV\Sharing\Plugin::ACCESS_READ : \Sabre\DAV\Sharing\Plugin::ACCESS_READWRITE,
'inviteStatus' => \Sabre\DAV\Sharing\Plugin::INVITE_ACCEPTED,
Copy link
Member

Choose a reason for hiding this comment

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

thought: This makes me want invite support. :D

@blizzz blizzz added this to the Nextcloud 29 milestone Nov 23, 2023
@blizzz blizzz added the 2. developing Work in progress label Nov 23, 2023
This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 31 milestone Aug 14, 2024
@sorbaugh
Copy link
Contributor

sorbaugh commented Aug 16, 2024

@rotdrop following @tcitworld 's comment, it seems like this PR won't be pursued any longer? Please let me know if you'd like us to reopen this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants