From c21ee451c65989cb2784817ed4ecf20e00213f4a Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Fri, 23 Aug 2024 13:16:06 -0400
Subject: [PATCH] Address followup issues for preset/atomic-write
 implementation. (#35175)

* Puts some file-local functions in an anonymous namespace.
* Fixes the "is this attribute supported?" check to correctly check for global
  attributes that are not in Ember metadata.
* Moves the comment explaining why it's OK to skip the spec-required ACL check
  to the place where that check is being skipped.
* Removes a non-spec-compliant "error if the timeout is 0" bit.

Fixes https://github.com/project-chip/connectedhomeip/issues/35168
---
 src/app/GlobalAttributes.h                    | 13 ++++++++
 .../thermostat-server-atomic.cpp              | 31 ++++++++++++-------
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/src/app/GlobalAttributes.h b/src/app/GlobalAttributes.h
index 5096792309a880..461294267f38c0 100644
--- a/src/app/GlobalAttributes.h
+++ b/src/app/GlobalAttributes.h
@@ -40,5 +40,18 @@ constexpr AttributeId GlobalAttributesNotInMetadata[] = {
 
 static_assert(ArrayIsSorted(GlobalAttributesNotInMetadata), "Array of global attribute ids must be sorted");
 
+inline bool IsSupportedGlobalAttributeNotInMetadata(AttributeId attributeId)
+{
+    for (auto & attr : GlobalAttributesNotInMetadata)
+    {
+        if (attr == attributeId)
+        {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 } // namespace app
 } // namespace chip
diff --git a/src/app/clusters/thermostat-server/thermostat-server-atomic.cpp b/src/app/clusters/thermostat-server/thermostat-server-atomic.cpp
index 2a6e52e504887e..19a6ffa3387e12 100644
--- a/src/app/clusters/thermostat-server/thermostat-server-atomic.cpp
+++ b/src/app/clusters/thermostat-server/thermostat-server-atomic.cpp
@@ -17,6 +17,7 @@
 
 #include "thermostat-server.h"
 
+#include <app/GlobalAttributes.h>
 #include <platform/internal/CHIPDeviceLayerInternal.h>
 
 using namespace chip;
@@ -47,6 +48,8 @@ void TimerExpiredCallback(System::Layer * systemLayer, void * callbackContext)
     gThermostatAttrAccess.ResetAtomicWrite(endpoint);
 }
 
+namespace {
+
 /**
  * @brief Schedules a timer for the given timeout in milliseconds.
  *
@@ -185,15 +188,25 @@ Status BuildAttributeStatuses(const EndpointId endpoint, const DataModel::Decoda
         const EmberAfAttributeMetadata * metadata =
             emberAfLocateAttributeMetadata(endpoint, Thermostat::Id, attributeStatus.attributeID);
 
-        if (metadata == nullptr)
+        if (metadata != nullptr)
+        {
+            // This is definitely an attribute we know about.
+            continue;
+        }
+
+        if (IsSupportedGlobalAttributeNotInMetadata(attributeStatus.attributeID))
         {
-            // This is not a valid attribute on the Thermostat cluster on the supplied endpoint
-            return Status::InvalidCommand;
+            continue;
         }
+
+        // This is not a valid attribute on the Thermostat cluster on the supplied endpoint
+        return Status::InvalidCommand;
     }
     return Status::Success;
 }
 
+} // anonymous namespace
+
 bool ThermostatAttrAccess::InAtomicWrite(EndpointId endpoint, Optional<AttributeId> attributeId)
 {
 
@@ -422,6 +435,10 @@ void ThermostatAttrAccess::BeginAtomicWrite(CommandHandler * commandObj, const C
     status = Status::Success;
     for (size_t i = 0; i < attributeStatuses.AllocatedSize(); ++i)
     {
+        // If we've gotten this far, then the client has manage permission to call AtomicRequest,
+        // which is also the privilege necessary to write to the atomic attributes, so no need to do
+        // the "If the client does not have sufficient privilege to write to the attribute" check
+        // from the spec.
         auto & attributeStatus = attributeStatuses[i];
         auto statusCode        = Status::Success;
         switch (attributeStatus.attributeID)
@@ -442,11 +459,6 @@ void ThermostatAttrAccess::BeginAtomicWrite(CommandHandler * commandObj, const C
     }
 
     auto timeout = std::min(System::Clock::Milliseconds16(commandData.timeout.Value()), maximumTimeout);
-    if (timeout.count() == 0)
-    {
-        commandObj->AddStatus(commandPath, Status::InvalidInState);
-        return;
-    }
 
     if (status == Status::Success)
     {
@@ -605,9 +617,6 @@ bool emberAfThermostatClusterAtomicRequestCallback(CommandHandler * commandObj,
 {
     auto & requestType = commandData.requestType;
 
-    // If we've gotten this far, then the client has manage permission to call AtomicRequest, which is also the
-    // privilege necessary to write to the atomic attributes, so no need to check
-
     switch (requestType)
     {
     case Globals::AtomicRequestTypeEnum::kBeginWrite: