diff --git a/src/controller/BUILD.gn b/src/controller/BUILD.gn index e3d39d57747957..f136f53786e0e1 100644 --- a/src/controller/BUILD.gn +++ b/src/controller/BUILD.gn @@ -50,6 +50,7 @@ static_library("controller") { "CommissioningWindowOpener.cpp", "CommissioningWindowOpener.h", "DeviceDiscoveryDelegate.h", + "DevicePairingDelegate.h", "EmptyDataModelHandler.cpp", "ExampleOperationalCredentialsIssuer.cpp", "ExampleOperationalCredentialsIssuer.h", diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index ad4a64161c30b7..1ed98fc3bf1205 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -481,7 +481,7 @@ CHIP_ERROR DeviceCommissioner::Shutdown() if (device != nullptr && device->IsSessionSetupInProgress()) { ChipLogDetail(Controller, "Setup in progress, stopping setup before shutting down"); - PairingFailed(CHIP_ERROR_CONNECTION_ABORTED); + OnSessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); } // TODO: If we have a commissioning step in progress, is there a way to cancel that callback? @@ -812,27 +812,7 @@ void DeviceCommissioner::RendezvousCleanup(CHIP_ERROR status) void DeviceCommissioner::OnSessionEstablishmentError(CHIP_ERROR err) { - // Need to null out mDeviceInPASEEstablishment before maybe trying to - // establish PASE again, so EstablishPASEConnection won't error out. - auto * oldDeviceInPASEEstablishment = mDeviceInPASEEstablishment; - mDeviceInPASEEstablishment = nullptr; - - if (mSetUpCodePairer.TryNextRendezvousParameters()) - { - ChipLogProgress(Controller, "Ignoring PASE error; will try commissioning over a different transport"); - // We don't need oldDeviceInPASEEstablishment anymore. - ReleaseCommissioneeDevice(oldDeviceInPASEEstablishment); - return; - } - - // Reset mDeviceInPASEEstablishment because PairingFailed checks for that to - // send the right notifications. - mDeviceInPASEEstablishment = oldDeviceInPASEEstablishment; - PairingFailed(err); -} - -void DeviceCommissioner::PairingFailed(CHIP_ERROR err) -{ + // PASE session establishment failure. mSystemState->SystemLayer()->CancelTimer(OnSessionEstablishmentTimeoutCallback, this); if (mPairingDelegate != nullptr) @@ -851,7 +831,7 @@ void DeviceCommissioner::OnSessionEstablished() // We are in the callback for this pairing. Reset so we can pair another device. mDeviceInPASEEstablishment = nullptr; - VerifyOrReturn(device != nullptr, PairingFailed(CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR)); + VerifyOrReturn(device != nullptr, OnSessionEstablishmentError(CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR)); PASESession * pairing = &device->GetPairing(); @@ -862,22 +842,19 @@ void DeviceCommissioner::OnSessionEstablished() if (err != CHIP_NO_ERROR) { ChipLogError(Controller, "Failed in setting up secure channel: err %s", ErrorStr(err)); - PairingFailed(err); + OnSessionEstablishmentError(err); return; } ChipLogDetail(Controller, "Remote device completed SPAKE2+ handshake"); + mPairingDelegate->OnPairingComplete(CHIP_NO_ERROR); + if (mRunCommissioningAfterConnection) { mRunCommissioningAfterConnection = false; mDefaultCommissioner->StartCommissioning(this, device); } - else - { - ChipLogProgress(Controller, "OnPairingComplete"); - mPairingDelegate->OnPairingComplete(CHIP_NO_ERROR); - } } CHIP_ERROR DeviceCommissioner::SendCertificateChainRequestCommand(DeviceProxy * device, diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index bae498e4ec1d35..a69758023edffb 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -111,47 +112,6 @@ struct ControllerInitParams uint16_t controllerVendorId; }; -class DLL_EXPORT DevicePairingDelegate -{ -public: - virtual ~DevicePairingDelegate() {} - - enum Status : uint8_t - { - SecurePairingSuccess = 0, - SecurePairingFailed, - }; - - /** - * @brief - * Called when the pairing reaches a certain stage. - * - * @param status Current status of pairing - */ - virtual void OnStatusUpdate(DevicePairingDelegate::Status status) {} - - /** - * @brief - * Called when the pairing is complete (with success or error) - * - * @param error Error cause, if any - */ - virtual void OnPairingComplete(CHIP_ERROR error) {} - - /** - * @brief - * Called when the pairing is deleted (with success or error) - * - * @param error Error cause, if any - */ - virtual void OnPairingDeleted(CHIP_ERROR error) {} - - /** - * Called when the commissioning process is complete (with success or error) - */ - virtual void OnCommissioningComplete(NodeId deviceId, CHIP_ERROR error) {} -}; - struct CommissionerInitParams : public ControllerInitParams { DevicePairingDelegate * pairingDelegate = nullptr; @@ -587,6 +547,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, void OnNodeDiscovered(const chip::Dnssd::DiscoveredNodeData & nodeData) override; void RegisterPairingDelegate(DevicePairingDelegate * pairingDelegate) { mPairingDelegate = pairingDelegate; } + DevicePairingDelegate * GetPairingDelegate() const { return mPairingDelegate; } // AttributeCache::Callback impl void OnDone() override; @@ -623,11 +584,6 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, static void OnSessionEstablishmentTimeoutCallback(System::Layer * aLayer, void * aAppState); - // Helper to call once we decide that pairing has failed. This might be - // called from OnSessionEstablishmentError if we have no other transports to - // try, or in various other failure cases. - void PairingFailed(CHIP_ERROR err); - /* This function sends a Device Attestation Certificate chain request to the device. The function does not hold a reference to the device object. */ diff --git a/src/controller/DevicePairingDelegate.h b/src/controller/DevicePairingDelegate.h new file mode 100644 index 00000000000000..fe1c0121ad9ab4 --- /dev/null +++ b/src/controller/DevicePairingDelegate.h @@ -0,0 +1,74 @@ +/* + * Copyright (c) 2021 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include +#include +#include + +namespace chip { +namespace Controller { + +/** + * A delegate that can be notified of progress as a "pairing" (which might mean + * "PASE session establishment" or "commissioning") proceeds. + */ +class DLL_EXPORT DevicePairingDelegate +{ +public: + virtual ~DevicePairingDelegate() {} + + enum Status : uint8_t + { + SecurePairingSuccess = 0, + SecurePairingFailed, + }; + + /** + * @brief + * Called when the pairing reaches a certain stage. + * + * @param status Current status of pairing + */ + virtual void OnStatusUpdate(DevicePairingDelegate::Status status) {} + + /** + * @brief + * Called when PASE session establishment is complete (with success or error) + * + * @param error Error cause, if any + */ + virtual void OnPairingComplete(CHIP_ERROR error) {} + + /** + * @brief + * Called when the pairing is deleted (with success or error) + * + * @param error Error cause, if any + */ + virtual void OnPairingDeleted(CHIP_ERROR error) {} + + /** + * Called when the commissioning process is complete (with success or error) + */ + virtual void OnCommissioningComplete(NodeId deviceId, CHIP_ERROR error) {} +}; + +} // namespace Controller +} // namespace chip diff --git a/src/controller/SetUpCodePairer.cpp b/src/controller/SetUpCodePairer.cpp index 4405da2d9f9ed6..c9c53996c320ab 100644 --- a/src/controller/SetUpCodePairer.cpp +++ b/src/controller/SetUpCodePairer.cpp @@ -162,10 +162,10 @@ CHIP_ERROR SetUpCodePairer::StopConnectOverSoftAP() bool SetUpCodePairer::ConnectToDiscoveredDevice() { - if (mWaitingForConnection) + if (mWaitingForPASE) { - // Nothing to do. Just wait until we either succeed at that connection - // or fail and get asked to try another device. + // Nothing to do. Just wait until we either succeed or fail at that + // PASE session establishment. return false; } @@ -189,6 +189,9 @@ bool SetUpCodePairer::ConnectToDiscoveredDevice() ChipLogProgress(Controller, "Attempting PASE connection to %s", buf); #endif // CHIP_PROGRESS_LOGGING + // Handle possibly-sync call backs from attempts to establish PASE. + ExpectPASEEstablishment(); + CHIP_ERROR err; if (mConnectionType == SetupCodePairerBehaviour::kCommission) { @@ -198,12 +201,15 @@ bool SetUpCodePairer::ConnectToDiscoveredDevice() { err = mCommissioner->EstablishPASEConnection(mRemoteId, params); } + LogErrorOnFailure(err); if (err == CHIP_NO_ERROR) { - mWaitingForConnection = true; return true; } + + // Failed to start establishing PASE. + PASEEstablishmentComplete(); } return false; @@ -284,12 +290,6 @@ void SetUpCodePairer::NotifyCommissionableDeviceDiscovered(const Dnssd::Discover bool SetUpCodePairer::TryNextRendezvousParameters() { - // Don't call PairDevice again; just do PASE as needed. If we called - // PairDevice before and PASE succeeds, the commmissioning that's been - // queued up will happen. - mConnectionType = SetupCodePairerBehaviour::kPaseOnly; - mWaitingForConnection = false; - if (ConnectToDiscoveredDevice()) { ChipLogProgress(Controller, "Trying connection to commissionee over different transport"); @@ -321,5 +321,91 @@ void SetUpCodePairer::ResetDiscoveryState() } } +void SetUpCodePairer::ExpectPASEEstablishment() +{ + mWaitingForPASE = true; + auto * delegate = mCommissioner->GetPairingDelegate(); + if (this == delegate) + { + // This should really not happen, but if it does, do nothing, to avoid + // delegate loops. + return; + } + + mPairingDelegate = delegate; + mCommissioner->RegisterPairingDelegate(this); +} + +void SetUpCodePairer::PASEEstablishmentComplete() +{ + mWaitingForPASE = false; + mCommissioner->RegisterPairingDelegate(mPairingDelegate); + mPairingDelegate = nullptr; +} + +void SetUpCodePairer::OnStatusUpdate(DevicePairingDelegate::Status status) +{ + if (mPairingDelegate) + { + mPairingDelegate->OnStatusUpdate(status); + } +} + +void SetUpCodePairer::OnPairingComplete(CHIP_ERROR error) +{ + // Save the pairing delegate so we can notify it. We want to notify it + // _after_ we restore the state on the commissioner, in case the delegate + // ends up immediately calling back into the commissioner again when + // notified. + auto * pairingDelegate = mPairingDelegate; + PASEEstablishmentComplete(); + + if (CHIP_NO_ERROR == error) + { + // StopConnectOverBle calls CancelBleIncompleteConnection, which will + // cancel connections that are in fact completed. In particular, if we + // just established PASE over BLE calling StopConnectOverBle here + // unconditionally would cancel the BLE connection underlying the PASE + // session. So make sure to only call StopConnectOverBle if we're still + // waiting to hear back on the BLE discovery bits. + if (mWaitingForDiscovery[kBLETransport]) + { + StopConnectOverBle(); + } + StopConnectOverIP(); + StopConnectOverSoftAP(); + ResetDiscoveryState(); + pairingDelegate->OnPairingComplete(error); + return; + } + + // We failed to establish PASE. Try the next thing we have discovered, if + // any. + if (TryNextRendezvousParameters()) + { + // Keep waiting until that finishes. Don't call OnPairingComplete yet. + return; + } + + pairingDelegate->OnPairingComplete(error); +} + +void SetUpCodePairer::OnPairingDeleted(CHIP_ERROR error) +{ + if (mPairingDelegate) + { + mPairingDelegate->OnPairingDeleted(error); + } +} + +void SetUpCodePairer::OnCommissioningComplete(NodeId deviceId, CHIP_ERROR error) +{ + // Not really expecting this, but handle it anyway. + if (mPairingDelegate) + { + mPairingDelegate->OnCommissioningComplete(deviceId, error); + } +} + } // namespace Controller } // namespace chip diff --git a/src/controller/SetUpCodePairer.h b/src/controller/SetUpCodePairer.h index 1f3bce6705f427..b60f94c284c139 100644 --- a/src/controller/SetUpCodePairer.h +++ b/src/controller/SetUpCodePairer.h @@ -26,6 +26,7 @@ #pragma once +#include #include #include #include @@ -50,7 +51,8 @@ enum class SetupCodePairerBehaviour : uint8_t kCommission, kPaseOnly, }; -class DLL_EXPORT SetUpCodePairer + +class DLL_EXPORT SetUpCodePairer : public DevicePairingDelegate { public: SetUpCodePairer(DeviceCommissioner * commissioner) : mCommissioner(commissioner) { ResetDiscoveryState(); } @@ -66,19 +68,13 @@ class DLL_EXPORT SetUpCodePairer void SetBleLayer(Ble::BleLayer * bleLayer) { mBleLayer = bleLayer; }; #endif // CONFIG_NETWORK_LAYER_BLE - // Called by the commissioner when PASE establishment fails. - // - // May start a new PASE establishment. - // - // Will return whether we might in fact have more rendezvous parameters to - // try (e.g. because we started a new PASE establishment or are waiting on - // more device discovery). - // - // The commissioner can use the return value to decide whether pairing has - // actually failed or not. - bool TryNextRendezvousParameters(); - private: + // DevicePairingDelegate implementation. + void OnStatusUpdate(DevicePairingDelegate::Status status) override; + void OnPairingComplete(CHIP_ERROR error) override; + void OnPairingDeleted(CHIP_ERROR error) override; + void OnCommissioningComplete(NodeId deviceId, CHIP_ERROR error) override; + CHIP_ERROR Connect(SetupPayload & paload); CHIP_ERROR StartDiscoverOverBle(SetupPayload & payload); CHIP_ERROR StopConnectOverBle(); @@ -94,6 +90,27 @@ class DLL_EXPORT SetUpCodePairer // pending work. void ResetDiscoveryState(); + // Get ready to start PASE establishment via mCommissioner. Sets up + // whatever state is needed for that. + void ExpectPASEEstablishment(); + + // PASE establishment by mCommissioner has completed: we either have a PASE + // session now or we failed to set one up, but we are done waiting on + // mCommissioner. + void PASEEstablishmentComplete(); + + // Called when PASE establishment fails. + // + // May start a new PASE establishment. + // + // Will return whether we might in fact have more rendezvous parameters to + // try (e.g. because we started a new PASE establishment or are waiting on + // more device discovery). + // + // The commissioner can use the return value to decide whether pairing has + // actually failed or not. + bool TryNextRendezvousParameters(); + // Not an enum class because we use this for indexing into arrays. enum TransportTypes { @@ -120,6 +137,11 @@ class DLL_EXPORT SetUpCodePairer uint32_t mSetUpPINCode = 0; SetupCodePairerBehaviour mConnectionType = SetupCodePairerBehaviour::kCommission; + // While we are trying to pair, we intercept the DevicePairingDelegate + // notifications from mCommissioner. We want to make sure we send them on + // to the original pairing delegate, if any. + DevicePairingDelegate * mPairingDelegate = nullptr; + // Boolean will be set to true if we currently have an async discovery // process happening via the relevant transport. bool mWaitingForDiscovery[kTransportTypeCount] = { false }; @@ -131,10 +153,10 @@ class DLL_EXPORT SetUpCodePairer // mWaitingForDiscovery are false. RendezvousParameters mDiscoveredParameters[kTransportTypeCount]; - // mWaitingForConnection is true if we have called either + // mWaitingForPASE is true if we have called either // EstablishPASEConnection or PairDevice on mCommissioner and are now just // waiting to see whether that works. - bool mWaitingForConnection = false; + bool mWaitingForPASE = false; }; } // namespace Controller