Event validation simplification - pull logic into InteractionModelEng…

…ine.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 <[email protected]> * Rename method * Restyle * More renames * Restyle * Fix some renames * Restyle * A few more renames * Restyle --------- Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Nov 22, 2024 · 4414201 · 4414201
1 parent ff66bd5
commit 4414201
Show file tree

Hide file tree

Showing 10 changed files with 153 additions and 212 deletions.
diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn
@@ -255,7 +255,6 @@ static_library("interaction-model") {
       "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 @@ static_library("interaction-model") {
       "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
@@ -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 @@ CHIP_ERROR InteractionModelEngine::ParseEventPaths(const Access::SubjectDescript
             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 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
             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 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
             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 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
                 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
@@ -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 @@ std::optional<DataModel::DeviceTypeEntry> CodegenDataModelProvider::NextDeviceTy
     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
@@ -141,8 +141,6 @@ class CodegenDataModelProvider : public chip::app::DataModel::Provider
         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
@@ -39,6 +39,5 @@ codegen_data_model_SOURCES = [
 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
@@ -59,16 +59,6 @@ class Provider : public ProviderMetadataTree
     // 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