Fix incorrect advertisement after RemoveFabric. (#15890)

We ended up advertising ourselves as commissionable even though we
weren't.

Remove the guessing about what we should advertise and add an explicit
API for the DNS-SD server to be able to ask what commissioning mode we
should be in if it's not told a specific mode.

Removed some StartServer() calls in places where we did manual
advertising right before or after opening a comissioning window (which
already resets advertising).

Fixes #15875

examples/all-clusters-app/esp32/main/DeviceWithDisplay.cpp

@@ -354,7 +354,6 @@ class SetupListModel : public TouchesMatterStackModel
         }
         else if (i == 2)
         {
-            app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kEnabledBasic);
             chip::Server::GetInstance().GetFabricTable().DeleteAllFabrics();
             chip::Server::GetInstance().GetCommissioningWindowManager().OpenBasicCommissioningWindow(
                 kNoCommissioningTimeout, CommissioningWindowAdvertisement::kDnssdOnly);

examples/tv-casting-app/linux/main.cpp

@@ -164,9 +164,6 @@ void PrepareForCommissioning(const Dnssd::DiscoveredNodeData * selectedCommissio
 #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY_CLIENT
     if (selectedCommissioner != nullptr)
     {
-        // Advertise self as Commissionable Node over mDNS
-        app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kEnabledBasic);
-
         // Send User Directed commissioning request
         ReturnOnFailure(Server::GetInstance().SendUserDirectedCommissioningRequest(chip::Transport::PeerAddress::UDP(
             selectedCommissioner->ipAddress[0], selectedCommissioner->port, selectedCommissioner->interfaceId)));

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

@@ -375,6 +375,8 @@ bool emberAfOperationalCredentialsClusterRemoveFabricCallback(app::CommandHandle
     CHIP_ERROR err = Server::GetInstance().GetFabricTable().Delete(fabricBeingRemoved);
     SuccessOrExit(err);
 
+    // We need to withdraw the advertisement for the now-removed fabric, so need
+    // to restart advertising altogether.
     app::DnssdServer::Instance().StartServer();
 
 exit:
@@ -629,11 +631,10 @@ bool emberAfOperationalCredentialsClusterUpdateNOCCallback(app::CommandHandler *
 
     fabricIndex = fabric->GetFabricIndex();
 
-    // We have a new operational identity and should start advertising it.  We
-    // can't just wait until we get network configuration commands, because we
-    // might be on the operational network already, in which case we are
-    // expected to be live with our new identity at this point.
-    app::DnssdServer::Instance().AdvertiseOperational();
+    // We might have a new operational identity, so we should start advertising
+    // it right away.  Also, we need to withdraw our old operational identity.
+    // So we need to StartServer() here.
+    app::DnssdServer::Instance().StartServer();
 
     // The Fabric Index associated with the armed fail-safe context SHALL be updated to match the Fabric
     // Index associated with the UpdateNOC command being invoked.

src/app/server/BUILD.gn

@@ -37,6 +37,7 @@ static_library("server") {
   output_name = "libCHIPAppServer"
 
   sources = [
+    "CommissioningModeProvider.h",
     "CommissioningWindowManager.cpp",
     "CommissioningWindowManager.h",
     "Dnssd.cpp",

src/app/server/CommissioningModeProvider.h

@@ -0,0 +1,39 @@
+/*
+ *
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#pragma once
+
+#include <lib/dnssd/Advertiser.h>
+
+namespace chip {
+namespace app {
+
+/**
+ * A way to ask what commissioning mode DnssdServer should be advertising.  This
+ * needs to match the actual commissioning mode of the device (i.e. whether the
+ * device will accept an attempt to establish a PASE session).
+ */
+class DLL_EXPORT CommissioningModeProvider
+{
+public:
+    virtual Dnssd::CommissioningMode GetCommissioningMode() const = 0;
+
+    virtual ~CommissioningModeProvider() {}
+};
+
+} // namespace app
+} // namespace chip

src/app/server/CommissioningWindowManager.cpp

@@ -177,6 +177,7 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow()
 
     ReturnErrorOnFailure(mServer->GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(
         Protocols::SecureChannel::MsgType::PBKDFParamRequest, &mPairingSession));
+    mListeningForPASE = true;
 
     if (mUseECM)
     {
@@ -279,6 +280,27 @@ void CommissioningWindowManager::CloseCommissioningWindow()
     }
 }
 
+Dnssd::CommissioningMode CommissioningWindowManager::GetCommissioningMode() const
+{
+    if (!mListeningForPASE)
+    {
+        // We should not be advertising ourselves as in commissioning mode.
+        // We need to check this before mWindowStatus, because we might have an
+        // open window even while we are not listening for PASE.
+        return Dnssd::CommissioningMode::kDisabled;
+    }
+
+    switch (mWindowStatus)
+    {
+    case AdministratorCommissioning::CommissioningWindowStatus::kEnhancedWindowOpen:
+        return Dnssd::CommissioningMode::kEnabledEnhanced;
+    case AdministratorCommissioning::CommissioningWindowStatus::kBasicWindowOpen:
+        return Dnssd::CommissioningMode::kEnabledBasic;
+    default:
+        return Dnssd::CommissioningMode::kDisabled;
+    }
+}
+
 CHIP_ERROR CommissioningWindowManager::StartAdvertisement()
 {
 #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING
@@ -303,20 +325,17 @@ CHIP_ERROR CommissioningWindowManager::StartAdvertisement()
         mAppDelegate->OnPairingWindowOpened();
     }
 
-    Dnssd::CommissioningMode commissioningMode;
     if (mUseECM)
     {
-        mWindowStatus     = AdministratorCommissioning::CommissioningWindowStatus::kEnhancedWindowOpen;
-        commissioningMode = Dnssd::CommissioningMode::kEnabledEnhanced;
+        mWindowStatus = AdministratorCommissioning::CommissioningWindowStatus::kEnhancedWindowOpen;
     }
     else
     {
-        mWindowStatus     = AdministratorCommissioning::CommissioningWindowStatus::kBasicWindowOpen;
-        commissioningMode = Dnssd::CommissioningMode::kEnabledBasic;
+        mWindowStatus = AdministratorCommissioning::CommissioningWindowStatus::kBasicWindowOpen;
     }
 
-    // reset all advertising, indicating we are in commissioningMode
-    app::DnssdServer::Instance().StartServer(commissioningMode);
+    // reset all advertising, switching to our new commissioning mode.
+    app::DnssdServer::Instance().StartServer();
 
     return CHIP_NO_ERROR;
 }
@@ -326,6 +345,7 @@ CHIP_ERROR CommissioningWindowManager::StopAdvertisement(bool aShuttingDown)
     RestoreDiscriminator();
 
     mServer->GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Protocols::SecureChannel::MsgType::PBKDFParamRequest);
