Skip to content

Commit

Permalink
[Fabric-Admin] Refactor to use API methods instead of PushCommand (2/…
Browse files Browse the repository at this point in the history 
…3) (#35867)

* Refactor to use API methods instead of PushCommand for pairing

* Restyled by whitespace

* Restyled by clang-format

* Schedule work on the Matter thread

* Update examples/fabric-admin/device_manager/PairingManager.cpp

Co-authored-by: Terence Hampson <[email protected]>

* Update examples/fabric-admin/device_manager/PairingManager.cpp

Co-authored-by: Terence Hampson <[email protected]>

* Update examples/fabric-admin/device_manager/PairingManager.cpp

Co-authored-by: Terence Hampson <[email protected]>

* Update examples/fabric-admin/device_manager/PairingManager.cpp

Co-authored-by: Terence Hampson <[email protected]>

* Address review comments

* Cleanup CCTRL attestation logic from PairingCommand

---------

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Terence Hampson <[email protected]>
  • Loading branch information
@yufengwangca @restyled-commits
@tehampson @pull
3 people authored and pull[bot] committed Nov 11, 2024
1 parent 14527cc commit 88c37d1
Show file tree
Hide file tree
Showing 10 changed files with 711 additions and 242 deletions.
2 changes: 1 addition & 1 deletion examples/fabric-admin/commands/common/CHIPCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack()
mCredIssuerCmds->SetCredentialIssuerOption(CredentialIssuerCommands::CredentialIssuerOptions::kAllowTestCdSigningKey,
allowTestCdSigningKey);

PairingManager::Instance().Init(&CurrentCommissioner());
ReturnLogErrorOnFailure(PairingManager::Instance().Init(&CurrentCommissioner(), mCredIssuerCmds));

return CHIP_NO_ERROR;
}
Expand Down
3 changes: 2 additions & 1 deletion examples/fabric-admin/commands/common/CHIPCommand.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ class CHIPCommand : public Command
StopWaiting();
}

static chip::app::DefaultICDClientStorage sICDClientStorage;

protected:
// Will be called in a setting in which it's safe to touch the CHIP
// stack. The rules for Run() are as follows:
Expand Down Expand Up @@ -167,7 +169,6 @@ class CHIPCommand : public Command
static chip::Crypto::RawKeySessionKeystore sSessionKeystore;

static chip::Credentials::GroupDataProviderImpl sGroupDataProvider;
static chip::app::DefaultICDClientStorage sICDClientStorage;
static chip::app::CheckInHandler sCheckInHandler;
CredentialIssuerCommands * mCredIssuerCmds;

Expand Down
46 changes: 5 additions & 41 deletions examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,8 @@ CHIP_ERROR FabricSyncAddBridgeCommand::RunCommand(NodeId remoteId)
return CHIP_NO_ERROR;
}

PairingCommand * pairingCommand = static_cast<PairingCommand *>(CommandMgr().GetCommandByName("pairing", "already-discovered"));
PairingManager::Instance().SetCommissioningDelegate(this);

if (pairingCommand == nullptr)
{
ChipLogError(NotSpecified, "Pairing already-discovered command is not available");
return CHIP_ERROR_NOT_IMPLEMENTED;
}

pairingCommand->RegisterCommissioningDelegate(this);
mBridgeNodeId = remoteId;

DeviceMgr().PairRemoteFabricBridge(remoteId, mSetupPINCode, reinterpret_cast<const char *>(mRemoteAddr.data()), mRemotePort);
Expand Down Expand Up @@ -146,16 +139,7 @@ CHIP_ERROR FabricSyncRemoveBridgeCommand::RunCommand()

mBridgeNodeId = bridgeNodeId;

PairingCommand * pairingCommand = static_cast<PairingCommand *>(CommandMgr().GetCommandByName("pairing", "unpair"));

if (pairingCommand == nullptr)
{
ChipLogError(NotSpecified, "Pairing unpair command is not available");
return CHIP_ERROR_NOT_IMPLEMENTED;
}

