[Nearby] Do not upload device name to server

The Device proto |display_name| field is a relic of the past. It could
also be a privacy concern. Remove all use of it. The device name will
continue being included in the certificate's encrypted metadata.

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

chrome/browser/nearby_sharing/common/nearby_share_prefs.cc

@@ -34,8 +34,6 @@ const char kNearbySharingSchedulerContactUploadPrefName[] =
     "nearby_sharing.scheduler.contact_upload";
 const char kNearbySharingSchedulerDownloadDeviceDataPrefName[] =
     "nearby_sharing.scheduler.download_device_data";
-const char kNearbySharingSchedulerUploadDeviceNamePrefName[] =
-    "nearby_sharing.scheduler.upload_device_name";
 const char kNearbySharingPublicCertificateExpirationDictPrefName[] =
     "nearbyshare.public_certificate_expiration_dict";
 const char kNearbySharingPrivateCertificateListPrefName[] =
@@ -73,8 +71,6 @@ void RegisterNearbySharingPrefs(PrefRegistrySimple* registry) {
       prefs::kNearbySharingSchedulerContactUploadPrefName);
   registry->RegisterDictionaryPref(
       prefs::kNearbySharingSchedulerDownloadDeviceDataPrefName);
-  registry->RegisterDictionaryPref(
-      prefs::kNearbySharingSchedulerUploadDeviceNamePrefName);
   registry->RegisterTimePref(
       prefs::kNearbySharingOnboardingDismissedTimePrefName,
       /*default_value=*/base::Time());

chrome/browser/nearby_sharing/common/nearby_share_prefs.h

@@ -22,7 +22,6 @@ extern const char kNearbySharingOnboardingDismissedTimePrefName[];
 extern const char kNearbySharingSchedulerContactDownloadPrefName[];
 extern const char kNearbySharingSchedulerContactUploadPrefName[];
 extern const char kNearbySharingSchedulerDownloadDeviceDataPrefName[];
-extern const char kNearbySharingSchedulerUploadDeviceNamePrefName[];
 extern const char kNearbySharingPublicCertificateExpirationDictPrefName[];
 extern const char kNearbySharingPrivateCertificateListPrefName[];
 extern const char kNearbySharingSchedulerDownloadPublicCertificatesPrefName[];

chrome/browser/nearby_sharing/local_device_data/nearby_share_device_data_updater.cc

@@ -8,13 +8,11 @@
 #include "chrome/browser/nearby_sharing/local_device_data/nearby_share_device_data_updater.h"
 
 NearbyShareDeviceDataUpdater::Request::Request(
-    base::Optional<std::string> device_name,
     base::Optional<std::vector<nearbyshare::proto::Contact>> contacts,
     base::Optional<std::vector<nearbyshare::proto::PublicCertificate>>
         certificates,
     ResultCallback callback)
-    : device_name(std::move(device_name)),
-      contacts(std::move(contacts)),
+    : contacts(std::move(contacts)),
       certificates(std::move(certificates)),
       callback(std::move(callback)) {}
 
