[stable34] fix(webpush): Correctly hide already read notifications from webpush

docs/webpush-v2.md

@@ -238,6 +238,8 @@ Notifications are filtered per subscription using the stored `appTypes` before s
 
 The notification `urgency` ([RFC 8030 §5.3](https://www.rfc-editor.org/rfc/rfc8030#section-5.3)) is derived from the app/type: Talk, two-factor and phonetrack notifications use `high`, everything else `normal`.
 
+Content notifications and single delete notifications are sent with the `Topic` `nid{{nid}}` ([RFC 8030 §5.4](https://www.rfc-editor.org/rfc/rfc8030#section-5.4)). When a notification is removed before the push service delivered it (e.g. the browser was offline and the notification was read on another device), the delete replaces the pending notification, so the browser never shows it.
+
 ### Normal content notification
 
 The web push payload uses the same shape as the [mobile push subject](push-v2.md#encrypted-subject-data), but it is delivered as clear (already-decrypted) JSON and may carry a longer `subject` (the payload budget is ~3000 bytes instead of ~240). If you are missing any information necessary to parse the notification, use the `nid` to fetch the full notification via the [OCS API](ocs-endpoint-v2.md).
@@ -288,6 +290,8 @@ These notifications should not be shown to the user. Instead, delete pending not
 
 ### Silent delete notifications (multiple)
 
+The server sends a single delete notification per `nid` to web push subscriptions, so the `Topic` replacement works. Clients should still handle this payload.
+
 ```json
 {
   "nids" : [1337],
@@ -318,7 +322,7 @@ These notifications should not be shown to the user. Instead, delete pending not
 The app ships a service worker (registered from `/apps/notifications/service-worker.js`) that handles the browser `push` event:
 
 1. If a Nextcloud tab is open, the decrypted payload is forwarded to it via `postMessage` (`{ type: 'push', content }`) and the in-page UI handles it. If the payload carries an `activationToken`, the page performs the activation request instead.
-2. If no tab is open and the payload has a `subject`, the service worker shows a background notification (first line of the subject becomes the title, the rest the body).
+2. If no tab is open and the payload has a `subject`, the service worker shows a background notification (first line of the subject becomes the title, the rest the body) tagged `nid{{nid}}`. A delete payload arriving with no open tab closes the background notifications with the matching tags (all of them for `delete-all`).
 3. If there is no displayable content (e.g. a delete payload arriving with no open tab) and the browser requires `userVisibleOnly`, a short-lived silent placeholder notification is shown so the browser does not unregister the subscription for staying silent.
 
 On `pushsubscriptionchange` the service worker asks the page to re-register (`{ type: 'pushEndpoint' }`), driving the renewal flow described above.

js/service-worker.js

@@ -6,16 +6,33 @@
 'use strict'
 
 /**
- * @param {string} content received by Nextcloud, splitted to be shown in UI notification
+ * @param {object} content received by Nextcloud, the subject is splitted to be shown in UI notification
  */
 function showBgNotification(content) {
-	const [title, ...msg] = content.split('\n')
+	const [title, ...msg] = content.subject.split('\n')
 	const options = {
 		body: msg.join('\n'),
+		tag: 'nid' + content.nid,
 	}
 	return self.registration.showNotification(title, options)
 }
 
+/**
+ * Close the shown notifications of a delete payload
+ *
+ * @param {object} content received by Nextcloud
+ */
+function closeBgNotifications(content) {
+	let tags = [undefined]
+	if (content.delete) {
+		tags = ['nid' + content.nid]
+	} else if (content['delete-multiple']) {
+		tags = content.nids.map((nid) => 'nid' + nid)
+	}
+	return Promise.all(tags.map((tag) => self.registration.getNotifications({ tag })
+		.then((notifications) => notifications.forEach((notification) => notification.close()))))
+}
+
 /**
  * For Chrom* and Apple (who followed Chrome) users:
  * We need to show a silent notification that we remove to avoid being unregistered,
@@ -53,7 +70,10 @@ self.addEventListener('push', function(event) {
 					// Here, the user has an active tab, we don't need to show a notification from the sw
 				} else if (content.subject) {
 					console.debug('No valid client to send notif - showing bg notif')
-					return showBgNotification(content.subject)
+					return showBgNotification(content)
+				} else if (content.delete || content['delete-multiple'] || content['delete-all']) {
+					console.debug('No valid client to send notif - closing bg notifs')
+					return closeBgNotifications(content).then(silentNotification)
 				} else {
 					console.warn('No valid client to send notif')
 					return silentNotification()

lib/Push.php

@@ -419,7 +419,8 @@ public function webPushToDevice(int $id, IUser $user, array $devices, INotificat
 					$device['ua_public'],
 					$device['auth'],
 					json_encode($data, JSON_THROW_ON_ERROR),
-					urgency: $urgency
+					urgency: $urgency,
+					topic: $this->getWebPushTopic($id),
 				);
 			} catch (\JsonException $e) {
 				$this->log->error('JSON error while encoding push notification: ' . $e->getMessage(), ['exception' => $e]);
@@ -586,14 +587,13 @@ public function webPushDeleteToDevice(string $userId, IUser $user, array $device
 						$this->log->error('JSON error while encoding push notification: ' . $e->getMessage(), ['exception' => $e]);
 					}
 				} else {
-					$temp = $notificationIds;
-
-					while (!empty($temp)) {
-						$data = $this->encodeDeleteNotifs($temp);
-						$temp = $data['remaining'];
+					// Single deletes with the notification's topic replace the
+					// content push if it was not delivered yet (RFC 8030 §5.4)
+					foreach ($notificationIds ?? [] as $notificationId) {
+						$data = $this->encodeDeleteNotifs([$notificationId]);
 						try {
 							$payload = json_encode($data['data'], JSON_THROW_ON_ERROR);
-							$this->wpClient->enqueue($device['endpoint'], $device['ua_public'], $device['auth'], $payload);
+							$this->wpClient->enqueue($device['endpoint'], $device['ua_public'], $device['auth'], $payload, topic: $this->getWebPushTopic($notificationId));
 						} catch (\JsonException $e) {
 							$this->log->error('JSON error while encoding push notification: ' . $e->getMessage(), ['exception' => $e]);
 						}
@@ -606,7 +606,7 @@ public function webPushDeleteToDevice(string $userId, IUser $user, array $device
 		}
 
 		if (!$this->deferPayloads) {
-			$this->sendNotificationsToProxies();
+			$this->wpClient->flush(fn ($r) => $this->webPushCallback($r));
 		}
 	}
 
@@ -921,6 +921,14 @@ protected function encodeNotif(int $id, INotification $notification, int $maxLen
 		return $data;
 	}
 
+	/**
+	 * Topic of the web push messages of a notification, so a delete replaces
+	 * a not yet delivered notification at the push service (RFC 8030 §5.4)
+	 */
+	protected function getWebPushTopic(int $notificationId): string {
+		return 'nid' . $notificationId;
+	}
+
 	/**
 	 * @param ?int[] $ids
 	 * @return array

lib/WebPushClient.php

@@ -124,13 +124,14 @@ public function notify(string $endpoint, string $uaPublicKey, string $auth, stri
 	 * Queue one notification. [flush] needs to be called to actually send the notifications
 	 * @throws \ErrorException
 	 */
-	public function enqueue(string $endpoint, string $uaPublicKey, string $auth, string $body, string $urgency = 'normal'): void {
+	public function enqueue(string $endpoint, string $uaPublicKey, string $auth, string $body, string $urgency = 'normal', ?string $topic = null): void {
 		$c = $this->getClient();
 		$c->queueNotification(
 			new Subscription($endpoint, $uaPublicKey, $auth, 'aes128gcm'),
 			$body,
 			options: [
-				'urgency' => $urgency
+				'urgency' => $urgency,
+				'topic' => $topic,
 			]
 		);
 	}

src/workers/service-worker.js

@@ -6,16 +6,33 @@
 'use strict'
 
 /**
- * @param {string} content received by Nextcloud, splitted to be shown in UI notification
+ * @param {object} content received by Nextcloud, the subject is splitted to be shown in UI notification
  */
 function showBgNotification(content) {
-	const [title, ...msg] = content.split('\n')
+	const [title, ...msg] = content.subject.split('\n')
 	const options = {
 		body: msg.join('\n'),
+		tag: 'nid' + content.nid,
 	}
 	return self.registration.showNotification(title, options)
 }
 
+/**
+ * Close the shown notifications of a delete payload
+ *
+ * @param {object} content received by Nextcloud
+ */
+function closeBgNotifications(content) {
+	let tags = [undefined]
+	if (content.delete) {
+		tags = ['nid' + content.nid]
+	} else if (content['delete-multiple']) {
+		tags = content.nids.map((nid) => 'nid' + nid)
+	}
+	return Promise.all(tags.map((tag) => self.registration.getNotifications({ tag })
+		.then((notifications) => notifications.forEach((notification) => notification.close()))))
+}
+
 /**
  * For Chrom* and Apple (who followed Chrome) users:
  * We need to show a silent notification that we remove to avoid being unregistered,
@@ -53,7 +70,10 @@ self.addEventListener('push', function(event) {
 					// Here, the user has an active tab, we don't need to show a notification from the sw
 				} else if (content.subject) {
 					console.debug('No valid client to send notif - showing bg notif')
-					return showBgNotification(content.subject)
+					return showBgNotification(content)
+				} else if (content.delete || content['delete-multiple'] || content['delete-all']) {
+					console.debug('No valid client to send notif - closing bg notifs')
+					return closeBgNotifications(content).then(silentNotification)
 				} else {
 					console.warn('No valid client to send notif')
 					return silentNotification()

tests/Unit/PushTest.php

@@ -1216,7 +1216,8 @@ public function testWebPushToDeviceSending(bool $isRateLimited): void {
 			->method('deleteWebPushToken');
 
 		$this->wpClient->expects($this->exactly($isRateLimited ? 1 : 2))
-			->method('enqueue');
+			->method('enqueue')
+			->with($this->anything(), self::EX_UA_PUBLIC, self::EX_AUTH, $this->anything(), 'normal', 'nid207787');
 
 		if ($isRateLimited) {
 			$this->cache
@@ -1236,6 +1237,39 @@ public function testWebPushToDeviceSending(bool $isRateLimited): void {
 		$push->pushToDevice(207787, $notification);
 	}
 
+	public static function dataWebPushDeleteToDevice(): array {
+		return [
+			'single' => [[42], [['{"nid":42,"delete":true}', 'nid42']]],
+			'multiple' => [[42, 1337], [['{"nid":42,"delete":true}', 'nid42'], ['{"nid":1337,"delete":true}', 'nid1337']]],
+			'all' => [null, [['{"delete-all":true}', null]]],
+		];
+	}
+
+	#[DataProvider(methodName: 'dataWebPushDeleteToDevice')]
+	public function testWebPushDeleteToDevice(?array $notificationIds, array $expected): void {
+		$push = $this->getPush(['deleteWebPushToken']);
+
+		$calls = [];
+		$this->wpClient->expects($this->exactly(count($expected)))
+			->method('enqueue')
+			->willReturnCallback(static function (string $endpoint, string $uaPublicKey, string $auth, string $body, string $urgency = 'normal', ?string $topic = null) use (&$calls): void {
+				$calls[] = [$body, $topic];
+			});
+		$this->wpClient->expects($this->once())
+			->method('flush');
+
+		$push->webPushDeleteToDevice('valid', $this->createStub(IUser::class), [
+			[
+				'endpoint' => 'endpoint1',
+				'ua_public' => self::EX_UA_PUBLIC,
+				'auth' => self::EX_AUTH,
+				'token' => 16,
+			],
+		], $notificationIds === null, $notificationIds);
+
+		$this->assertSame($expected, $calls);
+	}
+
 	public static function dataFilterWebPushDeviceList(): array {
 		return [
 			[false, 'all', 'myApp', false],