From ece828ab5559f519550e35fd5d85ddfcdaf50318 Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Thu, 6 Jun 2024 18:37:00 -0700 Subject: [PATCH] Simplify PairKeyVerificationRunner interface. PiperOrigin-RevId: 641087956 --- sharing/BUILD | 29 +++----- sharing/nearby_sharing_service_impl.cc | 15 ++-- sharing/paired_key_verification_runner.cc | 47 ++++++------ sharing/paired_key_verification_runner.h | 23 +++--- .../paired_key_verification_runner_test.cc | 73 ++++++++----------- 5 files changed, 87 insertions(+), 100 deletions(-) diff --git a/sharing/BUILD b/sharing/BUILD index 7513e441..db0c40c2 100644 --- a/sharing/BUILD +++ b/sharing/BUILD @@ -287,14 +287,7 @@ cc_test( "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_absl//absl/types:span", "@com_google_googletest//:gtest_main", - ] + select({ - "@platforms//os:windows": [ - "//internal/platform/implementation/windows", - ], - "//conditions:default": [ - "//internal/platform/implementation/g3", - ], - }), + ], ) cc_test( @@ -304,14 +297,7 @@ cc_test( ":connection_types", "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_googletest//:gtest_main", - ] + select({ - "@platforms//os:windows": [ - "//internal/platform/implementation/windows", - ], - "//conditions:default": [ - "//internal/platform/implementation/g3", - ], - }), + ], ) cc_test( @@ -322,7 +308,7 @@ cc_test( ":nearby_sharing_service", ":test_support", ":types", - "//internal/platform/implementation/g3", # fixdeps: keep + "//internal/platform/implementation:types", "//internal/test", "//proto:sharing_enums_cc_proto", "//sharing/certificates", @@ -336,7 +322,14 @@ cc_test( "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_absl//absl/time", "@com_google_googletest//:gtest_main", - ], + ] + select({ + "@platforms//os:windows": [ + "//internal/platform/implementation/windows", + ], + "//conditions:default": [ + "//internal/platform/implementation/g3", + ], + }), ) cc_test( diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index df1e2f61..a46553a2 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -3362,13 +3362,14 @@ void NearbySharingServiceImpl::RunPairedKeyVerification( share_target_info->set_key_verification_runner( std::make_shared( - context_->GetClock(), device_info_, share_target_id, - share_target_info->IsIncoming(), settings_->GetVisibility(), - settings_->GetLastVisibility(), - settings_->GetLastVisibilityTimestamp(), *token, - share_target_info->connection(), share_target_info->certificate(), - GetCertificateManager(), share_target_info->frames_reader(), - kReadFramesTimeout)); + context_->GetClock(), device_info_.GetOsType(), + share_target_info->IsIncoming(), + PairedKeyVerificationRunner::VisibilityHistory{ + settings_->GetVisibility(), settings_->GetLastVisibility(), + settings_->GetLastVisibilityTimestamp()}, + *token, share_target_info->connection(), + share_target_info->certificate(), GetCertificateManager(), + share_target_info->frames_reader(), kReadFramesTimeout)); share_target_info->key_verification_runner()->Run(std::move(callback)); } diff --git a/sharing/paired_key_verification_runner.cc b/sharing/paired_key_verification_runner.cc index 0b017621..070d59af 100644 --- a/sharing/paired_key_verification_runner.cc +++ b/sharing/paired_key_verification_runner.cc @@ -26,10 +26,8 @@ #include #include -#include "absl/strings/string_view.h" #include "absl/time/time.h" #include "internal/platform/clock.h" -#include "internal/platform/device_info.h" #include "internal/platform/implementation/device_info.h" #include "proto/sharing_enums.pb.h" #include "sharing/certificates/common.h" @@ -114,16 +112,16 @@ std::ostream& operator<<( } PairedKeyVerificationRunner::PairedKeyVerificationRunner( - Clock* clock, DeviceInfo& device_info, int64_t share_target_id, - bool share_target_is_incoming, DeviceVisibility visibility, - DeviceVisibility last_visibility, absl::Time last_visibility_time, - const std::vector& token, NearbyConnection* connection, + Clock* clock, nearby::api::DeviceInfo::OsType os_type, + bool share_target_is_incoming, + const VisibilityHistory& visibility_history, + const std::vector& token, + NearbyConnection* connection, const std::optional& certificate, NearbyShareCertificateManager* certificate_manager, IncomingFramesReader* frames_reader, absl::Duration read_frame_timeout) : clock_(clock), - device_info_(device_info), - share_target_id_(share_target_id), + os_type_(os_type), raw_token_(token), connection_(connection), certificate_(certificate), @@ -138,16 +136,16 @@ PairedKeyVerificationRunner::PairedKeyVerificationRunner( if (share_target_is_incoming) { local_prefix_ = kNearbyShareReceiverVerificationPrefix; remote_prefix_ = kNearbyShareSenderVerificationPrefix; - visibility_ = visibility; - last_visibility_ = last_visibility; - last_visibility_time_ = last_visibility_time; + visibility_history_ = visibility_history; } else { remote_prefix_ = kNearbyShareReceiverVerificationPrefix; local_prefix_ = kNearbyShareSenderVerificationPrefix; // Sender always uses ALL_CONTACTS cert to sign and verify signature. - visibility_ = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS; - last_visibility_ = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS; - last_visibility_time_ = absl::UnixEpoch(); + visibility_history_ = { + .visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility_time = absl::UnixEpoch(), + }; } } @@ -184,12 +182,13 @@ void PairedKeyVerificationRunner::OnReadPairedKeyEncryptionFrame( } PairedKeyVerificationResult auth_token_hash_result = - VerifyAuthTokenHashWithPrivateCertificate(visibility_, *frame); + VerifyAuthTokenHashWithPrivateCertificate(visibility_history_.visibility, + *frame); if (auth_token_hash_result != PairedKeyVerificationResult::kSuccess) { if (IsVisibilityRecentlyUpdated()) { - auth_token_hash_result = - VerifyAuthTokenHashWithPrivateCertificate(last_visibility_, *frame); + auth_token_hash_result = VerifyAuthTokenHashWithPrivateCertificate( + visibility_history_.last_visibility, *frame); } } @@ -271,7 +270,7 @@ void PairedKeyVerificationRunner::SendPairedKeyResultFrame( } // Set OS type to allow remote device knowns the paring device OS type. - result_frame->set_os_type(ToProtoOsType(device_info_.GetOsType())); + result_frame->set_os_type(ToProtoOsType(os_type_)); std::vector data(frame.ByteSizeLong()); frame.SerializeToArray(data.data(), frame.ByteSizeLong()); @@ -281,8 +280,8 @@ void PairedKeyVerificationRunner::SendPairedKeyResultFrame( void PairedKeyVerificationRunner::SendPairedKeyEncryptionFrame() { std::optional> signature = - certificate_manager_->SignWithPrivateCertificate(visibility_, - PadPrefix(local_prefix_, raw_token_)); + certificate_manager_->SignWithPrivateCertificate( + visibility_history_.visibility, PadPrefix(local_prefix_, raw_token_)); if (!signature.has_value() || signature->empty()) { signature = GenerateRandomBytes(kNearbyShareNumBytesRandomSignature); } @@ -308,7 +307,8 @@ void PairedKeyVerificationRunner::SendPairedKeyEncryptionFrame() { << "Attempts to sign authentication token with a previous private key."; std::optional> optional_signature = certificate_manager_->SignWithPrivateCertificate( - last_visibility_, PadPrefix(local_prefix_, raw_token_)); + visibility_history_.last_visibility, + PadPrefix(local_prefix_, raw_token_)); if (optional_signature.has_value()) { encryption_frame->set_optional_signed_data(optional_signature->data(), @@ -413,8 +413,9 @@ void PairedKeyVerificationRunner::ApplyResult( } bool PairedKeyVerificationRunner::IsVisibilityRecentlyUpdated() const { - return visibility_ != last_visibility_ && - (clock_->Now() - last_visibility_time_ < + return visibility_history_.visibility != + visibility_history_.last_visibility && + (clock_->Now() - visibility_history_.last_visibility_time < kRelaxAfterSetVisibilityTimeout); } diff --git a/sharing/paired_key_verification_runner.h b/sharing/paired_key_verification_runner.h index 981c266a..80c203dc 100644 --- a/sharing/paired_key_verification_runner.h +++ b/sharing/paired_key_verification_runner.h @@ -20,13 +20,11 @@ #include #include #include -#include #include -#include "absl/strings/string_view.h" #include "absl/time/time.h" #include "internal/platform/clock.h" -#include "internal/platform/device_info.h" +#include "internal/platform/implementation/device_info.h" #include "proto/sharing_enums.pb.h" #include "sharing/certificates/nearby_share_certificate_manager.h" #include "sharing/certificates/nearby_share_decrypted_public_certificate.h" @@ -52,10 +50,16 @@ class PairedKeyVerificationRunner kUnable, }; + struct VisibilityHistory { + proto::DeviceVisibility visibility; + proto::DeviceVisibility last_visibility; + absl::Time last_visibility_time; + }; + PairedKeyVerificationRunner( - Clock* clock, DeviceInfo& device_info, int64_t share_target_id, - bool share_target_is_incoming, proto::DeviceVisibility visibility, - proto::DeviceVisibility last_visibility, absl::Time last_visibility_time, + Clock* clock, nearby::api::DeviceInfo::OsType os_type, + bool share_target_is_incoming, + const VisibilityHistory& visibility_history, const std::vector& token, NearbyConnection* connection, const std::optional& certificate, NearbyShareCertificateManager* certificate_manager, @@ -91,11 +95,8 @@ class PairedKeyVerificationRunner bool IsVisibilityRecentlyUpdated() const; nearby::Clock* const clock_; - nearby::DeviceInfo& device_info_; - const int64_t share_target_id_; - proto::DeviceVisibility visibility_; - proto::DeviceVisibility last_visibility_; - absl::Time last_visibility_time_; + const nearby::api::DeviceInfo::OsType os_type_; + VisibilityHistory visibility_history_; std::vector raw_token_; NearbyConnection* connection_; std::optional certificate_; diff --git a/sharing/paired_key_verification_runner_test.cc b/sharing/paired_key_verification_runner_test.cc index b0bfc958..562bd27e 100644 --- a/sharing/paired_key_verification_runner_test.cc +++ b/sharing/paired_key_verification_runner_test.cc @@ -28,8 +28,8 @@ #include "protobuf-matchers/protocol-buffer-matchers.h" #include "gtest/gtest.h" #include "absl/time/time.h" +#include "internal/platform/implementation/device_info.h" #include "internal/test/fake_clock.h" -#include "internal/test/fake_device_info.h" #include "proto/sharing_enums.pb.h" #include "sharing/certificates/fake_nearby_share_certificate_manager.h" #include "sharing/certificates/nearby_share_decrypted_public_certificate.h" @@ -45,7 +45,6 @@ #include "sharing/proto/enums.pb.h" #include "sharing/proto/rpc_resources.pb.h" #include "sharing/proto/wire_format.pb.h" -#include "sharing/share_target.h" namespace nearby { namespace sharing { @@ -117,22 +116,18 @@ std::list GeneratePairedKeyResultFrame() { return result; } -struct VisibilityChange { - DeviceVisibility visibility; - DeviceVisibility last_visibility; -}; - -std::list GenerateVisibilityChanges() { +std::list +GenerateVisibilityHistory() { constexpr DeviceVisibility kValidVisibilities[] = { DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, DeviceVisibility::DEVICE_VISIBILITY_SELF_SHARE, DeviceVisibility::DEVICE_VISIBILITY_EVERYONE, DeviceVisibility::DEVICE_VISIBILITY_HIDDEN, }; - std::list result; + std::list result; for (DeviceVisibility visibility : kValidVisibilities) { for (DeviceVisibility last_visibility : kValidVisibilities) { - result.push_back({visibility, last_visibility}); + result.push_back({visibility, last_visibility, absl::UnixEpoch()}); } } return result; @@ -195,13 +190,11 @@ class PairedKeyVerificationRunnerTest : public testing::Test { void SetUp() override { GetFakeClock()->FastForward(absl::Minutes(15)); - share_target_.is_incoming = true; } void RunVerification( - bool is_incoming, - bool use_valid_public_certificate, DeviceVisibility visibility, - DeviceVisibility last_visibility, absl::Time last_visibility_time, + bool is_incoming, bool use_valid_public_certificate, + const PairedKeyVerificationRunner::VisibilityHistory& visibility_history, PairedKeyVerificationRunner::PairedKeyVerificationResult expected_result, OSType expected_os_type = OSType::UNKNOWN_OS_TYPE) { std::optional public_certificate = @@ -211,10 +204,10 @@ class PairedKeyVerificationRunnerTest : public testing::Test { : std::nullopt; auto runner = std::make_shared( - context_.GetClock(), fake_device_info_, share_target_.id, is_incoming, - visibility, last_visibility, last_visibility_time, GetAuthToken(), - &connection_, std::move(public_certificate), &certificate_manager_, - &frames_reader_, kTimeout); + context_.GetClock(), nearby::api::DeviceInfo::OsType::kWindows, + is_incoming, visibility_history, GetAuthToken(), &connection_, + std::move(public_certificate), &certificate_manager_, &frames_reader_, + kTimeout); runner->Run( [&, expected_result, expected_os_type]( @@ -331,11 +324,7 @@ class PairedKeyVerificationRunnerTest : public testing::Test { FakeClock* GetFakeClock() { return context_.fake_clock(); } - protected: - ShareTarget share_target_; - private: - FakeDeviceInfo fake_device_info_; FakeContext context_; FakeNearbyConnection connection_; NearbySharingDecoderImpl decoder_; @@ -350,11 +339,11 @@ TEST_F(PairedKeyVerificationRunnerTest, SetUpPairedKeyResultFrame(ReturnFrameType::kValid); RunVerification( - share_target_.is_incoming, + true, /*use_valid_public_certificate=*/false, - DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, - DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, - GetFakeClock()->Now(), + {.visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility_time = GetFakeClock()->Now()}, /*expected_result=*/ PairedKeyVerificationResult::kUnable); @@ -370,11 +359,11 @@ TEST_F(PairedKeyVerificationRunnerTest, SetUpPairedKeyResultFrame(ReturnFrameType::kNull); RunVerification( - share_target_.is_incoming, + true, /*use_valid_public_certificate=*/true, - DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, - DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, - GetFakeClock()->Now(), + {.visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility_time = GetFakeClock()->Now()}, /*expected_result=*/ PairedKeyVerificationResult::kFail); @@ -413,7 +402,8 @@ struct TestParameters { }; using KeyVerificationTestParam = - std::tuple; + std::tuple; class ParameterisedPairedKeyVerificationRunnerTest : public PairedKeyVerificationRunnerTest, @@ -423,7 +413,8 @@ TEST_P(ParameterisedPairedKeyVerificationRunnerTest, ValidEncryptionFrame_ValidResultFrame) { const TestParameters& params = std::get<0>(GetParam()); PairedKeyResultFrame result_frame = std::get<1>(GetParam()); - VisibilityChange visibility_changes = std::get<2>(GetParam()); + PairedKeyVerificationRunner::VisibilityHistory visibility_history = + std::get<2>(GetParam()); PairedKeyVerificationRunner::PairedKeyVerificationResult expected_result = Merge(params.result, result_frame.status()); @@ -435,9 +426,9 @@ TEST_P(ParameterisedPairedKeyVerificationRunnerTest, << ", result=" << (int)params.result << ", expected_result=" << (int)expected_result << ", result_frame=" << (int)result_frame.status() - << ", visibility=" << (int)visibility_changes.visibility + << ", visibility=" << (int)visibility_history.visibility << ", last_visibility=" - << (int)visibility_changes.last_visibility; + << (int)visibility_history.last_visibility; SetUpPairedKeyEncryptionFrame(params.encryption_frame_type); bool encryption_frame_timeout = @@ -454,13 +445,13 @@ TEST_P(ParameterisedPairedKeyVerificationRunnerTest, // If our visibility has no certificates, then downgrade expected result to // kUnable if it is not expected to fail. - if ((visibility_changes.visibility == + if ((visibility_history.visibility == DeviceVisibility::DEVICE_VISIBILITY_EVERYONE || - visibility_changes.visibility == + visibility_history.visibility == DeviceVisibility::DEVICE_VISIBILITY_HIDDEN) && - (visibility_changes.last_visibility == + (visibility_history.last_visibility == DeviceVisibility::DEVICE_VISIBILITY_EVERYONE || - visibility_changes.last_visibility == + visibility_history.last_visibility == DeviceVisibility::DEVICE_VISIBILITY_HIDDEN)) { if (expected_result == PairedKeyVerificationRunner::PairedKeyVerificationResult::kSuccess) { @@ -468,11 +459,11 @@ TEST_P(ParameterisedPairedKeyVerificationRunnerTest, PairedKeyVerificationRunner::PairedKeyVerificationResult::kUnable; } } + visibility_history.last_visibility_time = GetFakeClock()->Now(); RunVerification( /*is_incoming=*/params.is_incoming, /*use_valid_public_certificate=*/params.has_valid_certificate, - visibility_changes.visibility, visibility_changes.last_visibility, - GetFakeClock()->Now(), expected_result, + visibility_history, expected_result, result_frame.has_os_type() && !encryption_frame_timeout ? result_frame.os_type() : OSType::UNKNOWN_OS_TYPE); @@ -510,7 +501,7 @@ INSTANTIATE_TEST_SUITE_P( /*no prefix*/, ParameterisedPairedKeyVerificationRunnerTest, testing::Combine(testing::ValuesIn(kParameters), testing::ValuesIn(GeneratePairedKeyResultFrame()), - testing::ValuesIn(GenerateVisibilityChanges()))); + testing::ValuesIn(GenerateVisibilityHistory()))); } // namespace } // namespace sharing