From bdca4aa7075759db248cbee411da640dabc3cd6f Mon Sep 17 00:00:00 2001 From: Suet-Fei Li Date: Mon, 23 Sep 2024 13:09:36 -0700 Subject: [PATCH] Add de-duplicate in NearbySharing based on device_id matching. PiperOrigin-RevId: 677910987 --- sharing/BUILD | 6 +- .../generated/nearby_sharing_feature_flags.h | 4 + sharing/nearby_sharing_service_impl.cc | 176 +++++++++++++----- sharing/nearby_sharing_service_impl.h | 12 ++ sharing/nearby_sharing_service_impl_test.cc | 168 +++++++++++++++++ sharing/outgoing_share_session.cc | 14 ++ sharing/outgoing_share_session.h | 7 + sharing/outgoing_share_session_test.cc | 48 ++++- sharing/share_session.h | 10 +- 9 files changed, 395 insertions(+), 50 deletions(-) diff --git a/sharing/BUILD b/sharing/BUILD index 391203d4dc..096c1510c0 100644 --- a/sharing/BUILD +++ b/sharing/BUILD @@ -19,7 +19,7 @@ cc_library( hdrs = ["nearby_connections_types.h"], deps = [ "//internal/base:files", - "//internal/crypto_cros", + "//internal/crypto_cros", # buildcleaner: keep "//internal/interop:authentication_status", "//sharing/common:compatible_u8_string", "@com_google_absl//absl/random", @@ -336,7 +336,6 @@ cc_library( "@com_google_absl//absl/hash", "@com_google_absl//absl/meta:type_traits", "@com_google_absl//absl/random", - "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:str_format", @@ -806,9 +805,12 @@ cc_test( ":transfer_metadata_matchers", ":types", "//internal/analytics:mock_event_logger", + "//internal/network:url", "//internal/platform/implementation/g3", # fixdeps: keep "//internal/test", "//sharing/analytics", + "//sharing/certificates:test_support", + "//sharing/common:enum", "//sharing/proto:wire_format_cc_proto", "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_absl//absl/strings:string_view", diff --git a/sharing/flags/generated/nearby_sharing_feature_flags.h b/sharing/flags/generated/nearby_sharing_feature_flags.h index 7a630ec674..f63de976ac 100755 --- a/sharing/flags/generated/nearby_sharing_feature_flags.h +++ b/sharing/flags/generated/nearby_sharing_feature_flags.h @@ -94,6 +94,9 @@ constexpr auto kShowAdminModeWarning = // Update track constexpr auto kUpdateTrack = flags::Flag(kConfigPackage, "45409861", ""); +// Apply endpoints de-duplication algorithms. +constexpr auto kApplyEndpointsDedup = + flags::Flag(kConfigPackage, "45656298", false); inline absl::btree_map&> GetBoolFlags() { return { @@ -115,6 +118,7 @@ inline absl::btree_map&> GetBoolFlags() { {45630055, kUseGrpcClient}, {45417647, kEnableQrCodeUi}, {45410558, kShowAdminModeWarning}, + {45656298, kApplyEndpointsDedup}, }; } diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index 68ff6a0584..7e132c7a69 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -1351,12 +1351,11 @@ void NearbySharingServiceImpl::OnPrivateCertificatesChanged() { } void NearbySharingServiceImpl::OnLoginSucceeded(absl::string_view account_id) { - RunOnNearbySharingServiceThread( - "on_login_succeeded", [this]() { - NL_LOG(INFO) << __func__ << ": Account login."; + RunOnNearbySharingServiceThread("on_login_succeeded", [this]() { + NL_LOG(INFO) << __func__ << ": Account login."; - ResetAllSettings(/*logout=*/false); - }); + ResetAllSettings(/*logout=*/false); + }); } void NearbySharingServiceImpl::OnLogoutSucceeded(absl::string_view account_id, @@ -1683,12 +1682,18 @@ void NearbySharingServiceImpl::HandleEndpointDiscovered( // found one. // Looking for the ShareTarget based on endpoint id. - if (outgoing_share_target_map_.find(endpoint_id) != - outgoing_share_target_map_.end()) { - FinishEndpointDiscoveryEvent(); - return; + if (!NearbyFlags::GetInstance().GetBoolFlag( + config_package_nearby::nearby_sharing_feature:: + kApplyEndpointsDedup)) { + if (outgoing_share_target_map_.find(endpoint_id) != + outgoing_share_target_map_.end()) { + LOG(INFO) << __func__ << ": Ignoring endpoint_id: " << endpoint_id + << " because it is already in the " + << "outgoingShareTargetMap."; + FinishEndpointDiscoveryEvent(); + return; + } } - // Once we get the advertisement, the first thing to do is decrypt the // certificate. NearbyShareEncryptedMetadataKey encrypted_metadata_key( @@ -1707,7 +1712,7 @@ void NearbySharingServiceImpl::HandleEndpointDiscovered( "outgoing_decrypted_certificate", [this, endpoint_id_copy, endpoint_info_copy, advertisement_copy, decrypted_public_certificate]() { - NL_LOG(INFO) << __func__ << ": Decrypted public certificate"; + LOG(INFO) << __func__ << ": Decrypted public certificate"; OnOutgoingDecryptedCertificate( endpoint_id_copy, endpoint_info_copy, advertisement_copy, decrypted_public_certificate); @@ -1749,11 +1754,18 @@ void NearbySharingServiceImpl::OnOutgoingDecryptedCertificate( absl::string_view endpoint_id, absl::Span endpoint_info, const Advertisement& advertisement, std::optional certificate) { - // Check again for this endpoint id, to avoid race conditions. - if (outgoing_share_target_map_.find(endpoint_id) != - outgoing_share_target_map_.end()) { - FinishEndpointDiscoveryEvent(); - return; + bool apply_endpoints_dedup = NearbyFlags::GetInstance().GetBoolFlag( + config_package_nearby::nearby_sharing_feature::kApplyEndpointsDedup); + if (!apply_endpoints_dedup) { + // Check again for this endpoint id, to avoid race conditions. + if (outgoing_share_target_map_.find(endpoint_id) != + outgoing_share_target_map_.end()) { + LOG(INFO) << __func__ << ": Ignoring endpoint_id: " << endpoint_id + << " because it is already in the " + << "outgoingShareTargetMap."; + FinishEndpointDiscoveryEvent(); + return; + } } // The certificate provides the device name, in order to create a ShareTarget @@ -1763,7 +1775,7 @@ void NearbySharingServiceImpl::OnOutgoingDecryptedCertificate( /*is_incoming=*/false); if (!share_target.has_value()) { if (discovered_advertisements_retried_set_.contains(endpoint_id)) { - NL_LOG(INFO) + LOG(INFO) << __func__ << ": Don't try to download public certificates again for endpoint=" << endpoint_id; @@ -1771,9 +1783,10 @@ void NearbySharingServiceImpl::OnOutgoingDecryptedCertificate( return; } - NL_LOG(INFO) - << __func__ << ": Failed to convert discovered advertisement to share " - << "target. Ignoring endpoint until next certificate download."; + LOG(INFO) << __func__ + << ": Failed to convert discovered advertisement to share " + << "target. Ignoring endpoint: " << endpoint_id + << " until next certificate download."; std::vector endpoint_info_data(endpoint_info.begin(), endpoint_info.end()); @@ -1781,13 +1794,23 @@ void NearbySharingServiceImpl::OnOutgoingDecryptedCertificate( FinishEndpointDiscoveryEvent(); return; } + if (apply_endpoints_dedup) { + if (FindDuplicateInOutgoingShareTargets(endpoint_id, *share_target)) { + DeduplicateInOutgoingShareTarget(*share_target, endpoint_id, + std::move(certificate)); + FinishEndpointDiscoveryEvent(); + return; + } + } + CreateOutgoingShareSession(*share_target, endpoint_id, std::move(certificate)); // Update the endpoint id for the share target. - NL_LOG(INFO) << __func__ - << ": An endpoint has been discovered, with an advertisement " - "containing a valid share target."; + LOG(INFO) << __func__ << ": An endpoint: " << endpoint_id + << " has been discovered, with an advertisement " + "containing a valid share target with id: " + << share_target->id; // Log analytics event of discovering share target. analytics_recorder_->NewDiscoverShareTarget( @@ -1802,10 +1825,10 @@ void NearbySharingServiceImpl::OnOutgoingDecryptedCertificate( share_foreground_send_surface_start_timestamp_)); // Notifies the user that we discovered a device. - NL_VLOG(1) << __func__ << ": There are " - << (foreground_send_surface_map_.size() + - background_send_surface_map_.size()) - << " discovery callbacks be called."; + VLOG(1) << __func__ << ": There are " + << (foreground_send_surface_map_.size() + + background_send_surface_map_.size()) + << " discovery callbacks be called."; for (auto& entry : foreground_send_surface_map_) { entry.second.OnShareTargetDiscovered(*share_target); @@ -1814,8 +1837,8 @@ void NearbySharingServiceImpl::OnOutgoingDecryptedCertificate( entry.second.OnShareTargetDiscovered(*share_target); } - NL_VLOG(1) << __func__ << ": Reported OnShareTargetDiscovered " - << (context_->GetClock()->Now() - scanning_start_timestamp_); + VLOG(1) << __func__ << ": Reported OnShareTargetDiscovered at timestamp: " + << (context_->GetClock()->Now() - scanning_start_timestamp_); FinishEndpointDiscoveryEvent(); } @@ -2356,9 +2379,9 @@ void NearbySharingServiceImpl::RemoveOutgoingShareTargetWithEndpointId( return; } - NL_VLOG(1) << __func__ << ": Removing (endpoint_id=" << it->first - << ", share_target.id=" << it->second.id - << ") from outgoing share target map"; + LOG(INFO) << __func__ << ": Removing (endpoint_id=" << it->first + << ", share_target.id=" << it->second.id + << ") from outgoing share target map"; ShareTarget share_target = std::move(it->second); outgoing_share_target_map_.erase(it); @@ -2597,10 +2620,9 @@ void NearbySharingServiceImpl::OnIncomingAdvertisementDecoded( placeholder_share_target_id, decrypted_public_certificate = std::move(decrypted_public_certificate)]() { - OnIncomingDecryptedCertificate( - endpoint_id, advertisement_copy, - placeholder_share_target_id, - decrypted_public_certificate); + OnIncomingDecryptedCertificate(endpoint_id, advertisement_copy, + placeholder_share_target_id, + decrypted_public_certificate); }); }); } @@ -2698,7 +2720,7 @@ void NearbySharingServiceImpl::OnOutgoingTransferUpdate( is_connecting_ = false; OnTransferComplete(); } else if (metadata.status() == - TransferMetadata::Status::kAwaitingLocalConfirmation) { + TransferMetadata::Status::kAwaitingLocalConfirmation) { is_connecting_ = false; OnTransferStarted(/*is_incoming=*/false); } @@ -3264,18 +3286,82 @@ IncomingShareSession& NearbySharingServiceImpl::CreateIncomingShareSession( return it->second; } -OutgoingShareSession& NearbySharingServiceImpl::CreateOutgoingShareSession( +void NearbySharingServiceImpl::DeduplicateInOutgoingShareTarget( const ShareTarget& share_target, absl::string_view endpoint_id, std::optional certificate) { - // We need to explicitly remove any previous share target for - // |endpoint_id| if one exists, notifying observers that a share target is - // lost. - const auto it = outgoing_share_target_map_.find(endpoint_id); - if (it != outgoing_share_target_map_.end() && - it->second.id != share_target.id) { - RemoveOutgoingShareTargetWithEndpointId(endpoint_id); + // TODO(b/343764269): may need to update last_outgoing_metadata_ if the + // deduped target id matches the one in last_outgoing_metadata_. + // But since we do not modify the share target of a connected session, it may + // not happen. + + auto session_it = outgoing_share_session_map_.find(share_target.id); + if (session_it == outgoing_share_session_map_.end()) { + LOG(WARNING) << __func__ << ": share_target.id=" << share_target.id + << " not found in outgoing share session map."; + return; + } + if (session_it->second.IsConnected()) { + LOG(INFO) << __func__ << ": share_target.id=" << share_target.id + << " is connected, not updating outgoing_share_session_map_."; + return; + } + session_it->second.UpdateSessionForDedup(share_target, std::move(certificate), + endpoint_id); + + for (auto& entry : foreground_send_surface_map_) { + entry.second.OnShareTargetUpdated(share_target); + } + for (auto& entry : background_send_surface_map_) { + entry.second.OnShareTargetUpdated(share_target); + } + + LOG(INFO) << __func__ + << ": [Dedupped] Reported OnShareTargetUpdated to all surfaces " + "for share_target: " + << share_target.ToString(); +} + +bool NearbySharingServiceImpl::FindDuplicateInOutgoingShareTargets( + absl::string_view endpoint_id, ShareTarget& share_target) { + // If the duplicate is found, share_target.id needs to be updated to the old + // "discovered" share_target_id so OnShareTargetUpdated matches a target that + // was discovered before. + + auto it = outgoing_share_target_map_.find(endpoint_id); + if (it != outgoing_share_target_map_.end()) { + // If endpoint info changes for an endpoint ID, NC will send a rediscovery + // event for the same endpoint id. + LOG(INFO) << __func__ + << ": [Dedupped] Found duplicate endpoint_id: " << endpoint_id + << " in outgoing_share_target_map, share_target.id changed from: " + << share_target.id << " to " << it->second.id; + share_target.id = it->second.id; + it->second = share_target; + return true; + } + + for (auto it = outgoing_share_target_map_.begin(); + it != outgoing_share_target_map_.end(); ++it) { + if (it->second.device_id == share_target.device_id) { + LOG(INFO) + << __func__ + << ": [Dedupped] Found duplicate device_id. endpoint ID " + "changed from: " + << it->first << " to " << endpoint_id + << " in outgoing_share_target_map, share_target.id changed from: " + << share_target.id << " to " << it->second.id; + share_target.id = it->second.id; + outgoing_share_target_map_.erase(it); + outgoing_share_target_map_.insert_or_assign(endpoint_id, share_target); + return true; + } } + return false; +} +OutgoingShareSession& NearbySharingServiceImpl::CreateOutgoingShareSession( + const ShareTarget& share_target, absl::string_view endpoint_id, + std::optional certificate) { NL_VLOG(1) << __func__ << ": Adding (endpoint_id=" << endpoint_id << ", share_target_id=" << share_target.id << ") to outgoing share target map"; diff --git a/sharing/nearby_sharing_service_impl.h b/sharing/nearby_sharing_service_impl.h index cb58786b98..6d8f0131ad 100644 --- a/sharing/nearby_sharing_service_impl.h +++ b/sharing/nearby_sharing_service_impl.h @@ -369,6 +369,18 @@ class NearbySharingServiceImpl const ShareTarget& share_target, absl::string_view endpoint_id, std::optional certificate); + // The share_target's id is updated to match the old one + // and OnShareTargetUpdated is called. + void DeduplicateInOutgoingShareTarget( + const ShareTarget& share_target, absl::string_view endpoint_id, + std::optional certificate); + + // Looks for a duplicate of the given share target in the outgoing share + // target map. The share target's id is changed to match an existing target if + // available. Returns true if the duplicate is found. + bool FindDuplicateInOutgoingShareTargets(absl::string_view endpoint_id, + ShareTarget& share_target); + ShareSession* GetShareSession(int64_t share_target_id); IncomingShareSession* GetIncomingShareSession(int64_t share_target_id); OutgoingShareSession* GetOutgoingShareSession(int64_t share_target_id); diff --git a/sharing/nearby_sharing_service_impl_test.cc b/sharing/nearby_sharing_service_impl_test.cc index e7fe0a662b..e26f962746 100644 --- a/sharing/nearby_sharing_service_impl_test.cc +++ b/sharing/nearby_sharing_service_impl_test.cc @@ -112,10 +112,12 @@ using ::nearby::sharing::service::proto::IntroductionFrame; using ::nearby::sharing::service::proto::PairedKeyResultFrame; using ::nearby::sharing::service::proto::TextMetadata; using ::nearby::sharing::service::proto::V1Frame; +using ::testing::_; using ::testing::InSequence; using ::testing::NiceMock; using ::testing::Return; using ::testing::ReturnRef; +using ::testing::SaveArg; using ::testing::StrictMock; using ::testing::UnorderedElementsAre; @@ -391,6 +393,7 @@ class NearbySharingServiceImplTest : public testing::Test { nullptr); NearbyShareContactManagerImpl::Factory::SetFactoryForTesting(nullptr); NearbyShareCertificateManagerImpl::Factory::SetFactoryForTesting(nullptr); + NearbyFlags::GetInstance().ResetOverridedValues(); nearby_fast_initiation_factory_.reset(); } @@ -4063,8 +4066,59 @@ TEST_F(NearbySharingServiceImplTest, service_.reset(); } +TEST_F(NearbySharingServiceImplTest, DedupSameEndpointId) { + NearbyFlags::GetInstance().OverrideBoolFlagValue( + config_package_nearby::nearby_sharing_feature::kApplyEndpointsDedup, + true); + // Start discovery. + SetConnectionType(ConnectionType::kWifi); + MockTransferUpdateCallback transfer_callback; + MockShareTargetDiscoveredCallback discovery_callback; + RegisterSendSurface(&transfer_callback, &discovery_callback, + SendSurfaceState::kForeground); + EXPECT_EQ(certificate_manager()->num_download_public_certificates_calls(), + 1u); + EXPECT_TRUE(fake_nearby_connections_manager_->IsDiscovering()); + { + absl::Notification notification; + // vendor_id is default to 0. + FindEndpoint(/*endpoint_id=*/"1"); + FindEndpointWithVendorId( + /*endpoint_id=*/"1", + static_cast(Advertisement::BlockedVendorId::kSamsung)); + + ::testing::InSequence s; + ShareTarget share_target_1; + ShareTarget share_target_2; + EXPECT_CALL(discovery_callback, OnShareTargetDiscovered(_)) + .WillOnce(SaveArg<0>(&share_target_1)); + + EXPECT_CALL(discovery_callback, OnShareTargetUpdated(_)) + .WillOnce([&](ShareTarget share_target) { + share_target_2 = share_target; + notification.Notify(); + }); + + ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, + /*success=*/true); + ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/2, + /*success=*/true); + EXPECT_EQ(share_target_1.id, share_target_2.id); + // Vendor_id updated. + EXPECT_EQ(share_target_1.vendor_id, 0); + EXPECT_EQ(share_target_2.vendor_id, + static_cast(Advertisement::BlockedVendorId::kSamsung)); + EXPECT_TRUE(notification.WaitForNotificationWithTimeout(kWaitTimeout)); + } + Shutdown(); + service_.reset(); +} + TEST_F(NearbySharingServiceImplTest, RetryDiscoveredEndpointsDownloadCertsAndRetryDecryption) { + NearbyFlags::GetInstance().OverrideBoolFlagValue( + config_package_nearby::nearby_sharing_feature::kApplyEndpointsDedup, + false); // Start discovery. SetConnectionType(ConnectionType::kWifi); MockTransferUpdateCallback transfer_callback; @@ -4125,6 +4179,120 @@ TEST_F(NearbySharingServiceImplTest, service_.reset(); } +// This test verifies the de-dup logic. Since certificates are the same, all the +// share targets are duplicates. +TEST_F(NearbySharingServiceImplTest, EndpointDedupBAsedOnDeviceId) { + NearbyFlags::GetInstance().OverrideBoolFlagValue( + config_package_nearby::nearby_sharing_feature::kApplyEndpointsDedup, + true); + // Start discovery. + SetConnectionType(ConnectionType::kWifi); + MockTransferUpdateCallback transfer_callback; + MockShareTargetDiscoveredCallback discovery_callback; + RegisterSendSurface(&transfer_callback, &discovery_callback, + SendSurfaceState::kForeground); + EXPECT_EQ(certificate_manager()->num_download_public_certificates_calls(), + 1u); + EXPECT_TRUE(fake_nearby_connections_manager_->IsDiscovering()); + // Order of events: + // - Discover endpoint 1 --> decrypts public certificate + // - Discover endpoint 2 --> cannot decrypt public certificate + // - Discover endpoint 3 --> decrypts public certificate + // --> De-dup endpoint 1 with endpoint 3 + // - Discover endpoint 4 --> cannot decrypt public certificate + // - Lose endpoint 3 --> endpoint 3 is purged from map + // ---------------------------------------------------------------- + // - Fire certificate download timer --> certificates downloaded + // - (Re)discover endpoints 2 --> endpoint 2 is insert into map + // - (Re)discover endpoints 4 --> De-dup endpoint 2 with endpoint 4 + { + absl::Notification notification; + // vendor_id is default to 0. + FindEndpoint(/*endpoint_id=*/"1"); + FindInvalidEndpoint(/*endpoint_id=*/"2"); + FindEndpointWithVendorId( + /*endpoint_id=*/"3", + static_cast(Advertisement::BlockedVendorId::kSamsung)); + FindInvalidEndpoint(/*endpoint_id=*/"4"); + LoseEndpoint(/*endpoint_id=*/"3"); + ::testing::InSequence s; + ShareTarget share_target_ep_1; + ShareTarget share_target_ep_3; + EXPECT_CALL(discovery_callback, OnShareTargetDiscovered(_)) + .WillOnce(SaveArg<0>(&share_target_ep_1)); + + // Update endpoint 1 to endpoint 3 + EXPECT_CALL(discovery_callback, OnShareTargetUpdated(_)) + .WillOnce(SaveArg<0>(&share_target_ep_3)); + + // Lost endpoint 3 + EXPECT_CALL(discovery_callback, OnShareTargetLost) + .WillOnce([&](ShareTarget share_target) { notification.Notify(); }); + + ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, + /*success=*/true); + ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/2, + /*success=*/false); + ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/3, + /*success=*/true); + ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/4, + /*success=*/false); + EXPECT_EQ(share_target_ep_1.id, share_target_ep_3.id); + // Vendor_id updated. + EXPECT_EQ(share_target_ep_3.vendor_id, + static_cast(Advertisement::BlockedVendorId::kSamsung)); + EXPECT_TRUE(notification.WaitForNotificationWithTimeout(kWaitTimeout)); + } + FastForward(kCertificateDownloadDuringDiscoveryPeriod); + EXPECT_EQ(certificate_manager()->num_download_public_certificates_calls(), + 2u); + certificate_manager()->NotifyPublicCertificatesDownloaded(); + FlushTesting(); + ShareTarget share_target_ep_4; + { + absl::Notification notification; + ::testing::InSequence s; + EXPECT_CALL(discovery_callback, OnShareTargetDiscovered); + // Update endpoint 2 to endpoint 4 + EXPECT_CALL(discovery_callback, OnShareTargetUpdated) + .WillOnce([&](ShareTarget share_target) { + share_target_ep_4 = share_target; + notification.Notify(); + }); + ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/5, + /*success=*/true); + ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/6, + /*success=*/true); + EXPECT_TRUE(notification.WaitForNotificationWithTimeout(kWaitTimeout)); + } + EXPECT_CALL(discovery_callback, OnShareTargetLost).Times(1); + + // Introduce a small delay for things to settle. + FastForward(absl::Seconds(1)); + + // Send Text to updated endpoint 4. + SetUpKeyVerification( + /*is_incoming=*/false, PairedKeyResultFrame::SUCCESS); + + fake_nearby_connections_manager_->SetRawAuthenticationToken( + /*endpoint_id=*/"4", GetToken()); + fake_nearby_connections_manager_->set_nearby_connection(connection_.get()); + + SetUpOutgoingConnectionUntilAccept(transfer_callback, share_target_ep_4.id); + PayloadInfo info = + AcceptAndSendPayload(transfer_callback, share_target_ep_4.id); + + FinishOutgoingTransfer(transfer_callback, share_target_ep_4.id, + /*complete=*/true, info); + + EXPECT_TRUE(fake_nearby_connections_manager_ + ->connection_endpoint_info(/*endpoint_id=*/"4") + .has_value()); + + Shutdown(); + service_.reset(); +} + TEST_F(NearbySharingServiceImplTest, RetryDiscoveredEndpointsDiscoveryRestartClearsCache) { // Start discovery. diff --git a/sharing/outgoing_share_session.cc b/sharing/outgoing_share_session.cc index b977abd198..484506af53 100644 --- a/sharing/outgoing_share_session.cc +++ b/sharing/outgoing_share_session.cc @@ -29,6 +29,7 @@ #include "internal/platform/task_runner.h" #include "sharing/analytics/analytics_recorder.h" #include "sharing/attachment_container.h" +#include "sharing/certificates/nearby_share_decrypted_public_certificate.h" #include "sharing/constants.h" #include "sharing/file_attachment.h" #include "sharing/internal/public/logging.h" @@ -462,4 +463,17 @@ void OutgoingShareSession::DisconnectionTimeout() { Disconnect(); } +void OutgoingShareSession::UpdateSessionForDedup( + const ShareTarget& share_target, + std::optional certificate, + absl::string_view endpoint_id) { + if (IsConnected()) return; + set_share_target(share_target); + set_endpoint_id(endpoint_id); + if (certificate.has_value()) { + set_certificate(std::move(certificate.value())); + } else { + clear_certificate(); + } +} } // namespace nearby::sharing diff --git a/sharing/outgoing_share_session.h b/sharing/outgoing_share_session.h index 85d10d7e13..9b3d10a828 100644 --- a/sharing/outgoing_share_session.h +++ b/sharing/outgoing_share_session.h @@ -24,9 +24,11 @@ #include #include +#include "absl/strings/string_view.h" #include "internal/platform/clock.h" #include "internal/platform/task_runner.h" #include "sharing/analytics/analytics_recorder.h" +#include "sharing/certificates/nearby_share_decrypted_public_certificate.h" #include "sharing/nearby_connection.h" #include "sharing/nearby_connections_types.h" #include "sharing/nearby_file_handler.h" @@ -129,6 +131,11 @@ class OutgoingShareSession : public ShareSession { void DelayCompleteMetadata(const TransferMetadata& complete_metadata); // Disconnect timeout expired without receiving client disconnect. void DisconnectionTimeout(); + // Used only for OutgoingShareSession De-duplication. + void UpdateSessionForDedup( + const ShareTarget& share_target, + std::optional certificate, + absl::string_view endpoint_id); protected: void InvokeTransferUpdateCallback(const TransferMetadata& metadata) override; diff --git a/sharing/outgoing_share_session_test.cc b/sharing/outgoing_share_session_test.cc index 55d1b5aa4f..2e439d2ffc 100644 --- a/sharing/outgoing_share_session_test.cc +++ b/sharing/outgoing_share_session_test.cc @@ -29,10 +29,13 @@ #include "absl/time/time.h" #include "internal/analytics/mock_event_logger.h" #include "internal/analytics/sharing_log_matchers.h" +#include "internal/network/url.h" #include "internal/test/fake_clock.h" #include "internal/test/fake_task_runner.h" #include "sharing/analytics/analytics_recorder.h" #include "sharing/attachment_container.h" +#include "sharing/certificates/test_util.h" +#include "sharing/common/nearby_share_enums.h" #include "sharing/fake_nearby_connection.h" #include "sharing/fake_nearby_connections_manager.h" #include "sharing/file_attachment.h" @@ -461,7 +464,7 @@ TEST_F(OutgoingShareSessionTest, } TEST_F(OutgoingShareSessionTest, - HandleConnectionResponseUnsuportedTypeResponse) { + HandleConnectionResponseUnsupportedTypeResponse) { ConnectionResponseFrame response; response.set_status(ConnectionResponseFrame::UNSUPPORTED_ATTACHMENT_TYPE); std::optional status = @@ -680,7 +683,7 @@ TEST_F(OutgoingShareSessionTest, ProcessKeyVerificationResultSuccess) { EXPECT_THAT(session_.os_type(), Eq(OSType::WINDOWS)); } -TEST_F(OutgoingShareSessionTest, DelayCompleteMetadataRecevierDisconnect) { +TEST_F(OutgoingShareSessionTest, DelayCompleteMetadataReceiverDisconnect) { FakeNearbyConnection connection; session_.OnConnected(absl::Now(), &connections_manager_, &connection); TransferMetadata complete_metadata = @@ -713,5 +716,46 @@ TEST_F(OutgoingShareSessionTest, DelayCompleteMetadataDisconnectTimeout) { EXPECT_THAT(connection.IsClosed(), IsTrue()); } +TEST_F(OutgoingShareSessionTest, UpdateSessionForDedupWithCertificate) { + EXPECT_FALSE(session_.certificate().has_value()); + EXPECT_FALSE(session_.self_share()); + ShareTarget share_target2{ + "test_update_name", ::nearby::network::Url(), ShareTargetType::kPhone, + /* is_incoming */ true, "test_update_full_name", + /* is_known */ true, "test_update_device_id", true}; + session_.UpdateSessionForDedup(share_target2, + GetNearbyShareTestDecryptedPublicCertificate(), + "test_update_endpoint_id"); + EXPECT_THAT(session_.share_target().ToString(), Eq(share_target2.ToString())); + EXPECT_TRUE(session_.certificate().has_value()); + EXPECT_THAT(session_.endpoint_id(), Eq("test_update_endpoint_id")); + EXPECT_TRUE(session_.self_share()); +} + +TEST_F(OutgoingShareSessionTest, UpdateSessionForDedupWithoutCertificate) { + session_.set_certificate(GetNearbyShareTestDecryptedPublicCertificate()); + ShareTarget share_target2{ + "test_update_name", ::nearby::network::Url(), ShareTargetType::kPhone, + /* is_incoming */ true, "test_update_full_name", + /* is_known */ true, "test_update_device_id", true}; + session_.UpdateSessionForDedup(share_target2, std::nullopt, + "test_update_endpoint_id"); + // Certificate is cleared. + EXPECT_FALSE(session_.certificate().has_value()); +} + +TEST_F(OutgoingShareSessionTest, UpdateSessionForDedupConnectedIsNoOp) { + auto share_target_org = session_.share_target(); + FakeNearbyConnection connection; + session_.OnConnected(absl::Now(), &connections_manager_, &connection); + ShareTarget share_target2{ + "test_update_name", ::nearby::network::Url(), ShareTargetType::kPhone, + /* is_incoming */ true, "test_update_full_name", + /* is_known */ true, "test_update_device_id", true}; + session_.UpdateSessionForDedup(share_target2, std::nullopt, + "test_update_endpoint_id"); + EXPECT_THAT(session_.share_target().ToString(), + Eq(share_target_org.ToString())); +} } // namespace } // namespace nearby::sharing diff --git a/sharing/share_session.h b/sharing/share_session.h index af2cfe8aca..70ab1b223c 100644 --- a/sharing/share_session.h +++ b/sharing/share_session.h @@ -24,6 +24,7 @@ #include #include "absl/container/flat_hash_map.h" +#include "absl/strings/string_view.h" #include "absl/time/time.h" #include "internal/platform/clock.h" #include "internal/platform/task_runner.h" @@ -57,7 +58,6 @@ class ShareSession { virtual bool IsIncoming() const = 0; std::string endpoint_id() const { return endpoint_id_; } - const std::optional& certificate() const { return certificate_; @@ -66,6 +66,7 @@ class ShareSession { void set_certificate(NearbyShareDecryptedPublicCertificate certificate) { certificate_ = std::move(certificate); } + void clear_certificate() { certificate_ = std::nullopt; } NearbyConnection* connection() const { return connection_; } bool IsConnected() const { return connection_ != nullptr; } @@ -174,6 +175,13 @@ class ShareSession { NearbyConnectionsManager* connections_manager() { return connections_manager_; } + void set_endpoint_id(absl::string_view endpoint_id) { + endpoint_id_ = std::string(endpoint_id); + } + void set_share_target(const ShareTarget& share_target) { + share_target_ = share_target; + self_share_ = share_target.for_self_share; + } private: TaskRunner& service_thread_;