[CrOSCellular] Fix non-active eSIM profiles after powerwash.

This CL fixes an issue with non-active eSIM profiles becoming unmanaged
after a powerwash. This issue was caused because after powerwash, Shill
is only aware of active eSIM profiles and has configuration entries only
for this profile. Policy applicator is unable to match ONC configs by
ICCID since it only looks at configuration entries. This causes such
configs to attempt installation but this never succeeds.

Fixed this by adding a check for existing eSIM profiles in
CellularPolicyHandler. If an existing profile is found, then we perform
the shill configuration step in CellularESimInstaller so that the
Shill service is configured correctly as a managed network.

This also adds additional wait steps in CellularPolicyHandler so
that installation waits for EUICC to become available and profile list
to be refreshed.

Bug: b/218874306
Change-Id: Ibc59bb7e73a4844bb46847bce727f63f22b4ea6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3466366
Reviewed-by: Jason Zhang <jiajunz@google.com>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Azeem Arshad <azeemarshad@chromium.org>
Cr-Commit-Position: refs/heads/main@{#972290}

chromeos/network/cellular_esim_installer.cc

@@ -195,68 +195,71 @@ void CellularESimInstaller::OnProfileInstallResult(
 
   RecordInstallESimProfileResult(InstallESimProfileResult::kSuccess, is_managed,
                                  is_initial_install);
+  pending_inhibit_locks_.emplace(*profile_path, std::move(inhibit_lock));
+  ConfigureESimService(
+      new_shill_properties, euicc_path, *profile_path,
+      base::BindOnce(&CellularESimInstaller::EnableProfile,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback),
+                     euicc_path, *profile_path));
+}
 