@@ -34,13 +32,12 @@ NearbyShareDeviceDataUpdater::NearbyShareDeviceDataUpdater(
 NearbyShareDeviceDataUpdater::~NearbyShareDeviceDataUpdater() = default;
 
 void NearbyShareDeviceDataUpdater::UpdateDeviceData(
-    base::Optional<std::string> device_name,
     base::Optional<std::vector<nearbyshare::proto::Contact>> contacts,
     base::Optional<std::vector<nearbyshare::proto::PublicCertificate>>
         certificates,
     ResultCallback callback) {
-  pending_requests_.emplace(std::move(device_name), std::move(contacts),
-                            std::move(certificates), std::move(callback));
+  pending_requests_.emplace(std::move(contacts), std::move(certificates),
+                            std::move(callback));
   ProcessRequestQueue();
 }
 

chrome/browser/nearby_sharing/local_device_data/nearby_share_device_data_updater.h

@@ -18,6 +18,11 @@
 // overriding HandleNextRequest(), which is invoked when the next request is
 // ready to be run. Implementations should call FinishAttempt() with the result
 // of the attempt and possibly the response.
+//
+// NOTE: We do *not* support upload of the device name. This field of the proto
+// appears to be a relic of the old Nearby Share model. Upload of the field
+// could be a privacy concern that we want to avoid.
+//
 // TODO(crbug.com/1105547): Instead of queuing requests, hold a single pending
 // request and update the fields as other UpdateDeviceData() call are made.
 // Then, queue up all of callbacks from the merged requests in the Request
@@ -31,8 +36,7 @@ class NearbyShareDeviceDataUpdater {
           response)>;
 
   struct Request {
-    Request(base::Optional<std::string> device_name,
-            base::Optional<std::vector<nearbyshare::proto::Contact>> contacts,
+    Request(base::Optional<std::vector<nearbyshare::proto::Contact>> contacts,
             base::Optional<std::vector<nearbyshare::proto::PublicCertificate>>
                 certificates,
             ResultCallback callback);
@@ -42,7 +46,6 @@ class NearbyShareDeviceDataUpdater {
     Request& operator=(const Request&) = delete;
     ~Request();
 
-    base::Optional<std::string> device_name;
     base::Optional<std::vector<nearbyshare::proto::Contact>> contacts;
     base::Optional<std::vector<nearbyshare::proto::PublicCertificate>>
         certificates;
@@ -58,8 +61,6 @@ class NearbyShareDeviceDataUpdater {
   // Queue up an UpdateDevice RPC request to update the following fields on the
   // Nearby server if the parameter is not base::nullopt:
   //
-  // |device_name|: The device display name, for example, "Joe's Pixel".
-  //
   // |contacts|: The list of contacts that the Nearby server will send
   //             all-contacts-visibility certificates to. Contacts marked
   //             is_selected will be sent selected-contacts-visibility
@@ -71,7 +72,6 @@ class NearbyShareDeviceDataUpdater {
   // If only the UpdateDevice RPC response data is desired, set all
   // aforementioned parameters to base::nullopt.
   void UpdateDeviceData(
-      base::Optional<std::string> device_name,
       base::Optional<std::vector<nearbyshare::proto::Contact>> contacts,
       base::Optional<std::vector<nearbyshare::proto::PublicCertificate>>
           certificates,

chrome/browser/nearby_sharing/local_device_data/nearby_share_device_data_updater_impl.cc

@@ -11,7 +11,6 @@
 namespace {
 
 const char kDeviceIdPrefix[] = "users/me/devices/";
-const char kDeviceNameFieldMaskPath[] = "device.display_name";
 const char kContactsFieldMaskPath[] = "device.contacts";
 const char kCertificatesFieldMaskPath[] = "device.public_certificates";
 
@@ -63,11 +62,6 @@ void NearbyShareDeviceDataUpdaterImpl::HandleNextRequest() {
 
   nearbyshare::proto::UpdateDeviceRequest request;
   request.mutable_device()->set_name(kDeviceIdPrefix + device_id_);
-  if (pending_requests_.front().device_name) {
-    request.mutable_device()->set_display_name(
-        *pending_requests_.front().device_name);
-    request.mutable_update_mask()->add_paths(kDeviceNameFieldMaskPath);
-  }
   if (pending_requests_.front().contacts) {
     *request.mutable_device()->mutable_contacts() = {
         pending_requests_.front().contacts->begin(),

chrome/browser/nearby_sharing/local_device_data/nearby_share_device_data_updater_impl_unittest.cc

@@ -23,7 +23,6 @@ namespace {
 
 const char kDeviceIdPrefix[] = "users/me/devices/";
 const char kTestDeviceId[] = "test_device_id";
-const char kTestDeviceName[] = "test_device_name";
 const char kTestContactEmail1[] = "test1@gmail.com";
 const char kTestContactEmail2[] = "test2@gmail.com";
 const char kTestCertificateId1[] = "cert_id_1";
@@ -68,7 +67,6 @@ const nearbyshare::proto::UpdateDeviceResponse& TestResponse() {
 }
 
 void VerifyRequest(
-    const base::Optional<std::string>& expected_device_name,
     const base::Optional<std::vector<nearbyshare::proto::Contact>>&
         expected_contacts,
     const base::Optional<std::vector<nearbyshare::proto::PublicCertificate>>&
@@ -77,12 +75,10 @@ void VerifyRequest(
   std::vector<std::string> field_mask{request.update_mask().paths().begin(),
                                       request.update_mask().paths().end()};
 
-  EXPECT_EQ(static_cast<size_t>(expected_device_name.has_value()) +
-                static_cast<size_t>(expected_contacts.has_value()) +
+  EXPECT_EQ(static_cast<size_t>(expected_contacts.has_value()) +
                 static_cast<size_t>(expected_certificates.has_value()),
             field_mask.size());
-  EXPECT_EQ(expected_device_name.has_value(),
-            base::Contains(field_mask, "device.display_name"));
+
   EXPECT_EQ(expected_contacts.has_value(),
             base::Contains(field_mask, "device.contacts"));
   EXPECT_EQ(expected_certificates.has_value(),
@@ -91,9 +87,6 @@ void VerifyRequest(
   EXPECT_EQ(std::string(kDeviceIdPrefix) + kTestDeviceId,
             request.device().name());
 
-  EXPECT_EQ(expected_device_name.value_or(std::string()),
-            request.device().display_name());
-
   if (expected_contacts) {
     ASSERT_EQ(static_cast<int>(expected_contacts->size()),
               request.device().contacts().size());
@@ -145,18 +138,16 @@ class NearbyShareDeviceDataUpdaterImplTest : public ::testing::Test {
   }
 
   void CallUpdateDeviceData(
-      const base::Optional<std::string>& device_name,
       const base::Optional<std::vector<nearbyshare::proto::Contact>>& contacts,
       const base::Optional<std::vector<nearbyshare::proto::PublicCertificate>>&
           certificates) {
     updater_->UpdateDeviceData(
-        device_name, contacts, certificates,
+        contacts, certificates,
         base::BindOnce(&NearbyShareDeviceDataUpdaterImplTest::OnResult,
                        base::Unretained(this)));
   }
 
   void ProcessNextUpdateDeviceDataRequest(
-      const base::Optional<std::string>& expected_device_name,
       const base::Optional<std::vector<nearbyshare::proto::Contact>>&
           expected_contacts,
       const base::Optional<std::vector<nearbyshare::proto::PublicCertificate>>&
@@ -166,8 +157,7 @@ class NearbyShareDeviceDataUpdaterImplTest : public ::testing::Test {
     ASSERT_FALSE(fake_client_factory_.instances().empty());
     FakeNearbyShareClient* client = fake_client_factory_.instances().back();
     ASSERT_EQ(1u, client->update_device_requests().size());
-    VerifyRequest(expected_device_name, expected_contacts,
-                  expected_certificates,
+    VerifyRequest(expected_contacts, expected_certificates,
                   client->update_device_requests()[0].request);
 
     // Send and verify the response.
@@ -214,72 +204,55 @@ class NearbyShareDeviceDataUpdaterImplTest : public ::testing::Test {
 };
 
 TEST_F(NearbyShareDeviceDataUpdaterImplTest, Success_NoParameters) {
-  CallUpdateDeviceData(/*device_name=*/base::nullopt,
-                       /*contacts=*/base::nullopt,
+  CallUpdateDeviceData(/*contacts=*/base::nullopt,
                        /*certificates=*/base::nullopt);
   ProcessNextUpdateDeviceDataRequest(
-      /*expected_device_name=*/base::nullopt,
       /*expected_contacts=*/base::nullopt,
       /*expected_certificates=*/base::nullopt,
       UpdateDeviceRequestResult::kSuccess);
 }
 
 TEST_F(NearbyShareDeviceDataUpdaterImplTest, Success_AllParameters) {
-  CallUpdateDeviceData(kTestDeviceName, TestContactList(),
-                       TestCertificateList());
-  ProcessNextUpdateDeviceDataRequest(kTestDeviceName, TestContactList(),
-                                     TestCertificateList(),
+  CallUpdateDeviceData(TestContactList(), TestCertificateList());
+  ProcessNextUpdateDeviceDataRequest(TestContactList(), TestCertificateList(),
                                      UpdateDeviceRequestResult::kSuccess);
 }
 
 TEST_F(NearbyShareDeviceDataUpdaterImplTest, Success_OneParameter) {
-  CallUpdateDeviceData(kTestDeviceName,
-                       /*contacts=*/base::nullopt,
+  CallUpdateDeviceData(TestContactList(),
                        /*certificates=*/base::nullopt);
-  ProcessNextUpdateDeviceDataRequest(kTestDeviceName,
-                                     /*expected_contacts=*/base::nullopt,
+  ProcessNextUpdateDeviceDataRequest(TestContactList(),
                                      /*expected_certificates=*/base::nullopt,
                                      UpdateDeviceRequestResult::kSuccess);
 }
 
 TEST_F(NearbyShareDeviceDataUpdaterImplTest, Failure_Timeout) {
-  CallUpdateDeviceData(kTestDeviceName, TestContactList(),
-                       TestCertificateList());
-  ProcessNextUpdateDeviceDataRequest(kTestDeviceName, TestContactList(),
-                                     TestCertificateList(),
+  CallUpdateDeviceData(TestContactList(), TestCertificateList());
+  ProcessNextUpdateDeviceDataRequest(TestContactList(), TestCertificateList(),
                                      UpdateDeviceRequestResult::kTimeout);
 }
 
 TEST_F(NearbyShareDeviceDataUpdaterImplTest, Failure_HttpError) {
-  CallUpdateDeviceData(kTestDeviceName, TestContactList(),
-                       TestCertificateList());
-  ProcessNextUpdateDeviceDataRequest(kTestDeviceName, TestContactList(),
-                                     TestCertificateList(),
+  CallUpdateDeviceData(TestContactList(), TestCertificateList());
+  ProcessNextUpdateDeviceDataRequest(TestContactList(), TestCertificateList(),
                                      UpdateDeviceRequestResult::kHttpFailure);
 }
 
 TEST_F(NearbyShareDeviceDataUpdaterImplTest, QueuedRequests) {
   // Queue requests while waiting to process.
-  CallUpdateDeviceData(/*device_name=*/base::nullopt,
-                       /*contacts=*/base::nullopt,
-                       /*certificates=*/base::nullopt);
-  CallUpdateDeviceData(kTestDeviceName, TestContactList(),
-                       TestCertificateList());
-  CallUpdateDeviceData(kTestDeviceName,
-                       /*contacts=*/base::nullopt,
+  CallUpdateDeviceData(/*contacts=*/base::nullopt,
                        /*certificates=*/base::nullopt);
+  CallUpdateDeviceData(TestContactList(), TestCertificateList());
+  CallUpdateDeviceData(/*contacts=*/base::nullopt, TestCertificateList());
 
   // Requests are processed in the order they are received.
   ProcessNextUpdateDeviceDataRequest(
-      /*expected_device_name=*/base::nullopt,
       /*expected_contacts=*/base::nullopt,
       /*expected_certificates=*/base::nullopt,
       UpdateDeviceRequestResult::kSuccess);
-  ProcessNextUpdateDeviceDataRequest(kTestDeviceName, TestContactList(),
-                                     TestCertificateList(),
+  ProcessNextUpdateDeviceDataRequest(TestContactList(), TestCertificateList(),
                                      UpdateDeviceRequestResult::kTimeout);
-  ProcessNextUpdateDeviceDataRequest(kTestDeviceName,
-                                     /*expected_contacts=*/base::nullopt,
-                                     /*expected_certificates=*/base::nullopt,
+  ProcessNextUpdateDeviceDataRequest(/*expected_contacts=*/base::nullopt,
+                                     TestCertificateList(),
                                      UpdateDeviceRequestResult::kHttpFailure);
 }

chrome/browser/nearby_sharing/local_device_data/nearby_share_local_device_data_manager.h

@@ -59,9 +59,10 @@ class NearbyShareLocalDeviceDataManager {
   // not yet been set from an UpdateDevice RPC response.
   virtual base::Optional<std::string> GetIconUrl() const = 0;
 
-  // Uses the UpdateDevice RPC to change the local device name in the Nearby
-  // Share server and in local storage. Must be UTF-8. Observers are notified
-  // via OnLocalDeviceDataChanged() if the device name changes.
+  // Sets and persists the device name in prefs. The device name is *not*
+  // uploaded to the Nearby Share server; the UpdateDevice proto device_name
+  // field in an artifact. Observers are notified via OnLocalDeviceDataChanged()
+  // if the device name changes.
   virtual void SetDeviceName(const std::string& name) = 0;
 
   // Makes an UpdateDevice RPC call to the Nearby Share server to retrieve all

chrome/browser/nearby_sharing/local_device_data/nearby_share_local_device_data_manager_impl.cc

@@ -81,15 +81,6 @@ NearbyShareLocalDeviceDataManagerImpl::NearbyShareLocalDeviceDataManagerImpl(
               pref_service_,
               base::BindRepeating(&NearbyShareLocalDeviceDataManagerImpl::
                                       OnDownloadDeviceDataRequested,
-                                  base::Unretained(this)))),
-      upload_device_name_scheduler_(
-          NearbyShareSchedulerFactory::CreateOnDemandScheduler(
-              /*retry_failures=*/true,
-              /*require_connectivity=*/true,
-              prefs::kNearbySharingSchedulerUploadDeviceNamePrefName,
-              pref_service_,
-              base::BindRepeating(&NearbyShareLocalDeviceDataManagerImpl::
-                                      OnUploadDeviceNameRequested,
                                   base::Unretained(this)))) {}
 
 NearbyShareLocalDeviceDataManagerImpl::
@@ -132,7 +123,6 @@ void NearbyShareLocalDeviceDataManagerImpl::SetDeviceName(
 
   // TODO(b/161297140): Perform input validation.
   SetStringPref(prefs::kNearbySharingDeviceNamePrefName, name);
-  upload_device_name_scheduler_->MakeImmediateRequest();
 
   NotifyLocalDeviceDataChanged(/*did_device_name_change=*/true,
                                /*did_full_name_change=*/false,
@@ -147,7 +137,7 @@ void NearbyShareLocalDeviceDataManagerImpl::UploadContacts(
     std::vector<nearbyshare::proto::Contact> contacts,
     UploadCompleteCallback callback) {
   device_data_updater_->UpdateDeviceData(
-      /*device_name=*/base::nullopt, std::move(contacts),
+      std::move(contacts),
       /*certificates=*/base::nullopt,
       base::BindOnce(
           &NearbyShareLocalDeviceDataManagerImpl::OnUploadContactsFinished,
@@ -158,7 +148,6 @@ void NearbyShareLocalDeviceDataManagerImpl::UploadCertificates(
     std::vector<nearbyshare::proto::PublicCertificate> certificates,
     UploadCompleteCallback callback) {
   device_data_updater_->UpdateDeviceData(
-      /*device_name=*/base::nullopt,
       /*contacts=*/base::nullopt, std::move(certificates),
       base::BindOnce(
           &NearbyShareLocalDeviceDataManagerImpl::OnUploadCertificatesFinished,
@@ -169,13 +158,10 @@ void NearbyShareLocalDeviceDataManagerImpl::OnStart() {
   // This schedules an immediate download of the full name and icon URL from the
   // server if that has never happened before.
   download_device_data_scheduler_->Start();
-
-  upload_device_name_scheduler_->Start();
 }
 
 void NearbyShareLocalDeviceDataManagerImpl::OnStop() {
   download_device_data_scheduler_->Stop();
-  upload_device_name_scheduler_->Stop();
 }
 
 base::Optional<std::string>
@@ -199,24 +185,13 @@ void NearbyShareLocalDeviceDataManagerImpl::SetStringPref(
 
 void NearbyShareLocalDeviceDataManagerImpl::OnDownloadDeviceDataRequested() {
   device_data_updater_->UpdateDeviceData(
-      /*device_name=*/base::nullopt,
       /*contacts=*/base::nullopt,
       /*certificates=*/base::nullopt,
       base::BindOnce(
           &NearbyShareLocalDeviceDataManagerImpl::OnDownloadDeviceDataFinished,
           base::Unretained(this)));
 }
 
-void NearbyShareLocalDeviceDataManagerImpl::OnUploadDeviceNameRequested() {
-  device_data_updater_->UpdateDeviceData(
-      GetDeviceName(),
-      /*contacts=*/base::nullopt,
-      /*certificates=*/base::nullopt,
-      base::BindOnce(
-          &NearbyShareLocalDeviceDataManagerImpl::OnUploadDeviceNameFinished,
-          base::Unretained(this)));
-}
-
 void NearbyShareLocalDeviceDataManagerImpl::OnDownloadDeviceDataFinished(
     const base::Optional<nearbyshare::proto::UpdateDeviceResponse>& response) {
   if (response)
@@ -226,15 +201,6 @@ void NearbyShareLocalDeviceDataManagerImpl::OnDownloadDeviceDataFinished(
       /*success=*/response.has_value());
 }
 
-void NearbyShareLocalDeviceDataManagerImpl::OnUploadDeviceNameFinished(
-    const base::Optional<nearbyshare::proto::UpdateDeviceResponse>& response) {
-  if (response)
-    HandleUpdateDeviceResponse(response);
-
-  upload_device_name_scheduler_->HandleResult(
-      /*success=*/response.has_value());
-}
-
 void NearbyShareLocalDeviceDataManagerImpl::OnUploadContactsFinished(
     UploadCompleteCallback callback,
     const base::Optional<nearbyshare::proto::UpdateDeviceResponse>& response) {