From 3d5b2c191a2043fe54e5094eeec00a13170e66a1 Mon Sep 17 00:00:00 2001
From: Jonathan Treffler <mail@jonathan-treffler.de>
Date: Wed, 24 Jul 2024 13:52:09 +0200
Subject: [PATCH] conform to spec by updating existing subscription when
 registering with equivalent options twice instead of creating new
 subscription

---
 lib/Dav/SubscriptionManagementPlugin.php | 69 ++++++++++++++++++------
 lib/Db/WebPushSubscriptionMapper.php     |  4 +-
 lib/PushTransports/WebPushTransport.php  |  7 ++-
 lib/Service/SubscriptionService.php      |  6 +--
 4 files changed, 62 insertions(+), 24 deletions(-)

diff --git a/lib/Dav/SubscriptionManagementPlugin.php b/lib/Dav/SubscriptionManagementPlugin.php
index f4fd3b4..9e990c7 100644
--- a/lib/Dav/SubscriptionManagementPlugin.php
+++ b/lib/Dav/SubscriptionManagementPlugin.php
@@ -127,19 +127,62 @@ class SubscriptionManagementPlugin extends ServerPlugin {
 
 			if(!is_null($transport)) {
 				[
-					'success' => $registerSuccess,
-					'error' => $registerError,
-					'responseStatus' => $responseStatus,
-					'response' => $responseContent,
-					'unsubscribeLink' => $unsubscribeLink,
-					'data' => $data
-				] = $transport->registerSubscription($subscriptionOptions);
+					'success' => $validateSuccess,
+					'errors' => $validateErrors,
+				] = $transport->validateOptions($subscriptionOptions);
 
-				$responseStatus = $responseStatus ?? Http::STATUS_CREATED;
-				$data = $data ?? False;
+				if(!$validateSuccess) {
+					if(isset($validateErrors) && !empty($validateErrors)) {
+						$errors = array_merge($errors, $validateErrors);
+					} else {
+						$errors[] = "options validation error";
+					}
+				} else {
+					$existingSubscriptionId = $transport->getSubscriptionIdFromOptions($subscriptionOptions);
 
-				if(!$registerSuccess) {
-					$errors[] = $registerError;
+					$user = $this->userSession->getUser();
+
+					if(!is_int($existingSubscriptionId)) {
+						// create new subscription entry in db
+						$subscription = $this->subscriptionService->create($user->getUID(), $node->getName(), $subscriptionType, $subscriptionExpires);
+
+						[
+							'success' => $registerSuccess,
+							'errors' => $registerErrors,
+							'responseStatus' => $responseStatus,
+							'response' => $responseContent,
+							'unsubscribeLink' => $unsubscribeLink,
+						] = $transport->registerSubscription($subscription->getId(), $subscriptionOptions);
+
+						$responseStatus = $responseStatus ?? Http::STATUS_CREATED;
+
+						if(!$registerSuccess) {
+							if(isset($registerErrors) && !empty($registerErrors)) {
+								$errors = array_merge($errors, $registerErrors);
+							} else {
+								$errors[] = "registration error";
+							}
+						}
+					} else {
+						$subscription = $this->subscriptionService->find($user->getUID(), $existingSubscriptionId);
+
+						[
+							'success' => $updateSuccess,
+							'errors' => $updateErrors,
+						] =	$transport->updateSubscription($subscription->getId(), $subscriptionOptions);
+
+						if(!$updateSuccess) {
+							if(isset($updateErrors) && !empty($updateErrors)) {
+								$errors = array_merge($errors, $updateErrors);
+							} else {
+								$errors[] = "subscription update error";
+							}
+						} else {
+							$subscription = $this->subscriptionService->update($user->getUID(), $subscription->getId(), $subscriptionExpires);
+
+							$responseStatus = Http::STATUS_CREATED;
+						}
+					}
 				}
 			} else {
 				$errors[] = $subscriptionType . " transport does not exist";
@@ -148,10 +191,6 @@ class SubscriptionManagementPlugin extends ServerPlugin {
 			if(sizeof($errors) == 0) {
 				$response->setStatus($responseStatus);
 				
-				// create subscription entry in db
-				$user = $this->userSession->getUser();
-				$subscription = $this->subscriptionService->create($user->getUID(), $node->getName(), $subscriptionType, $subscriptionExpires, $data);
-				
 				// generate default unsubscribe link, unless transport requested a custom url
 				$unsubscribeLink = $unsubscribeLink ?? $this->URLGenerator->getAbsoluteURL("/apps/dav_push/subscriptions/" . $subscription->getId());
 				$response->setHeader("Location", $unsubscribeLink);
diff --git a/lib/Db/WebPushSubscriptionMapper.php b/lib/Db/WebPushSubscriptionMapper.php
index be848ad..5ffce09 100644
--- a/lib/Db/WebPushSubscriptionMapper.php
+++ b/lib/Db/WebPushSubscriptionMapper.php
@@ -21,7 +21,7 @@ class WebPushSubscriptionMapper extends QBMapper {
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
 	 * @throws DoesNotExistException
 	 */
-	public function findBySubscriptionId(int $subscriptionId): Subscription {
+	public function findBySubscriptionId(int $subscriptionId): WebPushSubscription {
 		/* @var $qb IQueryBuilder */
 		$qb = $this->db->getQueryBuilder();
 		$qb->select('*')
@@ -37,7 +37,7 @@ class WebPushSubscriptionMapper extends QBMapper {
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
 	 * @throws DoesNotExistException
 	 */
-	public function findByPushResource(string $pushResource): Subscription {
+	public function findByPushResource(string $pushResource): WebPushSubscription {
 		/* @var $qb IQueryBuilder */
 		$qb = $this->db->getQueryBuilder();
 		$qb->select('*')
diff --git a/lib/PushTransports/WebPushTransport.php b/lib/PushTransports/WebPushTransport.php
index b0e9dad..6a8f3f4 100644
--- a/lib/PushTransports/WebPushTransport.php
+++ b/lib/PushTransports/WebPushTransport.php
@@ -57,11 +57,11 @@ class WebPushTransport extends Transport {
 
 		if(isset($pushResource) && $pushResource !== '') {
 			return [
-				'valid' => True,
+				'success' => True,
 			];
 		} else {
 			return [
-				'valid' => False,
+				'success' => False,
 				'errors' => ["push resource not provided"]
 			];
 		}
@@ -104,5 +104,8 @@ class WebPushTransport extends Transport {
 
 	public function updateSubscription($subsciptionId, $options) {
 		// there are no options which can be edited -> NOOP
+		return [
+			'success' => True,
+		];
 	}
 }
diff --git a/lib/Service/SubscriptionService.php b/lib/Service/SubscriptionService.php
index 1d2b95a..400a0a5 100644
--- a/lib/Service/SubscriptionService.php
+++ b/lib/Service/SubscriptionService.php
@@ -55,7 +55,7 @@ class SubscriptionService {
 		return $subscription;
 	}
 
-	public function update(string $userId, int $id, ?int $expirationTimestamp, mixed $data) {
+	public function update(string $userId, int $id, ?int $expirationTimestamp) {
 		try {
 			$subscription = $this->mapper->find($userId, $id);
 			
@@ -63,10 +63,6 @@ class SubscriptionService {
 				$subscription->setExpirationTimestamp($expirationTimestamp);
 			}
 
-			if (!is_null($data)) {
-				$subscription->setData(json_encode($data));
-			}
-
 			return $this->mapper->update($subscription);
 		} catch (Exception $e) {
 			$this->handleException($e);