|
3 | 3 | ## Context |
4 | 4 |
|
5 | 5 | The `GVodyanov/calendar` frontend already implements the full delegation UI on top of the |
6 | | -CalDAV proxy-principal system. Two gaps in `nextcloud/server` must be closed before the |
| 6 | +CalDAV proxy-principal system. Three gaps in `nextcloud/server` must be closed before the |
7 | 7 | feature is end-to-end functional. Everything else (database layer, `group-member-set` |
8 | 8 | read/write, `group-membership`, individual Calendar ACL, user search) is already |
9 | 9 | implemented. |
10 | 10 |
|
11 | 11 | --- |
12 | 12 |
|
13 | | -## Scope β exactly two files must change |
| 13 | +## β
Will delegated calendars appear in the delegate's sidebar? |
| 14 | + |
| 15 | +**Yes.** Here is the exact data-flow that proves it: |
| 16 | + |
| 17 | +``` |
| 18 | +[Alice adds Bob as delegate] |
| 19 | + β PROPPATCH /dav/principals/users/alice/calendar-proxy-write |
| 20 | + sets group-member-set = [.../principals/users/bob] β already works |
| 21 | +
|
| 22 | +[Bob logs into the calendar app] |
| 23 | + β fetchDelegators() |
| 24 | + PROPFIND /dav/principals/users/bob β group-membership |
| 25 | + finds: principals/users/alice/calendar-proxy-write |
| 26 | + stores: delegatorUserIds = ['alice'] |
| 27 | +
|
| 28 | + β fetchDelegatedCalendars() (for each delegatorUserId) |
| 29 | + PROPFIND /dav/calendars/alice/ β BLOCKED today (403) |
| 30 | + β Fixed by Change 1 below (CalendarHome.getACL) β 207 OK |
| 31 | + maps each calendar β { ...calendarObj, isDelegated: true } |
| 32 | + pushes into calendarsStore |
| 33 | +
|
| 34 | +[CalendarList.vue renders] |
| 35 | + β "Delegated" section shows all calendars where isDelegated === true |
| 36 | + β Bob sees Alice's calendars in his sidebar β |
| 37 | +``` |
| 38 | + |
| 39 | +Without Change 1 the `PROPFIND /dav/calendars/alice/` step returns **403 Forbidden** and |
| 40 | +no calendars are loaded. With Change 1 it returns **207 Multi-Status** and all of Alice's |
| 41 | +calendars populate the "Delegated" section of Bob's sidebar. |
| 42 | + |
| 43 | +--- |
| 44 | + |
| 45 | +## Scope β three changes required |
14 | 46 |
|
15 | 47 | | # | File | What to add | |
16 | 48 | |---|------|-------------| |
17 | 49 | | 1 | `apps/dav/lib/CalDAV/CalendarHome.php` | Override `getACL()` to grant `{DAV:}read` to the owner's `calendar-proxy-write` and `calendar-proxy-read` sub-principals | |
18 | 50 | | 2 | `apps/dav/lib/Connector/Sabre/Principal.php` | Return an explicit `{DAV:}acl` for proxy-group principals so only the owner can PROPPATCH them | |
| 51 | +| 3 | `apps/dav/lib/Connector/Sabre/Principal.php` | Detect newly added delegates inside `updatePrincipal()` and send a notification email via `OCP\Mail\IMailer` | |
19 | 52 |
|
20 | | -Do **not** modify any other file unless a test fixture requires it. |
| 53 | +Do **not** modify any other file unless a test fixture or DI container wiring requires it. |
21 | 54 |
|
22 | 55 | --- |
23 | 56 |
|
@@ -176,7 +209,137 @@ File: `apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php` |
176 | 209 |
|
177 | 210 | --- |
178 | 211 |
|
179 | | -## SPDX headers |
| 212 | +## Change 3 β Email notification when a delegate is added (`apps/dav/lib/Connector/Sabre/Principal.php`) |
| 213 | + |
| 214 | +### Problem |
| 215 | + |
| 216 | +When Alice adds Bob as a delegate, Bob has no way of knowing until he logs in. An |
| 217 | +automated email notification ("You have been granted delegate access to Alice's calendars") |
| 218 | +lets Bob act immediately and avoids confusion. |
| 219 | + |
| 220 | +### Where to hook |
| 221 | + |
| 222 | +`Principal.php` already handles `PROPPATCH` on proxy-group principals through the |
| 223 | +`updatePrincipal()` method (or the method that calls `setGroupMemberSet` on the |
| 224 | +`ProxyMapper`). After writing the new member set to the database, compare the old |
| 225 | +membership list with the new one and send an email to every **newly added** member. |
| 226 | + |
| 227 | +### Constructor changes |
| 228 | + |
| 229 | +Add `OCP\Mail\IMailer` and `OCP\IUserManager` and `OCP\IL10N` (or |
| 230 | +`OCP\L10N\IFactory`) to the constructor's injected dependencies: |
| 231 | + |
| 232 | +```php |
| 233 | +public function __construct( |
| 234 | + // ... existing parameters ... |
| 235 | + private IMailer $mailer, |
| 236 | + private IUserManager $userManager, |
| 237 | + private IFactory $l10nFactory, |
| 238 | +) {} |
| 239 | +``` |
| 240 | + |
| 241 | +> These services are already available in the DAV app's DI container. No new |
| 242 | +> container registrations are needed. |
| 243 | +
|
| 244 | +### Implementation β diff inside `updatePrincipal()` (or equivalent proxy-write method) |
| 245 | + |
| 246 | +```php |
| 247 | +// Immediately before writing the new member set, snapshot the old set: |
| 248 | +$oldMembers = $this->proxyMapper->getProxiesOf($ownerUid, Proxy::PERMISSION_WRITE); |
| 249 | +$oldMemberUids = array_column($oldMembers, 'proxyId'); |
| 250 | + |
| 251 | +// ... existing write call, e.g.: |
| 252 | +$this->proxyMapper->setProxiesOf($ownerUid, $newMemberUids, Proxy::PERMISSION_WRITE); |
| 253 | + |
| 254 | +// After the write, find newly added members and notify them: |
| 255 | +$addedUids = array_diff($newMemberUids, $oldMemberUids); |
| 256 | +foreach ($addedUids as $delegateUid) { |
| 257 | + $this->sendDelegationNotification($ownerUid, $delegateUid); |
| 258 | +} |
| 259 | +``` |
| 260 | + |
| 261 | +Add the private helper method to the same class: |
| 262 | + |
| 263 | +```php |
| 264 | +/** |
| 265 | + * Send an email to a newly added delegate informing them of the delegation. |
| 266 | + * |
| 267 | + * @param string $ownerUid User ID of the calendar owner who granted access |
| 268 | + * @param string $delegateUid User ID of the user who was just granted access |
| 269 | + */ |
| 270 | +private function sendDelegationNotification(string $ownerUid, string $delegateUid): void { |
| 271 | + $delegateUser = $this->userManager->get($delegateUid); |
| 272 | + $ownerUser = $this->userManager->get($ownerUid); |
| 273 | + |
| 274 | + if ($delegateUser === null || $ownerUser === null) { |
| 275 | + return; |
| 276 | + } |
| 277 | + |
| 278 | + $delegateEmail = $delegateUser->getEMailAddress(); |
| 279 | + if ($delegateEmail === null || $delegateEmail === '') { |
| 280 | + return; // No email address on file β skip silently. |
| 281 | + } |
| 282 | + |
| 283 | + $l = $this->l10nFactory->get('dav'); |
| 284 | + |
| 285 | + $ownerDisplayName = $ownerUser->getDisplayName() ?: $ownerUid; |
| 286 | + $delegateDisplayName = $delegateUser->getDisplayName() ?: $delegateUid; |
| 287 | + |
| 288 | + $subject = $l->t('%s has granted you access to their calendars', [$ownerDisplayName]); |
| 289 | + $bodyText = $l->t( |
| 290 | + 'Hello %1$s, |
| 291 | + |
| 292 | +%2$s has added you as a calendar delegate. You can now view and manage their |
| 293 | +calendars in the Nextcloud Calendar app under the "Delegated" section. |
| 294 | + |
| 295 | +To remove yourself as a delegate, ask %2$s to revoke your access in their |
| 296 | +Calendar settings.', |
| 297 | + [$delegateDisplayName, $ownerDisplayName] |
| 298 | + ); |
| 299 | + |
| 300 | + try { |
| 301 | + $message = $this->mailer->createMessage(); |
| 302 | + $message->setTo([$delegateEmail => $delegateDisplayName]); |
| 303 | + $message->setSubject($subject); |
| 304 | + $message->setPlainBody($bodyText); |
| 305 | + $this->mailer->send($message); |
| 306 | + } catch (\Exception $e) { |
| 307 | + // Notification failure must never block the PROPPATCH response. |
| 308 | + $this->logger->warning( |
| 309 | + 'Could not send delegation notification email', |
| 310 | + ['owner' => $ownerUid, 'delegate' => $delegateUid, 'error' => $e->getMessage()] |
| 311 | + ); |
| 312 | + } |
| 313 | +} |
| 314 | +``` |
| 315 | + |
| 316 | +### Important constraints |
| 317 | + |
| 318 | +* **Never throw** from `sendDelegationNotification`. A failed email must not cause the |
| 319 | + PROPPATCH to return an error β the delegation itself succeeded. |
| 320 | +* Only send notifications for **newly added** members (diff old vs new set). Do **not** |
| 321 | + send on revocation; that is handled by the UI. |
| 322 | +* Respect privacy: only the delegate's own email address is used. Do not expose the |
| 323 | + owner's email in the notification unless it is already part of the user's public |
| 324 | + profile. |
| 325 | + |
| 326 | +### Tests to add |
| 327 | + |
| 328 | +File: `apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php` |
| 329 | + |
| 330 | +1. **`testDelegationNotificationEmailIsSentToNewDelegate`** β mock `IMailer::send()` to |
| 331 | + expect exactly one call; mock `IUserManager` to return valid users with email |
| 332 | + addresses; call the proxy-write PROPPATCH with a net-new member; assert `send()` was |
| 333 | + called once with the correct recipient and subject. |
| 334 | +2. **`testDelegationNotificationNotSentForExistingDelegate`** β add bob to alice's group |
| 335 | + twice; assert `send()` is called only on the first invocation. |
| 336 | +3. **`testDelegationNotificationSkippedWhenNoEmail`** β set `getEMailAddress()` to return |
| 337 | + `null`; assert `send()` is never called. |
| 338 | +4. **`testDelegationNotificationFailureDoesNotBlockProppatch`** β make `IMailer::send()` |
| 339 | + throw a `\RuntimeException`; assert that `updatePrincipal()` still returns without |
| 340 | + throwing and that the proxy row was written to the database. |
| 341 | + |
| 342 | +--- |
180 | 343 |
|
181 | 344 | All new or substantially modified PHP file blocks must use year **2026**: |
182 | 345 |
|
@@ -206,16 +369,17 @@ The following are fully implemented and must not be modified: |
206 | 369 |
|
207 | 370 | ## Verification checklist |
208 | 371 |
|
209 | | -After implementing both changes, the following end-to-end flow must work: |
| 372 | +After implementing all three changes, the following end-to-end flow must work: |
210 | 373 |
|
211 | 374 | 1. User **alice** opens the Delegation modal in the calendar app and adds **bob** as a delegate. |
212 | 375 | - `PROPPATCH /dav/principals/users/alice/calendar-proxy-write` succeeds (HTTP 207). |
| 376 | + - **Bob receives an email**: "Alice has granted you access to their calendars" β **Change 3**. |
213 | 377 | 2. User **bob** logs in; the calendar app calls `fetchDelegators()` which reads |
214 | 378 | `PROPFIND /dav/principals/users/bob` β `group-membership` and finds `alice/calendar-proxy-write`. |
215 | 379 | 3. The app calls `fetchDelegatedCalendars()` which does |
216 | 380 | `PROPFIND /dav/calendars/alice/` β this now succeeds (HTTP 207) because of **Change 1**. |
217 | | -4. Bob can see Alice's calendars in the "Delegated" section of the sidebar. |
| 381 | +4. **Bob can see Alice's calendars in the "Delegated" section of his sidebar** β confirmed by Change 1 + frontend store. |
218 | 382 | 5. Bob can create/edit events in Alice's calendars (existing Calendar ACL already handles this). |
219 | | -6. Alice revokes Bob's access β `PROPPATCH` removes Bob from the proxy group. |
| 383 | +6. Alice revokes Bob's access β `PROPPATCH` removes Bob from the proxy group (no email sent on revocation). |
220 | 384 | 7. User **charlie** (unrelated) cannot modify Alice's proxy group β `PROPPATCH` returns 403 |
221 | 385 | because of **Change 2**. |
0 commit comments