Stop calling OnPairingComplete on CASE session establishment. (#18524)

* Stop calling OnPairingComplete on CASE session establishment.

OnPairingComplete is documented as indicating the end of PASE
establishment, but our implementation calls it for _some_ CASE
sessions as well: only the ones we kick off from UpdateDevice.

This is unexpected by API consumers, and we should not be doing it.

Fixes #18230

* Address review comments.

* Stop calling UpdateDevice from the Python controller.

Use GetConnectedDevice instead, so we don't have to rely on global callbacks.

src/controller/CHIPDeviceController.cpp

@@ -1528,32 +1528,25 @@ void DeviceCommissioner::OnDeviceConnectedFn(void * context, OperationalDevicePr
     DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
     VerifyOrReturn(commissioner != nullptr, ChipLogProgress(Controller, "Device connected callback with null context. Ignoring"));
 
-    if (commissioner->mCommissioningStage == CommissioningStage::kFindOperational)
+    if (commissioner->mCommissioningStage != CommissioningStage::kFindOperational)
     {
-        if (commissioner->mDeviceBeingCommissioned != nullptr &&
-            commissioner->mDeviceBeingCommissioned->GetDeviceId() == device->GetDeviceId() &&
-            commissioner->mCommissioningDelegate != nullptr)
-        {
-            CommissioningDelegate::CommissioningReport report;
-            report.Set<OperationalNodeFoundData>(OperationalNodeFoundData(device));
-            commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report);
-        }
+        // This call is definitely not us finding our commissionee device.
+        // This is presumably us trying to re-establish CASE on MRP failure.
+        return;
     }
-    else
+
+    if (commissioner->mDeviceBeingCommissioned == nullptr ||
+        commissioner->mDeviceBeingCommissioned->GetDeviceId() != device->GetDeviceId())
     {
-        if (commissioner->mPairingDelegate != nullptr)
-        {
-            commissioner->mPairingDelegate->OnPairingComplete(CHIP_NO_ERROR);
-        }
-        // Only release the PASE session if we're not commissioning. If we're commissioning, we're going to hold onto that PASE
-        // session until we send the commissioning complete command just in case it fails and we need to go back to the PASE
-        // connection to re-setup the network. This is unlikely, given that we just connected over the operational network, but is
-        // required by the spec.
-        CommissioneeDeviceProxy * commissionee = commissioner->FindCommissioneeDevice(device->GetDeviceId());
-        if (commissionee != nullptr)
-        {
-            commissioner->ReleaseCommissioneeDevice(commissionee);
-        }
+        // Not the device we are trying to commission.
+        return;
+    }
+
+    if (commissioner->mCommissioningDelegate != nullptr)
+    {
+        CommissioningDelegate::CommissioningReport report;
+        report.Set<OperationalNodeFoundData>(OperationalNodeFoundData(device));
+        commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report);
     }
 }
 
@@ -1565,8 +1558,6 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, PeerId peer
     ChipLogProgress(Controller, "Device connection failed. Error %s", ErrorStr(error));
     VerifyOrReturn(commissioner != nullptr,
                    ChipLogProgress(Controller, "Device connection failure callback with null context. Ignoring"));
-    VerifyOrReturn(commissioner->mPairingDelegate != nullptr,
-                   ChipLogProgress(Controller, "Device connection failure callback with null pairing delegate. Ignoring"));
 
     // Ensure that commissioning stage advancement is done based on seeing an error.
     if (error == CHIP_NO_ERROR)
@@ -1580,10 +1571,6 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, PeerId peer
     {
         commissioner->CommissioningStageComplete(error);
     }
-    else
-    {
-        commissioner->mPairingDelegate->OnPairingComplete(error);
-    }
     commissioner->mSystemState->CASESessionMgr()->ReleaseSession(peerId);
 }
 

src/controller/python/ChipDeviceController-ScriptBinding.cpp