+void CellularESimInstaller::ConfigureESimService(
+    const base::Value& new_shill_properties,
+    const dbus::ObjectPath& euicc_path,
+    const dbus::ObjectPath& profile_path,
+    ConfigureESimServiceCallback callback) {
   const NetworkProfile* profile =
       network_profile_handler_->GetProfileForUserhash(
           /*userhash=*/std::string());
   if (!profile) {
     NET_LOG(ERROR)
-        << "Error installing eSIM profile. Default profile not initialized.";
-    std::move(callback).Run(HermesResponseStatus::kErrorUnknown,
-                            /*profile_path=*/absl::nullopt,
-                            /*service_path=*/absl::nullopt);
+        << "Error configuring eSIM profile. Default profile not initialized.";
+    std::move(callback).Run(
+        /*service_path=*/absl::nullopt);
     return;
   }
 
   base::Value properties_to_set = new_shill_properties.Clone();
-  AppendRequiredCellularProperties(euicc_path, *profile_path, profile,
+  AppendRequiredCellularProperties(euicc_path, profile_path, profile,
                                    &properties_to_set);
-  NET_LOG(EVENT) << "Creating shill configuration for newly installed eSim "
-                    "profile with properties: "
-                 << properties_to_set;
+  NET_LOG(EVENT)
+      << "Creating shill configuration for eSim profile with properties: "
+      << properties_to_set;
 
-  pending_inhibit_locks_.emplace(*profile_path, std::move(inhibit_lock));
   auto callback_split = base::SplitOnceCallback(std::move(callback));
   ShillManagerClient::Get()->ConfigureServiceForProfile(
       dbus::ObjectPath(profile->path), properties_to_set,
       base::BindOnce(
           &CellularESimInstaller::OnShillConfigurationCreationSuccess,
-          weak_ptr_factory_.GetWeakPtr(), std::move(callback_split.first),
-          euicc_path, *profile_path),
+          weak_ptr_factory_.GetWeakPtr(), std::move(callback_split.first)),
       base::BindOnce(
           &CellularESimInstaller::OnShillConfigurationCreationFailure,
-          weak_ptr_factory_.GetWeakPtr(), std::move(callback_split.second),
-          euicc_path, *profile_path));
+          weak_ptr_factory_.GetWeakPtr(), std::move(callback_split.second)));
 }
 
 void CellularESimInstaller::OnShillConfigurationCreationSuccess(
-    InstallProfileFromActivationCodeCallback callback,
-    const dbus::ObjectPath& euicc_path,
-    const dbus::ObjectPath& profile_path,
+    ConfigureESimServiceCallback callback,
     const dbus::ObjectPath& service_path) {
   NET_LOG(EVENT)
       << "Successfully creating shill configuration on service path: "
       << service_path.value();
-
-  EnableProfile(std::move(callback), euicc_path, profile_path);
+  std::move(callback).Run(service_path);
 }
 
 void CellularESimInstaller::OnShillConfigurationCreationFailure(
-    InstallProfileFromActivationCodeCallback callback,
-    const dbus::ObjectPath& euicc_path,
-    const dbus::ObjectPath& profile_path,
+    ConfigureESimServiceCallback callback,
     const std::string& error_name,
     const std::string& error_message) {
   NET_LOG(ERROR) << "Create shill configuration failed, error:" << error_name
                  << ", message: " << error_message;
-
-  EnableProfile(std::move(callback), euicc_path, profile_path);
+  std::move(callback).Run(/*service_path=*/absl::nullopt);
 }
 
 void CellularESimInstaller::EnableProfile(
     InstallProfileFromActivationCodeCallback callback,
     const dbus::ObjectPath& euicc_path,
-    const dbus::ObjectPath& profile_path) {
+    const dbus::ObjectPath& profile_path,
+    absl::optional<dbus::ObjectPath> service_path) {
   auto it = pending_inhibit_locks_.find(profile_path);
   DCHECK(it != pending_inhibit_locks_.end());
 

chromeos/network/cellular_esim_installer.h

@@ -11,6 +11,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/memory/weak_ptr.h"
 #include "chromeos/dbus/hermes/hermes_response_status.h"
+#include "chromeos/network/cellular_esim_profile_handler.h"
 #include "chromeos/network/cellular_inhibitor.h"
 
 namespace dbus {
@@ -56,6 +57,12 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularESimInstaller {
                               absl::optional<dbus::ObjectPath> profile_path,
                               absl::optional<std::string> service_path)>;
 
+  // Return callback for the ConfigureESimService method. |service_path|
+  // is the path of the newly configured eSIM service. A nullopt |service_path|
+  // indicates failure.
+  using ConfigureESimServiceCallback =
+      base::OnceCallback<void(absl::optional<dbus::ObjectPath> service_path)>;
+
   // Installs an ESim profile and network with given |activation_code|,
   // |confirmation_code| and |euicc_path|. This method will attempt to create
   // the Shill configuration with given |new_shill_properties| and then enable
@@ -71,6 +78,14 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularESimInstaller {
       InstallProfileFromActivationCodeCallback callback,
       bool is_initial_install = true);
 
+  // Attempts to create a Shill service configuration with given
+  // |new_shill_properties| for eSIM with |profile_path| and |euicc_path|.
+  // |callback| is called with the newly configure service path.
+  void ConfigureESimService(const base::Value& new_shill_properties,
+                            const dbus::ObjectPath& euicc_path,
+                            const dbus::ObjectPath& profile_path,
+                            ConfigureESimServiceCallback callback);
+
  private:
   friend class CellularESimInstallerTest;
   friend class CellularPolicyHandlerTest;
@@ -119,19 +134,16 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularESimInstaller {
       HermesResponseStatus status,
       const dbus::ObjectPath* object_path);
   void OnShillConfigurationCreationSuccess(
-      InstallProfileFromActivationCodeCallback callback,
-      const dbus::ObjectPath& euicc_path,
-      const dbus::ObjectPath& profile_path,
+      ConfigureESimServiceCallback callback,
       const dbus::ObjectPath& service_path);
   void OnShillConfigurationCreationFailure(
-      InstallProfileFromActivationCodeCallback callback,
-      const dbus::ObjectPath& euicc_path,
-      const dbus::ObjectPath& profile_path,
+      ConfigureESimServiceCallback callback,
       const std::string& error_name,
       const std::string& error_message);
   void EnableProfile(InstallProfileFromActivationCodeCallback callback,
                      const dbus::ObjectPath& euicc_path,
-                     const dbus::ObjectPath& profile_path);
+                     const dbus::ObjectPath& profile_path,
+                     absl::optional<dbus::ObjectPath> service_path);
   void OnPrepareCellularNetworkForConnectionSuccess(
       const dbus::ObjectPath& profile_path,
       InstallProfileFromActivationCodeCallback callback,
@@ -154,8 +166,8 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularESimInstaller {
   NetworkProfileHandler* network_profile_handler_;
   NetworkStateHandler* network_state_handler_;
 
-  // Maps profile dbus paths to unique pointer of InhibitLocks that are pending
-  // to uninhibit.
+  // Maps profile dbus paths to unique pointer of InhibitLocks that are
+  // pending to uninhibit.
   std::map<dbus::ObjectPath, std::unique_ptr<CellularInhibitor::InhibitLock>>
       pending_inhibit_locks_;
 
@@ -164,4 +176,4 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularESimInstaller {
 
 }  // namespace chromeos
 
-#endif  // CHROMEOS_NETWORK_CELLULAR_ESIM_INSTALLER_H_
+#endif  // CHROMEOS_NETWORK_CELLULAR_ESIM_INSTALLER_H_

chromeos/network/cellular_esim_installer_unittest.cc

@@ -13,6 +13,7 @@
 #include "chromeos/dbus/hermes/hermes_clients.h"
 #include "chromeos/dbus/hermes/hermes_euicc_client.h"
 #include "chromeos/dbus/hermes/hermes_manager_client.h"
+#include "chromeos/dbus/hermes/hermes_profile_client.h"
 #include "chromeos/dbus/hermes/hermes_response_status.h"
 #include "chromeos/dbus/shill/fake_shill_manager_client.h"
 #include "chromeos/dbus/shill/shill_clients.h"
@@ -180,6 +181,23 @@ class CellularESimInstallerTest : public testing::Test {
                            out_service_path);
   }
 
+  absl::optional<dbus::ObjectPath> ConfigureESimService(
+      const dbus::ObjectPath euicc_path,
+      const dbus::ObjectPath& profile_path,
+      base::Value& new_shill_properties) {
+    absl::optional<dbus::ObjectPath> service_path_out;
+    base::RunLoop run_loop;
+    cellular_esim_installer_->ConfigureESimService(
+        new_shill_properties, euicc_path, profile_path,
+        base::BindLambdaForTesting(
+            [&](absl::optional<dbus::ObjectPath> service_path) {
+              service_path_out = service_path;
+              run_loop.Quit();
+            }));
+    run_loop.Run();
+    return service_path_out;
+  }
+
   void CheckInstallSuccess(const InstallResultTuple& actual_result_tuple) {
     EXPECT_EQ(HermesResponseStatus::kSuccess, std::get<0>(actual_result_tuple));
     EXPECT_NE(std::get<1>(actual_result_tuple), absl::nullopt);
@@ -451,4 +469,55 @@ TEST_F(CellularESimInstallerTest, InstallProfileCreateShillConfigFailure) {
   CheckInstallSuccess(result_tuple);
 }
 
+TEST_F(CellularESimInstallerTest, ConfigureESimService) {
+  dbus::ObjectPath profile_path =
+      HermesEuiccClient::Get()->GetTestInterface()->AddFakeCarrierProfile(
+          dbus::ObjectPath(kTestEuiccPath), hermes::profile::State::kInactive,
+          /*activation_code=*/"",
+          HermesEuiccClient::TestInterface::AddCarrierProfileBehavior::
+              kAddProfileWithoutService);
+
+  base::Value new_shill_properties(base::Value::Type::DICTIONARY);
+  std::unique_ptr<NetworkUIData> ui_data =
+      NetworkUIData::CreateFromONC(::onc::ONCSource::ONC_SOURCE_DEVICE_POLICY);
+  new_shill_properties.SetStringKey(shill::kUIDataProperty,
+                                    ui_data->GetAsJson());
+  absl::optional<dbus::ObjectPath> service_path = ConfigureESimService(
+      dbus::ObjectPath(kTestEuiccPath), profile_path, new_shill_properties);
+  EXPECT_TRUE(service_path.has_value());
+
+  HermesProfileClient::Properties* profile_properties =
+      HermesProfileClient::Get()->GetProperties(profile_path);
+  const base::Value* service_properties =
+      ShillServiceClient::Get()->GetTestInterface()->GetServiceProperties(
+          service_path->value());
+  ASSERT_TRUE(service_properties);
+  const std::string* type =
+      service_properties->FindStringKey(shill::kTypeProperty);
+  EXPECT_EQ(shill::kTypeCellular, *type);
+  const std::string* iccid =
+      service_properties->FindStringKey(shill::kIccidProperty);
+  EXPECT_EQ(profile_properties->iccid().value(), *iccid);
+  const std::string* eid =
+      service_properties->FindStringKey(shill::kEidProperty);
+  EXPECT_EQ(kTestEid, *eid);
+}
+
+TEST_F(CellularESimInstallerTest, ConfigureESimServiceFailure) {
+  dbus::ObjectPath profile_path =
+      HermesEuiccClient::Get()->GetTestInterface()->AddFakeCarrierProfile(
+          dbus::ObjectPath(kTestEuiccPath), hermes::profile::State::kInactive,
+          /*activation_code=*/"",
+          HermesEuiccClient::TestInterface::AddCarrierProfileBehavior::
+              kAddProfileWithoutService);
+
+  ShillManagerClient::Get()->GetTestInterface()->SetSimulateConfigurationResult(
+      FakeShillSimulatedResult::kFailure);
+
+  base::Value new_shill_properties(base::Value::Type::DICTIONARY);
+  absl::optional<dbus::ObjectPath> service_path = ConfigureESimService(
+      dbus::ObjectPath(kTestEuiccPath), profile_path, new_shill_properties);
+  EXPECT_FALSE(service_path.has_value());
+}
+
 }  // namespace chromeos

chromeos/network/cellular_esim_profile_handler.h

@@ -68,10 +68,12 @@ 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
+  // Returns whether profiles for the EUICC with the given EID or path 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 bool HasRefreshedProfilesForEuicc(
+      const dbus::ObjectPath& euicc_path) = 0;
 
   virtual void SetDevicePrefs(PrefService* device_prefs) = 0;
 

chromeos/network/cellular_esim_profile_handler_impl.cc

@@ -116,6 +116,19 @@ bool CellularESimProfileHandlerImpl::HasRefreshedProfilesForEuicc(
   return false;
 }
 
+bool CellularESimProfileHandlerImpl::HasRefreshedProfilesForEuicc(
+    const dbus::ObjectPath& euicc_path) {
+  base::flat_set<std::string> euicc_paths =
+      GetAutoRefreshedEuiccPathsFromPrefs();
+
+  for (const auto& path : euicc_paths) {
+    if (euicc_path.value() == path)
+      return true;
+  }
+
+  return false;
+}
+
 void CellularESimProfileHandlerImpl::SetDevicePrefs(PrefService* device_prefs) {
   device_prefs_ = device_prefs;
   OnHermesPropertiesUpdated();

chromeos/network/cellular_esim_profile_handler_impl.h

@@ -61,6 +61,8 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularESimProfileHandlerImpl
   void InitInternal() override;
   std::vector<CellularESimProfile> GetESimProfiles() override;
   bool HasRefreshedProfilesForEuicc(const std::string& eid) override;
+  bool HasRefreshedProfilesForEuicc(
+      const dbus::ObjectPath& euicc_path) override;
   void SetDevicePrefs(PrefService* device_prefs) override;
   void OnHermesPropertiesUpdated() override;
 

chromeos/network/cellular_esim_profile_handler_impl_unittest.cc

@@ -152,7 +152,11 @@ class CellularESimProfileHandlerImplTest : public testing::Test {
   }
 
   bool HasAutoRefreshedEuicc(int euicc_num) {
-    return handler_->HasRefreshedProfilesForEuicc(CreateTestEid(euicc_num));
+    // Check both variants of HasRefreshedProfilesForEuicc using EID and EUICC
+    // Path.
+    return handler_->HasRefreshedProfilesForEuicc(CreateTestEid(euicc_num)) &&
+           handler_->HasRefreshedProfilesForEuicc(
+               dbus::ObjectPath(CreateTestEuiccPath(euicc_num)));
   }
 
   void DisableActiveESimProfile() { handler_->DisableActiveESimProfile(); }

chromeos/network/cellular_esim_uninstall_handler_unittest.cc

@@ -187,7 +187,7 @@ class CellularESimUninstallHandlerTest : public testing::Test {
 
   void SetHasRefreshedProfiles(bool has_refreshed) {
     cellular_esim_profile_handler_->SetHasRefreshedProfilesForEuicc(
-        kDefaultEid, has_refreshed);
+        kDefaultEid, dbus::ObjectPath(kDefaultEuiccPath), has_refreshed);
   }
 
   void ExpectResult(CellularESimUninstallHandler::UninstallESimResult result,