Merge pull request #129 from iMattPro/updates

Preserve each user's push token + fixes

Author: iMattPro

migrations/handle_subscriptions.php

@@ -89,17 +89,32 @@ public function copy_subscription_tables()
 		$wpn_notification_push_table = $this->table_prefix . 'wpn_notification_push';
 		$wpn_push_subscriptions_table = $this->table_prefix . 'wpn_push_subscriptions';
 
-		/*
-		 * If webpush notification method exists in phpBB core,
-		 * copy all subscriptions data over the corresponding core tables.
-		 */
-		foreach ([
-				$core_notification_push_table => $wpn_notification_push_table,
-				$core_push_subscriptions_table => $wpn_push_subscriptions_table
-			] as $core_table => $ext_table)
+		// Copy push table data
+		$sql = 'INSERT INTO ' . $core_notification_push_table . '
+    			(notification_type_id, item_id, item_parent_id, user_id, push_data, notification_time, push_token)
+    		SELECT notification_type_id, item_id, item_parent_id, user_id, push_data, notification_time, push_token
+    		FROM ' . $wpn_notification_push_table;
+		$this->db->sql_query($sql);
+
+		// Turn on identity insert on mssql to be able to insert into
+		// identity columns (e.g. id)
+		if (strpos($this->db->get_sql_layer(), 'mssql') !== false)
+		{
+			$sql = 'SET IDENTITY_INSERT ' . $core_push_subscriptions_table . ' ON';
+			$this->db->sql_query($sql);
+		}
+
+		// Copy subscription table data
+		$sql = 'INSERT INTO ' . $core_push_subscriptions_table . '
+    			(subscription_id, user_id, endpoint, expiration_time, p256dh, auth)
+    		SELECT subscription_id, user_id, endpoint, expiration_time, p256dh, auth
+    		FROM ' . $wpn_push_subscriptions_table;
+		$this->db->sql_query($sql);
+
+		// Disable identity insert on mssql again
+		if (strpos($this->db->get_sql_layer(), 'mssql') !== false)
 		{
-			$sql = 'INSERT INTO ' . $core_table . '
-					SELECT * FROM ' . $ext_table;
+			$sql = 'SET IDENTITY_INSERT ' . $core_push_subscriptions_table . ' OFF';
 			$this->db->sql_query($sql);
 		}
 	}

notification/method/webpush.php

@@ -158,7 +158,7 @@ public function notify()
 			];
 			$data = self::clean_data($data);
 			$insert_buffer->insert($data);
-			$this->push_token_map[$notification->notification_type_id][$notification->item_id] = $data['push_token'];
+			$this->push_token_map[$notification->notification_type_id][$notification->item_id][$notification->user_id] = $data['push_token'];
 		}
 
 		$insert_buffer->flush();
@@ -196,8 +196,11 @@ protected function notify_using_webpush(): void
 
 		// Load all the users we need
 		$notify_users = array_diff($user_ids, $banned_users);
+
+		// If we have no users (e.g. all recipients are banned) empty queue and exit
 		if (empty($notify_users))
 		{
+			$this->empty_queue();
 			return;
 		}
 
@@ -239,7 +242,7 @@ protected function notify_using_webpush(): void
 				'type_id'	=> $notification->notification_type_id,
 				'user_id'	=> $notification->user_id,
 				'version'	=> $this->config['assets_version'],
-				'token'		=> hash('sha256', $user['user_form_salt'] . $this->push_token_map[$notification->notification_type_id][$notification->item_id]),
+				'token'		=> hash('sha256', $user['user_form_salt'] . $this->push_token_map[$notification->notification_type_id][$notification->item_id][$notification->user_id]),
 			];
 			$json_data = json_encode($data);
 
@@ -531,6 +534,6 @@ protected function is_subscription_unauthorized(\Minishlink\WebPush\MessageSentR
 	protected function is_endpoint_permanently_removed(string $endpoint): bool
 	{
 		$host = parse_url($endpoint, PHP_URL_HOST);
-		return $host !== null && substr($host, -strlen('.invalid')) === '.invalid';
+		return is_string($host) && substr($host, -strlen('.invalid')) === '.invalid';
 	}
 }

styles/all/template/webpush.js

@@ -479,19 +479,29 @@ function PhpbbWebpush() {
 			},
 			body: getFormData({ endpoint: subscription.endpoint }),
 		})