@@ -152,7 +152,6 @@ ChipError::StorageType pychip_DeviceController_OpenCommissioningWindow(chip::Con
 void pychip_DeviceController_PrintDiscoveredDevices(chip::Controller::DeviceCommissioner * devCtrl);
 bool pychip_DeviceController_GetIPForDiscoveredDevice(chip::Controller::DeviceCommissioner * devCtrl, int idx, char * addrStr,
                                                       uint32_t len);
-ChipError::StorageType pychip_DeviceController_UpdateDevice(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid);
 
 // Pairing Delegate
 ChipError::StorageType
@@ -538,11 +537,6 @@ ChipError::StorageType pychip_ScriptDevicePairingDelegate_SetCommissioningStatus
     return CHIP_NO_ERROR.AsInteger();
 }
 
-ChipError::StorageType pychip_DeviceController_UpdateDevice(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid)
-{
-    return devCtrl->UpdateDevice(nodeid).AsInteger();
-}
-
 ChipError::StorageType pychip_Stack_Init()
 {
     CHIP_ERROR err = CHIP_NO_ERROR;

src/controller/python/chip/ChipDeviceCtrl.py

@@ -342,10 +342,11 @@ def SetThreadOperationalDataset(self, threadOperationalDataset):
     def ResolveNode(self, nodeid):
         self.CheckIsActive()
 
-        return self._ChipStack.CallAsync(
-            lambda: self._dmLib.pychip_DeviceController_UpdateDevice(
-                self.devCtrl, nodeid)
-        )
+        try:
+            self.GetConnectedDeviceSync(nodeid, allowPASE=False)
+            return 0
+        except ChipStackError as ex:
+            return ex.err
 
     def GetAddressAndPort(self, nodeid):
         self.CheckIsActive()
@@ -479,7 +480,7 @@ def GetClusterHandler(self):
 
         return self._Cluster
 
-    def GetConnectedDeviceSync(self, nodeid):
+    def GetConnectedDeviceSync(self, nodeid, allowPASE=True):
         self.CheckIsActive()
 
         returnDevice = c_void_p(None)
@@ -496,12 +497,12 @@ def DeviceAvailableCallback(device, err):
                 print("Failed in getting the connected device: {}".format(err))
                 raise self._ChipStack.ErrorToException(err)
 
-        res = self._ChipStack.Call(lambda: self._dmLib.pychip_GetDeviceBeingCommissioned(
-            self.devCtrl, nodeid, byref(returnDevice)))
-        if res == 0:
-            # TODO: give users more contrtol over whether they want to send this command over a PASE established connection
-            print('Using PASE connection')
-            return returnDevice
+        if allowPASE:
+            res = self._ChipStack.Call(lambda: self._dmLib.pychip_GetDeviceBeingCommissioned(
+                self.devCtrl, nodeid, byref(returnDevice)))
+            if res == 0:
+                print('Using PASE connection')
+                return returnDevice
 
         res = self._ChipStack.Call(lambda: self._dmLib.pychip_GetConnectedDeviceByNodeId(
             self.devCtrl, nodeid, DeviceAvailableCallback))
@@ -1019,10 +1020,6 @@ def _InitLib(self):
                 c_void_p, _DevicePairingDelegate_OnCommissioningStatusUpdateFunct]
             self._dmLib.pychip_ScriptDevicePairingDelegate_SetCommissioningCompleteCallback.restype = c_uint32
 
-            self._dmLib.pychip_DeviceController_UpdateDevice.argtypes = [
-                c_void_p, c_uint64]
-            self._dmLib.pychip_DeviceController_UpdateDevice.restype = c_uint32
-
             self._dmLib.pychip_GetConnectedDeviceByNodeId.argtypes = [
                 c_void_p, c_uint64, _DeviceAvailableFunct]
             self._dmLib.pychip_GetConnectedDeviceByNodeId.restype = c_uint32