Separate DataModel APIs for encoding fabric-sensitive structs (#15284)

* Separate DataModel APIs for encoding fabric-sensitive structs

This builds upon bzbarsky's original PR #15268 based upon discussions
with him to create separate EncodeForWrite/EncodeForRead APIs that only
apply to fabric-scoped structs.

This is then used (for now) to omit encoding the fabric index on writes.

Tests: Ran chip-tool to write an ACL entry and confirmed that that the
fabric index field is omitted on writes.

* WIP

* Review feedback

* Tests were using fabric index 0 for fake fabric testing, but that trips the invalid fabric checker in AttributeValueEncoder::EncodeListItem...

* Review feedback

examples/chip-tool/commands/clusters/CustomArgument.h

@@ -255,6 +255,10 @@ class CustomArgument
         return writer.CopyElement(tag, reader);
     }
 
+    // We trust our consumers to do the encoding of our data correctly, so don't
+    // need to know whether we are being encoded for a write.
+    static constexpr bool kIsFabricScoped = false;
+
 private:
     uint8_t * mData                       = nullptr;
     uint32_t mDataLen                     = 0;

src/app/AttributeAccessInterface.h

@@ -77,11 +77,19 @@ class AttributeReportBuilder
     /**
      * EncodeValue encodes the value field of the report, it should be called exactly once.
      */
-    template <typename... Ts>
-    CHIP_ERROR EncodeValue(AttributeReportIBs::Builder & aAttributeReportIBs, Ts &&... aArgs)
+    template <typename T, std::enable_if_t<!DataModel::IsFabricScoped<T>::value, bool> = true, typename... Ts>
+    CHIP_ERROR EncodeValue(AttributeReportIBs::Builder & aAttributeReportIBs, T && item, Ts &&... aArgs)
     {
         return DataModel::Encode(*(aAttributeReportIBs.GetAttributeReport().GetAttributeData().GetWriter()),
-                                 TLV::ContextTag(to_underlying(AttributeDataIB::Tag::kData)), std::forward<Ts>(aArgs)...);
+                                 TLV::ContextTag(to_underlying(AttributeDataIB::Tag::kData)), item, std::forward<Ts>(aArgs)...);
+    }
+
+    template <typename T, std::enable_if_t<DataModel::IsFabricScoped<T>::value, bool> = true, typename... Ts>
+    CHIP_ERROR EncodeValue(AttributeReportIBs::Builder & aAttributeReportIBs, FabricIndex accessingFabricIndex, T && item,
+                           Ts &&... aArgs)
+    {
+        return DataModel::EncodeForRead(*(aAttributeReportIBs.GetAttributeReport().GetAttributeData().GetWriter()),
+                                        TLV::ContextTag(to_underlying(AttributeDataIB::Tag::kData)), accessingFabricIndex, item);
     }
 };
 
@@ -104,12 +112,14 @@ class AttributeValueEncoder
         template <typename T, std::enable_if_t<DataModel::IsFabricScoped<T>::value, bool> = true>
         CHIP_ERROR Encode(T && aArg) const
         {
+            VerifyOrReturnError(aArg.GetFabricIndex() != kUndefinedFabricIndex, CHIP_ERROR_INVALID_FABRIC_ID);
+
             // If we are encoding for a fabric filtered attribute read and the fabric index does not match that present in the
             // request, skip encoding this list item.
             VerifyOrReturnError(!mAttributeValueEncoder.mIsFabricFiltered ||
                                     aArg.GetFabricIndex() == mAttributeValueEncoder.mAccessingFabricIndex,
                                 CHIP_NO_ERROR);
-            return mAttributeValueEncoder.EncodeListItem(std::forward<T>(aArg));
+            return mAttributeValueEncoder.EncodeListItem(mAttributeValueEncoder.mAccessingFabricIndex, std::forward<T>(aArg));
         }
 
         template <typename T, std::enable_if_t<!DataModel::IsFabricScoped<T>::value, bool> = true>

src/app/EventLogging.h

