-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat: federated calendar shares #54127
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
Conversation
41a55e7 to
47fd0be
Compare
|
I get the architecture logic but having a one-hour sync interval between changes is quite a bumper, and in this case it's far more important for usability than the regular cached subscriptions sync interval. Instead of going for a pull model, why not go for a push model? Queue a |
7b4c8da to
4a1252f
Compare
That was my idea initially but then I ran out of time. We could use |
|
Done, I implemented a simple push sync to make the feature a bit more useful. The periodic job will pick up all other calendars. |
|
I'm sorry for creating this monster. Have fun reviewing ... 😅 |
|
|
||
| $this->dispatcher->dispatchTyped(new CachedCalendarObjectCreatedEvent($calendarId, $subscriptionRow, [], $objectRow)); | ||
| } elseif ($calendarType === self::CALENDAR_TYPE_FEDERATED) { | ||
| // TODO: implement custom event for federated calendars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still todo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is still a TODO left for the future. I didn't implement the events as there is no use case for them yet.
|
Reviewed a tiny part for today, will continue next week, some random questions popping:
|
ChristophWurst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense conceptually. Thanks a lot for the architecture overview
Code mostly makes sense. A bit much to review and process at a time 🙈
SebastianKrupinski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some other thought and things,
Not sure what the reasons was to hard wire the federation in to the DAV stack (CalendarHome, Provider and Root), I think doing this as a DAV Calendar Plugin would have been much simpler. I think it would have been more modular, easier to maintain and control but this not a blocker.
One thing I did find in my testing was that after unsharing the calendar all the calendar object are still present in the database, of the user the calendar was shared with
| if (isset($status[200])) { | ||
| $absoluteUrl = $this->prepareUri($url, $resource); | ||
| $vCard = $this->download($absoluteUrl, $username, $sharedSecret); | ||
| $this->atomic(function () use ($calendar, $objectUri, $vCard): void { | ||
| $existingObject = $this->backend->getCalendarObject($calendar->getId(), $objectUri, CalDavBackend::CALENDAR_TYPE_FEDERATED); | ||
| if (!$existingObject) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a blocker, but more of a this might become a issue when syncing large calendars. I noticed while testing with a large calendar (12k) that if the process is interrupted, for what ever reason, the sync start back at the beginning, even after it already syncing 2k of entries.
Since syncing large calendars can take a long time (12k = 3hours on my machine) and causes thousands of requests this can get interrupted but firewall rate limits, slow connection, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is a problem. However, this has to be solved on the CalDAV protocol level as I didn't implement a custom one.
AFAIK, there is no way to continue an interrupted initial sync according to the RFCs.
apps/dav/lib/CalDAV/Federation/FederatedCalendarSyncService.php
Outdated
Show resolved
Hide resolved
Not at all because both behave very differently. Calendar federation will cause a local copy of all remote events to be synced. File federation will only request the data from the other instance if required. So I don't think they can be compared in a meaningful way.
Yes, for now it only supports users. Adding support for groups should be straight forward and not too much effort.
I did not look into that. I have no idea about circle federation and can give no estimate on the work that would need to be done. |
|
Tested again working good |
4397af3 to
f285ec9
Compare
|
I rebased to fix conflicts. Should be all green now. |
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
f285ec9 to
b7dc720
Compare
Summary
The beast ...
Terminology
Architecture
Sharing works by leveraging our on-instance sharing via
CalDavBackend(and appending the_shared_by_<uid>suffix). The remote user is authenticated in Sabre through a "virtual" principal inprincipals/remote-users/<base64-encoded-cloud-id>with whom an outgoing federated calendar is shared "locally". The cloud id needs to be encoded as it might contain slashes, for example, when the remote instance is hosted inside a web root. The outgoing shared calendars are hosted inside a separate calendar root inremote-calendars/<base64-encoded-cloud-id>/<calendar_name>_shared_by_<sharer>on the host instance. The remote instance will send all sync requests to this address and authenticate using the remote user's cloud id and a shared secret (token) via HTTP basic auth.A federation provider was implemented to send (and receive) shares across federated instances. The remote instance will track the incoming federated calendar inside a new table
oc_calendars_federated. The host instance will create a regular DAV share insideoc_dav_sharesin case the remote user logs in and syncs the calendar. I had to add a new columntokentooc_dav_sharesin order to track the auth token (shared secret) for the remote user (so that the sharer is able to revoke individual shared calendars).If a calendar object is changed on the host instance, a cloud notification is sent to all remote instances with which the calendar is shared with. Remote servers will then queue a sync job at the next cron interval to sync the affected calendar. This also has the benefit of nicely debouncing the sync requests. As a backup, there is an hourly background job which syncs all remaining calendars (starting with the least recently synced calendar).
Please let me know in case there are questions or further elaboration is needed.
How to test?
occ federation:sync-addressbooks) to make finding the remote users easier in the sharing search.Run the following command on the remote instance:
occ federation:sync-calendarsNow, you should be able to see all the events in the calendar on the remote instance.
TODO
Chunk results (reuse Hamza's truncation?)-> deferredConsider multi-get to bundle ICS download-> deferredChecklist