Code review feedback: comment about internal flags and implement path expansion logic for skipping data encoding

Author: andreilitvin

src/app/codegen-data-model/CodegenDataModel_Read.cpp

@@ -14,6 +14,7 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+#include "lib/core/CHIPError.h"
 #include <app/codegen-data-model/CodegenDataModel.h>
 
 #include <optional>
@@ -112,7 +113,13 @@ std::optional<CHIP_ERROR> TryReadViaAccessInterface(const ConcreteAttributePath
 
     if (err != CHIP_NO_ERROR)
     {
-        return std::make_optional(err);
+        // Implementation of 8.4.3.2 of the spec for path expansion
+        if (path.mExpanded && (err == CHIP_IM_GLOBAL_STATUS(UnsupportedRead)))
+        {
+            return CHIP_NO_ERROR;
+        }
+
+        return err;
     }
 
     // If the encoder tried to encode, then a value should have been written.
@@ -293,8 +300,17 @@ CHIP_ERROR CodegenDataModel::ReadAttribute(const InteractionModel::ReadAttribute
         ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), CHIP_ERROR_INVALID_ARGUMENT);
 
         Access::RequestPath requestPath{ .cluster = request.path.mClusterId, .endpoint = request.path.mEndpointId };
-        ReturnErrorOnFailure(Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath,
-                                                              RequiredPrivilege::ForReadAttribute(request.path)));
+        CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath,
+                                                          RequiredPrivilege::ForReadAttribute(request.path));
+        if (err != CHIP_NO_ERROR)
+        {
+            // Implementation of 8.4.3.2 of the spec for path expansion
+            if (request.path.mExpanded && (err == CHIP_ERROR_ACCESS_DENIED))
+            {
+                return CHIP_NO_ERROR;
+            }
+            return err;
+        }
     }
 
     auto metadata = FindAttributeMetadata(request.path);

src/app/data-model-interface/DataModel.h

@@ -55,13 +55,18 @@ class DataModel : public DataModelMetadataTree
     // event emitting, path marking and other operations
     virtual InteractionModelContext CurrentContext() const { return mContext; }
 
+    /// TEMPORARY/TRANSITIONAL requirement for transitioning from ember-specific code
+    ///   ReadAttribute is REQUIRED to perform:
+    ///     - ACL validation (see notes on OperationFlags::kInternal)
+    ///     - Validation of readability/writability
+    ///     - use request.path.mExpanded to skip encoding replies for data according
+    ///       to 8.4.3.2 of the spec:
+    ///         > If the path indicates attribute data that is not readable, then the path SHALL 
+    ///           be discarded.
+    ///         > Else if reading from the attribute in the path requires a privilege that is not
+    ///           granted to access the cluster in the path, then the path SHALL be discarded.
+    ///
     /// Return codes:
-    ///   CHIP_ERROR_ACCESS_DENIED:
-    ///      - May be ignored if reads use path expansion (e.g. to skip inaccessible attributes
-    ///        during subscription requests). This code MUST be used for ACL failures and only for ACL failures.
-    ///   CHIP_IM_GLOBAL_STATUS(UnsupportedRead):
-    ///      - May be ignored if reads use path expansion (e.g. to skip reading write-only attributes
-    ///        during subscription requests).
     ///   CHIP_ERROR_NO_MEMORY or CHIP_ERROR_BUFFER_TOO_SMALL:
     ///      - Indicates that list encoding had insufficient buffer space to encode elements.
     ///      - encoder::GetState().AllowPartialData() determines if these errors are permanent (no partial

src/app/data-model-interface/OperationTypes.h

@@ -31,7 +31,17 @@ namespace InteractionModel {
 /// Contains common flags among all interaction model operations: read/write/invoke
 enum class OperationFlags : uint32_t
 {
-    kInternal = 0x0001, // Internal request for data changes (can bypass checks/ACL etc.)
+    // NOTE: temporary flag. This flag exists to faciliate transition from ember-compatibilty-functions
+    //       implementation to DataModel Interface functionality. Specifically currently the
+    //       model is expected to perform ACL and readability/writability checks.
+    //
+    //       In the future, this flag will be removed and InteractionModelEngine/ReportingEngine
+    //       will perform the required validation.
+    //
+    //       Currently the flag FORCES a bypass of:
+    //         - ACL validation (will allow any read/write)
+    //         - Access validation (will allow reading write-only data for example)
+    kInternal = 0x0001,
 };
 
 /// This information is available for ALL interactions: read/write/invoke
@@ -42,6 +52,8 @@ struct OperationRequest
     /// Current authentication data EXCEPT for internal requests.
     ///  - Non-internal requests MUST have this set.
     ///  - operationFlags.Has(OperationFlags::kInternal) MUST NOT have this set
+    ///
+    /// NOTE: once kInternal flag is removed, this will become non-optional
     std::optional<chip::Access::SubjectDescriptor> subjectDescriptor;
 };