@@ -75,6 +75,15 @@ CHIP_ERROR LogEvent(const T & aEventData, EndpointId aEndpoint, EventNumber & aE
     eventOptions.mPath        = path;
     eventOptions.mPriority    = aEventData.GetPriorityLevel();
     eventOptions.mFabricIndex = aEventData.GetFabricIndex();
+
+    //
+    // Unlike attributes which have a different 'EncodeForRead' for fabric-scoped structs,
+    // fabric-sensitive events don't require that since the actual omission of the event in its entirety
+    // happens within the event management framework itself at the time of access.
+    //
+    // The 'mFabricIndex' field in the event options above is encoded out-of-band alongside the event payload
+    // and used to match against the accessing fabric.
+    //
     return logMgmt.LogEvent(&eventData, eventOptions, aEventNumber);
 }
 

src/app/WriteClient.h

@@ -26,6 +26,7 @@
 #include <app/MessageDef/StatusIB.h>
 #include <app/MessageDef/WriteRequestMessage.h>
 #include <app/data-model/Encode.h>
+#include <app/data-model/FabricScoped.h>
 #include <app/data-model/List.h>
 #include <lib/core/CHIPCore.h>
 #include <lib/core/CHIPTLVDebug.hpp>
@@ -260,7 +261,7 @@ class WriteClient : public Messaging::ExchangeDelegate
     /**
      *  Encode an attribute value that can be directly encoded using DataModel::Encode.
      */
-    template <class T>
+    template <class T, std::enable_if_t<!DataModel::IsFabricScoped<T>::value, int> = 0>
     CHIP_ERROR TryEncodeSingleAttributeDataIB(const ConcreteDataAttributePath & attributePath, const T & value)
     {
         chip::TLV::TLVWriter * writer = nullptr;
@@ -274,6 +275,20 @@ class WriteClient : public Messaging::ExchangeDelegate
         return CHIP_NO_ERROR;
     }
 
