[Push] Delete expired subscriptions

This CL will check for and remove expired subscriptions after the
profile is initialized.

By enabling the feature flag for push subscriptions with expiration
time, a expiration time of kPushSubscriptionExpirationPeriodTimeDelta
specified in chrome/browser/push_messaging/push_messaging_constants.h
is added to push subscriptions.

These subscriptions become invalid after they expired and need to be
removed from the browser.


Bug: 1119690
Test: unit_tests --gtest_filter=Push*Remove*
Change-Id: I1d899daa0e79ff7fd4b08b0bc70ef273f9a54872
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2365953Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Richard Knoll <knollr@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Commit-Queue: Viviane Yang <viviy@google.com>
Cr-Commit-Position: refs/heads/master@{#800487}

chrome/browser/push_messaging/push_messaging_app_identifier.cc

@@ -252,6 +252,10 @@ PushMessagingAppIdentifier::PushMessagingAppIdentifier(
 
 PushMessagingAppIdentifier::~PushMessagingAppIdentifier() {}
 
+bool PushMessagingAppIdentifier::IsExpired() const {
+  return (expiration_time_) ? *expiration_time_ < base::Time::Now() : false;
+}
+
 void PushMessagingAppIdentifier::PersistToPrefs(Profile* profile) const {
   DCheckValid();
 

chrome/browser/push_messaging/push_messaging_app_identifier.h

@@ -104,6 +104,8 @@ class PushMessagingAppIdentifier {
     expiration_time_ = expiration_time;
   }
 
+  bool IsExpired() const;
+
   base::Optional<base::Time> expiration_time() const {
     DCHECK(!is_null());
     return expiration_time_;

chrome/browser/push_messaging/push_messaging_service_impl.cc

@@ -202,8 +202,59 @@ void PushMessagingServiceImpl::InitializeForProfile(Profile* profile) {
 
   PushMessagingServiceImpl* push_service =
       PushMessagingServiceFactory::GetForProfile(profile);
-  if (push_service)
+  if (push_service) {
     push_service->IncreasePushSubscriptionCount(count, false /* is_pending */);
+    push_service->RemoveExpiredSubscriptions();
+  }
+}
+
+void PushMessagingServiceImpl::RemoveExpiredSubscriptions() {
+  if (!base::FeatureList::IsEnabled(
+          features::kPushSubscriptionWithExpirationTime)) {
+    return;
+  }
+
+  base::RepeatingClosure barrier_closure = base::BarrierClosure(
+      PushMessagingAppIdentifier::GetCount(profile_),
+      remove_expired_subscriptions_callback_for_testing_.is_null()
+          ? base::DoNothing()
+          : std::move(remove_expired_subscriptions_callback_for_testing_));
+
+  for (const auto& identifier : PushMessagingAppIdentifier::GetAll(profile_)) {
+    if (!identifier.IsExpired()) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, barrier_closure);
+      continue;
+    }
+    content::BrowserThread::PostBestEffortTask(
+        FROM_HERE, base::ThreadTaskRunnerHandle::Get(),
+        base::BindOnce(
+            &PushMessagingServiceImpl::UnexpectedChange,
+            weak_factory_.GetWeakPtr(), identifier,
+            blink::mojom::PushUnregistrationReason::SUBSCRIPTION_EXPIRED,
+            barrier_closure));
+  }
+}
+
+void PushMessagingServiceImpl::UnexpectedChange(
+    PushMessagingAppIdentifier identifier,
+    blink::mojom::PushUnregistrationReason reason,
+    base::OnceClosure completed_closure) {
+  auto unsubscribe_closure =
+      base::BindOnce(&PushMessagingServiceImpl::UnexpectedUnsubscribe,
+                     weak_factory_.GetWeakPtr(), identifier, reason,
+                     base::BindOnce(&UnregisterCallbackToClosure,
+                                    std::move(completed_closure)));
+  if (base::FeatureList::IsEnabled(features::kPushSubscriptionChangeEvent)) {
+    // Find old subscription and fire a `pushsubscriptionchange` event
+    GetPushSubscriptionFromAppIdentifier(
+        identifier,
+        base::BindOnce(&PushMessagingServiceImpl::FirePushSubscriptionChange,
+                       weak_factory_.GetWeakPtr(), identifier,
+                       std::move(unsubscribe_closure),
+                       nullptr /* new_subscription */));
+  } else {
+    std::move(unsubscribe_closure).Run();
+  }
 }
 
 PushMessagingServiceImpl::PushMessagingServiceImpl(Profile* profile)
@@ -985,7 +1036,7 @@ void PushMessagingServiceImpl::DidClearPushSubscriptionId(
                        weak_factory_.GetWeakPtr(), was_subscribed);
 #if defined(OS_ANDROID)
     // On Android the backend is different, and requires the original sender_id.
-    // DidGetSenderIdUnsubscribePermissionRevoked and
+    // DidGetSenderIdUnexpectedUnsubscribe and
     // DidDeleteServiceWorkerRegistration sometimes call us with an empty one.
     if (sender_id.empty()) {
       std::move(unregister_callback).Run(gcm::GCMClient::INVALID_PARAMETER);
@@ -1135,26 +1186,15 @@ void PushMessagingServiceImpl::OnContentSettingChanged(
       continue;
     }
 
-    auto unsubscribe_closure = base::BindOnce(
-        &PushMessagingServiceImpl::UnsubscribePermissionRevoked,
-        weak_factory_.GetWeakPtr(), app_identifier,
-        base::BindOnce(&UnregisterCallbackToClosure, barrier_closure));
-    // Fire pushsubscriptionchange and then unsubscribe if flag enabled
-    if (base::FeatureList::IsEnabled(features::kPushSubscriptionChangeEvent)) {
-      GetPushSubscriptionFromAppIdentifier(
-          app_identifier,
-          base::BindOnce(&PushMessagingServiceImpl::FirePushSubscriptionChange,
-                         weak_factory_.GetWeakPtr(), app_identifier,
-                         std::move(unsubscribe_closure),
-                         nullptr /* new_subscription */));
-    } else {
-      std::move(unsubscribe_closure).Run();
-    }
+    UnexpectedChange(app_identifier,
+                     blink::mojom::PushUnregistrationReason::PERMISSION_REVOKED,
+                     barrier_closure);
   }
 }
 
-void PushMessagingServiceImpl::UnsubscribePermissionRevoked(
+void PushMessagingServiceImpl::UnexpectedUnsubscribe(
     const PushMessagingAppIdentifier& app_identifier,
+    blink::mojom::PushUnregistrationReason reason,
     UnregisterCallback unregister_callback) {
   // When `pushsubscriptionchange` is supported by default, get |sender_id| from
   // GetPushSubscriptionFromAppIdentifier callback and do not get the info from
@@ -1165,19 +1205,19 @@ void PushMessagingServiceImpl::UnsubscribePermissionRevoked(
         !PushMessagingAppIdentifier::UseInstanceID(app_identifier.app_id());
 #endif
     if (need_sender_id) {
-      GetSenderId(profile_, app_identifier.origin(),
-                  app_identifier.service_worker_registration_id(),
-                  base::BindOnce(&PushMessagingServiceImpl::
-                                     DidGetSenderIdUnsubscribePermissionRevoked,
-                                 weak_factory_.GetWeakPtr(), app_identifier,
-                                 std::move(unregister_callback)));
-    } else {
-      UnsubscribeInternal(
-          blink::mojom::PushUnregistrationReason::PERMISSION_REVOKED,
-          app_identifier.origin(),
+      GetSenderId(
+          profile_, app_identifier.origin(),
           app_identifier.service_worker_registration_id(),
-          app_identifier.app_id(), std::string() /* sender_id */,
-          std::move(unregister_callback));
+          base::BindOnce(
+              &PushMessagingServiceImpl::DidGetSenderIdUnexpectedUnsubscribe,
+              weak_factory_.GetWeakPtr(), app_identifier, reason,
+              std::move(unregister_callback)));
+    } else {
+      UnsubscribeInternal(reason, app_identifier.origin(),
+                          app_identifier.service_worker_registration_id(),
+                          app_identifier.app_id(),
+                          std::string() /* sender_id */,
+                          std::move(unregister_callback));
     }
 }
 
@@ -1264,8 +1304,9 @@ void PushMessagingServiceImpl::FirePushSubscriptionChangeCallback(
   RecordPushSubcriptionChangeStatus(status);
 }
 
-void PushMessagingServiceImpl::DidGetSenderIdUnsubscribePermissionRevoked(
+void PushMessagingServiceImpl::DidGetSenderIdUnexpectedUnsubscribe(
     const PushMessagingAppIdentifier& app_identifier,
+    blink::mojom::PushUnregistrationReason reason,
     UnregisterCallback callback,
     const std::string& sender_id) {
   // Unsubscribe the PushMessagingAppIdentifier with the push service.
@@ -1275,10 +1316,9 @@ void PushMessagingServiceImpl::DidGetSenderIdUnsubscribePermissionRevoked(
   // Android, Unsubscribe will just delete the app identifier to block future
   // messages.
   // TODO(johnme): Auto-unregister before SW DB is cleared (crbug.com/402458).
-  UnsubscribeInternal(
-      blink::mojom::PushUnregistrationReason::PERMISSION_REVOKED,
-      app_identifier.origin(), app_identifier.service_worker_registration_id(),
-      app_identifier.app_id(), sender_id, std::move(callback));
+  UnsubscribeInternal(reason, app_identifier.origin(),
+                      app_identifier.service_worker_registration_id(),
+                      app_identifier.app_id(), sender_id, std::move(callback));
 }
 
 void PushMessagingServiceImpl::SetContentSettingChangedCallbackForTesting(
@@ -1308,6 +1348,11 @@ void PushMessagingServiceImpl::Observe(
 
 // Helper methods --------------------------------------------------------------
 
+void PushMessagingServiceImpl::SetRemoveExpiredSubscriptionsCallbackForTesting(
+    base::OnceClosure closure) {
+  remove_expired_subscriptions_callback_for_testing_ = std::move(closure);
+}
+
 std::string PushMessagingServiceImpl::NormalizeSenderInfo(
     const std::string& application_server_key) const {
   if (!IsVapidKey(application_server_key))

chrome/browser/push_messaging/push_messaging_service_impl.h

@@ -70,6 +70,9 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
   explicit PushMessagingServiceImpl(Profile* profile);
   ~PushMessagingServiceImpl() override;
 
+  // Check and remove subscriptions that are expired when |this| is initialized
+  void RemoveExpiredSubscriptions();
+
   // Gets the permission status for the given |origin|.
   blink::mojom::PermissionStatus GetPermissionStatus(const GURL& origin,
                                                      bool user_visible);
@@ -150,6 +153,8 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
       base::RepeatingClosure callback);
   void SetServiceWorkerDatabaseWipedCallbackForTesting(
       base::RepeatingClosure callback);
+  void SetRemoveExpiredSubscriptionsCallbackForTesting(
+      base::OnceClosure closure);
 
  private:
   friend class PushMessagingBrowserTest;
@@ -251,15 +256,6 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
 
   // OnContentSettingChanged methods -------------------------------------------
 
-  void UnsubscribePermissionRevoked(
-      const PushMessagingAppIdentifier& app_identifier,
-      UnregisterCallback unregister_callback);
-
-  void DidGetSenderIdUnsubscribePermissionRevoked(
-      const PushMessagingAppIdentifier& app_identifier,
-      UnregisterCallback callback,
-      const std::string& sender_id);
-
   void GetPushSubscriptionFromAppIdentifier(
       const PushMessagingAppIdentifier& app_identifier,
       base::OnceCallback<void(blink::mojom::PushSubscriptionPtr)> callback);
@@ -280,6 +276,25 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
       const std::vector<uint8_t>& auth);
 
   // Helper methods ------------------------------------------------------------
+
+  // The subscription given in |identifier| will be unsubscribed (and a
+  // `pushsubscriptionchange` event fires if
+  // features::kPushSubscriptionChangeEvent is enabled)
+  // |completed_closure|
+  void UnexpectedChange(PushMessagingAppIdentifier identifier,
+                        blink::mojom::PushUnregistrationReason reason,
+                        base::OnceClosure completed_closure);
+
+  void UnexpectedUnsubscribe(const PushMessagingAppIdentifier& app_identifier,
+                             blink::mojom::PushUnregistrationReason reason,
+                             UnregisterCallback unregister_callback);
+
+  void DidGetSenderIdUnexpectedUnsubscribe(
+      const PushMessagingAppIdentifier& app_identifier,
+      blink::mojom::PushUnregistrationReason reason,
+      UnregisterCallback callback,
+      const std::string& sender_id);
+
   void FirePushSubscriptionChangeCallback(
       const PushMessagingAppIdentifier& app_identifier,
       blink::mojom::PushEventStatus status);
@@ -334,6 +349,7 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
   base::Closure content_setting_changed_callback_for_testing_;
   base::Closure service_worker_unregistered_callback_for_testing_;
   base::Closure service_worker_database_wiped_callback_for_testing_;
+  base::OnceClosure remove_expired_subscriptions_callback_for_testing_;
 
   PushMessagingNotificationManager notification_manager_;
 

chrome/browser/push_messaging/push_messaging_service_unittest.cc

@@ -14,12 +14,14 @@
 #include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/gcm/gcm_profile_service_factory.h"
 #include "chrome/browser/permissions/permission_manager_factory.h"
 #include "chrome/browser/push_messaging/push_messaging_app_identifier.h"
+#include "chrome/browser/push_messaging/push_messaging_features.h"
 #include "chrome/browser/push_messaging/push_messaging_service_factory.h"
 #include "chrome/browser/push_messaging/push_messaging_service_impl.h"
 #include "chrome/test/base/testing_profile.h"
@@ -29,6 +31,7 @@
 #include "components/gcm_driver/fake_gcm_profile_service.h"
 #include "components/gcm_driver/gcm_profile_service.h"
 #include "components/permissions/permission_manager.h"
+#include "content/public/common/content_features.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/mojom/push_messaging/push_messaging_status.mojom.h"
@@ -144,6 +147,67 @@ class PushMessagingServiceTest : public ::testing::Test {
     *payload_out = std::move(payload);
   }
 
+  class TestPushSubscription {
+   public:
+    std::string subscription_id_;
+    GURL endpoint_;
+    base::Optional<base::Time> expiration_time_;
+    std::vector<uint8_t> p256dh_;
+    std::vector<uint8_t> auth_;
+    TestPushSubscription(const std::string& subscription_id,
+                         const GURL& endpoint,
+                         const base::Optional<base::Time>& expiration_time,
+                         const std::vector<uint8_t>& p256dh,
+                         const std::vector<uint8_t>& auth)
+        : subscription_id_(subscription_id),
+          endpoint_(endpoint),
+          expiration_time_(expiration_time),
+          p256dh_(p256dh),
+          auth_(auth) {}
+    TestPushSubscription() = default;
+  };
+
+  void Subscribe(PushMessagingServiceImpl* push_service,
+                 const GURL& origin,
+                 TestPushSubscription* subscription = nullptr) {
+    std::string subscription_id;
+    GURL endpoint;
+    base::Optional<base::Time> expiration_time;
+    std::vector<uint8_t> p256dh, auth;
+
+    base::RunLoop run_loop;
+
+    auto options = blink::mojom::PushSubscriptionOptions::New();
+    options->user_visible_only = true;
+    options->application_server_key = std::vector<uint8_t>(
+        kTestSenderId,
+        kTestSenderId + sizeof(kTestSenderId) / sizeof(char) - 1);
+
+    push_service->SubscribeFromWorker(
+        origin, kTestServiceWorkerId, std::move(options),
+        base::BindOnce(&PushMessagingServiceTest::DidRegister,
+                       base::Unretained(this), &subscription_id, &endpoint,
+                       &expiration_time, &p256dh, &auth,
+                       run_loop.QuitClosure()));
+
+    EXPECT_EQ(0u, subscription_id.size());  // this must be asynchronous
+
+    run_loop.Run();
+
+    ASSERT_GT(subscription_id.size(), 0u);
+    ASSERT_TRUE(endpoint.is_valid());
+    ASSERT_GT(endpoint.spec().size(), 0u);
+    ASSERT_GT(p256dh.size(), 0u);
+    ASSERT_GT(auth.size(), 0u);
+
+    if (subscription) {
+      subscription->subscription_id_ = subscription_id;
+      subscription->endpoint_ = endpoint;
+      subscription->p256dh_ = p256dh;
+      subscription->auth_ = auth;
+    }
+  }
+
  protected:
   PushMessagingTestingProfile* profile() { return &profile_; }
 
@@ -167,36 +231,10 @@ TEST_F(PushMessagingServiceTest, PayloadEncryptionTest) {
   ASSERT_EQ(blink::mojom::PermissionStatus::GRANTED,
             push_service->GetPermissionStatus(origin, true));
 
-  std::string subscription_id;
-  GURL endpoint;
-  base::Optional<base::Time> expiration_time;
-  std::vector<uint8_t> p256dh, auth;
-
-  base::RunLoop run_loop;
-
   // (2) Subscribe for Push Messaging, and verify that we've got the required
   // information in order to be able to create encrypted messages.
-  auto options = blink::mojom::PushSubscriptionOptions::New();
-  options->user_visible_only = true;
-  options->application_server_key = std::vector<uint8_t>(
-      kTestSenderId, kTestSenderId + sizeof(kTestSenderId) / sizeof(char) - 1);
-
-  push_service->SubscribeFromWorker(
-      origin, kTestServiceWorkerId, std::move(options),
-      base::BindOnce(&PushMessagingServiceTest::DidRegister,
-                     base::Unretained(this), &subscription_id, &endpoint,
-                     &expiration_time, &p256dh, &auth, run_loop.QuitClosure()));
-
-  EXPECT_EQ(0u, subscription_id.size());  // this must be asynchronous
-
-  run_loop.Run();
-
-  ASSERT_GT(subscription_id.size(), 0u);
-  ASSERT_TRUE(endpoint.is_valid());
-  ASSERT_GT(endpoint.spec().size(), 0u);
-  ASSERT_GT(p256dh.size(), 0u);
-  ASSERT_GT(auth.size(), 0u);
-  ASSERT_EQ(expiration_time, base::nullopt);
+  TestPushSubscription subscription;
+  Subscribe(push_service, origin, &subscription);
 
   // (3) Encrypt a message using the public key and authentication secret that
   // are associated with the subscription.
@@ -206,8 +244,10 @@ TEST_F(PushMessagingServiceTest, PayloadEncryptionTest) {
 
   ASSERT_TRUE(gcm::CreateEncryptedPayloadForTesting(
       kTestPayload,
-      base::StringPiece(reinterpret_cast<char*>(p256dh.data()), p256dh.size()),
-      base::StringPiece(reinterpret_cast<char*>(auth.data()), auth.size()),
+      base::StringPiece(reinterpret_cast<char*>(subscription.p256dh_.data()),
+                        subscription.p256dh_.size()),
+      base::StringPiece(reinterpret_cast<char*>(subscription.auth_.data()),
+                        subscription.auth_.size()),
       &message));
 
   ASSERT_GT(message.raw_data.size(), 0u);
@@ -271,3 +311,47 @@ TEST_F(PushMessagingServiceTest, NormalizeSenderInfo) {
 
   EXPECT_EQ(p256dh, push_service->NormalizeSenderInfo(p256dh));
 }
+
+TEST_F(PushMessagingServiceTest, RemoveExpiredSubscriptions) {
+  // (1) Enable push subscriptions with expiration time and
+  // `pushsubscriptionchange` events
+  base::test::ScopedFeatureList scoped_feature_list_;
+  scoped_feature_list_.InitWithFeatures(
+      /* enabled features */
+      {features::kPushSubscriptionWithExpirationTime,
+       features::kPushSubscriptionChangeEvent},
+      /* disabled features */
+      {});
+
+  // (2) Set up push service and test origin
+  PushMessagingServiceImpl* push_service = profile()->GetPushMessagingService();
+  ASSERT_TRUE(push_service);
+  const GURL origin(kTestOrigin);
+
+  // (3) Subscribe origin to push service and find corresponding
+  // |app_identifier|
+  Subscribe(push_service, origin);
+  PushMessagingAppIdentifier app_identifier =
+      PushMessagingAppIdentifier::FindByServiceWorker(profile(), origin,
+                                                      kTestServiceWorkerId);
+  ASSERT_FALSE(app_identifier.is_null());
+
+  // (4) Manually set the time as expired, save the time in preferences
+  app_identifier.set_expiration_time(base::Time::UnixEpoch());
+  app_identifier.PersistToPrefs(profile());
+  ASSERT_EQ(1u, PushMessagingAppIdentifier::GetCount(profile()));
+
+  // (3) Remove all expired subscriptions
+  base::RunLoop run_loop;
+  push_service->SetRemoveExpiredSubscriptionsCallbackForTesting(
+      run_loop.QuitClosure());
+  push_service->RemoveExpiredSubscriptions();
+  run_loop.Run();
+
+  // (5) We expect the subscription to be deleted
+  ASSERT_EQ(0u, PushMessagingAppIdentifier::GetCount(profile()));
+  PushMessagingAppIdentifier deleted_identifier =
+      PushMessagingAppIdentifier::FindByAppId(profile(),
+                                              app_identifier.app_id());
+  EXPECT_TRUE(deleted_identifier.is_null());
+}

third_party/blink/public/mojom/push_messaging/push_messaging_status.mojom

@@ -182,6 +182,10 @@ enum PushUnregistrationReason {
   // The Service Worker database got wiped, most likely due to corruption.
   SERVICE_WORKER_DATABASE_WIPED = 10,
 
+  // The subscription has expired, delete local subscription automatically.
+  // Only happens if features::kPushSubscriptionWithExpirationTime is enabled.
+  SUBSCRIPTION_EXPIRED = 11,
+
   // NOTE: Do not renumber or delete these as that would confuse interpretation
   // of previously logged data. When making changes, also update the enum list
   // in tools/metrics/histograms/histograms.xml to keep it in sync.

tools/metrics/histograms/enums.xml

@@ -58930,6 +58930,7 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
   <int value="8" label="subscribe() storage corrupt"/>
   <int value="9" label="getSubscription() storage corrupt"/>
   <int value="10" label="Service Worker database got wiped"/>
+  <int value="11" label="Subscription has expired"/>
 </enum>
 
 <enum name="PushUnregistrationStatus">