[CrOS MultiDevice] Adjust RemoteDevice Mojo struct for setup flow.
As part of the MultiDeviceSetup flow, we use JS <==> C++ Mojo bindings,
so we were transferring RemoteDevice objects from C++ to JS.
RemoteDevice objects hold a "public_key" field, which is a std::string
containing randomly-generated bits. These bits are not all necessarily
ASCII characters and often contain NULL characters before the end of the
string. The std::string class handles this situation, but JS strings do
not. Thus, transferring the public_key field was problematic because the
key would not be received correctly on the JS side.
This CL modifies device_sync.mojom's RemoteDevice struct:
* The device ID (human-readable string) is included instead of the
public key.
* StructTraits use RemoteDeviceRef::DerivePublicKey() to retrieve the
public key on the other side of the pipe.
Additionally, this CL modifies multidevice_setup.mojom such that
SetHostDevice() now takes a device ID instead of a public key as a
parameter.
Bug: 824568
Change-Id: I16bda595ce25ba9321f11d3f455fbbeec71d7809
Reviewed-on: https://chromium-review.googlesource.com/1149328
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578392}diff --git a/chromeos/services/device_sync/public/mojom/device_sync.mojom b/chromeos/services/device_sync/public/mojom/device_sync.mojom
index 19090b09..06f1e3f 100644
--- a/chromeos/services/device_sync/public/mojom/device_sync.mojom
+++ b/chromeos/services/device_sync/public/mojom/device_sync.mojom
@@ -86,8 +86,10 @@
// Metadata describing a remote device with which the current device can
// communicate.
struct RemoteDevice {
- // Public key used to authenticate a communication channel.
- string public_key;
+ // Unique identifier of the device. Unlike |public_key|, this field is
+ // guaranteed to be human-readable (i.e., it does not contain non-ASCII
+ // characters).
+ string device_id;
// Identifier for the user to whom this device is registered.
string user_id;
diff --git a/chromeos/services/device_sync/public/mojom/device_sync_mojom_traits.cc b/chromeos/services/device_sync/public/mojom/device_sync_mojom_traits.cc
index 167b754..f31d92e 100644
--- a/chromeos/services/device_sync/public/mojom/device_sync_mojom_traits.cc
+++ b/chromeos/services/device_sync/public/mojom/device_sync_mojom_traits.cc
@@ -5,6 +5,7 @@
#include "chromeos/services/device_sync/public/mojom/device_sync_mojom_traits.h"
#include "base/logging.h"
+#include "components/cryptauth/remote_device_ref.h"
#include "mojo/public/cpp/base/time_mojom_traits.h"
namespace mojo {
@@ -48,11 +49,11 @@
return true;
}
-const std::string&
+std::string
StructTraits<chromeos::device_sync::mojom::RemoteDeviceDataView,
- cryptauth::RemoteDevice>::public_key(const cryptauth::RemoteDevice&
- remote_device) {
- return remote_device.public_key;
+ cryptauth::RemoteDevice>::device_id(const cryptauth::RemoteDevice&
+ remote_device) {
+ return remote_device.GetDeviceId();
}
const std::string&
@@ -100,10 +101,11 @@
cryptauth::RemoteDevice>::
Read(chromeos::device_sync::mojom::RemoteDeviceDataView in,
cryptauth::RemoteDevice* out) {
+ std::string device_id;
base::Time last_update_time;
if (!in.ReadUserId(&out->user_id) || !in.ReadDeviceName(&out->name) ||
- !in.ReadPublicKey(&out->public_key) ||
+ !in.ReadDeviceId(&device_id) ||
!in.ReadPersistentSymmetricKey(&out->persistent_symmetric_key) ||
!in.ReadLastUpdateTime(&last_update_time) ||
!in.ReadSoftwareFeatures(&out->software_features) ||
@@ -111,6 +113,7 @@
return false;
}
+ out->public_key = cryptauth::RemoteDeviceRef::DerivePublicKey(device_id);
out->last_update_time_millis = last_update_time.ToJavaTime();
return true;
diff --git a/chromeos/services/device_sync/public/mojom/device_sync_mojom_traits.h b/chromeos/services/device_sync/public/mojom/device_sync_mojom_traits.h
index d65e1c6..a20eb1d 100644
--- a/chromeos/services/device_sync/public/mojom/device_sync_mojom_traits.h
+++ b/chromeos/services/device_sync/public/mojom/device_sync_mojom_traits.h
@@ -33,8 +33,7 @@
class StructTraits<chromeos::device_sync::mojom::RemoteDeviceDataView,
cryptauth::RemoteDevice> {
public:
- static const std::string& public_key(
- const cryptauth::RemoteDevice& remote_device);
+ static std::string device_id(const cryptauth::RemoteDevice& remote_device);
static const std::string& user_id(
const cryptauth::RemoteDevice& remote_device);
static const std::string& device_name(
diff --git a/chromeos/services/multidevice_setup/multidevice_setup_impl.cc b/chromeos/services/multidevice_setup/multidevice_setup_impl.cc
index 661497d4..9bb2e4c 100644
--- a/chromeos/services/multidevice_setup/multidevice_setup_impl.cc
+++ b/chromeos/services/multidevice_setup/multidevice_setup_impl.cc
@@ -109,14 +109,14 @@
std::move(callback).Run(eligible_remote_devices);
}
-void MultiDeviceSetupImpl::SetHostDevice(const std::string& host_public_key,
+void MultiDeviceSetupImpl::SetHostDevice(const std::string& host_device_id,
SetHostDeviceCallback callback) {
cryptauth::RemoteDeviceRefList eligible_devices =
eligible_host_devices_provider_->GetEligibleHostDevices();
auto it =
std::find_if(eligible_devices.begin(), eligible_devices.end(),
- [&host_public_key](const auto& eligible_device) {
- return eligible_device.public_key() == host_public_key;
+ [&host_device_id](const auto& eligible_device) {
+ return eligible_device.GetDeviceId() == host_device_id;
});
if (it == eligible_devices.end()) {
diff --git a/chromeos/services/multidevice_setup/multidevice_setup_impl.h b/chromeos/services/multidevice_setup/multidevice_setup_impl.h
index 909be39..d5ab44c 100644
--- a/chromeos/services/multidevice_setup/multidevice_setup_impl.h
+++ b/chromeos/services/multidevice_setup/multidevice_setup_impl.h
@@ -66,7 +66,7 @@
mojom::AccountStatusChangeDelegatePtr delegate) override;
void AddHostStatusObserver(mojom::HostStatusObserverPtr observer) override;
void GetEligibleHostDevices(GetEligibleHostDevicesCallback callback) override;
- void SetHostDevice(const std::string& host_public_key,
+ void SetHostDevice(const std::string& host_device_id,
SetHostDeviceCallback callback) override;
void RemoveHostDevice() override;
void GetHostStatus(GetHostStatusCallback callback) override;
diff --git a/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc b/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc
index 7cb0576e..a03ab74 100644
--- a/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc
+++ b/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc
@@ -399,10 +399,10 @@
return eligible_devices_list;
}
- bool CallSetHostDevice(const std::string& host_public_key) {
+ bool CallSetHostDevice(const std::string& host_device_id) {
base::RunLoop run_loop;
multidevice_setup_->SetHostDevice(
- host_public_key,
+ host_device_id,
base::BindOnce(&MultiDeviceSetupImplTest::OnHostSet,
base::Unretained(this), run_loop.QuitClosure()));
run_loop.Run();
@@ -650,11 +650,11 @@
EXPECT_FALSE(CallRetrySetHostNow());
// Set an invalid host as the host device; this should fail.
- EXPECT_FALSE(CallSetHostDevice("invalidHostPublicKey"));
+ EXPECT_FALSE(CallSetHostDevice("invalidHostDeviceId"));
EXPECT_FALSE(fake_host_backend_delegate()->HasPendingHostRequest());
// Set device 0 as the host; this should succeed.
- EXPECT_TRUE(CallSetHostDevice(test_devices()[0].public_key()));
+ EXPECT_TRUE(CallSetHostDevice(test_devices()[0].GetDeviceId()));
EXPECT_TRUE(fake_host_backend_delegate()->HasPendingHostRequest());
EXPECT_EQ(test_devices()[0],
fake_host_backend_delegate()->GetPendingHostRequest());
diff --git a/chromeos/services/multidevice_setup/multidevice_setup_initializer.cc b/chromeos/services/multidevice_setup/multidevice_setup_initializer.cc
index 5f57c55..77892e43 100644
--- a/chromeos/services/multidevice_setup/multidevice_setup_initializer.cc
+++ b/chromeos/services/multidevice_setup/multidevice_setup_initializer.cc
@@ -94,11 +94,10 @@
}
void MultiDeviceSetupInitializer::SetHostDevice(
- const std::string& host_public_key,
+ const std::string& host_device_id,
SetHostDeviceCallback callback) {
if (multidevice_setup_impl_) {
- multidevice_setup_impl_->SetHostDevice(host_public_key,
- std::move(callback));
+ multidevice_setup_impl_->SetHostDevice(host_device_id, std::move(callback));
return;
}
@@ -111,7 +110,7 @@
// If a pending request to remove the current device exists, cancel it.
pending_should_remove_host_device_ = false;
- pending_set_host_args_ = std::make_pair(host_public_key, std::move(callback));
+ pending_set_host_args_ = std::make_pair(host_device_id, std::move(callback));
}
void MultiDeviceSetupInitializer::RemoveHostDevice() {
diff --git a/chromeos/services/multidevice_setup/multidevice_setup_initializer.h b/chromeos/services/multidevice_setup/multidevice_setup_initializer.h
index 5440cb0b..51533d75 100644
--- a/chromeos/services/multidevice_setup/multidevice_setup_initializer.h
+++ b/chromeos/services/multidevice_setup/multidevice_setup_initializer.h
@@ -56,7 +56,7 @@
mojom::AccountStatusChangeDelegatePtr delegate) override;
void AddHostStatusObserver(mojom::HostStatusObserverPtr observer) override;
void GetEligibleHostDevices(GetEligibleHostDevicesCallback callback) override;
- void SetHostDevice(const std::string& host_public_key,
+ void SetHostDevice(const std::string& host_device_id,
SetHostDeviceCallback callback) override;
void RemoveHostDevice() override;
void GetHostStatus(GetHostStatusCallback callback) override;
diff --git a/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup.cc b/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup.cc
index 08ea447c..fce02d2 100644
--- a/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup.cc
+++ b/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup.cc
@@ -64,9 +64,9 @@
get_eligible_hosts_args_.push_back(std::move(callback));
}
-void FakeMultiDeviceSetup::SetHostDevice(const std::string& host_public_key,
+void FakeMultiDeviceSetup::SetHostDevice(const std::string& host_device_id,
SetHostDeviceCallback callback) {
- set_host_args_.emplace_back(host_public_key, std::move(callback));
+ set_host_args_.emplace_back(host_device_id, std::move(callback));
}
void FakeMultiDeviceSetup::RemoveHostDevice() {
diff --git a/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup.h b/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup.h
index 3bd52f3..8bd486b 100644
--- a/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup.h
+++ b/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup.h
@@ -56,7 +56,7 @@
mojom::AccountStatusChangeDelegatePtr delegate) override;
void AddHostStatusObserver(mojom::HostStatusObserverPtr observer) override;
void GetEligibleHostDevices(GetEligibleHostDevicesCallback callback) override;
- void SetHostDevice(const std::string& host_public_key,
+ void SetHostDevice(const std::string& host_device_id,
SetHostDeviceCallback callback) override;
void RemoveHostDevice() override;
void GetHostStatus(GetHostStatusCallback callback) override;
diff --git a/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.cc b/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.cc
index 43a1848..e15ddf4 100644
--- a/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.cc
+++ b/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.cc
@@ -12,7 +12,7 @@
FakeMultiDeviceSetupClient::~FakeMultiDeviceSetupClient() {
DCHECK(get_eligible_host_devices_callback_queue_.empty());
- DCHECK(set_host_device_public_key_and_callback_queue_.empty());
+ DCHECK(set_host_device_id_and_callback_queue_.empty());
DCHECK(get_host_status_callback_queue_.empty());
DCHECK(retry_set_host_now_callback_queue_.empty());
DCHECK(trigger_event_for_debugging_type_and_callback_queue_.empty());
@@ -26,13 +26,12 @@
}
void FakeMultiDeviceSetupClient::InvokePendingSetHostDeviceCallback(
- const std::string& expected_public_key,
+ const std::string& expected_device_id,
bool success) {
- DCHECK_EQ(expected_public_key,
- set_host_device_public_key_and_callback_queue_.front().first);
- std::move(set_host_device_public_key_and_callback_queue_.front().second)
- .Run(success);
- set_host_device_public_key_and_callback_queue_.pop();
+ DCHECK_EQ(expected_device_id,
+ set_host_device_id_and_callback_queue_.front().first);
+ std::move(set_host_device_id_and_callback_queue_.front().second).Run(success);
+ set_host_device_id_and_callback_queue_.pop();
}
void FakeMultiDeviceSetupClient::InvokePendingGetHostStatusCallback(
@@ -65,10 +64,10 @@
}
void FakeMultiDeviceSetupClient::SetHostDevice(
- const std::string& public_key,
+ const std::string& host_device_id,
mojom::MultiDeviceSetup::SetHostDeviceCallback callback) {
- set_host_device_public_key_and_callback_queue_.emplace(public_key,
- std::move(callback));
+ set_host_device_id_and_callback_queue_.emplace(host_device_id,
+ std::move(callback));
}
void FakeMultiDeviceSetupClient::RemoveHostDevice() {
diff --git a/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h b/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h
index 05a83b633..aa10cd27 100644
--- a/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h
+++ b/chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h
@@ -28,9 +28,8 @@
void InvokePendingGetEligibleHostDevicesCallback(
const cryptauth::RemoteDeviceRefList& eligible_devices);
- void InvokePendingSetHostDeviceCallback(
- const std::string& expected_public_key,
- bool success);
+ void InvokePendingSetHostDeviceCallback(const std::string& expected_device_id,
+ bool success);
void InvokePendingGetHostStatusCallback(
mojom::HostStatus host_status,
const base::Optional<cryptauth::RemoteDeviceRef>& host_device);
@@ -48,7 +47,7 @@
private:
void GetEligibleHostDevices(GetEligibleHostDevicesCallback callback) override;
void SetHostDevice(
- const std::string& public_key,
+ const std::string& host_device_id,
mojom::MultiDeviceSetup::SetHostDeviceCallback callback) override;
void RemoveHostDevice() override;
void GetHostStatus(GetHostStatusCallback callback) override;
@@ -65,7 +64,7 @@
get_eligible_host_devices_callback_queue_;
std::queue<
std::pair<std::string, mojom::MultiDeviceSetup::SetHostDeviceCallback>>
- set_host_device_public_key_and_callback_queue_;
+ set_host_device_id_and_callback_queue_;
std::queue<GetHostStatusCallback> get_host_status_callback_queue_;
std::queue<mojom::MultiDeviceSetup::RetrySetHostNowCallback>
retry_set_host_now_callback_queue_;
diff --git a/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h b/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h
index a5923ca..5ae6b2b 100644
--- a/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h
+++ b/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h
@@ -50,7 +50,7 @@
virtual void GetEligibleHostDevices(
GetEligibleHostDevicesCallback callback) = 0;
virtual void SetHostDevice(
- const std::string& public_key,
+ const std::string& host_device_id,
mojom::MultiDeviceSetup::SetHostDeviceCallback callback) = 0;
virtual void RemoveHostDevice() = 0;
virtual void GetHostStatus(GetHostStatusCallback callback) = 0;
diff --git a/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client_impl.cc b/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client_impl.cc
index 76bd4916..60ce25b 100644
--- a/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client_impl.cc
+++ b/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client_impl.cc
@@ -64,9 +64,9 @@
}
void MultiDeviceSetupClientImpl::SetHostDevice(
- const std::string& public_key,
+ const std::string& host_device_id,
mojom::MultiDeviceSetup::SetHostDeviceCallback callback) {
- multidevice_setup_ptr_->SetHostDevice(public_key, std::move(callback));
+ multidevice_setup_ptr_->SetHostDevice(host_device_id, std::move(callback));
}
void MultiDeviceSetupClientImpl::RemoveHostDevice() {
diff --git a/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client_impl.h b/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client_impl.h
index 4720263..f266f2b 100644
--- a/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client_impl.h
+++ b/chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client_impl.h
@@ -47,7 +47,7 @@
// MultiDeviceSetupClient:
void GetEligibleHostDevices(GetEligibleHostDevicesCallback callback) override;
void SetHostDevice(
- const std::string& public_key,
+ const std::string& host_device_id,
mojom::MultiDeviceSetup::SetHostDeviceCallback callback) override;
void RemoveHostDevice() override;
void GetHostStatus(GetHostStatusCallback callback) override;
