[CrOS Cellular] Update CellularESimProfileHandlerImpl auto-refresh logic

Before this CL, whenever we discovered a new EUICC exposed by Hermes,
we'd kick off a refresh for profiles on that EUICC and immediately
update system prefs to store that EUICC's path. This CL updates this
logic so that we do not update prefs until the operation completes
successfully. We also expose a new HasRefreshedProfilesForEuicc()
function which returns whether or not we have ever refreshed profiles
for a given EUICC.

A follow-up CL uses this logic to ensure that we do not unintentionally
remove Shill services corresponding to EUICCs which are in the process
of being refreshed.

Bug: b/186753024
Change-Id: I6d682b3e57bfdb962cf59fb9c45ea32c8fcdafd6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2870535
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Azeem Arshad <azeemarshad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#879215}

chromeos/network/cellular_esim_profile_handler.h

@@ -68,6 +68,11 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularESimProfileHandler
   // (e.g., if ModemManager is currently pointed to a pSIM slot).
   virtual std::vector<CellularESimProfile> GetESimProfiles() = 0;
 
+  // Returns whether profiles for the EUICC with the given EID have been
+  // refreshsed. If this function returns true, any known eSIM profiles for the
+  // associated EUICC should be returned by GetESimProfiles().
+  virtual bool HasRefreshedProfilesForEuicc(const std::string& eid) = 0;
+
   virtual void SetDevicePrefs(PrefService* device_prefs) = 0;
 
   void AddObserver(Observer* observer);

chromeos/network/cellular_esim_profile_handler_impl.cc

@@ -4,6 +4,7 @@
 
 #include "chromeos/network/cellular_esim_profile_handler_impl.h"
 
+#include <sstream>
 #include <vector>
 
 #include "ash/constants/ash_pref_names.h"
