Write access over CalDAV by Jaggob · Pull Request #7655 · nextcloud/deck

Jaggob · 2026-02-18T13:36:12Z

Summary

This PR implements a first small part of #2399: updating existing Deck cards via CalDAV.

The scope is intentionally limited to PUT on existing card VTODOs. Create, delete, move, tags/categories, attendees, recurrence, attachments, and lossless raw VTODO round-tripping are left for follow-up PRs.

Implemented

Existing Deck cards can be updated via CalDAV PUT.
Write ACLs are exposed only for users with board edit permissions.
Stack VTODOs remain read-only.
CREATE and DELETE remain forbidden.
Supported VTODO mapping:
- SUMMARY -> card title
- DESCRIPTION -> card description
- DUE -> due date
- STATUS, COMPLETED, PERCENT-COMPLETE -> done state
Unsupported VTODO fields are ignored and not stored.

Not Included

Intentionally out of scope:

Creating new cards (needs a clear CalDAV object identity / href strategy)
Deleting cards
Moving cards between stacks
Mapping RELATED-TO
Syncing CATEGORIES / Deck tags
Handling ATTENDEE, RRULE, attachments, or raw VTODO data

Unsupported properties may therefore be accepted on PUT, but they will not be returned by the next GET.

Notes

DELETE is deferred because some clients implement moves as DELETE + CREATE; allowing delete while create is forbidden could make failed moves destructive.

Testing

Added unit tests for update mapping, done-state handling, read-only stack objects, forbidden create/delete, ACL behavior, ETag refresh, invalid payloads, stream PUT payloads, exception mapping, and permission-cache behavior.

Manually tested with Thunderbird 150.0.1 and macOS Reminders 26.4.1

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36747a6fd3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

grnd-alt

Hey, thanks for your contribution, would be nice to get this feature in. I have some smaller code-style remarks, that are partly up for discussion.

The most important remark is that one though: https://github.com/nextcloud/deck/pull/7655/changes#r2840797696

I think this should be oriented to work with tasks as much as possible before merging.

grnd-alt · 2026-02-19T15:46:43Z

+		$lastModified->setTimestamp($lastModifiedTs);
+		$event->DTSTAMP = $lastModified;
+		$event->{'LAST-MODIFIED'} = $lastModified;
+		$event->STATUS = 'NEEDS-ACTION';


why is it always needs-action? Isn't it fine to have no status as the default?

My concern was that leaving the synthetic list VTODOs without an explicit status could lead some clients to infer or write back their own status, which would make the behavior less consistent across clients. I did not test that in depth though, so this was mainly a defensive choice.

