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.

examples/all-clusters-app/all-clusters-common/all-clusters-app.matter

@@ -5512,7 +5512,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;

examples/all-clusters-app/all-clusters-common/all-clusters-app.zap

@@ -4878,7 +4878,7 @@
               "storageOption": "RAM",
               "singleton": 0,
               "bounded": 0,
-              "defaultValue": "false",
+              "defaultValue": "true",
               "reportable": 1,
               "minInterval": 1,
               "maxInterval": 65534,
@@ -21789,5 +21789,6 @@
       "endpointId": 65534,
       "networkId": 0
     }
-  ]
-}
+  ],
+  "log": []
+}

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)

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 = dnsResolve ? (isIPv6 || isDomain) : isIPv6;
+        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;
         }
     }
 

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,12 +198,14 @@ 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);
                 // This buffer is statically allocated and if of size kMaxSupportedTimeZones, so this should never fail
-                CopyCharSpanToMutableCharSpan(mTimeZoneBuf[i].name.Value(), span);
+                CopyCharSpanToMutableCharSpan(params.GetTimeZone().Value()[i].name.Value(), span);
                 mTimeZoneBuf[i].name.SetValue(span);
+            } else {
+                mTimeZoneBuf[i].name = NullOptional;
             }
         }
         auto list = app::DataModel::List<app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type>(mTimeZoneBuf, size);
@@ -208,15 +214,12 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
     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()) {
-            size_t size = std::min(params.GetDefaultNTP().Value().Value().size(), kMaxDefaultNtpSize);
+        if (!params.GetDefaultNTP().Value().IsNull() && params.GetDefaultNTP().Value().Value().size() <= kMaxDefaultNtpSize) {
             // This buffer 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, size));
+            auto default_ntp = MakeNullable(CharSpan(mDefaultNtp, params.GetDefaultNTP().Value().Value().size()));
             mParams.SetDefaultNTP(default_ntp);
-        } else {
-            mParams.SetDefaultNTP(NullNullable);
         }
     }
 

src/controller/CommissioningDelegate.h

@@ -513,6 +513,7 @@ class CommissioningParameters
         mDAC.ClearValue();
         mTimeZone.ClearValue();
         mDSTOffsets.ClearValue();
+        mDefaultNTP.ClearValue();
     }
 
 private:

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,19 @@ 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);

src/controller/python/chip/ChipDeviceCtrl.py

@@ -1341,10 +1341,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
@@ -1654,10 +1654,10 @@ 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 = ""):
         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):

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

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