From 364664133b69494054b4e3314a81613c0b05d86b Mon Sep 17 00:00:00 2001 From: Vivien Nicolas Date: Tue, 21 Feb 2023 17:59:56 +0100 Subject: [PATCH] Rewrite discover-once to really stop right after the first PASE session failure (#25176) --- .../commands/pairing/PairingCommand.cpp | 36 +++++++++++-------- .../commands/pairing/PairingCommand.h | 1 - .../pairing/DeviceControllerDelegateBridge.mm | 2 +- examples/platform/linux/CommissionerMain.cpp | 3 -- .../commissioner/CommissionerCommands.cpp | 15 +++----- .../commissioner/CommissionerCommands.h | 3 -- src/controller/DevicePairingDelegate.h | 1 - src/controller/SetUpCodePairer.cpp | 19 +++++++--- src/controller/SetUpCodePairer.h | 1 + ...Controller-ScriptDevicePairingDelegate.cpp | 2 -- .../CHIP/MTRDeviceControllerDelegate.h | 7 ++-- .../CHIP/MTRDeviceControllerDelegateBridge.mm | 3 -- 12 files changed, 48 insertions(+), 45 deletions(-) diff --git a/examples/chip-tool/commands/pairing/PairingCommand.cpp b/examples/chip-tool/commands/pairing/PairingCommand.cpp index c0cefcd3420af3..b331c1e013c225 100644 --- a/examples/chip-tool/commands/pairing/PairingCommand.cpp +++ b/examples/chip-tool/commands/pairing/PairingCommand.cpp @@ -109,8 +109,17 @@ CommissioningParameters PairingCommand::GetCommissioningParameters() CHIP_ERROR PairingCommand::PaseWithCode(NodeId remoteId) { - DiscoveryType discoveryType = - mUseOnlyOnNetworkDiscovery.ValueOr(false) ? DiscoveryType::kDiscoveryNetworkOnly : DiscoveryType::kAll; + auto discoveryType = DiscoveryType::kAll; + if (mUseOnlyOnNetworkDiscovery.ValueOr(false)) + { + discoveryType = DiscoveryType::kDiscoveryNetworkOnly; + } + + if (mDiscoverOnce.ValueOr(false)) + { + discoveryType = DiscoveryType::kDiscoveryNetworkOnlyWithoutPASEAutoRetry; + } + return CurrentCommissioner().EstablishPASEConnection(remoteId, mOnboardingPayload, discoveryType); } @@ -126,7 +135,17 @@ CHIP_ERROR PairingCommand::PairWithCode(NodeId remoteId) auto wiFiCredentials = commissioningParams.GetWiFiCredentials(); mUseOnlyOnNetworkDiscovery.SetValue(!threadCredentials.HasValue() && !wiFiCredentials.HasValue()); } - DiscoveryType discoveryType = mUseOnlyOnNetworkDiscovery.Value() ? DiscoveryType::kDiscoveryNetworkOnly : DiscoveryType::kAll; + + auto discoveryType = DiscoveryType::kAll; + if (mUseOnlyOnNetworkDiscovery.ValueOr(false)) + { + discoveryType = DiscoveryType::kDiscoveryNetworkOnly; + } + + if (mDiscoverOnce.ValueOr(false)) + { + discoveryType = DiscoveryType::kDiscoveryNetworkOnlyWithoutPASEAutoRetry; + } return CurrentCommissioner().PairDevice(remoteId, mOnboardingPayload, commissioningParams, discoveryType); } @@ -195,17 +214,6 @@ void PairingCommand::OnStatusUpdate(DevicePairingDelegate::Status status) ChipLogError(chipTool, "Secure Pairing Failed"); SetCommandExitStatus(CHIP_ERROR_INCORRECT_STATE); break; - case DevicePairingDelegate::Status::SecurePairingDiscoveringMoreDevices: - if (IsDiscoverOnce()) - { - ChipLogError(chipTool, "Secure Pairing Failed"); - SetCommandExitStatus(CHIP_ERROR_INCORRECT_STATE); - } - else - { - ChipLogProgress(chipTool, "Secure Pairing Discovering More Devices"); - } - break; } } diff --git a/examples/chip-tool/commands/pairing/PairingCommand.h b/examples/chip-tool/commands/pairing/PairingCommand.h index 516d5f68b56110..970b4c78587c0d 100644 --- a/examples/chip-tool/commands/pairing/PairingCommand.h +++ b/examples/chip-tool/commands/pairing/PairingCommand.h @@ -158,7 +158,6 @@ class PairingCommand : public CHIPCommand, /////////// DeviceDiscoveryDelegate Interface ///////// void OnDiscoveredDevice(const chip::Dnssd::DiscoveredNodeData & nodeData) override; - bool IsDiscoverOnce() { return mDiscoverOnce.ValueOr(false); } /////////// DeviceAttestationDelegate ///////// chip::Optional FailSafeExpiryTimeoutSecs() const override; diff --git a/examples/darwin-framework-tool/commands/pairing/DeviceControllerDelegateBridge.mm b/examples/darwin-framework-tool/commands/pairing/DeviceControllerDelegateBridge.mm index dc53280b2d2553..b5166d8b707b54 100644 --- a/examples/darwin-framework-tool/commands/pairing/DeviceControllerDelegateBridge.mm +++ b/examples/darwin-framework-tool/commands/pairing/DeviceControllerDelegateBridge.mm @@ -36,7 +36,7 @@ - (void)onStatusUpdate:(MTRCommissioningStatus)status _commandBridge->SetCommandExitStatus(CHIP_ERROR_INCORRECT_STATE); break; case MTRCommissioningStatusDiscoveringMoreDevices: - ChipLogProgress(chipTool, "Secure Pairing Discovering More Devices"); + ChipLogError(chipTool, "MTRCommissioningStatusDiscoveringMoreDevices: This should not happen."); break; case MTRCommissioningStatusUnknown: ChipLogError(chipTool, "Uknown Pairing Status"); diff --git a/examples/platform/linux/CommissionerMain.cpp b/examples/platform/linux/CommissionerMain.cpp index f59ff10325c156..c2a4e8292455a8 100644 --- a/examples/platform/linux/CommissionerMain.cpp +++ b/examples/platform/linux/CommissionerMain.cpp @@ -277,9 +277,6 @@ void PairingCommand::OnStatusUpdate(DevicePairingDelegate::Status status) case DevicePairingDelegate::Status::SecurePairingFailed: ChipLogError(AppServer, "Secure Pairing Failed"); break; - case DevicePairingDelegate::Status::SecurePairingDiscoveringMoreDevices: - ChipLogProgress(AppServer, "Secure Pairing Discovering More Device"); - break; } } diff --git a/src/app/tests/suites/commands/commissioner/CommissionerCommands.cpp b/src/app/tests/suites/commands/commissioner/CommissionerCommands.cpp index d9c45b1b5d5341..d60c46d42ce5b0 100644 --- a/src/app/tests/suites/commands/commissioner/CommissionerCommands.cpp +++ b/src/app/tests/suites/commands/commissioner/CommissionerCommands.cpp @@ -33,11 +33,13 @@ CommissionerCommands::PairWithCode(const char * identity, memcpy(code, value.payload.data(), value.payload.size()); ChipLogError(chipTool, "Pairing Code is %s", code); - mDiscoverOnce = value.discoverOnce; - // To reduce the scanning latency in some setups, and since the primary use for PairWithCode is to commission a device to // another commissioner, assume that the commissionable device is available on the network. - chip::Controller::DiscoveryType discoveryType = chip::Controller::DiscoveryType::kDiscoveryNetworkOnly; + auto discoveryType = chip::Controller::DiscoveryType::kDiscoveryNetworkOnly; + if (value.discoverOnce.ValueOr(false)) + { + discoveryType = chip::Controller::DiscoveryType::kDiscoveryNetworkOnlyWithoutPASEAutoRetry; + } return GetCommissioner(identity).PairDevice(value.nodeId, code, discoveryType); } @@ -129,13 +131,6 @@ void CommissionerCommands::OnStatusUpdate(DevicePairingDelegate::Status status) ChipLogError(chipTool, "Secure Pairing Failed"); OnResponse(ConvertToStatusIB(CHIP_ERROR_INCORRECT_STATE), nullptr); break; - case DevicePairingDelegate::Status::SecurePairingDiscoveringMoreDevices: - if (mDiscoverOnce.ValueOr(false)) - { - ChipLogError(chipTool, "Secure Pairing Failed"); - OnResponse(ConvertToStatusIB(CHIP_ERROR_INCORRECT_STATE), nullptr); - } - break; } } diff --git a/src/app/tests/suites/commands/commissioner/CommissionerCommands.h b/src/app/tests/suites/commands/commissioner/CommissionerCommands.h index 31cbcef4396c3a..9056f3a53947e3 100644 --- a/src/app/tests/suites/commands/commissioner/CommissionerCommands.h +++ b/src/app/tests/suites/commands/commissioner/CommissionerCommands.h @@ -45,7 +45,4 @@ class CommissionerCommands : public chip::Controller::DevicePairingDelegate void OnPairingComplete(CHIP_ERROR error) override; void OnPairingDeleted(CHIP_ERROR error) override; void OnCommissioningComplete(chip::NodeId deviceId, CHIP_ERROR error) override; - -private: - chip::Optional mDiscoverOnce; }; diff --git a/src/controller/DevicePairingDelegate.h b/src/controller/DevicePairingDelegate.h index 016fe344d8a412..7f8b98dc6c2b4b 100644 --- a/src/controller/DevicePairingDelegate.h +++ b/src/controller/DevicePairingDelegate.h @@ -40,7 +40,6 @@ class DLL_EXPORT DevicePairingDelegate { SecurePairingSuccess = 0, SecurePairingFailed, - SecurePairingDiscoveringMoreDevices, }; /** diff --git a/src/controller/SetUpCodePairer.cpp b/src/controller/SetUpCodePairer.cpp index d0979d15e06174..e630d7630b9fdc 100644 --- a/src/controller/SetUpCodePairer.cpp +++ b/src/controller/SetUpCodePairer.cpp @@ -348,9 +348,20 @@ void SetUpCodePairer::NotifyCommissionableDeviceDiscovered(const Dnssd::Discover ChipLogProgress(Controller, "Discovered device to be commissioned over DNS-SD"); auto & resolutionData = nodeData.resolutionData; - for (size_t i = 0; i < resolutionData.numIPs; i++) + + if (mDiscoveryType == DiscoveryType::kDiscoveryNetworkOnlyWithoutPASEAutoRetry) + { + // If the discovery type does not want the PASE auto retry mechanism, we will just store + // a single IP. So the discovery process is stopped as it won't be of any help anymore. + StopConnectOverIP(); + mDiscoveredParameters.emplace_back(nodeData.resolutionData, 0); + } + else { - mDiscoveredParameters.emplace_back(nodeData.resolutionData, i); + for (size_t i = 0; i < resolutionData.numIPs; i++) + { + mDiscoveredParameters.emplace_back(nodeData.resolutionData, i); + } } ConnectToDiscoveredDevice(); @@ -447,14 +458,14 @@ void SetUpCodePairer::OnStatusUpdate(DevicePairingDelegate::Status status) if (!mDiscoveredParameters.empty()) { ChipLogProgress(Controller, "Ignoring SecurePairingFailed status for now; we have more discovered devices to try"); - status = DevicePairingDelegate::Status::SecurePairingDiscoveringMoreDevices; + return; } if (DiscoveryInProgress()) { ChipLogProgress(Controller, "Ignoring SecurePairingFailed status for now; we are waiting to see if we discover more devices"); - status = DevicePairingDelegate::Status::SecurePairingDiscoveringMoreDevices; + return; } } diff --git a/src/controller/SetUpCodePairer.h b/src/controller/SetUpCodePairer.h index 0b665a9cbbaa1b..82c6fc33157cb8 100644 --- a/src/controller/SetUpCodePairer.h +++ b/src/controller/SetUpCodePairer.h @@ -68,6 +68,7 @@ enum class SetupCodePairerBehaviour : uint8_t enum class DiscoveryType : uint8_t { kDiscoveryNetworkOnly, + kDiscoveryNetworkOnlyWithoutPASEAutoRetry, kAll, }; diff --git a/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.cpp b/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.cpp index f6373245d27679..66f461d0071b3c 100644 --- a/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.cpp +++ b/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.cpp @@ -84,8 +84,6 @@ void ScriptDevicePairingDelegate::OnStatusUpdate(DevicePairingDelegate::Status s mOnPairingCompleteCallback(ToPyChipError(CHIP_ERROR_INCORRECT_STATE)); } break; - case DevicePairingDelegate::Status::SecurePairingDiscoveringMoreDevices: - break; } } diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDelegate.h b/src/darwin/Framework/CHIP/MTRDeviceControllerDelegate.h index f136de3c04f8b7..e101927729ec23 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDelegate.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDelegate.h @@ -23,7 +23,8 @@ typedef NS_ENUM(NSInteger, MTRCommissioningStatus) { MTRCommissioningStatusUnknown = 0, MTRCommissioningStatusSuccess = 1, MTRCommissioningStatusFailed = 2, - MTRCommissioningStatusDiscoveringMoreDevices = 3 + MTRCommissioningStatusDiscoveringMoreDevices MTR_NEWLY_DEPRECATED("MTRCommissioningStatusDiscoveringMoreDevices is not used.") + = 3, } API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)); @class MTRDeviceController; @@ -64,8 +65,8 @@ typedef NS_ENUM(NSUInteger, MTRPairingStatus) { MTRPairingStatusFailed API_DEPRECATED( "Please use MTRCommissioningStatusFailed", ios(16.1, 16.4), macos(13.0, 13.3), watchos(9.1, 9.4), tvos(16.1, 16.4)) = 2, - MTRPairingStatusDiscoveringMoreDevices API_DEPRECATED("Please use MTRCommissioningStatusDiscoveringMoreDevices", - ios(16.1, 16.4), macos(13.0, 13.3), watchos(9.1, 9.4), tvos(16.1, 16.4)) + MTRPairingStatusDiscoveringMoreDevices API_DEPRECATED("MTRPairingStatusDiscoveringMoreDevices is not used.", ios(16.1, 16.4), + macos(13.0, 13.3), watchos(9.1, 9.4), tvos(16.1, 16.4)) = 3 } API_DEPRECATED("Please use MTRCommissioningStatus", ios(16.1, 16.4), macos(13.0, 13.3), watchos(9.1, 9.4), tvos(16.1, 16.4)); diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.mm index bc7865a0594e06..59d18882e536c4 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.mm @@ -51,9 +51,6 @@ case chip::Controller::DevicePairingDelegate::Status::SecurePairingFailed: rv = MTRCommissioningStatusFailed; break; - case chip::Controller::DevicePairingDelegate::Status::SecurePairingDiscoveringMoreDevices: - rv = MTRCommissioningStatusDiscoveringMoreDevices; - break; } return rv; }