Add finch config to prefer using VAPID over Sync
Bug: 1081819
Change-Id: Ia241e28e60142e757a820f533e828345d54ce366
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2196666
Commit-Queue: Alex Chau <[email protected]>
Reviewed-by: Michael van Ouwerkerk <[email protected]>
Cr-Commit-Position: refs/heads/master@{#768288}
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index 727a0eb..3632a07 100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -4359,6 +4359,10 @@
#endif // defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) ||
// defined(OS_CHROMEOS)
+ {"sharing-prefer-vapid", flag_descriptions::kSharingPreferVapidName,
+ flag_descriptions::kSharingPreferVapidDescription, kOsAll,
+ FEATURE_VALUE_TYPE(kSharingPreferVapid)},
+
{"sharing-qr-code-generator",
flag_descriptions::kSharingQRCodeGeneratorName,
flag_descriptions::kSharingQRCodeGeneratorDescription, kOsDesktop,
diff --git a/chrome/browser/flag-metadata.json b/chrome/browser/flag-metadata.json
index fd6037e3..7bd7e95 100644
--- a/chrome/browser/flag-metadata.json
+++ b/chrome/browser/flag-metadata.json
@@ -3769,6 +3769,11 @@
"expiry_milestone": 84
},
{
+ "name": "sharing-prefer-vapid",
+ "owners": [ "//chrome/browser/sharing/OWNERS" ],
+ "expiry_milestone": 87
+ },
+ {
"name": "sharing-qr-code-generator",
"owners": [ "//components/send_tab_to_self/OWNERS" ],
"expiry_milestone": 86
diff --git a/chrome/browser/flag_descriptions.cc b/chrome/browser/flag_descriptions.cc
index 10379ab..46b5a0c 100644
--- a/chrome/browser/flag_descriptions.cc
+++ b/chrome/browser/flag_descriptions.cc
@@ -1769,6 +1769,11 @@
"Enables the sender devices to connect with chosen device using a peer to "
"peer connection for transferring data.";
+const char kSharingPreferVapidName[] =
+ "Prefer sending Sharing message via VAPID";
+const char kSharingPreferVapidDescription[] =
+ "Prefer sending Sharing message via FCM WebPush authenticated using VAPID.";
+
const char kSharingQRCodeGeneratorName[] = "Enable sharing page via QR Code";
const char kSharingQRCodeGeneratorDescription[] =
"Enables right-click UI to share the page's URL via a generated QR Code.";
diff --git a/chrome/browser/flag_descriptions.h b/chrome/browser/flag_descriptions.h
index 243f3f8..3e7a2f7 100644
--- a/chrome/browser/flag_descriptions.h
+++ b/chrome/browser/flag_descriptions.h
@@ -1037,6 +1037,9 @@
extern const char kSharingPeerConnectionSenderName[];
extern const char kSharingPeerConnectionSenderDescription[];
+extern const char kSharingPreferVapidName[];
+extern const char kSharingPreferVapidDescription[];
+
extern const char kSharingQRCodeGeneratorName[];
extern const char kSharingQRCodeGeneratorDescription[];
diff --git a/chrome/browser/sharing/features.cc b/chrome/browser/sharing/features.cc
index cb7fd38..e43c6ca0 100644
--- a/chrome/browser/sharing/features.cc
+++ b/chrome/browser/sharing/features.cc
@@ -42,3 +42,6 @@
const base::Feature kSharingSendViaSync{"SharingSendViaSync",
base::FEATURE_ENABLED_BY_DEFAULT};
+
+const base::Feature kSharingPreferVapid{"SharingPreferVapid",
+ base::FEATURE_DISABLED_BY_DEFAULT};
diff --git a/chrome/browser/sharing/features.h b/chrome/browser/sharing/features.h
index db0ff1a..3acb082 100644
--- a/chrome/browser/sharing/features.h
+++ b/chrome/browser/sharing/features.h
@@ -54,4 +54,7 @@
// Feature flag for sending sharing message via Sync.
extern const base::Feature kSharingSendViaSync;
+// Feature flag for prefer sending sharing message using VAPID.
+extern const base::Feature kSharingPreferVapid;
+
#endif // CHROME_BROWSER_SHARING_FEATURES_H_
diff --git a/chrome/browser/sharing/sharing_fcm_sender.cc b/chrome/browser/sharing/sharing_fcm_sender.cc
index 2a17ebd..337358a 100644
--- a/chrome/browser/sharing/sharing_fcm_sender.cc
+++ b/chrome/browser/sharing/sharing_fcm_sender.cc
@@ -70,11 +70,18 @@
SendMessageCallback callback) {
TRACE_EVENT0("sharing", "SharingFCMSender::SendMessageToFcmTarget");
- if (base::FeatureList::IsEnabled(kSharingSendViaSync) &&
+ bool canSendViaSync =
+ base::FeatureList::IsEnabled(kSharingSendViaSync) &&
sync_service_->GetActiveDataTypes().Has(syncer::SHARING_MESSAGE) &&
!fcm_configuration.sender_id_fcm_token().empty() &&
!fcm_configuration.sender_id_p256dh().empty() &&
- !fcm_configuration.sender_id_auth_secret().empty()) {
+ !fcm_configuration.sender_id_auth_secret().empty();
+ bool canSendViaVapid = !fcm_configuration.vapid_fcm_token().empty() &&
+ !fcm_configuration.vapid_p256dh().empty() &&
+ !fcm_configuration.vapid_auth_secret().empty();
+
+ if (canSendViaSync && (!canSendViaVapid ||
+ !base::FeatureList::IsEnabled(kSharingPreferVapid))) {
message.set_message_id(base::GenerateGUID());
EncryptMessage(
kSharingSenderID, fcm_configuration.sender_id_p256dh(),
@@ -87,9 +94,7 @@
return;
}
- if (!fcm_configuration.vapid_fcm_token().empty() &&
- !fcm_configuration.vapid_p256dh().empty() &&
- !fcm_configuration.vapid_auth_secret().empty()) {
+ if (canSendViaVapid) {
base::Optional<SharingSyncPreference::FCMRegistration> fcm_registration =
sync_preference_->GetFCMRegistration();
if (!fcm_registration || !fcm_registration->authorized_entity) {
diff --git a/chrome/browser/sharing/sharing_fcm_sender_unittest.cc b/chrome/browser/sharing/sharing_fcm_sender_unittest.cc
index 06604c5..3753d4a4 100644
--- a/chrome/browser/sharing/sharing_fcm_sender_unittest.cc
+++ b/chrome/browser/sharing/sharing_fcm_sender_unittest.cc
@@ -93,14 +93,14 @@
const std::string& fcm_token() { return fcm_token_; }
crypto::ECPrivateKey* vapid_key() { return vapid_key_; }
- const WebPushMessage& message() { return message_; }
+ const base::Optional<WebPushMessage>& message() { return message_; }
void set_result(SendWebPushMessageResult result) { result_ = result; }
private:
std::string fcm_token_;
crypto::ECPrivateKey* vapid_key_;
- WebPushMessage message_;
+ base::Optional<WebPushMessage> message_;
SendWebPushMessageResult result_;
DISALLOW_COPY_AND_ASSIGN(FakeWebPushSender);
@@ -127,7 +127,9 @@
return nullptr;
}
- const sync_pb::SharingMessageSpecifics& specifics() { return specifics_; }
+ const base::Optional<sync_pb::SharingMessageSpecifics>& specifics() {
+ return specifics_;
+ }
void set_error_code(
const sync_pb::SharingMessageCommitError::ErrorCode& error_code) {
@@ -135,7 +137,7 @@
}
private:
- sync_pb::SharingMessageSpecifics specifics_;
+ base::Optional<sync_pb::SharingMessageSpecifics> specifics_;
sync_pb::SharingMessageCommitError::ErrorCode error_code_;
DISALLOW_COPY_AND_ASSIGN(FakeSharingMessageBridge);
@@ -330,6 +332,100 @@
EXPECT_EQ(SharingChannelType::kUnknown, channel_type);
}
+TEST_F(SharingFCMSenderTest, PreferVapid) {
+ scoped_feature_list_.InitAndEnableFeature(kSharingPreferVapid);
+ test_sync_service_.SetActiveDataTypes({syncer::SHARING_MESSAGE});
+ sync_prefs_.SetFCMRegistration(SharingSyncPreference::FCMRegistration(
+ kAuthorizedEntity, base::Time::Now()));
+
+ fake_web_push_sender_->set_result(SendWebPushMessageResult::kSuccessful);
+ fake_sharing_message_bridge_.set_error_code(
+ sync_pb::SharingMessageCommitError::NONE);
+
+ std::unique_ptr<crypto::ECPrivateKey> vapid_key =
+ crypto::ECPrivateKey::Create();
+ ON_CALL(vapid_key_manager_, GetOrCreateKey())
+ .WillByDefault(testing::Return(vapid_key.get()));
+
+ chrome_browser_sharing::FCMChannelConfiguration fcm_channel;
+ // Set both VAPID and Sender ID channel.
+ fcm_channel.set_vapid_fcm_token(kVapidFcmToken);
+ fcm_channel.set_vapid_p256dh(kVapidP256dh);
+ fcm_channel.set_vapid_auth_secret(kVapidAuthSecret);
+ fcm_channel.set_sender_id_fcm_token(kSenderIdFcmToken);
+ fcm_channel.set_sender_id_p256dh(kSenderIdP256dh);
+ fcm_channel.set_sender_id_auth_secret(kSenderIdAuthSecret);
+
+ SharingSendMessageResult result;
+ base::Optional<std::string> message_id;
+ SharingChannelType channel_type;
+ chrome_browser_sharing::SharingMessage sharing_message;
+ sharing_message.mutable_ping_message();
+ sharing_fcm_sender_.SendMessageToFcmTarget(
+ fcm_channel, base::TimeDelta::FromSeconds(kTtlSeconds),
+ std::move(sharing_message),
+ base::BindOnce(&SharingFCMSenderTest::OnMessageSent,
+ base::Unretained(this), &result, &message_id,
+ &channel_type));
+
+ EXPECT_EQ(SharingSendMessageResult::kSuccessful, result);
+ // Ensures that a Ping message is sent through WebPushSender.
+ chrome_browser_sharing::SharingMessage message_sent;
+ ASSERT_TRUE(fake_web_push_sender_->message());
+ message_sent.ParseFromString(fake_web_push_sender_->message()->payload);
+ EXPECT_TRUE(message_sent.has_ping_message());
+ // Ensures that no message is sent through SharingMessageBridge.
+ EXPECT_FALSE(fake_sharing_message_bridge_.specifics());
+}
+
+TEST_F(SharingFCMSenderTest, PreferSync) {
+ scoped_feature_list_.InitAndDisableFeature(kSharingPreferVapid);
+ test_sync_service_.SetActiveDataTypes({syncer::SHARING_MESSAGE});
+ sync_prefs_.SetFCMRegistration(SharingSyncPreference::FCMRegistration(
+ kAuthorizedEntity, base::Time::Now()));
+
+ fake_web_push_sender_->set_result(SendWebPushMessageResult::kSuccessful);
+ fake_sharing_message_bridge_.set_error_code(
+ sync_pb::SharingMessageCommitError::NONE);
+
+ std::unique_ptr<crypto::ECPrivateKey> vapid_key =
+ crypto::ECPrivateKey::Create();
+ ON_CALL(vapid_key_manager_, GetOrCreateKey())
+ .WillByDefault(testing::Return(vapid_key.get()));
+
+ chrome_browser_sharing::FCMChannelConfiguration fcm_channel;
+ // Set both VAPID and Sender ID channel.
+ fcm_channel.set_vapid_fcm_token(kVapidFcmToken);
+ fcm_channel.set_vapid_p256dh(kVapidP256dh);
+ fcm_channel.set_vapid_auth_secret(kVapidAuthSecret);
+ fcm_channel.set_sender_id_fcm_token(kSenderIdFcmToken);
+ fcm_channel.set_sender_id_p256dh(kSenderIdP256dh);
+ fcm_channel.set_sender_id_auth_secret(kSenderIdAuthSecret);
+
+ SharingSendMessageResult result;
+ base::Optional<std::string> message_id;
+ SharingChannelType channel_type;
+ chrome_browser_sharing::SharingMessage sharing_message;
+ sharing_message.mutable_ping_message();
+ sharing_fcm_sender_.SendMessageToFcmTarget(
+ fcm_channel, base::TimeDelta::FromSeconds(kTtlSeconds),
+ std::move(sharing_message),
+ base::BindOnce(&SharingFCMSenderTest::OnMessageSent,
+ base::Unretained(this), &result, &message_id,
+ &channel_type));
+
+ EXPECT_EQ(SharingSendMessageResult::kSuccessful, result);
+ // Ensures that a Ping message is sent through SharingMessageBridge.
+ chrome_browser_sharing::SharingMessage message_sent;
+ ASSERT_TRUE(fake_sharing_message_bridge_.specifics());
+ ASSERT_TRUE(fake_sharing_message_bridge_.specifics()->has_payload());
+ message_sent.ParseFromString(
+ fake_sharing_message_bridge_.specifics()->payload());
+ EXPECT_TRUE(message_sent.has_ping_message());
+ // Ensures that no message is sent through WebPushSender.
+ EXPECT_FALSE(fake_web_push_sender_->message());
+}
+
struct WebPushResultTestData {
const SendWebPushMessageResult web_push_result;
const SharingSendMessageResult expected_result;
@@ -392,11 +488,12 @@
EXPECT_EQ(kVapidFcmToken, fake_web_push_sender_->fcm_token());
EXPECT_EQ(vapid_key.get(), fake_web_push_sender_->vapid_key());
- EXPECT_EQ(kTtlSeconds, fake_web_push_sender_->message().time_to_live);
+ EXPECT_EQ(kTtlSeconds, fake_web_push_sender_->message()->time_to_live);
EXPECT_EQ(WebPushMessage::Urgency::kHigh,
- fake_web_push_sender_->message().urgency);
+ fake_web_push_sender_->message()->urgency);
chrome_browser_sharing::SharingMessage message_sent;
- message_sent.ParseFromString(fake_web_push_sender_->message().payload);
+ ASSERT_TRUE(fake_web_push_sender_->message());
+ message_sent.ParseFromString(fake_web_push_sender_->message()->payload);
EXPECT_TRUE(message_sent.has_ping_message());
EXPECT_EQ(GetParam().expected_result, result);
@@ -469,16 +566,17 @@
EXPECT_EQ(kSenderIdAuthSecret, fake_gcm_driver_.auth_secret());
auto specifics = fake_sharing_message_bridge_.specifics();
- ASSERT_TRUE(specifics.has_channel_configuration());
- ASSERT_TRUE(specifics.channel_configuration().has_fcm());
- auto fcm = specifics.channel_configuration().fcm();
+ ASSERT_TRUE(specifics);
+ ASSERT_TRUE(specifics->has_channel_configuration());
+ ASSERT_TRUE(specifics->channel_configuration().has_fcm());
+ auto fcm = specifics->channel_configuration().fcm();
EXPECT_EQ(kSenderIdFcmToken, fcm.token());
EXPECT_EQ(kTtlSeconds, fcm.ttl());
EXPECT_EQ(10, fcm.priority());
chrome_browser_sharing::SharingMessage message_sent;
- ASSERT_TRUE(specifics.has_payload());
- message_sent.ParseFromString(specifics.payload());
+ ASSERT_TRUE(specifics->has_payload());
+ message_sent.ParseFromString(specifics->payload());
EXPECT_TRUE(message_sent.has_ping_message());
EXPECT_EQ(GetParam().expected_result, result);
@@ -518,12 +616,13 @@
EXPECT_EQ(kServerAuthSecret, fake_gcm_driver_.auth_secret());
auto specifics = fake_sharing_message_bridge_.specifics();
- ASSERT_TRUE(specifics.has_channel_configuration());
- EXPECT_EQ(kServerConfiguration, specifics.channel_configuration().server());
+ ASSERT_TRUE(specifics);
+ ASSERT_TRUE(specifics->has_channel_configuration());
+ EXPECT_EQ(kServerConfiguration, specifics->channel_configuration().server());
chrome_browser_sharing::SharingMessage message_sent;
- ASSERT_TRUE(specifics.has_payload());
- message_sent.ParseFromString(specifics.payload());
+ ASSERT_TRUE(specifics->has_payload());
+ message_sent.ParseFromString(specifics->payload());
EXPECT_TRUE(message_sent.has_ping_message());
EXPECT_EQ(SharingSendMessageResult::kSuccessful, result);
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
index c2f1b5b..79b57e2 100644
--- a/tools/metrics/histograms/enums.xml
+++ b/tools/metrics/histograms/enums.xml
@@ -39766,6 +39766,7 @@
<int value="-361948582" label="material-security-verbose"/>
<int value="-360453785" label="LeftToRightUrls:disabled"/>
<int value="-360038744" label="invert-viewport-scroll-order"/>
+ <int value="-357464687" label="SharingPreferVapid:enabled"/>
<int value="-355278724" label="CorbAllowlistAlsoAppliesToOorCors:disabled"/>
<int value="-354783358" label="NTPSaveToOffline:disabled"/>
<int value="-353182790" label="ConsistentOmniboxGeolocation:disabled"/>
@@ -39999,6 +40000,7 @@
<int value="-79327236" label="ModeSpecificPowerButton:enabled"/>
<int value="-78035185" label="custom_summary"/>
<int value="-77872983" label="BookmarkAppsMac:disabled"/>
+ <int value="-77789682" label="SharingPreferVapid:disabled"/>
<int value="-77084779" label="ExperimentalFlingAnimation:enabled"/>
<int value="-76631048" label="disable-offline-auto-reload-visible-only"/>
<int value="-76445689" label="WasmCodeCache:enabled"/>
