Skip to content

fix(dav): default reminder on received invitations + preserve recipie…#61162

Open
ndo84bw wants to merge 1 commit into
nextcloud:masterfrom
ndo84bw:fix/6315-default-reminder-and-preserve-on-update
Open

fix(dav): default reminder on received invitations + preserve recipie…#61162
ndo84bw wants to merge 1 commit into
nextcloud:masterfrom
ndo84bw:fix/6315-default-reminder-and-preserve-on-update

Conversation

@ndo84bw

@ndo84bw ndo84bw commented Jun 10, 2026

Copy link
Copy Markdown

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's Broker::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 to parent::scheduleLocalDelivery():

  • First-receipt path - when the recipient has no local copy of this UID yet, inject the recipient's configured default reminder. The selection follows the calendar app's existing fallback chain: defaultReminderFullDay for all-day events / defaultReminderPartDay for timed events → legacy defaultReminder → app-level calendar.defaultReminder'none'. Gated by a new per-user toggle calendar: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.
  • Update path - when the recipient already has a local copy, copy every VALARM from the existing copy onto the incoming components, matched per-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-ID overrides the organizer VALARMs on the overrides survived. This stripping is now applied per-component.

The implementation reuses lufer22's secondsToIso8601Duration and createAlarm shape from #48226 (Co-Authored-By trailer), but resolves recipient principals via $aclPlugin->getPrincipalByUri() instead of the non-unique userManager->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

  • Maintainer review (especially the user-toggle default value).

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Screenshots before/after for front-end changes (N/A - backend only)
  • Documentation (manuals or wiki) has been updated or is not required (will follow once the maintainer signs off on the toggle's default)
  • Backports requested where applicable (ex: critical bugfixes)
  • Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
  • Milestone added for target branch/version (ex: 32.x for 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:

  • T1 - Timed single event, recipient's part-day default set in the web UI: VALARM with the matching TRIGGER appears on the recipient side (web UI + Thunderbird).
  • T2 - All-day single event, recipient's all-day default set in the web UI (these are day-relative, e.g. "1 day before at 9:00" stores -54000 / -PT15H): VALARM with the matching day-relative TRIGGER appears on the recipient side.

Preserve-on-update:

  • T3 - Organizer re-times a single event the recipient had already accepted: a fresh invitation goes out, the recipient's existing reminders are still present afterwards.
  • T4 - Recurring series where the recipient has detached one occurrence. Setup: the recipient adds an extra reminder to occurrence 3 only. Under current Nextcloud behaviour, editing a single occurrence's reminder turns that occurrence into its own RECURRENCE-ID override (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.
  • T5 - Recurring series, organizer updates occurrences 1-4 except an overridden 3: the recipient's VALARMs on master and on each existing override survive every update without accumulation (this is the cycle that previously produced 4-6 VALARMs per VEVENT after repeated updates).

Round-trip / replies:

  • R1 - The injected reminder does not interfere with the normal accept flow. The recipient opens a fresh invitation that already carries the injected default reminder and accepts it (tested both in the calendar web UI and via Thunderbird's accept button). The organizer then sees the acceptance in the web UI and receives the REPLY mail as usual. The injected VALARM rides along on the recipient's copy without affecting the REPLY.
  • R2 - Recipient deletes the injected reminder, organizer updates the event again: the deleted reminder is not re-injected (existence-based first-receipt detection, not "zero alarms").

Counter-tests:

  • G1 - applyDefaultReminderToInvitations=no (per-user, set via occ - no web UI yet): a fresh invitation arrives without any injected VALARM; existing VALARMs on prior invitations remain.
  • G2 - Recipient has no reminder default set at all (web UI "No reminder", and the keys fully unset): no VALARM injected on a fresh invitation.
  • G3 - Organizer invites themselves (self-invite) with their own reminder default set: no injected/duplicated reminder; the recipient policy skips the organizer's own copy.
  • G4 - REPLY (recipient declines) and CANCEL (organizer cancels the event): the new reminder logic does not run, because it is gated to REQUEST messages only. No reminder is injected and the organizer's VALARMs stay stripped. A reminder the recipient had already added to an event they previously accepted is correctly kept when they later decline (the recipient owns the VALARMs on their own copy).

Blocker found and fixed during live testing: this change adds a 4th constructor argument (CalDavBackend) to Schedule\Plugin, but InvitationResponseServer (the helper that builds the plugin to process incoming iTip responses) was still constructing it with only 3 arguments, which raised an ArgumentCountError. It was observed in the live log on the NC Mail app's Manager::handleIMipMessage() auto-processing path. The same InvitationResponseServer also 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 the InvitationResponseServer call 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 - -PT15M from typed PartDay setting.
  • testFirstReceiptInjectsTypedFullDayDefault - -PT1H from typed FullDay setting for an all-day event.
  • testFirstReceiptFallsBackToLegacyAndAppValue - typed none + legacy none → admin app value.
  • testFirstReceiptSkippedWhenToggleOff - applyDefaultReminderToInvitations=no skips inject.
  • testPreservesExistingValarmsOnUpdate - recipient's existing VALARM survives organizer update.
  • testExistingCopyWithoutAlarmsSkipsInjection - local copy with zero alarms → no re-inject (deleted stays deleted).
  • testPreservesPerOverrideValarms - per-RECURRENCE-ID matching 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)

Trade-offs

  • Wholesale preserve, not delta: on update, every existing VALARM from the recipient's matching component is copied. Per-occurrence attendee VALARM customisations that the recipient explicitly diverged from the master are preserved as-is. Good enough; a delta approach would need a pre-update snapshot of the recipient's intent.
  • applyDefaultReminderToInvitations defaults 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)

  • The content of this PR was partly or fully generated using AI

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The default reminder isn't added to the attendee

1 participant