Don't remove PASE connection until after commissioning complete

src/app/clusters/general-commissioning-server/general-commissioning-server.cpp

@@ -97,6 +97,9 @@ CHIP_ERROR GeneralCommissioningAttrAccess::Read(const ConcreteReadAttributePath
     case SupportsConcurrentConnection::Id: {
         return ReadSupportsConcurrentConnection(aEncoder);
     }
+    case Breadcrumb::Id: {
+        return aEncoder.Encode(DeviceLayer::DeviceControlServer::DeviceControlSvr().GetBreadcrumb());
+    }
     default: {
         break;
     }
@@ -178,6 +181,7 @@ bool emberAfGeneralCommissioningClusterArmFailSafeCallback(app::CommandHandler *
         }
         response.errorCode = CommissioningError::kOk;
         commandObj->AddResponse(commandPath, response);
+        DeviceLayer::DeviceControlServer::DeviceControlSvr().SetBreadcrumb(commandData.breadcrumb);
     }
     else
     {
@@ -252,5 +256,6 @@ bool emberAfGeneralCommissioningClusterSetRegulatoryConfigCallback(app::CommandH
 
 void MatterGeneralCommissioningPluginServerInitCallback()
 {
+    DeviceLayer::DeviceControlServer::DeviceControlSvr().SetBreadcrumb(0);
     registerAttributeAccessOverride(&gAttrAccess);
 }

src/app/server/Server.cpp

@@ -251,11 +251,7 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
     err = mCASESessionManager.Init(&DeviceLayer::SystemLayer(), caseSessionManagerConfig);
     SuccessOrExit(err);
 
-    err = mCASEServer.ListenForSessionEstablishment(&mExchangeMgr, &mTransports,
-#if CONFIG_NETWORK_LAYER_BLE
-                                                    chip::DeviceLayer::ConnectivityMgr().GetBleLayer(),
-#endif
-                                                    &mSessions, &mFabrics, mGroupsProvider);
+    err = mCASEServer.ListenForSessionEstablishment(&mExchangeMgr, &mTransports, &mSessions, &mFabrics, mGroupsProvider);
     SuccessOrExit(err);
 
     // This code is necessary to restart listening to existing groups after a reboot

src/controller/CHIPDeviceController.cpp

@@ -556,6 +556,16 @@ CommissioneeDeviceProxy * DeviceCommissioner::FindCommissioneeDevice(const Trans
 
 void DeviceCommissioner::ReleaseCommissioneeDevice(CommissioneeDeviceProxy * device)
 {
+    // TODO: Call CloseSession here see #16440 and #16805 (blocking)
+
+#if CONFIG_NETWORK_LAYER_BLE
+    if (mSystemState->BleLayer() != nullptr && device->GetDeviceTransportType() == Transport::Type::kBle)
+    {
+        // We only support one BLE connection, so if this is BLE, close it
+        ChipLogProgress(Discovery, "Closing all BLE connections");
+        mSystemState->BleLayer()->CloseAllBleConnections();
+    }
+#endif
     mCommissioneeDevicePool.ReleaseObject(device);
     // Make sure that there will be no dangling pointer
     if (mDeviceInPASEEstablishment == device)
@@ -1360,10 +1370,20 @@ void OnBasicFailure(void * context, CHIP_ERROR error)
 void DeviceCommissioner::CleanupCommissioning(DeviceProxy * proxy, NodeId nodeId, const CompletionStatus & completionStatus)
 {
     commissioningCompletionStatus = completionStatus;
-    if (completionStatus.err == CHIP_NO_ERROR ||
-        (completionStatus.failedStage.HasValue() && completionStatus.failedStage.Value() >= kWiFiNetworkSetup))
+    if (completionStatus.err == CHIP_NO_ERROR)
+    {
+
+        CommissioneeDeviceProxy * commissionee = FindCommissioneeDevice(nodeId);
+        if (commissionee != nullptr)
+        {
+            ReleaseCommissioneeDevice(commissionee);
+        }
+        // Send the callbacks, we're done.
+        CommissioningStageComplete(CHIP_NO_ERROR);
+        SendCommissioningCompleteCallbacks(nodeId, commissioningCompletionStatus);
+    }
+    else if (completionStatus.failedStage.HasValue() && completionStatus.failedStage.Value() >= kWiFiNetworkSetup)
     {
-        // If we Completed successfully, send the callbacks, we're done.
         // If we were already doing network setup, we need to retain the pase session and start again from network setup stage.
         // We do not need to reset the failsafe here because we want to keep everything on the device up to this point, so just
         // send the completion callbacks.
@@ -1462,6 +1482,7 @@ void DeviceCommissioner::CommissioningStageComplete(CHIP_ERROR err, Commissionin
         completionStatus.err         = status;
         completionStatus.failedStage = MakeOptional(report.stageCompleted);
         mCommissioningStage          = CommissioningStage::kCleanup;
+        mDeviceBeingCommissioned     = proxy;
         CleanupCommissioning(proxy, nodeId, completionStatus);
     }
 }
@@ -1483,16 +1504,21 @@ void DeviceCommissioner::OnDeviceConnectedFn(void * context, OperationalDevicePr
             commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report);
         }
     }
-    else if (commissioner->mPairingDelegate != nullptr)
-    {
-        commissioner->mPairingDelegate->OnPairingComplete(CHIP_NO_ERROR);
-    }
-
-    // Release any CommissioneeDeviceProxies we have here as we now have an OperationalDeviceProxy.
-    CommissioneeDeviceProxy * commissionee = commissioner->FindCommissioneeDevice(device->GetDeviceId());
-    if (commissionee != nullptr)
+    else
     {
-        commissioner->ReleaseCommissioneeDevice(commissionee);
+        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);
+        }
     }
 }
 