-			.then(() => {
-				loadingIndicator.fadeOut(phpbb.alertTime);
-				return subscription.unsubscribe();
-			})
-			.then(unsubscribed => {
+			.then(async(response) => {
+				let data = null;
+				try {
+					data = await response.json();
+				} catch (e) {
+					// Ignore JSON parsing failures and fall back below.
+				}
+				if (!response.ok || !data || !data.success) {
+					throw new Error(data && data.message ? data.message : 'Unsubscribe failed.');
+				}
+
+				const unsubscribed = await subscription.unsubscribe();
+
 				if (unsubscribed) {
 					removeStoredSubscription(subscription.endpoint);
 					setSubscriptionState(false);
 				}
 			})
 			.catch(error => {
+				phpbb.alert(ajaxErrorTitle, error.message || error);
+			})
+			.finally(() => {
 				loadingIndicator.fadeOut(phpbb.alertTime);
-				phpbb.alert(ajaxErrorTitle, error);
 			});
 	}
 

tests/notification/notification_method_webpush_test.php

@@ -19,9 +19,6 @@
 require_once __DIR__ . '/../../../../../../tests/notification/base.php';
 require_once __DIR__ . '/../../vendor/autoload.php'; // load the composer dependencies for this extension
 
-/**
- * @group slow
- */
 class notification_method_webpush_test extends \phpbb_tests_notification_base
 {
 	/** @var string[] VAPID keys for testing purposes */
@@ -648,6 +645,58 @@ public function test_get_type(): void
 		$this->assertEquals('notification.method.phpbb.wpn.webpush', $this->notification_method_webpush->get_type());
 	}
 
+	public function test_push_token_map_is_per_user(): void
+	{
+		// Verifies that when multiple users are notified about the same item,
+		// each user's push token is stored and used independently.
+		// Previously the map was keyed [type_id][item_id], so the last user's
+		// token overwrote all others, making every other user's token invalid.
+		$subscription_info = [];
+		$expected_users = [2 => ['user_id' => '2'], 3 => ['user_id' => '3'], 4 => ['user_id' => '4']];
+		foreach ($expected_users as $user_id => $user_data)
+		{
+			$subscription_info[$user_id] = $this->create_subscription_for_user($user_id);
+		}
+
+		$post_data = [
+			'post_time'		=> 1349413322,
+			'poster_id'		=> 1,
+			'topic_title'	=> '',
+			'post_subject'	=> '',
+			'post_username'	=> '',
+			'forum_name'	=> '',
+			'forum_id'		=> '1',
+			'post_id'		=> '2',
+			'topic_id'		=> '1',
+		];
+
+		$this->notifications->add_notifications('notification.type.post', $post_data);
+
+		// Fetch the stored rows only for users we created subscriptions for
+		$webpush_table = $this->container->getParameter('tables.phpbb.wpn.notification_push');
+		$sql = 'SELECT user_id, push_token FROM ' . $webpush_table . ' WHERE ' . $this->db->sql_in_set('user_id', array_keys($expected_users)) . ' ORDER BY user_id ASC';
+		$result = $this->db->sql_query($sql);
+		$rows = $this->db->sql_fetchrowset($result);
+		$this->db->sql_freeresult($result);
+
+		$this->assertCount(count($expected_users), $rows, 'Each expected user must have a notification row');
+		$tokens = array_column($rows, 'push_token');
+		$this->assertEquals(count($tokens), count(array_unique($tokens)), 'Each user must have a unique push_token');
+
+		// Verify each message payload contains the token hashed with that specific user's salt
+		foreach ($rows as $row)
+		{
+			$user_id = (int) $row['user_id'];
+			$client_hash = basename($subscription_info[$user_id]['endpoint']);
+			$messages = $this->get_messages_for_subscription($client_hash);
+			$this->assertNotEmpty($messages);
+			$payload = json_decode($messages[0], true);
+			$user = $this->user_loader->get_user($user_id);
+			$expected_token = hash('sha256', $user['user_form_salt'] . $row['push_token']);
+			$this->assertEquals($expected_token, $payload['token'], 'Token in push payload must match hash of that user\'s salt + their own push_token');
+		}
+	}
+
 	public function test_get_ucp_template_data_uses_millisecond_expiration_time(): void
 	{
 		$this->user->data['user_id'] = 2;

ucp/controller/webpush.php

@@ -304,7 +304,7 @@ public function is_valid_endpoint(string $endpoint): bool
 	{
 		$host = parse_url($endpoint, PHP_URL_HOST);
 
-		if (empty($host))
+		if (!is_string($host) || empty($host))
 		{
 			return false;
 		}
@@ -409,8 +409,13 @@ public function subscribe(symfony_request $symfony_request): JsonResponse
 	 */
 	protected function get_subscription_write_data(array $data): array
 	{
-		$p256dh = is_string($data['keys']['p256dh'] ?? null) ? $data['keys']['p256dh'] : '';
-		$auth = is_string($data['keys']['auth'] ?? null) ? $data['keys']['auth'] : '';
+		$keys = $data['keys'] ?? [];
+		if (!is_array($keys))
+		{
+			$keys = [];
+		}
+		$p256dh = is_string($keys['p256dh'] ?? null) ? $keys['p256dh'] : '';
+		$auth = is_string($keys['auth'] ?? null) ? $keys['auth'] : '';
 
 		if ($p256dh === '' || $auth === '')
 		{
@@ -458,7 +463,7 @@ public function unsubscribe(symfony_request $symfony_request): JsonResponse
 
 		$data = json_sanitizer::decode($symfony_request->get('data', ''));
 
-		$endpoint = $data['endpoint'];
+		$endpoint = is_string($data['endpoint'] ?? null) ? $data['endpoint'] : '';
 
 		$sql = 'DELETE FROM ' . $this->push_subscriptions_table . '
 			WHERE user_id = ' . (int) $this->user->id() . "