[Nearby] Periodically upload contacts to Nearby Share server

Upload the user's contact list and allowlist to the Nearby Share server
every 24 hours to ensure that the server has the information needed to
distribute certificates. This upload supplements the current strategy of
uploading contacts whenever the contact list or allowlist changes. This
ensures that, if the device record is removed from the Nearby Share
database--after 9 days of inactivity, for example--the client will
resend the contact list even if the contact list has not changed.

Manually tested and verified.

Fixed: 1151498
Change-Id: I9d4669ff1bafc3ac22c385cc8ef4aaf6f9dc355b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2553035
Commit-Queue: Josh Nohle <nohle@chromium.org>
Commit-Queue: James Vecore <vecore@google.com>
Auto-Submit: Josh Nohle <nohle@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#829863}

chrome/browser/nearby_sharing/common/nearby_share_prefs.cc

@@ -42,6 +42,8 @@ const char kNearbySharingSchedulerDownloadDeviceDataPrefName[] =
     "nearby_sharing.scheduler.download_device_data";
 const char kNearbySharingSchedulerDownloadPublicCertificatesPrefName[] =
     "nearby_sharing.scheduler.download_public_certificates";
+const char kNearbySharingSchedulerPeriodicContactUploadPrefName[] =
+    "nearby_sharing.scheduler.periodic_contact_upload";
 const char kNearbySharingSchedulerPrivateCertificateExpirationPrefName[] =
     "nearby_sharing.scheduler.private_certificate_expiration";
 const char kNearbySharingSchedulerPublicCertificateExpirationPrefName[] =
@@ -90,6 +92,8 @@ void RegisterNearbySharingPrefs(PrefRegistrySimple* registry) {
       prefs::kNearbySharingSchedulerDownloadDeviceDataPrefName);
   registry->RegisterDictionaryPref(
       prefs::kNearbySharingSchedulerDownloadPublicCertificatesPrefName);
