Make MinMDNS only report commissiabe nodes for active browse. (#27670)

* Make MinMDNS only report commissiabe nodes for active browse. MinMDNS had no way to differentiate what type of active commissionable browse operations existed so would report any commissioner or commissionable node the same to devices. This would make commissioner devices like chip-tool be able to detect themselves as 'commissionable' resulting in failed processing. * Restyled by clang-format * Fix unit tests --------- Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Restyled.io <[email protected]>
project-chip · Jan 10, 2024 · 1227467 · 1227467
1 parent 0419ae3
commit 1227467
Show file tree

Hide file tree

Showing 6 changed files with 97 additions and 32 deletions.
diff --git a/src/lib/dnssd/ActiveResolveAttempts.cpp b/src/lib/dnssd/ActiveResolveAttempts.cpp
@@ -55,18 +55,48 @@ void ActiveResolveAttempts::Complete(const PeerId & peerId)
 #endif
 }
 
-void ActiveResolveAttempts::Complete(const chip::Dnssd::DiscoveredNodeData & data)
+void ActiveResolveAttempts::CompleteCommissioner(const chip::Dnssd::DiscoveredNodeData & data)
 {
     for (auto & item : mRetryQueue)
     {
-        if (item.attempt.Matches(data))
+        if (item.attempt.Matches(data, chip::Dnssd::DiscoveryType::kCommissionerNode))
         {
             item.attempt.Clear();
             return;
         }
     }
 }
 