+    mListeningForPASE = false;
     mPairingSession.Clear();
 
 #if CHIP_DEVICE_CONFIG_ENABLE_SED
@@ -341,7 +361,7 @@ CHIP_ERROR CommissioningWindowManager::StopAdvertisement(bool aShuttingDown)
         // Stop advertising commissioning mode, since we're not accepting PASE
         // connections right now.  If we start accepting them again (via
         // OpenCommissioningWindow) that will call StartAdvertisement as needed.
-        app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kDisabled);
+        app::DnssdServer::Instance().StartServer();
     }
 
     if (mIsBLE)

src/app/server/CommissioningWindowManager.h

@@ -19,6 +19,8 @@
 
 #include <app-common/zap-generated/cluster-objects.h>
 #include <app/server/AppDelegate.h>
+#include <app/server/CommissioningModeProvider.h>
+#include <lib/dnssd/Advertiser.h>
 #include <protocols/secure_channel/RendezvousParameters.h>
 #include <protocols/secure_channel/SessionIDAllocator.h>
 
@@ -34,7 +36,7 @@ enum class CommissioningWindowAdvertisement
 
 class Server;
 
-class CommissioningWindowManager : public SessionEstablishmentDelegate
+class CommissioningWindowManager : public SessionEstablishmentDelegate, public app::CommissioningModeProvider
 {
 public:
     CommissioningWindowManager(Server * server) : mAppDelegate(nullptr), mServer(server) {}
@@ -61,6 +63,9 @@ class CommissioningWindowManager : public SessionEstablishmentDelegate
 
     app::Clusters::AdministratorCommissioning::CommissioningWindowStatus CommissioningWindowStatus() const { return mWindowStatus; }
 
+    // CommissioningModeProvider implemetation.
+    Dnssd::CommissioningMode GetCommissioningMode() const override;
+
     //////////// SessionEstablishmentDelegate Implementation ///////////////
     void OnSessionEstablishmentError(CHIP_ERROR error) override;
     void OnSessionEstablishmentStarted() override;
@@ -109,8 +114,11 @@ class CommissioningWindowManager : public SessionEstablishmentDelegate
     Spake2pVerifier mECMPASEVerifier;
     uint16_t mECMDiscriminator = 0;
     PasscodeId mECMPasscodeID  = kDefaultCommissioningPasscodeId;
-    uint32_t mECMIterations    = 0;
-    uint32_t mECMSaltLength    = 0;
+    // mListeningForPASE is true only when we are listening for
+    // PBKDFParamRequest messages.
+    bool mListeningForPASE  = false;
+    uint32_t mECMIterations = 0;
+    uint32_t mECMSaltLength = 0;
     uint8_t mECMSalt[kSpake2p_Max_PBKDF_Salt_Length];
 };
 

src/app/server/Dnssd.cpp

@@ -418,20 +418,17 @@ CHIP_ERROR DnssdServer::AdvertiseCommissionableNode(chip::Dnssd::CommissioningMo
 
 void DnssdServer::StartServer()
 {
-    return StartServer(NullOptional);
+    Dnssd::CommissioningMode mode = Dnssd::CommissioningMode::kDisabled;
+    if (mCommissioningModeProvider)
+    {
+        mode = mCommissioningModeProvider->GetCommissioningMode();
+    }
+    return StartServer(mode);
 }
 
 void DnssdServer::StartServer(Dnssd::CommissioningMode mode)
 {
-    return StartServer(MakeOptional(mode));
-}
-
-void DnssdServer::StartServer(Optional<Dnssd::CommissioningMode> mode)
-{
-    // Default to CommissioningMode::kDisabled if no value is provided.
-    Dnssd::CommissioningMode modeValue = mode.ValueOr(Dnssd::CommissioningMode::kDisabled);
-
-    ChipLogDetail(Discovery, "DNS-SD StartServer modeHasValue=%d modeValue=%d", mode.HasValue(), static_cast<int>(modeValue));
+    ChipLogDetail(Discovery, "DNS-SD StartServer mode=%d", static_cast<int>(mode));
 
     ClearTimeouts();
 
@@ -458,9 +455,9 @@ void DnssdServer::StartServer(Optional<Dnssd::CommissioningMode> mode)
     if (HaveOperationalCredentials())
     {
         ChipLogProgress(Discovery, "Have operational credentials");
-        if (modeValue != chip::Dnssd::CommissioningMode::kDisabled)
+        if (mode != chip::Dnssd::CommissioningMode::kDisabled)
         {
-            err = AdvertiseCommissionableNode(modeValue);
+            err = AdvertiseCommissionableNode(mode);
             if (err != CHIP_NO_ERROR)
             {
                 ChipLogError(Discovery, "Failed to advertise commissionable node: %s", chip::ErrorStr(err));
@@ -470,7 +467,7 @@ void DnssdServer::StartServer(Optional<Dnssd::CommissioningMode> mode)
 #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
         else if (GetExtendedDiscoveryTimeoutSecs() != CHIP_DEVICE_CONFIG_DISCOVERY_DISABLED)
         {
-            err = AdvertiseCommissionableNode(modeValue);
+            err = AdvertiseCommissionableNode(mode);
             if (err != CHIP_NO_ERROR)
             {
                 ChipLogError(Discovery, "Failed to advertise extended commissionable node: %s", chip::ErrorStr(err));
@@ -484,7 +481,7 @@ void DnssdServer::StartServer(Optional<Dnssd::CommissioningMode> mode)
     {
 #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY
         ChipLogProgress(Discovery, "Start dns-sd server - no current nodeId");
-        err = AdvertiseCommissionableNode(mode.ValueOr(chip::Dnssd::CommissioningMode::kEnabledBasic));
+        err = AdvertiseCommissionableNode(mode);
         if (err != CHIP_NO_ERROR)
         {
             ChipLogError(Discovery, "Failed to advertise unprovisioned commissionable node: %s", chip::ErrorStr(err));

src/app/server/Dnssd.h

@@ -17,6 +17,7 @@
 
 #pragma once
 
+#include <app/server/CommissioningModeProvider.h>
 #include <credentials/FabricTable.h>
 #include <lib/core/CHIPError.h>
 #include <lib/core/Optional.h>
@@ -74,6 +75,10 @@ class DLL_EXPORT DnssdServer
         mFabricTable = table;
     }
 
+    // Set the commissioning mode provider to use.  Null provider will mean we
+    // assume the commissioning mode is kDisabled.
+    void SetCommissioningModeProvider(CommissioningModeProvider * provider) { mCommissioningModeProvider = provider; }
+
     /// Callback from Discovery Expiration timer
     /// Checks if discovery has expired and if so,
     /// kicks off extend discovery (when enabled)
@@ -95,9 +100,8 @@ class DLL_EXPORT DnssdServer
     /// Start operational advertising
     CHIP_ERROR AdvertiseOperational();
 
-    /// (Re-)starts the Dnssd server
-    /// - if device has not yet been commissioned, then commissioning mode will show as enabled (CM=1)
-    /// - if device has been commissioned, then commissioning mode will be disabled.
+    /// (Re-)starts the Dnssd server, using the commissioning mode from our
+    /// commissioning mode provider.
     void StartServer();
 
     /// (Re-)starts the Dnssd server, using the provided commissioning mode.
@@ -138,10 +142,8 @@ class DLL_EXPORT DnssdServer
 #endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
     }
 
-    FabricTable * mFabricTable = nullptr;
-
-    // Helper for StartServer.
-    void StartServer(Optional<Dnssd::CommissioningMode> mode);
+    FabricTable * mFabricTable                             = nullptr;
+    CommissioningModeProvider * mCommissioningModeProvider = nullptr;
 
     uint16_t mSecuredPort          = CHIP_PORT;
     uint16_t mUnsecuredPort        = CHIP_UDC_PORT;

src/app/server/Server.cpp

@@ -140,6 +140,7 @@ CHIP_ERROR Server::Init(AppDelegate * delegate, uint16_t secureServicePort, uint
     SuccessOrExit(err);
 
     app::DnssdServer::Instance().SetFabricTable(&mFabrics);
+    app::DnssdServer::Instance().SetCommissioningModeProvider(&mCommissioningWindowManager);
 
     // Group data provider must be initialized after mDeviceStorage
     err = mGroupsProvider.Init();
@@ -334,6 +335,7 @@ void Server::FactoryReset(intptr_t arg)
 
 void Server::Shutdown()
 {
+    app::DnssdServer::Instance().SetCommissioningModeProvider(nullptr);
     chip::Dnssd::ServiceAdvertiser::Instance().Shutdown();
     chip::app::InteractionModelEngine::GetInstance()->Shutdown();
     CHIP_ERROR err = mExchangeMgr.Shutdown();

src/controller/CHIPDeviceControllerFactory.cpp

@@ -191,9 +191,11 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
         app::DnssdServer::Instance().SetFabricTable(stateParams.fabricTable);
 
         //
-        // Start up the DNS-SD server
+        // Start up the DNS-SD server.  We are not giving it a
+        // CommissioningModeProvider, so it will not claim we are in
+        // commissioning mode.
         //
-        chip::app::DnssdServer::Instance().StartServer(chip::Dnssd::CommissioningMode::kDisabled);
+        chip::app::DnssdServer::Instance().StartServer();
     }
 
     // store the system state