Skip to content

Commit f07385d

Browse files
iMattProiMattPro
authored
Merge pull request #123 from iMattPro/401-subs
Remove unauthorized subscriptions
2 parents 7fe0aa0 + f6694e0 commit f07385d

2 files changed

Lines changed: 172 additions & 1 deletion

File tree

‎notification/method/webpush.php‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,8 @@ protected function notify_using_webpush(): void
282282
if (!$report->isSuccess())
283283
{
284284
// Fill array of endpoints to remove if subscription has expired
285-
if ($report->isSubscriptionExpired())
285+
// Library checks for 404/410; we also check for 401 (Unauthorized)
286+
if ($report->isSubscriptionExpired() || $this->is_subscription_unauthorized($report))
286287
{
287288
$expired_endpoints[] = $report->getEndpoint();
288289
}
@@ -495,4 +496,20 @@ protected function set_endpoint_padding(\Minishlink\WebPush\WebPush $web_push, s
495496
}
496497
}
497498
}
499+
500+
/**
501+
* Check if subscription push failed with 401 Unauthorized status
502+
*
503+
* 401 indicates the push service no longer accepts this subscription,
504+
* typically due to revoked credentials or subscription no longer being valid.
505+
*
506+
* @param \Minishlink\WebPush\MessageSentReport $report
507+
*
508+
* @return bool True if subscription returned 401 Unauthorized
509+
*/
510+
protected function is_subscription_unauthorized(\Minishlink\WebPush\MessageSentReport $report): bool
511+
{
512+
$response = $report->getResponse();
513+
return $response && $response->getStatusCode() === 401;
514+
}
498515
}

‎tests/notification/notification_method_webpush_test.php‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -551,11 +551,143 @@ public function test_notify_expired($notification_type, $post_data, $expected_us
551551
}
552552
}
553553

554+
/**
555+
* @dataProvider data_notification_webpush
556+
*/
557+
public function test_expired_subscriptions_deleted($notification_type, $post_data, $expected_users)
558+
{
559+
// Skip test if no expected users
560+
if (empty($expected_users))
561+
{
562+
$this->assertTrue(true);
563+
return;
564+
}
565+
566+
$subscription_info = [];
567+
foreach ($expected_users as $user_id => $user_data)
568+
{
569+
$subscription_info[$user_id][] = $this->create_subscription_for_user($user_id);
570+
}
571+
572+
// Get first user and expire their subscription
573+
$first_user_id = array_key_first($expected_users);
574+
$first_user_sub = $subscription_info[$first_user_id][0];
575+
$this->expire_subscription($first_user_sub['clientHash']);
576+
577+
// Count subscriptions before notification
578+
$subscriptions_before = $this->get_subscription_count();
579+
$this->assertEquals(count($expected_users), $subscriptions_before, 'Expected ' . count($expected_users) . ' subscriptions before notification');
580+
581+
$post_data = array_merge([
582+
'post_time' => 1349413322,
583+
'poster_id' => 1,
584+
'topic_title' => '',
585+
'post_subject' => '',
586+
'post_username' => '',
587+
'forum_name' => '',
588+
], $post_data);
589+
590+
// Send notifications, which should trigger cleanup of expired subscription
591+
$this->notifications->add_notifications($notification_type, $post_data);
592+
593+
// Count subscriptions after notification - expired one should be deleted
594+
$subscriptions_after = $this->get_subscription_count();
595+
$this->assertEquals(count($expected_users) - 1, $subscriptions_after, 'Expected expired subscription to be deleted');
596+
597+
// Verify the expired subscription is actually gone
598+
$remaining_subs = $this->get_all_subscriptions();
599+
foreach ($remaining_subs as $sub)
600+
{
601+
$this->assertNotEquals($first_user_sub['endpoint'], $sub['endpoint'], 'Expired subscription should be deleted');
602+
}
603+
}
604+
554605
public function test_get_type(): void
555606
{
556607
$this->assertEquals('notification.method.phpbb.wpn.webpush', $this->notification_method_webpush->get_type());
557608
}
558609

