From 780de25b5703e483375d7ea121dc90024d5bbcb0 Mon Sep 17 00:00:00 2001 From: Jakub Pawlowski Date: Thu, 13 Feb 2020 00:10:12 +0100 Subject: [PATCH] Get rid of UI callbacks from SecurityManagerListener Test: updated unittests Bug: 142341141 Change-Id: If6b086f74d92e757d02df4174af73edb36d6de3d --- gd/security/facade.cc | 29 ++++++----- gd/security/internal/security_manager_impl.cc | 2 +- gd/security/pairing/classic_pairing_handler.cc | 56 ++++++++-------------- gd/security/pairing/classic_pairing_handler.h | 16 ++++--- .../pairing/classic_pairing_handler_unittest.cc | 6 ++- gd/security/security_manager_listener.h | 34 ------------- gd/shim/security.cc | 41 ++++------------ 7 files changed, 59 insertions(+), 125 deletions(-) diff --git a/gd/security/facade.cc b/gd/security/facade.cc index 6d78d8757..d85148144 100644 --- a/gd/security/facade.cc +++ b/gd/security/facade.cc @@ -25,7 +25,7 @@ namespace bluetooth { namespace security { -class SecurityModuleFacadeService : public SecurityModuleFacade::Service, public ISecurityManagerListener { +class SecurityModuleFacadeService : public SecurityModuleFacade::Service, public ISecurityManagerListener, public UI { public: SecurityModuleFacadeService(SecurityModule* security_module, ::bluetooth::os::Handler* security_handler) : security_module_(security_module), security_handler_(security_handler) { @@ -87,10 +87,18 @@ class SecurityModuleFacadeService : public SecurityModuleFacade::Service, public return bond_events_.RunLoop(context, writer); } - void OnDisplayYesNoDialogWithValue(const bluetooth::hci::AddressWithType& peer, uint32_t numeric_value, - common::OnceCallback callback) override { + void DisplayPairingPrompt(const bluetooth::hci::AddressWithType& peer, std::string name) { + LOG_INFO("%s", peer.ToString().c_str()); + UiMsg display_yes_no; + display_yes_no.mutable_peer()->mutable_address()->set_address(peer.ToString()); + display_yes_no.mutable_peer()->set_type(facade::BluetoothAddressTypeEnum::PUBLIC_DEVICE_ADDRESS); + display_yes_no.set_message_type(UiMsgType::DISPLAY_YES_NO); + display_yes_no.set_unique_id(unique_id++); + } + + virtual void DisplayConfirmValue(const bluetooth::hci::AddressWithType& peer, std::string name, + uint32_t numeric_value) { LOG_INFO("%s value = 0x%x", peer.ToString().c_str(), numeric_value); - user_yes_no_callbacks_.emplace(unique_id, std::move(callback)); UiMsg display_with_value; display_with_value.mutable_peer()->mutable_address()->set_address(peer.ToString()); display_with_value.mutable_peer()->set_type(facade::BluetoothAddressTypeEnum::PUBLIC_DEVICE_ADDRESS); @@ -100,19 +108,16 @@ class SecurityModuleFacadeService : public SecurityModuleFacade::Service, public ui_events_.OnIncomingEvent(display_with_value); } - void OnDisplayYesNoDialog(const bluetooth::hci::AddressWithType& peer, - common::OnceCallback callback) override { + void DisplayYesNoDialog(const bluetooth::hci::AddressWithType& peer, std::string name) override { LOG_INFO("%s", peer.ToString().c_str()); - user_yes_no_callbacks_.emplace(unique_id, std::move(callback)); UiMsg display_yes_no; display_yes_no.mutable_peer()->mutable_address()->set_address(peer.ToString()); display_yes_no.mutable_peer()->set_type(facade::BluetoothAddressTypeEnum::PUBLIC_DEVICE_ADDRESS); display_yes_no.set_message_type(UiMsgType::DISPLAY_YES_NO); display_yes_no.set_unique_id(unique_id++); - ui_events_.OnIncomingEvent(display_yes_no); } - void OnDisplayPasskeyDialog(const bluetooth::hci::AddressWithType& peer, uint32_t passkey) override { + void DisplayPasskey(const bluetooth::hci::AddressWithType& peer, std::string name, uint32_t passkey) override { LOG_INFO("%s value = 0x%x", peer.ToString().c_str(), passkey); UiMsg display_passkey; display_passkey.mutable_peer()->mutable_address()->set_address(peer.ToString()); @@ -123,10 +128,8 @@ class SecurityModuleFacadeService : public SecurityModuleFacade::Service, public ui_events_.OnIncomingEvent(display_passkey); } - void OnDisplayPasskeyInputDialog(const bluetooth::hci::AddressWithType& peer, - common::OnceCallback callback) override { + void DisplayEnterPasskeyDialog(const bluetooth::hci::AddressWithType& peer, std::string name) override { LOG_INFO("%s", peer.ToString().c_str()); - user_passkey_callbacks_.emplace(unique_id, std::move(callback)); UiMsg display_passkey_input; display_passkey_input.mutable_peer()->mutable_address()->set_address(peer.ToString()); display_passkey_input.mutable_peer()->set_type(facade::BluetoothAddressTypeEnum::PUBLIC_DEVICE_ADDRESS); @@ -135,7 +138,7 @@ class SecurityModuleFacadeService : public SecurityModuleFacade::Service, public ui_events_.OnIncomingEvent(display_passkey_input); } - void OnDisplayCancelDialog(const bluetooth::hci::AddressWithType& peer) override { + void Cancel(const bluetooth::hci::AddressWithType& peer) override { LOG_INFO("%s", peer.ToString().c_str()); UiMsg display_cancel; display_cancel.mutable_peer()->mutable_address()->set_address(peer.ToString()); diff --git a/gd/security/internal/security_manager_impl.cc b/gd/security/internal/security_manager_impl.cc index 0df1a6a61..f7afa4fcd 100644 --- a/gd/security/internal/security_manager_impl.cc +++ b/gd/security/internal/security_manager_impl.cc @@ -48,7 +48,7 @@ void SecurityManagerImpl::DispatchPairingHandler(record::SecurityRecord& record, std::make_shared(record.GetPseudoAddress()); pairing_handler = std::make_shared( l2cap_classic_module_->GetFixedChannelManager(), security_manager_channel_, record_copy, security_handler_, - std::move(callback), listeners_); + std::move(callback), user_interface_, user_interface_handler_, "TODO: grab device name properly"); break; } default: diff --git a/gd/security/pairing/classic_pairing_handler.cc b/gd/security/pairing/classic_pairing_handler.cc index bb16f0a9e..3bc094d29 100644 --- a/gd/security/pairing/classic_pairing_handler.cc +++ b/gd/security/pairing/classic_pairing_handler.cc @@ -23,42 +23,29 @@ namespace bluetooth { namespace security { namespace pairing { -void ClassicPairingHandler::NotifyUiDisplayYesNo(uint32_t numeric_value, - common::OnceCallback input_callback) { - for (auto& iter : client_listeners_) { - iter.second->Post(common::BindOnce(&ISecurityManagerListener::OnDisplayYesNoDialogWithValue, - common::Unretained(iter.first), GetRecord()->GetPseudoAddress(), numeric_value, - std::move(input_callback))); - } +void ClassicPairingHandler::NotifyUiDisplayYesNo(uint32_t numeric_value) { + user_interface_handler_->Post(common::BindOnce(&UI::DisplayConfirmValue, common::Unretained(user_interface_), + GetRecord()->GetPseudoAddress(), device_name_, numeric_value)); } -void ClassicPairingHandler::NotifyUiDisplayYesNo(common::OnceCallback input_callback) { - for (auto& iter : client_listeners_) { - iter.second->Post(common::BindOnce(&ISecurityManagerListener::OnDisplayYesNoDialog, common::Unretained(iter.first), - GetRecord()->GetPseudoAddress(), std::move(input_callback))); - } +void ClassicPairingHandler::NotifyUiDisplayYesNo() { + user_interface_handler_->Post(common::BindOnce(&UI::DisplayYesNoDialog, common::Unretained(user_interface_), + GetRecord()->GetPseudoAddress(), device_name_)); } void ClassicPairingHandler::NotifyUiDisplayPasskey(uint32_t passkey) { - for (auto& iter : client_listeners_) { - iter.second->Post(common::BindOnce(&ISecurityManagerListener::OnDisplayPasskeyDialog, - common::Unretained(iter.first), GetRecord()->GetPseudoAddress(), passkey)); - } + user_interface_handler_->Post(common::BindOnce(&UI::DisplayPasskey, common::Unretained(user_interface_), + GetRecord()->GetPseudoAddress(), device_name_, passkey)); } -void ClassicPairingHandler::NotifyUiDisplayPasskeyInput(common::OnceCallback input_callback) { - for (auto& iter : client_listeners_) { - iter.second->Post(common::BindOnce(&ISecurityManagerListener::OnDisplayPasskeyInputDialog, - common::Unretained(iter.first), GetRecord()->GetPseudoAddress(), - std::move(input_callback))); - } +void ClassicPairingHandler::NotifyUiDisplayPasskeyInput() { + user_interface_handler_->Post(common::BindOnce(&UI::DisplayEnterPasskeyDialog, common::Unretained(user_interface_), + GetRecord()->GetPseudoAddress(), device_name_)); } void ClassicPairingHandler::NotifyUiDisplayCancel() { - for (auto& iter : client_listeners_) { - iter.second->Post(common::BindOnce(&ISecurityManagerListener::OnDisplayCancelDialog, common::Unretained(iter.first), - GetRecord()->GetPseudoAddress())); - } + user_interface_handler_->Post( + common::BindOnce(&UI::Cancel, common::Unretained(user_interface_), GetRecord()->GetPseudoAddress())); } void ClassicPairingHandler::OnRegistrationComplete( @@ -301,15 +288,13 @@ void ClassicPairingHandler::OnReceive(hci::UserConfirmationRequestView packet) { case hci::IoCapability::DISPLAY_ONLY: // NumericComparison, Initiator display, Responder auto confirm LOG_INFO("Numeric Comparison: A DisplayYesNo, B auto confirm"); - NotifyUiDisplayYesNo(packet.GetNumericValue(), - common::BindOnce(&ClassicPairingHandler::OnUserInput, common::Unretained(this))); + NotifyUiDisplayYesNo(packet.GetNumericValue()); // Unauthenticated break; case hci::IoCapability::DISPLAY_YES_NO: // NumericComparison Both Display, Both confirm LOG_INFO("Numeric Comparison: A and B DisplayYesNo"); - NotifyUiDisplayYesNo(packet.GetNumericValue(), - common::BindOnce(&ClassicPairingHandler::OnUserInput, common::Unretained(this))); + NotifyUiDisplayYesNo(packet.GetNumericValue()); // Authenticated break; case hci::IoCapability::KEYBOARD_ONLY: @@ -320,7 +305,7 @@ void ClassicPairingHandler::OnReceive(hci::UserConfirmationRequestView packet) { break; case hci::IoCapability::NO_INPUT_NO_OUTPUT: // NumericComparison, auto confirm Responder, Yes/No confirm Initiator. Don't show confirmation value - NotifyUiDisplayYesNo(common::BindOnce(&ClassicPairingHandler::OnUserInput, common::Unretained(this))); + NotifyUiDisplayYesNo(); LOG_INFO("Numeric Comparison: A DisplayYesNo, B auto confirm, no show value"); // Unauthenticated break; @@ -330,22 +315,19 @@ void ClassicPairingHandler::OnReceive(hci::UserConfirmationRequestView packet) { switch (responder_io_capability) { case hci::IoCapability::DISPLAY_ONLY: // PassKey Entry, Responder display, Initiator input - NotifyUiDisplayPasskeyInput( - common::BindOnce(&ClassicPairingHandler::OnPasskeyInput, common::Unretained(this))); + NotifyUiDisplayPasskeyInput(); LOG_INFO("Passkey Entry: A input, B display"); // Authenticated break; case hci::IoCapability::DISPLAY_YES_NO: // PassKey Entry, Responder display, Initiator input - NotifyUiDisplayPasskeyInput( - common::BindOnce(&ClassicPairingHandler::OnPasskeyInput, common::Unretained(this))); + NotifyUiDisplayPasskeyInput(); LOG_INFO("Passkey Entry: A input, B display"); // Authenticated break; case hci::IoCapability::KEYBOARD_ONLY: // PassKey Entry, both input - NotifyUiDisplayPasskeyInput( - common::BindOnce(&ClassicPairingHandler::OnPasskeyInput, common::Unretained(this))); + NotifyUiDisplayPasskeyInput(); LOG_INFO("Passkey Entry: A input, B input"); // Authenticated break; diff --git a/gd/security/pairing/classic_pairing_handler.h b/gd/security/pairing/classic_pairing_handler.h index 49640911f..b740a301f 100644 --- a/gd/security/pairing/classic_pairing_handler.h +++ b/gd/security/pairing/classic_pairing_handler.h @@ -44,13 +44,14 @@ class ClassicPairingHandler : public PairingHandler { channel::SecurityManagerChannel* security_manager_channel, std::shared_ptr record, os::Handler* security_handler, common::OnceCallback complete_callback, - std::vector>& client_listeners) + UI* user_interface, os::Handler* user_interface_handler, std::string device_name) : PairingHandler(security_manager_channel, std::move(record)), fixed_channel_manager_(std::move(fixed_channel_manager)), security_policy_(), security_handler_(security_handler), remote_io_capability_(kDefaultIoCapability), local_io_capability_(kDefaultIoCapability), local_oob_present_(kDefaultOobDataPresent), local_authentication_requirements_(kDefaultAuthenticationRequirements), - complete_callback_(std::move(complete_callback)), client_listeners_(client_listeners) {} + complete_callback_(std::move(complete_callback)), user_interface_(user_interface), + user_interface_handler_(user_interface_handler), device_name_(device_name) {} ~ClassicPairingHandler() override = default; @@ -84,10 +85,10 @@ class ClassicPairingHandler : public PairingHandler { void OnConnectionClose(hci::ErrorCode error_code); void OnUserInput(bool user_input); void OnPasskeyInput(uint32_t passkey); - void NotifyUiDisplayYesNo(uint32_t numeric_value, common::OnceCallback input_callback); - void NotifyUiDisplayYesNo(common::OnceCallback input_callback); + void NotifyUiDisplayYesNo(uint32_t numeric_value); + void NotifyUiDisplayYesNo(); void NotifyUiDisplayPasskey(uint32_t passkey); - void NotifyUiDisplayPasskeyInput(common::OnceCallback input_callback); + void NotifyUiDisplayPasskeyInput(); void NotifyUiDisplayCancel(); void UserClickedYes(); void UserClickedNo(); @@ -102,7 +103,10 @@ class ClassicPairingHandler : public PairingHandler { hci::AuthenticationRequirements local_authentication_requirements_ __attribute__((unused)); std::unique_ptr fixed_channel_{nullptr}; common::OnceCallback complete_callback_; - std::vector>& client_listeners_; + UI* user_interface_; + os::Handler* user_interface_handler_; + std::string device_name_; + hci::ErrorCode last_status_; bool locally_initiated_ = false; uint32_t passkey_ = 0; diff --git a/gd/security/pairing/classic_pairing_handler_unittest.cc b/gd/security/pairing/classic_pairing_handler_unittest.cc index c4e0b8fd6..d134ff88d 100644 --- a/gd/security/pairing/classic_pairing_handler_unittest.cc +++ b/gd/security/pairing/classic_pairing_handler_unittest.cc @@ -126,7 +126,8 @@ class ClassicPairingHandlerTest : public ::testing::Test { EXPECT_CALL(*sptr, RegisterService(::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_)) .Times(::testing::AnyNumber()); pairing_handler_ = new pairing::ClassicPairingHandler(sptr, channel_, security_record_, handler_, - common::Bind(&pairing_complete_callback), client_listeners_); + common::Bind(&pairing_complete_callback), user_interface_, + user_interface_handler_, "Fake name"); channel_callback_ = new SecurityManagerChannelCallback(pairing_handler_); channel_->SetChannelListener(channel_callback_); } @@ -151,7 +152,8 @@ class ClassicPairingHandlerTest : public ::testing::Test { channel::SecurityManagerChannel* channel_ = nullptr; pairing::ClassicPairingHandler* pairing_handler_ = nullptr; std::shared_ptr security_record_ = nullptr; - std::vector> client_listeners_; + UI* user_interface_; + os::Handler* user_interface_handler_; }; // Security Manager Boot Sequence (Required for SSP, these are already set at boot time) diff --git a/gd/security/security_manager_listener.h b/gd/security/security_manager_listener.h index 3f00fbe67..edc4e6b94 100644 --- a/gd/security/security_manager_listener.h +++ b/gd/security/security_manager_listener.h @@ -31,40 +31,6 @@ class ISecurityManagerListener { virtual ~ISecurityManagerListener() = 0; /** - * Display Yes/No dialog with numeric value - * - * @param address pairing device - * @param numeric_value numeric comparison value to verify - */ - virtual void OnDisplayYesNoDialogWithValue(const bluetooth::hci::AddressWithType& address, uint32_t numeric_value, - common::OnceCallback input_callback) = 0; - - /** - * Display Yes/No - * - * @param address pairing device - */ - virtual void OnDisplayYesNoDialog(const bluetooth::hci::AddressWithType& address, - common::OnceCallback input_callback) = 0; - - /** - * Present the passkey value to the user - */ - virtual void OnDisplayPasskeyDialog(const bluetooth::hci::AddressWithType& address, uint32_t passkey) = 0; - - /** - * Display a dialog box that will let user enter the Passkey - */ - virtual void OnDisplayPasskeyInputDialog(const bluetooth::hci::AddressWithType& address, - common::OnceCallback input_callback) = 0; - - /** - * Remove the pairing prompt from DisplayPairingPrompt, i.e. remote device - * disconnected, or some application requested bond with this device. So the UI should dismiss the pairing prompt. - */ - virtual void OnDisplayCancelDialog(const bluetooth::hci::AddressWithType& address) = 0; - - /** * Called when a device is successfully bonded. * * @param address of the newly bonded device diff --git a/gd/shim/security.cc b/gd/shim/security.cc index 24dab4847..0e2b0699a 100644 --- a/gd/shim/security.cc +++ b/gd/shim/security.cc @@ -43,50 +43,27 @@ constexpr uint8_t kLegacyAddressTypeRandomIdentity = 3; // TOOD: implement properly, have it passed from above shim ? class UIHandler : public ::bluetooth::security::UI { public: - void DisplayPairingPrompt(const hci::AddressWithType& address, std::string name) {} - void Cancel(const hci::AddressWithType& address) {} - void DisplayConfirmValue(const hci::AddressWithType& address, std::string name, uint32_t numeric_value) {} - void DisplayYesNoDialog(const bluetooth::hci::AddressWithType& address, std::string name) {} - void DisplayEnterPasskeyDialog(const hci::AddressWithType& address, std::string name) {} - void DisplayPasskey(const hci::AddressWithType& address, std::string name, uint32_t passkey) {} + void DisplayPairingPrompt(const hci::AddressWithType& address, std::string name) override {} + void Cancel(const hci::AddressWithType& address) override {} + void DisplayConfirmValue(const hci::AddressWithType& address, std::string name, uint32_t numeric_value) override {} + void DisplayYesNoDialog(const bluetooth::hci::AddressWithType& address, std::string name) override {} + void DisplayEnterPasskeyDialog(const hci::AddressWithType& address, std::string name) override {} + void DisplayPasskey(const hci::AddressWithType& address, std::string name, uint32_t passkey) override {} }; UIHandler static_ui_handler; } // namespace struct Security::impl : public security::ISecurityManagerListener { - void OnDisplayYesNoDialogWithValue(const bluetooth::hci::AddressWithType& address, uint32_t numeric_value, - common::OnceCallback input_callback) { - std::move(input_callback).Run(simple_pairing_callback_(address.ToString(), numeric_value, false)); - } - - void OnDisplayYesNoDialog(const bluetooth::hci::AddressWithType& address, - common::OnceCallback input_callback) { - LOG_DEBUG("UNIMPLEMENTED %s", __func__); - } - - void OnDisplayPasskeyDialog(const bluetooth::hci::AddressWithType& address, uint32_t passkey) { - LOG_DEBUG("UNIMPLEMENTED %s", __func__); - } - - void OnDisplayPasskeyInputDialog(const bluetooth::hci::AddressWithType& address, - common::OnceCallback input_callback) { - LOG_DEBUG("UNIMPLEMENTED %s", __func__); - } - - void OnDisplayCancelDialog(const bluetooth::hci::AddressWithType& address) { - LOG_DEBUG("UNIMPLEMENTED %s", __func__); - } - - void OnDeviceBonded(bluetooth::hci::AddressWithType device) { + void OnDeviceBonded(bluetooth::hci::AddressWithType device) override { LOG_DEBUG("UNIMPLEMENTED %s", __func__); } - void OnDeviceUnbonded(bluetooth::hci::AddressWithType device) { + void OnDeviceUnbonded(bluetooth::hci::AddressWithType device) override { LOG_DEBUG("UNIMPLEMENTED %s", __func__); } - void OnDeviceBondFailed(bluetooth::hci::AddressWithType device) { + void OnDeviceBondFailed(bluetooth::hci::AddressWithType device) override { LOG_DEBUG("UNIMPLEMENTED %s", __func__); } -- 2.11.0