@@ -56,7 +57,7 @@ void CellularESimProfileHandlerImpl::DeviceListChanged() {
   if (!device_prefs_)
     return;
 
-  RefreshEuiccsIfNecessary();
+  AutoRefreshEuiccsIfNecessary();
 }
 
 void CellularESimProfileHandlerImpl::InitInternal() {
@@ -96,6 +97,24 @@ CellularESimProfileHandlerImpl::GetESimProfiles() {
   return profiles;
 }
 
+bool CellularESimProfileHandlerImpl::HasRefreshedProfilesForEuicc(
+    const std::string& eid) {
+  base::flat_set<std::string> euicc_paths =
+      GetAutoRefreshedEuiccPathsFromPrefs();
+
+  for (const auto& path : euicc_paths) {
+    HermesEuiccClient::Properties* euicc_properties =
+        HermesEuiccClient::Get()->GetProperties(dbus::ObjectPath(path));
+    if (!euicc_properties)
+      continue;
+
+    if (eid == euicc_properties->eid().value())
+      return true;
+  }
+
+  return false;
+}
+
 void CellularESimProfileHandlerImpl::SetDevicePrefs(PrefService* device_prefs) {
   device_prefs_ = device_prefs;
   OnHermesPropertiesUpdated();
@@ -105,88 +124,122 @@ void CellularESimProfileHandlerImpl::OnHermesPropertiesUpdated() {
   if (!device_prefs_)
     return;
 
-  RefreshEuiccsIfNecessary();
+  AutoRefreshEuiccsIfNecessary();
   UpdateProfilesFromHermes();
 }
 
-void CellularESimProfileHandlerImpl::RefreshEuiccsIfNecessary() {
+void CellularESimProfileHandlerImpl::AutoRefreshEuiccsIfNecessary() {
   if (!CellularDeviceExists())
     return;
 
   base::flat_set<std::string> euicc_paths_from_hermes =
       GetEuiccPathsFromHermes();
-  base::flat_set<std::string> euicc_paths_from_prefs = GetEuiccPathsFromPrefs();
+  base::flat_set<std::string> auto_refreshed_euicc_paths =
+      GetAutoRefreshedEuiccPaths();
 
-  // If the paths in prefs and Hermes match, we have already tried refreshing
-  // them both, and there is nothing else to do.
-  if (euicc_paths_from_hermes == euicc_paths_from_prefs)
-    return;
-
-  base::flat_set<std::string> paths_in_hermes_but_not_prefs;
-  for (const auto& hermes_path : euicc_paths_from_hermes) {
-    if (!base::Contains(euicc_paths_from_prefs, hermes_path))
-      paths_in_hermes_but_not_prefs.insert(hermes_path);
+  base::flat_set<std::string> paths_needing_auto_refresh;
+  for (const auto& euicc_path : euicc_paths_from_hermes) {
+    if (!base::Contains(auto_refreshed_euicc_paths, euicc_path))
+      paths_needing_auto_refresh.insert(euicc_path);
   }
 
   // We only need to request profiles if we see a new EUICC from Hermes that we
   // have not yet seen before. If no such EUICCs exist, return early.
-  if (paths_in_hermes_but_not_prefs.empty())
+  if (paths_needing_auto_refresh.empty())
     return;
 
-  // If there is more than one EUICC, log a warning. This configuration is not
-  // officially supported, so this may be helpful in feedback reports.
-  if (paths_in_hermes_but_not_prefs.size() > 1u)
-    NET_LOG(ERROR) << "Attempting to refresh profiles from multiple EUICCs";
+  StartAutoRefresh(paths_needing_auto_refresh);
+}
 
-  // Combine both sets together and store them to prefs to ensure that we do not
-  // need to refresh again for the same EUICCs.
-  base::flat_set<std::string> all_paths;
-  all_paths.insert(euicc_paths_from_prefs.begin(),
-                   euicc_paths_from_prefs.end());
-  all_paths.insert(euicc_paths_from_hermes.begin(),
-                   euicc_paths_from_hermes.end());
-  StoreEuiccPathsToPrefs(all_paths);
+void CellularESimProfileHandlerImpl::StartAutoRefresh(
+    const base::flat_set<std::string>& euicc_paths) {
+  paths_pending_auto_refresh_.insert(euicc_paths.begin(), euicc_paths.end());
+
+  // If there is more than one EUICC, log an error. This configuration is not
+  // officially supported, so this log may be helpful in feedback reports.
+  if (euicc_paths.size() > 1u)
+    NET_LOG(ERROR) << "Attempting to refresh profiles from multiple EUICCs";
 
   // Refresh profiles from the unknown EUICCs. Note that this will internally
   // start an inhibit operation, temporarily blocking the user from changing
   // cellular settings. This operation is only expected to occur when the device
   // originally boots or after a powerwash.
-  for (const auto& path : paths_in_hermes_but_not_prefs) {
+  for (const auto& path : euicc_paths) {
     NET_LOG(EVENT) << "Found new EUICC whose profiles have not yet been "
                    << "refreshsed. Refreshing profile list for " << path;
-    RefreshProfileList(dbus::ObjectPath(path), base::DoNothing());
+    RefreshProfileList(
+        dbus::ObjectPath(path),
+        base::BindOnce(
+            &CellularESimProfileHandlerImpl::OnAutoRefreshEuiccComplete,
+            weak_ptr_factory_.GetWeakPtr(), path));
   }
 }
 
 base::flat_set<std::string>
-CellularESimProfileHandlerImpl::GetEuiccPathsFromPrefs() const {
+CellularESimProfileHandlerImpl::GetAutoRefreshedEuiccPaths() const {
+  // Add all paths stored in prefs.
+  base::flat_set<std::string> euicc_paths =
+      GetAutoRefreshedEuiccPathsFromPrefs();
+
+  // Add paths which are currently pending a refresh.
+  euicc_paths.insert(paths_pending_auto_refresh_.begin(),
+                     paths_pending_auto_refresh_.end());
+
+  return euicc_paths;
+}
+
+base::flat_set<std::string>
+CellularESimProfileHandlerImpl::GetAutoRefreshedEuiccPathsFromPrefs() const {
   DCHECK(device_prefs_);
+
   const base::ListValue* euicc_paths_from_prefs =
       device_prefs_->GetList(prefs::kESimRefreshedEuiccs);
   if (!euicc_paths_from_prefs) {
     NET_LOG(ERROR) << "Could not fetch refreshed EUICCs pref.";
     return {};
   }
 
-  base::flat_set<std::string> euicc_paths_from_prefs_set;
+  base::flat_set<std::string> euicc_paths;
   for (const auto& euicc : *euicc_paths_from_prefs) {
     if (!euicc.is_string()) {
       NET_LOG(ERROR) << "Non-string EUICC path: " << euicc;
       continue;
     }
-    euicc_paths_from_prefs_set.insert(euicc.GetString());
+    euicc_paths.insert(euicc.GetString());
+  }
+  return euicc_paths;
+}
+
+void CellularESimProfileHandlerImpl::OnAutoRefreshEuiccComplete(
+    const std::string& path,
+    std::unique_ptr<CellularInhibitor::InhibitLock> inhibit_lock) {
+  paths_pending_auto_refresh_.erase(path);
+
+  if (!inhibit_lock) {
+    NET_LOG(ERROR) << "Auto-refresh failed due to inhibit error. Path: "
+                   << path;
+    return;
   }
-  return euicc_paths_from_prefs_set;
+
+  NET_LOG(EVENT) << "Finished auto-refresh for path: " << path;
+  AddNewlyRefreshedEuiccPathToPrefs(path);
 }
 
-void CellularESimProfileHandlerImpl::StoreEuiccPathsToPrefs(
-    const base::flat_set<std::string>& paths) {
+void CellularESimProfileHandlerImpl::AddNewlyRefreshedEuiccPathToPrefs(
+    const std::string& new_path) {
   DCHECK(device_prefs_);
 
+  base::flat_set<std::string> auto_refreshed_euicc_paths =
+      GetAutoRefreshedEuiccPathsFromPrefs();
+
+  // Keep all paths which were already in prefs.
   base::Value euicc_paths(base::Value::Type::LIST);
-  for (const auto& path : paths)
+  for (const auto& path : auto_refreshed_euicc_paths)
     euicc_paths.Append(path);
 
+  // Add new path.
+  euicc_paths.Append(new_path);
+
   device_prefs_->Set(prefs::kESimRefreshedEuiccs, std::move(euicc_paths));
 }
 
@@ -219,14 +272,21 @@ void CellularESimProfileHandlerImpl::UpdateProfilesFromHermes() {
   if (profiles_from_hermes == profiles_before_fetch)
     return;
 
-  NET_LOG(EVENT) << "New set of eSIM profiles have been fetched from Hermes";
+  std::stringstream ss;
+  ss << "New set of eSIM profiles have been fetched from Hermes: ";
 
   // Store the updated list of profiles in prefs.
   base::Value list(base::Value::Type::LIST);
-  for (const auto& profile : profiles_from_hermes)
+  for (const auto& profile : profiles_from_hermes) {
     list.Append(profile.ToDictionaryValue());
+    ss << "{iccid: " << profile.iccid() << ", eid: " << profile.eid() << "}, ";
+  }
   device_prefs_->Set(prefs::kESimProfiles, std::move(list));
 
+  if (profiles_from_hermes.empty())
+    ss << "<empty>";
+  NET_LOG(EVENT) << ss.str();
+
   network_state_handler()->SyncStubCellularNetworks();
   NotifyESimProfileListUpdated();
 }

chromeos/network/cellular_esim_profile_handler_impl.h

@@ -7,6 +7,7 @@
 
 #include "base/component_export.h"
 #include "base/containers/flat_set.h"
+#include "base/memory/weak_ptr.h"
 #include "chromeos/network/cellular_esim_profile_handler.h"
 #include "chromeos/network/network_state_handler_observer.h"
 
@@ -15,6 +16,21 @@ class PrefRegistrySimple;
 
 namespace chromeos {
 
+// CellularESimProfileHandler implementation which utilizes the local state
+// PrefService to track eSIM profiles.
+//
+// eSIM profiles can only be retrieved from the device hardware when an EUICC is
+// the "active" slot on the device, and only one slot can be active at a time.
+// This means that if the physical SIM slot is active, we cannot fetch an
+// updated list of profiles without switching slots, which can be disruptive if
+// the user is utilizing a cellular connection from the physical SIM slot. To
+// ensure that clients can access eSIM metadata regardless of the active slot,
+// this class stores all known eSIM profiles persistently in prefs.
+//
+// Additionally, this class tracks all known EUICC paths. If it detects a new
+// EUICC which it previously had not known about, it automatically refreshes
+// profile metadata from that slot. This ensures that after a powerwash, we
+// still expose information about installed profiles.
 class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularESimProfileHandlerImpl
     : public CellularESimProfileHandler,
       public NetworkStateHandlerObserver {
@@ -38,13 +54,21 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularESimProfileHandlerImpl
   // CellularESimProfileHandler:
   void InitInternal() override;
   std::vector<CellularESimProfile> GetESimProfiles() override;
+  bool HasRefreshedProfilesForEuicc(const std::string& eid) override;
   void SetDevicePrefs(PrefService* device_prefs) override;
   void OnHermesPropertiesUpdated() override;
 
-  void RefreshEuiccsIfNecessary();
-  base::flat_set<std::string> GetEuiccPathsFromPrefs() const;
-  void StoreEuiccPathsToPrefs(const base::flat_set<std::string>& paths);
+  void AutoRefreshEuiccsIfNecessary();
+  void StartAutoRefresh(const base::flat_set<std::string>& euicc_paths);
+  base::flat_set<std::string> GetAutoRefreshedEuiccPaths() const;
+  base::flat_set<std::string> GetAutoRefreshedEuiccPathsFromPrefs() const;
+  void OnAutoRefreshEuiccComplete(
+      const std::string& path,
+      std::unique_ptr<CellularInhibitor::InhibitLock> inhibit_lock);
+  void AddNewlyRefreshedEuiccPathToPrefs(const std::string& path);
+
   void UpdateProfilesFromHermes();
+
   bool CellularDeviceExists() const;
 
   // Used by chrome://network debug page; not meant to be called during normal
@@ -53,6 +77,10 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularESimProfileHandlerImpl
 
   // Initialized to null and set once SetDevicePrefs() is called.
   PrefService* device_prefs_ = nullptr;
+
+  base::flat_set<std::string> paths_pending_auto_refresh_;
+
+  base::WeakPtrFactory<CellularESimProfileHandlerImpl> weak_ptr_factory_{this};
 };
 
 }  // namespace chromeos

chromeos/network/cellular_esim_profile_handler_impl_unittest.cc

@@ -148,6 +148,10 @@ class CellularESimProfileHandlerImplTest : public testing::Test {
     return handler_->GetESimProfiles();
   }
 
+  bool HasAutoRefreshedEuicc(int euicc_num) {
+    return handler_->HasRefreshedProfilesForEuicc(CreateTestEid(euicc_num));
+  }
+
   size_t NumObserverEvents() const { return observer_.num_updates(); }
 
   std::unique_ptr<CellularInhibitor::InhibitLock> InhibitCellularScanning() {
@@ -450,12 +454,21 @@ TEST_F(CellularESimProfileHandlerImplTest,
   EXPECT_TRUE(euicc_paths_from_prefs.is_list());
   EXPECT_TRUE(euicc_paths_from_prefs.GetList().empty());
 
+  // Set device prefs; a new auto-refresh should have started but not yet
+  // completed.
   SetDevicePrefs();
   euicc_paths_from_prefs = GetEuiccListFromPrefs();
   EXPECT_TRUE(euicc_paths_from_prefs.is_list());
+  EXPECT_TRUE(euicc_paths_from_prefs.GetList().empty());
+  EXPECT_FALSE(HasAutoRefreshedEuicc(/*euicc_num=*/1));
+
+  base::RunLoop().RunUntilIdle();
+  euicc_paths_from_prefs = GetEuiccListFromPrefs();
+  EXPECT_TRUE(euicc_paths_from_prefs.is_list());
   EXPECT_EQ(1u, euicc_paths_from_prefs.GetList().size());
   EXPECT_EQ(CreateTestEuiccPath(/*euicc_num=*/1),
             euicc_paths_from_prefs.GetList()[0].GetString());
+  EXPECT_TRUE(HasAutoRefreshedEuicc(/*euicc_num=*/1));
 }
 
 TEST_F(CellularESimProfileHandlerImplTest, IgnoresESimProfilesWithNoIccid) {

chromeos/network/test_cellular_esim_profile_handler.cc

@@ -13,11 +13,27 @@ TestCellularESimProfileHandler::TestCellularESimProfileHandler() = default;
 
 TestCellularESimProfileHandler::~TestCellularESimProfileHandler() = default;
 
+void TestCellularESimProfileHandler::SetHasRefreshedProfilesForEuicc(
+    const std::string& eid,
+    bool has_refreshed) {
+  if (has_refreshed) {
+    refreshed_eids_.insert(eid);
+    return;
+  }
+
+  refreshed_eids_.erase(eid);
+}
+
 std::vector<CellularESimProfile>
 TestCellularESimProfileHandler::GetESimProfiles() {
   return esim_profile_states_;
 }
 
+bool TestCellularESimProfileHandler::HasRefreshedProfilesForEuicc(
+    const std::string& eid) {
+  return base::Contains(refreshed_eids_, eid);
+}
+
 void TestCellularESimProfileHandler::SetDevicePrefs(PrefService* device_prefs) {
 }
 

chromeos/network/test_cellular_esim_profile_handler.h

@@ -5,6 +5,9 @@
 #ifndef CHROMEOS_NETWORK_TEST_CELLULAR_ESIM_PROFILE_HANDLER_H_
 #define CHROMEOS_NETWORK_TEST_CELLULAR_ESIM_PROFILE_HANDLER_H_
 
+#include <string>
+
+#include "base/containers/flat_set.h"
 #include "chromeos/network/cellular_esim_profile_handler.h"
 
 namespace chromeos {
@@ -16,13 +19,18 @@ class TestCellularESimProfileHandler : public CellularESimProfileHandler {
   TestCellularESimProfileHandler();
   ~TestCellularESimProfileHandler() override;
 
+  void SetHasRefreshedProfilesForEuicc(const std::string& eid,
+                                       bool has_refreshed);
+
   // CellularESimProfileHandler:
   std::vector<CellularESimProfile> GetESimProfiles() override;
+  bool HasRefreshedProfilesForEuicc(const std::string& eid) override;
   void SetDevicePrefs(PrefService* device_prefs) override;
   void OnHermesPropertiesUpdated() override;
 
  private:
   std::vector<CellularESimProfile> esim_profile_states_;
+  base::flat_set<std::string> refreshed_eids_;
 };
 
 }  // namespace chromeos