[captive_portal] Show each network once in the scan list (#17847)

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: Bluetooth Devices Bot <bluetooth@koston.org>

esphome/components/captive_portal/captive_portal.cpp

@@ -6,6 +6,7 @@
 #include "esphome/core/string_ref.h"
 #include "esphome/components/wifi/wifi_component.h"
 #include "captive_index.h"
+#include "scan_list.h"
 
 namespace esphome::captive_portal {
 
@@ -33,8 +34,10 @@ void CaptivePortal::handle_config(AsyncWebServerRequest *request) {
     // Invariant: only bounded in-memory work under the lock; the network send
     // happens later in request->send()
     wifi::ScanResultsLock lock(wifi::global_wifi_component);
-    for (const auto &scan : wifi::global_wifi_component->get_scan_result()) {
+    const auto &results = wifi::global_wifi_component->get_scan_result();
-      if (scan.get_is_hidden())
+    for (const auto &scan : results) {
+      bool with_auth = false;
+      if (!should_show_scan_entry(results, scan, with_auth))
         continue;
 
       json_escape_into_buffer(escaped_ssid, scan.get_ssid());
@@ -44,10 +47,10 @@ void CaptivePortal::handle_config(AsyncWebServerRequest *request) {
       stream->print(ESPHOME_F("\",\"rssi\":"));
       stream->print(scan.get_rssi());
       stream->print(ESPHOME_F(",\"lock\":"));
-      stream->print(scan.get_with_auth());
+      stream->print(with_auth);
       stream->print(ESPHOME_F("}"));
 #else
-      stream->printf(R"(,{"ssid":"%s","rssi":%d,"lock":%d})", escaped_ssid, scan.get_rssi(), scan.get_with_auth());
+      stream->printf(R"(,{"ssid":"%s","rssi":%d,"lock":%d})", escaped_ssid, scan.get_rssi(), with_auth);
 #endif
     }
   }

esphome/components/captive_portal/scan_list.h

@@ -0,0 +1,28 @@
+#pragma once
+#include <cstdint>
+
+namespace esphome::captive_portal {
+
+// A scan lists every BSSID, so one SSID can appear several times. Returns true for
+// the strongest entry per SSID (earliest on ties), never for hidden entries. scan
+// must be an element of results. with_auth is written only when returning true and
+// is set if any entry with that SSID needs a key. Templated for host tests.
+template<typename Results, typename Entry>
+bool should_show_scan_entry(const Results &results, const Entry &scan, bool &with_auth) {
+  if (scan.get_is_hidden())
+    return false;
+  const int8_t rssi = scan.get_rssi();
+  bool any_auth = false;
+  for (const auto &other : results) {
+    if (other.get_is_hidden() || !other.ssid_equals(scan))
+      continue;
+    // Same array, so address order is index order. scan fails both checks against itself.
+    if (other.get_rssi() > rssi || (other.get_rssi() == rssi && &other < &scan))
+      return false;
+    any_auth |= other.get_with_auth();
+  }
+  with_auth = any_auth;
+  return true;
+}
+
+}  // namespace esphome::captive_portal

esphome/components/wifi/wifi_component.h

@@ -327,6 +327,7 @@ class WiFiScanResult {
   int8_t get_rssi() const { return this->rssi_; }
   bool get_with_auth() const { return this->with_auth_; }
   bool get_is_hidden() const { return this->is_hidden_; }
+  bool ssid_equals(const WiFiScanResult &other) const { return this->ssid_ == other.ssid_; }
   int8_t get_priority() const { return priority_; }
   void set_priority(int8_t priority) { priority_ = priority; }
 

tests/components/captive_portal/__init__.py

@@ -0,0 +1,10 @@
+from tests.testing_helpers import ComponentManifestOverride
+
+
+def override_manifest(manifest: ComponentManifestOverride) -> None:
+    # The scan list helper is header-only and needs none of the component's real
+    # dependencies. Pulling them in breaks the host build: web_server_base
+    # includes ESPAsyncWebServer.h and ota.web_server includes md5/md5.h, neither
+    # of which exists there.
+    manifest.dependencies = []
+    manifest.auto_load = []

tests/components/captive_portal/scan_list_test.cpp

@@ -0,0 +1,130 @@
+#include <gtest/gtest.h>
+
+#include <cstdint>
+#include <string>
+#include <vector>
+
+#include "esphome/components/captive_portal/scan_list.h"
+
+namespace esphome::captive_portal::testing {
+
+namespace {
+
+// Stand-in for wifi::WiFiScanResult, which does not compile on the host.
+struct Entry {
+  std::string ssid;
+  int8_t rssi;
+  bool with_auth{true};
+  bool is_hidden{false};
+
+  // Compares length and bytes like CompactString does, so an embedded NUL counts.
+  bool ssid_equals(const Entry &other) const { return this->ssid == other.ssid; }
+  int8_t get_rssi() const { return this->rssi; }
+  bool get_with_auth() const { return this->with_auth; }
+  bool get_is_hidden() const { return this->is_hidden; }
+};
+
+// One row as the portal would emit it.
+struct Row {
+  std::string ssid;
+  int8_t rssi;
+  bool lock;
+
+  bool operator==(const Row &rhs) const { return ssid == rhs.ssid && rssi == rhs.rssi && lock == rhs.lock; }
+};
+
+// Walk the results the way handle_config does and collect the rows that survive.
+std::vector<Row> rows(const std::vector<Entry> &results) {
+  std::vector<Row> out;
+  for (size_t i = 0; i < results.size(); i++) {
+    bool with_auth = false;
+    if (!should_show_scan_entry(results, results[i], with_auth))
+      continue;
+    out.push_back({results[i].ssid, results[i].rssi, with_auth});
+  }
+  return out;
+}
+
+}  // namespace
+
+TEST(ScanList, SingleEntryShown) {
+  std::vector<Entry> results = {{"Home", -60}};
+  EXPECT_EQ(rows(results), (std::vector<Row>{{"Home", -60, true}}));
+}
+
+TEST(ScanList, DistinctSsidsAllShownInOrder) {
+  std::vector<Entry> results = {{"Home", -60}, {"Guest", -70}, {"Cafe", -40}};
+  EXPECT_EQ(rows(results), (std::vector<Row>{{"Home", -60, true}, {"Guest", -70, true}, {"Cafe", -40, true}}));
+}
+
+// Results are ordered by connection preference, not RSSI, so the strongest entry
+// can sit anywhere in the list.
+TEST(ScanList, SameSsidKeepsStrongest) {
+  std::vector<Entry> results = {{"Home", -70}, {"Home", -50}, {"Home", -60}};
+  EXPECT_EQ(rows(results), (std::vector<Row>{{"Home", -50, true}}));
+}
+
+TEST(ScanList, EqualRssiKeepsFirst) {
+  std::vector<Entry> results = {{"Home", -60}, {"Home", -60}, {"Home", -60}};
+  bool with_auth = false;
+  EXPECT_TRUE(should_show_scan_entry(results, results[0], with_auth));
+  EXPECT_FALSE(should_show_scan_entry(results, results[1], with_auth));
+  EXPECT_FALSE(should_show_scan_entry(results, results[2], with_auth));
+  EXPECT_EQ(rows(results), (std::vector<Row>{{"Home", -60, true}}));
+}
+
+// with_auth is an out-parameter that must only be written for a shown entry.
+TEST(ScanList, WithAuthUntouchedWhenNotShown) {
+  std::vector<Entry> results = {{"Home", -50, false}, {"Home", -70, true}};
+  bool with_auth = false;
+  EXPECT_FALSE(should_show_scan_entry(results, results[1], with_auth));
+  EXPECT_FALSE(with_auth);
+}
+
+TEST(ScanList, DuplicatesInterleavedWithOtherNetworks) {
+  std::vector<Entry> results = {{"Home", -70}, {"Guest", -55}, {"Home", -50}, {"Guest", -65}};
+  EXPECT_EQ(rows(results), (std::vector<Row>{{"Guest", -55, true}, {"Home", -50, true}}));
+}
+
+// Hidden networks scan with an empty SSID. They are never listed and do not
+// collapse into each other or into anything else.
+TEST(ScanList, HiddenEntriesNeverShown) {
+  std::vector<Entry> results = {{"", -40, true, true}, {"Home", -70}, {"", -30, true, true}};
+  EXPECT_EQ(rows(results), (std::vector<Row>{{"Home", -70, true}}));
+}
+
+// On ESP8266 the hidden flag comes from the driver alongside a real SSID, so a
+// hidden access point can share its name with a visible one. It must not
+// outrank that visible entry and leave the network unlisted.
+TEST(ScanList, HiddenEntryDoesNotSuppressVisibleSameSsid) {
+  std::vector<Entry> results = {{"Home", -40, true, true}, {"Home", -70}};
+  EXPECT_EQ(rows(results), (std::vector<Row>{{"Home", -70, true}}));
+}
+
+// An open access point and a secured one sharing an SSID collapse to one row that
+// still asks for a password, whichever of them is strongest.
+TEST(ScanList, LockSetWhenAnyEntryRequiresAuth) {
+  std::vector<Entry> open_stronger = {{"Home", -50, false}, {"Home", -70, true}};
+  EXPECT_EQ(rows(open_stronger), (std::vector<Row>{{"Home", -50, true}}));
+
+  std::vector<Entry> secured_stronger = {{"Home", -70, false}, {"Home", -50, true}};
+  EXPECT_EQ(rows(secured_stronger), (std::vector<Row>{{"Home", -50, true}}));
+}
+
+TEST(ScanList, LockClearWhenEveryEntryIsOpen) {
+  std::vector<Entry> results = {{"Cafe", -60, false}, {"Cafe", -50, false}};
+  EXPECT_EQ(rows(results), (std::vector<Row>{{"Cafe", -50, false}}));
+}
+
+// The auth flag of an unrelated network must not leak into another SSID's row.
+TEST(ScanList, LockIsPerSsid) {
+  std::vector<Entry> results = {{"Cafe", -60, false}, {"Home", -50, true}};
+  EXPECT_EQ(rows(results), (std::vector<Row>{{"Cafe", -60, false}, {"Home", -50, true}}));
+}
+
+TEST(ScanList, EmptyListShowsNothing) {
+  std::vector<Entry> results;
+  EXPECT_TRUE(rows(results).empty());
+}
+
+}  // namespace esphome::captive_portal::testing