+    template <class T, std::enable_if_t<DataModel::IsFabricScoped<T>::value, int> = 0>
+    CHIP_ERROR TryEncodeSingleAttributeDataIB(const ConcreteDataAttributePath & attributePath, const T & value)
+    {
+        chip::TLV::TLVWriter * writer = nullptr;
+
+        ReturnErrorOnFailure(PrepareAttributeIB(attributePath));
+        VerifyOrReturnError((writer = GetAttributeDataIBTLVWriter()) != nullptr, CHIP_ERROR_INCORRECT_STATE);
+        ReturnErrorOnFailure(DataModel::EncodeForWrite(
+            *writer, chip::TLV::ContextTag(to_underlying(chip::app::AttributeDataIB::Tag::kData)), value));
+        ReturnErrorOnFailure(FinishAttributeIB());
+
+        return CHIP_NO_ERROR;
+    }
+
     /**
      * A wrapper for TryEncodeSingleAttributeDataIB which will start a new chunk when failed with CHIP_ERROR_NO_MEMORY or
      * CHIP_ERROR_BUFFER_TOO_SMALL.

src/app/clusters/access-control-server/access-control-server.cpp

@@ -225,7 +225,7 @@ struct AccessControlEntryCodec
         return CHIP_NO_ERROR;
     }
 
-    CHIP_ERROR Encode(TLV::TLVWriter & aWriter, TLV::Tag aTag) const
+    CHIP_ERROR EncodeForRead(TLV::TLVWriter & aWriter, TLV::Tag aTag, FabricIndex accessingFabricIndex) const
     {
         AccessControlCluster::Structs::AccessControlEntry::Type staging;
 
@@ -272,7 +272,7 @@ struct AccessControlEntryCodec
             staging.targets.SetNonNull(targetBuffer, targetCount);
         }
 
-        return staging.Encode(aWriter, aTag);
+        return staging.EncodeForRead(aWriter, aTag, accessingFabricIndex);
     }
 
     CHIP_ERROR Decode(TLV::TLVReader & aReader)

src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp

@@ -72,7 +72,7 @@ struct GroupTableCodec
 
     auto GetFabricIndex() const { return mFabric; }
 
-    CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag) const
+    CHIP_ERROR EncodeForRead(TLV::TLVWriter & writer, TLV::Tag tag, FabricIndex accessingFabricIndex) const
     {
         TLV::TLVType outer;
         ReturnErrorOnFailure(writer.StartContainer(tag, TLV::kTLVType_Structure, outer));

src/app/clusters/test-cluster-server/test-cluster-server.cpp

@@ -546,9 +546,6 @@ CHIP_ERROR TestAttrAccess::ReadListFabricScopedAttribute(AttributeValueEncoder &
             ReturnErrorOnFailure(encoder.Encode(val));
         }
 
-        // Always append a fake fabric index so we can test fabric filter even when there is only one fabric provisioned.
-        val.fabricIndex = kUndefinedFabricIndex;
-        ReturnErrorOnFailure(encoder.Encode(val));
         return CHIP_NO_ERROR;
     });
 }

src/app/data-model/Encode.h

@@ -18,8 +18,10 @@
 
 #pragma once
 
+#include <app/data-model/FabricScoped.h>
 #include <app/data-model/Nullable.h>
 #include <lib/core/CHIPTLV.h>
+#include <lib/core/DataModelTypes.h>
 #include <lib/core/Optional.h>
 #include <protocols/interaction_model/Constants.h>
 
@@ -71,7 +73,7 @@ inline CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, Span<const char>
 /*
  * @brief
  *
- * This specific variant that encodes cluster objects (like structs, commands, events) to TLV
+ * This specific variant that encodes cluster objects (like non fabric-scoped structs, commands, events) to TLV
  * depends on the presence of an Encode method on the object. The signature of that method
  * is as follows:
  *
@@ -90,6 +92,44 @@ CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, const X & x)
     return x.Encode(writer, tag);
 }
 
+/*
+ * @brief
+ *
+ * A way to encode fabric-scoped structs for a write that omits encoding the containing fabric index field.
+ */
+template <typename X,
+          typename std::enable_if_t<std::is_class<X>::value &&
+                                        std::is_same<decltype(std::declval<X>().EncodeForWrite(std::declval<TLV::TLVWriter &>(),
+                                                                                               std::declval<TLV::Tag>())),
+                                                     CHIP_ERROR>::value &&
+                                        DataModel::IsFabricScoped<X>::value,
+                                    X> * = nullptr>
+CHIP_ERROR EncodeForWrite(TLV::TLVWriter & writer, TLV::Tag tag, const X & x)
+{
+    return x.EncodeForWrite(writer, tag);
+}
+
+/*
+ * @brief
+ *
+ * A way to encode fabric-scoped structs for a read that always encodes the containing fabric index field.
+ *
+ * An accessing fabric index must be passed in to permit including/omitting sensitive fields based on a match with the fabric index
+ * associated with the scoped struct.
+ */
+template <typename X,
+          typename std::enable_if_t<
+              std::is_class<X>::value &&
+                  std::is_same<decltype(std::declval<X>().EncodeForRead(std::declval<TLV::TLVWriter &>(), std::declval<TLV::Tag>(),
+                                                                        std::declval<FabricIndex>())),
+                               CHIP_ERROR>::value &&
+                  DataModel::IsFabricScoped<X>::value,
+              X> * = nullptr>
+CHIP_ERROR EncodeForRead(TLV::TLVWriter & writer, TLV::Tag tag, FabricIndex accessingFabricIndex, const X & x)
+{
+    return x.EncodeForRead(writer, tag, accessingFabricIndex);
+}
+
 /*
  * @brief
  *
@@ -140,6 +180,54 @@ CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, const Nullable<X> & x)
 #pragma GCC diagnostic pop
 }
 
+/*
+ * @brief
+ *
+ * Encodes a nullable fabric-scoped struct for a write.
+ */
+template <typename X, std::enable_if_t<DataModel::IsFabricScoped<X>::value, bool> = true>
+CHIP_ERROR EncodeForWrite(TLV::TLVWriter & writer, TLV::Tag tag, const Nullable<X> & x)
+{
+    if (x.IsNull())
+    {
+        return writer.PutNull(tag);
+    }
+
+    // The -Wmaybe-uninitialized warning gets confused about the fact
+    // that x.mValue is always initialized if x.IsNull() is not
+    // true, so suppress it for our access to x.Value().
+#pragma GCC diagnostic push
+#if !defined(__clang__)
+#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+#endif // !defined(__clang__)
+    return EncodeForWrite(writer, tag, x.Value());
+#pragma GCC diagnostic pop
+}
+
+/*
+ * @brief
+ *
+ * Encodes a nullable fabric-scoped struct for a read.
+ */
+template <typename X, std::enable_if_t<DataModel::IsFabricScoped<X>::value, bool> = true>
+CHIP_ERROR EncodeForRead(TLV::TLVWriter & writer, TLV::Tag tag, FabricIndex accessingFabricIndex, const Nullable<X> & x)
+{
+    if (x.IsNull())
+    {
+        return writer.PutNull(tag);
+    }
+
+    // The -Wmaybe-uninitialized warning gets confused about the fact
+    // that x.mValue is always initialized if x.IsNull() is not
+    // true, so suppress it for our access to x.Value().
+#pragma GCC diagnostic push
+#if !defined(__clang__)
+#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+#endif // !defined(__clang__)
+    return EncodeForRead(writer, tag, accessingFabricIndex, x.Value());
+#pragma GCC diagnostic pop
+}
+
 } // namespace DataModel
 } // namespace app
 } // namespace chip

