From f5e526317bc3c48c884b45554a58bb4b2db2fd3f Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Thu, 21 Nov 2024 13:44:28 -0800 Subject: [PATCH] Force certificate generation and upload on first time uploading contacts. PiperOrigin-RevId: 698901640 --- .../nearby_share_certificate_manager_impl.cc | 10 ++-- .../nearby_share_contact_manager_impl.cc | 10 ++-- .../nearby_share_contact_manager_impl_test.cc | 55 ++++++++++++++----- 3 files changed, 50 insertions(+), 25 deletions(-) diff --git a/sharing/certificates/nearby_share_certificate_manager_impl.cc b/sharing/certificates/nearby_share_certificate_manager_impl.cc index 686b51cd..fdb94d23 100644 --- a/sharing/certificates/nearby_share_certificate_manager_impl.cc +++ b/sharing/certificates/nearby_share_certificate_manager_impl.cc @@ -513,10 +513,12 @@ void NearbyShareCertificateManagerImpl::OnContactsDownloaded( void NearbyShareCertificateManagerImpl::OnContactsUploaded( bool did_contacts_change_since_last_upload) { - executor_->PostTask([&, did_contacts_change_since_last_upload]() { - NL_LOG(INFO) << __func__ << ": Handle to Contacts uploaded."; - if (!did_contacts_change_since_last_upload) return; - + if (!did_contacts_change_since_last_upload) { + LOG(INFO) << "Contacts not changed since last upload."; + return; + } + executor_->PostTask([this]() { + LOG(INFO) << "Handle Contacts uploaded."; // If any of the uploaded contact data - the contact list or the allowlist - // has changed since the previous successful upload, recreate certificates. // We do not want to continue using the current certificates because they diff --git a/sharing/contacts/nearby_share_contact_manager_impl.cc b/sharing/contacts/nearby_share_contact_manager_impl.cc index 5688e2ee..07afb338 100644 --- a/sharing/contacts/nearby_share_contact_manager_impl.cc +++ b/sharing/contacts/nearby_share_contact_manager_impl.cc @@ -382,12 +382,10 @@ void NearbyShareContactManagerImpl::OnContactsUploadFinished( absl::ToUnixSeconds(upload_time)); if (last_contact_upload_hash.empty()) { - // If no contacts are uploaded before, set the flag to false in order to - // prevent the certificate manager from regenerating certificates. - NL_LOG(WARNING) << __func__ - << ": Mark contacts change flag to false due to no " - "contacts upload before."; - did_contacts_change_since_last_upload = false; + // If no contacts are uploaded before, set the flag to true in order to + // force a certificate regeneration and upload. + LOG(WARNING) << "Set contacts change flag to true, no previous upload."; + did_contacts_change_since_last_upload = true; } NotifyContactsUploaded(did_contacts_change_since_last_upload); } diff --git a/sharing/contacts/nearby_share_contact_manager_impl_test.cc b/sharing/contacts/nearby_share_contact_manager_impl_test.cc index 722829e6..8c36cc06 100644 --- a/sharing/contacts/nearby_share_contact_manager_impl_test.cc +++ b/sharing/contacts/nearby_share_contact_manager_impl_test.cc @@ -226,6 +226,7 @@ class NearbyShareContactManagerImplTest void DownloadContacts(bool download_success, bool expect_upload, bool upload_success, std::optional> contacts, + bool expect_contacts_changed, std::optional> expected_contacts = std::nullopt) { // Track for download contacts. @@ -280,6 +281,11 @@ class NearbyShareContactManagerImplTest // Verify upload notification was sent on success. EXPECT_EQ(contacts_uploaded_notifications_.size(), num_upload_notifications + (upload_success ? 1 : 0)); + if (upload_success) { + EXPECT_EQ(contacts_uploaded_notifications_.back() + .did_contacts_change_since_last_upload, + expect_contacts_changed); + } // Verify that the result is sent to download/upload scheduler. EXPECT_EQ(download_and_upload_scheduler()->handled_results().size(), num_download_and_upload_handled_results + 1); @@ -373,6 +379,9 @@ class NearbyShareContactManagerImplTest }; TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_WithFirstUpload) { + // Clear contact hash + preference_manager().SetString(prefs::kNearbySharingContactUploadHashName, + ""); std::vector contact_records = TestContactRecordList(/*num_contacts=*/4u); @@ -382,7 +391,8 @@ TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_WithFirstUpload) { // requested, which succeeds. DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); SetDownloadSuccessResult(contact_records); SetUploadResult(true); @@ -390,7 +400,8 @@ TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_WithFirstUpload) { // changed, so no upload should be made DownloadContacts(/*download_success=*/true, /*expect_upload=*/false, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/false); } TEST_F(NearbyShareContactManagerImplTest, @@ -404,7 +415,8 @@ TEST_F(NearbyShareContactManagerImplTest, // requested, which succeeds. DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); // When contacts are downloaded again, we detect that contacts have changed // since the last upload. @@ -413,7 +425,8 @@ TEST_F(NearbyShareContactManagerImplTest, SetUploadResult(true); DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); } TEST_F(NearbyShareContactManagerImplTest, @@ -429,7 +442,8 @@ TEST_F(NearbyShareContactManagerImplTest, // requested, which succeeds. DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); fake_context().fake_clock()->FastForward(absl::Hours(72)); SetDownloadSuccessResult(contact_records); @@ -438,7 +452,8 @@ TEST_F(NearbyShareContactManagerImplTest, // changed, but since the hash expired, we upload again. DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); } TEST_F(NearbyShareContactManagerImplTest, @@ -454,7 +469,8 @@ TEST_F(NearbyShareContactManagerImplTest, // requested, which succeeds. DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); fake_context().fake_clock()->FastForward(absl::Hours(72)); SetDownloadSuccessResult(contact_records); @@ -463,7 +479,8 @@ TEST_F(NearbyShareContactManagerImplTest, // changed, but since the hash expired, we upload again. DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/false, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); SetDownloadSuccessResult(contact_records); SetUploadResult(true); @@ -471,14 +488,16 @@ TEST_F(NearbyShareContactManagerImplTest, // changed, last upload failed, we upload again. DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); } TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_FailDownload) { SetDownloadFailureResult(); DownloadContacts(/*download_success=*/false, /*expect_upload=*/false, /*upload_success=*/false, - /*contacts=*/std::nullopt); + /*contacts=*/std::nullopt, + /*expect_contacts_changed=*/false); } TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_RetryFailedUpload) { @@ -492,7 +511,8 @@ TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_RetryFailedUpload) { // requested, which succeeds. DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); // When contacts are downloaded again, we detect that contacts have changed // since the last upload. Fail this upload. @@ -501,7 +521,8 @@ TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_RetryFailedUpload) { SetUploadResult(false); DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/false, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); // When contacts are downloaded again, we should continue to indicate that // contacts have changed since the last upload, and attempt another upload. @@ -511,7 +532,8 @@ TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_RetryFailedUpload) { SetUploadResult(true); DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); } TEST_F(NearbyShareContactManagerImplTest, @@ -531,6 +553,7 @@ TEST_F(NearbyShareContactManagerImplTest, DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, /*contacts=*/contact_records, + /*expect_contacts_changed=*/true, /*expected_contacts=*/expected_contacts); ASSERT_EQ(contacts_downloaded_notifications().size(), 1); @@ -556,7 +579,8 @@ TEST_F(NearbyShareContactManagerImplTest, ContactUploadHash) { SetUploadResult(true); DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, - /*contacts=*/contact_records); + /*contacts=*/contact_records, + /*expect_contacts_changed=*/true); // Hardcode expected contact upload hash to ensure that hashed value is // consistent across process starts. If this test starts to fail, check one @@ -586,7 +610,8 @@ TEST_F(NearbyShareContactManagerImplTest, ContactUploadHash) { SetUploadResult(true); DownloadContacts(/*download_success=*/true, /*expect_upload=*/false, /*upload_success=*/true, - /*contacts=*/shuffled_contacts); + /*contacts=*/shuffled_contacts, + /*expect_contacts_changed=*/false); EXPECT_EQ(preference_manager().GetString( prefs::kNearbySharingContactUploadHashName, std::string()),