@@ -1513,14 +1539,7 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, PeerId peer
         ChipLogError(Controller, "Device connection failed without a valid error code. Making one up.");
         error = CHIP_ERROR_INTERNAL;
     }
-    // TODO: Determine if we really want the PASE session removed here. See #16089.
-    CommissioneeDeviceProxy * commissionee = commissioner->FindCommissioneeDevice(peerId.GetNodeId());
-    if (commissionee != nullptr)
-    {
-        commissioner->ReleaseCommissioneeDevice(commissionee);
-    }
 
-    commissioner->mSystemState->CASESessionMgr()->ReleaseSession(peerId);
     if (commissioner->mCommissioningStage == CommissioningStage::kFindOperational &&
         commissioner->mCommissioningDelegate != nullptr)
     {
@@ -1530,6 +1549,7 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, PeerId peer
     {
         commissioner->mPairingDelegate->OnPairingComplete(error);
     }
+    commissioner->mSystemState->CASESessionMgr()->ReleaseSession(peerId);
 }
 
 // AttributeCache::Callback impl

src/controller/CHIPDeviceControllerFactory.cpp

@@ -182,11 +182,8 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
         // especially since it will interrupt other potential usages of BLE by the controller acting in a commissioning capacity.
         //
         ReturnErrorOnFailure(stateParams.caseServer->ListenForSessionEstablishment(
-            stateParams.exchangeMgr, stateParams.transportMgr,
-#if CONFIG_NETWORK_LAYER_BLE
-            nullptr,
-#endif
-            stateParams.sessionMgr, stateParams.fabricTable, stateParams.groupDataProvider));
+            stateParams.exchangeMgr, stateParams.transportMgr, stateParams.sessionMgr, stateParams.fabricTable,
+            stateParams.groupDataProvider));
 
         //
         // We need to advertise the port that we're listening to for unsolicited messages over UDP. However, we have both a IPv4

src/controller/CommissioneeDeviceProxy.cpp

@@ -155,10 +155,7 @@ void CommissioneeDeviceProxy::Reset()
     mState              = ConnectionState::NotConnected;
     mSessionManager     = nullptr;
     mUDPEndPointManager = nullptr;
-#if CONFIG_NETWORK_LAYER_BLE
-    mBleLayer = nullptr;
-#endif
-    mExchangeMgr = nullptr;
+    mExchangeMgr        = nullptr;
 }
 
 CHIP_ERROR CommissioneeDeviceProxy::LoadSecureSessionParameters()