src/app/data-model/List.h

@@ -20,6 +20,7 @@
 
 #include <app/data-model/Decode.h>
 #include <app/data-model/Encode.h>
+#include <app/data-model/FabricScoped.h>
 #include <lib/core/CHIPTLV.h>
 
 namespace chip {
@@ -55,6 +56,11 @@ struct List : public Span<T>
         Span<T>::operator=(databuf);
         return (*this);
     }
+
+    //
+    // A list is deemed fabric scoped if the type of its elements is as well.
+    //
+    static constexpr bool kIsFabricScoped = DataModel::IsFabricScoped<T>::value;
 };
 
 template <typename X>
@@ -72,6 +78,36 @@ inline CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, List<X> list)
     return CHIP_NO_ERROR;
 }
 
+template <typename X, std::enable_if_t<DataModel::IsFabricScoped<X>::value, bool> = true>
+inline CHIP_ERROR EncodeForWrite(TLV::TLVWriter & writer, TLV::Tag tag, List<X> list)
+{
+    TLV::TLVType type;
+
+    ReturnErrorOnFailure(writer.StartContainer(tag, TLV::kTLVType_Array, type));
+    for (auto & item : list)
+    {
+        ReturnErrorOnFailure(EncodeForWrite(writer, TLV::AnonymousTag(), item));
+    }
+    ReturnErrorOnFailure(writer.EndContainer(type));
+
+    return CHIP_NO_ERROR;
+}
+
+template <typename X, std::enable_if_t<DataModel::IsFabricScoped<X>::value, bool> = true>
+inline CHIP_ERROR EncodeForRead(TLV::TLVWriter & writer, TLV::Tag tag, FabricIndex accessingFabricIndex, List<X> list)
+{
+    TLV::TLVType type;
+
+    ReturnErrorOnFailure(writer.StartContainer(tag, TLV::kTLVType_Array, type));
+    for (auto & item : list)
+    {
+        ReturnErrorOnFailure(EncodeForRead(writer, TLV::AnonymousTag(), accessingFabricIndex, item));
+    }
+    ReturnErrorOnFailure(writer.EndContainer(type));
+
+    return CHIP_NO_ERROR;
+}
+
 } // namespace DataModel
 } // namespace app
 } // namespace chip

src/app/tests/TestAttributeValueDecoder.cpp

@@ -52,7 +52,7 @@ struct TestSetup
         TLVType ignored;
         writer.Init(buf);
         ReturnErrorOnFailure(writer.StartContainer(AnonymousTag(), kTLVType_Structure, ignored));
-        ReturnErrorOnFailure(DataModel::Encode(writer, TLV::ContextTag(0), value));
+        ReturnErrorOnFailure(DataModel::EncodeForWrite(writer, TLV::ContextTag(0), value));
         ReturnErrorOnFailure(writer.EndContainer(ignored));
         ReturnErrorOnFailure(writer.Finalize());
         return CHIP_NO_ERROR;

src/app/tests/TestAttributeValueEncoder.cpp

@@ -255,9 +255,9 @@ void TestEncodeFabricScoped(nlTestSuite * aSuite, void * aContext)
 {
     TestSetup test(aSuite, kTestFabricIndex);
     Clusters::AccessControl::Structs::ExtensionEntry::Type items[3];
-    items[0].fabricIndex = 0;
-    items[1].fabricIndex = 1;
-    items[2].fabricIndex = 2;
+    items[0].fabricIndex = 1;
+    items[1].fabricIndex = 2;
+    items[2].fabricIndex = 3;
 
     // We tried to encode three items, however, the encoder should only put the item with matching fabric index into the final list.
     CHIP_ERROR err = test.encoder.EncodeList([items](const auto & encoder) -> CHIP_ERROR {