Fix NetworkCommissioning post-review from #32156 (#32172)

* Fix NetworkCommissioning post-review from #32156

- Found a regression on Thread scanning.
- Changed some ConstraintError to InvalidCommand where
  more applicable.
- Removed an update of cluster state on fail safe expiry.

Testing done:

- Retested on Wi-Fi
- Testing on Thread as well

* Re-notify errors on empty network at fail-safe expiry

* Fix MobileDeviceTest

* Fix Cirque tests

Author: tcarmelveilleux

src/app/clusters/network-commissioning/network-commissioning.cpp

@@ -266,7 +266,9 @@ void Instance::HandleScanNetworks(HandlerContext & ctx, const Commands::ScanNetw
         }
         if (ssid.size() > DeviceLayer::Internal::kMaxWiFiSSIDLength)
         {
-            ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand);
+            // Clients should never use too large a SSID.
+            ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError);
+            SetLastNetworkingStatusValue(MakeNullable(Status::kUnknownError));
             return;
         }
         mCurrentOperationBreadcrumb = req.breadcrumb;
@@ -276,6 +278,13 @@ void Instance::HandleScanNetworks(HandlerContext & ctx, const Commands::ScanNetw
     }
     else if (mFeatureFlags.Has(NetworkCommissioningFeature::kThreadNetworkInterface))
     {
+        // SSID present on Thread violates the `[WI]` conformance.
+        if (req.ssid.HasValue())
+        {
+            ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand);
+            return;
+        }
+
         mCurrentOperationBreadcrumb = req.breadcrumb;
         mAsyncCommandHandle         = CommandHandler::Handle(&ctx.mCommandHandler);
         ctx.mCommandHandler.FlushAcksRightAwayOnSlowCommand();
@@ -322,6 +331,26 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands
 
     VerifyOrReturn(CheckFailSafeArmed(ctx));
 
+    if (req.ssid.empty() || req.ssid.size() > DeviceLayer::Internal::kMaxWiFiSSIDLength)
+    {
+        ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError, "ssid");
+        return;
+    }
+
+    // Presence of a Network Identity indicates we're configuring for Per-Device Credentials
+    if (req.networkIdentity.HasValue())
+    {
+#if CHIP_DEVICE_CONFIG_ENABLE_WIFI_PDC
+        if (mFeatureFlags.Has(Feature::kWiFiNetworkInterface))
+        {
+            HandleAddOrUpdateWiFiNetworkWithPDC(ctx, req);
+            return;
+        }
+#endif // CHIP_DEVICE_CONFIG_ENABLE_WIFI_PDC
+        ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand);
+        return;
+    }
+
     // Spec 11.8.8.4
     // Valid Credentials length are:
     // - 0 bytes: Unsecured (open) connection
@@ -671,6 +700,17 @@ void Instance::OnFailSafeTimerExpired()
     ChipLogDetail(Zcl, "Failsafe timeout, tell platform driver to revert network credentials.");
     mpWirelessDriver->RevertConfiguration();
     mAsyncCommandHandle.Release();
+
+    // Mark the network list changed since `mpWirelessDriver->RevertConfiguration()` may have updated it.
+    ReportNetworksListChanged();
+
+    // If no networks are left, clear-out errors;
+    if (mpBaseDriver && (CountAndRelease(mpBaseDriver->GetNetworks()) == 0))
+    {
+        SetLastNetworkId(ByteSpan{});
+        SetLastConnectErrorValue(NullNullable);
+        SetLastNetworkingStatusValue(NullNullable);
+    }
 }
 
 CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context)

src/controller/python/test/test_scripts/network_commissioning.py

@@ -140,10 +140,16 @@ async def test_wifi(self, endpointId):
                 f"LastNetworkID, LastNetworkingStatus and LastConnectErrorValue should be Null")
 
         # Scan networks
-        logger.info(f"Scan networks")
-        req = Clusters.NetworkCommissioning.Commands.ScanNetworks(
-            ssid=b'', breadcrumb=self.with_breadcrumb())
-        res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req)
+        logger.info("Scan networks")
+        req = Clusters.NetworkCommissioning.Commands.ScanNetworks(breadcrumb=self.with_breadcrumb())
+        interactionTimeoutMs = self._devCtrl.ComputeRoundTripTimeout(self._nodeid, upperLayerProcessingTimeoutMs=30000)
+        logger.info(f"Request: {req}")
+        res = await self._devCtrl.SendCommand(
+            nodeid=self._nodeid,
+            endpoint=endpointId,
+            payload=req,
+            interactionTimeoutMs=interactionTimeoutMs
+        )
         logger.info(f"Received response: {res}")
         if res.networkingStatus != Clusters.NetworkCommissioning.Enums.NetworkCommissioningStatus.kSuccess:
             raise AssertionError(f"Unexpected result: {res.networkingStatus}")
@@ -274,10 +280,14 @@ async def test_thread(self, endpointId):
                 f"LastNetworkID, LastNetworkingStatus and LastConnectErrorValue should be Null")
 
         # Scan networks
-        logger.info(f"Scan networks")
-        req = Clusters.NetworkCommissioning.Commands.ScanNetworks(
-            ssid=b'', breadcrumb=self.with_breadcrumb())
-        res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req)
+        logger.info("Scan networks")
+        req = Clusters.NetworkCommissioning.Commands.ScanNetworks(breadcrumb=self.with_breadcrumb())
+        logger.info(f"Request: {req}")
+        interactionTimeoutMs = self._devCtrl.ComputeRoundTripTimeout(self._nodeid, upperLayerProcessingTimeoutMs=30000)
+        res = await self._devCtrl.SendCommand(nodeid=self._nodeid,
+                                              endpoint=endpointId,
+                                              payload=req,
+                                              interactionTimeoutMs=interactionTimeoutMs)
         logger.info(f"Received response: {res}")
         if res.networkingStatus != Clusters.NetworkCommissioning.Enums.NetworkCommissioningStatus.kSuccess:
             raise AssertionError(f"Unexpected result: {res.networkingStatus}")