+  registry->RegisterDictionaryPref(
+      prefs::kNearbySharingSchedulerPeriodicContactUploadPrefName);
   registry->RegisterDictionaryPref(
       prefs::kNearbySharingSchedulerPrivateCertificateExpirationPrefName);
   registry->RegisterDictionaryPref(

chrome/browser/nearby_sharing/common/nearby_share_prefs.h

@@ -26,6 +26,7 @@ extern const char kNearbySharingPublicCertificateExpirationDictPrefName[];
 extern const char kNearbySharingSchedulerContactDownloadAndUploadPrefName[];
 extern const char kNearbySharingSchedulerDownloadDeviceDataPrefName[];
 extern const char kNearbySharingSchedulerDownloadPublicCertificatesPrefName[];
+extern const char kNearbySharingSchedulerPeriodicContactUploadPrefName[];
 extern const char kNearbySharingSchedulerPrivateCertificateExpirationPrefName[];
 extern const char kNearbySharingSchedulerPublicCertificateExpirationPrefName[];
 extern const char kNearbySharingSchedulerUploadDeviceNamePrefName[];

chrome/browser/nearby_sharing/contacts/nearby_share_contact_manager_impl.cc

@@ -26,6 +26,7 @@
 
 namespace {
 
+constexpr base::TimeDelta kContactUploadPeriod = base::TimeDelta::FromHours(24);
 constexpr base::TimeDelta kContactDownloadPeriod =
     base::TimeDelta::FromHours(12);
 constexpr base::TimeDelta kContactDownloadRpcTimeout =
@@ -176,6 +177,16 @@ NearbyShareContactManagerImpl::NearbyShareContactManagerImpl(
       http_client_factory_(http_client_factory),
       local_device_data_manager_(local_device_data_manager),
       profile_user_name_(profile_user_name),
+      periodic_contact_upload_scheduler_(
+          NearbyShareSchedulerFactory::CreatePeriodicScheduler(
+              kContactUploadPeriod,
+              /*retry_failures=*/false,
+              /*require_connectivity=*/true,
+              prefs::kNearbySharingSchedulerPeriodicContactUploadPrefName,
+              pref_service_,
+              base::BindRepeating(&NearbyShareContactManagerImpl::
+                                      OnPeriodicContactsUploadRequested,
+                                  base::Unretained(this)))),
       contact_download_and_upload_scheduler_(
           NearbyShareSchedulerFactory::CreatePeriodicScheduler(
               kContactDownloadPeriod,
@@ -205,10 +216,12 @@ void NearbyShareContactManagerImpl::SetAllowedContacts(
 }
 
 void NearbyShareContactManagerImpl::OnStart() {
+  periodic_contact_upload_scheduler_->Start();
   contact_download_and_upload_scheduler_->Start();
 }
 
 void NearbyShareContactManagerImpl::OnStop() {
+  periodic_contact_upload_scheduler_->Stop();
   contact_download_and_upload_scheduler_->Stop();
 }
 
@@ -234,6 +247,12 @@ std::set<std::string> NearbyShareContactManagerImpl::GetAllowedContacts()
   return allowlist;
 }
 
+void NearbyShareContactManagerImpl::OnPeriodicContactsUploadRequested() {
+  NS_LOG(VERBOSE) << __func__
+                  << ": Periodic Nearby Share contacts upload requested. "
+                  << "Upload will occur after next contacts download.";
+}
+
 void NearbyShareContactManagerImpl::OnContactsDownloadRequested() {
   NS_LOG(VERBOSE) << __func__ << ": Nearby Share contacts download requested.";
 
@@ -279,24 +298,31 @@ void NearbyShareContactManagerImpl::OnContactsDownloadSuccess(
     contacts_to_upload.push_back(CreateLocalContact(profile_user_name_));
   }
 
-  // Only request a contacts upload if the contact list or allowlist has changed
-  // since the last successful upload.
   std::string contact_upload_hash = ComputeHash(contacts_to_upload);
-  if (contact_upload_hash ==
-      pref_service_->GetString(
-          prefs::kNearbySharingContactUploadHashPrefName)) {
-    contact_download_and_upload_scheduler_->HandleResult(/*success=*/true);
+  bool did_contacts_change_since_last_upload =
+      contact_upload_hash !=
+      pref_service_->GetString(prefs::kNearbySharingContactUploadHashPrefName);
+  if (did_contacts_change_since_last_upload) {
+    NS_LOG(VERBOSE) << __func__
+                    << ": Contact list or allowlist changed since last "
+                    << "successful upload to the Nearby Share server.";
+  }
+
+  // Request a contacts upload if the contact list or allowlist has changed
+  // since the last successful upload. Also request an upload periodically.
+  if (did_contacts_change_since_last_upload ||
+      periodic_contact_upload_scheduler_->IsWaitingForResult()) {
+    local_device_data_manager_->UploadContacts(
+        std::move(contacts_to_upload),
+        base::BindOnce(&NearbyShareContactManagerImpl::OnContactsUploadFinished,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       did_contacts_change_since_last_upload,
+                       contact_upload_hash));
     return;
   }
 
-  NS_LOG(VERBOSE) << __func__
-                  << ": Contact list or allowlist changed since last "
-                  << "successful upload to the Nearby Share server. "
-                  << "Starting contacts upload.";
-  local_device_data_manager_->UploadContacts(
-      std::move(contacts_to_upload),
-      base::BindOnce(&NearbyShareContactManagerImpl::OnContactsUploadFinished,
-                     weak_ptr_factory_.GetWeakPtr(), contact_upload_hash));
+  // No upload is needed.
+  contact_download_and_upload_scheduler_->HandleResult(/*success=*/true);
 }
 
 void NearbyShareContactManagerImpl::OnContactsDownloadFailure() {
@@ -313,15 +339,24 @@ void NearbyShareContactManagerImpl::OnContactsDownloadFailure() {
 }
 
 void NearbyShareContactManagerImpl::OnContactsUploadFinished(
+    bool did_contacts_change_since_last_upload,
     const std::string& contact_upload_hash,
     bool success) {
   NS_LOG(VERBOSE) << __func__ << ": Upload of contacts to Nearby Share server "
                   << (success ? "succeeded." : "failed.")
                   << " Contact upload hash: " << contact_upload_hash;
   if (success) {
+    // Only resolve the periodic upload request on success; let the
+    // download-and-upload scheduler handle any failure retries. The periodic
+    // upload scheduler will remember that it has an outstanding request even
+    // after reboot.
+    if (periodic_contact_upload_scheduler_->IsWaitingForResult()) {
+      periodic_contact_upload_scheduler_->HandleResult(success);
+    }
+
     pref_service_->SetString(prefs::kNearbySharingContactUploadHashPrefName,
                              contact_upload_hash);
-    NotifyContactsUploaded(/*did_contacts_change_since_last_upload=*/true);
+    NotifyContactsUploaded(did_contacts_change_since_last_upload);
   }
 
   contact_download_and_upload_scheduler_->HandleResult(success);

chrome/browser/nearby_sharing/contacts/nearby_share_contact_manager_impl.h

@@ -35,7 +35,8 @@ class PrefService;
 // certificates accordingly. This implementation persists a hash of the last
 // uploaded contact data, and after every contacts download, a subsequent upload
 // request is made if we detect that the contact list or allowlist has changed
-// since the last successful upload.
+// since the last successful upload. We also schedule periodic contact uploads
+// just in case the server removed the record.
 //
 // In addition to supporting on-demand contact downloads, this implementation
 // periodically checks in with the Nearby Share server to see if the user's
@@ -87,12 +88,14 @@ class NearbyShareContactManagerImpl : public NearbyShareContactManager {
           observer) override;
 
   std::set<std::string> GetAllowedContacts() const;
+  void OnPeriodicContactsUploadRequested();
   void OnContactsDownloadRequested();
   void OnContactsDownloadSuccess(
       std::vector<nearbyshare::proto::ContactRecord> contacts,
       uint32_t num_unreachable_contacts_filtered_out);
   void OnContactsDownloadFailure();
-  void OnContactsUploadFinished(const std::string& contact_upload_hash,
+  void OnContactsUploadFinished(bool did_contacts_change_since_last_upload,
+                                const std::string& contact_upload_hash,
                                 bool success);
   bool SetAllowlist(const std::set<std::string>& new_allowlist);
   void NotifyMojoObserverContactsDownloaded(
@@ -104,6 +107,7 @@ class NearbyShareContactManagerImpl : public NearbyShareContactManager {
   NearbyShareClientFactory* http_client_factory_ = nullptr;
   NearbyShareLocalDeviceDataManager* local_device_data_manager_ = nullptr;
   std::string profile_user_name_;
+  std::unique_ptr<NearbyShareScheduler> periodic_contact_upload_scheduler_;
   std::unique_ptr<NearbyShareScheduler> contact_download_and_upload_scheduler_;
   std::unique_ptr<NearbyShareContactDownloader> contact_downloader_;
   mojo::RemoteSet<nearby_share::mojom::DownloadContactsObserver> observers_set_;

chrome/browser/nearby_sharing/contacts/nearby_share_contact_manager_impl_unittest.cc

@@ -38,6 +38,7 @@ const char kTestDefaultDeviceName[] = "Josh's Chromebook";
 const char kTestProfileUserName[] = "test@google.com";
 
 // From nearby_share_contact_manager_impl.cc.
+constexpr base::TimeDelta kContactUploadPeriod = base::TimeDelta::FromHours(24);
 constexpr base::TimeDelta kContactDownloadPeriod =
     base::TimeDelta::FromHours(12);
 constexpr base::TimeDelta kContactDownloadRpcTimeout =
@@ -207,7 +208,7 @@ class NearbyShareContactManagerImplTest
       const std::vector<nearbyshare::proto::ContactRecord>& contacts,
       const std::set<std::string>& expected_allowed_contact_ids,
       bool expect_upload) {
-    TriggerScheduler();
+    TriggerDownloadScheduler();
 
     size_t num_handled_results =
         download_and_upload_scheduler()->handled_results().size();
@@ -243,7 +244,7 @@ class NearbyShareContactManagerImplTest
   }
 
   void FailDownload() {
-    TriggerScheduler();
+    TriggerDownloadScheduler();
 
     // Fail download and verify that the result is sent to the scheduler.
     size_t num_handled_results =
@@ -259,6 +260,11 @@ class NearbyShareContactManagerImplTest
     EXPECT_TRUE(mojo_observer_.on_contacts_download_failed_called_);
   }
 
+  void MakePeriodicUploadRequest() {
+    periodic_upload_scheduler()->InvokeRequestCallback();
+    periodic_upload_scheduler()->SetIsWaitingForResult(true);
+  }
+
   void FinishUpload(
       bool success,
       const std::vector<nearbyshare::proto::Contact>& expected_contacts) {
@@ -272,8 +278,10 @@ class NearbyShareContactManagerImplTest
 
     // Invoke upload callback from local device data manager.
     size_t num_upload_notifications = contacts_uploaded_notifications_.size();
-    size_t num_handled_results =
+    size_t num_download_and_upload_handled_results =
         download_and_upload_scheduler()->handled_results().size();
+    size_t num_periodic_upload_handeled_results =
+        periodic_upload_scheduler()->handled_results().size();
     std::move(call.callback).Run(success);
 
     // Verify upload notification was sent on success.
@@ -281,13 +289,24 @@ class NearbyShareContactManagerImplTest
               contacts_uploaded_notifications_.size());
     if (success) {
       // We only expect uploads to occur if contacts have changed since the last
-      // upload.
+      // upload or is a periodic upload was requested.
       EXPECT_TRUE(contacts_uploaded_notifications_.back()
-                      .did_contacts_change_since_last_upload);
+                      .did_contacts_change_since_last_upload ||
+                  periodic_upload_scheduler()->IsWaitingForResult());
+
+      if (periodic_upload_scheduler()->IsWaitingForResult()) {
+        EXPECT_EQ(num_periodic_upload_handeled_results + 1,
+                  periodic_upload_scheduler()->handled_results().size());
+        EXPECT_TRUE(periodic_upload_scheduler()->handled_results().back());
+        periodic_upload_scheduler()->SetIsWaitingForResult(false);
+      } else {
+        EXPECT_EQ(num_periodic_upload_handeled_results,
+                  periodic_upload_scheduler()->handled_results().size());
+      }
     }
 
     // Verify that result is sent to download/upload scheduler.
-    EXPECT_EQ(num_handled_results + 1,
+    EXPECT_EQ(num_download_and_upload_handled_results + 1,
               download_and_upload_scheduler()->handled_results().size());
     EXPECT_EQ(success,
               download_and_upload_scheduler()->handled_results().back());
@@ -330,14 +349,20 @@ class NearbyShareContactManagerImplTest
     return downloader_factory_.instances().back();
   }
 
+  FakeNearbyShareScheduler* periodic_upload_scheduler() {
+    return scheduler_factory_.pref_name_to_periodic_instance()
+        .at(prefs::kNearbySharingSchedulerPeriodicContactUploadPrefName)
+        .fake_scheduler;
+  }
+
   FakeNearbyShareScheduler* download_and_upload_scheduler() {
     return scheduler_factory_.pref_name_to_periodic_instance()
         .at(prefs::kNearbySharingSchedulerContactDownloadAndUploadPrefName)
         .fake_scheduler;
   }
 
+  // Verify scheduler input parameters.
   void VerifySchedulerInitialization() {
-    // Verify scheduler input parameters.
     FakeNearbyShareSchedulerFactory::PeriodicInstance
         download_and_upload_scheduler_instance =
             scheduler_factory_.pref_name_to_periodic_instance().at(
@@ -349,9 +374,20 @@ class NearbyShareContactManagerImplTest
     EXPECT_TRUE(download_and_upload_scheduler_instance.require_connectivity);
     EXPECT_EQ(&pref_service_,
               download_and_upload_scheduler_instance.pref_service);
+
+    FakeNearbyShareSchedulerFactory::PeriodicInstance
+        periodic_upload_scheduler_instance =
+            scheduler_factory_.pref_name_to_periodic_instance().at(
+                prefs::kNearbySharingSchedulerPeriodicContactUploadPrefName);
+    EXPECT_TRUE(periodic_upload_scheduler_instance.fake_scheduler);
+    EXPECT_EQ(kContactUploadPeriod,
+              periodic_upload_scheduler_instance.request_period);
+    EXPECT_FALSE(periodic_upload_scheduler_instance.retry_failures);
+    EXPECT_TRUE(periodic_upload_scheduler_instance.require_connectivity);
+    EXPECT_EQ(&pref_service_, periodic_upload_scheduler_instance.pref_service);
   }
 
-  void TriggerScheduler() {
+  void TriggerDownloadScheduler() {
     // Fire scheduler and verify downloader creation.
     size_t num_downloaders = downloader_factory_.instances().size();
     download_and_upload_scheduler()->InvokeRequestCallback();
@@ -452,7 +488,7 @@ TEST_F(NearbyShareContactManagerImplTest, SetAllowlist) {
                      /*expect_allowlist_changed=*/false);
 }
 
-TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_WithUpload) {
+TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_WithFirstUpload) {
   std::vector<nearbyshare::proto::ContactRecord> contact_records =
       TestContactRecordList(/*num_contacts=*/3u);
   std::set<std::string> allowlist = TestContactIds(/*num_contacts=*/2u);
@@ -518,6 +554,32 @@ TEST_F(NearbyShareContactManagerImplTest,
                    allowlist, contact_records));
 }
 
+TEST_F(NearbyShareContactManagerImplTest,
+       DownloadContacts_PeriodicUploadRequest) {
+  std::vector<nearbyshare::proto::ContactRecord> contact_records =
+      TestContactRecordList(/*num_contacts=*/3u);
+  std::set<std::string> allowlist = TestContactIds(/*num_contacts=*/2u);
+  SetAllowedContacts(allowlist, /*expect_allowlist_changed=*/true);
+
+  // Because contacts have never been uploaded, a subsequent upload is
+  // requested, which succeeds.
+  DownloadContacts();
+  SucceedDownload(contact_records, allowlist, /*expect_upload=*/true);
+  FinishUpload(/*success=*/true, /*expected_contacts=*/BuildContactListToUpload(
+                   allowlist, contact_records));
+
+  // Because device records on the server will be removed after a few days if
+  // the device does not contact the server, we ensure that contacts are
+  // uploaded periodically. Make that request now. Contacts will be uploaded
+  // after the next contact download. It will not force a download now, however.
+  MakePeriodicUploadRequest();
+
+  // When contacts are downloaded again, we decect that contacts have not
+  // changed. However, we expect an upload because a periodic request was made.
+  DownloadContacts();
+  SucceedDownload(contact_records, allowlist, /*expect_upload=*/true);
+}
+
 TEST_F(NearbyShareContactManagerImplTest, DownloadContacts_FailDownload) {
   DownloadContacts();
   FailDownload();