Disallow writes to readonly attributes via AttributeAccessInterface (#…

…11942)

* Disallow writes to readonly attributes even when the writes are intercepted.

Need to move the IsReadOnly() check higher up the callstack.

* Mark attributes in test cluster writable as needed.

examples/chip-tool/templates/commands.zapt

@@ -459,6 +459,9 @@ private:
 };
 
 {{#if isWritableAttribute}}
+{{! No list support for writing yet.  Need to figure out how to represent the
+    values. }}
+{{#unless isList}}
 class Write{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}: public ModelCommand
 {
 public:
@@ -494,6 +497,7 @@ private:
     {{chipType}} mValue;
 };
 
+{{/unless}}
 {{/if}}
 {{#if isReportableAttribute}}
 class Report{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}: public ModelCommand
@@ -563,7 +567,11 @@ void registerCluster{{asUpperCamelCase name}}(Commands & commands)
         {{#chip_server_cluster_attributes}}
         make_unique<Read{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}>(), //
         {{#if isWritableAttribute}}
+        {{! No list support for writing yet.  Need to figure out how to
+            represent the values. }}
+        {{#unless isList}}
         make_unique<Write{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}>(), //
+        {{/unless}}
         {{/if}}
         {{#if isReportableAttribute}}
         make_unique<Report{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}>(), //

src/app/util/af-types.h

@@ -217,6 +217,11 @@ struct EmberAfAttributeMetadata
      * Check whether this attribute is nullable.
      */
     bool IsNullable() const { return mask & ATTRIBUTE_MASK_NULLABLE; }
+
+    /**
+     * Check whether this attribute is readonly.
+     */
+    bool IsReadOnly() const { return !(mask & ATTRIBUTE_MASK_WRITABLE); }
 };
 
 /**

src/app/util/af.h

@@ -373,13 +373,6 @@ uint8_t emberAfGetDataSize(uint8_t dataType);
  */
 #define emberAfClusterIsManufacturerSpecific(cluster) ((cluster)->clusterId >= 0xFC00)
 
-/**
- * @brief macro that returns true if attribute is read only.
- *
- * @param metadata EmberAfAttributeMetadata* to consider.
- */
-#define emberAfAttributeIsReadOnly(metadata) (((metadata)->mask & ATTRIBUTE_MASK_WRITABLE) == 0)
-
 /**
  * @brief macro that returns true if client attribute, and false if server.
  *

src/app/util/attribute-table.cpp

@@ -296,7 +296,7 @@ void emberAfPrintAttributeTable(void)
                     emberAfAttributesPrint("%2x", mfgCode);
                 }
                 emberAfAttributesPrint(" / %x (%x) / %p / %p / ", metaData->attributeType, emberAfAttributeSize(metaData),
-                                       (emberAfAttributeIsReadOnly(metaData) ? "RO" : "RW"),
+                                       (metaData->IsReadOnly() ? "RO" : "RW"),
                                        (emberAfAttributeIsTokenized(metaData)
                                             ? " token "
                                             : (emberAfAttributeIsExternal(metaData) ? "extern " : "  RAM  ")));
@@ -524,7 +524,7 @@ EmberAfStatus emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, Attribu
             return EMBER_ZCL_STATUS_INVALID_DATA_TYPE;
         }
 
-        if (emberAfAttributeIsReadOnly(metadata))
+        if (metadata->IsReadOnly())
         {
             emberAfAttributesPrintln("%pattr not writable", "WRITE ERR: ");
             emberAfAttributesFlush();

src/app/util/ember-compatibility-functions.cpp

@@ -542,61 +542,66 @@ CHIP_ERROR prepareWriteData(const EmberAfAttributeMetadata * metadata, TLV::TLVR
 }
 } // namespace
 
-static Protocols::InteractionModel::Status WriteSingleClusterDataInternal(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader,
-                                                                          WriteHandler * apWriteHandler)
+static Protocols::InteractionModel::Status WriteSingleClusterDataInternal(const ConcreteAttributePath aPath,
+                                                                          const EmberAfAttributeMetadata * aMetadata,
+                                                                          TLV::TLVReader & aReader, WriteHandler * apWriteHandler)
 {
-    // Passing nullptr as buf to emberAfReadAttribute means we only need attribute type here, and ember will not do data read &
-    // copy in this case.
-    const EmberAfAttributeMetadata * attributeMetadata = emberAfLocateAttributeMetadata(
-        aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mAttributeId, CLUSTER_MASK_SERVER, 0);
-
-    if (attributeMetadata == nullptr)
-    {
-        return Protocols::InteractionModel::Status::UnsupportedAttribute;
-    }
-
     CHIP_ERROR preparationError = CHIP_NO_ERROR;
     uint16_t dataLen            = 0;
-    if ((preparationError = prepareWriteData(attributeMetadata, aReader, dataLen)) != CHIP_NO_ERROR)
+    if ((preparationError = prepareWriteData(aMetadata, aReader, dataLen)) != CHIP_NO_ERROR)
     {
         ChipLogDetail(Zcl, "Failed to prepare data to write: %s", ErrorStr(preparationError));
         return Protocols::InteractionModel::Status::InvalidValue;
     }
 
-    if (dataLen > attributeMetadata->size)
+    if (dataLen > aMetadata->size)
     {
         ChipLogDetail(Zcl, "Data to write exceedes the attribute size claimed.");
         return Protocols::InteractionModel::Status::InvalidValue;
     }
 
-    return ToInteractionModelStatus(emberAfWriteAttributeExternal(aClusterInfo.mEndpointId, aClusterInfo.mClusterId,
-                                                                  aClusterInfo.mAttributeId, CLUSTER_MASK_SERVER, 0, attributeData,
-                                                                  attributeMetadata->attributeType));
+    return ToInteractionModelStatus(emberAfWriteAttributeExternal(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId,
+                                                                  CLUSTER_MASK_SERVER, 0, attributeData, aMetadata->attributeType));
 }
 
 CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader, WriteHandler * apWriteHandler)
 {
+    // TODO: Refactor WriteSingleClusterData and all dependent functions to take ConcreteAttributePath instead of ClusterInfo
+    // as the input argument.
     AttributePathParams attributePathParams;
     attributePathParams.mEndpointId  = aClusterInfo.mEndpointId;
     attributePathParams.mClusterId   = aClusterInfo.mClusterId;
     attributePathParams.mAttributeId = aClusterInfo.mAttributeId;
 
-    // TODO: Refactor WriteSingleClusterData and all dependent functions to take ConcreteAttributePath instead of ClusterInfo
-    // as the input argument.
+    // Named aPath for now to reduce the amount of code change that needs to
+    // happen when the above TODO is resolved.
+    ConcreteAttributePath aPath(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mAttributeId);
+    const EmberAfAttributeMetadata * attributeMetadata =
+        emberAfLocateAttributeMetadata(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId, CLUSTER_MASK_SERVER, 0);
+
+    if (attributeMetadata == nullptr)
+    {
+        return apWriteHandler->AddStatus(attributePathParams, Protocols::InteractionModel::Status::UnsupportedAttribute);
+    }
+
+    if (attributeMetadata->IsReadOnly())
+    {
+        return apWriteHandler->AddStatus(attributePathParams, Protocols::InteractionModel::Status::UnsupportedWrite);
+    }
+
     AttributeAccessInterface * attrOverride = findAttributeAccessOverride(aClusterInfo.mEndpointId, aClusterInfo.mClusterId);
     if (attrOverride != nullptr)
     {
-        ConcreteAttributePath path(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mAttributeId);
         AttributeValueDecoder valueDecoder(aReader);
-        ReturnErrorOnFailure(attrOverride->Write(path, valueDecoder));
+        ReturnErrorOnFailure(attrOverride->Write(aPath, valueDecoder));
 
         if (valueDecoder.TriedDecode())
         {
             return apWriteHandler->AddStatus(attributePathParams, Protocols::InteractionModel::Status::Success);
         }
     }
 
-    auto imCode = WriteSingleClusterDataInternal(aClusterInfo, aReader, apWriteHandler);
+    auto imCode = WriteSingleClusterDataInternal(aPath, attributeMetadata, aReader, apWriteHandler);
     return apWriteHandler->AddStatus(attributePathParams, imCode);
 }
 

src/app/zap-templates/templates/app/CHIPClusters-src.zapt

@@ -83,6 +83,7 @@ CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::ReadAttribute{{asUpperCamelC
 }
 
 {{#if isWritableAttribute}}
+{{#unless isList}}
 CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::WriteAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, {{chipType}} value)
 {
     app::WriteClientHandle handle;
@@ -91,6 +92,7 @@ CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::WriteAttribute{{asUpperCamel
     return mDevice->SendWriteAttributeRequest(std::move(handle), onSuccessCallback, onFailureCallback);
 }
 
+{{/unless}}
 {{/if}}
 {{#if isReportableAttribute}}
 CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::SubscribeAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, uint16_t minInterval, uint16_t maxInterval)

src/app/zap-templates/templates/app/CHIPClusters.zapt

@@ -34,7 +34,9 @@ public:
     {{/chip_server_cluster_attributes}}
     {{#chip_server_cluster_attributes}}
     {{#if isWritableAttribute}}
+    {{#unless isList}}
     CHIP_ERROR WriteAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, {{chipType}} value);
+    {{/unless}}
     {{/if}}
     {{/chip_server_cluster_attributes}}
     {{#chip_server_cluster_attributes}}

src/app/zap-templates/zcl/data-model/chip/test-cluster.xml

@@ -124,9 +124,9 @@ limitations under the License.
     <!--<attribute side="server" code="0x0017" define="FLOAT_SINGLE" type="FLOAT_SINGLE" writable="true" default="0" optional="false">float_single</attribute>-->
     <!--<attribute side="server" code="0x0018" define="FLOAT_DOUBLE" type="FLOAT_DOUBLE" writable="true" default="0" optional="false">float_double</attribute>-->
     <attribute side="server" code="0x0019" define="OCTET_STRING" type="OCTET_STRING" length="10" writable="true" optional="false">octet_string</attribute>
-    <attribute side="server" code="0x001A" define="LIST" type="ARRAY" entryType="INT8U" length="10" writable="false" optional="false">list_int8u</attribute>
-    <attribute side="server" code="0x001B" define="LIST_OCTET_STRING" type="ARRAY" entryType="OCTET_STRING" length="254" writable="false" optional="false">list_octet_string</attribute>
-    <attribute side="server" code="0x001C" define="LIST_STRUCT_OCTET_STRING" type="ARRAY" entryType="TestListStructOctet" length="254" writable="false" optional="false">list_struct_octet_string</attribute>
+    <attribute side="server" code="0x001A" define="LIST" type="ARRAY" entryType="INT8U" length="10" writable="true" optional="false">list_int8u</attribute>
+    <attribute side="server" code="0x001B" define="LIST_OCTET_STRING" type="ARRAY" entryType="OCTET_STRING" length="254" writable="true" optional="false">list_octet_string</attribute>
+    <attribute side="server" code="0x001C" define="LIST_STRUCT_OCTET_STRING" type="ARRAY" entryType="TestListStructOctet" length="254" writable="true" optional="false">list_struct_octet_string</attribute>
     <!--<attribute side="server" code="0x001B" define="STRUCT" type="ARRAY" writable="true" optional="false">struct</attribute>-->
     <attribute side="server" code="0x001D" define="LONG_OCTET_STRING" type="LONG_OCTET_STRING" length="1000" writable="true" optional="false">long_octet_string</attribute>
     <attribute side="server" code="0x001E" define="CHAR_STRING" type="CHAR_STRING" length="10" writable="true" optional="false">char_string</attribute>

src/controller/java/templates/CHIPClusters-JNI.zapt

@@ -78,6 +78,8 @@ JNI_METHOD(void, {{asUpperCamelCase ../name}}Cluster, {{asLowerCamelCase name}})
 {{/chip_cluster_commands}}
 {{#chip_server_cluster_attributes}}
 {{#if isWritableAttribute}}
+{{! TODO: Lists not supported in attribute writes yet. }}
+{{#unless isList}}
 
 JNI_METHOD(void, {{asUpperCamelCase ../name}}Cluster, write{{asUpperCamelCase name}}Attribute)(JNIEnv * env, jobject self, jlong clusterPtr, jobject callback, {{asJniBasicType type false}} value)
 {
@@ -106,6 +108,7 @@ JNI_METHOD(void, {{asUpperCamelCase ../name}}Cluster, write{{asUpperCamelCase na
     onSuccess.release();
     onFailure.release();
 }
+{{/unless}}
 {{/if}}
 {{#if isReportableAttribute}}
 

src/controller/java/templates/ChipClusters-java.zapt

@@ -204,10 +204,13 @@ public class ChipClusters {
       read{{asUpperCamelCase name}}Attribute(chipClusterPtr, callback);
     }
   {{#if isWritableAttribute}}
+  {{! TODO: Lists not supported in attribute writes yet. }}
+  {{#unless isList}}
 
     public void write{{asUpperCamelCase name}}Attribute(DefaultClusterCallback callback, {{asJavaBasicType type}} value) {
       write{{asUpperCamelCase name}}Attribute(chipClusterPtr, callback, value);
     }
+  {{/unless}}
   {{/if}}
   {{#if isReportableAttribute}}
 
@@ -230,8 +233,11 @@ public class ChipClusters {
 {{/if}}
     );
   {{#if isWritableAttribute}}
+  {{! TODO: Lists not supported in attribute writes yet. }}
+  {{#unless isList}}
 
     private native void write{{asUpperCamelCase name}}Attribute(long chipClusterPtr, DefaultClusterCallback callback, {{asJavaBasicType type}} value);
+  {{/unless}}
   {{/if}}
   {{#if isReportableAttribute}}
 

src/controller/java/templates/ClusterInfo-write-interaction.zapt

@@ -17,6 +17,7 @@ public class ClusterWriteMapping {
       Map<String, InteractionInfo> write{{asUpperCamelCase name}}InteractionInfo = new LinkedHashMap<>();
         {{#chip_server_cluster_attributes}}
         {{#if isWritableAttribute}}
+        {{#unless isList}}
         Map<String, CommandParameterInfo> write{{asUpperCamelCase ../name}}{{asUpperCamelCase name}}CommandParams = new LinkedHashMap<String, CommandParameterInfo>();
         CommandParameterInfo {{asLowerCamelCase ../name}}{{asLowerCamelCase name}}CommandParameterInfo = new CommandParameterInfo("value", {{asJavaBasicType type}}.class);
         write{{asUpperCamelCase ../name}}{{asUpperCamelCase name}}CommandParams.put("value",{{asLowerCamelCase ../name}}{{asLowerCamelCase name}}CommandParameterInfo);
@@ -32,6 +33,7 @@ public class ClusterWriteMapping {
           write{{asUpperCamelCase ../name}}{{asUpperCamelCase name}}CommandParams
         );
         write{{asUpperCamelCase ../name}}InteractionInfo.put("write{{asUpperCamelCase name}}Attribute", write{{asUpperCamelCase ../name}}{{asUpperCamelCase name}}AttributeInteractionInfo);
+        {{/unless}}
         {{/if}}
         {{/chip_server_cluster_attributes}}
         writeAttributeMap.put("{{asLowerCamelCase name}}", write{{asUpperCamelCase name}}InteractionInfo);