src/controller/CommissioneeDeviceProxy.h

@@ -45,11 +45,6 @@
 #include <transport/raw/MessageHeader.h>
 #include <transport/raw/UDP.h>
 
-#if CONFIG_NETWORK_LAYER_BLE
-#include <ble/BleLayer.h>
-#include <transport/raw/BLE.h>
-#endif
-
 namespace chip {
 
 constexpr size_t kAttestationNonceLength = 32;
@@ -68,10 +63,7 @@ struct ControllerDeviceInitParams
     Messaging::ExchangeManager * exchangeMgr                      = nullptr;
     Inet::EndPointManager<Inet::UDPEndPoint> * udpEndPointManager = nullptr;
     PersistentStorageDelegate * storageDelegate                   = nullptr;
-#if CONFIG_NETWORK_LAYER_BLE
-    Ble::BleLayer * bleLayer = nullptr;
-#endif
-    FabricTable * fabricsTable = nullptr;
+    FabricTable * fabricsTable                                    = nullptr;
 };
 
 class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegate
@@ -118,9 +110,6 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat
         mExchangeMgr        = params.exchangeMgr;
         mUDPEndPointManager = params.udpEndPointManager;
         mFabricIndex        = fabric;
-#if CONFIG_NETWORK_LAYER_BLE
-        mBleLayer = params.bleLayer;
-#endif
     }
 
     /**
@@ -252,10 +241,6 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat
     bool mActive           = false;
     ConnectionState mState = ConnectionState::NotConnected;
 
-#if CONFIG_NETWORK_LAYER_BLE
-    Ble::BleLayer * mBleLayer = nullptr;
-#endif
-
     PASESession mPairing;
 
     SessionManager * mSessionManager = nullptr;

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

@@ -97,7 +97,7 @@ def main():
 
     # TODO: Start at stage 2 once handling for arming failsafe on pase is done.
     if options.report:
-        for testFailureStage in range(3, 17):
+        for testFailureStage in range(3, 18):
             FailIfNot(test.TestPaseOnly(ip=options.deviceAddress1,
                                         setuppin=20202021,
                                         nodeid=1),
@@ -106,7 +106,7 @@ def main():
                       "Commissioning failure tests failed for simulated report failure on stage {}".format(testFailureStage))
 
     else:
-        for testFailureStage in range(3, 17):
+        for testFailureStage in range(3, 18):
             FailIfNot(test.TestPaseOnly(ip=options.deviceAddress1,
                                         setuppin=20202021,
                                         nodeid=1),

src/include/platform/DeviceControlServer.h

@@ -95,6 +95,8 @@ class DeviceControlServer final
     void SetSwitchDelegate(SwitchDeviceControlDelegate * delegate) { mSwitchDelegate = delegate; }
     SwitchDeviceControlDelegate * GetSwitchDelegate() const { return mSwitchDelegate; }
     FailSafeContext & GetFailSafeContext() { return mFailSafeContext; }
+    CHIP_ERROR SetBreadcrumb(uint64_t breadcrumb);
+    uint64_t GetBreadcrumb();
 
     static DeviceControlServer & DeviceControlSvr();
 

src/platform/DeviceControlServer.cpp

@@ -47,6 +47,17 @@ CHIP_ERROR DeviceControlServer::CommissioningComplete(NodeId peerNodeId, FabricI
     return PlatformMgr().PostEvent(&event);
 }
 
+CHIP_ERROR DeviceControlServer::SetBreadcrumb(uint64_t breadcrumb)
+{
+    return ConfigurationMgr().StoreBreadcrumb(breadcrumb);
+}
+uint64_t DeviceControlServer::GetBreadcrumb()
+{
+    uint64_t breadcrumb = 0;
+    ConfigurationMgr().GetBreadcrumb(breadcrumb);
+    return breadcrumb;
+}
+
 CHIP_ERROR DeviceControlServer::SetRegulatoryConfig(uint8_t location, const CharSpan & countryCode, uint64_t breadcrumb)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;

src/protocols/secure_channel/CASEServer.cpp

@@ -30,9 +30,6 @@ using namespace ::chip::Credentials;
 namespace chip {
 
 CHIP_ERROR CASEServer::ListenForSessionEstablishment(Messaging::ExchangeManager * exchangeManager, TransportMgrBase * transportMgr,
-#if CONFIG_NETWORK_LAYER_BLE
-                                                     Ble::BleLayer * bleLayer,
-#endif
                                                      SessionManager * sessionManager, FabricTable * fabrics,
                                                      Credentials::GroupDataProvider * responderGroupDataProvider)
 {
@@ -42,9 +39,6 @@ CHIP_ERROR CASEServer::ListenForSessionEstablishment(Messaging::ExchangeManager
     VerifyOrReturnError(sessionManager != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(responderGroupDataProvider != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
-#if CONFIG_NETWORK_LAYER_BLE
-    mBleLayer = bleLayer;
-#endif
     mSessionManager    = sessionManager;
     mFabrics           = fabrics;
     mExchangeManager   = exchangeManager;
@@ -58,22 +52,6 @@ CHIP_ERROR CASEServer::InitCASEHandshake(Messaging::ExchangeContext * ec)
 {
     ReturnErrorCodeIf(ec == nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
-    // Mark any PASE sessions used for commissioning as stale.
-    // This is a workaround, as we currently don't have a way to identify
-    // secure sessions established via PASE protocol.
-    // TODO - Identify which PASE base secure channel was used
-    //        for commissioning and drop it once commissioning is complete.
-    mSessionManager->ExpireAllPairings(kUndefinedNodeId, kUndefinedFabricIndex);
-
-#if CONFIG_NETWORK_LAYER_BLE
-    // Close all BLE connections now since a CASE handshake has been initiated.
-    if (mBleLayer != nullptr)
-    {
-        ChipLogProgress(Discovery, "CASE handshake initiated, closing all BLE Connections");
-        mBleLayer->CloseAllBleConnections();
-    }
-#endif
-
     // Setup CASE state machine using the credentials for the current fabric.
     GetSession().SetGroupDataProvider(mGroupDataProvider);
     ReturnErrorOnFailure(GetSession().ListenForSessionEstablishment(

src/protocols/secure_channel/CASEServer.h

@@ -17,9 +17,6 @@
 
 #pragma once
 
-#if CONFIG_NETWORK_LAYER_BLE
-#include <ble/BleLayer.h>
-#endif
 #include <credentials/GroupDataProvider.h>
 #include <messaging/ExchangeDelegate.h>
 #include <messaging/ExchangeMgr.h>
@@ -40,9 +37,6 @@ class CASEServer : public SessionEstablishmentDelegate, public Messaging::Exchan
     }
 
     CHIP_ERROR ListenForSessionEstablishment(Messaging::ExchangeManager * exchangeManager, TransportMgrBase * transportMgr,
-#if CONFIG_NETWORK_LAYER_BLE
-                                             Ble::BleLayer * bleLayer,
-#endif
                                              SessionManager * sessionManager, FabricTable * fabrics,
                                              Credentials::GroupDataProvider * responderGroupDataProvider);
 
@@ -63,9 +57,6 @@ class CASEServer : public SessionEstablishmentDelegate, public Messaging::Exchan
 
     CASESession mPairingSession;
     SessionManager * mSessionManager = nullptr;
-#if CONFIG_NETWORK_LAYER_BLE
-    Ble::BleLayer * mBleLayer = nullptr;
-#endif
 
     FabricTable * mFabrics                              = nullptr;
     Credentials::GroupDataProvider * mGroupDataProvider = nullptr;

src/protocols/secure_channel/tests/TestCASESession.cpp

@@ -317,9 +317,6 @@ void CASE_SecurePairingHandshakeServerTest(nlTestSuite * inSuite, void * inConte
 
     NL_TEST_ASSERT(inSuite,
                    gPairingServer.ListenForSessionEstablishment(&ctx.GetExchangeManager(), &ctx.GetTransportMgr(),
-#if CONFIG_NETWORK_LAYER_BLE
-                                                                nullptr,
-#endif
                                                                 &ctx.GetSecureSessionManager(), &gDeviceFabrics,
                                                                 &gDeviceGroupDataProvider) == CHIP_NO_ERROR);