+void ActiveResolveAttempts::CompleteCommissionable(const chip::Dnssd::DiscoveredNodeData & data)
+{
+    for (auto & item : mRetryQueue)
+    {
+        if (item.attempt.Matches(data, chip::Dnssd::DiscoveryType::kCommissionableNode))
+        {
+            item.attempt.Clear();
+            return;
+        }
+    }
+}
+
+bool ActiveResolveAttempts::HasBrowseFor(chip::Dnssd::DiscoveryType type) const
+{
+    for (auto & item : mRetryQueue)
+    {
+        if (!item.attempt.IsBrowse())
+        {
+            continue;
+        }
+
+        if (item.attempt.BrowseData().type == type)
+        {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 void ActiveResolveAttempts::CompleteIpResolution(SerializedQNameIterator targetHostName)
 {
     for (auto & item : mRetryQueue)

diff --git a/src/lib/dnssd/ActiveResolveAttempts.h b/src/lib/dnssd/ActiveResolveAttempts.h
@@ -140,7 +140,7 @@ class ActiveResolveAttempts
         {
             return resolveData.Is<Resolve>() && (resolveData.Get<Resolve>().peerId == peer);
         }
-        bool Matches(const chip::Dnssd::DiscoveredNodeData & data) const
+        bool Matches(const chip::Dnssd::DiscoveredNodeData & data, chip::Dnssd::DiscoveryType type) const
         {
             if (!resolveData.Is<Browse>())
             {
@@ -149,13 +149,11 @@ class ActiveResolveAttempts
 
             auto & browse = resolveData.Get<Browse>();
 
-            // TODO: we should mark returned node data based on the query
-            if (browse.type != chip::Dnssd::DiscoveryType::kCommissionableNode)
+            if (browse.type != type)
             {
-                // We don't currently have markers in the returned DiscoveredNodeData to differentiate these, so assume all returned
-                // packets match
-                return true;
+                return false;
             }
+
             switch (browse.filter.type)
             {
             case chip::Dnssd::DiscoveryFilterType::kNone:
@@ -251,7 +249,8 @@ class ActiveResolveAttempts
 
     /// Mark a resolution as a success, removing it from the internal list
     void Complete(const chip::PeerId & peerId);
-    void Complete(const chip::Dnssd::DiscoveredNodeData & data);
+    void CompleteCommissioner(const chip::Dnssd::DiscoveredNodeData & data);
+    void CompleteCommissionable(const chip::Dnssd::DiscoveredNodeData & data);
     void CompleteIpResolution(SerializedQNameIterator targetHostName);
 
     /// Mark all browse-type scheduled attemptes as a success, removing them
@@ -289,6 +288,9 @@ class ActiveResolveAttempts
     /// IP resolution.
     bool IsWaitingForIpResolutionFor(SerializedQNameIterator hostName) const;
 
+    /// Check if a browse operation is active for the given discovery type
+    bool HasBrowseFor(chip::Dnssd::DiscoveryType type) const;
+
 private:
     struct RetryEntry
     {

diff --git a/src/lib/dnssd/IncrementalResolve.cpp b/src/lib/dnssd/IncrementalResolve.cpp
@@ -54,20 +54,12 @@ class TxtParser : public mdns::Minimal::TxtRecordDelegate
     DataType & mData;
 };
 
-enum class ServiceNameType
-{
-    kInvalid, // not a matter service name
-    kOperational,
-    kCommissioner,
-    kCommissionable,
-};
-
 // Common prefix to check for all operational/commissioner/commissionable name parts
 constexpr QNamePart kOperationalSuffix[]    = { kOperationalServiceName, kOperationalProtocol, kLocalDomain };
 constexpr QNamePart kCommissionableSuffix[] = { kCommissionableServiceName, kCommissionProtocol, kLocalDomain };
 constexpr QNamePart kCommissionerSuffix[]   = { kCommissionerServiceName, kCommissionProtocol, kLocalDomain };
 
-ServiceNameType ComputeServiceNameType(SerializedQNameIterator name)
+IncrementalResolver::ServiceNameType ComputeServiceNameType(SerializedQNameIterator name)
 {
     // SRV record names look like:
     //   <compressed-fabric-id>-<node-id>._matter._tcp.local  (operational)
@@ -78,25 +70,25 @@ ServiceNameType ComputeServiceNameType(SerializedQNameIterator name)
     if (!name.Next() || !name.IsValid())
     {
         // missing required components - empty service name
-        return ServiceNameType::kInvalid;
+        return IncrementalResolver::ServiceNameType::kInvalid;
     }
 
     if (name == kOperationalSuffix)
     {
-        return ServiceNameType::kOperational;
+        return IncrementalResolver::ServiceNameType::kOperational;
     }
 
     if (name == kCommissionableSuffix)
     {
-        return ServiceNameType::kCommissionable;
+        return IncrementalResolver::ServiceNameType::kCommissionable;
     }
 
     if (name == kCommissionerSuffix)
     {
-        return ServiceNameType::kCommissioner;
+        return IncrementalResolver::ServiceNameType::kCommissioner;
     }
 
-    return ServiceNameType::kInvalid;
+    return IncrementalResolver::ServiceNameType::kInvalid;
 }
 
 /// Automatically resets a IncrementalResolver to inactive in its destructor
@@ -171,7 +163,9 @@ CHIP_ERROR IncrementalResolver::InitializeParsing(mdns::Minimal::SerializedQName
         Platform::CopyString(mCommonResolutionData.hostName, serverName.Value());
     }
 
-    switch (ComputeServiceNameType(name))
+    mServiceNameType = ComputeServiceNameType(name);
+
+    switch (mServiceNameType)
     {
     case ServiceNameType::kOperational:
         mSpecificResolutionData.Set<OperationalNodeData>();

diff --git a/src/lib/dnssd/IncrementalResolve.h b/src/lib/dnssd/IncrementalResolve.h
@@ -84,7 +84,16 @@ class IncrementalResolver
     };
     using RequiredInformationFlags = BitFlags<RequiredInformationBitFlags>;
 
-    IncrementalResolver() {}
+    // Type of service name that is contained in this resolver
+    enum class ServiceNameType
+    {
+        kInvalid, // not a matter service name
+        kOperational,
+        kCommissioner,
+        kCommissionable,
+    };
+
+    IncrementalResolver() = default;
 
     /// Checks if object has been initialized using the `InitializeParsing`
     /// method.
@@ -93,6 +102,8 @@ class IncrementalResolver
     bool IsActiveCommissionParse() const { return mSpecificResolutionData.Is<CommissionNodeData>(); }
     bool IsActiveOperationalParse() const { return mSpecificResolutionData.Is<OperationalNodeData>(); }
 
+    ServiceNameType GetCurrentType() const { return mServiceNameType; }
+
     /// Start parsing a new record. SRV records are the records we are mainly
     /// interested on, after which TXT and A/AAAA are looked for.
     ///
@@ -171,6 +182,7 @@ class IncrementalResolver
 
     StoredServerName mRecordName;     // Record name for what is parsed (SRV/PTR/TXT)
     StoredServerName mTargetHostName; // `Target` for the SRV record
+    ServiceNameType mServiceNameType = ServiceNameType::kInvalid;
     CommonResolutionData mCommonResolutionData;
     ParsedRecordSpecificData mSpecificResolutionData;
 };

diff --git a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp
@@ -380,18 +380,45 @@ void MinMdnsResolver::AdvancePendingResolverStates()
             if (err != CHIP_NO_ERROR)
             {
                 ChipLogError(Discovery, "Failed to take discovery result: %" CHIP_ERROR_FORMAT, err.Format());
+                continue;
             }
 
-            mActiveResolves.Complete(nodeData);
-            if (mCommissioningDelegate != nullptr)
+            // TODO: Ideally commissioning delegates should be aware of the
+            //       node types they receive, however they are currently not
+            //       so try to help out by only calling the delegate when an
+            //       active browse exists.
+            //
+            // This is NOT ok and probably we should have separate comissioner
+            // or commissionable delegates or pass in a node type argument.
+            bool discoveredNodeIsRelevant = false;
+
+            switch (resolver->GetCurrentType())
             {
-                mCommissioningDelegate->OnNodeDiscovered(nodeData);
+            case IncrementalResolver::ServiceNameType::kCommissioner:
+                discoveredNodeIsRelevant = mActiveResolves.HasBrowseFor(chip::Dnssd::DiscoveryType::kCommissionerNode);
+                mActiveResolves.CompleteCommissioner(nodeData);
+                break;
+            case IncrementalResolver::ServiceNameType::kCommissionable:
+                discoveredNodeIsRelevant = mActiveResolves.HasBrowseFor(chip::Dnssd::DiscoveryType::kCommissionableNode);
+                mActiveResolves.CompleteCommissionable(nodeData);
+                break;
+            default:
+                ChipLogError(Discovery, "Unexpected type for commission data parsing");
+                continue;
             }
-            else
+
+            if (discoveredNodeIsRelevant)
             {
+                if (mCommissioningDelegate != nullptr)
+                {
+                    mCommissioningDelegate->OnNodeDiscovered(nodeData);
+                }
+                else
+                {
 #if CHIP_MINMDNS_HIGH_VERBOSITY
-                ChipLogError(Discovery, "No delegate to report commissioning node discovery");
+                    ChipLogError(Discovery, "No delegate to report commissioning node discovery");
 #endif
+                }
             }
         }
         else if (resolver->IsActiveOperationalParse())

diff --git a/src/lib/dnssd/tests/TestActiveResolveAttempts.cpp b/src/lib/dnssd/tests/TestActiveResolveAttempts.cpp
@@ -127,7 +127,7 @@ void TestSingleBrowseAddRemove(nlTestSuite * inSuite, void * inContext)
     // once complete, nothing to schedule
     Dnssd::DiscoveredNodeData data;
     data.commissionData.longDiscriminator = 1234;
-    attempts.Complete(data);
+    attempts.CompleteCommissionable(data);
     NL_TEST_ASSERT(inSuite, !attempts.GetTimeUntilNextExpectedResponse().HasValue());
     NL_TEST_ASSERT(inSuite, !attempts.NextScheduled().HasValue());
 }
@@ -376,7 +376,7 @@ void TestCombination(nlTestSuite * inSuite, void * inContext)
     attempts.Complete(MakePeerId(1));
     Dnssd::DiscoveredNodeData data;
     data.commissionData.longDiscriminator = 1234;
-    attempts.Complete(data);
+    attempts.CompleteCommissionable(data);
 
     NL_TEST_ASSERT(inSuite, !attempts.GetTimeUntilNextExpectedResponse().HasValue());
     NL_TEST_ASSERT(inSuite, !attempts.NextScheduled().HasValue());