[Sharing] Clean up SharingDisableVapid feature toggle The feature toggle was enabled by default in M133 and can be cleaned up now. This CL also cleans up some related code which supported Vapid registration flow. Other parts of code related to using Vapid messages will be deleted in follow-up CLs. Bug: 40253551 Change-Id: I20ae4f85f6fa4b2f2daf9376a3ddce6532956bfe Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6563588 Reviewed-by: Marc Treib <treib@chromium.org> Commit-Queue: Rushan Suleymanov <rushans@google.com> Cr-Commit-Position: refs/heads/main@{#1462124}
diff --git a/chrome/browser/sharing/sharing_device_registration_impl.cc b/chrome/browser/sharing/sharing_device_registration_impl.cc index 679963e..89bf41ad 100644 --- a/chrome/browser/sharing/sharing_device_registration_impl.cc +++ b/chrome/browser/sharing/sharing_device_registration_impl.cc
@@ -37,16 +37,6 @@ using instance_id::InstanceID; using sync_pb::SharingSpecificFields; -namespace { - -// When enabled, sharing messages will not be sent via VAPID and the device will -// not register with VAPID. -BASE_FEATURE(kSharingDisableVapid, - "SharingDisableVapid", - base::FEATURE_ENABLED_BY_DEFAULT); - -} // namespace - SharingDeviceRegistrationImpl::SharingDeviceRegistrationImpl( PrefService* pref_service, SharingSyncPreference* sharing_sync_preference, @@ -63,45 +53,44 @@ void SharingDeviceRegistrationImpl::RegisterDevice( RegistrationCallback callback) { - std::optional<std::string> authorized_entity = GetAuthorizationEntity(); - if (!authorized_entity) { - OnVapidTargetInfoRetrieved(std::move(callback), - /*authorized_entity=*/std::nullopt, - SharingDeviceRegistrationResult::kSuccess, - /*vapid_target_info=*/std::nullopt); + if (!CanSendViaSenderID(sync_service_)) { + OnSharingTargetInfoRetrieved(std::move(callback), + SharingDeviceRegistrationResult::kSuccess, + /*sharing_target_info=*/std::nullopt); return; } + // Attempt to register using sender ID when enabled. RetrieveTargetInfo( - *authorized_entity, - base::BindOnce(&SharingDeviceRegistrationImpl::OnVapidTargetInfoRetrieved, - weak_ptr_factory_.GetWeakPtr(), std::move(callback), - *authorized_entity)); + kSharingSenderID, + base::BindOnce( + &SharingDeviceRegistrationImpl::OnSharingTargetInfoRetrieved, + weak_ptr_factory_.GetWeakPtr(), std::move(callback))); } void SharingDeviceRegistrationImpl::RetrieveTargetInfo( - const std::string& authorized_entity, + const std::string& sender_id, TargetInfoCallback callback) { instance_id_driver_->GetInstanceID(kSharingFCMAppID) ->GetToken( - authorized_entity, instance_id::kGCMScope, + sender_id, instance_id::kGCMScope, /*time_to_live=*/base::TimeDelta(), /*flags=*/{InstanceID::Flags::kBypassScheduler}, base::BindOnce(&SharingDeviceRegistrationImpl::OnFCMTokenReceived, weak_ptr_factory_.GetWeakPtr(), std::move(callback), - authorized_entity)); + sender_id)); } void SharingDeviceRegistrationImpl::OnFCMTokenReceived( TargetInfoCallback callback, - const std::string& authorized_entity, + const std::string& sender_id, const std::string& fcm_token, instance_id::InstanceID::Result result) { switch (result) { case InstanceID::SUCCESS: instance_id_driver_->GetInstanceID(kSharingFCMAppID) ->GetEncryptionInfo( - authorized_entity, + sender_id, base::BindOnce( &SharingDeviceRegistrationImpl::OnEncryptionInfoReceived, weak_ptr_factory_.GetWeakPtr(), std::move(callback), @@ -133,39 +122,8 @@ fcm_token, p256dh, auth_secret})); } -void SharingDeviceRegistrationImpl::OnVapidTargetInfoRetrieved( - RegistrationCallback callback, - std::optional<std::string> authorized_entity, - SharingDeviceRegistrationResult result, - std::optional<syncer::DeviceInfo::SharingTargetInfo> vapid_target_info) { - if (result != SharingDeviceRegistrationResult::kSuccess) { - // Clear results if the registration failed but proceed to register using - // sender ID. - authorized_entity = std::nullopt; - vapid_target_info = std::nullopt; - } - - if (!CanSendViaSenderID(sync_service_)) { - OnSharingTargetInfoRetrieved(std::move(callback), - std::move(authorized_entity), - std::move(vapid_target_info), result, - /*sharing_target_info=*/std::nullopt); - return; - } - - // Attempt to register using sender ID when enabled. - RetrieveTargetInfo( - kSharingSenderID, - base::BindOnce( - &SharingDeviceRegistrationImpl::OnSharingTargetInfoRetrieved, - weak_ptr_factory_.GetWeakPtr(), std::move(callback), - std::move(authorized_entity), std::move(vapid_target_info))); -} - void SharingDeviceRegistrationImpl::OnSharingTargetInfoRetrieved( RegistrationCallback callback, - std::optional<std::string> authorized_entity, - std::optional<syncer::DeviceInfo::SharingTargetInfo> vapid_target_info, SharingDeviceRegistrationResult result, std::optional<syncer::DeviceInfo::SharingTargetInfo> sharing_target_info) { if (result != SharingDeviceRegistrationResult::kSuccess) { @@ -173,7 +131,7 @@ return; } - if (!vapid_target_info && !sharing_target_info) { + if (!sharing_target_info) { std::move(callback).Run(SharingDeviceRegistrationResult::kInternalError); return; } @@ -181,10 +139,9 @@ base::UmaHistogramBoolean("Sharing.LocalSharingTargetInfoSupportsSync", !!sharing_target_info); std::set<SharingSpecificFields::EnabledFeatures> enabled_features = - GetEnabledFeatures(/*supports_vapid=*/authorized_entity.has_value()); + GetEnabledFeatures(); syncer::DeviceInfo::SharingInfo sharing_info( - vapid_target_info ? std::move(*vapid_target_info) - : syncer::DeviceInfo::SharingTargetInfo(), + /*vapid_target_info=*/syncer::DeviceInfo::SharingTargetInfo(), sharing_target_info ? std::move(*sharing_target_info) : syncer::DeviceInfo::SharingTargetInfo(), /*chime_representative_target_id=*/std::string(), @@ -192,7 +149,7 @@ sharing_sync_preference_->SetLocalSharingInfo(std::move(sharing_info)); sharing_sync_preference_->SetFCMRegistration( // Clears authorized_entity in preferences if it's not populated. - SharingSyncPreference::FCMRegistration(std::move(authorized_entity), + SharingSyncPreference::FCMRegistration(/*authorized_entity=*/std::nullopt, base::Time::Now())); std::move(callback).Run(SharingDeviceRegistrationResult::kSuccess); } @@ -267,32 +224,8 @@ NOTREACHED(); } -std::optional<std::string> -SharingDeviceRegistrationImpl::GetAuthorizationEntity() const { - if (base::FeatureList::IsEnabled(kSharingDisableVapid)) { - return std::nullopt; - } - - // TODO(himanshujaju) : Extract a static function to convert ECPrivateKey* to - // Base64PublicKey in library. - crypto::ECPrivateKey* vapid_key = vapid_key_manager_->GetOrCreateKey(); - if (!vapid_key) { - return std::nullopt; - } - - std::string public_key; - if (!gcm::GetRawPublicKey(*vapid_key, &public_key)) { - return std::nullopt; - } - - std::string base64_public_key; - base::Base64UrlEncode(public_key, base::Base64UrlEncodePolicy::OMIT_PADDING, - &base64_public_key); - return std::make_optional(std::move(base64_public_key)); -} - std::set<SharingSpecificFields::EnabledFeatures> -SharingDeviceRegistrationImpl::GetEnabledFeatures(bool supports_vapid) const { +SharingDeviceRegistrationImpl::GetEnabledFeatures() const { // Used in tests if (enabled_features_testing_value_) { return enabled_features_testing_value_.value(); @@ -301,15 +234,9 @@ std::set<SharingSpecificFields::EnabledFeatures> enabled_features; if (IsClickToCallSupported()) { enabled_features.insert(SharingSpecificFields::CLICK_TO_CALL_V2); - if (supports_vapid) { - enabled_features.insert(SharingSpecificFields::CLICK_TO_CALL_VAPID); - } } if (IsSharedClipboardSupported()) { enabled_features.insert(SharingSpecificFields::SHARED_CLIPBOARD_V2); - if (supports_vapid) { - enabled_features.insert(SharingSpecificFields::SHARED_CLIPBOARD_VAPID); - } } if (IsSmsFetcherSupported()) { enabled_features.insert(SharingSpecificFields::SMS_FETCHER);
diff --git a/chrome/browser/sharing/sharing_device_registration_impl.h b/chrome/browser/sharing/sharing_device_registration_impl.h index 16e706a..2610d2bb 100644 --- a/chrome/browser/sharing/sharing_device_registration_impl.h +++ b/chrome/browser/sharing/sharing_device_registration_impl.h
@@ -85,11 +85,11 @@ FRIEND_TEST_ALL_PREFIXES(SharingDeviceRegistrationImplTest, RegisterDeviceTest_Success); - void RetrieveTargetInfo(const std::string& authorized_entity, + void RetrieveTargetInfo(const std::string& sender_id, TargetInfoCallback callback); void OnFCMTokenReceived(TargetInfoCallback callback, - const std::string& authorized_entity, + const std::string& sender_id, const std::string& fcm_token, instance_id::InstanceID::Result result); @@ -98,16 +98,8 @@ std::string p256dh, std::string auth_secret); - void OnVapidTargetInfoRetrieved( - RegistrationCallback callback, - std::optional<std::string> authorized_entity, - SharingDeviceRegistrationResult result, - std::optional<syncer::DeviceInfo::SharingTargetInfo> vapid_target_info); - void OnSharingTargetInfoRetrieved( RegistrationCallback callback, - std::optional<std::string> authorized_entity, - std::optional<syncer::DeviceInfo::SharingTargetInfo> vapid_target_info, SharingDeviceRegistrationResult result, std::optional<syncer::DeviceInfo::SharingTargetInfo> sharing_target_info); @@ -120,15 +112,9 @@ void OnFCMTokenDeleted(RegistrationCallback callback, instance_id::InstanceID::Result result); - // Returns the authorization entity for FCM registration. - std::optional<std::string> GetAuthorizationEntity() const; - // Computes and returns a set of all enabled features on the device. - // |supports_vapid|: If set to true, then enabled features with VAPID suffix - // will be returned, meaning old clients can send VAPID message to this device - // for those features. - std::set<sync_pb::SharingSpecificFields_EnabledFeatures> GetEnabledFeatures( - bool supports_vapid) const; + std::set<sync_pb::SharingSpecificFields_EnabledFeatures> GetEnabledFeatures() + const; raw_ptr<PrefService> pref_service_; raw_ptr<SharingSyncPreference> sharing_sync_preference_;
diff --git a/chrome/browser/sharing/sharing_device_registration_impl_unittest.cc b/chrome/browser/sharing/sharing_device_registration_impl_unittest.cc index 1d43f51..c48620e 100644 --- a/chrome/browser/sharing/sharing_device_registration_impl_unittest.cc +++ b/chrome/browser/sharing/sharing_device_registration_impl_unittest.cc
@@ -76,6 +76,7 @@ base::TimeDelta time_to_live, std::set<Flags> flags, GetTokenCallback callback) override { + // TODO(crbug.com/40253551): Verify that only `kSharingSenderID` is used. if (authorized_entity == kSharingSenderID) { std::move(callback).Run(kSenderIdFCMToken, result_); } else { @@ -196,22 +197,16 @@ } std::set<sync_pb::SharingSpecificFields::EnabledFeatures> - GetExpectedEnabledFeatures(bool supports_vapid) { + GetExpectedEnabledFeatures() { std::set<sync_pb::SharingSpecificFields::EnabledFeatures> features; // IsClickToCallSupported() involves JNI call which is hard to test. if (sharing_device_registration_.IsClickToCallSupported()) { features.insert(sync_pb::SharingSpecificFields::CLICK_TO_CALL_V2); - if (supports_vapid) { - features.insert(sync_pb::SharingSpecificFields::CLICK_TO_CALL_VAPID); - } } // Shared clipboard should always be supported. features.insert(sync_pb::SharingSpecificFields::SHARED_CLIPBOARD_V2); - if (supports_vapid) { - features.insert(sync_pb::SharingSpecificFields::SHARED_CLIPBOARD_VAPID); - } if (sharing_device_registration_.IsRemoteCopySupported()) { features.insert(sync_pb::SharingSpecificFields::REMOTE_COPY); @@ -285,7 +280,7 @@ RegisterDeviceSync(); std::set<sync_pb::SharingSpecificFields::EnabledFeatures> enabled_features = - GetExpectedEnabledFeatures(/*supports_vapid=*/false); + GetExpectedEnabledFeatures(); syncer::DeviceInfo::SharingInfo expected_sharing_info( /*vapid_target_info=*/{}, {kSenderIdFCMToken, kSenderIdP256dh, kSenderIdAuthSecret}, @@ -323,7 +318,7 @@ RegisterDeviceSync(); std::set<sync_pb::SharingSpecificFields::EnabledFeatures> enabled_features = - GetExpectedEnabledFeatures(/*supports_vapid=*/false); + GetExpectedEnabledFeatures(); syncer::DeviceInfo::SharingInfo expected_sharing_info( syncer::DeviceInfo::SharingTargetInfo(), {kSenderIdFCMToken, kSenderIdP256dh, kSenderIdAuthSecret}, @@ -402,7 +397,7 @@ // Device should be registered with the new FCM token. std::set<sync_pb::SharingSpecificFields::EnabledFeatures> enabled_features = - GetExpectedEnabledFeatures(/*supports_vapid=*/false); + GetExpectedEnabledFeatures(); syncer::DeviceInfo::SharingInfo expected_sharing_info( /*vapid_target_info=*/{}, {kSenderIdFCMToken, kSenderIdP256dh, kSenderIdAuthSecret},
diff --git a/components/sharing_message/sharing_device_source_sync.cc b/components/sharing_message/sharing_device_source_sync.cc index 92deb30..d4ca138 100644 --- a/components/sharing_message/sharing_device_source_sync.cc +++ b/components/sharing_message/sharing_device_source_sync.cc
@@ -153,13 +153,6 @@ sync_pb::SharingSpecificFields::EnabledFeatures required_feature) const { std::set<SharingSpecificFields::EnabledFeatures> accepted_features{ required_feature}; - if (required_feature == SharingSpecificFields::CLICK_TO_CALL_V2) { - accepted_features.insert(SharingSpecificFields::CLICK_TO_CALL_VAPID); - } - if (required_feature == SharingSpecificFields::SHARED_CLIPBOARD_V2) { - accepted_features.insert(SharingSpecificFields::SHARED_CLIPBOARD_VAPID); - } - bool can_send_via_vapid = CanSendViaVapid(sync_service_); bool can_send_via_sender_id = CanSendViaSenderID(sync_service_); @@ -191,7 +184,7 @@ return true; } - // Checks whether |device| supports any of |accepted_features|. + // Checks whether `device` supports any of `accepted_features`. return base::STLSetIntersection< std::vector<SharingSpecificFields::EnabledFeatures>>( device->sharing_info()->enabled_features, accepted_features)
diff --git a/components/sharing_message/sharing_device_source_sync_unittest.cc b/components/sharing_message/sharing_device_source_sync_unittest.cc index b6cad26..075192f 100644 --- a/components/sharing_message/sharing_device_source_sync_unittest.cc +++ b/components/sharing_message/sharing_device_source_sync_unittest.cc
@@ -303,19 +303,6 @@ EXPECT_TRUE(candidates.empty()); } -TEST_F(SharingDeviceSourceSyncTest, - GetDeviceCandidates_AlternativeRequirement) { - auto device_source = CreateDeviceSource(/*wait_until_ready=*/true); - auto device_info = CreateDeviceInfo( - "client_name", sync_pb::SharingSpecificFields::CLICK_TO_CALL_VAPID); - fake_device_info_tracker_.Add(device_info.get()); - - auto devices = device_source->GetDeviceCandidates( - sync_pb::SharingSpecificFields::CLICK_TO_CALL_V2); - ASSERT_EQ(1u, devices.size()); - EXPECT_EQ(device_info->guid(), devices[0].guid()); -} - TEST_F(SharingDeviceSourceSyncTest, GetDeviceCandidates_RenameAfterFiltering) { auto device_source = CreateDeviceSource(/*wait_until_ready=*/true);
diff --git a/components/sharing_message/sharing_sync_preference.cc b/components/sharing_message/sharing_sync_preference.cc index 8337b45..4d7da2a 100644 --- a/components/sharing_message/sharing_sync_preference.cc +++ b/components/sharing_message/sharing_sync_preference.cc
@@ -72,6 +72,7 @@ using sync_pb::SharingSpecificFields; +// TODO(crbug.com/40253551): Remove `authorized_entity` field. SharingSyncPreference::FCMRegistration::FCMRegistration( std::optional<std::string> authorized_entity, base::Time timestamp)
diff --git a/components/sharing_message/sharing_utils.cc b/components/sharing_message/sharing_utils.cc index 08e0f553..b915efa1 100644 --- a/components/sharing_message/sharing_utils.cc +++ b/components/sharing_message/sharing_utils.cc
@@ -30,6 +30,8 @@ } // namespace +// TODO(crbug.com/40253551): remove the method (and it should return false by +// default). bool CanSendViaVapid(syncer::SyncService* sync_service) { // Can send using VAPID key in sharing.vapid_key preferences. return sync_service->GetActiveDataTypes().Has(syncer::PREFERENCES);
diff --git a/components/sync/protocol/device_info_specifics.proto b/components/sync/protocol/device_info_specifics.proto index 89518c1..2800793 100644 --- a/components/sync/protocol/device_info_specifics.proto +++ b/components/sync/protocol/device_info_specifics.proto
@@ -125,27 +125,25 @@ // devices. message SharingSpecificFields { // FCM registration token of device subscribed using VAPID key. - // TODO(crbug.com/40102247): Deprecate when VAPID migration is over. - optional string vapid_fcm_token = 1; + // TODO(crbug.com/40253551): Deprecate when VAPID migration is over. + optional string vapid_fcm_token = 1 [deprecated = true]; // Public key for message encryption [RFC8291] using VAPID key. - // TODO(crbug.com/40102247): Deprecate when VAPID migration is over. - optional bytes vapid_p256dh = 2; + optional bytes vapid_p256dh = 2 [deprecated = true]; // Auth secret for message encryption [RFC8291] using VAPID key. - // TODO(crbug.com/40102247): Deprecate when VAPID migration is over. - optional bytes vapid_auth_secret = 3; + optional bytes vapid_auth_secret = 3 [deprecated = true]; // Enum defining available Sharing features. enum EnabledFeatures { reserved 5; reserved "PEER_CONNECTION"; + reserved 1; + reserved "CLICK_TO_CALL_VAPID"; + reserved 2; + reserved "SHARED_CLIPBOARD_VAPID"; UNKNOWN = 0; - // Supports Click to Call only via VAPID. - CLICK_TO_CALL_VAPID = 1; - // Supports Shared Clipboard only via VAPID. - SHARED_CLIPBOARD_VAPID = 2; SMS_FETCHER = 3; REMOTE_COPY = 4; DISCOVERY = 6;
diff --git a/components/sync_device_info/device_info.h b/components/sync_device_info/device_info.h index e3d97a9..abc8c23 100644 --- a/components/sync_device_info/device_info.h +++ b/components/sync_device_info/device_info.h
@@ -44,6 +44,7 @@ // A struct that holds information regarding to Sharing features. struct SharingInfo { + // TODO(crbug.com/40253551): remove `vapid_target_info`. SharingInfo(SharingTargetInfo vapid_target_info, SharingTargetInfo sharing_target_info, std::string chime_representative_target_id,