610+
/**
611+
* Test is_subscription_unauthorized method with various HTTP status codes
612+
*/
613+
public function test_is_subscription_unauthorized(): void
614+
{
615+
$reflection = new \ReflectionMethod($this->notification_method_webpush, 'is_subscription_unauthorized');
616+
$reflection->setAccessible(true);
617+
618+
// Test 401 status (should return true)
619+
$response_401 = $this->createMockResponse(401);
620+
$request_401 = $this->createMockRequest();
621+
$report_401 = new \Minishlink\WebPush\MessageSentReport($request_401, $response_401, false, 'Unauthorized');
622+
$this->assertTrue($reflection->invoke($this->notification_method_webpush, $report_401), 'Expected 401 to be treated as unauthorized');
623+
624+
// Test 404 status (should return false, handled by isSubscriptionExpired)
625+
$response_404 = $this->createMockResponse(404);
626+
$request_404 = $this->createMockRequest();
627+
$report_404 = new \Minishlink\WebPush\MessageSentReport($request_404, $response_404, false, 'Not Found');
628+
$this->assertFalse($reflection->invoke($this->notification_method_webpush, $report_404), 'Expected 404 to not be treated as unauthorized');
629+
630+
// Test 410 status (should return false, handled by isSubscriptionExpired)
631+
$response_410 = $this->createMockResponse(410);
632+
$request_410 = $this->createMockRequest();
633+
$report_410 = new \Minishlink\WebPush\MessageSentReport($request_410, $response_410, false, 'Gone');
634+
$this->assertFalse($reflection->invoke($this->notification_method_webpush, $report_410), 'Expected 410 to not be treated as unauthorized');
635+
636+
// Test 429 status (should return false, temporary error)
637+
$response_429 = $this->createMockResponse(429);
638+
$request_429 = $this->createMockRequest();
639+
$report_429 = new \Minishlink\WebPush\MessageSentReport($request_429, $response_429, false, 'Too Many Requests');
640+
$this->assertFalse($reflection->invoke($this->notification_method_webpush, $report_429), 'Expected 429 to not be treated as unauthorized');
641+
642+
// Test 500 status (should return false, temporary error)
643+
$response_500 = $this->createMockResponse(500);
644+
$request_500 = $this->createMockRequest();
645+
$report_500 = new \Minishlink\WebPush\MessageSentReport($request_500, $response_500, false, 'Internal Server Error');
646+
$this->assertFalse($reflection->invoke($this->notification_method_webpush, $report_500), 'Expected 500 to not be treated as unauthorized');
647+
648+
// Test null response (network failure - should return false)
649+
$request_null = $this->createMockRequest();
650+
$report_null = new \Minishlink\WebPush\MessageSentReport($request_null, null, false, 'Network error');
651+
$this->assertFalse($reflection->invoke($this->notification_method_webpush, $report_null), 'Expected null response to not be treated as unauthorized');
652+
}
653+
654+
/**
655+
* Create a mock PSR-7 ResponseInterface with specified status code
656+
*/
657+
protected function createMockResponse(int $status_code): \Psr\Http\Message\ResponseInterface
658+
{
659+
$response = $this->getMockBuilder(\Psr\Http\Message\ResponseInterface::class)
660+
->getMock();
661+
$response->method('getStatusCode')
662+
->willReturn($status_code);
663+
return $response;
664+
}
665+
666+
/**
667+
* Create a mock PSR-7 RequestInterface
668+
*/
669+
protected function createMockRequest(): \Psr\Http\Message\RequestInterface
670+
{
671+
$uri = $this->getMockBuilder(\Psr\Http\Message\UriInterface::class)
672+
->getMock();
673+
$uri->method('__toString')
674+
->willReturn('http://localhost:9012/notify/test');
675+
676+
$request = $this->getMockBuilder(\Psr\Http\Message\RequestInterface::class)
677+
->getMock();
678+
$request->method('getUri')
679+
->willReturn($uri);
680+
681+
$body = $this->getMockBuilder(\Psr\Http\Message\StreamInterface::class)
682+
->getMock();
683+
$body->method('getContents')
684+
->willReturn('test payload');
685+
$request->method('getBody')
686+
->willReturn($body);
687+
688+
return $request;
689+
}
690+
559691
/**
560692
* @dataProvider data_notification_webpush
561693
*/
@@ -751,6 +883,28 @@ protected function get_notifications(): array
751883
return $sql_ary;
752884
}
753885

886+
protected function get_subscription_count(): int
887+
{
888+
$push_subscriptions_table = $this->container->getParameter('tables.phpbb.wpn.push_subscriptions');
889+
$sql = 'SELECT COUNT(*) as count FROM ' . $push_subscriptions_table;
890+
$result = $this->db->sql_query($sql);
891+
$count = (int) $this->db->sql_fetchfield('count');
892+
$this->db->sql_freeresult($result);
893+
894+
return $count;
895+
}
896+
897+
protected function get_all_subscriptions(): array
898+
{
899+
$push_subscriptions_table = $this->container->getParameter('tables.phpbb.wpn.push_subscriptions');
900+
$sql = 'SELECT * FROM ' . $push_subscriptions_table;
901+
$result = $this->db->sql_query($sql);
902+
$sql_ary = $this->db->sql_fetchrowset($result);
903+
$this->db->sql_freeresult($result);
904+
905+
return $sql_ary;
906+
}
907+
754908
/**
755909
* @depends test_get_subscription
756910
*/

0 commit comments

Comments
 (0)