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