Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Terms and Conditions (T&C) Feature Support for Commissioning #36863

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion examples/chip-tool/commands/pairing/PairingCommand.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020 Project CHIP Authors
* Copyright (c) 2020-2024 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -129,6 +129,20 @@ CommissioningParameters PairingCommand::GetCommissioningParameters()
params.SetCountryCode(CharSpan::fromCharString(mCountryCode.Value()));
}

// Default requiring TCs to false, to preserve release 1.3 chip-tool behavior
params.SetRequireTermsAndConditionsAcknowledgement(mRequireTCAcknowledgements.ValueOr(false));

// mTCAcknowledgements and mTCAcknowledgementVersion are optional, but related. When one is missing, default the value to 0, to
// increase the test tools ability to test the applications.
if (mTCAcknowledgements.HasValue() || mTCAcknowledgementVersion.HasValue())
{
TermsAndConditionsAcknowledgement termsAndConditionsAcknowledgement = {
.acceptedTermsAndConditions = mTCAcknowledgements.ValueOr(0),
.acceptedTermsAndConditionsVersion = mTCAcknowledgementVersion.ValueOr(0),
};
params.SetTermsAndConditionsAcknowledgement(termsAndConditionsAcknowledgement);
}

// mTimeZoneList is an optional argument managed by TypedComplexArgument mComplex_TimeZones.
// Since optional Complex arguments are not currently supported via the <chip::Optional> class,
// we will use mTimeZoneList.data() value to determine if the argument was provided.
Expand Down
17 changes: 16 additions & 1 deletion examples/chip-tool/commands/pairing/PairingCommand.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020 Project CHIP Authors
* Copyright (c) 2020-2024 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -202,6 +202,18 @@ class PairingCommand : public CHIPCommand,
AddArgument("dst-offset", &mComplex_DSTOffsets,
"DSTOffset list to use when setting Time Synchronization cluster's DSTOffset attribute",
Argument::kOptional);

AddArgument("require-tc-acknowledgements", 0, 1, &mRequireTCAcknowledgements,
"Terms and Conditions acknowledgements is known to be required or not, (when required, the commissioner "
"will wait until they are provided)");
Comment on lines +207 to +208
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem like the right description, in that there is no waiting; when required you can provide them via the other args, right?

Also descriptions for optional args should say what the default behavior is.


AddArgument("tc-acknowledgements", 0, UINT16_MAX, &mTCAcknowledgements,
"Terms and Conditions acknowledgements to use to set the General Commissioning cluster's TC "
"Acknowledgements bit-field");

AddArgument("tc-acknowledgements-version", 0, UINT16_MAX, &mTCAcknowledgementVersion,
"Terms and Conditions acknowledgement version to use to set the General Commissioning cluster's TC "
"Acknowledgement version");
}