pairingCommand->RegisterPairingDelegate(this);

PairingManager::Instance().SetPairingDelegate(this);
DeviceMgr().UnpairRemoteFabricBridge();

return CHIP_NO_ERROR;
Expand Down Expand Up @@ -203,10 +187,7 @@ CHIP_ERROR FabricSyncAddLocalBridgeCommand::RunCommand(NodeId deviceId)
return CHIP_NO_ERROR;
}

PairingCommand * pairingCommand = static_cast<PairingCommand *>(CommandMgr().GetCommandByName("pairing", "already-discovered"));
VerifyOrDie(pairingCommand != nullptr);

pairingCommand->RegisterCommissioningDelegate(this);
PairingManager::Instance().SetCommissioningDelegate(this);
mLocalBridgeNodeId = deviceId;

if (mSetupPINCode.HasValue())
Expand Down Expand Up @@ -259,16 +240,7 @@ CHIP_ERROR FabricSyncRemoveLocalBridgeCommand::RunCommand()

mLocalBridgeNodeId = bridgeNodeId;

PairingCommand * pairingCommand = static_cast<PairingCommand *>(CommandMgr().GetCommandByName("pairing", "unpair"));

if (pairingCommand == nullptr)
{
ChipLogError(NotSpecified, "Pairing unpair command is not available");
return CHIP_ERROR_NOT_IMPLEMENTED;
}

pairingCommand->RegisterPairingDelegate(this);

PairingManager::Instance().SetPairingDelegate(this);
DeviceMgr().UnpairLocalFabricBridge();

