From b6ba2002815a14608160849d71d15ceabf3db7eb Mon Sep 17 00:00:00 2001 From: Jonathan Treffler Date: Mon, 12 Aug 2024 20:11:19 +0200 Subject: [PATCH] fix #6 --- lib/Dav/SubscriptionManagementPlugin.php | 40 +++++++++++++--------- lib/Db/WebPushSubscriptionMapper.php | 14 +++++--- lib/PushTransports/WebPushTransport.php | 4 +-- lib/Service/WebPushSubscriptionService.php | 4 +-- lib/Transport/Transport.php | 6 ++-- 5 files changed, 41 insertions(+), 27 deletions(-) diff --git a/lib/Dav/SubscriptionManagementPlugin.php b/lib/Dav/SubscriptionManagementPlugin.php index 14f3e8c..353ed2f 100644 --- a/lib/Dav/SubscriptionManagementPlugin.php +++ b/lib/Dav/SubscriptionManagementPlugin.php @@ -138,10 +138,10 @@ class SubscriptionManagementPlugin extends ServerPlugin { $errors[] = "options validation error"; } } else { - $existingSubscriptionId = $transport->getSubscriptionIdFromOptions($subscriptionOptions); - $user = $this->userSession->getUser(); + $existingSubscriptionId = $transport->getSubscriptionIdFromOptions($user->getUID(), $node->getName(), $subscriptionOptions); + if(!is_int($existingSubscriptionId)) { // create new subscription entry in db $subscription = $this->subscriptionService->create($user->getUID(), $node->getName(), $subscriptionType, $subscriptionExpires); @@ -164,24 +164,30 @@ class SubscriptionManagementPlugin extends ServerPlugin { } } } else { + // implicitly checks if subscription found by transport is really owned by correct user $subscription = $this->subscriptionService->find($user->getUID(), $existingSubscriptionId); - - [ - 'success' => $updateSuccess, - 'errors' => $updateErrors, - 'response' => $responseContent, - ] = $transport->updateSubscription($subscription->getId(), $subscriptionOptions); - - if(!$updateSuccess) { - if(isset($updateErrors) && !empty($updateErrors)) { - $errors = array_merge($errors, $updateErrors); - } else { - $errors[] = "subscription update error"; - } + + // check if subscription found by transport is really for correct collection + if($subscription->getCollectionName() !== $node->getName()) { + $errors[] = "subscription update error"; } else { - $subscription = $this->subscriptionService->update($user->getUID(), $subscription->getId(), $subscriptionExpires); + [ + 'success' => $updateSuccess, + 'errors' => $updateErrors, + 'response' => $responseContent, + ] = $transport->updateSubscription($subscription->getId(), $subscriptionOptions); - $responseStatus = Http::STATUS_CREATED; + 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; + } } } } diff --git a/lib/Db/WebPushSubscriptionMapper.php b/lib/Db/WebPushSubscriptionMapper.php index 5ffce09..6b38bc4 100644 --- a/lib/Db/WebPushSubscriptionMapper.php +++ b/lib/Db/WebPushSubscriptionMapper.php @@ -10,6 +10,7 @@ use OCP\IDBConnection; class WebPushSubscriptionMapper extends QBMapper { public const TABLENAME = 'dav_push_subscriptions_webpush'; + public const SUBSCRIPTIONS_TABLENAME = "dav_push_subscriptions"; public function __construct(IDBConnection $db) { parent::__construct($db, self::TABLENAME, WebPushSubscription::class); @@ -37,12 +38,17 @@ class WebPushSubscriptionMapper extends QBMapper { * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException * @throws DoesNotExistException */ - public function findByPushResource(string $pushResource): WebPushSubscription { + public function findByPushResource(string $userId, string $collectionName, string $pushResource): WebPushSubscription { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); - $qb->select('*') - ->from(self::TABLENAME) - ->where($qb->expr()->eq('push_resource', $qb->createNamedParameter($pushResource))); + $qb->select('webpush.*') + ->from(self::TABLENAME, 'webpush'); + + $qb->innerJoin('webpush', self::SUBSCRIPTIONS_TABLENAME, 'subscription', $qb->expr()->eq('webpush.subscription_id', 'subscription.id')); + + $qb->where($qb->expr()->eq('webpush.push_resource', $qb->createNamedParameter($pushResource))) + ->andWhere($qb->expr()->eq('subscription.user_id', $qb->createNamedParameter($userId))) + ->andWhere($qb->expr()->eq('subscription.collection_name', $qb->createNamedParameter($collectionName))); return $this->findEntity($qb); } diff --git a/lib/PushTransports/WebPushTransport.php b/lib/PushTransports/WebPushTransport.php index 134b971..330446a 100644 --- a/lib/PushTransports/WebPushTransport.php +++ b/lib/PushTransports/WebPushTransport.php @@ -99,11 +99,11 @@ class WebPushTransport extends Transport { $result = file_get_contents($pushResource, false, $context); } - public function getSubscriptionIdFromOptions($options): ?int { + public function getSubscriptionIdFromOptions(string $userId, string $collectionName, $options): ?int { ['pushResource' => $pushResource] = $this->parseOptions($options); try { - return $this->webPushSubscriptionService->findByPushResource($pushResource)->getSubscriptionId(); + return $this->webPushSubscriptionService->findByPushResource($userId, $collectionName, $pushResource)->getSubscriptionId(); } catch (WebPushSubscriptionNotFound $e) { return null; } diff --git a/lib/Service/WebPushSubscriptionService.php b/lib/Service/WebPushSubscriptionService.php index 2d84c1d..719286b 100644 --- a/lib/Service/WebPushSubscriptionService.php +++ b/lib/Service/WebPushSubscriptionService.php @@ -35,9 +35,9 @@ class WebPushSubscriptionService { } } - public function findByPushResource(string $pushResource): ?WebPushSubscription { + public function findByPushResource(string $userId, string $collectionName, string $pushResource): ?WebPushSubscription { try { - return $this->mapper->findByPushResource($pushResource); + return $this->mapper->findByPushResource($userId, $collectionName, $pushResource); } catch (Exception $e) { $this->handleException($e); } diff --git a/lib/Transport/Transport.php b/lib/Transport/Transport.php index 648f10b..f3da2bd 100644 --- a/lib/Transport/Transport.php +++ b/lib/Transport/Transport.php @@ -59,10 +59,12 @@ abstract class Transport { */ abstract public function registerSubscription($subsciptionId, $options); - // Transport needs to be able to map subscription options back to a subscription id. + // Transport needs to be able to map subscription options + userId + collectionName back to a subscription id. // API Requests to create and update a subscription are the same, therefore if a subscription id is associated with the given options the subscription is updated, otherwise a new subscription is added. // Which option(s) uniquely identify a subscription is implementation specific. - abstract public function getSubscriptionIdFromOptions($options): ?int; + // Options themselves do not necessarily uniquely identify a subscription, but combined with userId and collectionName it does. + // It is not recommended for transports to save userId and collectionName for themselves for this, you can just use a join with the general subscriptions table. + abstract public function getSubscriptionIdFromOptions(string $userId, string $collectionName, $options): ?int; // Change mutable options of the subscription (if any exist) abstract public function updateSubscription($subsciptionId, $options);