AddArgument("timeout", 0, UINT16_MAX, &mTimeout);
Expand Down Expand Up @@ -259,6 +271,9 @@ class PairingCommand : public CHIPCommand,
chip::Optional<uint64_t> mICDMonitoredSubject;
chip::Optional<chip::app::Clusters::IcdManagement::ClientTypeEnum> mICDClientType;
chip::Optional<uint32_t> mICDStayActiveDurationMsec;
chip::Optional<bool> mRequireTCAcknowledgements;
chip::Optional<uint16_t> mTCAcknowledgements;
chip::Optional<uint16_t> mTCAcknowledgementVersion;
chip::app::DataModel::List<chip::app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type> mTimeZoneList;
TypedComplexArgument<chip::app::DataModel::List<chip::app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type>>
mComplex_TimeZones;
Expand Down
6 changes: 5 additions & 1 deletion src/controller/AutoCommissioner.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2021 Project CHIP Authors
* Copyright (c) 2021-2024 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -363,6 +363,10 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
case CommissioningStage::kArmFailsafe:
return CommissioningStage::kConfigRegulatory;
case CommissioningStage::kConfigRegulatory:
return CommissioningStage::kGetTCAcknowledgments;
case CommissioningStage::kGetTCAcknowledgments:
return CommissioningStage::kConfigureTCAcknowledgments;
case CommissioningStage::kConfigureTCAcknowledgments:
if (mDeviceCommissioningInfo.requiresUTC)
{
return CommissioningStage::kConfigureUTCTime;
Expand Down
74 changes: 73 additions & 1 deletion src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2020-2022 Project CHIP Authors
* Copyright (c) 2020-2024 Project CHIP Authors
* Copyright (c) 2013-2017 Nest Labs, Inc.
* All rights reserved.
*
Expand Down Expand Up @@ -2795,6 +2795,22 @@ void DeviceCommissioner::OnSetRegulatoryConfigResponse(
commissioner->CommissioningStageComplete(err, report);
}

void DeviceCommissioner::OnSetTCAcknowledgementsResponse(
void * context, const GeneralCommissioning::Commands::SetTCAcknowledgementsResponse::DecodableType & data)
{
CommissioningDelegate::CommissioningReport report;
CHIP_ERROR err = CHIP_NO_ERROR;

ChipLogProgress(Controller, "Received SetTCAcknowledgements response errorCode=%u", to_underlying(data.errorCode));
if (data.errorCode != GeneralCommissioning::CommissioningErrorEnum::kOk)
{
err = CHIP_ERROR_INTERNAL;
report.Set<CommissioningErrorInfo>(data.errorCode);
}
DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
commissioner->CommissioningStageComplete(err, report);
}

void DeviceCommissioner::OnSetTimeZoneResponse(void * context,
const TimeSynchronization::Commands::SetTimeZoneResponse::DecodableType & data)
{
Expand Down Expand Up @@ -2870,6 +2886,16 @@ CHIP_ERROR DeviceCommissioner::ICDRegistrationInfoReady()
return CHIP_NO_ERROR;
}

CHIP_ERROR DeviceCommissioner::TermsAndConditionsAcknowledgementsReady()
{
VerifyOrReturnError(mCommissioningStage == CommissioningStage::kGetTCAcknowledgments, CHIP_ERROR_INCORRECT_STATE);

// need to advance to next step
CommissioningStageComplete(CHIP_NO_ERROR);

return CHIP_NO_ERROR;
}

void DeviceCommissioner::OnNetworkConfigResponse(void * context,
const NetworkCommissioning::Commands::NetworkConfigResponse::DecodableType & data)
{
Expand Down Expand Up @@ -3207,6 +3233,52 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
}
}
break;
case CommissioningStage::kGetTCAcknowledgments: {
ChipLogProgress(Controller, "Get Terms and Conditions Acknowledgments");

// If terms and conditions acknowledgements are not required, or have already been provided, then proceed
if (!params.GetRequireTermsAndConditionsAcknowledgement() || params.GetTermsAndConditionsAcknowledgement().HasValue())
{
TermsAndConditionsAcknowledgementsReady();
return;
}

ChipLogProgress(Controller, "Waiting for Terms and Conditions");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this need to extend the fail-safe or something? What happens if the user takes 10 minutes to read the terms (which is a bare minimum for how long it can possibly take to actually read those things typically)?

break;
}
case CommissioningStage::kConfigureTCAcknowledgments: {
ChipLogProgress(Controller, "Setting Terms and Conditions");

if (!params.GetRequireTermsAndConditionsAcknowledgement())
{
ChipLogProgress(Controller, "Setting Terms and Conditions: Skipped");
CommissioningStageComplete(CHIP_NO_ERROR);
return;
}

if (!params.GetTermsAndConditionsAcknowledgement().HasValue())
{
ChipLogError(Controller, "No acknowledgements provided");
CommissioningStageComplete(CHIP_ERROR_INCORRECT_STATE);
return;
}

GeneralCommissioning::Commands::SetTCAcknowledgements::Type request;
TermsAndConditionsAcknowledgement termsAndConditionsAcknowledgement = params.GetTermsAndConditionsAcknowledgement().Value();
request.TCUserResponse = termsAndConditionsAcknowledgement.acceptedTermsAndConditions;
request.TCVersion = termsAndConditionsAcknowledgement.acceptedTermsAndConditionsVersion;

ChipLogProgress(Controller, "Setting Terms and Conditions: %hu, %hu", request.TCUserResponse, request.TCVersion);
CHIP_ERROR err =
SendCommissioningCommand(proxy, request, OnSetTCAcknowledgementsResponse, OnBasicFailure, endpoint, timeout);
if (err != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Failed to send SetTCAcknowledgements command: %" CHIP_ERROR_FORMAT, err.Format());
CommissioningStageComplete(err);
return;
}
break;
}
case CommissioningStage::kSendPAICertificateRequest: {
ChipLogProgress(Controller, "Sending request for PAI certificate");
CHIP_ERROR err = SendCertificateChainRequestCommand(proxy, CertificateType::kPAI, timeout);
Expand Down
24 changes: 23 additions & 1 deletion src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2020-2022 Project CHIP Authors
* Copyright (c) 2020-2024 Project CHIP Authors
* Copyright (c) 2013-2017 Nest Labs, Inc.
* All rights reserved.
*
Expand Down Expand Up @@ -706,6 +706,25 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
*/
CHIP_ERROR ICDRegistrationInfoReady();

/**
* @brief
* This function is called by the upper layer application to indicate that the required terms and conditions
* acknowledgements have been set. This function should be called after the terms and conditions bitmask and version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Have been set" where and how?

* have been defined using the appropriate configuration macros and the application has gathered the necessary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which configuration macros, exactly?

* acknowledgements from the user.
*
* The upper layer application should call this method once it has successfully presented and obtained acknowledgements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the upper layer application know it needs to do that?

* for the required terms and conditions from the user. This indicates that the commissioning process can advance to the
* next stage.
*
* When the terms and conditions acknowledgements process is completed, this function will signal the readiness to proceed
* to the next step in the commissioning process.
*
* @return CHIP_ERROR The return status. Returns CHIP_ERROR_INCORRECT_STATE if the function is called when the device
* is not in the correct state to accept terms and conditions acknowledgements.
*/
CHIP_ERROR TermsAndConditionsAcknowledgementsReady();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other very important thing: this API surface would ensure that we can never do multiple commissionings in parallel, because nothing ties this call to a specific instance of the commissioning process. This is even an issue or "serial" commissionings: one might get canceled and another started, then this call comes in for the first one....

This needs a better solution.


/**
* @brief
* This function returns the current CommissioningStage for this commissioner.
Expand Down Expand Up @@ -960,6 +979,9 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
static void OnSetRegulatoryConfigResponse(
void * context,
const chip::app::Clusters::GeneralCommissioning::Commands::SetRegulatoryConfigResponse::DecodableType & data);
static void OnSetTCAcknowledgementsResponse(
void * context,
const chip::app::Clusters::GeneralCommissioning::Commands::SetTCAcknowledgementsResponse::DecodableType & data);
static void OnSetUTCError(void * context, CHIP_ERROR error);
static void
OnSetTimeZoneResponse(void * context,
Expand Down
6 changes: 6 additions & 0 deletions src/controller/CommissioningDelegate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ const char * StageToString(CommissioningStage stage)
case kConfigRegulatory:
return "ConfigRegulatory";

case kGetTCAcknowledgments:
return "GetTCAcknowledgments";

case kConfigureTCAcknowledgments:
return "ConfigureTCAcknowledgments";

case kConfigureUTCTime:
return "ConfigureUTCTime";

Expand Down
37 changes: 34 additions & 3 deletions src/controller/CommissioningDelegate.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2021 Project CHIP Authors
* Copyright (c) 2021-2024 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -69,7 +69,6 @@ enum CommissioningStage : uint8_t
///< Commissioning Complete command
kSendComplete, ///< Send CommissioningComplete (0x30:4) command to the device
kICDSendStayActive, ///< Send Keep Alive to ICD
kCleanup, ///< Call delegates with status, free memory, clear timers and state
/// Send ScanNetworks (0x31:0) command to the device.
/// ScanNetworks can happen anytime after kArmFailsafe.
/// However, the cirque tests fail if it is earlier in the list
Expand All @@ -81,7 +80,10 @@ enum CommissioningStage : uint8_t
kPrimaryOperationalNetworkFailed, ///< Indicate that the primary operational network (on root endpoint) failed, should remove
///< the primary network config later.
kRemoveWiFiNetworkConfig, ///< Remove Wi-Fi network config.
kRemoveThreadNetworkConfig ///< Remove Thread network config.
kRemoveThreadNetworkConfig, ///< Remove Thread network config.
kGetTCAcknowledgments, ///< Waiting for the higher layer to provide terms and conditions acknowledgements.
kConfigureTCAcknowledgments, ///< Send SetTCAcknowledgements (0x30:6) command to the device
kCleanup, ///< Call delegates with status, free memory, clear timers and state
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments above claim this value cannot be moved later in the list. Either it can and the comments need fixing, or it cannot and should not be.

};

enum class ICDRegistrationStrategy : uint8_t
Expand All @@ -104,6 +106,12 @@ struct WiFiCredentials
WiFiCredentials(ByteSpan newSsid, ByteSpan newCreds) : ssid(newSsid), credentials(newCreds) {}
};

struct TermsAndConditionsAcknowledgement
{
uint16_t acceptedTermsAndConditions;
uint16_t acceptedTermsAndConditionsVersion;
};

struct NOCChainGenerationParameters
{
ByteSpan nocsrElements;
Expand Down Expand Up @@ -168,6 +176,13 @@ class CommissioningParameters
// The country code to be used for the node, if set.
Optional<CharSpan> GetCountryCode() const { return mCountryCode; }

bool GetRequireTermsAndConditionsAcknowledgement() const { return mRequireTermsAndConditionsAcknowledgement; }

Optional<TermsAndConditionsAcknowledgement> GetTermsAndConditionsAcknowledgement() const
{
return mTermsAndConditionsAcknowledgement;
}

// Time zone to set for the node
// If required, this will be truncated to fit the max size allowable on the node
Optional<app::DataModel::List<app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type>> GetTimeZone() const
Expand Down Expand Up @@ -340,6 +355,19 @@ class CommissioningParameters
return *this;
}

CommissioningParameters & SetRequireTermsAndConditionsAcknowledgement(bool requireTermsAndConditionsAcknowledgement)
{
mRequireTermsAndConditionsAcknowledgement = requireTermsAndConditionsAcknowledgement;
return *this;
}

CommissioningParameters &
SetTermsAndConditionsAcknowledgement(TermsAndConditionsAcknowledgement termsAndConditionsAcknowledgement)
{
mTermsAndConditionsAcknowledgement.SetValue(termsAndConditionsAcknowledgement);
return *this;
}

// The lifetime of the list buffer needs to exceed the lifetime of the CommissioningParameters object.
CommissioningParameters &
SetTimeZone(app::DataModel::List<app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type> timeZone)
Expand Down Expand Up @@ -581,6 +609,7 @@ class CommissioningParameters
mAttestationNonce.ClearValue();
mWiFiCreds.ClearValue();
mCountryCode.ClearValue();
mTermsAndConditionsAcknowledgement.ClearValue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This is not an "ExternalBufferDependentValue", is it?

mThreadOperationalDataset.ClearValue();
mNOCChainGenerationParameters.ClearValue();
mRootCert.ClearValue();
Expand Down Expand Up @@ -611,6 +640,7 @@ class CommissioningParameters
Optional<ByteSpan> mAttestationNonce;
Optional<WiFiCredentials> mWiFiCreds;
Optional<CharSpan> mCountryCode;
Optional<TermsAndConditionsAcknowledgement> mTermsAndConditionsAcknowledgement;
Optional<ByteSpan> mThreadOperationalDataset;
Optional<NOCChainGenerationParameters> mNOCChainGenerationParameters;
Optional<ByteSpan> mRootCert;
Expand Down Expand Up @@ -643,6 +673,7 @@ class CommissioningParameters
Optional<uint32_t> mICDStayActiveDurationMsec;
ICDRegistrationStrategy mICDRegistrationStrategy = ICDRegistrationStrategy::kIgnore;
bool mCheckForMatchingFabric = false;
bool mRequireTermsAndConditionsAcknowledgement = false;
};

struct RequestedCertificate
Expand Down
25 changes: 25 additions & 0 deletions src/controller/python/ChipDeviceController-ScriptBinding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,12 @@ PyChipError pychip_DeviceController_OpenCommissioningWindow(chip::Controller::De
bool pychip_DeviceController_GetIPForDiscoveredDevice(chip::Controller::DeviceCommissioner * devCtrl, int idx, char * addrStr,
uint32_t len);

PyChipError pychip_DeviceController_SetRequireTermsAndConditionsAcknowledgement(bool tcRequired);

PyChipError pychip_DeviceController_SetTermsAcknowledgements(uint16_t tcVersion, uint16_t tcUserResponse);

PyChipError pychip_DeviceController_SetSkipCommissioningComplete(bool skipCommissioningComplete);

// Pairing Delegate
PyChipError
pychip_ScriptDevicePairingDelegate_SetKeyExchangeCallback(chip::Controller::ScriptDevicePairingDelegate * pairingDelegate,
Expand Down Expand Up @@ -572,6 +578,25 @@ PyChipError pychip_DeviceController_SetDefaultNtp(const char * defaultNTP)
return ToPyChipError(CHIP_NO_ERROR);
}

PyChipError pychip_DeviceController_SetRequireTermsAndConditionsAcknowledgement(bool tcRequired)
{
sCommissioningParameters.SetRequireTermsAndConditionsAcknowledgement(tcRequired);
return ToPyChipError(CHIP_NO_ERROR);
}

PyChipError pychip_DeviceController_SetTermsAcknowledgements(uint16_t tcVersion, uint16_t tcUserResponse)
{
sCommissioningParameters.SetTermsAndConditionsAcknowledgement(
{ .acceptedTermsAndConditions = tcUserResponse, .acceptedTermsAndConditionsVersion = tcVersion });
return ToPyChipError(CHIP_NO_ERROR);
}

PyChipError pychip_DeviceController_SetSkipCommissioningComplete(bool skipCommissioningComplete)
{
sCommissioningParameters.SetSkipCommissioningComplete(skipCommissioningComplete);
return ToPyChipError(CHIP_NO_ERROR);
}

PyChipError pychip_DeviceController_SetTrustedTimeSource(chip::NodeId nodeId, chip::EndpointId endpoint)
{
chip::app::Clusters::TimeSynchronization::Structs::FabricScopedTrustedTimeSourceStruct::Type timeSource = { .nodeID = nodeId,
Expand Down
Loading
Loading