return CHIP_NO_ERROR;
Expand All @@ -287,15 +259,7 @@ void FabricSyncDeviceCommand::OnCommissioningWindowOpened(NodeId deviceId, CHIP_
{
NodeId nodeId = DeviceMgr().GetNextAvailableNodeId();

PairingCommand * pairingCommand = static_cast<PairingCommand *>(CommandMgr().GetCommandByName("pairing", "code"));

if (pairingCommand == nullptr)
{
ChipLogError(NotSpecified, "Pairing code command is not available");
return;
}

pairingCommand->RegisterCommissioningDelegate(this);
PairingManager::Instance().SetCommissioningDelegate(this);
mAssignedNodeId = nodeId;

usleep(kCommissionPrepareTimeMs * 1000);
Expand Down
2 changes: 0 additions & 2 deletions examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,10 @@
#pragma once

#include <commands/common/CHIPCommand.h>
#include <commands/pairing/PairingCommand.h>
#include <device_manager/PairingManager.h>

// Constants
constexpr uint32_t kCommissionPrepareTimeMs = 500;
constexpr uint16_t kMaxManualCodeLength = 21;

class FabricSyncAddBridgeCommand : public CHIPCommand, public CommissioningDelegate
{
Expand Down
111 changes: 7 additions & 104 deletions examples/fabric-admin/commands/pairing/PairingCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#include <protocols/secure_channel/PASESession.h>
#include <setup_payload/ManualSetupPayloadParser.h>
#include <setup_payload/QRCodeSetupPayloadParser.h>
#include <setup_payload/SetupPayload.h>

#include <string>

Expand All @@ -41,28 +40,6 @@
using namespace ::chip;
using namespace ::chip::Controller;

namespace {

CHIP_ERROR GetPayload(const char * setUpCode, SetupPayload & payload)
{
VerifyOrReturnValue(setUpCode, CHIP_ERROR_INVALID_ARGUMENT);
bool isQRCode = strncmp(setUpCode, kQRCodePrefix, strlen(kQRCodePrefix)) == 0;
if (isQRCode)
{
ReturnErrorOnFailure(QRCodeSetupPayloadParser(setUpCode).populatePayload(payload));
VerifyOrReturnError(payload.isValidQRCodePayload(), CHIP_ERROR_INVALID_ARGUMENT);
}
else
{
ReturnErrorOnFailure(ManualSetupPayloadParser(setUpCode).populatePayload(payload));
VerifyOrReturnError(payload.isValidManualCode(), CHIP_ERROR_INVALID_ARGUMENT);
}

return CHIP_NO_ERROR;
}

} // namespace

CHIP_ERROR PairingCommand::RunCommand()
{
CurrentCommissioner().RegisterPairingDelegate(this);
Expand Down Expand Up @@ -128,7 +105,10 @@ CommissioningParameters PairingCommand::GetCommissioningParameters()
{
auto params = CommissioningParameters();
params.SetSkipCommissioningComplete(mSkipCommissioningComplete.ValueOr(false));
params.SetDeviceAttestationDelegate(this);
if (mBypassAttestationVerifier.ValueOr(false))
{
params.SetDeviceAttestationDelegate(this);
}

switch (mNetworkType)
{
Expand Down Expand Up @@ -442,12 +422,6 @@ void PairingCommand::OnCommissioningComplete(NodeId nodeId, CHIP_ERROR err)
ChipLogProgress(NotSpecified, "Device commissioning Failure: %s", ErrorStr(err));
}

if (mCommissioningDelegate)
{
mCommissioningDelegate->OnCommissioningComplete(nodeId, err);
this->UnregisterCommissioningDelegate();
}

SetCommandExitStatus(err);
}

Expand Down Expand Up @@ -576,13 +550,6 @@ void PairingCommand::OnCurrentFabricRemove(void * context, NodeId nodeId, CHIP_E
ChipLogProgress(NotSpecified, "Device unpair Failure: " ChipLogFormatX64 " %s", ChipLogValueX64(nodeId), ErrorStr(err));
}

PairingDelegate * pairingDelegate = command->GetPairingDelegate();
if (pairingDelegate)
{
pairingDelegate->OnDeviceRemoved(nodeId, err);
command->UnregisterPairingDelegate();
}

command->SetCommandExitStatus(err);
}

Expand All @@ -592,77 +559,13 @@ Optional<uint16_t> PairingCommand::FailSafeExpiryTimeoutSecs() const
return Optional<uint16_t>();
}

bool PairingCommand::ShouldWaitAfterDeviceAttestation()
{
// If there is a vendor ID and product ID, request OnDeviceAttestationCompleted().
// Currently this is added in the case that the example is performing reverse commissioning,
// but it would be an improvement to store that explicitly.
// TODO: Issue #35297 - [Fabric Sync] Improve where we get VID and PID when validating CCTRL CommissionNode command
SetupPayload payload;
CHIP_ERROR err = GetPayload(mOnboardingPayload, payload);
return err == CHIP_NO_ERROR && (payload.vendorID != 0 || payload.productID != 0);
}

void PairingCommand::OnDeviceAttestationCompleted(Controller::DeviceCommissioner * deviceCommissioner, DeviceProxy * device,
const Credentials::DeviceAttestationVerifier::AttestationDeviceInfo & info,
Credentials::AttestationVerificationResult attestationResult)
{
SetupPayload payload;
CHIP_ERROR parse_error = GetPayload(mOnboardingPayload, payload);
if (parse_error == CHIP_NO_ERROR && (payload.vendorID != 0 || payload.productID != 0))
{
if (payload.vendorID == 0 || payload.productID == 0)
{
ChipLogProgress(NotSpecified,
"Failed validation: vendorID or productID must not be 0."
"Requested VID: %u, Requested PID: %u.",
payload.vendorID, payload.productID);
deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(
device, Credentials::AttestationVerificationResult::kInvalidArgument);
return;
}

if (payload.vendorID != info.BasicInformationVendorId() || payload.productID != info.BasicInformationProductId())
{
ChipLogProgress(NotSpecified,
"Failed validation of vendorID or productID."
"Requested VID: %u, Requested PID: %u,"
"Detected VID: %u, Detected PID %u.",
payload.vendorID, payload.productID, info.BasicInformationVendorId(), info.BasicInformationProductId());
deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(
device,
payload.vendorID == info.BasicInformationVendorId()
? Credentials::AttestationVerificationResult::kDacProductIdMismatch
: Credentials::AttestationVerificationResult::kDacVendorIdMismatch);
return;
}

// NOTE: This will log errors even if the attestion was successful.
auto err = deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(device, attestationResult);
if (CHIP_NO_ERROR != err)
{
SetCommandExitStatus(err);
}
return;
}

// OnDeviceAttestationCompleted() is called if ShouldWaitAfterDeviceAttestation() returns true
// or if there is an attestation error. The conditions for ShouldWaitAfterDeviceAttestation() have
// already been checked, so the call to OnDeviceAttestationCompleted() was an error.
if (mBypassAttestationVerifier.ValueOr(false))
{
// Bypass attestation verification, continue with success
auto err = deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(
device, Credentials::AttestationVerificationResult::kSuccess);
if (CHIP_NO_ERROR != err)
{
SetCommandExitStatus(err);
}
return;
}

// Don't bypass attestation, continue with error.
auto err = deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(device, attestationResult);
// Bypass attestation verification, continue with success
auto err = deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(
device, Credentials::AttestationVerificationResult::kSuccess);
if (CHIP_NO_ERROR != err)
{
SetCommandExitStatus(err);
Expand Down
27 changes: 0 additions & 27 deletions examples/fabric-admin/commands/pairing/PairingCommand.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,6 @@ enum class PairingNetworkType
Thread,
};

class CommissioningDelegate
{
public:
virtual void OnCommissioningComplete(chip::NodeId deviceId, CHIP_ERROR err) = 0;
virtual ~CommissioningDelegate() = default;
};

class PairingDelegate
{
public:
virtual void OnDeviceRemoved(chip::NodeId deviceId, CHIP_ERROR err) = 0;
virtual ~PairingDelegate() = default;
};

class PairingCommand : public CHIPCommand,
public chip::Controller::DevicePairingDelegate,
public chip::Controller::DeviceDiscoveryDelegate,
Expand Down Expand Up @@ -221,20 +207,10 @@ class PairingCommand : public CHIPCommand,

/////////// DeviceAttestationDelegate Interface /////////
chip::Optional<uint16_t> FailSafeExpiryTimeoutSecs() const override;
bool ShouldWaitAfterDeviceAttestation() override;
void OnDeviceAttestationCompleted(chip::Controller::DeviceCommissioner * deviceCommissioner, chip::DeviceProxy * device,
const chip::Credentials::DeviceAttestationVerifier::AttestationDeviceInfo & info,
chip::Credentials::AttestationVerificationResult attestationResult) override;

/////////// CommissioningDelegate /////////
void RegisterCommissioningDelegate(CommissioningDelegate * delegate) { mCommissioningDelegate = delegate; }
void UnregisterCommissioningDelegate() { mCommissioningDelegate = nullptr; }

/////////// PairingDelegate /////////
void RegisterPairingDelegate(PairingDelegate * delegate) { mPairingDelegate = delegate; }
void UnregisterPairingDelegate() { mPairingDelegate = nullptr; }
PairingDelegate * GetPairingDelegate() { return mPairingDelegate; }

private:
CHIP_ERROR RunInternal(NodeId remoteId);
CHIP_ERROR Pair(NodeId remoteId, PeerAddress address);
Expand Down Expand Up @@ -290,9 +266,6 @@ class PairingCommand : public CHIPCommand,
chip::Platform::UniquePtr<chip::Controller::CurrentFabricRemover> mCurrentFabricRemover;
chip::Callback::Callback<chip::Controller::OnCurrentFabricRemove> mCurrentFabricRemoveCallback;

CommissioningDelegate * mCommissioningDelegate = nullptr;
PairingDelegate * mPairingDelegate = nullptr;

static void OnCurrentFabricRemove(void * context, NodeId remoteNodeId, CHIP_ERROR status);
void PersistIcdInfo();
};
Loading

0 comments on commit 88c37d1

Please sign in to comment.