Event validation simplification - pull logic into InteractionModelEngine.cpp only (#36251)
* Pull back the event path validity mixin, start with a datamodel implementation
* Fix compile logic after I moved things away
* Add one more check
* Restyle
* Fix typo
* Fix includes
* Restyle
* Update src/app/InteractionModelEngine.cpp
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Rename method
* Restyle
* More renames
* Restyle
* Fix some renames
* Restyle
* A few more renames
* Restyle
---------
Co-authored-by: Andrei Litvin <andreilitvin@google.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn
index 13a15f2..0a000a2 100644
--- a/src/app/BUILD.gn
+++ b/src/app/BUILD.gn
@@ -255,7 +255,6 @@
"reporting/Read-Ember.cpp",
"reporting/Read-Ember.h",
]
- public_deps += [ "${chip_root}/src/app/ember_coupling" ]
} else if (chip_use_data_model_interface == "check") {
sources += [
"reporting/Read-Checked.cpp",
@@ -265,10 +264,7 @@
"reporting/Read-Ember.cpp",
"reporting/Read-Ember.h",
]
- public_deps += [
- "${chip_root}/src/app/data-model-provider",
- "${chip_root}/src/app/ember_coupling",
- ]
+ public_deps += [ "${chip_root}/src/app/data-model-provider" ]
} else { # enabled
sources += [
"reporting/Read-DataModel.cpp",
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
index 40ec6e7..49baa70 100644
--- a/src/app/InteractionModelEngine.cpp
+++ b/src/app/InteractionModelEngine.cpp
@@ -27,10 +27,13 @@
#include <cinttypes>
+#include <access/AccessRestrictionProvider.h>
+#include <access/Privilege.h>
#include <access/RequestPath.h>
#include <access/SubjectDescriptor.h>
#include <app/AppConfig.h>
#include <app/CommandHandlerInterfaceRegistry.h>
+#include <app/EventPathParams.h>
#include <app/RequiredPrivilege.h>
#include <app/data-model-provider/ActionReturnStatus.h>
#include <app/data-model-provider/MetadataTypes.h>
@@ -39,6 +42,7 @@
#include <app/util/af-types.h>
#include <app/util/ember-compatibility-functions.h>
#include <app/util/endpoint-config-api.h>
+#include <lib/core/CHIPError.h>
#include <lib/core/DataModelTypes.h>
#include <lib/core/Global.h>
#include <lib/core/TLVUtilities.h>
@@ -53,12 +57,148 @@
#include <app/codegen-data-model-provider/Instance.h>
#endif
-#if !CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
-#include <app/ember_coupling/EventPathValidity.mixin.h> // nogncheck
-#endif
-
namespace chip {
namespace app {
+namespace {
+
+/**
+ * Helper to handle wildcard events in the event path.
+ *
+ * Validates that ACL access is permitted to:
+ * - Cluster::View in case the path is a wildcard for the event id
+ * - Event read if the path is a concrete event path
+ */
+bool MayHaveAccessibleEventPathForEndpointAndCluster(const ConcreteClusterPath & path, const EventPathParams & aEventPath,
+ const Access::SubjectDescriptor & aSubjectDescriptor)
+{
+ Access::RequestPath requestPath{ .cluster = path.mClusterId,
+ .endpoint = path.mEndpointId,
+ .requestType = Access::RequestType::kEventReadRequest };
+
+ Access::Privilege requiredPrivilege = Access::Privilege::kView;
+
+ if (!aEventPath.HasWildcardEventId())
+ {
+ requestPath.entityId = aEventPath.mEventId;
+ requiredPrivilege =
+ RequiredPrivilege::ForReadEvent(ConcreteEventPath(path.mEndpointId, path.mClusterId, aEventPath.mEventId));
+ }
+
+ return (Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, requiredPrivilege) == CHIP_NO_ERROR);
+}
+
+#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
+
+bool MayHaveAccessibleEventPathForEndpoint(DataModel::Provider * aProvider, EndpointId aEndpoint,
+ const EventPathParams & aEventPath, const Access::SubjectDescriptor & aSubjectDescriptor)
+{
+ if (!aEventPath.HasWildcardClusterId())
+ {
+ return MayHaveAccessibleEventPathForEndpointAndCluster(ConcreteClusterPath(aEndpoint, aEventPath.mClusterId), aEventPath,
+ aSubjectDescriptor);
+ }
+
+ DataModel::ClusterEntry clusterEntry = aProvider->FirstCluster(aEventPath.mEndpointId);
+ while (clusterEntry.IsValid())
+ {
+ if (MayHaveAccessibleEventPathForEndpointAndCluster(clusterEntry.path, aEventPath, aSubjectDescriptor))
+ {
+ return true;
+ }
+ clusterEntry = aProvider->NextCluster(clusterEntry.path);
+ }
+
+ return false;
+}
+
+bool MayHaveAccessibleEventPath(DataModel::Provider * aProvider, const EventPathParams & aEventPath,
+ const Access::SubjectDescriptor & subjectDescriptor)
+{
+ VerifyOrReturnValue(aProvider != nullptr, false);
+
+ if (!aEventPath.HasWildcardEndpointId())
+ {
+ return MayHaveAccessibleEventPathForEndpoint(aProvider, aEventPath.mEndpointId, aEventPath, subjectDescriptor);
+ }
+
+ for (EndpointId endpointId = aProvider->FirstEndpoint(); endpointId != kInvalidEndpointId;
+ endpointId = aProvider->NextEndpoint(endpointId))
+ {
+ if (MayHaveAccessibleEventPathForEndpoint(aProvider, endpointId, aEventPath, subjectDescriptor))
+ {
+ return true;
+ }
+ }
+ return false;
+}
+
+#else
+
+/**
+ * Helper to handle wildcard clusters in the event path.
+ */
+bool MayHaveAccessibleEventPathForEndpoint(EndpointId aEndpoint, const EventPathParams & aEventPath,
+ const Access::SubjectDescriptor & aSubjectDescriptor)
+{
+ if (aEventPath.HasWildcardClusterId())
+ {
+ auto * endpointType = emberAfFindEndpointType(aEndpoint);
+ if (endpointType == nullptr)
+ {
+ // Not going to have any valid paths in here.
+ return false;
+ }
+
+ for (decltype(endpointType->clusterCount) idx = 0; idx < endpointType->clusterCount; ++idx)
+ {
+ bool mayHaveAccessiblePath = MayHaveAccessibleEventPathForEndpointAndCluster(
+ ConcreteClusterPath(aEndpoint, endpointType->cluster[idx].clusterId), aEventPath, aSubjectDescriptor);
+ if (mayHaveAccessiblePath)
+ {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ auto * cluster = emberAfFindServerCluster(aEndpoint, aEventPath.mClusterId);
+ if (cluster == nullptr)
+ {
+ // Nothing valid here.
+ return false;
+ }
+ return MayHaveAccessibleEventPathForEndpointAndCluster(ConcreteClusterPath(aEndpoint, cluster->clusterId), aEventPath,
+ aSubjectDescriptor);
+}
+
+bool MayHaveAccessibleEventPath(const EventPathParams & aEventPath, const Access::SubjectDescriptor & aSubjectDescriptor)
+{
+ if (!aEventPath.HasWildcardEndpointId())
+ {
+ // No need to check whether the endpoint is enabled, because
+ // emberAfFindEndpointType returns null for disabled endpoints.
+ return MayHaveAccessibleEventPathForEndpoint(aEventPath.mEndpointId, aEventPath, aSubjectDescriptor);
+ }
+
+ for (uint16_t endpointIndex = 0; endpointIndex < emberAfEndpointCount(); ++endpointIndex)
+ {
+ if (!emberAfEndpointIndexIsEnabled(endpointIndex))
+ {
+ continue;
+ }
+ if (MayHaveAccessibleEventPathForEndpoint(emberAfEndpointFromIndex(endpointIndex), aEventPath, aSubjectDescriptor))
+ {
+ return true;
+ }
+ }
+
+ // none of the paths matched
+ return false;
+}
+#endif
+
+} // namespace
class AutoReleaseSubscriptionInfoIterator
{
@@ -583,30 +723,13 @@
continue;
}
-#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
- aHasValidEventPath = mDataModelProvider->EventPathIncludesAccessibleConcretePath(eventPath, aSubjectDescriptor);
-#else
// The definition of "valid path" is "path exists and ACL allows
// access". We need to do some expansion of wildcards to handle that.
- if (eventPath.HasWildcardEndpointId())
- {
- for (uint16_t endpointIndex = 0; !aHasValidEventPath && endpointIndex < emberAfEndpointCount(); ++endpointIndex)
- {
- if (!emberAfEndpointIndexIsEnabled(endpointIndex))
- {
- continue;
- }
- aHasValidEventPath =
- HasValidEventPathForEndpoint(emberAfEndpointFromIndex(endpointIndex), eventPath, aSubjectDescriptor);
- }
- }
- else
- {
- // No need to check whether the endpoint is enabled, because
- // emberAfFindEndpointType returns null for disabled endpoints.
- aHasValidEventPath = HasValidEventPathForEndpoint(eventPath.mEndpointId, eventPath, aSubjectDescriptor);
- }
-#endif // CHIP_CONFIG_USE_EMBER_DATA_MODEL
+#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
+ aHasValidEventPath = MayHaveAccessibleEventPath(mDataModelProvider, eventPath, aSubjectDescriptor);
+#else
+ aHasValidEventPath = MayHaveAccessibleEventPath(eventPath, aSubjectDescriptor);
+#endif
}
if (err == CHIP_ERROR_END_OF_TLV)
@@ -676,7 +799,7 @@
size_t requestedEventPathCount = 0;
AttributePathIBs::Parser attributePathListParser;
bool hasValidAttributePath = false;
- bool hasValidEventPath = false;
+ bool mayHaveValidEventPath = false;
CHIP_ERROR err = subscribeRequestParser.GetAttributeRequests(&attributePathListParser);
if (err == CHIP_NO_ERROR)
@@ -699,7 +822,7 @@
if (err == CHIP_NO_ERROR)
{
auto subjectDescriptor = apExchangeContext->GetSessionHandle()->AsSecureSession()->GetSubjectDescriptor();
- err = ParseEventPaths(subjectDescriptor, eventPathListParser, hasValidEventPath, requestedEventPathCount);
+ err = ParseEventPaths(subjectDescriptor, eventPathListParser, mayHaveValidEventPath, requestedEventPathCount);
if (err != CHIP_NO_ERROR)
{
return Status::InvalidAction;
@@ -719,7 +842,7 @@
return Status::InvalidAction;
}
- if (!hasValidAttributePath && !hasValidEventPath)
+ if (!hasValidAttributePath && !mayHaveValidEventPath)
{
ChipLogError(InteractionModel,
"Subscription from [%u:" ChipLogFormatX64 "] has no access at all. Rejecting request.",
diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp
index dd320e5..eab221a 100644
--- a/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp
+++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp
@@ -33,9 +33,6 @@
#include <lib/core/DataModelTypes.h>
#include <lib/support/CodeUtils.h>
-// separated out for code-reuse
-#include <app/ember_coupling/EventPathValidity.mixin.h>
-
#include <optional>
#include <variant>
@@ -772,30 +769,5 @@
return DeviceTypeEntryFromEmber(deviceTypes[idx]);
}
-bool CodegenDataModelProvider::EventPathIncludesAccessibleConcretePath(const EventPathParams & path,
- const Access::SubjectDescriptor & descriptor)
-{
-
- if (!path.HasWildcardEndpointId())
- {
- // No need to check whether the endpoint is enabled, because
- // emberAfFindEndpointType returns null for disabled endpoints.
- return HasValidEventPathForEndpoint(path.mEndpointId, path, descriptor);
- }
-
- for (uint16_t endpointIndex = 0; endpointIndex < emberAfEndpointCount(); ++endpointIndex)
- {
- if (!emberAfEndpointIndexIsEnabled(endpointIndex))
- {
- continue;
- }
- if (HasValidEventPathForEndpoint(emberAfEndpointFromIndex(endpointIndex), path, descriptor))
- {
- return true;
- }
- }
- return false;
-}
-
} // namespace app
} // namespace chip
diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h
index 5c87e26..7549ef1 100644
--- a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h
+++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h
@@ -141,8 +141,6 @@
return CHIP_NO_ERROR;
}
- bool EventPathIncludesAccessibleConcretePath(const EventPathParams & path,
- const Access::SubjectDescriptor & descriptor) override;
DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request,
AttributeValueEncoder & encoder) override;
DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
diff --git a/src/app/codegen-data-model-provider/model.gni b/src/app/codegen-data-model-provider/model.gni
index 4205c6f..f134d51 100644
--- a/src/app/codegen-data-model-provider/model.gni
+++ b/src/app/codegen-data-model-provider/model.gni
@@ -39,6 +39,5 @@
codegen_data_model_PUBLIC_DEPS = [
"${chip_root}/src/app/common:attribute-type",
"${chip_root}/src/app/data-model-provider",
- "${chip_root}/src/app/ember_coupling",
"${chip_root}/src/app/codegen-data-model-provider:instance-header",
]
diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h
index 2bafd1a..16dd732 100644
--- a/src/app/data-model-provider/Provider.h
+++ b/src/app/data-model-provider/Provider.h
@@ -59,16 +59,6 @@
// event emitting, path marking and other operations
virtual InteractionModelContext CurrentContext() const { return mContext; }
- /// Validates that the given event path is supported, where path may contain wildcards.
- ///
- /// If any wild cards exist on the given path, the implementation is expected to validate
- /// that an accessible event path exists on some wildcard expansion.
- ///
- /// At the very minimum this will validate that a valid endpoint/cluster can be expanded
- /// from the input path and that the given descriptor has access to it.
- virtual bool EventPathIncludesAccessibleConcretePath(const EventPathParams & path,
- const Access::SubjectDescriptor & descriptor) = 0;
-
/// TEMPORARY/TRANSITIONAL requirement for transitioning from ember-specific code
/// ReadAttribute is REQUIRED to perform:
/// - ACL validation (see notes on OperationFlags::kInternal)
diff --git a/src/app/ember_coupling/BUILD.gn b/src/app/ember_coupling/BUILD.gn
deleted file mode 100644
index 422fc8b..0000000
--- a/src/app/ember_coupling/BUILD.gn
+++ /dev/null
@@ -1,23 +0,0 @@
-# Copyright (c) 2024 Project CHIP Authors
-#
-# 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.
-
-import("//build_overrides/chip.gni")
-
-# The sources in this directory exist to limit the amount of code duplication
-# and assume strong ember and access coupling
-#
-# They are NOT stand-alone compilable and are rather for `#include` support.
-source_set("ember_coupling") {
- sources = [ "EventPathValidity.mixin.h" ]
-}
diff --git a/src/app/ember_coupling/EventPathValidity.mixin.h b/src/app/ember_coupling/EventPathValidity.mixin.h
deleted file mode 100644
index 54b8bcf..0000000
--- a/src/app/ember_coupling/EventPathValidity.mixin.h
+++ /dev/null
@@ -1,104 +0,0 @@
-/*
- *
- * Copyright (c) 2020-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.
- */
-
-// This is intended as a TOP LEVEL INCLUDE inside code that uses it
-
-namespace chip {
-namespace app {
-namespace {
-
-static bool CanAccessEvent(const Access::SubjectDescriptor & aSubjectDescriptor, const ConcreteClusterPath & aPath,
- Access::Privilege aNeededPrivilege)
-{
- Access::RequestPath requestPath{ .cluster = aPath.mClusterId,
- .endpoint = aPath.mEndpointId,
- .requestType = Access::RequestType::kEventReadRequest };
- // leave requestPath.entityId optional value unset to indicate wildcard
- CHIP_ERROR err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, aNeededPrivilege);
- return (err == CHIP_NO_ERROR);
-}
-
-static bool CanAccessEvent(const Access::SubjectDescriptor & aSubjectDescriptor, const ConcreteEventPath & aPath)
-{
- Access::RequestPath requestPath{ .cluster = aPath.mClusterId,
- .endpoint = aPath.mEndpointId,
- .requestType = Access::RequestType::kEventReadRequest,
- .entityId = aPath.mEventId };
- CHIP_ERROR err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, RequiredPrivilege::ForReadEvent(aPath));
- return (err == CHIP_NO_ERROR);
-}
-
-/**
- * Helper to handle wildcard events in the event path.
- */
-static bool HasValidEventPathForEndpointAndCluster(EndpointId aEndpoint, const EmberAfCluster * aCluster,
- const EventPathParams & aEventPath,
- const Access::SubjectDescriptor & aSubjectDescriptor)
-{
- if (aEventPath.HasWildcardEventId())
- {
- // We have no way to expand wildcards. Just assume that we would need
- // View permissions for whatever events are involved.
- ConcreteClusterPath clusterPath(aEndpoint, aCluster->clusterId);
- return CanAccessEvent(aSubjectDescriptor, clusterPath, Access::Privilege::kView);
- }
-
- ConcreteEventPath path(aEndpoint, aCluster->clusterId, aEventPath.mEventId);
- return CanAccessEvent(aSubjectDescriptor, path);
-}
-
-/**
- * Helper to handle wildcard clusters in the event path.
- */
-static bool HasValidEventPathForEndpoint(EndpointId aEndpoint, const EventPathParams & aEventPath,
- const Access::SubjectDescriptor & aSubjectDescriptor)
-{
- if (aEventPath.HasWildcardClusterId())
- {
- auto * endpointType = emberAfFindEndpointType(aEndpoint);
- if (endpointType == nullptr)
- {
- // Not going to have any valid paths in here.
- return false;
- }
-
- for (decltype(endpointType->clusterCount) idx = 0; idx < endpointType->clusterCount; ++idx)
- {
- bool hasValidPath =
- HasValidEventPathForEndpointAndCluster(aEndpoint, &endpointType->cluster[idx], aEventPath, aSubjectDescriptor);
- if (hasValidPath)
- {
- return true;
- }
- }
-
- return false;
- }
-
- auto * cluster = emberAfFindServerCluster(aEndpoint, aEventPath.mClusterId);
- if (cluster == nullptr)
- {
- // Nothing valid here.
- return false;
- }
- return HasValidEventPathForEndpointAndCluster(aEndpoint, cluster, aEventPath, aSubjectDescriptor);
-}
-
-} // namespace
-} // namespace app
-} // namespace chip
diff --git a/src/app/tests/test-interaction-model-api.h b/src/app/tests/test-interaction-model-api.h
index ae5cc63..df410c6 100644
--- a/src/app/tests/test-interaction-model-api.h
+++ b/src/app/tests/test-interaction-model-api.h
@@ -117,11 +117,6 @@
CHIP_ERROR Shutdown() override { return CHIP_NO_ERROR; }
- bool EventPathIncludesAccessibleConcretePath(const EventPathParams & path,
- const Access::SubjectDescriptor & descriptor) override
- {
- return true;
- }
DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request,
AttributeValueEncoder & encoder) override;
DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
diff --git a/src/controller/tests/data_model/DataModelFixtures.h b/src/controller/tests/data_model/DataModelFixtures.h
index bdc251e..e9b32ae 100644
--- a/src/controller/tests/data_model/DataModelFixtures.h
+++ b/src/controller/tests/data_model/DataModelFixtures.h
@@ -115,11 +115,6 @@
CHIP_ERROR Shutdown() override { return CHIP_NO_ERROR; }
- bool EventPathIncludesAccessibleConcretePath(const EventPathParams & path,
- const Access::SubjectDescriptor & descriptor) override
- {
- return true;
- }
DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request,
AttributeValueEncoder & encoder) override;
DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
