AutoCommissioner: Set default NTP to internal buffer (project-chip#29954

) * AutoCommissioner: Set default NTP to internal buffer Also, it would be good to actually set the time zone to our own buffer. * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Size check and fail big strings, add tests, fix bug - neither DefaultNTP nor the time zone name is useful to the device if they're truncated, so don't attempt to send anything unless the value is spec compliant - NOTE: Right now, we still attempt to commission with the remaining parameters, but I'm open to returning an error here too - add tests for too-long names AND max size names - fix a bug in the server default NTP handling that prevented domain names from ever being set - turn on dns resolution in the all-clusters. Note that the all clusters app will never actually DO anything with these names, but it's still nice to have it available for testing. * Restyled by clang-format * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Address review comments * Restyled by clang-format --------- Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Restyled.io <[email protected]>
plauric · Nov 21, 2023 · 5774359 · 5774359
1 parent c19f26f
commit 5774359
Show file tree

Hide file tree

Showing 11 changed files with 119 additions and 29 deletions.
diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter
@@ -5525,7 +5525,7 @@ endpoint 0 {
     ram      attribute timeZoneDatabase default = 0;
     callback attribute timeZoneListMaxSize default = 3;
     callback attribute DSTOffsetListMaxSize;
-    ram      attribute supportsDNSResolve default = false;
+    ram      attribute supportsDNSResolve default = true;
     callback attribute generatedCommandList;
     callback attribute acceptedCommandList;
     callback attribute attributeList;

diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap b/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap
@@ -4926,7 +4926,7 @@
               "storageOption": "RAM",
               "singleton": 0,
               "bounded": 0,
-              "defaultValue": "false",
+              "defaultValue": "true",
               "reportable": 1,
               "minInterval": 1,
               "maxInterval": 65534,
@@ -21585,6 +21585,7 @@
       "endpointId": 65534,
       "networkId": 0
     }
+
   ]
 }
 

diff --git a/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp b/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp
@@ -44,8 +44,10 @@ bool DefaultTimeSyncDelegate::IsNTPAddressValid(chip::CharSpan ntp)
 
 bool DefaultTimeSyncDelegate::IsNTPAddressDomain(chip::CharSpan ntp)
 {
-    // placeholder implementation
-    return false;
+    // For now, assume anything that includes a . is a domain name.
+    // Delegates are free to evaluate this properly if they actually HAVE domain
+    // name resolution, rather than just implementing a dummy for testing.
+    return !IsNTPAddressValid(ntp) && (memchr(ntp.data(), '.', ntp.size()) != nullptr);
 }
 
 CHIP_ERROR DefaultTimeSyncDelegate::UpdateTimeFromPlatformSource(chip::Callback::Callback<OnTimeSyncCompletion> * callback)

diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp
@@ -1284,24 +1284,19 @@ bool emberAfTimeSynchronizationClusterSetDefaultNTPCallback(
             commandObj->AddStatus(commandPath, Status::ConstraintError);
             return true;
         }
-        if (!GetDelegate()->IsNTPAddressValid(dNtpChar.Value()))
+        bool dnsResolve;
+        if (EMBER_ZCL_STATUS_SUCCESS != SupportsDNSResolve::Get(commandPath.mEndpointId, &dnsResolve))
         {
-            commandObj->AddStatus(commandPath, Status::InvalidCommand);
+            commandObj->AddStatus(commandPath, Status::Failure);
             return true;
         }
-        if (GetDelegate()->IsNTPAddressDomain(dNtpChar.Value()))
+        bool isDomain = GetDelegate()->IsNTPAddressDomain(dNtpChar.Value());
+        bool isIPv6   = GetDelegate()->IsNTPAddressValid(dNtpChar.Value());
+        bool useable  = isIPv6 || (isDomain && dnsResolve);
+        if (!useable)
         {
-            bool dnsResolve;
-            if (EMBER_ZCL_STATUS_SUCCESS != SupportsDNSResolve::Get(commandPath.mEndpointId, &dnsResolve))
-            {
-                commandObj->AddStatus(commandPath, Status::Failure);
-                return true;
-            }
-            if (!dnsResolve)
-            {
-                commandObj->AddStatus(commandPath, Status::InvalidCommand);
-                return true;
-            }
+            commandObj->AddStatus(commandPath, Status::InvalidCommand);
+            return true;
         }
     }
 

diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp
@@ -27,6 +27,8 @@ namespace chip {
 namespace Controller {
 
 using namespace chip::app::Clusters;
+using chip::app::DataModel::MakeNullable;
+using chip::app::DataModel::NullNullable;
 
 AutoCommissioner::AutoCommissioner()
 {
@@ -87,7 +89,9 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
          IsUnsafeSpan(params.GetAttestationSignature(), mParams.GetAttestationSignature()) ||
          IsUnsafeSpan(params.GetPAI(), mParams.GetPAI()) || IsUnsafeSpan(params.GetDAC(), mParams.GetDAC()) ||
          IsUnsafeSpan(params.GetTimeZone(), mParams.GetTimeZone()) ||
-         IsUnsafeSpan(params.GetDSTOffsets(), mParams.GetDSTOffsets()));
+         IsUnsafeSpan(params.GetDSTOffsets(), mParams.GetDSTOffsets()) ||
+         (params.GetDefaultNTP().HasValue() && !params.GetDefaultNTP().Value().IsNull() &&
+          params.GetDefaultNTP().Value().Value().data() != mDefaultNtp));
 
     mParams = params;
 
@@ -194,16 +198,36 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
         for (size_t i = 0; i < size; ++i)
         {
             mTimeZoneBuf[i] = params.GetTimeZone().Value()[i];
-            if (mTimeZoneBuf[i].name.HasValue())
+            if (params.GetTimeZone().Value()[i].name.HasValue() &&
+                params.GetTimeZone().Value()[i].name.Value().size() <= kMaxTimeZoneNameLen)
             {
                 auto span = MutableCharSpan(mTimeZoneNames[i], kMaxTimeZoneNameLen);
-                CopyCharSpanToMutableCharSpan(mTimeZoneBuf[i].name.Value(), span);
+                // The buffer backing "span" is statically allocated and is of size kMaxSupportedTimeZones, so this should never
+                // fail.
+                CopyCharSpanToMutableCharSpan(params.GetTimeZone().Value()[i].name.Value(), span);
                 mTimeZoneBuf[i].name.SetValue(span);
             }
+            else
+            {
+                mTimeZoneBuf[i].name.ClearValue();
+            }
         }
         auto list = app::DataModel::List<app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type>(mTimeZoneBuf, size);
         mParams.SetTimeZone(list);
     }
+    if (params.GetDefaultNTP().HasValue())
+    {
+        ChipLogProgress(Controller, "Setting Default NTP from parameters");
+        // This parameter is an optional nullable, so we need to go two levels deep here.
+        if (!params.GetDefaultNTP().Value().IsNull() && params.GetDefaultNTP().Value().Value().size() <= kMaxDefaultNtpSize)
+        {
+            // The buffer backing "span" is statically allocated and is of size kMaxDefaultNtpSize.
+            auto span = MutableCharSpan(mDefaultNtp, kMaxDefaultNtpSize);
+            CopyCharSpanToMutableCharSpan(params.GetDefaultNTP().Value().Value(), span);
+            auto default_ntp = MakeNullable(CharSpan(mDefaultNtp, params.GetDefaultNTP().Value().Value().size()));
+            mParams.SetDefaultNTP(default_ntp);
+        }
+    }
 
     return CHIP_NO_ERROR;
 }

diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h
@@ -113,6 +113,9 @@ class AutoCommissioner : public CommissioningDelegate
     static constexpr size_t kMaxSupportedDstStructs = 10;
     app::Clusters::TimeSynchronization::Structs::DSTOffsetStruct::Type mDstOffsetsBuf[kMaxSupportedDstStructs];
 
+    static constexpr size_t kMaxDefaultNtpSize = 128;
+    char mDefaultNtp[kMaxDefaultNtpSize];
+
     bool mNeedsNetworkSetup = false;
     ReadCommissioningInfo mDeviceCommissioningInfo;
     bool mNeedsDST = false;

diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h
@@ -527,6 +527,7 @@ class CommissioningParameters
         mDAC.ClearValue();
         mTimeZone.ClearValue();
         mDSTOffsets.ClearValue();
+        mDefaultNTP.ClearValue();
     }
 
 private:

diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp
@@ -100,6 +100,7 @@ chip::Platform::ScopedMemoryBuffer<uint8_t> sThreadBuf;
 chip::Platform::ScopedMemoryBuffer<char> sDefaultNTPBuf;
 app::Clusters::TimeSynchronization::Structs::DSTOffsetStruct::Type sDSTBuf;
 app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type sTimeZoneBuf;
+chip::Platform::ScopedMemoryBuffer<char> sTimeZoneNameBuf;
 chip::Controller::CommissioningParameters sCommissioningParameters;
 
 } // namespace
@@ -140,7 +141,7 @@ PyChipError pychip_DeviceController_UnpairDevice(chip::Controller::DeviceCommiss
                                                  DeviceUnpairingCompleteFunct callback);
 PyChipError pychip_DeviceController_SetThreadOperationalDataset(const char * threadOperationalDataset, uint32_t size);
 PyChipError pychip_DeviceController_SetWiFiCredentials(const char * ssid, const char * credentials);
-PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt);
+PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt, const char * name);
 PyChipError pychip_DeviceController_SetDSTOffset(int32_t offset, uint64_t validStarting, uint64_t validUntil);
 PyChipError pychip_DeviceController_SetDefaultNtp(const char * defaultNTP);
 PyChipError pychip_DeviceController_SetTrustedTimeSource(chip::NodeId nodeId, chip::EndpointId endpoint);
@@ -509,10 +510,22 @@ PyChipError pychip_DeviceController_SetWiFiCredentials(const char * ssid, const
     return ToPyChipError(CHIP_NO_ERROR);
 }
 
-PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt)
+PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt, const char * name)
 {
     sTimeZoneBuf.offset  = offset;
     sTimeZoneBuf.validAt = validAt;
+    if (strcmp(name, "") == 0)
+    {
+        sTimeZoneNameBuf.Free();
+        sTimeZoneBuf.name = NullOptional;
+    }
+    else
+    {
+        size_t len = strlen(name);
+        ReturnErrorCodeIf(!sTimeZoneNameBuf.Alloc(len), ToPyChipError(CHIP_ERROR_NO_MEMORY));
+        memcpy(sTimeZoneNameBuf.Get(), name, len);
+        sTimeZoneBuf.name.SetValue(CharSpan(sTimeZoneNameBuf.Get(), len));
+    }
     app::DataModel::List<app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type> list(&sTimeZoneBuf, 1);
     sCommissioningParameters.SetTimeZone(list);
     return ToPyChipError(CHIP_NO_ERROR);

diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py
@@ -1421,10 +1421,10 @@ def _InitLib(self):
                 c_char_p, c_char_p]
             self._dmLib.pychip_DeviceController_SetWiFiCredentials.restype = PyChipError
 
-            # Currently only supports 1 list item, no name
+            # Currently only supports 1 list item
             self._dmLib.pychip_DeviceController_SetTimeZone.restype = PyChipError
             self._dmLib.pychip_DeviceController_SetTimeZone.argtypes = [
-                c_int32, c_uint64]
+                c_int32, c_uint64, c_char_p]
 
             # Currently only supports 1 list item
             self._dmLib.pychip_DeviceController_SetDSTOffset.restype = PyChipError
@@ -1737,11 +1737,11 @@ def ResetCommissioningParameters(self):
             lambda: self._dmLib.pychip_DeviceController_ResetCommissioningParameters()
         ).raise_on_error()
 
-    def SetTimeZone(self, offset: int, validAt: int):
+    def SetTimeZone(self, offset: int, validAt: int, name: str = ""):
         ''' Set the time zone to set during commissioning. Currently only one time zone entry is supported'''
         self.CheckIsActive()
         self._ChipStack.Call(
-            lambda: self._dmLib.pychip_DeviceController_SetTimeZone(offset, validAt)
+            lambda: self._dmLib.pychip_DeviceController_SetTimeZone(offset, validAt, name.encode("utf-8"))
         ).raise_on_error()
 
     def SetDSTOffset(self, offset: int, validStarting: int, validUntil: int):

diff --git a/src/python_testing/TC_TIMESYNC_2_6.py b/src/python_testing/TC_TIMESYNC_2_6.py
@@ -35,7 +35,7 @@ async def send_set_default_ntp_cmd(self, ntp: typing.Union[Nullable, str]) -> No
     async def send_set_default_ntp_cmd_expect_error(self, ntp: typing.Union[Nullable, str], error: Status) -> None:
         try:
             await self.send_single_cmd(cmd=Clusters.Objects.TimeSynchronization.Commands.SetDefaultNTP(defaultNTP=ntp))
