From 0b8ffb7c3f6a7a09e9294a00994cb90571808089 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 18 Sep 2024 14:53:18 -0400 Subject: [PATCH 1/3] Make use of DataModel::Provider in writes (#34754) * Implement DM::Provider::Write usage * Fix compile * Fix java builds * Update src/app/InteractionModelEngine.cpp Co-authored-by: Boris Zbarsky * Make codegen data model call increasing the cluster data version * Restyle * Make sure that attribute changed information can be propagated out of ember * Optimize storage size of WriteHandler * Restyle * Make the code more obvious identical with what was there before * Change Provider to not double-call the dirty and version increase * Update src/app/InteractionModelEngine.cpp Co-authored-by: Boris Zbarsky * Cleaner usage: no need of a separate function that is used in one place only * Attempt an API update * Fix typos in the Accessors src * Fix typo and regen * More fixes on accessors * Update signature for emAfWriteAttributeExternal * Add a comment about all the checks being vague * Update src/app/util/af-types.h Co-authored-by: Boris Zbarsky * Update src/app/util/af-types.h Co-authored-by: Boris Zbarsky * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky * Update src/app/util/mock/CodegenEmberMocks.cpp Co-authored-by: Boris Zbarsky * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky * zap regen and restyle * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky * Update src/app/util/attribute-table.cpp Co-authored-by: Boris Zbarsky * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky * Update src/app/util/attribute-table.cpp Co-authored-by: Boris Zbarsky * Update src/app/util/attribute-table.cpp Co-authored-by: Boris Zbarsky * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky * Update src/app/util/attribute-table.h Co-authored-by: Boris Zbarsky * Rename ChangedPathListener to AttributesChangedListener * Remove chip:: and chip::app * Update constructors of AttributePathParams and add nodiscard according to the linter to never call const methods without considering their return value * Restyled by clang-format * Remove auto-inserted include * Update again and zap regen: removed extra namespace prefixes in accessors.h/cpp * Add comment about uint8_t non-const usage... * Another rename given that the listener is now an attributes and not a path listener * Update src/app/util/attribute-table.h Co-authored-by: Tennessee Carmel-Veilleux * Update after merge to have more things compile * Everything compiles now * Restyle * Make unit tests pass: mocks also have to call the change listeners * Comment update to talk more about AttributesChangedListener * Restyle * Add support for a previous path write ... this is similar to what ACL caching and tokenizing currently does, but in a explicit manner describing the ACL use case * Remove some extra added includes * Another include fix * Follow the comment and do not restrict the cache to ACL cluster, since this is what current ember does * More self code review changes * Make tests pass, more code cleanup * Match ordering to ember-compatibility functions. This is ODD because we check attribute existance before access! * Adjust test ordering and add a large note that we are probably doing the wrong thing, however tests force us to do the wrong thing * Fix unit test * Remove odd comment * Add a few ending endifs * Renamed ScopedExchangeContext * Update src/app/InteractionModelEngine.h Co-authored-by: Boris Zbarsky * Update src/app/WriteHandler.h Co-authored-by: Boris Zbarsky * Update src/app/WriteHandler.h Co-authored-by: Boris Zbarsky * Update src/app/data-model-provider/OperationTypes.h Co-authored-by: Boris Zbarsky * Update src/app/data-model-provider/OperationTypes.h Co-authored-by: Boris Zbarsky * Update src/app/data-model-provider/OperationTypes.h Co-authored-by: Boris Zbarsky * Update src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp Co-authored-by: Boris Zbarsky * Restyle to reorder includes * Add issue link * Update equality logic * Rename member to mLastSuccessfullyWrittenPath * Update argument logic * Make ActionContext private in IME so it is not such a public API * Clean up comments * fix up compares * Restyle --------- Co-authored-by: Andrei Litvin Co-authored-by: Boris Zbarsky Co-authored-by: Restyled.io Co-authored-by: Tennessee Carmel-Veilleux --- src/app/AttributeValueDecoder.h | 2 + src/app/EventManagement.cpp | 7 + src/app/EventManagement.h | 8 +- src/app/InteractionModelEngine.cpp | 43 +++++- src/app/InteractionModelEngine.h | 27 +++- src/app/WriteHandler.cpp | 52 ++++++- src/app/WriteHandler.h | 14 +- .../CodegenDataModelProvider_Write.cpp | 119 ++++++++++----- .../tests/EmberReadWriteOverride.cpp | 13 ++ .../tests/TestCodegenModelViaMocks.cpp | 56 +++++-- src/app/data-model-provider/OperationTypes.h | 15 +- .../ProviderChangeListener.h | 4 +- src/app/reporting/Engine.cpp | 12 +- src/app/reporting/Engine.h | 6 +- src/app/tests/TestWriteInteraction.cpp | 11 +- src/app/tests/test-interaction-model-api.cpp | 28 +++- src/app/util/mock/CodegenEmberMocks.cpp | 8 +- src/app/util/mock/attribute-storage.cpp | 7 + .../tests/data_model/DataModelFixtures.cpp | 141 +++++++++++++++++- src/controller/tests/data_model/TestWrite.cpp | 17 ++- 20 files changed, 513 insertions(+), 77 deletions(-) diff --git a/src/app/AttributeValueDecoder.h b/src/app/AttributeValueDecoder.h index 5f5e2255b2fa3e..79a749bbb812b4 100644 --- a/src/app/AttributeValueDecoder.h +++ b/src/app/AttributeValueDecoder.h @@ -65,6 +65,8 @@ class AttributeValueDecoder const Access::SubjectDescriptor & GetSubjectDescriptor() const { return mSubjectDescriptor; } private: + friend class TestOnlyAttributeValueDecoderAccessor; + TLV::TLVReader & mReader; bool mTriedDecode = false; const Access::SubjectDescriptor mSubjectDescriptor; diff --git a/src/app/EventManagement.cpp b/src/app/EventManagement.cpp index 7c0df844872b47..271f72ec107f3b 100644 --- a/src/app/EventManagement.cpp +++ b/src/app/EventManagement.cpp @@ -855,6 +855,12 @@ void EventManagement::SetScheduledEventInfo(EventNumber & aEventNumber, uint32_t aInitialWrittenEventBytes = mBytesWritten; } +CHIP_ERROR EventManagement::GenerateEvent(EventLoggingDelegate * eventPayloadWriter, const EventOptions & options, + EventNumber & generatedEventNumber) +{ + return LogEvent(eventPayloadWriter, options, generatedEventNumber); +} + void CircularEventBuffer::Init(uint8_t * apBuffer, uint32_t aBufferLength, CircularEventBuffer * apPrev, CircularEventBuffer * apNext, PriorityLevel aPriorityLevel) { @@ -914,5 +920,6 @@ CHIP_ERROR CircularEventBufferWrapper::GetNextBuffer(TLVReader & aReader, const exit: return err; } + } // namespace app } // namespace chip diff --git a/src/app/EventManagement.h b/src/app/EventManagement.h index ce5a34039ea0fe..76220350fc2507 100644 --- a/src/app/EventManagement.h +++ b/src/app/EventManagement.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -196,7 +197,7 @@ struct LogStorageResources * more space for new events. */ -class EventManagement +class EventManagement : public DataModel::EventsGenerator { public: /** @@ -387,6 +388,10 @@ class EventManagement */ void SetScheduledEventInfo(EventNumber & aEventNumber, uint32_t & aInitialWrittenEventBytes) const; + /* EventsGenerator implementation */ + CHIP_ERROR GenerateEvent(EventLoggingDelegate * eventPayloadWriter, const EventOptions & options, + EventNumber & generatedEventNumber) override; + private: /** * @brief @@ -559,5 +564,6 @@ class EventManagement System::Clock::Milliseconds64 mMonotonicStartupTime; }; + } // namespace app } // namespace chip diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 9b4282fb036d88..338e1d1f6dce8c 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -895,7 +895,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnWriteRequest(Messa { if (writeHandler.IsFree()) { - VerifyOrReturnError(writeHandler.Init(this) == CHIP_NO_ERROR, Status::Busy); + VerifyOrReturnError(writeHandler.Init(GetDataModelProvider(), this) == CHIP_NO_ERROR, Status::Busy); return writeHandler.OnWriteRequest(apExchangeContext, std::move(aPayload), aIsTimedWrite); } } @@ -996,6 +996,9 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext Protocols::InteractionModel::Status status = Status::Failure; + // Ensure that DataModel::Provider has access to the exchange the message was received on. + CurrentExchangeValueScope scopedExchangeContext(*this, apExchangeContext); + // Group Message can only be an InvokeCommandRequest or WriteRequest if (apExchangeContext->IsGroupExchangeContext() && !aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandRequest) && @@ -1749,16 +1752,44 @@ DataModel::Provider * InteractionModelEngine::SetDataModelProvider(DataModel::Pr // Alternting data model should not be done while IM is actively handling requests. VerifyOrDie(mReadHandlers.begin() == mReadHandlers.end()); - DataModel::Provider * oldModel = GetDataModelProvider(); - mDataModelProvider = model; + DataModel::Provider * oldModel = mDataModelProvider; + if (oldModel != nullptr) + { + CHIP_ERROR err = oldModel->Shutdown(); + if (err != CHIP_NO_ERROR) + { + ChipLogError(InteractionModel, "Failure on interaction model shutdown: %" CHIP_ERROR_FORMAT, err.Format()); + } + } + + mDataModelProvider = model; + if (mDataModelProvider != nullptr) + { + DataModel::InteractionModelContext context; + + context.eventsGenerator = &EventManagement::GetInstance(); + context.dataModelChangeListener = &mReportingEngine; + context.actionContext = this; + + CHIP_ERROR err = mDataModelProvider->Startup(context); + if (err != CHIP_NO_ERROR) + { + ChipLogError(InteractionModel, "Failure on interaction model startup: %" CHIP_ERROR_FORMAT, err.Format()); + } + } + return oldModel; } -DataModel::Provider * InteractionModelEngine::GetDataModelProvider() const +DataModel::Provider * InteractionModelEngine::GetDataModelProvider() { #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE - // TODO: this should be temporary, we should fully inject the data model - VerifyOrReturnValue(mDataModelProvider != nullptr, CodegenDataModelProviderInstance()); + if (mDataModelProvider == nullptr) + { + // These should be called within the CHIP processing loop. + assertChipStackLockedByCurrentThread(); + SetDataModelProvider(CodegenDataModelProviderInstance()); + } #endif return mDataModelProvider; } diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index c371bad23b2d2c..8b7bd0743782cd 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -86,6 +86,7 @@ namespace app { */ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, public Messaging::ExchangeDelegate, + private DataModel::ActionContext, public CommandResponseSender::Callback, public CommandHandlerImpl::Callback, public ReadHandler::ManagementCallback, @@ -402,7 +403,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, } #endif - DataModel::Provider * GetDataModelProvider() const; + // Temporarily NOT const because the data model provider will be auto-set + // to codegen on first usage. This behaviour will be changed once each + // application must explicitly set the data model provider. + DataModel::Provider * GetDataModelProvider(); // MUST NOT be used while the interaction model engine is running as interaction // model functionality (e.g. active reads/writes/subscriptions) rely on data model @@ -412,6 +416,9 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, DataModel::Provider * SetDataModelProvider(DataModel::Provider * model); private: + /* DataModel::ActionContext implementation */ + Messaging::ExchangeContext * CurrentExchange() override { return mCurrentExchange; } + friend class reporting::Engine; friend class TestCommandInteraction; friend class TestInteractionModelEngine; @@ -698,7 +705,23 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, SubscriptionResumptionStorage * mpSubscriptionResumptionStorage = nullptr; - DataModel::Provider * mDataModelProvider = nullptr; + DataModel::Provider * mDataModelProvider = nullptr; + Messaging::ExchangeContext * mCurrentExchange = nullptr; + + // Changes the current exchange context of a InteractionModelEngine to a given context + class CurrentExchangeValueScope + { + public: + CurrentExchangeValueScope(InteractionModelEngine & engine, Messaging::ExchangeContext * context) : mEngine(engine) + { + mEngine.mCurrentExchange = context; + } + + ~CurrentExchangeValueScope() { mEngine.mCurrentExchange = nullptr; } + + private: + InteractionModelEngine & mEngine; + }; }; } // namespace app diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index 15776eb545fc21..5050cbbee2fcb5 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -18,19 +18,25 @@ #include #include +#include #include #include #include #include #include +#include #include #include #include #include +#include #include #include +#include #include +#include + namespace chip { namespace app { @@ -38,10 +44,14 @@ using namespace Protocols::InteractionModel; using Status = Protocols::InteractionModel::Status; constexpr uint8_t kListAttributeType = 0x48; -CHIP_ERROR WriteHandler::Init(WriteHandlerDelegate * apWriteHandlerDelegate) +CHIP_ERROR WriteHandler::Init(DataModel::Provider * apProvider, WriteHandlerDelegate * apWriteHandlerDelegate) { VerifyOrReturnError(!mExchangeCtx, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(apWriteHandlerDelegate, CHIP_ERROR_INVALID_ARGUMENT); +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + VerifyOrReturnError(apProvider, CHIP_ERROR_INVALID_ARGUMENT); + mDataModelProvider = apProvider; +#endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE mDelegate = apWriteHandlerDelegate; MoveToState(State::Initialized); @@ -63,6 +73,9 @@ void WriteHandler::Close() DeliverFinalListWriteEnd(false /* wasSuccessful */); mExchangeCtx.Release(); mStateFlags.Clear(StateBits::kSuppressResponse); +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + mDataModelProvider = nullptr; +#endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE MoveToState(State::Uninitialized); } @@ -354,7 +367,7 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData err = CHIP_NO_ERROR; } SuccessOrExit(err); - err = WriteSingleClusterData(subjectDescriptor, dataAttributePath, dataReader, this); + err = WriteClusterData(subjectDescriptor, dataAttributePath, dataReader); if (err != CHIP_NO_ERROR) { mWriteResponseBuilder.GetWriteResponses().Rollback(backup); @@ -501,7 +514,7 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write, DataModelCallbacks::OperationOrder::Pre, dataAttributePath); - err = WriteSingleClusterData(subjectDescriptor, dataAttributePath, tmpDataReader, this); + err = WriteClusterData(subjectDescriptor, dataAttributePath, tmpDataReader); if (err != CHIP_NO_ERROR) { ChipLogError(DataManagement, @@ -552,6 +565,10 @@ Status WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload, // our callees hand out Status as well. Status status = Status::InvalidAction; +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + mLastSuccessfullyWrittenPath = std::nullopt; +#endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + reader.Init(std::move(aPayload)); err = writeRequestParser.Init(reader); @@ -559,7 +576,7 @@ Status WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload, #if CHIP_CONFIG_IM_PRETTY_PRINT writeRequestParser.PrettyPrint(); -#endif +#endif // CHIP_CONFIG_IM_PRETTY_PRINT bool boolValue; boolValue = mStateFlags.Has(StateBits::kSuppressResponse); @@ -703,5 +720,32 @@ void WriteHandler::MoveToState(const State aTargetState) ChipLogDetail(DataManagement, "IM WH moving to [%s]", GetStateStr()); } +CHIP_ERROR WriteHandler::WriteClusterData(const Access::SubjectDescriptor & aSubject, const ConcreteDataAttributePath & aPath, + TLV::TLVReader & aData) +{ + // Writes do not have a checked-path. If data model interface is enabled (both checked and only version) + // the write is done via the DataModel interface +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + VerifyOrReturnError(mDataModelProvider != nullptr, CHIP_ERROR_INCORRECT_STATE); + + DataModel::WriteAttributeRequest request; + + request.path = aPath; + request.subjectDescriptor = aSubject; + request.previousSuccessPath = mLastSuccessfullyWrittenPath; + request.writeFlags.Set(DataModel::WriteFlags::kTimed, IsTimedWrite()); + + AttributeValueDecoder decoder(aData, aSubject); + + DataModel::ActionReturnStatus status = mDataModelProvider->WriteAttribute(request, decoder); + + mLastSuccessfullyWrittenPath = status.IsSuccess() ? std::make_optional(aPath) : std::nullopt; + + return AddStatusInternal(aPath, StatusIB(status.GetStatusCode())); +#else + return WriteSingleClusterData(aSubject, aPath, aData, this); +#endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE +} + } // namespace app } // namespace chip diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h index 0723a316738498..fe63e028b8dfee 100644 --- a/src/app/WriteHandler.h +++ b/src/app/WriteHandler.h @@ -21,8 +21,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -69,6 +71,7 @@ class WriteHandler : public Messaging::ExchangeDelegate * construction until a call to Close is made to terminate the * instance. * + * @param[in] apProvider A valid pointer to the model used to forward writes towards * @param[in] apWriteHandlerDelegate A Valid pointer to the WriteHandlerDelegate. * * @retval #CHIP_ERROR_INVALID_ARGUMENT on invalid pointers @@ -77,7 +80,7 @@ class WriteHandler : public Messaging::ExchangeDelegate * @retval #CHIP_NO_ERROR On success. * */ - CHIP_ERROR Init(WriteHandlerDelegate * apWriteHandlerDelegate); + CHIP_ERROR Init(DataModel::Provider * apProvider, WriteHandlerDelegate * apWriteHandlerDelegate); /** * Process a write request. Parts of the processing may end up being asynchronous, but the WriteHandler @@ -182,11 +185,20 @@ class WriteHandler : public Messaging::ExchangeDelegate System::PacketBufferHandle && aPayload) override; void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override; + // Write the given data to the given path + CHIP_ERROR WriteClusterData(const Access::SubjectDescriptor & aSubject, const ConcreteDataAttributePath & aPath, + TLV::TLVReader & aData); + Messaging::ExchangeHolder mExchangeCtx; WriteResponseMessage::Builder mWriteResponseBuilder; Optional mProcessingAttributePath; Optional mACLCheckCache = NullOptional; +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + DataModel::Provider * mDataModelProvider = nullptr; + std::optional mLastSuccessfullyWrittenPath; +#endif + // This may be a "fake" pointer or a real delegate pointer, depending // on CHIP_CONFIG_STATIC_GLOBAL_INTERACTION_MODEL_ENGINE setting. // diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp index ffb44eb492dc5a..9eee57d1491fa5 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp @@ -15,6 +15,7 @@ * limitations under the License. */ #include +#include #include #include @@ -45,6 +46,17 @@ namespace { using namespace chip::app::Compatibility::Internal; using Protocols::InteractionModel::Status; +class ContextAttributesChangeListener : public AttributesChangedListener +{ +public: + ContextAttributesChangeListener(const DataModel::InteractionModelContext & context) : mListener(context.dataModelChangeListener) + {} + void MarkDirty(const AttributePathParams & path) override { mListener->MarkDirty(path); } + +private: + DataModel::ProviderChangeListener * mListener; +}; + /// Attempts to write via an attribute access interface (AAI) /// /// If it returns a CHIP_ERROR, then this is a FINAL result (i.e. either failure or success) @@ -273,27 +285,14 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat ChipLogDetail(DataManagement, "Writing attribute: Cluster=" ChipLogFormatMEI " Endpoint=0x%x AttributeId=" ChipLogFormatMEI, ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId)); - // ACL check for non-internal requests - if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal)) - { - ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), Status::UnsupportedAccess); - - Access::RequestPath requestPath{ .cluster = request.path.mClusterId, - .endpoint = request.path.mEndpointId, - .requestType = Access::RequestType::kAttributeWriteRequest, - .entityId = request.path.mAttributeId }; - CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath, - RequiredPrivilege::ForWriteAttribute(request.path)); - - if (err != CHIP_NO_ERROR) - { - ReturnErrorCodeIf((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err); - - // TODO: when wildcard/group writes are supported, handle them to discard rather than fail with status - return err == CHIP_ERROR_ACCESS_DENIED ? Status::UnsupportedAccess : Status::AccessRestricted; - } - } - + // TODO: ordering is to check writability/existence BEFORE ACL and this seems wrong, however + // existing unit tests (TC_AcessChecker.py) validate that we get UnsupportedWrite instead of UnsupportedAccess + // + // This should likely be fixed in spec (probably already fixed by + // https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/9024) + // and tests and implementation + // + // Open issue that needs fixing: https://github.com/project-chip/connectedhomeip/issues/33735 auto metadata = Ember::FindAttributeMetadata(request.path); // Explicit failure in finding a suitable metadata @@ -322,7 +321,56 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal)) { VerifyOrReturnError(!isReadOnly, Status::UnsupportedWrite); + } + + // ACL check for non-internal requests + bool checkAcl = !request.operationFlags.Has(DataModel::OperationFlags::kInternal); + // For chunking, ACL check is not re-done if the previous write was successful for the exact same + // path. We apply this everywhere as a shortcut, although realistically this is only for AccessControl cluster + if (checkAcl && request.previousSuccessPath.has_value()) + { + // NOTE: explicit cast/check only for attribute path and nothing else. + // + // In particular `request.path` is a DATA path (contains a list index) + // and we do not want request.previousSuccessPath to be auto-cast to a + // data path with a empty list and fail the compare. + // + // This could be `request.previousSuccessPath != request.path` (where order + // is important) however that would seem more brittle (relying that a != b + // behaves differently than b != a due to casts). Overall Data paths are not + // the same as attribute paths. + // + // Also note that Concrete path have a mExpanded that is not used in compares. + const ConcreteAttributePath & attributePathA = request.path; + const ConcreteAttributePath & attributePathB = *request.previousSuccessPath; + + checkAcl = (attributePathA != attributePathB); + } + + if (checkAcl) + { + ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), Status::UnsupportedAccess); + + Access::RequestPath requestPath{ .cluster = request.path.mClusterId, + .endpoint = request.path.mEndpointId, + .requestType = Access::RequestType::kAttributeWriteRequest, + .entityId = request.path.mAttributeId }; + CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath, + RequiredPrivilege::ForWriteAttribute(request.path)); + + if (err != CHIP_NO_ERROR) + { + VerifyOrReturnValue(err != CHIP_ERROR_ACCESS_DENIED, Status::UnsupportedAccess); + VerifyOrReturnValue(err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL, Status::AccessRestricted); + + return err; + } + } + + // Internal is allowed to bypass timed writes and read-only. + if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal)) + { VerifyOrReturnError(!(*attributeMetadata)->MustUseTimedWrite() || request.writeFlags.Has(DataModel::WriteFlags::kTimed), Status::NeedsTimedInteraction); } @@ -349,6 +397,8 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat } } + ContextAttributesChangeListener change_listener(CurrentContext()); + AttributeAccessInterface * aai = AttributeAccessInterfaceRegistry::Instance().Get(request.path.mEndpointId, request.path.mClusterId); std::optional aai_result = TryWriteViaAccessInterface(request.path, aai, decoder); @@ -356,11 +406,9 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat { if (*aai_result == CHIP_NO_ERROR) { - // TODO: change callbacks should likely be routed through the context `MarkDirty` only - // however for now this is called directly because ember code does this call - // inside emberAfWriteAttribute. - MatterReportingAttributeChangeCallback(request.path); - CurrentContext().dataModelChangeListener->MarkDirty(request.path); + // TODO: this is awkward since it provides AAI no control over this, specifically + // AAI may not want to increase versions for some attributes that are Q + emberAfAttributeChanged(request.path.mEndpointId, request.path.mClusterId, request.path.mAttributeId, &change_listener); } return *aai_result; } @@ -376,18 +424,21 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat return Status::InvalidValue; } + EmberAfWriteDataInput dataInput(dataBuffer.data(), (*attributeMetadata)->attributeType); + + dataInput.SetChangeListener(&change_listener); + // TODO: dataInput.SetMarkDirty() should be according to `ChangesOmmited` + if (request.operationFlags.Has(DataModel::OperationFlags::kInternal)) { // Internal requests use the non-External interface that has less enforcement // than the external version (e.g. does not check/enforce writable settings, does not // validate attribute types) - see attribute-table.h documentation for details. - status = emberAfWriteAttribute(request.path.mEndpointId, request.path.mClusterId, request.path.mAttributeId, - dataBuffer.data(), (*attributeMetadata)->attributeType); + status = emberAfWriteAttribute(request.path, dataInput); } else { - status = - emAfWriteAttributeExternal(request.path, EmberAfWriteDataInput(dataBuffer.data(), (*attributeMetadata)->attributeType)); + status = emAfWriteAttributeExternal(request.path, dataInput); } if (status != Protocols::InteractionModel::Status::Success) @@ -395,14 +446,6 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat return status; } - // TODO: this WILL requre updates - // - // - Internal writes may need to be able to decide if to mark things dirty or not (see AAI as well) - // - Changes-ommited paths should not be marked dirty (ember is not aware of that flag) - // - This likely maps to `MatterReportingAttributeChangeCallback` HOWEVER current ember write functions - // will selectively call that one depending on old attribute state (i.e. calling every time is a - // change in behavior) - CurrentContext().dataModelChangeListener->MarkDirty(request.path); return CHIP_NO_ERROR; } diff --git a/src/app/codegen-data-model-provider/tests/EmberReadWriteOverride.cpp b/src/app/codegen-data-model-provider/tests/EmberReadWriteOverride.cpp index ac918049b860f4..5befbc14aa5715 100644 --- a/src/app/codegen-data-model-provider/tests/EmberReadWriteOverride.cpp +++ b/src/app/codegen-data-model-provider/tests/EmberReadWriteOverride.cpp @@ -16,6 +16,8 @@ */ #include "EmberReadWriteOverride.h" +#include +#include #include #include #include @@ -116,9 +118,15 @@ Status emAfWriteAttributeExternal(const chip::app::ConcreteAttributePath & path, // copy over as much data as possible // NOTE: we do NOT use (*metadata)->size since it is unclear if our mocks set that correctly size_t len = std::min(sizeof(gEmberIoBuffer), chip::app::Compatibility::Internal::gEmberAttributeIOBufferSpan.size()); + memcpy(gEmberIoBuffer, input.dataPtr, len); gEmberIoBufferFill = len; + if (input.changeListener != nullptr) + { + input.changeListener->MarkDirty(chip::app::AttributePathParams(path.mEndpointId, path.mClusterId, path.mAttributeId)); + } + return Status::Success; } @@ -128,3 +136,8 @@ Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, return emAfWriteAttributeExternal(chip::app::ConcreteAttributePath(endpoint, cluster, attributeID), EmberAfWriteDataInput(dataPtr, dataType)); } + +Status emberAfWriteAttribute(const chip::app::ConcreteAttributePath & path, const EmberAfWriteDataInput & input) +{ + return emAfWriteAttributeExternal(path, input); +} diff --git a/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp index d5c0ea673fec80..164d8856a9de62 100644 --- a/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp @@ -132,13 +132,13 @@ bool operator==(const Access::SubjectDescriptor & a, const Access::SubjectDescri class TestProviderChangeListener : public ProviderChangeListener { public: - void MarkDirty(const ConcreteAttributePath & path) override { mDirtyList.push_back(path); } + void MarkDirty(const AttributePathParams & path) override { mDirtyList.push_back(path); } - std::vector & DirtyList() { return mDirtyList; } - const std::vector & DirtyList() const { return mDirtyList; } + std::vector & DirtyList() { return mDirtyList; } + const std::vector & DirtyList() const { return mDirtyList; } private: - std::vector mDirtyList; + std::vector mDirtyList; }; class TestEventGenerator : public EventsGenerator @@ -802,12 +802,32 @@ void TestEmberScalarTypeWrite(const typename NumericAttributeTraits::WorkingT EXPECT_EQ(actual, value); ASSERT_EQ(model.ChangeListener().DirtyList().size(), 1u); - EXPECT_EQ(model.ChangeListener().DirtyList()[0], test.request.path); + EXPECT_EQ(model.ChangeListener().DirtyList()[0], + AttributePathParams(test.request.path.mEndpointId, test.request.path.mClusterId, test.request.path.mAttributeId)); // reset for the next test model.ChangeListener().DirtyList().clear(); } + // nullable test: write null to make sure content of buffer changed (otherwise it will be a noop for dirty checking) + { + TestWriteRequest test( + kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZclType))); + + using NumericType = NumericAttributeTraits; + using NullableType = chip::app::DataModel::Nullable; + AttributeValueDecoder decoder = test.DecoderFor(NullableType()); + + // write should succeed + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + + // dirty: we changed the value to null + ASSERT_EQ(model.ChangeListener().DirtyList().size(), 1u); + EXPECT_EQ(model.ChangeListener().DirtyList()[0], + AttributePathParams(test.request.path.mEndpointId, test.request.path.mClusterId, test.request.path.mAttributeId)); + } + // nullable test { TestWriteRequest test( @@ -827,8 +847,10 @@ void TestEmberScalarTypeWrite(const typename NumericAttributeTraits::WorkingT typename NumericAttributeTraits::WorkingType actual = NumericAttributeTraits::StorageToWorking(storage); ASSERT_EQ(actual, value); - ASSERT_EQ(model.ChangeListener().DirtyList().size(), 1u); - EXPECT_EQ(model.ChangeListener().DirtyList()[0], test.request.path); + // dirty a 2nd time when we moved from null to a real value + ASSERT_EQ(model.ChangeListener().DirtyList().size(), 2u); + EXPECT_EQ(model.ChangeListener().DirtyList()[1], + AttributePathParams(test.request.path.mEndpointId, test.request.path.mClusterId, test.request.path.mAttributeId)); } } @@ -1994,7 +2016,20 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteAclDeny) CodegenDataModelProviderWithContext model; ScopedMockAccessControl accessControl; - TestWriteRequest test(kDenySubjectDescriptor, ConcreteDataAttributePath(kMockEndpoint1, MockClusterId(1), MockAttributeId(10))); + /* Using this path is also failing existence checks, so this cannot be enabled + * until we fix ordering of ACL to be done before existence checks + + TestWriteRequest test(kDenySubjectDescriptor, + ConcreteDataAttributePath(kMockEndpoint1, MockClusterId(1), MockAttributeId(10))); + AttributeValueDecoder decoder = test.DecoderFor(1234); + + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::UnsupportedAccess); + ASSERT_TRUE(model.ChangeListener().DirtyList().empty()); + */ + + TestWriteRequest test(kDenySubjectDescriptor, + ConcreteDataAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_INT32U_ATTRIBUTE_TYPE))); AttributeValueDecoder decoder = test.DecoderFor(1234); ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::UnsupportedAccess); @@ -2431,12 +2466,13 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest) // AAI marks dirty paths ASSERT_EQ(model.ChangeListener().DirtyList().size(), 1u); - EXPECT_EQ(model.ChangeListener().DirtyList()[0], kStructPath); + EXPECT_EQ(model.ChangeListener().DirtyList()[0], + AttributePathParams(kStructPath.mEndpointId, kStructPath.mClusterId, kStructPath.mAttributeId)); // AAI does not prevent read/write of regular attributes // validate that once AAI is added, we still can go through writing regular bits (i.e. // AAI returning "unknown" has fallback to ember) - TestEmberScalarTypeWrite(1234); + TestEmberScalarTypeWrite(4321); TestEmberScalarNullWrite(); } diff --git a/src/app/data-model-provider/OperationTypes.h b/src/app/data-model-provider/OperationTypes.h index 00ec0424763cf5..8758e02ef89c5d 100644 --- a/src/app/data-model-provider/OperationTypes.h +++ b/src/app/data-model-provider/OperationTypes.h @@ -70,15 +70,24 @@ struct ReadAttributeRequest : OperationRequest enum class WriteFlags : uint32_t { - kTimed = 0x0001, // Write is a timed write (i.e. a Timed Request Action preceeded it) - kListBegin = 0x0002, // This is the FIRST list of data elements - kListEnd = 0x0004, // This is the LAST list element to write + kTimed = 0x0001, // Write is a timed write (i.e. a Timed Request Action preceeded it) }; struct WriteAttributeRequest : OperationRequest { ConcreteDataAttributePath path; // NOTE: this also contains LIST operation options (i.e. "data" path type) BitFlags writeFlags; + + // The path of the previous successful write in the same write transaction, if any. + // + // In particular this means that a write to this path has succeeded before (i.e. it passed required ACL checks). + // The intent for this is to allow short-cutting ACL checks when ACL is in progress of being updated: + // - During write chunking, list writes can be of the form "reset list" followed by "append item by item" + // - When ACL is updating, a reset to empty would result in the entire ACL being deny and the "append" + // would fail. + // callers are expected to keep track of a `previousSuccessPath` whenever a write succeeds (otherwise ACL + // checks may fail) + std::optional previousSuccessPath; }; enum class InvokeFlags : uint32_t diff --git a/src/app/data-model-provider/ProviderChangeListener.h b/src/app/data-model-provider/ProviderChangeListener.h index 97061865921b0f..b0f6aab5f811f3 100644 --- a/src/app/data-model-provider/ProviderChangeListener.h +++ b/src/app/data-model-provider/ProviderChangeListener.h @@ -16,7 +16,7 @@ */ #pragma once -#include +#include namespace chip { namespace app { @@ -39,7 +39,7 @@ class ProviderChangeListener /// Mark all attributes matching the given path (which may be a wildcard) dirty. /// /// Wildcards are supported. - virtual void MarkDirty(const ConcreteAttributePath & path) = 0; + virtual void MarkDirty(const AttributePathParams & path) = 0; }; } // namespace DataModel diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 072aa100e10312..cb4169420ee513 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include @@ -1010,7 +1011,16 @@ void Engine::ScheduleUrgentEventDeliverySync(Optional fabricIndex) Run(); } -}; // namespace reporting +void Engine::MarkDirty(const AttributePathParams & path) +{ + CHIP_ERROR err = SetDirty(path); + if (err != CHIP_NO_ERROR) + { + ChipLogError(DataManagement, "Failed to set path dirty: %" CHIP_ERROR_FORMAT, err.Format()); + } +} + +} // namespace reporting } // namespace app } // namespace chip diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h index cff36ff41ccb5b..a16b0d9151d3d9 100644 --- a/src/app/reporting/Engine.h +++ b/src/app/reporting/Engine.h @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -54,7 +55,7 @@ namespace reporting { * At its core, it tries to gather and pack as much relevant attributes changes and/or events as possible into a report * message before sending that to the reader. It continues to do so until it has no more work to do. */ -class Engine +class Engine : public DataModel::ProviderChangeListener { public: /** @@ -140,6 +141,9 @@ class Engine size_t GetGlobalDirtySetSize() { return mGlobalDirtySet.Allocated(); } #endif + /* ProviderChangeListener implementation */ + void MarkDirty(const AttributePathParams & path) override; + private: /** * Main work-horse function that executes the run-loop. diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp index 625d6c3328d2f2..6500b5fe0db6a8 100644 --- a/src/app/tests/TestWriteInteraction.cpp +++ b/src/app/tests/TestWriteInteraction.cpp @@ -15,8 +15,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - -#include #include #include @@ -70,9 +68,12 @@ class TestWriteInteraction : public chip::Test::AppContext chip::MutableByteSpan span(buf); ASSERT_EQ(GetBobFabric()->GetCompressedFabricIdBytes(span), CHIP_NO_ERROR); ASSERT_EQ(chip::GroupTesting::InitData(&gGroupsProvider, GetBobFabricIndex(), span), CHIP_NO_ERROR); + + mOldProvider = InteractionModelEngine::GetInstance()->SetDataModelProvider(&TestImCustomDataModel::Instance()); } void TearDown() override { + InteractionModelEngine::GetInstance()->SetDataModelProvider(mOldProvider); chip::Credentials::GroupDataProvider * provider = chip::Credentials::GetGroupDataProvider(); if (provider != nullptr) { @@ -93,6 +94,9 @@ class TestWriteInteraction : public chip::Test::AppContext static void AddAttributeStatus(WriteHandler & aWriteHandler); static void GenerateWriteRequest(bool aIsTimedWrite, System::PacketBufferHandle & aPayload); static void GenerateWriteResponse(System::PacketBufferHandle & aPayload); + +private: + chip::app::DataModel::Provider * mOldProvider = nullptr; }; class TestExchangeDelegate : public Messaging::ExchangeDelegate @@ -296,7 +300,8 @@ TEST_F(TestWriteInteraction, TestWriteHandler) System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); - writeHandler.Init(chip::app::InteractionModelEngine::GetInstance()); + writeHandler.Init(chip::app::InteractionModelEngine::GetInstance()->GetDataModelProvider(), + chip::app::InteractionModelEngine::GetInstance()); GenerateWriteRequest(messageIsTimed, buf); diff --git a/src/app/tests/test-interaction-model-api.cpp b/src/app/tests/test-interaction-model-api.cpp index 4ecc4d66706b0c..d24c586efc2e12 100644 --- a/src/app/tests/test-interaction-model-api.cpp +++ b/src/app/tests/test-interaction-model-api.cpp @@ -46,6 +46,18 @@ class TestOnlyAttributeValueEncoderAccessor AttributeValueEncoder & mEncoder; }; +class TestOnlyAttributeValueDecoderAccessor +{ +public: + TestOnlyAttributeValueDecoderAccessor(AttributeValueDecoder & decoder) : mDecoder(decoder) {} + + TLV::TLVReader & GetTlvReader() { return mDecoder.mReader; } + void SetTriedDecode(bool triedDecode) { mDecoder.mTriedDecode = triedDecode; } + +private: + AttributeValueDecoder & mDecoder; +}; + // Used by the code in TestWriteInteraction.cpp (and generally tests that interact with the WriteHandler may need this). const EmberAfAttributeMetadata * GetAttributeMetadata(const ConcreteAttributePath & aConcreteClusterPath) { @@ -170,7 +182,21 @@ ActionReturnStatus TestImCustomDataModel::ReadAttribute(const ReadAttributeReque ActionReturnStatus TestImCustomDataModel::WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) { - return CHIP_ERROR_NOT_IMPLEMENTED; + if (request.path.mDataVersion.HasValue() && request.path.mDataVersion.Value() == Test::kRejectedDataVersion) + { + return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch); + } + + TestOnlyAttributeValueDecoderAccessor decodeAccess(decoder); + + decodeAccess.SetTriedDecode(true); + + TLV::TLVWriter writer; + writer.Init(chip::Test::attributeDataTLV); + writer.CopyElement(TLV::AnonymousTag(), decodeAccess.GetTlvReader()); + chip::Test::attributeDataTLVLen = writer.GetLengthWritten(); + + return CHIP_NO_ERROR; } std::optional TestImCustomDataModel::Invoke(const InvokeRequest & request, diff --git a/src/app/util/mock/CodegenEmberMocks.cpp b/src/app/util/mock/CodegenEmberMocks.cpp index 521f36c9569647..a1963ab71d732e 100644 --- a/src/app/util/mock/CodegenEmberMocks.cpp +++ b/src/app/util/mock/CodegenEmberMocks.cpp @@ -15,6 +15,7 @@ * limitations under the License. */ #include +#include #include #include @@ -41,8 +42,13 @@ Status emAfWriteAttributeExternal(const chip::app::ConcreteAttributePath & path, } Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * dataPtr, - EmberAfAttributeType dataType) + EmberAfAttributeType dataType, chip::app::MarkAttributeDirty markDirty) { return emAfWriteAttributeExternal(chip::app::ConcreteAttributePath(endpoint, cluster, attributeID), EmberAfWriteDataInput(dataPtr, dataType)); } + +Status emberAfWriteAttribute(const chip::app::ConcreteAttributePath & path, const EmberAfWriteDataInput & input) +{ + return emAfWriteAttributeExternal(path, input); +} diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp index 93d3219fc04a41..87ffa8bbbdd89f 100644 --- a/src/app/util/mock/attribute-storage.cpp +++ b/src/app/util/mock/attribute-storage.cpp @@ -313,6 +313,13 @@ DataVersion * emberAfDataVersionStorage(const chip::app::ConcreteClusterPath & a return &dataVersion; } +void emberAfAttributeChanged(EndpointId endpoint, ClusterId clusterId, AttributeId attributeId, + AttributesChangedListener * listener) +{ + dataVersion++; + listener->MarkDirty(AttributePathParams(endpoint, clusterId, attributeId)); +} + namespace chip { namespace app { diff --git a/src/controller/tests/data_model/DataModelFixtures.cpp b/src/controller/tests/data_model/DataModelFixtures.cpp index 0333b24239e0e7..1850bc0799cdc6 100644 --- a/src/controller/tests/data_model/DataModelFixtures.cpp +++ b/src/controller/tests/data_model/DataModelFixtures.cpp @@ -50,6 +50,17 @@ class TestOnlyAttributeValueEncoderAccessor AttributeValueEncoder & mEncoder; }; +class TestOnlyAttributeValueDecoderAccessor +{ +public: + TestOnlyAttributeValueDecoderAccessor(AttributeValueDecoder & decoder) : mDecoder(decoder) {} + + TLV::TLVReader & GetTlvReader() { return mDecoder.mReader; } + +private: + AttributeValueDecoder & mDecoder; +}; + namespace DataModelTests { ScopedChangeOnly gReadResponseDirective(ReadResponseDirective::kSendDataResponse); @@ -300,7 +311,8 @@ CHIP_ERROR WriteSingleClusterData(const Access::SubjectDescriptor & aSubjectDesc } if (aPath.mClusterId == Clusters::UnitTesting::Id && aPath.mAttributeId == Attributes::ListFabricScoped::Id) { - // Mock a invalid SubjectDescriptor + // Mock an invalid SubjectDescriptor. + // NOTE: completely ignores the passed-in subjectDescriptor AttributeValueDecoder decoder(aReader, Access::SubjectDescriptor()); if (!aPath.IsListOperation() || aPath.mListOp == ConcreteDataAttributePath::ListOperation::ReplaceAll) { @@ -522,7 +534,132 @@ ActionReturnStatus CustomDataModel::ReadAttribute(const ReadAttributeRequest & r ActionReturnStatus CustomDataModel::WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) { - return CHIP_ERROR_NOT_IMPLEMENTED; + static ListIndex listStructOctetStringElementCount = 0; + + if (request.path.mDataVersion.HasValue() && request.path.mDataVersion.Value() == kRejectedDataVersion) + { + return InteractionModel::Status::DataVersionMismatch; + } + + if (request.path.mClusterId == Clusters::UnitTesting::Id && + request.path.mAttributeId == Attributes::ListStructOctetString::TypeInfo::GetAttributeId()) + { + if (gWriteResponseDirective == WriteResponseDirective::kSendAttributeSuccess) + { + if (!request.path.IsListOperation() || request.path.mListOp == ConcreteDataAttributePath::ListOperation::ReplaceAll) + { + + Attributes::ListStructOctetString::TypeInfo::DecodableType value; + + ReturnErrorOnFailure(decoder.Decode(value)); + + auto iter = value.begin(); + listStructOctetStringElementCount = 0; + while (iter.Next()) + { + auto & item = iter.GetValue(); + + VerifyOrReturnError(item.member1 == listStructOctetStringElementCount, CHIP_ERROR_INVALID_ARGUMENT); + listStructOctetStringElementCount++; + } + return CHIP_NO_ERROR; + } + + if (request.path.mListOp == ConcreteDataAttributePath::ListOperation::AppendItem) + { + Structs::TestListStructOctet::DecodableType item; + ReturnErrorOnFailure(decoder.Decode(item)); + VerifyOrReturnError(item.member1 == listStructOctetStringElementCount, CHIP_ERROR_INVALID_ARGUMENT); + listStructOctetStringElementCount++; + + return CHIP_NO_ERROR; + } + + return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; + } + + return CHIP_IM_GLOBAL_STATUS(Failure); + } + if (request.path.mClusterId == Clusters::UnitTesting::Id && request.path.mAttributeId == Attributes::ListFabricScoped::Id) + { + // TODO(backwards compatibility): unit tests here undoes the subject descriptor usage + // - original tests were completely bypassing the passed in subject descriptor for this test + // and overriding it with a invalid subject descriptor + // - we do the same here, however this seems somewhat off: decoder.Decode() will fail for list + // items so we could just return the error directly without this extra step + + // Mock an invalid Subject Descriptor + AttributeValueDecoder invalidSubjectDescriptorDecoder(TestOnlyAttributeValueDecoderAccessor(decoder).GetTlvReader(), + Access::SubjectDescriptor()); + if (!request.path.IsListOperation() || request.path.mListOp == ConcreteDataAttributePath::ListOperation::ReplaceAll) + { + Attributes::ListFabricScoped::TypeInfo::DecodableType value; + + ReturnErrorOnFailure(invalidSubjectDescriptorDecoder.Decode(value)); + + auto iter = value.begin(); + while (iter.Next()) + { + auto & item = iter.GetValue(); + (void) item; + } + } + else if (request.path.mListOp == ConcreteDataAttributePath::ListOperation::AppendItem) + { + Structs::TestFabricScoped::DecodableType item; + ReturnErrorOnFailure(invalidSubjectDescriptorDecoder.Decode(item)); + } + else + { + return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; + } + return CHIP_NO_ERROR; + } + + // Boolean attribute of unit testing cluster triggers "multiple errors" case. + if (request.path.mClusterId == Clusters::UnitTesting::Id && + request.path.mAttributeId == Attributes::Boolean::TypeInfo::GetAttributeId()) + { + // TODO(IMDM): this used to send 4 responses (hence the multiple status) + // + // for (size_t i = 0; i < 4; ++i) + // { + // aWriteHandler->AddStatus(request.path, status); + // } + // + // which are NOT encodable by a simple response. It is unclear how this is + // convertible (if at all): we write path by path only. Having multiple + // responses for the same path within the write code makes no sense + // + // This should NOT be possible anymore when one can only return a single + // status (nobody has access to multiple path status updates at this level) + switch (gWriteResponseDirective) + { + case WriteResponseDirective::kSendMultipleSuccess: + return InteractionModel::Status::Success; + case WriteResponseDirective::kSendMultipleErrors: + return InteractionModel::Status::Failure; + default: + chipDie(); + } + } + + if (request.path.mClusterId == Clusters::UnitTesting::Id && + request.path.mAttributeId == Attributes::Int8u::TypeInfo::GetAttributeId()) + { + switch (gWriteResponseDirective) + { + case WriteResponseDirective::kSendClusterSpecificSuccess: + return InteractionModel::ClusterStatusCode::ClusterSpecificSuccess(kExampleClusterSpecificSuccess); + case WriteResponseDirective::kSendClusterSpecificFailure: + return InteractionModel::ClusterStatusCode::ClusterSpecificFailure(kExampleClusterSpecificFailure); + default: + // this should not be reached, our tests only set up these for this test case + chipDie(); + } + } + + return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; } std::optional CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, diff --git a/src/controller/tests/data_model/TestWrite.cpp b/src/controller/tests/data_model/TestWrite.cpp index 03d100ab33dabb..218832702b230f 100644 --- a/src/controller/tests/data_model/TestWrite.cpp +++ b/src/controller/tests/data_model/TestWrite.cpp @@ -85,6 +85,19 @@ class SingleWriteCallback : public WriteClient::Callback class TestWrite : public chip::Test::AppContext { public: + void SetUp() override + { + chip::Test::AppContext::SetUp(); + mOldProvider = InteractionModelEngine::GetInstance()->SetDataModelProvider(&CustomDataModel::Instance()); + } + + // Performs teardown for each individual test in the test suite + void TearDown() override + { + InteractionModelEngine::GetInstance()->SetDataModelProvider(mOldProvider); + chip::Test::AppContext::TearDown(); + } + void ResetCallback() { mSingleWriteCallback.reset(); } void PrepareWriteCallback(ConcreteAttributePath path) { mSingleWriteCallback = std::make_unique(path); } @@ -93,6 +106,7 @@ class TestWrite : public chip::Test::AppContext protected: std::unique_ptr mSingleWriteCallback; + chip::app::DataModel::Provider * mOldProvider = nullptr; }; TEST_F(TestWrite, TestDataResponse) @@ -128,7 +142,8 @@ TEST_F(TestWrite, TestDataResponse) DrainAndServiceIO(); - EXPECT_TRUE(onSuccessCbInvoked && !onFailureCbInvoked); + EXPECT_TRUE(onSuccessCbInvoked); + EXPECT_FALSE(onFailureCbInvoked); EXPECT_EQ(chip::app::InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers(), 0u); EXPECT_EQ(GetExchangeManager().GetNumActiveExchanges(), 0u); } From 010decd199af5bcafb3f5e9c38a5871a5e33ea1a Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Wed, 18 Sep 2024 13:12:52 -0700 Subject: [PATCH 2/3] [Fabric-Admin] Refactor to use API methods instead of PushCommand (1/3) (#35614) * [Fabric-Admin] Refactor to use API methods instead of PushCommand to talk to SDK * Address review comments * Update examples/fabric-admin/device_manager/PairingManager.h Co-authored-by: Andrei Litvin * Update per review comments * Update commissioningTimeout to commissioningTimeoutSec --------- Co-authored-by: Andrei Litvin --- examples/fabric-admin/BUILD.gn | 2 + .../commands/common/CHIPCommand.cpp | 3 + .../fabric-sync/FabricSyncCommand.cpp | 10 +- .../commands/fabric-sync/FabricSyncCommand.h | 2 +- .../OpenCommissioningWindowCommand.cpp | 8 - .../pairing/OpenCommissioningWindowCommand.h | 11 -- .../device_manager/DeviceManager.cpp | 63 ++++--- .../device_manager/DeviceManager.h | 10 +- .../device_manager/PairingManager.cpp | 170 ++++++++++++++++++ .../device_manager/PairingManager.h | 122 +++++++++++++ examples/fabric-admin/rpc/RpcServer.cpp | 29 ++- 11 files changed, 348 insertions(+), 82 deletions(-) create mode 100644 examples/fabric-admin/device_manager/PairingManager.cpp create mode 100644 examples/fabric-admin/device_manager/PairingManager.h diff --git a/examples/fabric-admin/BUILD.gn b/examples/fabric-admin/BUILD.gn index d1add205f8826c..ab584459582657 100644 --- a/examples/fabric-admin/BUILD.gn +++ b/examples/fabric-admin/BUILD.gn @@ -88,6 +88,8 @@ static_library("fabric-admin-utils") { "device_manager/DeviceSubscriptionManager.h", "device_manager/DeviceSynchronization.cpp", "device_manager/DeviceSynchronization.h", + "device_manager/PairingManager.cpp", + "device_manager/PairingManager.h", "device_manager/UniqueIdGetter.cpp", "device_manager/UniqueIdGetter.h", ] diff --git a/examples/fabric-admin/commands/common/CHIPCommand.cpp b/examples/fabric-admin/commands/common/CHIPCommand.cpp index 0c5455439b22d9..b18eb9f2de472a 100644 --- a/examples/fabric-admin/commands/common/CHIPCommand.cpp +++ b/examples/fabric-admin/commands/common/CHIPCommand.cpp @@ -21,6 +21,7 @@ #include "IcdManager.h" #include #include +#include #include #include #include @@ -181,6 +182,8 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack() mCredIssuerCmds->SetCredentialIssuerOption(CredentialIssuerCommands::CredentialIssuerOptions::kAllowTestCdSigningKey, allowTestCdSigningKey); + PairingManager::Instance().Init(&CurrentCommissioner()); + return CHIP_NO_ERROR; } diff --git a/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp b/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp index 85a3aada9f88ae..64d43cce11f3ac 100644 --- a/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp +++ b/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp @@ -344,15 +344,7 @@ CHIP_ERROR FabricSyncDeviceCommand::RunCommand(EndpointId remoteId) return CHIP_NO_ERROR; } - OpenCommissioningWindowCommand * openCommand = - static_cast(CommandMgr().GetCommandByName("pairing", "open-commissioning-window")); - - if (openCommand == nullptr) - { - return CHIP_ERROR_NOT_IMPLEMENTED; - } - - openCommand->RegisterDelegate(this); + PairingManager::Instance().SetOpenCommissioningWindowDelegate(this); DeviceMgr().OpenRemoteDeviceCommissioningWindow(remoteId); diff --git a/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.h b/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.h index 669edd75d653e6..e80f10133853f7 100644 --- a/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.h +++ b/examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.h @@ -19,8 +19,8 @@ #pragma once #include -#include #include +#include // Constants constexpr uint32_t kCommissionPrepareTimeMs = 500; diff --git a/examples/fabric-admin/commands/pairing/OpenCommissioningWindowCommand.cpp b/examples/fabric-admin/commands/pairing/OpenCommissioningWindowCommand.cpp index 53073160108036..cfe212fda00131 100644 --- a/examples/fabric-admin/commands/pairing/OpenCommissioningWindowCommand.cpp +++ b/examples/fabric-admin/commands/pairing/OpenCommissioningWindowCommand.cpp @@ -70,14 +70,6 @@ CHIP_ERROR OpenCommissioningWindowCommand::RunCommand() void OpenCommissioningWindowCommand::OnOpenCommissioningWindowResponse(void * context, NodeId remoteId, CHIP_ERROR err, chip::SetupPayload payload) { - OpenCommissioningWindowCommand * self = static_cast(context); - if (self->mDelegate) - { - self->mDelegate->OnCommissioningWindowOpened(remoteId, err, payload); - self->UnregisterDelegate(); - } - - LogErrorOnFailure(err); OnOpenBasicCommissioningWindowResponse(context, remoteId, err); } diff --git a/examples/fabric-admin/commands/pairing/OpenCommissioningWindowCommand.h b/examples/fabric-admin/commands/pairing/OpenCommissioningWindowCommand.h index 7edcdba7115665..a8760a464827a9 100644 --- a/examples/fabric-admin/commands/pairing/OpenCommissioningWindowCommand.h +++ b/examples/fabric-admin/commands/pairing/OpenCommissioningWindowCommand.h @@ -22,13 +22,6 @@ #include #include -class CommissioningWindowDelegate -{ -public: - virtual void OnCommissioningWindowOpened(chip::NodeId deviceId, CHIP_ERROR err, chip::SetupPayload payload) = 0; - virtual ~CommissioningWindowDelegate() = default; -}; - class OpenCommissioningWindowCommand : public CHIPCommand { public: @@ -57,9 +50,6 @@ class OpenCommissioningWindowCommand : public CHIPCommand "params if absent"); } - void RegisterDelegate(CommissioningWindowDelegate * delegate) { mDelegate = delegate; } - void UnregisterDelegate() { mDelegate = nullptr; } - /////////// CHIPCommand Interface ///////// CHIP_ERROR RunCommand() override; @@ -71,7 +61,6 @@ class OpenCommissioningWindowCommand : public CHIPCommand NodeId mNodeId; chip::EndpointId mEndpointId; chip::Controller::CommissioningWindowOpener::CommissioningWindowOption mCommissioningWindowOption; - CommissioningWindowDelegate * mDelegate = nullptr; uint16_t mCommissioningWindowTimeout; uint32_t mIteration; uint16_t mDiscriminator; diff --git a/examples/fabric-admin/device_manager/DeviceManager.cpp b/examples/fabric-admin/device_manager/DeviceManager.cpp index ae8fa507ceaaa4..897b1b60e0b68b 100644 --- a/examples/fabric-admin/device_manager/DeviceManager.cpp +++ b/examples/fabric-admin/device_manager/DeviceManager.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -30,13 +31,12 @@ using namespace chip::app::Clusters; namespace { -constexpr uint16_t kWindowTimeout = 300; -constexpr uint16_t kIteration = 1000; -constexpr uint16_t kSubscribeMinInterval = 0; -constexpr uint16_t kSubscribeMaxInterval = 60; -constexpr uint16_t kAggragatorEndpointId = 1; -constexpr uint16_t kMaxDiscriminatorLength = 4095; -constexpr uint8_t kEnhancedCommissioningMethod = 1; +constexpr uint16_t kWindowTimeout = 300; +constexpr uint16_t kIteration = 1000; +constexpr uint16_t kSubscribeMinInterval = 0; +constexpr uint16_t kSubscribeMaxInterval = 60; +constexpr uint16_t kAggragatorEndpointId = 1; +constexpr uint16_t kMaxDiscriminatorLength = 4095; } // namespace @@ -115,19 +115,18 @@ void DeviceManager::RemoveSyncedDevice(NodeId nodeId) ChipLogValueX64(device->GetNodeId()), device->GetEndpointId()); } -void DeviceManager::OpenDeviceCommissioningWindow(NodeId nodeId, uint32_t commissioningTimeout, uint32_t iterations, - uint32_t discriminator, const char * saltHex, const char * verifierHex) +void DeviceManager::OpenDeviceCommissioningWindow(NodeId nodeId, uint32_t commissioningTimeoutSec, uint32_t iterations, + uint16_t discriminator, const ByteSpan & salt, const ByteSpan & verifier) { - ChipLogProgress(NotSpecified, "Open the commissioning window of device with NodeId:" ChipLogFormatX64, ChipLogValueX64(nodeId)); + ChipLogProgress(NotSpecified, "Opening commissioning window for Node ID: " ChipLogFormatX64, ChipLogValueX64(nodeId)); // Open the commissioning window of a device within its own fabric. - StringBuilder commandBuilder; - - commandBuilder.Add("pairing open-commissioning-window "); - commandBuilder.AddFormat("%lu %d %d %d %d %d --salt hex:%s --verifier hex:%s", nodeId, kRootEndpointId, - kEnhancedCommissioningMethod, commissioningTimeout, iterations, discriminator, saltHex, verifierHex); - - PushCommand(commandBuilder.c_str()); + CHIP_ERROR err = PairingManager::Instance().OpenCommissioningWindow(nodeId, kRootEndpointId, commissioningTimeoutSec, + iterations, discriminator, salt, verifier); + if (err != CHIP_NO_ERROR) + { + ChipLogError(NotSpecified, "Failed to open commissioning window: %s", ErrorStr(err)); + } } void DeviceManager::OpenRemoteDeviceCommissioningWindow(EndpointId remoteEndpointId) @@ -135,17 +134,20 @@ void DeviceManager::OpenRemoteDeviceCommissioningWindow(EndpointId remoteEndpoin // Open the commissioning window of a device from another fabric via its fabric bridge. // This method constructs and sends a command to open the commissioning window for a device // that is part of a different fabric, accessed through a fabric bridge. - StringBuilder commandBuilder; // Use random discriminator to have less chance of collision. uint16_t discriminator = Crypto::GetRandU16() % (kMaxDiscriminatorLength + 1); // Include the upper limit kMaxDiscriminatorLength - commandBuilder.Add("pairing open-commissioning-window "); - commandBuilder.AddFormat("%lu %d %d %d %d %d", mRemoteBridgeNodeId, remoteEndpointId, kEnhancedCommissioningMethod, - kWindowTimeout, kIteration, discriminator); + ByteSpan emptySalt; + ByteSpan emptyVerifier; - PushCommand(commandBuilder.c_str()); + CHIP_ERROR err = PairingManager::Instance().OpenCommissioningWindow(mRemoteBridgeNodeId, remoteEndpointId, kWindowTimeout, + kIteration, discriminator, emptySalt, emptyVerifier); + if (err != CHIP_NO_ERROR) + { + ChipLogError(NotSpecified, "Failed to open commissioning window: %s", ErrorStr(err)); + } } void DeviceManager::PairRemoteFabricBridge(chip::NodeId nodeId, uint32_t setupPINCode, const char * deviceRemoteIp, @@ -421,6 +423,7 @@ void DeviceManager::HandleReverseOpenCommissioningWindow(TLV::TLVReader & data) { CommissionerControl::Commands::ReverseOpenCommissioningWindow::DecodableType value; CHIP_ERROR error = app::DataModel::Decode(data, value); + if (error != CHIP_NO_ERROR) { ChipLogError(NotSpecified, "Failed to decode command response value. Error: %" CHIP_ERROR_FORMAT, error.Format()); @@ -432,18 +435,12 @@ void DeviceManager::HandleReverseOpenCommissioningWindow(TLV::TLVReader & data) ChipLogProgress(NotSpecified, " commissioningTimeout: %u", value.commissioningTimeout); ChipLogProgress(NotSpecified, " discriminator: %u", value.discriminator); ChipLogProgress(NotSpecified, " iterations: %u", value.iterations); + ChipLogProgress(NotSpecified, " PAKEPasscodeVerifier size: %lu", value.PAKEPasscodeVerifier.size()); + ChipLogProgress(NotSpecified, " salt size: %lu", value.salt.size()); - char verifierHex[Crypto::kSpake2p_VerifierSerialized_Length * 2 + 1]; - Encoding::BytesToHex(value.PAKEPasscodeVerifier.data(), value.PAKEPasscodeVerifier.size(), verifierHex, sizeof(verifierHex), - Encoding::HexFlags::kNullTerminate); - ChipLogProgress(NotSpecified, " PAKEPasscodeVerifier: %s", verifierHex); - - char saltHex[Crypto::kSpake2p_Max_PBKDF_Salt_Length * 2 + 1]; - Encoding::BytesToHex(value.salt.data(), value.salt.size(), saltHex, sizeof(saltHex), Encoding::HexFlags::kNullTerminate); - ChipLogProgress(NotSpecified, " salt: %s", saltHex); - - OpenDeviceCommissioningWindow(mLocalBridgeNodeId, value.commissioningTimeout, value.iterations, value.discriminator, saltHex, - verifierHex); + OpenDeviceCommissioningWindow(mLocalBridgeNodeId, value.commissioningTimeout, value.iterations, value.discriminator, + ByteSpan(value.salt.data(), value.salt.size()), + ByteSpan(value.PAKEPasscodeVerifier.data(), value.PAKEPasscodeVerifier.size())); } void DeviceManager::HandleAttributeData(const app::ConcreteDataAttributePath & path, TLV::TLVReader & data) diff --git a/examples/fabric-admin/device_manager/DeviceManager.h b/examples/fabric-admin/device_manager/DeviceManager.h index 5f617845f7dc76..939329be82c691 100644 --- a/examples/fabric-admin/device_manager/DeviceManager.h +++ b/examples/fabric-admin/device_manager/DeviceManager.h @@ -94,18 +94,18 @@ class DeviceManager : public PairingDelegate * This function initiates the process to open the commissioning window for a device identified by the given node ID. * * @param nodeId The ID of the node that should open the commissioning window. - * @param commissioningTimeout The time in seconds before the commissioning window closes. This value determines + * @param commissioningTimeoutSec The time in seconds before the commissioning window closes. This value determines * how long the commissioning window remains open for incoming connections. * @param iterations The number of PBKDF (Password-Based Key Derivation Function) iterations to use * for deriving the PAKE (Password Authenticated Key Exchange) verifier. * @param discriminator The device-specific discriminator, determined during commissioning, which helps * to uniquely identify the device among others. - * @param saltHex The hexadecimal-encoded salt used in the cryptographic operations for commissioning. - * @param verifierHex The hexadecimal-encoded PAKE verifier used to authenticate the commissioning process. + * @param salt The salt used in the cryptographic operations for commissioning. + * @param verifier The PAKE verifier used to authenticate the commissioning process. * */ - void OpenDeviceCommissioningWindow(chip::NodeId nodeId, uint32_t commissioningTimeout, uint32_t iterations, - uint32_t discriminator, const char * saltHex, const char * verifierHex); + void OpenDeviceCommissioningWindow(chip::NodeId nodeId, uint32_t commissioningTimeoutSec, uint32_t iterations, + uint16_t discriminator, const chip::ByteSpan & salt, const chip::ByteSpan & verifier); /** * @brief Open the commissioning window of a device from another fabric via its fabric bridge. diff --git a/examples/fabric-admin/device_manager/PairingManager.cpp b/examples/fabric-admin/device_manager/PairingManager.cpp new file mode 100644 index 00000000000000..aa56af4bc6bf4e --- /dev/null +++ b/examples/fabric-admin/device_manager/PairingManager.cpp @@ -0,0 +1,170 @@ +/* + * + * Copyright (c) 2024 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. + */ + +#include "PairingManager.h" + +#include +#include +#include + +using namespace ::chip; + +PairingManager::PairingManager() : + mOnOpenCommissioningWindowCallback(OnOpenCommissioningWindowResponse, this), + mOnOpenCommissioningWindowVerifierCallback(OnOpenCommissioningWindowVerifierResponse, this) +{} + +void PairingManager::Init(Controller::DeviceCommissioner * commissioner) +{ + VerifyOrDie(mCommissioner == nullptr); + mCommissioner = commissioner; +} + +CHIP_ERROR PairingManager::OpenCommissioningWindow(NodeId nodeId, EndpointId endpointId, uint16_t commissioningTimeoutSec, + uint32_t iterations, uint16_t discriminator, const ByteSpan & salt, + const ByteSpan & verifier) +{ + if (mCommissioner == nullptr) + { + ChipLogError(NotSpecified, "Commissioner is null, cannot open commissioning window"); + return CHIP_ERROR_INCORRECT_STATE; + } + + // Check if a window is already open + if (mWindowOpener != nullptr) + { + ChipLogError(NotSpecified, "A commissioning window is already open"); + return CHIP_ERROR_INCORRECT_STATE; + } + + auto params = Platform::MakeUnique(); + params->nodeId = nodeId; + params->endpointId = endpointId; + params->commissioningWindowTimeout = commissioningTimeoutSec; + params->iteration = iterations; + params->discriminator = discriminator; + + if (!salt.empty()) + { + if (salt.size() > sizeof(params->saltBuffer)) + { + ChipLogError(NotSpecified, "Salt size exceeds buffer capacity"); + return CHIP_ERROR_BUFFER_TOO_SMALL; + } + + memcpy(params->saltBuffer, salt.data(), salt.size()); + params->salt = ByteSpan(params->saltBuffer, salt.size()); + } + + if (!verifier.empty()) + { + if (verifier.size() > sizeof(params->verifierBuffer)) + { + ChipLogError(NotSpecified, "Verifier size exceeds buffer capacity"); + return CHIP_ERROR_BUFFER_TOO_SMALL; + } + + memcpy(params->verifierBuffer, verifier.data(), verifier.size()); + params->verifier = ByteSpan(params->verifierBuffer, verifier.size()); + } + + // Schedule work on the Matter thread + return DeviceLayer::PlatformMgr().ScheduleWork(OnOpenCommissioningWindow, reinterpret_cast(params.release())); +} + +void PairingManager::OnOpenCommissioningWindow(intptr_t context) +{ + Platform::UniquePtr params(reinterpret_cast(context)); + PairingManager & self = PairingManager::Instance(); + + if (self.mCommissioner == nullptr) + { + ChipLogError(NotSpecified, "Commissioner is null, cannot open commissioning window"); + return; + } + + self.mWindowOpener = Platform::MakeUnique(self.mCommissioner); + + if (!params->verifier.empty()) + { + if (params->salt.empty()) + { + ChipLogError(NotSpecified, "Salt is required when verifier is set"); + self.mWindowOpener.reset(); + return; + } + + CHIP_ERROR err = + self.mWindowOpener->OpenCommissioningWindow(Controller::CommissioningWindowVerifierParams() + .SetNodeId(params->nodeId) + .SetEndpointId(params->endpointId) + .SetTimeout(params->commissioningWindowTimeout) + .SetIteration(params->iteration) + .SetDiscriminator(params->discriminator) + .SetVerifier(params->verifier) + .SetSalt(params->salt) + .SetCallback(&self.mOnOpenCommissioningWindowVerifierCallback)); + if (err != CHIP_NO_ERROR) + { + ChipLogError(NotSpecified, "Failed to open commissioning window with verifier: %s", ErrorStr(err)); + self.mWindowOpener.reset(); + } + } + else + { + SetupPayload ignored; + CHIP_ERROR err = self.mWindowOpener->OpenCommissioningWindow(Controller::CommissioningWindowPasscodeParams() + .SetNodeId(params->nodeId) + .SetEndpointId(params->endpointId) + .SetTimeout(params->commissioningWindowTimeout) + .SetIteration(params->iteration) + .SetDiscriminator(params->discriminator) + .SetSetupPIN(NullOptional) + .SetSalt(NullOptional) + .SetCallback(&self.mOnOpenCommissioningWindowCallback), + ignored); + if (err != CHIP_NO_ERROR) + { + ChipLogError(NotSpecified, "Failed to open commissioning window with passcode: %s", ErrorStr(err)); + self.mWindowOpener.reset(); + } + } +} + +void PairingManager::OnOpenCommissioningWindowResponse(void * context, NodeId remoteId, CHIP_ERROR err, SetupPayload payload) +{ + VerifyOrDie(context != nullptr); + PairingManager * self = static_cast(context); + if (self->mCommissioningWindowDelegate) + { + self->mCommissioningWindowDelegate->OnCommissioningWindowOpened(remoteId, err, payload); + self->SetOpenCommissioningWindowDelegate(nullptr); + } + + OnOpenCommissioningWindowVerifierResponse(context, remoteId, err); +} + +void PairingManager::OnOpenCommissioningWindowVerifierResponse(void * context, NodeId remoteId, CHIP_ERROR err) +{ + VerifyOrDie(context != nullptr); + PairingManager * self = static_cast(context); + LogErrorOnFailure(err); + + // Reset the window opener once the window operation is complete + self->mWindowOpener.reset(); +} diff --git a/examples/fabric-admin/device_manager/PairingManager.h b/examples/fabric-admin/device_manager/PairingManager.h new file mode 100644 index 00000000000000..de1c0887f3b1b7 --- /dev/null +++ b/examples/fabric-admin/device_manager/PairingManager.h @@ -0,0 +1,122 @@ +/* + * + * Copyright (c) 2024 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 + +class CommissioningWindowDelegate +{ +public: + virtual void OnCommissioningWindowOpened(chip::NodeId deviceId, CHIP_ERROR err, chip::SetupPayload payload) = 0; + virtual ~CommissioningWindowDelegate() = default; +}; + +/** + * The PairingManager class is responsible for managing the commissioning and pairing process + * of Matter devices. PairingManager is designed to be used as a singleton, meaning that there + * should only be one instance of it running at any given time. + * + * Usage: + * + * 1. The class should be initialized when the system starts up, typically by invoking the static + * instance method to get the singleton. + * 2. To open a commissioning window, the appropriate method should be called on the PairingManager instance. + * 3. The PairingManager will handle the lifecycle of the CommissioningWindowOpener and ensure that + * resources are cleaned up appropriately when pairing is complete or the process is aborted. + * + * Example: + * + * @code + * PairingManager& manager = PairingManager::Instance(); + * manager.OpenCommissioningWindow(); + * @endcode + */ +class PairingManager +{ +public: + static PairingManager & Instance() + { + static PairingManager instance; + return instance; + } + + void Init(chip::Controller::DeviceCommissioner * commissioner); + + /** + * Opens a commissioning window on the specified node and endpoint. + * Only one commissioning window can be active at a time. If a commissioning + * window is already open, this function will return an error. + * + * @param nodeId The target node ID for commissioning. + * @param endpointId The target endpoint ID for commissioning. + * @param commissioningTimeoutSec Timeout for the commissioning window in seconds. + * @param iterations Iterations for PBKDF calculations. + * @param discriminator Discriminator for commissioning. + * @param salt Optional salt for verifier-based commissioning. + * @param verifier Optional verifier for enhanced commissioning security. + * + * @return CHIP_ERROR_INCORRECT_STATE if a commissioning window is already open. + */ + CHIP_ERROR OpenCommissioningWindow(chip::NodeId nodeId, chip::EndpointId endpointId, uint16_t commissioningTimeoutSec, + uint32_t iterations, uint16_t discriminator, const chip::ByteSpan & salt, + const chip::ByteSpan & verifier); + + void SetOpenCommissioningWindowDelegate(CommissioningWindowDelegate * delegate) { mCommissioningWindowDelegate = delegate; } + +private: + PairingManager(); + PairingManager(const PairingManager &) = delete; + PairingManager & operator=(const PairingManager &) = delete; + + chip::Controller::DeviceCommissioner * mCommissioner = nullptr; + + /////////// Open Commissioning Window Command Interface ///////// + struct CommissioningWindowParams + { + chip::NodeId nodeId; + chip::EndpointId endpointId; + uint16_t commissioningWindowTimeout; + uint32_t iteration; + uint16_t discriminator; + chip::Optional setupPIN; + uint8_t verifierBuffer[chip::Crypto::kSpake2p_VerifierSerialized_Length]; + chip::ByteSpan verifier; + uint8_t saltBuffer[chip::Crypto::kSpake2p_Max_PBKDF_Salt_Length]; + chip::ByteSpan salt; + }; + + CommissioningWindowDelegate * mCommissioningWindowDelegate = nullptr; + + /** + * Holds the unique_ptr to the current CommissioningWindowOpener. + * Only one commissioning window opener can be active at a time. + * The pointer is reset when the commissioning window is closed or when an error occurs. + */ + chip::Platform::UniquePtr mWindowOpener; + + static void OnOpenCommissioningWindow(intptr_t context); + static void OnOpenCommissioningWindowResponse(void * context, chip::NodeId deviceId, CHIP_ERROR status, + chip::SetupPayload payload); + static void OnOpenCommissioningWindowVerifierResponse(void * context, chip::NodeId deviceId, CHIP_ERROR status); + + chip::Callback::Callback mOnOpenCommissioningWindowCallback; + chip::Callback::Callback mOnOpenCommissioningWindowVerifierCallback; +}; diff --git a/examples/fabric-admin/rpc/RpcServer.cpp b/examples/fabric-admin/rpc/RpcServer.cpp index 3fb181eac5f21b..6e5e2ec8d66442 100644 --- a/examples/fabric-admin/rpc/RpcServer.cpp +++ b/examples/fabric-admin/rpc/RpcServer.cpp @@ -86,21 +86,20 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate pw::Status OpenCommissioningWindow(const chip_rpc_DeviceCommissioningWindowInfo & request, chip_rpc_OperationStatus & response) override { - NodeId nodeId = request.node_id; - uint32_t commissioningTimeout = request.commissioning_timeout; - uint32_t iterations = request.iterations; - uint32_t discriminator = request.discriminator; - - char saltHex[Crypto::kSpake2p_Max_PBKDF_Salt_Length * 2 + 1]; - Encoding::BytesToHex(request.salt.bytes, request.salt.size, saltHex, sizeof(saltHex), Encoding::HexFlags::kNullTerminate); - - char verifierHex[Crypto::kSpake2p_VerifierSerialized_Length * 2 + 1]; - Encoding::BytesToHex(request.verifier.bytes, request.verifier.size, verifierHex, sizeof(verifierHex), - Encoding::HexFlags::kNullTerminate); - - ChipLogProgress(NotSpecified, "Received OpenCommissioningWindow request: 0x%lx", nodeId); - - DeviceMgr().OpenDeviceCommissioningWindow(nodeId, commissioningTimeout, iterations, discriminator, saltHex, verifierHex); + NodeId nodeId = request.node_id; + uint32_t commissioningTimeoutSec = request.commissioning_timeout; + uint32_t iterations = request.iterations; + uint16_t discriminator = request.discriminator; + + // Log the request details for debugging + ChipLogProgress(NotSpecified, + "Received OpenCommissioningWindow request: NodeId 0x%lx, Timeout: %u, Iterations: %u, Discriminator: %u", + static_cast(nodeId), commissioningTimeoutSec, iterations, discriminator); + + // Open the device commissioning window using raw binary data for salt and verifier + DeviceMgr().OpenDeviceCommissioningWindow(nodeId, commissioningTimeoutSec, iterations, discriminator, + ByteSpan(request.salt.bytes, request.salt.size), + ByteSpan(request.verifier.bytes, request.verifier.size)); response.success = true; From 0d17a40e6d6028bd7174b6ee5376cdbbc933d855 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 18 Sep 2024 16:25:58 -0400 Subject: [PATCH 3/3] Disable OpenIOT CI (#35659) --- .github/workflows/examples-openiotsdk.yaml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/.github/workflows/examples-openiotsdk.yaml b/.github/workflows/examples-openiotsdk.yaml index bd95868be8028a..579a9d7b4eccee 100644 --- a/.github/workflows/examples-openiotsdk.yaml +++ b/.github/workflows/examples-openiotsdk.yaml @@ -15,11 +15,7 @@ name: Build example - Open IoT SDK on: - push: - branches-ignore: - - 'dependabot/**' - pull_request: - merge_group: + # Workflow disabled due to lack of maintainer workflow_dispatch: concurrency: