From 0222177e94769f1e6a67cfbe4415b7f1f5f5a423 Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Mon, 31 Mar 2025 15:43:43 -0700 Subject: [PATCH] Move device ID generation to login success. PiperOrigin-RevId: 742428028 --- .../nearby_share_certificate_manager_impl.cc | 8 +-- sharing/local_device_data/BUILD | 1 - ...by_share_local_device_data_manager_impl.cc | 49 +++++++------------ ...rby_share_local_device_data_manager_impl.h | 2 - ...are_local_device_data_manager_impl_test.cc | 49 +++++++++---------- sharing/nearby_sharing_service_impl.cc | 27 +++++++++- sharing/nearby_sharing_service_impl_test.cc | 14 ++++++ 7 files changed, 84 insertions(+), 66 deletions(-) diff --git a/sharing/certificates/nearby_share_certificate_manager_impl.cc b/sharing/certificates/nearby_share_certificate_manager_impl.cc index d6f38494..dfb0027c 100644 --- a/sharing/certificates/nearby_share_certificate_manager_impl.cc +++ b/sharing/certificates/nearby_share_certificate_manager_impl.cc @@ -39,7 +39,6 @@ #include "absl/synchronization/notification.h" #include "absl/time/time.h" #include "absl/types/span.h" -#include "internal/flags/nearby_flags.h" #include "internal/platform/implementation/account_manager.h" #include "proto/identity/v1/resources.pb.h" #include "proto/identity/v1/rpcs.pb.h" @@ -53,7 +52,6 @@ #include "sharing/certificates/nearby_share_private_certificate.h" #include "sharing/common/nearby_share_prefs.h" #include "sharing/contacts/nearby_share_contact_manager.h" -#include "sharing/flags/generated/nearby_sharing_feature_flags.h" #include "sharing/internal/api/bluetooth_adapter.h" #include "sharing/internal/api/preference_manager.h" #include "sharing/internal/api/public_certificate_database.h" @@ -424,7 +422,9 @@ void NearbyShareCertificateManagerImpl::DownloadPublicCertificates() { return; } - if (!account_manager_.GetCurrentAccount().has_value()) { + std::string device_id = local_device_data_manager_->GetId(); + if (!account_manager_.GetCurrentAccount().has_value() || + device_id.empty()) { LOG(WARNING) << "Ignore certificates download, no logged in account."; download_public_certificates_scheduler_->HandleResult(/*success=*/true); return; @@ -434,7 +434,7 @@ void NearbyShareCertificateManagerImpl::DownloadPublicCertificates() { // FetchNextPage() returns. auto context = std::make_unique( nearby_client_.get(), nearby_identity_client_.get(), - kDeviceIdPrefix + local_device_data_manager_->GetId(), + kDeviceIdPrefix + device_id, absl::bind_front(&NearbyShareCertificateManagerImpl:: OnPublicCertificatesDownloadFailure, this), diff --git a/sharing/local_device_data/BUILD b/sharing/local_device_data/BUILD index c599a203..5d59d3ab 100644 --- a/sharing/local_device_data/BUILD +++ b/sharing/local_device_data/BUILD @@ -42,7 +42,6 @@ cc_library( "//sharing/proto:share_cc_proto", "//util/hash:highway_fingerprint", "@com_google_absl//absl/algorithm", - "@com_google_absl//absl/functional:any_invocable", "@com_google_absl//absl/memory", "@com_google_absl//absl/random", "@com_google_absl//absl/status:statusor", diff --git a/sharing/local_device_data/nearby_share_local_device_data_manager_impl.cc b/sharing/local_device_data/nearby_share_local_device_data_manager_impl.cc index 35f34131..752e0b15 100644 --- a/sharing/local_device_data/nearby_share_local_device_data_manager_impl.cc +++ b/sharing/local_device_data/nearby_share_local_device_data_manager_impl.cc @@ -16,7 +16,6 @@ #include -#include #include #include #include @@ -27,7 +26,6 @@ #include "absl/algorithm/algorithm.h" #include "absl/memory/memory.h" -#include "absl/random/random.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" @@ -62,19 +60,6 @@ using ::nearby::sharing::api::SharingRpcClientFactory; using ::nearby::sharing::proto::UpdateDeviceRequest; using ::nearby::sharing::proto::UpdateDeviceResponse; -// Using the alphanumeric characters below, this provides 36^10 unique device -// IDs. Note that the uniqueness requirement is not global; the IDs are only -// used to differentiate between devices associated with a single GAIA account. -// This ID length agrees with the GmsCore implementation. -constexpr size_t kDeviceIdLength = 10; - -// Possible characters used in a randomly generated device ID. This agrees with -// the GmsCore implementation. -constexpr std::array kAlphaNumericChars = { - 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', - 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', - 'Y', 'Z', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9'}; - constexpr absl::string_view kDeviceIdPrefix = "users/me/devices/"; constexpr absl::string_view kContactsFieldMaskPath = "contacts"; constexpr absl::string_view kCertificatesFieldMaskPath = "public_certificates"; @@ -142,18 +127,7 @@ NearbyShareLocalDeviceDataManagerImpl:: ~NearbyShareLocalDeviceDataManagerImpl() = default; std::string NearbyShareLocalDeviceDataManagerImpl::GetId() { - std::string id = - preference_manager_.GetString(prefs::kNearbySharingDeviceIdName, ""); - if (!id.empty()) return id; - - absl::BitGen bitgen; - for (size_t i = 0; i < kDeviceIdLength; ++i) - id += kAlphaNumericChars[absl::Uniform( - bitgen, 0, static_cast(kAlphaNumericChars.size()))]; - - preference_manager_.SetString(prefs::kNearbySharingDeviceIdName, id); - - return id; + return preference_manager_.GetString(prefs::kNearbySharingDeviceIdName, ""); } std::string NearbyShareLocalDeviceDataManagerImpl::GetDeviceName() const { @@ -205,7 +179,9 @@ void NearbyShareLocalDeviceDataManagerImpl::UploadContacts( return; } - if (!account_manager_.GetCurrentAccount().has_value()) { + std::string device_id = GetId(); + if (!account_manager_.GetCurrentAccount().has_value() || + device_id.empty()) { LOG(WARNING) << __func__ << ": skip to upload contacts due " "to no login account."; @@ -215,7 +191,7 @@ void NearbyShareLocalDeviceDataManagerImpl::UploadContacts( UpdateDeviceRequest request; request.mutable_device()->set_name( - absl::StrCat(kDeviceIdPrefix, GetId())); + absl::StrCat(kDeviceIdPrefix, device_id)); request.mutable_device()->mutable_contacts()->Add(contacts.begin(), contacts.end()); request.mutable_update_mask()->add_paths( @@ -249,11 +225,18 @@ void NearbyShareLocalDeviceDataManagerImpl::PublishDevice( callback(/*success=*/false, /*contact_removed=*/false); return; } + std::string device_id = GetId(); + if (device_id.empty()) { + LOG(WARNING) << __func__ + << ": [Call Identity API] failed, device id is empty."; + callback(/*success=*/false, /*contact_removed=*/false); + return; + } LOG(INFO) << __func__ << ": [Call Identity API] Upload " << certificates.size() << " certificates."; PublishDeviceRequest request; - request.mutable_device()->set_name(absl::StrCat("devices/", GetId())); + request.mutable_device()->set_name(absl::StrCat("devices/", device_id)); LOG(INFO) << __func__ << ": [Call Identity API] PublishDeviceRequest with Device.name: " << request.device().name(); @@ -357,7 +340,9 @@ void NearbyShareLocalDeviceDataManagerImpl::UploadCertificates( return; } - if (!account_manager_.GetCurrentAccount().has_value()) { + std::string device_id = GetId(); + if (!account_manager_.GetCurrentAccount().has_value() || + device_id.empty()) { LOG(WARNING) << __func__ << ": skip to upload certificates due " "to no login account."; @@ -366,7 +351,7 @@ void NearbyShareLocalDeviceDataManagerImpl::UploadCertificates( } UpdateDeviceRequest request; request.mutable_device()->set_name( - absl::StrCat(kDeviceIdPrefix, GetId())); + absl::StrCat(kDeviceIdPrefix, device_id)); request.mutable_device()->mutable_public_certificates()->Add( certificates.begin(), certificates.end()); request.mutable_update_mask()->add_paths( diff --git a/sharing/local_device_data/nearby_share_local_device_data_manager_impl.h b/sharing/local_device_data/nearby_share_local_device_data_manager_impl.h index c6e0d2b6..9f1a30f6 100644 --- a/sharing/local_device_data/nearby_share_local_device_data_manager_impl.h +++ b/sharing/local_device_data/nearby_share_local_device_data_manager_impl.h @@ -16,7 +16,6 @@ #define THIRD_PARTY_NEARBY_SHARING_LOCAL_DEVICE_DATA_NEARBY_SHARE_LOCAL_DEVICE_DATA_MANAGER_IMPL_H_ #include -#include #include #include @@ -27,7 +26,6 @@ #include "sharing/common/nearby_share_enums.h" #include "sharing/internal/api/preference_manager.h" #include "sharing/internal/api/sharing_rpc_client.h" -#include "sharing/internal/impl/common/nearby_identity_grpc_client.h" #include "sharing/internal/public/context.h" #include "sharing/local_device_data/nearby_share_local_device_data_manager.h" #include "sharing/proto/rpc_resources.pb.h" diff --git a/sharing/local_device_data/nearby_share_local_device_data_manager_impl_test.cc b/sharing/local_device_data/nearby_share_local_device_data_manager_impl_test.cc index 58322b7a..51b9d423 100644 --- a/sharing/local_device_data/nearby_share_local_device_data_manager_impl_test.cc +++ b/sharing/local_device_data/nearby_share_local_device_data_manager_impl_test.cc @@ -16,7 +16,6 @@ #include -#include #include #include #include @@ -27,6 +26,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" #include "absl/strings/substitute.h" #include "absl/time/time.h" #include "internal/platform/implementation/account_manager.h" @@ -68,6 +68,7 @@ const char kFakeTooLongDeviceName[] = "this string is 33 bytes in UTF-8!"; const char kFakeTooLongGivenName[] = "this is a 33-byte string in utf-8"; constexpr char kTestAccountId[] = "test_account_id"; constexpr char kTestProfileUserName[] = "test@google.com"; +constexpr absl::string_view kTestDeviceId = "1234567890"; absl::StatusOr CreateResponse( const std::optional& full_name, @@ -174,6 +175,10 @@ class NearbyShareLocalDeviceDataManagerImplTest [&returned_success](bool success) { returned_success = success; }); Sync(); EXPECT_TRUE(client()->list_public_certificates_requests().empty()); + EXPECT_EQ(response.ok(), returned_success); + if (!response.ok()) { + return; + } std::vector expected_fake_contacts = GetFakeContacts(); for (size_t i = 0; i < expected_fake_contacts.size(); ++i) { EXPECT_EQ(expected_fake_contacts[i].SerializeAsString(), @@ -185,8 +190,6 @@ class NearbyShareLocalDeviceDataManagerImplTest .at(i) .SerializeAsString()); } - - EXPECT_EQ(response.ok(), returned_success); } void UploadCertificates( @@ -240,6 +243,10 @@ class NearbyShareLocalDeviceDataManagerImplTest return nearby_client_factory_.identity_instances().back(); } + void SetDeviceId(absl::string_view id) { + preference_manager_.SetString(prefs::kNearbySharingDeviceIdName, id); + } + void Sync() { EXPECT_TRUE(context_.last_sequenced_task_runner()->SyncWithTimeout( absl::Milliseconds(1000))); @@ -260,12 +267,11 @@ class NearbyShareLocalDeviceDataManagerImplTest TEST_F(NearbyShareLocalDeviceDataManagerImplTest, DeviceId) { CreateManager(); - + SetDeviceId(kTestDeviceId); // A 10-character alphanumeric ID is automatically generated if one doesn't // already exist. std::string id = manager()->GetId(); - EXPECT_EQ(id.size(), 10u); - for (const char c : id) EXPECT_TRUE(std::isalnum(c)); + EXPECT_EQ(id, kTestDeviceId); // The ID is persisted. DestroyManager(); @@ -349,22 +355,26 @@ TEST_F(NearbyShareLocalDeviceDataManagerImplTest, SetDeviceName) { TEST_F(NearbyShareLocalDeviceDataManagerImplTest, UploadContacts_Success) { CreateManager(); + SetDeviceId(kTestDeviceId); UploadContacts(CreateResponse(kFakeFullName, kFakeIconUrl, kFakeIconToken)); } TEST_F(NearbyShareLocalDeviceDataManagerImplTest, UploadContacts_Failure) { CreateManager(); + SetDeviceId(kTestDeviceId); UploadContacts(/*response=*/absl::InternalError("")); } TEST_F(NearbyShareLocalDeviceDataManagerImplTest, UploadCertificates_Success) { CreateManager(); + SetDeviceId(kTestDeviceId); UploadCertificates( CreateResponse(kFakeFullName, kFakeIconUrl, kFakeIconToken)); } TEST_F(NearbyShareLocalDeviceDataManagerImplTest, UploadCertificates_Failure) { CreateManager(); + SetDeviceId(kTestDeviceId); UploadCertificates(/*response=*/absl::InternalError("")); } @@ -412,6 +422,7 @@ std::vector GetTestCertificates() { TEST_F(NearbyShareLocalDeviceDataManagerImplTest, PublishDeviceInitialCall_ContactUpdateAdded) { CreateManager(); + SetDeviceId(kTestDeviceId); bool returned_success; bool returned_make_another_call; PublishDeviceResponse response; @@ -492,6 +503,7 @@ TEST_F(NearbyShareLocalDeviceDataManagerImplTest, TEST_F(NearbyShareLocalDeviceDataManagerImplTest, PublishDeviceInitialCall_ContactUpdateRemoved) { CreateManager(); + SetDeviceId(kTestDeviceId); bool returned_success; bool returned_make_another_call; PublishDeviceResponse response; @@ -518,6 +530,7 @@ TEST_F(NearbyShareLocalDeviceDataManagerImplTest, TEST_F(NearbyShareLocalDeviceDataManagerImplTest, PublishDeviceSecondCall_ContactUnchanged) { CreateManager(); + SetDeviceId(kTestDeviceId); bool returned_success; bool returned_make_another_call; PublishDeviceResponse response; @@ -544,6 +557,7 @@ TEST_F(NearbyShareLocalDeviceDataManagerImplTest, TEST_F(NearbyShareLocalDeviceDataManagerImplTest, PublishDevice_Failure) { CreateManager(); + SetDeviceId(kTestDeviceId); bool returned_success; bool returned_make_another_call; identity_client()->SetPublishDeviceResponse(absl::InternalError("")); @@ -560,7 +574,7 @@ TEST_F(NearbyShareLocalDeviceDataManagerImplTest, PublishDevice_Failure) { } TEST_F(NearbyShareLocalDeviceDataManagerImplTest, - PublishDevice_DeviceIdChangedAfterReset) { + PublishDevice_FailsWithEmptyDeviceId) { CreateManager(); bool returned_success; PublishDeviceResponse response; @@ -574,25 +588,8 @@ TEST_F(NearbyShareLocalDeviceDataManagerImplTest, }); Sync(); - EXPECT_TRUE(returned_success); - ASSERT_EQ(identity_client()->publish_device_requests().size(), 1); - std::string device_id = - identity_client()->publish_device_requests().back().device().name(); - // Reset device ID. - preference_manager_.SetString(prefs::kNearbySharingDeviceIdName, - std::string()); - manager()->PublishDevice( - GetTestCertificates(), /*is_second_call=*/true, - [&returned_success](bool success, bool make_another_call) { - returned_success = success; - }); - Sync(); - EXPECT_TRUE(returned_success); - ASSERT_EQ(identity_client()->publish_device_requests().size(), 2); - std::string new_device_id = - identity_client()->publish_device_requests().back().device().name(); - - EXPECT_NE(device_id, new_device_id); + EXPECT_FALSE(returned_success); + ASSERT_EQ(identity_client()->publish_device_requests().size(), 0); } } // namespace diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index 28344b04..2e2d874a 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -32,7 +32,6 @@ #include #include -#include "absl/algorithm/container.h" #include "absl/container/flat_hash_map.h" #include "absl/functional/any_invocable.h" #include "absl/functional/bind_front.h" @@ -150,6 +149,19 @@ constexpr absl::string_view kConnectionListenerName = "nearby-share-service"; constexpr absl::string_view kScreenStateListenerName = "nearby-share-service"; constexpr absl::string_view kProfileRelativePath = "Google/Nearby/Sharing"; +// Using the alphanumeric characters below, this provides 36^10 unique device +// IDs. Note that the uniqueness requirement is not global; the IDs are only +// used to differentiate between devices associated with a single GAIA account. +// This ID length agrees with the GmsCore implementation. +constexpr size_t kDeviceIdLength = 10; + +// Possible characters used in a randomly generated device ID. This agrees with +// the GmsCore implementation. +constexpr std::array kAlphaNumericChars = { + 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', + 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', + 'Y', 'Z', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9'}; + bool ShouldBlockSurfaceRegistration(BlockedVendorId registering_vendor_id, BlockedVendorId blocked_vendor_id) { return blocked_vendor_id != BlockedVendorId::kNone && @@ -188,6 +200,15 @@ OSType ToProtoOsType(::nearby::api::DeviceInfo::OsType os_type) { } } +std::string GenerateDeviceId() { + std::string id; + absl::BitGen bitgen; + for (size_t i = 0; i < kDeviceIdLength; ++i) + id += kAlphaNumericChars[absl::Uniform( + bitgen, 0, static_cast(kAlphaNumericChars.size()))]; + return id; +} + } // namespace NearbySharingServiceImpl::NearbySharingServiceImpl( @@ -3590,6 +3611,10 @@ void NearbySharingServiceImpl::ResetAllSettings(bool logout) { LOG(INFO) << "Clear public certificates. result: " << result; }); } else { + // on login generate a new device id + std::string device_id = GenerateDeviceId(); + preference_manager_.SetString(prefs::kNearbySharingDeviceIdName, device_id); + // should clear scheduled task to make it works immediately settings_->RemoveSettingsObserver(this); prefs::ResetSchedulers(preference_manager_); diff --git a/sharing/nearby_sharing_service_impl_test.cc b/sharing/nearby_sharing_service_impl_test.cc index 78b13acd..22738e05 100644 --- a/sharing/nearby_sharing_service_impl_test.cc +++ b/sharing/nearby_sharing_service_impl_test.cc @@ -17,6 +17,7 @@ #include #include +#include #include #include #include // NOLINT(build/c++17) @@ -4821,6 +4822,10 @@ TEST_F(NearbySharingServiceImplTest, LoginAndLogoutShouldResetSettings) { // Used to check whether the setting is cleared after login. service_->GetSettings()->SetIsAnalyticsEnabled(true); + std::string device_id = + preference_manager_.GetString(prefs::kNearbySharingDeviceIdName, ""); + EXPECT_TRUE(device_id.empty()); + // Create account. AccountManager::Account account; account.id = kTestAccountId; @@ -4830,11 +4835,17 @@ TEST_F(NearbySharingServiceImplTest, LoginAndLogoutShouldResetSettings) { std::unique_ptr signin_attempt = service_->GetAccountManager()->Login("test_client_id", "test_client_secret"); + account_manager().NotifyLogin(kTestAccountId); EXPECT_TRUE(sharing_service_task_runner_->SyncWithTimeout(kTaskWaitTimeout)); EXPECT_TRUE(service_->GetSettings()->GetIsAnalyticsEnabled()); ASSERT_TRUE(service_->GetAccountManager()->GetCurrentAccount().has_value()); EXPECT_EQ(service_->GetAccountManager()->GetCurrentAccount()->id, kTestAccountId); + device_id = + preference_manager_.GetString(prefs::kNearbySharingDeviceIdName, ""); + EXPECT_FALSE(device_id.empty()); + EXPECT_EQ(device_id.size(), 10u); + for (const char c : device_id) EXPECT_TRUE(std::isalnum(c)); // Logout user. absl::Notification logout_notification; @@ -4848,6 +4859,9 @@ TEST_F(NearbySharingServiceImplTest, LoginAndLogoutShouldResetSettings) { EXPECT_TRUE(service_->GetSettings()->GetIsAnalyticsEnabled()); EXPECT_FALSE(service_->GetAccountManager()->GetCurrentAccount().has_value()); EXPECT_TRUE(sharing_service_task_runner_->SyncWithTimeout(kTaskWaitTimeout)); + device_id = + preference_manager_.GetString(prefs::kNearbySharingDeviceIdName, ""); + EXPECT_TRUE(device_id.empty()); } TEST_F(NearbySharingServiceImplTest, LoginShouldSetContactsVisibility) {