From 7eb93e811e5eb3bf064b26ea79a2d1fb6a4b447f Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Fri, 25 Nov 2022 01:49:12 +0000 Subject: [PATCH] Revert "Revoke site-level Notifications permission." This reverts commit 9a8dfe09ddb8860885c903205d897dae0e0d7ec7. Reason for revert: suspect causing lint failure on android bots. Sample build faliure: https://ci.chromium.org/ui/p/chromium/builders/ci/android-archive-rel/29557/overview Sample log: ../../chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:586: Warning: This method should only be accessed from tests or within private scope [VisibleForTests] NotificationSystemStatusUtil.getAppNotificationStatus() ~~~~~~~~~~~~~~~~~~~~~~~~ 0 errors, 1 warnings (506 errors, 235 warnings filtered by baseline lint-baseline.xml) Original change's description: > Revoke site-level Notifications permission. > > This CL revokes site-level Notifications permission if Chrome has no > app-level Notifications permission on Android. There is a 3-day grace > period that starts as soon as Chrome tries to show a notification but > has no Notifications permissions. > > In this CL I'm moving triggering Notifications permission revocation > logic from `PushMessagingServiceImpl` to `NotificationPlatformBridge`. > The reason for that is we believe `PushMessagingServiceImpl` is too > early in the push message processing pipeline and in case of a cold > launch it may have inconsistent results regarding webapps verification. > > `NotificationPlatformBridge::displayNotificationInternal` is at the end > of the pipeline and allows filtering webapps from sites without side > effects. > > In the current iteration, we entirely skip the webapps. After a > successful launch, I will follow up by including webapps in > permission revocation. > > Bug: 1378515 > Change-Id: I786ac8c122b96b2e4539b59728f65db2d44a470e > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4030307 > Reviewed-by: Peter Beverloo > Reviewed-by: Ravjit Uppal > Reviewed-by: Avi Drissman > Commit-Queue: Illia Klimov > Cr-Commit-Position: refs/heads/main@{#1075636} Bug: 1378515 Change-Id: I274cde3ab168aee62cd7c3e3398a98827f68e6e8 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4054886 Commit-Queue: Takashi Sakamoto Owners-Override: Takashi Sakamoto Bot-Commit: Rubber Stamper Cr-Commit-Position: refs/heads/main@{#1075660} --- chrome/android/BUILD.gn | 1 - chrome/android/chrome_java_sources.gni | 1 - .../NotificationPlatformBridge.java | 9 - .../PushMessagingServiceBridge.java | 47 -- .../push_messaging_service_impl.cc | 171 +++---- .../push_messaging_service_impl.h | 38 +- .../push_messaging_service_unittest.cc | 466 ++++++++++-------- tools/metrics/histograms/enums.xml | 6 - .../metadata/permissions/histograms.xml | 21 - 9 files changed, 373 insertions(+), 387 deletions(-) delete mode 100644 chrome/android/java/src/org/chromium/chrome/browser/notifications/PushMessagingServiceBridge.java diff --git a/chrome/android/BUILD.gn b/chrome/android/BUILD.gn index cae23b177c7f8..93bc8fb28ae8e 100644 --- a/chrome/android/BUILD.gn +++ b/chrome/android/BUILD.gn @@ -4107,7 +4107,6 @@ generate_jni("chrome_jni_headers") { "java/src/org/chromium/chrome/browser/notifications/ActionInfo.java", "java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java", "java/src/org/chromium/chrome/browser/notifications/NotificationTriggerScheduler.java", - "java/src/org/chromium/chrome/browser/notifications/PushMessagingServiceBridge.java", "java/src/org/chromium/chrome/browser/notifications/scheduler/DisplayAgent.java", "java/src/org/chromium/chrome/browser/notifications/scheduler/NotificationSchedulerTask.java", "java/src/org/chromium/chrome/browser/ntp/ForeignSessionHelper.java", diff --git a/chrome/android/chrome_java_sources.gni b/chrome/android/chrome_java_sources.gni index 929c98a270945..f615caadb653b 100644 --- a/chrome/android/chrome_java_sources.gni +++ b/chrome/android/chrome_java_sources.gni @@ -772,7 +772,6 @@ chrome_java_sources = [ "java/src/org/chromium/chrome/browser/notifications/NotificationServiceImpl.java", "java/src/org/chromium/chrome/browser/notifications/NotificationTriggerBackgroundTask.java", "java/src/org/chromium/chrome/browser/notifications/NotificationTriggerScheduler.java", - "java/src/org/chromium/chrome/browser/notifications/PushMessagingServiceBridge.java", "java/src/org/chromium/chrome/browser/notifications/WebPlatformNotificationMetrics.java", "java/src/org/chromium/chrome/browser/notifications/scheduler/DisplayAgent.java", "java/src/org/chromium/chrome/browser/notifications/scheduler/NotificationSchedulerTask.java", diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java index 58fbf22d340e9..632466226eafa 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java @@ -577,15 +577,6 @@ private void displayNotificationInternal(String notificationId, } else { suspendedCallback.onResult(false /* suspended */); } - - // If Chrome has no app-level notifications permission, check if an origin-level permission - // should be revoked. - // Notifications permission is not allowed for incognito profile. - if (!origin.isEmpty() && !incognito) { - PushMessagingServiceBridge.getInstance().verify(origin, profileId, - NotificationSystemStatusUtil.getAppNotificationStatus() - != NotificationSystemStatusUtil.APP_NOTIFICATIONS_STATUS_DISABLED); - } } private NotificationBuilderBase prepareNotificationBuilder(String notificationId, diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/PushMessagingServiceBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/PushMessagingServiceBridge.java deleted file mode 100644 index 4616a007c072c..0000000000000 --- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/PushMessagingServiceBridge.java +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright 2022 The Chromium Authors -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -package org.chromium.chrome.browser.notifications; - -import org.chromium.base.annotations.NativeMethods; - -/** - * Provides the ability for the PushMessagingServiceImpl to revoke Notifications permission. - * - * This class should only be used on the UI thread. - */ -public class PushMessagingServiceBridge { - private static PushMessagingServiceBridge sInstance; - - /** - * Returns the current instance of the PushMessagingServiceBridge. - * - * @return The instance of the PushMessagingServiceBridge. - */ - static PushMessagingServiceBridge getInstance() { - if (sInstance == null) { - sInstance = new PushMessagingServiceBridge(); - } - - return sInstance; - } - - /** - * Verifies if Notifications permission should be revoked for an origin. - * - * @param origin Full text of the origin, including the protocol, owning this notification. - * @param profileId Id of the profile that showed the notification. - * @param appLevelNotificationsEnabled Whether Chrome has app-level Notifications permission. - */ - public void verify(String origin, String profileId, boolean appLevelNotificationsEnabled) { - PushMessagingServiceBridgeJni.get().verifyAndRevokeNotificationsPermission( - origin, profileId, appLevelNotificationsEnabled); - } - - @NativeMethods - interface Natives { - void verifyAndRevokeNotificationsPermission( - String origin, String profileId, boolean appLevelNotificationsEnabled); - } -} diff --git a/chrome/browser/push_messaging/push_messaging_service_impl.cc b/chrome/browser/push_messaging/push_messaging_service_impl.cc index d7d70bad51824..680a1858caba9 100644 --- a/chrome/browser/push_messaging/push_messaging_service_impl.cc +++ b/chrome/browser/push_messaging/push_messaging_service_impl.cc @@ -79,20 +79,11 @@ #if BUILDFLAG(IS_ANDROID) #include "base/android/jni_android.h" -#include "base/android/jni_string.h" -#include "chrome/android/chrome_jni_headers/PushMessagingServiceBridge_jni.h" #include "chrome/android/chrome_jni_headers/PushMessagingServiceObserver_jni.h" #include "chrome/browser/android/shortcut_helper.h" -#include "chrome/browser/notifications/notification_platform_bridge.h" -#include "chrome/browser/profiles/profile_manager.h" #include "components/permissions/android/android_permission_util.h" #endif -#if BUILDFLAG(IS_ANDROID) -using base::android::ConvertJavaStringToUTF8; -using base::android::JavaParamRef; -#endif - using instance_id::InstanceID; namespace { @@ -395,6 +386,15 @@ void PushMessagingServiceImpl::OnMessage(const std::string& app_id, /* was_encrypted= */ message.decrypted, std::string() /* error_message */, message.decrypted ? message.raw_data : std::string()); +#if BUILDFLAG(IS_ANDROID) + if (CheckAndRevokeNotificationPermissionIfNeeded(app_id, message, + app_identifier)) { + // `message` is processed inside + // `CheckAndRevokeNotificationPermissionIfNeeded()`. + return; + } +#endif + if (IsPermissionSet(app_identifier.origin())) { messages_pending_permission_check_.emplace(app_id, message); // Start abusive and disruptive origin verifications only if no other @@ -434,6 +434,77 @@ void PushMessagingServiceImpl::CheckOriginAndDispatchNextMessage() { weak_factory_.GetWeakPtr(), std::move(message))); } +#if BUILDFLAG(IS_ANDROID) +bool PushMessagingServiceImpl::CheckAndRevokeNotificationPermissionIfNeeded( + const std::string& app_id, + const gcm::IncomingMessage& message, + const PushMessagingAppIdentifier& app_identifier) { + if (!base::FeatureList::IsEnabled( + features::kRevokeNotificationsPermissionIfDisabledOnAppLevel)) { + return false; + } + + bool has_app_level_notification_permission = + enabled_app_level_notification_permission_for_testing_.has_value() + ? enabled_app_level_notification_permission_for_testing_.value() + : permissions::AreAppLevelNotificationsEnabled(); + + bool contains_webapk = ShortcutHelper::DoesOriginContainAnyInstalledWebApk( + app_identifier.origin()); + bool contains_twa = + ShortcutHelper::DoesOriginContainAnyInstalledTrustedWebActivity( + app_identifier.origin()); + bool contains_installed_webapp = contains_twa || contains_webapk; + + // If Notifications permission delegation is enabled, for the + // `app_identifier.origin()`, we should not revoke permissions because + // Notifications permissions are automatically synced with an installed app. + if (contains_installed_webapp) + return false; + + PrefService* prefs = prefs_for_testing_.has_value() + ? prefs_for_testing_.value() + : g_browser_process->local_state(); + + if (has_app_level_notification_permission) { + // Chrome has app-level Notifications permission. Reset the grace period + // flag and continue as normal. + prefs->ClearPref(kNotificationsPermissionRevocationGracePeriodDate); + return false; + } + + // Chrome has no app-level Notifications permission. + blink::mojom::PushEventStatus status; + + if (prefs->GetTime(kNotificationsPermissionRevocationGracePeriodDate) == + base::Time()) { + prefs->SetTime(kNotificationsPermissionRevocationGracePeriodDate, + base::Time::Now()); + } + + base::TimeDelta permission_revocation_activated_duration = + base::Time::Now() - + prefs->GetTime(kNotificationsPermissionRevocationGracePeriodDate); + if (permission_revocation_activated_duration.InDays() < + GetNotificationsRevocationGracePeriodInDays()) { + // Ignore a push message during the grace period. + status = blink::mojom::PushEventStatus::NO_APP_LEVEL_PERMISSION_IGNORE; + } else { + // Revoke site-level Notifications permission & FCM. + status = blink::mojom::PushEventStatus::NO_APP_LEVEL_PERMISSION_UNSUBSCRIBE; + + profile_->GetPermissionController()->ResetPermission( + blink::PermissionType::NOTIFICATIONS, + url::Origin::Create(app_identifier.origin())); + } + + DeliverMessageCallback(app_id, app_identifier.origin(), + app_identifier.service_worker_registration_id(), + message, /*did_enqueue_message=*/false, status); + return true; +} +#endif + void PushMessagingServiceImpl::OnCheckedOrigin( PendingMessage message, PermissionRevocationRequest::Outcome outcome) { @@ -912,88 +983,6 @@ void PushMessagingServiceImpl::RegisterPrefs(PrefRegistrySimple* registry) { registry->RegisterTimePref(kNotificationsPermissionRevocationGracePeriodDate, base::Time()); } - -static void -JNI_PushMessagingServiceBridge_VerifyAndRevokeNotificationsPermission( - JNIEnv* env, - const JavaParamRef& java_origin, - const JavaParamRef& java_profile_id, - jboolean app_level_notifications_enabled) { - if (!base::FeatureList::IsEnabled( - features::kRevokeNotificationsPermissionIfDisabledOnAppLevel)) { - return; - } - - GURL origin(ConvertJavaStringToUTF8(env, java_origin)); - std::string profile_id = ConvertJavaStringToUTF8(env, java_profile_id); - - ProfileManager* profile_manager = g_browser_process->profile_manager(); - DCHECK(profile_manager); - - profile_manager->LoadProfile( - NotificationPlatformBridge::GetProfileBaseNameFromProfileId(profile_id), - /*incognito=*/false, - base::BindOnce(&PushMessagingServiceImpl::RevokePermissionIfPossible, - GURL(ConvertJavaStringToUTF8(env, java_origin)), - app_level_notifications_enabled, - g_browser_process->local_state())); -} - -void PushMessagingServiceImpl::RevokePermissionIfPossible( - GURL origin, - bool app_level_notifications_enabled, - PrefService* prefs, - Profile* profile) { - if (app_level_notifications_enabled) { - if (prefs->GetTime(kNotificationsPermissionRevocationGracePeriodDate) != - base::Time()) { - // Record when the grace period was started so we can adjust the grace - // period duration. - base::UmaHistogramLongTimes( - "Permission.FCM.Revocation.ResetGracePeriod", - base::Time::Now() - - prefs->GetTime( - kNotificationsPermissionRevocationGracePeriodDate)); - } - - base::UmaHistogramEnumeration("Permission.FCM.Revocation", - FcmTokenRevocation::kResetGracePeriod); - - // Chrome has app-level Notifications permission. Reset the grace period - // flag and continue as normal. - prefs->ClearPref(kNotificationsPermissionRevocationGracePeriodDate); - return; - } - - if (prefs->GetTime(kNotificationsPermissionRevocationGracePeriodDate) == - base::Time()) { - prefs->SetTime(kNotificationsPermissionRevocationGracePeriodDate, - base::Time::Now()); - return; - } - - base::TimeDelta permission_revocation_activated_duration = - base::Time::Now() - - prefs->GetTime(kNotificationsPermissionRevocationGracePeriodDate); - if (permission_revocation_activated_duration.InDays() >= - GetNotificationsRevocationGracePeriodInDays()) { - content::PermissionController* permission_controller = - profile->GetPermissionController(); - - // As soon as permission is reset, - // `PushMessagingServiceImpl::OnContentSettingChanged` is notified and it - // revokes a push message registration token. - permission_controller->ResetPermission(blink::PermissionType::NOTIFICATIONS, - url::Origin::Create(origin)); - - base::UmaHistogramEnumeration("Permission.FCM.Revocation", - FcmTokenRevocation::kRevokePermission); - } else { - base::UmaHistogramEnumeration("Permission.FCM.Revocation", - FcmTokenRevocation::kGracePeriodIsNotOver); - } -} - #endif bool PushMessagingServiceImpl::SupportNonVisibleMessages() { diff --git a/chrome/browser/push_messaging/push_messaging_service_impl.h b/chrome/browser/push_messaging/push_messaging_service_impl.h index 96958dab7b632..3229a0cb96f7e 100644 --- a/chrome/browser/push_messaging/push_messaging_service_impl.h +++ b/chrome/browser/push_messaging/push_messaging_service_impl.h @@ -38,6 +38,7 @@ class GURL; class PrefRegistrySimple; +class PrefService; class Profile; class PushMessagingAppIdentifier; class PushMessagingServiceTest; @@ -45,10 +46,6 @@ class FCMRevocationTest; class ScopedKeepAlive; class ScopedProfileKeepAlive; -#if BUILDFLAG(IS_ANDROID) -class PrefService; -#endif - namespace blink { namespace mojom { enum class PushEventStatus; @@ -69,14 +66,6 @@ class InstanceIDDriver; } // namespace instance_id namespace { - -enum class FcmTokenRevocation { - kResetGracePeriod = 0, - kRevokePermission = 1, - kGracePeriodIsNotOver = 2, - kMaxValue = kGracePeriodIsNotOver, -}; - struct PendingMessage { PendingMessage(std::string app_id, gcm::IncomingMessage message); PendingMessage(const PendingMessage& other); @@ -117,11 +106,6 @@ class PushMessagingServiceImpl : public content::PushMessagingService, #if BUILDFLAG(IS_ANDROID) // Registers Local State prefs used by this class. static void RegisterPrefs(PrefRegistrySimple* registry); - - static void RevokePermissionIfPossible(GURL origin, - bool app_level_notifications_enabled, - PrefService* prefs, - Profile* profile); #endif // gcm::GCMAppHandler implementation. @@ -211,6 +195,12 @@ class PushMessagingServiceImpl : public content::PushMessagingService, base::RepeatingClosure callback); void SetRemoveExpiredSubscriptionsCallbackForTesting( base::OnceClosure closure); + void set_enabled_app_level_notification_permission_for_testing(bool enabled) { + enabled_app_level_notification_permission_for_testing_ = enabled; + } + void set_prefs_for_testing(PrefService* prefs_for_testing) { + prefs_for_testing_ = prefs_for_testing; + } private: friend class PushMessagingBrowserTestBase; @@ -262,6 +252,17 @@ class PushMessagingServiceImpl : public content::PushMessagingService, void CheckOriginAndDispatchNextMessage(); +#if BUILDFLAG(IS_ANDROID) + // Verifies if Chrome has Android app-level Notifications permission. If + // app-level permission is missing, `message` will be ignored, and site-level + // Notifications permission and FCM will be revoked. + // + // Returns true if `message` should be ignored, returns false otherwise. + bool CheckAndRevokeNotificationPermissionIfNeeded( + const std::string& app_id, + const gcm::IncomingMessage& message, + const PushMessagingAppIdentifier& app_identifier); +#endif // Subscribe methods --------------------------------------------------------- void DoSubscribe(PushMessagingAppIdentifier app_identifier, @@ -498,6 +499,9 @@ class PushMessagingServiceImpl : public content::PushMessagingService, int render_process_id_ = content::ChildProcessHost::kInvalidUniqueID; + absl::optional enabled_app_level_notification_permission_for_testing_; + absl::optional prefs_for_testing_; + base::WeakPtrFactory weak_factory_{this}; }; diff --git a/chrome/browser/push_messaging/push_messaging_service_unittest.cc b/chrome/browser/push_messaging/push_messaging_service_unittest.cc index 44de4087e2ccf..a21a7ba146fa1 100644 --- a/chrome/browser/push_messaging/push_messaging_service_unittest.cc +++ b/chrome/browser/push_messaging/push_messaging_service_unittest.cc @@ -42,10 +42,6 @@ #include "components/gcm_driver/instance_id/instance_id_android.h" #include "components/gcm_driver/instance_id/scoped_use_fake_instance_id_android.h" #include "components/prefs/testing_pref_service.h" -#include "content/public/browser/permission_controller.h" -#include "third_party/blink/public/common/permissions/permission_utils.h" -#include "url/gurl.h" -#include "url/origin.h" #endif // BUILDFLAG(IS_ANDROID) namespace { @@ -105,6 +101,10 @@ constexpr base::TimeDelta kPushEventHandleTime = base::Seconds(10); class PushMessagingServiceTest : public ::testing::Test { public: PushMessagingServiceTest() { +#if BUILDFLAG(IS_ANDROID) + PushMessagingServiceImpl::RegisterPrefs(prefs_.registry()); +#endif + // Override the GCM Profile service so that we can send fake messages. gcm::GCMProfileServiceFactory::GetInstance()->SetTestingFactory( &profile_, base::BindRepeating(&BuildFakeGCMProfileService)); @@ -230,10 +230,16 @@ class PushMessagingServiceTest : public ::testing::Test { return task_environment_; } +#if BUILDFLAG(IS_ANDROID) + PrefService* get_pref() { return &prefs_; } +#endif + private: content::BrowserTaskEnvironment task_environment_{ base::test::TaskEnvironment::TimeSource::MOCK_TIME}; - +#if BUILDFLAG(IS_ANDROID) + TestingPrefServiceSimple prefs_; +#endif PushMessagingTestingProfile profile_; #if BUILDFLAG(IS_ANDROID) @@ -252,6 +258,13 @@ TEST_F(PushMessagingServiceTest, MAYBE_PayloadEncryptionTest) { PushMessagingServiceImpl* push_service = profile()->GetPushMessagingService(); ASSERT_TRUE(push_service); +#if BUILDFLAG(IS_ANDROID) + // Without Android app-level Notifications permission a push message will be + // ignored. + push_service->set_enabled_app_level_notification_permission_for_testing(true); + push_service->set_prefs_for_testing(get_pref()); +#endif + const GURL origin(kTestOrigin); SetPermission(origin, CONTENT_SETTING_ALLOW); @@ -401,6 +414,13 @@ TEST_F(PushMessagingServiceTest, TestMultipleIncomingPushMessages) { PushMessagingServiceImpl* push_service = profile()->GetPushMessagingService(); ASSERT_TRUE(push_service); +#if BUILDFLAG(IS_ANDROID) + // Without Android app-level Notifications permission a push message will be + // ignored. + push_service->set_enabled_app_level_notification_permission_for_testing(true); + push_service->set_prefs_for_testing(get_pref()); +#endif + // Subscribe |origin| to push service. Subscribe(push_service, origin); PushMessagingAppIdentifier app_identifier = @@ -497,264 +517,322 @@ class FCMRevocationTest : public PushMessagingServiceTest { FCMRevocationTest() { scoped_feature_list_.InitAndEnableFeature( features::kRevokeNotificationsPermissionIfDisabledOnAppLevel); - PushMessagingServiceImpl::RegisterPrefs(prefs_.registry()); } ~FCMRevocationTest() override = default; - GURL GetUrl() { return origin_; } - - url::Origin GetOrigin() { return url::Origin::Create(origin_); } - - PrefService* pref() { return &prefs_; } - - void SetPermission(const GURL& origin, - ContentSetting value, - TestingProfile* profile) { - HostContentSettingsMap* host_content_settings_map = - HostContentSettingsMapFactory::GetForProfile(profile); - host_content_settings_map->SetContentSettingDefaultScope( - origin, origin, ContentSettingsType::NOTIFICATIONS, value); - } - private: base::test::ScopedFeatureList scoped_feature_list_; - GURL origin_ = GURL("https://example.com"); - TestingPrefServiceSimple prefs_; }; -TEST_F(FCMRevocationTest, ResetPrefs) { - base::HistogramTester histogram_tester; - - content::PermissionController* permission_controller = - profile()->GetPermissionController(); - - content::PermissionResult result = - permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::ASK); - - SetPermission(GetUrl(), ContentSetting::CONTENT_SETTING_ALLOW, profile()); - - result = permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::GRANTED); +// Tests that the grace period preferences will be cleared if app-level +// permissions are enabled. +TEST_F(FCMRevocationTest, TestPermissionRevocationClearPreferences) { + const GURL origin(kTestOrigin); + SetPermission(origin, CONTENT_SETTING_ALLOW); const char kNotificationsPermissionRevocationGracePeriodDate[] = "notifications_permission_revocation_grace_period"; + PushMessagingServiceImpl* push_service = profile()->GetPushMessagingService(); + ASSERT_TRUE(push_service); + + // Without Android app-level Notifications permission a push message will be + // ignored. + push_service->set_enabled_app_level_notification_permission_for_testing(true); + push_service->set_prefs_for_testing(get_pref()); + // Just random value to make sure it is reset. base::Time time = base::Time::FromTimeT(100); - pref()->SetTime(kNotificationsPermissionRevocationGracePeriodDate, time); + get_pref()->SetTime(kNotificationsPermissionRevocationGracePeriodDate, time); - PushMessagingServiceImpl::RevokePermissionIfPossible( - GetUrl(), /*app_level_notifications_enabled=*/true, pref(), profile()); + // (1) Make sure that |kExampleOrigin| has access to use Push Messaging. + ASSERT_TRUE(push_service->IsPermissionSet(origin, true /* user_visible */)); - // Time is reset. - base::Time grace_period_date = - pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate); - EXPECT_EQ(grace_period_date, base::Time()); - EXPECT_NE(grace_period_date, time); + // (2) Subscribe for Push Messaging, and verify that we've got the required + // information in order to be able to create encrypted messages. + TestPushSubscription subscription; + Subscribe(push_service, origin, &subscription); - // Permission is not reset. - result = permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::GRANTED); + // (3) Encrypt a message using the public key and authentication secret that + // are associated with the subscription. - histogram_tester.ExpectBucketCount( - "Permission.FCM.Revocation", - static_cast(FcmTokenRevocation::kResetGracePeriod), 1); -} + gcm::IncomingMessage message; + message.sender_id = kTestSenderId; -// This test verifies that if the grace period is not started, and there is no -// app-level Notifications permissions, we init the grace period prefs without -// revoking permissions. -TEST_F(FCMRevocationTest, NoAppLevelPermissionInitGracePeriodPrefsTest) { - content::PermissionController* permission_controller = - profile()->GetPermissionController(); + ASSERT_TRUE(gcm::CreateEncryptedPayloadForTesting( + kTestPayload, + base::StringPiece(reinterpret_cast(subscription.p256dh_.data()), + subscription.p256dh_.size()), + base::StringPiece(reinterpret_cast(subscription.auth_.data()), + subscription.auth_.size()), + &message)); - content::PermissionResult result = - permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::ASK); + ASSERT_GT(message.raw_data.size(), 0u); + ASSERT_NE(kTestPayload, message.raw_data); + ASSERT_FALSE(message.decrypted); - SetPermission(GetUrl(), ContentSetting::CONTENT_SETTING_ALLOW, profile()); + // (4) Find the app_id that has been associated with the subscription. + PushMessagingAppIdentifier app_identifier = + PushMessagingAppIdentifier::FindByServiceWorker(profile(), origin, + kTestServiceWorkerId); - result = permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::GRANTED); + ASSERT_FALSE(app_identifier.is_null()); - const char kNotificationsPermissionRevocationGracePeriodDate[] = - "notifications_permission_revocation_grace_period"; + std::string app_id; + GURL dispatched_origin; + int64_t service_worker_registration_id; + absl::optional payload; - // The grace period is not initialized. - EXPECT_EQ(pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate), - base::Time()); + // (5) Observe message dispatchings from the Push Messaging service, and + // then dispatch the |message| on the GCM driver as if it had actually + // been received by Google Cloud Messaging. + push_service->SetMessageDispatchedCallbackForTesting(base::BindRepeating( + &PushMessagingServiceTest::DidDispatchMessage, base::Unretained(this), + &app_id, &dispatched_origin, &service_worker_registration_id, &payload)); - PushMessagingServiceImpl::RevokePermissionIfPossible( - GetUrl(), /*app_level_notifications_enabled=*/false, pref(), profile()); + gcm::FakeGCMProfileService* fake_profile_service = + static_cast( + gcm::GCMProfileServiceFactory::GetForProfile(profile())); - // The grace period is initialized with non-default time value. - EXPECT_NE(pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate), - base::Time()); + fake_profile_service->DispatchMessage(app_identifier.app_id(), message); - // Permission is still granted. - result = permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::GRANTED); -} + base::RunLoop().RunUntilIdle(); -// This test verifies that if the grace period is over and there is no app-level -// Notifications permissions, site-level Notifications permission will be -// revoked. -TEST_F(FCMRevocationTest, NoAppLevelPermissionRevocationTest) { - base::HistogramTester histogram_tester; + // (6) Verify that the message, as received by the Push Messaging Service, has + // indeed been decrypted by the GCM Driver, and has been forwarded to the + // Service Worker that has been associated with the subscription. + EXPECT_EQ(app_identifier.app_id(), app_id); + EXPECT_EQ(origin, dispatched_origin); + EXPECT_EQ(service_worker_registration_id, kTestServiceWorkerId); - content::PermissionController* permission_controller = - profile()->GetPermissionController(); + EXPECT_TRUE(payload); + EXPECT_EQ(kTestPayload, *payload); - content::PermissionResult result = - permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::ASK); + base::Time grace_period_date = + get_pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate); + EXPECT_EQ(grace_period_date, base::Time()); + EXPECT_NE(grace_period_date, time); - SetPermission(GetUrl(), ContentSetting::CONTENT_SETTING_ALLOW, profile()); + // (7) |kExampleOrigin| still has access to use Push Messaging. + ASSERT_TRUE(push_service->IsPermissionSet(origin, true /* user_visible */)); +} - result = permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::GRANTED); +// Tests that the grace period is not enabled, and no app-level permission. +// Ignore a push message. +TEST_F(FCMRevocationTest, TestPermissionRevocationNoPermissionFirstMessage) { + base::HistogramTester histogram_tester; + const GURL origin(kTestOrigin); + SetPermission(origin, CONTENT_SETTING_ALLOW); const char kNotificationsPermissionRevocationGracePeriodDate[] = "notifications_permission_revocation_grace_period"; - // The grace period is not initialized. - EXPECT_EQ(pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate), - base::Time()); + PushMessagingServiceImpl* push_service = profile()->GetPushMessagingService(); + ASSERT_TRUE(push_service); - // Init `time` with 4 days old time value. - const base::Time time = base::Time::FromDeltaSinceWindowsEpoch( - base::Time::Now() - - base::Time::FromDeltaSinceWindowsEpoch(base::Days(4))); + // Without Android app-level Notifications permission a push message will be + // ignored. + push_service->set_enabled_app_level_notification_permission_for_testing( + false); + push_service->set_prefs_for_testing(get_pref()); - // Init the grace period date with a value that is older than 3 days (the - // default grace period). - pref()->SetTime(kNotificationsPermissionRevocationGracePeriodDate, time); + // (1) Make sure that |kExampleOrigin| has access to use Push Messaging. + ASSERT_TRUE(push_service->IsPermissionSet(origin, true /* user_visible */)); + + // (2) Subscribe for Push Messaging, and verify that we've got the required + // information in order to be able to create encrypted messages. + TestPushSubscription subscription; + Subscribe(push_service, origin, &subscription); - PushMessagingServiceImpl::RevokePermissionIfPossible( - GetUrl(), /*app_level_notifications_enabled=*/false, pref(), profile()); + // (3) Encrypt a message using the public key and authentication secret that + // are associated with the subscription. + gcm::IncomingMessage message; + message.sender_id = kTestSenderId; - EXPECT_EQ(pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate), - time); + ASSERT_TRUE(gcm::CreateEncryptedPayloadForTesting( + kTestPayload, + base::StringPiece(reinterpret_cast(subscription.p256dh_.data()), + subscription.p256dh_.size()), + base::StringPiece(reinterpret_cast(subscription.auth_.data()), + subscription.auth_.size()), + &message)); - // Permission is revoked. - result = permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::ASK); + ASSERT_GT(message.raw_data.size(), 0u); + ASSERT_NE(kTestPayload, message.raw_data); + ASSERT_FALSE(message.decrypted); - histogram_tester.ExpectBucketCount( - "Permission.FCM.Revocation", - static_cast(FcmTokenRevocation::kRevokePermission), 1); -} + // (4) Find the app_id that has been associated with the subscription. + PushMessagingAppIdentifier app_identifier = + PushMessagingAppIdentifier::FindByServiceWorker(profile(), origin, + kTestServiceWorkerId); -// This test verifies that if the grace period is not over and there is no -// app-level Notifications permissions, site-level Notifications permission will -// not be revoked. -TEST_F(FCMRevocationTest, NoAppLevelPermissionIgnoreTest) { - base::HistogramTester histogram_tester; + ASSERT_FALSE(app_identifier.is_null()); - content::PermissionController* permission_controller = - profile()->GetPermissionController(); + std::string app_id; + GURL dispatched_origin; + int64_t service_worker_registration_id; + absl::optional payload; - content::PermissionResult result = - permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::ASK); + // (5) Observe message dispatchings from the Push Messaging service, and + // then dispatch the |message| on the GCM driver as if it had actually + // been received by Google Cloud Messaging. + push_service->SetMessageDispatchedCallbackForTesting(base::BindRepeating( + &PushMessagingServiceTest::DidDispatchMessage, base::Unretained(this), + &app_id, &dispatched_origin, &service_worker_registration_id, &payload)); - SetPermission(GetUrl(), ContentSetting::CONTENT_SETTING_ALLOW, profile()); + gcm::FakeGCMProfileService* fake_profile_service = + static_cast( + gcm::GCMProfileServiceFactory::GetForProfile(profile())); - result = permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::GRANTED); + fake_profile_service->DispatchMessage(app_identifier.app_id(), message); - const char kNotificationsPermissionRevocationGracePeriodDate[] = - "notifications_permission_revocation_grace_period"; + base::RunLoop().RunUntilIdle(); - // The grace period is not initialized. - EXPECT_EQ(pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate), - base::Time()); + // (6) Verify that the message, as received by the Push Messaging Service, has + // not been decrypted by the GCM Driver, and has not been forwarded to the + // Service Worker that has been associated with the subscription. + EXPECT_NE(app_identifier.app_id(), app_id); + EXPECT_NE(origin, dispatched_origin); + EXPECT_NE(service_worker_registration_id, kTestServiceWorkerId); - // Init `time` with 2 days old time value. - const base::Time time = base::Time::FromDeltaSinceWindowsEpoch( - base::Time::Now() - - base::Time::FromDeltaSinceWindowsEpoch(base::Days(2))); - // Init the grace period date with a value that is fewer than 3 days (the - // default grace period). - pref()->SetTime(kNotificationsPermissionRevocationGracePeriodDate, time); + EXPECT_FALSE(payload); - PushMessagingServiceImpl::RevokePermissionIfPossible( - GetUrl(), /*app_level_notifications_enabled=*/false, pref(), profile()); + // The grace period date has been changed. + EXPECT_NE( + get_pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate), + base::Time()); - EXPECT_EQ(pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate), - time); + // (7) |kExampleOrigin| still has access to use Push Messaging. + ASSERT_TRUE(push_service->IsPermissionSet(origin, true /* user_visible */)); - // Permission is revoked. - result = permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::GRANTED); + histogram_tester.ExpectUniqueSample( + "PushMessaging.DeliveryStatus", + static_cast( + blink::mojom::PushEventStatus::NO_APP_LEVEL_PERMISSION_IGNORE), + 1); - histogram_tester.ExpectBucketCount( - "Permission.FCM.Revocation", - static_cast(FcmTokenRevocation::kGracePeriodIsNotOver), 1); + histogram_tester.ExpectTotalCount("PushMessaging.UnregistrationReason", 0); } -// This test verifies that if the grace period is not over and there is -// app-level Notifications permissions, the grace period reset will be tracked. -TEST_F(FCMRevocationTest, ResetAndRecordGracePeriodTest) { +// Tests that the grace period was enabled more than 3 days ago, and no +// app-level permission. An incoming push message should be ignored and +// origin-level Notifications permission should be revoked to prevent further +// push messages. +TEST_F(FCMRevocationTest, TestPermissionRevocationGracePeriodIsOver) { base::HistogramTester histogram_tester; - - content::PermissionController* permission_controller = - profile()->GetPermissionController(); - - content::PermissionResult result = - permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::ASK); - - SetPermission(GetUrl(), ContentSetting::CONTENT_SETTING_ALLOW, profile()); - - result = permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::GRANTED); + const GURL origin(kTestOrigin); + SetPermission(origin, CONTENT_SETTING_ALLOW); const char kNotificationsPermissionRevocationGracePeriodDate[] = "notifications_permission_revocation_grace_period"; - // The grace period is not initialized. - EXPECT_EQ(pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate), - base::Time()); + PushMessagingServiceImpl* push_service = profile()->GetPushMessagingService(); + ASSERT_TRUE(push_service); + + // Without Android app-level Notifications permission a push message will be + // ignored. + push_service->set_enabled_app_level_notification_permission_for_testing( + false); + push_service->set_prefs_for_testing(get_pref()); - // Init `time` with 2 days old time value. + // Init `time` with 4 days old time value. const base::Time time = base::Time::FromDeltaSinceWindowsEpoch( base::Time::Now() - - base::Time::FromDeltaSinceWindowsEpoch(base::Days(2))); - // Init the grace period date with a value that is fewer than 3 days (the + base::Time::FromDeltaSinceWindowsEpoch(base::Days(4))); + // Init the grace period date with a value that is older than 3 days (the // default grace period). - pref()->SetTime(kNotificationsPermissionRevocationGracePeriodDate, time); + get_pref()->SetTime(kNotificationsPermissionRevocationGracePeriodDate, time); - PushMessagingServiceImpl::RevokePermissionIfPossible( - GetUrl(), /*app_level_notifications_enabled=*/true, pref(), profile()); + // (1) Make sure that |kExampleOrigin| has access to use Push Messaging. + ASSERT_TRUE(push_service->IsPermissionSet(origin, true /* user_visible */)); - EXPECT_EQ(pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate), - base::Time()); + // (2) Subscribe for Push Messaging, and verify that we've got the required + // information in order to be able to create encrypted messages. + TestPushSubscription subscription; + Subscribe(push_service, origin, &subscription); - // Permission is revoked. - result = permission_controller->GetPermissionResultForOriginWithoutContext( - blink::PermissionType::NOTIFICATIONS, GetOrigin()); - EXPECT_EQ(result.status, blink::mojom::PermissionStatus::GRANTED); + // (3) Encrypt a message using the public key and authentication secret that + // are associated with the subscription. + gcm::IncomingMessage message; + message.sender_id = kTestSenderId; - histogram_tester.ExpectTimeBucketCount( - "Permission.FCM.Revocation.ResetGracePeriod", base::Days(2), 1); -} + ASSERT_TRUE(gcm::CreateEncryptedPayloadForTesting( + kTestPayload, + base::StringPiece(reinterpret_cast(subscription.p256dh_.data()), + subscription.p256dh_.size()), + base::StringPiece(reinterpret_cast(subscription.auth_.data()), + subscription.auth_.size()), + &message)); + + ASSERT_GT(message.raw_data.size(), 0u); + ASSERT_NE(kTestPayload, message.raw_data); + ASSERT_FALSE(message.decrypted); + + // (4) Find the app_id that has been associated with the subscription. + PushMessagingAppIdentifier app_identifier = + PushMessagingAppIdentifier::FindByServiceWorker(profile(), origin, + kTestServiceWorkerId); + ASSERT_FALSE(app_identifier.is_null()); + + std::string app_id; + GURL dispatched_origin; + int64_t service_worker_registration_id; + absl::optional payload; + + // (5) Observe message dispatchings from the Push Messaging service, and + // then dispatch the |message| on the GCM driver as if it had actually + // been received by Google Cloud Messaging. + push_service->SetMessageDispatchedCallbackForTesting(base::BindRepeating( + &PushMessagingServiceTest::DidDispatchMessage, base::Unretained(this), + &app_id, &dispatched_origin, &service_worker_registration_id, &payload)); + + gcm::FakeGCMProfileService* fake_profile_service = + static_cast( + gcm::GCMProfileServiceFactory::GetForProfile(profile())); + + fake_profile_service->DispatchMessage(app_identifier.app_id(), message); + + base::RunLoop().RunUntilIdle(); + + // (6) Verify that the message, as received by the Push Messaging Service, has + // not been decrypted by the GCM Driver, and has not been forwarded to the + // Service Worker that has been associated with the subscription. + EXPECT_NE(app_identifier.app_id(), app_id); + EXPECT_NE(origin, dispatched_origin); + EXPECT_NE(service_worker_registration_id, kTestServiceWorkerId); + + EXPECT_FALSE(payload); + + // The grace period date has not been changed. + EXPECT_EQ( + get_pref()->GetTime(kNotificationsPermissionRevocationGracePeriodDate), + time); + + // (7) |kExampleOrigin| has no access to use Push Messaging. + EXPECT_FALSE(push_service->IsPermissionSet(origin, true /* user_visible */)); + + histogram_tester.ExpectUniqueSample( + "PushMessaging.DeliveryStatus", + static_cast( + blink::mojom::PushEventStatus::NO_APP_LEVEL_PERMISSION_UNSUBSCRIBE), + 1); + + // 1st event - blink::mojom::PushUnregistrationReason::PERMISSION_REVOKED, + // because `PushMessagingServiceImpl::OnContentSettingChanged` will be + // notified when Notifications permission is revoked. + // + // 2nd event - + // blink::mojom::PushUnregistrationReason::NO_APP_LEVEL_PERMISSION. + histogram_tester.ExpectTotalCount("PushMessaging.UnregistrationReason", 2); + + histogram_tester.ExpectBucketCount( + "PushMessaging.UnregistrationReason", + blink::mojom::PushUnregistrationReason::PERMISSION_REVOKED, 1); + histogram_tester.ExpectBucketCount( + "PushMessaging.UnregistrationReason", + blink::mojom::PushUnregistrationReason::NO_APP_LEVEL_PERMISSION, 1); +} #endif diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml index 8b10353ae4875..7318cd58f2449 100644 --- a/tools/metrics/histograms/enums.xml +++ b/tools/metrics/histograms/enums.xml @@ -36974,12 +36974,6 @@ Called by update_extension_permission.py.--> - - - - - - Feature module has been requested but is not installed yet. diff --git a/tools/metrics/histograms/metadata/permissions/histograms.xml b/tools/metrics/histograms/metadata/permissions/histograms.xml index 47aef8ae43d45..c4fcc65b81083 100644 --- a/tools/metrics/histograms/metadata/permissions/histograms.xml +++ b/tools/metrics/histograms/metadata/permissions/histograms.xml @@ -488,27 +488,6 @@ chromium-metrics-reviews@google.com. - - elklm@chromium.org - src/components/permissions/PERMISSIONS_OWNERS - - Records when Chrome has no app-level Notifications permission on Android and - receives a push message. - - - - - elklm@chromium.org - src/components/permissions/PERMISSIONS_OWNERS - - Records when Chrome has app-level Notifications permission on Android, - receives a push message but the grace period was initialized and needs to be - reset. - - - andypaicu@chromium.org