-            asserts.assert_true(False, "Unexpected SetTimeZone command success")
+            asserts.assert_true(False, "Unexpected SetDefaultNTP command success")
         except InteractionModelError as e:
             asserts.assert_equal(e.status, error, "Unexpected error returned")
             pass

diff --git a/src/python_testing/TestCommissioningTimeSync.py b/src/python_testing/TestCommissioningTimeSync.py
@@ -89,7 +89,7 @@ async def commission_and_base_checks(self):
 
     async def create_commissioner(self):
         if self.commissioner:
-            self.destroy_current_commissioner()
+            await self.destroy_current_commissioner()
         new_certificate_authority = self.certificate_authority_manager.NewCertificateAuthority()
         new_fabric_admin = new_certificate_authority.NewFabricAdmin(vendorId=0xFFF1, fabricId=2)
         self.commissioner = new_fabric_admin.NewController(nodeId=112233, useTestCommissioner=True)
@@ -230,6 +230,57 @@ async def test_FabricCheckStage(self):
             kCheckForMatchingFabric), "Incorrectly ran check for matching fabric stage")
         asserts.assert_equal(self.commissioner.GetFabricCheckResult(), -1, "Fabric check result incorrectly set")
 
+    @async_test_body
+    async def test_TimeZoneName(self):
+        await self.create_commissioner()
+        self.commissioner.SetTimeZone(offset=3600, validAt=0, name="test")
+        await self.commission_and_base_checks()
+        asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureTimeZone), 'Time zone was not successfully set')
+
+        received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.TimeZone)
+        expected = [Clusters.TimeSynchronization.Structs.TimeZoneStruct(offset=3600, validAt=0, name="test")]
+        asserts.assert_equal(received, expected, "Time zone was not correctly set by commissioner")
+
+        await self.create_commissioner()
+        # name is max 64 per the spec
+        sixty_five_byte_string = "x" * 65
+        self.commissioner.SetTimeZone(offset=3600, validAt=0, name=sixty_five_byte_string)
+        await self.commission_and_base_checks()
+        asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureTimeZone), 'Time zone was not successfully set')
+
+        received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.TimeZone)
+        expected = [Clusters.TimeSynchronization.Structs.TimeZoneStruct(offset=3600, validAt=0, name=None)]
+        asserts.assert_equal(received, expected, "Commissioner did not ignore too-long name")
+
+        await self.create_commissioner()
+        # name is max 64 per the spec
+        sixty_four_byte_string = "x" * 64
+        self.commissioner.SetTimeZone(offset=3600, validAt=0, name=sixty_four_byte_string)
+        await self.commission_and_base_checks()
+        asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureTimeZone), 'Time zone was not successfully set')
+
+        received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.TimeZone)
+        expected = [Clusters.TimeSynchronization.Structs.TimeZoneStruct(offset=3600, validAt=0, name=sixty_four_byte_string)]
+        asserts.assert_equal(received, expected, "Time zone 64 byte name was not correctly set")
+
+    @async_test_body
+    async def test_DefaultNtpSize(self):
+        await self.create_commissioner()
+        too_long_name = "x." + "x" * 127
+        self.commissioner.SetDefaultNTP(too_long_name)
+        await self.commission_and_base_checks()
+        asserts.assert_false(self.commissioner.CheckStageSuccessful(kConfigureDefaultNTP),
+                             'Commissioner attempted to set default NTP to a too long value')
+
+        await self.create_commissioner()
+        just_fits_name = "x." + "x" * 126
+        self.commissioner.SetDefaultNTP(just_fits_name)
+        await self.commission_and_base_checks()
+        asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureDefaultNTP),
+                            'Commissioner did not correctly set default NTP')
+        received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.DefaultNTP)
+        asserts.assert_equal(received, just_fits_name, 'Commissioner incorrectly set default NTP name')
+
 
 # TODO(cecille): Test - Add hooks to change the time zone response to indicate no DST is needed
 # TODO(cecille): Test - Set commissioningParameters TimeZone and DST list size to > node list size to ensure they get truncated