Skip to content

Commit

Permalink
[DeviceSync v2] Add NotifyDevices to DeviceSync client
Browse files Browse the repository at this point in the history 
Bug: 951969
Change-Id: I0d02f81aa340c6bb145d885f2be92587fc4248c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1929495
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#722253}
  • Loading branch information
@nohle
nohle authored and Commit Bot committed Dec 5, 2019
1 parent e498350 commit 72376ad
Show file tree
Hide file tree
Showing 8 changed files with 222 additions and 19 deletions.
49 changes: 41 additions & 8 deletions chromeos/components/multidevice/remote_device_test_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,55 @@
#include "chromeos/components/multidevice/remote_device_test_util.h"

#include "base/base64.h"
#include "base/base64url.h"
#include "base/strings/string_number_conversions.h"

namespace chromeos {

namespace multidevice {

namespace {

// Attributes of the default test remote device.
const char kTestRemoteDeviceUserId[] = "example@gmail.com";
const char kTestRemoteDeviceInstanceId[] = "instanceId";
const char kTestRemoteDeviceName[] = "remote device";
const int64_t kTestRemoteDeviceInstanceId = 0L;
const char kTestRemoteDevicePiiFreeName[] = "no-pii device";
const char kTestRemoteDevicePublicKey[] = "public key";
const char kTestRemoteDevicePSK[] = "remote device psk";
const int64_t kTestRemoteDeviceLastUpdateTimeMillis = 0L;

// Create an Instance ID, which is a base64 URL-safe encoding of an 8-byte
// integer. This seems like overkill for tests, but some places in code might
// require the specific Instance ID formatting.
std::string InstanceIdFromInt64(int64_t number) {
// Big-endian representation of |number|.
uint8_t bytes[sizeof(int64_t)];
bytes[0] = static_cast<uint8_t>((number >> 56) & 0xff);
bytes[1] = static_cast<uint8_t>((number >> 48) & 0xff);
bytes[2] = static_cast<uint8_t>((number >> 40) & 0xff);
bytes[3] = static_cast<uint8_t>((number >> 32) & 0xff);
bytes[4] = static_cast<uint8_t>((number >> 24) & 0xff);
bytes[5] = static_cast<uint8_t>((number >> 16) & 0xff);
bytes[6] = static_cast<uint8_t>((number >> 8) & 0xff);
bytes[7] = static_cast<uint8_t>(number & 0xff);

// Transforms the first 4 bits to 0x7 which is required for Instance IDs.
bytes[0] &= 0x0f;
bytes[0] |= 0x70;

std::string iid;
base::Base64UrlEncode(
base::StringPiece(reinterpret_cast<const char*>(bytes), sizeof(bytes)),
base::Base64UrlEncodePolicy::OMIT_PADDING, &iid);

return iid;
}

} // namespace

// Attributes of the default test remote device.
const char kTestRemoteDeviceName[] = "remote device";
const char kTestRemoteDevicePublicKey[] = "public key";

RemoteDeviceRefBuilder::RemoteDeviceRefBuilder() {
remote_device_ = std::make_shared<RemoteDevice>(CreateRemoteDeviceForTest());
}
Expand Down Expand Up @@ -97,7 +131,8 @@ RemoteDevice CreateRemoteDeviceForTest() {
software_features[SoftwareFeature::kInstantTetheringHost] =
SoftwareFeatureState::kSupported;

return RemoteDevice(kTestRemoteDeviceUserId, kTestRemoteDeviceInstanceId,
return RemoteDevice(kTestRemoteDeviceUserId,
InstanceIdFromInt64(kTestRemoteDeviceInstanceId),
kTestRemoteDeviceName, kTestRemoteDevicePiiFreeName,
kTestRemoteDevicePublicKey, kTestRemoteDevicePSK,
kTestRemoteDeviceLastUpdateTimeMillis, software_features,
Expand All @@ -114,8 +149,7 @@ RemoteDeviceRefList CreateRemoteDeviceRefListForTest(size_t num_to_create) {
for (size_t i = 0; i < num_to_create; i++) {
RemoteDeviceRef remote_device =
RemoteDeviceRefBuilder()
.SetInstanceId(kTestRemoteDeviceInstanceId +
base::NumberToString(i))
.SetInstanceId(InstanceIdFromInt64(i))
.SetPublicKey("publicKey" + base::NumberToString(i))
.Build();
generated_devices.push_back(remote_device);
Expand All @@ -129,8 +163,7 @@ RemoteDeviceList CreateRemoteDeviceListForTest(size_t num_to_create) {

for (size_t i = 0; i < num_to_create; i++) {
RemoteDevice remote_device = CreateRemoteDeviceForTest();
remote_device.instance_id =
kTestRemoteDeviceInstanceId + base::NumberToString(i);
remote_device.instance_id = InstanceIdFromInt64(i);
remote_device.public_key = "publicKey" + base::NumberToString(i);
generated_devices.push_back(remote_device);
}
Expand Down
7 changes: 7 additions & 0 deletions chromeos/services/device_sync/public/cpp/device_sync_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <memory>
#include <string>
#include <vector>

#include "base/callback.h"
#include "base/macros.h"
Expand All @@ -16,6 +17,7 @@
#include "chromeos/components/multidevice/remote_device_ref.h"
#include "chromeos/components/multidevice/software_feature.h"
#include "chromeos/services/device_sync/feature_status_change.h"
#include "chromeos/services/device_sync/proto/cryptauth_common.pb.h"
#include "chromeos/services/device_sync/public/mojom/device_sync.mojom.h"
#include "mojo/public/cpp/bindings/remote.h"

Expand Down Expand Up @@ -94,6 +96,11 @@ class DeviceSyncClient {
virtual void FindEligibleDevices(
multidevice::SoftwareFeature software_feature,
FindEligibleDevicesCallback callback) = 0;
virtual void NotifyDevices(
const std::vector<std::string>& device_instance_ids,
cryptauthv2::TargetService target_service,
multidevice::SoftwareFeature feature,
mojom::DeviceSync::NotifyDevicesCallback callback) = 0;
virtual void GetDevicesActivityStatus(
mojom::DeviceSync::GetDevicesActivityStatusCallback callback) = 0;
virtual void GetDebugInfo(
Expand Down
47 changes: 47 additions & 0 deletions chromeos/services/device_sync/public/cpp/device_sync_client_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "chromeos/services/device_sync/public/cpp/device_sync_client_impl.h"

#include "base/base64url.h"
#include "base/bind.h"
#include "base/no_destructor.h"
#include "chromeos/components/multidevice/expiring_remote_device_cache.h"
Expand All @@ -19,6 +20,32 @@ namespace chromeos {

namespace device_sync {

namespace {

bool IsValidInstanceId(const std::string& instance_id) {
if (instance_id.empty()) {
PA_LOG(ERROR) << "Instance ID cannot be empty.";
return false;
}

std::string decoded_iid;
if (!base::Base64UrlDecode(instance_id,
base::Base64UrlDecodePolicy::IGNORE_PADDING,
&decoded_iid)) {
PA_LOG(ERROR) << "Instance ID must be Base64Url encoded.";
return false;
}

if (decoded_iid.size() != 8u) {
PA_LOG(ERROR) << "Instance ID must be 8 bytes after Base64Url decoding.";
return false;
}

return true;
}

} // namespace

// static
DeviceSyncClientImpl::Factory* DeviceSyncClientImpl::Factory::test_factory_ =
nullptr;
Expand Down Expand Up @@ -121,6 +148,11 @@ void DeviceSyncClientImpl::SetFeatureStatus(
multidevice::SoftwareFeature feature,
FeatureStatusChange status_change,
mojom::DeviceSync::SetFeatureStatusCallback callback) {
if (!IsValidInstanceId(device_instance_id)) {
std::move(callback).Run(mojom::NetworkRequestResult::kBadRequest);
return;
}

device_sync_->SetFeatureStatus(device_instance_id, feature, status_change,
std::move(callback));
}
Expand All @@ -133,6 +165,21 @@ void DeviceSyncClientImpl::FindEligibleDevices(
base::BindOnce(&DeviceSyncClientImpl::OnFindEligibleDevicesCompleted,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
}
void DeviceSyncClientImpl::NotifyDevices(
const std::vector<std::string>& device_instance_ids,
cryptauthv2::TargetService target_service,
multidevice::SoftwareFeature feature,
mojom::DeviceSync::NotifyDevicesCallback callback) {
for (const std::string& iid : device_instance_ids) {
if (!IsValidInstanceId(iid)) {
std::move(callback).Run(mojom::NetworkRequestResult::kBadRequest);
return;
}
}

device_sync_->NotifyDevices(device_instance_ids, target_service, feature,
std::move(callback));
}

void DeviceSyncClientImpl::GetDevicesActivityStatus(
mojom::DeviceSync::GetDevicesActivityStatusCallback callback) {
Expand Down
6 changes: 6 additions & 0 deletions chromeos/services/device_sync/public/cpp/device_sync_client_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "chromeos/components/multidevice/remote_device_ref.h"
#include "chromeos/components/multidevice/software_feature.h"
#include "chromeos/services/device_sync/feature_status_change.h"
#include "chromeos/services/device_sync/proto/cryptauth_common.pb.h"
#include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
#include "chromeos/services/device_sync/public/mojom/device_sync.mojom.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
Expand Down Expand Up @@ -76,6 +77,11 @@ class DeviceSyncClientImpl : public DeviceSyncClient,
mojom::DeviceSync::SetFeatureStatusCallback callback) override;
void FindEligibleDevices(multidevice::SoftwareFeature software_feature,
FindEligibleDevicesCallback callback) override;
void NotifyDevices(
const std::vector<std::string>& device_instance_ids,
cryptauthv2::TargetService target_service,
multidevice::SoftwareFeature feature,
mojom::DeviceSync::NotifyDevicesCallback callback) override;
void GetDevicesActivityStatus(
mojom::DeviceSync::GetDevicesActivityStatusCallback callback) override;
void GetDebugInfo(mojom::DeviceSync::GetDebugInfoCallback callback) override;
Expand Down
93 changes: 86 additions & 7 deletions chromeos/services/device_sync/public/cpp/device_sync_client_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
#include "chromeos/services/device_sync/public/cpp/device_sync_client_impl.h"

#include <algorithm>
#include <string>
#include <tuple>
#include <utility>
#include <vector>

#include "base/bind.h"
#include "base/memory/scoped_refptr.h"
Expand All @@ -18,9 +20,11 @@
#include "base/test/task_environment.h"
#include "base/test/test_simple_task_runner.h"
#include "chromeos/components/multidevice/remote_device_test_util.h"
#include "chromeos/components/multidevice/software_feature.h"
#include "chromeos/services/device_sync/device_sync_impl.h"
#include "chromeos/services/device_sync/fake_device_sync.h"
#include "chromeos/services/device_sync/feature_status_change.h"
#include "chromeos/services/device_sync/proto/cryptauth_common.pb.h"
#include "chromeos/services/device_sync/public/cpp/fake_client_app_metadata_provider.h"
#include "chromeos/services/device_sync/public/cpp/fake_gcm_device_info_provider.h"
#include "chromeos/services/device_sync/public/mojom/device_sync.mojom.h"
Expand Down Expand Up @@ -346,20 +350,26 @@ class DeviceSyncClientImplTest : public testing::Test {
EXPECT_EQ(expected_result_code, set_software_feature_state_result_code_);
}

void CallSetFeatureStatus(mojom::NetworkRequestResult expected_result_code) {
void CallSetFeatureStatus(
mojom::NetworkRequestResult expected_result_code,
const base::Optional<std::string> invalid_instance_id = base::nullopt) {
base::RunLoop run_loop;

std::string instance_id = invalid_instance_id.value_or(
test_remote_device_ref_list_[0].instance_id());

client_->SetFeatureStatus(
test_remote_device_ref_list_[0].instance_id(),
multidevice::SoftwareFeature::kBetterTogetherHost,
instance_id, multidevice::SoftwareFeature::kBetterTogetherHost,
FeatureStatusChange::kEnableExclusively,
base::BindOnce(&DeviceSyncClientImplTest::OnSetFeatureStatusCompleted,
base::Unretained(this), run_loop.QuitClosure()));

SendPendingMojoMessages();
if (!invalid_instance_id) {
SendPendingMojoMessages();
fake_device_sync_->InvokePendingSetFeatureStatusCallback(
expected_result_code);
}

fake_device_sync_->InvokePendingSetFeatureStatusCallback(
expected_result_code);
run_loop.Run();

EXPECT_EQ(expected_result_code, set_feature_status_result_code_);
Expand Down Expand Up @@ -395,6 +405,33 @@ class DeviceSyncClientImplTest : public testing::Test {
expected_ineligible_devices);
}

void CallNotifyDevices(mojom::NetworkRequestResult expected_result_code,
const base::Optional<std::vector<std::string>>&
invalid_instance_ids = base::nullopt) {
base::RunLoop run_loop;

std::vector<std::string> instance_ids =
invalid_instance_ids.value_or(std::vector<std::string>(
{test_remote_device_ref_list_[0].instance_id(),
test_remote_device_ref_list_[1].instance_id()}));

client_->NotifyDevices(
instance_ids, cryptauthv2::TargetService::DEVICE_SYNC,
multidevice::SoftwareFeature::kBetterTogetherHost,
base::BindOnce(&DeviceSyncClientImplTest::OnNotifyDevicesCompleted,
base::Unretained(this), run_loop.QuitClosure()));

if (!invalid_instance_ids) {
SendPendingMojoMessages();
fake_device_sync_->InvokePendingNotifyDevicesCallback(
expected_result_code);
}

run_loop.Run();

EXPECT_EQ(expected_result_code, notify_devices_result_code_);
}

void CallGetDevicesActivityStatus(
mojom::NetworkRequestResult expected_result_code,
base::Optional<std::vector<mojom::DeviceActivityStatusPtr>>
Expand Down Expand Up @@ -503,6 +540,7 @@ class DeviceSyncClientImplTest : public testing::Test {
multidevice::RemoteDeviceRefList,
multidevice::RemoteDeviceRefList>
find_eligible_devices_error_code_and_response_;
base::Optional<mojom::NetworkRequestResult> notify_devices_result_code_;
std::tuple<mojom::NetworkRequestResult,
base::Optional<std::vector<mojom::DeviceActivityStatusPtr>>>
get_devices_activity_status_code_and_response_;
Expand Down Expand Up @@ -543,6 +581,12 @@ class DeviceSyncClientImplTest : public testing::Test {
std::move(callback).Run();
}

void OnNotifyDevicesCompleted(base::OnceClosure callback,
mojom::NetworkRequestResult result_code) {
notify_devices_result_code_ = result_code;
std::move(callback).Run();
}

void OnGetDevicesActivityStatus(
base::OnceClosure callback,
mojom::NetworkRequestResult result_code,
Expand Down Expand Up @@ -737,12 +781,29 @@ TEST_F(DeviceSyncClientImplTest, TestSetSoftwareFeatureState) {
CallSetSoftwareFeatureState(mojom::NetworkRequestResult::kSuccess);
}

TEST_F(DeviceSyncClientImplTest, TestSetFeatureStatus) {
TEST_F(DeviceSyncClientImplTest, TestSetFeatureStatus_Success) {
SetupClient();

CallSetFeatureStatus(mojom::NetworkRequestResult::kSuccess);
}

TEST_F(DeviceSyncClientImplTest, TestSetFeatureStatus_InvalidInstanceId) {
SetupClient();

// Instance ID cannot be empty.
CallSetFeatureStatus(mojom::NetworkRequestResult::kBadRequest,
std::string() /* invalid_instance_id */);

// Instance ID must be base64 encoded.
CallSetFeatureStatus(mojom::NetworkRequestResult::kBadRequest,
"$%^&*#" /* invalid_instance_id */);

// Instance ID must be 8 bytes after base64 decoding.
CallSetFeatureStatus(
mojom::NetworkRequestResult::kBadRequest,
"thisislongerthaneightbytes==" /* invalid_instance_id */);
}

TEST_F(DeviceSyncClientImplTest, TestFindEligibleDevices_NoErrorCode) {
SetupClient();

Expand All @@ -765,6 +826,24 @@ TEST_F(DeviceSyncClientImplTest, TestFindEligibleDevices_ErrorCode) {
multidevice::RemoteDeviceList());
}

TEST_F(DeviceSyncClientImplTest, TestNotifyDevices_Success) {
SetupClient();

CallNotifyDevices(mojom::NetworkRequestResult::kSuccess);
}

TEST_F(DeviceSyncClientImplTest, TestNotifyDevices_InvalidInstanceIds) {
SetupClient();

// Instance IDs cannot be empty, must be base64 encoded, and must be 8 bytes
// after base64 decoding.
CallNotifyDevices(
mojom::NetworkRequestResult::kBadRequest,
std::vector<std::string>{
std::string(), "$%^&*#",
"thisislongerthaneightbytes=="} /* invalid_instance_ids */);
}

TEST_F(DeviceSyncClientImplTest, TestGetDevicesActivityStatus_NoErrorCode) {
SetupClient();
std::vector<mojom::DeviceActivityStatusPtr> expected_activity_statuses;
Expand Down
Loading

0 comments on commit 72376ad

Please sign in to comment.