Skip to content

Commit

Permalink
Revert "Revoke site-level Notifications permission."
Browse files Browse the repository at this point in the history 
This reverts commit 9a8dfe0.

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 <peter@chromium.org>
> Reviewed-by: Ravjit Uppal <ravjit@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Commit-Queue: Illia Klimov <elklm@chromium.org>
> 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 <tasak@google.com>
Owners-Override: Takashi Sakamoto <tasak@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1075660}
  • Loading branch information
@tasak
tasak authored and Chromium LUCI CQ committed Nov 25, 2022
1 parent cafc914 commit 7eb93e8
Show file tree
Hide file tree
Showing 9 changed files with 373 additions and 387 deletions.
1 change: 0 additions & 1 deletion chrome/android/BUILD.gn
View file
Expand Up @@ -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",
Expand Down
1 change: 0 additions & 1 deletion chrome/android/chrome_java_sources.gni
View file
Expand Up @@ -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",
Expand Down
9 changes: 0 additions & 9 deletions ...ndroid/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
View file
Expand Up @@ -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,
Expand Down
47 changes: 0 additions & 47 deletions ...ndroid/java/src/org/chromium/chrome/browser/notifications/PushMessagingServiceBridge.java
View file

This file was deleted.

171 changes: 80 additions & 91 deletions chrome/browser/push_messaging/push_messaging_service_impl.cc
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -912,88 +983,6 @@ void PushMessagingServiceImpl::RegisterPrefs(PrefRegistrySimple* registry) {
registry->RegisterTimePref(kNotificationsPermissionRevocationGracePeriodDate,
base::Time());
}

static void
JNI_PushMessagingServiceBridge_VerifyAndRevokeNotificationsPermission(
JNIEnv* env,
const JavaParamRef<jstring>& java_origin,
const JavaParamRef<jstring>& 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() {
Expand Down
38 changes: 21 additions & 17 deletions chrome/browser/push_messaging/push_messaging_service_impl.h
View file
Expand Up @@ -38,17 +38,14 @@

class GURL;
class PrefRegistrySimple;
class PrefService;
class Profile;
class PushMessagingAppIdentifier;
class PushMessagingServiceTest;
class FCMRevocationTest;
class ScopedKeepAlive;
class ScopedProfileKeepAlive;

#if BUILDFLAG(IS_ANDROID)
class PrefService;
#endif

namespace blink {
namespace mojom {
enum class PushEventStatus;
Expand All @@ -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);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -498,6 +499,9 @@ class PushMessagingServiceImpl : public content::PushMessagingService,

int render_process_id_ = content::ChildProcessHost::kInvalidUniqueID;

absl::optional<bool> enabled_app_level_notification_permission_for_testing_;
absl::optional<PrefService*> prefs_for_testing_;

base::WeakPtrFactory<PushMessagingServiceImpl> weak_factory_{this};
};

Expand Down

0 comments on commit 7eb93e8

Please sign in to comment.