grnd-alt · 2026-02-23T13:12:32Z

 		return $result;
 	}

 	public function createFile($name, $data = null) {


I am not entirely fluent with the webdav protocol, but when testing this with nextcloud tasks and thunderbird the behavior was not as I would've expected it.
I could not investigate thunderbirds requests, but for nc tasks the createFile was called with a filename and after creation the same file was re-requested to display the task, the name seems not to be respected so it is not served again under that name leading to a 404.

Thanks for the review and this good catch! Somehow missed that in my testings thanks to tolerant clients.

Fixed in f95476c47:
Created cards now persist the client-provided DAV href for non-canonical names, so follow-up requests to the same resource name no longer fail with a 404 after creation. Existing cards still fall back to the canonical card-<id>.ics naming.

Fixed in dea36cf8c:
While testing that change with Thunderbird moves, it also became clear that in per_list_calendar mode Thunderbird can send the target calendar URL while still keeping the old RELATED-TO in the payload. The follow-up fix now prefers the target list from the destination calendar in that mode, so the trailing source delete no longer removes the moved card.

I also added test coverage for stored href resolution, custom-href creation, and same-board stack moves via the target calendar.

An alternative would have been to keep a fully server-generated canonical href model and add extra mapping/alias persistence for client-provided object names. That would preserve uniform resource names, but it also adds noticeably more persistence and lookup complexity.

For this PR I preferred the smaller approach of persisting a single stable DAV href per card. That means object names can be client-shaped rather than fully uniform, but it keeps the DAV object identity stable without introducing a separate DAV object layer for Deck.

This is also closer to how Nextcloud's CalDAV backend handles object URIs in general: object hrefs are persisted as part of the DAV object state, rather than being recomputed into a server-generated card-<id>.ics style name on every request. The main difference is that Deck stores that URI on the card itself instead of using a dedicated calendarobjects persistence layer like core CalDAV does.

As a small interoperability follow-up fixed in 656466f6c:
direct GET/HEAD requests to stale source hrefs after same-board list moves now return 404 instead of a placeholder object. That avoids Thunderbird treating the old source entry as a still-readable changed item, while collection/report fallback handling remains in place.

github-actions · 2026-03-05T02:16:39Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

grnd-alt

I did not manage to grasp everything in here, but this feels quite spaghetti and hard to read or follow tbh. I do think this can be implemented in a more readable manner, but might as well be a skill issue on code reading on my side. However it would be nice if this could be split up to e.g. not have the list_modes in there as well and get this to be easier to review

grnd-alt · 2026-04-21T12:16:29Z

+	}
+
+	private function isSabreVCalendar($value): bool {
+		/** @psalm-suppress UndefinedClass */


wrapper function only to suppress psalm? same for VTodo

grnd-alt · 2026-04-21T12:58:22Z

+	private function extractStackIdFromRelatedTo($todo): ?int {
+		$parentCandidates = [];
+		$otherCandidates = [];
+		foreach ($todo->children() as $child) {


this for loop can be something like this using the sabre types to not do the property handling etc yourself, then you could also remove some of the helper functions.

$relatedToArray = $todo->select("RELATED-TO"); foreach($relatedToArray as $relatedTo) { $reltype = $relatedTo["RELTYPE"] ?? null; if ($reltype instanceof \Sabre\VObject\Parameter) { $reltypeValue = $reltype->getValue(); if ($reltypeValue === 'PARENT') { $parentCandidates[] = (string)$relatedTo; } else { $otherCandidates[] = (string)$relatedTo; } } }

grnd-alt · 2026-04-21T13:05:26Z

+		} elseif ($targetBoardId !== null && $currentBoardId !== $targetBoardId) {
+			$stackId = $this->getDefaultStackIdForBoard($targetBoardId);
+		} else {
+			$stackId = $card->getStackId();


this entire stackId handling is not very easily readable, and seems overly complex, it does not make clear why the stackId is selected the way it is.

grnd-alt · 2026-04-21T14:00:47Z

+		}
+	}
+
+	private function normalizeDavUriForStorage(?string $name): ?string {


weird naming, does not seem to normalize anything but rather validate.

Strange to have the parameter nullable, but OK. Personally, I would expect an exception thrown in the negative case.

Is this user controlled, though? Is the check sufficient?

grnd-alt · 2026-04-21T14:03:14Z

 		return $card;
 	}

+	public function findByDavUriLite(string $davUri, ?int $boardId = null, ?int $stackId = null, bool $includeDeleted = true): Card {


why is this named lite?

grnd-alt · 2026-04-21T14:15:48Z

 		}
+		$stack = $this->stackMapper->find($stackId);
+		$boardId = $stack->getBoardId();
+		$board = $this->boardMapper->find($boardId);


I don't think those have any benefit as the activitymanager gets board and stack data

grnd-alt · 2026-04-21T15:13:22Z

+		if ($knownBoardId !== null) {
+			$this->permissionService->checkPermission($this->boardMapper, $knownBoardId, Acl::PERMISSION_EDIT);
+		} else {
+			$this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT, allowDeletedCard: true);


only doing this check if knownBoardId is not passed feels like a security issue. If KnownBoardId does not match the board the card's on it's possible to move a card from a board where a user has only read permission. It looks like this is verified for in getBackendChildren() but that is multiple functions deep and very implicit.

blizzz

I am confused about wide parts of this PR. Some aspects maybe makes sense, but do not really belong here, about others I am really puzzled. Maybe we can extract the necessary parts and have a very specific and lean as possible change set? Aspects that are not directly related to the write access can still be implemented in a separate PR.

blizzz · 2026-04-24T13:36:45Z

+				:clearable="false"
+				label="label"
+				track-by="id"
+				:input-label="t('deck', 'CalDAV list mapping mode')" />


as a technical user, I do not know what this means. Seeing the possible value the meaning gets clearer, the main label should still be easier to understand.

Also, isn't this out of scope for the writable caldav feature and should be split into a separate PR?

blizzz · 2026-04-24T13:40:50Z

 	protected string $title = '';
 	protected $description;
 	protected $descriptionPrev;
+	protected $davUri = null;


type is missing

blizzz · 2026-04-24T13:42:00Z

 		$calendar = new VCalendar();
 		$event = $calendar->createComponent('VTODO');
 		$event->UID = 'deck-card-' . $this->getId();
+		$event->{'X-NC-DECK-CARD-ID'} = (string)$this->getId();


this is already part of the UID?

blizzz · 2026-04-24T13:44:20Z

+			if ($this->getDone()) {
+				$event->COMPLETED = $this->getDone();
+			} else {
+				$event->COMPLETED = $lastModified;


looks strange?

blizzz · 2026-04-24T13:48:08Z

+			}
 		} else {
 			$event->STATUS = 'NEEDS-ACTION';
+			$event->{'PERCENT-COMPLETE'} = 0;


blizzz · 2026-04-24T14:20:21Z

+		$qb->orderBy('order')
+			->addOrderBy('id');


The return code later suggest only one specific entry is returned, then why sorting?

blizzz · 2026-04-24T14:34:52Z

+		}
+	}
+
+	private function normalizeDavUriForStorage(?string $name): ?string {


Strange to have the parameter nullable, but OK. Personally, I would expect an exception thrown in the negative case.

Is this user controlled, though? Is the check sufficient?

blizzz · 2026-04-24T18:25:03Z

-		$card = $this->cardMapper->find($cardId);
-		$stack = $this->stackMapper->find($card->getStackId());
-		$board = $this->boardMapper->find($stack->getBoardId());
+	private function findDetailsForCard($cardOrId, ?string $subject = null, ?Stack $knownStack = null, ?array $knownBoard = null): array {


why is this extended? It does not seem that this is related to the feature?

CardDetails extends Card and keeps its own default entity properties. Direct property access can therefore read null from the wrapper while the magic getter delegates through __call() to the inner Card. That can crash any code path serializing a Card-like object with a due date via getCalendarObject(), not only CalDAV PUT handling. Signed-off-by: Jaggob <37583151+Jaggob@users.noreply.github.com>

Map SUMMARY, DESCRIPTION, DUE, and STATUS/COMPLETED/PERCENT-COMPLETE to existing Deck cards. Ignore CATEGORIES, ATTENDEE, RRULE, ATTACH, DTSTART, RELATED-TO, and raw VTODO data. This intentionally accepts round-trip loss for unsupported properties in this first step. Reject CREATE with 403, DELETE with 403, and Stack writes with 403 by filtering write-content on CalendarObject ACLs. Return isShared(): false so Nextcloud's Schedule plugin does not treat Deck external calendars as shared scheduling calendars during writable DAV hooks. Map StatusException to 403, DoesNotExistException to 404, and invalid calendar payloads to InvalidDataException for 400 responses. Tested with Thunderbird 148.0.1 and macOS Reminders 26.4.1. Refs nextcloud#2399: partial write access for existing items only. Signed-off-by: Jaggob <37583151+Jaggob@users.noreply.github.com>

Account for the expected integration query count increase from permission-aware CalDAV ACLs. Calendar and backend permission checks are cached, but exposing write-content only to board editors still requires real permission lookups during integration requests. Signed-off-by: Jaggob <37583151+Jaggob@users.noreply.github.com>

chatgpt-codex-connector · 2026-05-05T19:42:54Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Jaggob · 2026-05-05T19:43:00Z

Thanks for all the feedback, and sorry for the chaotic first version of this PR. I tried to cover too much of the CalDAV write feature and future improvements at once and ended up neglecting code quality and readability.

I have now reduced the PR to a much smaller first step: updating existing Deck cards via CalDAV. Also updated the PR description with the current scope and a list of things that are intentionally left for follow-up work.

For reference (and because I still think it might be a good way to deal with the create mapping flow), the previous broader implementation with the different create stack mapping approaches is still available here:
https://github.com/Jaggob/deck/tree/backup/feature-cal-dav-write-full-a2dce36

Jaggob requested review from elzody, grnd-alt and luka-nextcloud as code owners February 18, 2026 13:36

Jaggob force-pushed the feature/cal-dav-write branch from 36747a6 to 31b37dd Compare February 18, 2026 13:42

chatgpt-codex-connector Bot reviewed Feb 18, 2026

View reviewed changes

Comment thread lib/DAV/DeckCalendarBackend.php Outdated

Comment thread lib/DAV/Calendar.php Outdated

Jaggob force-pushed the feature/cal-dav-write branch from 584dcf5 to 8f1393f Compare February 18, 2026 14:43

grnd-alt requested changes Feb 23, 2026

View reviewed changes

github-actions Bot added the feedback-requested label Mar 5, 2026

Jaggob force-pushed the feature/cal-dav-write branch 3 times, most recently from 45ec9aa to 83626ce Compare March 20, 2026 22:28

Jaggob requested a review from grnd-alt March 21, 2026 10:11

Jaggob force-pushed the feature/cal-dav-write branch from fc98830 to a2dce36 Compare March 31, 2026 10:35

grnd-alt requested changes Apr 21, 2026

View reviewed changes

blizzz reviewed Apr 24, 2026

View reviewed changes

Jaggob marked this pull request as draft May 5, 2026 09:37

Jaggob closed this May 5, 2026

Jaggob force-pushed the feature/cal-dav-write branch from a2dce36 to 48e6628 Compare May 5, 2026 09:39

Jaggob reopened this May 5, 2026

Jaggob added 3 commits May 5, 2026 21:36

Jaggob force-pushed the feature/cal-dav-write branch from 9c9d4f1 to 651a993 Compare May 5, 2026 19:36

Jaggob marked this pull request as ready for review May 5, 2026 19:42

Conversation

Jaggob commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implemented

Not Included

Notes

Testing

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

grnd-alt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jaggob Mar 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 5, 2026

Uh oh!

grnd-alt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot commented May 5, 2026

Uh oh!

Jaggob commented Feb 18, 2026 •

edited

Loading

Jaggob Mar 14, 2026 •

edited

Loading