fix(dav): default reminder on received invitations + preserve recipie…#61162
Open
ndo84bw wants to merge 1 commit into
Open
fix(dav): default reminder on received invitations + preserve recipie…#61162ndo84bw wants to merge 1 commit into
ndo84bw wants to merge 1 commit into
Conversation
…nt VALARMs Co-Authored-By: Lucas Ferreira da Silva <lufer22@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Nico Donath <ndo84bw@gmx.de>
5 tasks
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
When a user receives a calendar invitation via Sabre's iTip scheduling, the organizer's VALARMs are stripped server-side at
apps/dav/lib/CalDAV/Schedule/Plugin.php(security baseline - the organizer must not be able to drive notifications on the recipient's devices). The recipient's own configured default reminder is not applied either, so the event arrives without any VALARM and the recipient gets no reminder notification (nextcloud/calendar#6315). A second, related gap surfaces in the same code path: when the recipient manually adds a VALARM to an invited event and the organizer later modifies the event, Sabre'sBroker::processMessageRequest()discards every component of the existing local copy and replaces it with the (VALARM-stripped) incoming one - the recipient's manually-added VALARMs are lost on every organizer update.This PR addresses both gaps in
Plugin::scheduleLocalDelivery(), between the existing strip block and the call toparent::scheduleLocalDelivery():defaultReminderFullDayfor all-day events /defaultReminderPartDayfor timed events → legacydefaultReminder→ app-levelcalendar.defaultReminder→'none'. Gated by a new per-user togglecalendar:applyDefaultReminderToInvitations(default'yes'), so users who don't want this can opt out and admins who don't want it as a fleet default can flip the app value.RECURRENCE-ID(master and each override individually). The Sabre broker then writes the merged shape into the recipient's calendar.Both paths skip ROOM/RESOURCE recipients and the organizer themselves. REPLY/CANCEL/REFRESH methods bypass the new logic entirely (and RFC 5546 forbids VALARM on those anyway). The existing VALARM strip block was also fixed in the same change - it operated on only the first VEVENT of the incoming message, so for recurring REQUESTs with
RECURRENCE-IDoverrides the organizer VALARMs on the overrides survived. This stripping is now applied per-component.The implementation reuses lufer22's
secondsToIso8601DurationandcreateAlarmshape from #48226 (Co-Authored-By trailer), but resolves recipient principals via$aclPlugin->getPrincipalByUri()instead of the non-uniqueuserManager->getByEmail()flagged in @SebastianKrupinski's review of that PR, extends the resolution to the typed (PartDay/FullDay) defaults, adds the user-toggle gate, and adds the preserve-on-update path lufer22's PR did not have. We don't close #48226 - that PR is the structural ancestor of this one and should stay discoverable for context. A short courtesy comment is being posted there.Why this is RFC-compliant
RFC 5545 defines VALARM structurally but is silent on receive-side semantics. RFC 5546 (iTIP) allows VALARM in REQUEST (0+) and forbids it in REPLY/CANCEL/REFRESH (the new logic respects both). RFC 6638 (CalDAV scheduling extensions) §3.2.2.1 is the load-bearing argument: attendees are explicitly permitted to "add, modify, or remove any 'VALARM' iCalendar components" without scheduling side-effects. Server-side default injection on behalf of the attendee, and preserving the attendee's own VALARMs across organizer updates, are both things the attendee is allowed to do on their own copy - the server does them earlier and automatically.
TODO
Checklist
3. to review, feature component)stable32)Tested
Live (manual, on a Nextcloud 33.0.3.2 instance, two real CalDAV users, Thunderbird and the calendar web UI; reminder defaults set through the calendar web UI, not occ):
First-receipt injection:
-54000/-PT15H): VALARM with the matching day-relative TRIGGER appears on the recipient side.Preserve-on-update:
RECURRENCE-IDoverride (questionable, but pre-existing and unrelated to this change). The organizer then renames the whole series. Result: occurrences 1/2/4 get the new title and keep their reminders, and the recipient's reminders on both the master and the occurrence-3 override are preserved. Occurrence 3 keeps the old title, because a master-level rename does not propagate to an override that already exists - that title-propagation gap is the separate out-of-scope item listed below, not a regression of this change.Round-trip / replies:
Counter-tests:
applyDefaultReminderToInvitations=no(per-user, set viaocc- no web UI yet): a fresh invitation arrives without any injected VALARM; existing VALARMs on prior invitations remain.Blocker found and fixed during live testing: this change adds a 4th constructor argument (
CalDavBackend) toSchedule\Plugin, butInvitationResponseServer(the helper that builds the plugin to process incoming iTip responses) was still constructing it with only 3 arguments, which raised anArgumentCountError. It was observed in the live log on the NC Mail app'sManager::handleIMipMessage()auto-processing path. The sameInvitationResponseServeralso backs the Accept/Decline links inside iMIP emails, so that path was broken as well (not separately exercised in this round of testing). Fixed by passing the 4th argument at theInvitationResponseServercall site; the error appears in the log before the fix and is gone after it.Unit (added in
apps/dav/tests/unit/CalDAV/Schedule/PluginTest.php):testSkipsNonRequest- REPLY/CANCEL/REFRESH never run the reminder hook.testStripsOrganizerValarmsFromAllComponents- verifies the strip fix for recurring REQUESTs.testFirstReceiptInjectsTypedPartDayDefault--PT15Mfrom typed PartDay setting.testFirstReceiptInjectsTypedFullDayDefault--PT1Hfrom typed FullDay setting for an all-day event.testFirstReceiptFallsBackToLegacyAndAppValue- typednone+ legacynone→ admin app value.testFirstReceiptSkippedWhenToggleOff-applyDefaultReminderToInvitations=noskips inject.testPreservesExistingValarmsOnUpdate- recipient's existing VALARM survives organizer update.testExistingCopyWithoutAlarmsSkipsInjection- local copy with zero alarms → no re-inject (deleted stays deleted).testPreservesPerOverrideValarms- per-RECURRENCE-IDmatching for master + override.testSecondsToIso8601Duration/testPrincipalUriToUserId- direct helper coverage via reflection.Not tested locally:
composer test(PHPUnit bootstrap requires an installed-and-configured DB which this worktree doesn't have); CI will validate.Out of scope (explicit)
nextcloud/calendar. Out of scope here; follow-up calendar-app PR once this lands.RECURRENCE-IDoverride, that override keeps its old values. This is what T4 shows for the title (occurrence 3 keeps the old name). It is a pre-existing behaviour, separate from the reminder bug fixed here, and is left as a follow-up.apps/dav/lib/CalDAV/TipBroker.php, tracked in PARTSTAT change on a single occurrence of a recurring event triggers METHOD:REQUEST re-invite to all other attendees #60452, fix(dav): suppress iTip REQUEST for PARTSTAT-only occurrence overrides #60536 and [Bug]: Accepting a recurring series via Thunderbird leaves the moved occurrence at NEEDS-ACTION #61114. Not related to reminders.Trade-offs
applyDefaultReminderToInvitationsdefaults to'yes': chosen so the behaviour requested in The default reminder isn't added to the attendee calendar#6315 is active without each user having to opt in first. Easy to flip to'no'if maintainers prefer the safer migration default - happy to change it during review.AI (if applicable)