[CrOS Cellular] Open SIM unlock settings on notification click

When a connection attempt fails due to a locked SIM, a notification is shown. This CL updates the notification logic such that when this notification is clicked, SIM unlock settings are displayed. (cherry picked from commit 8022659) Fixed: 1197374 Change-Id: I45580d00d7096af0bc40e252c7115c71153efbb1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2816451 Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Reviewed-by: Azeem Arshad <azeemarshad@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#871359} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2820657 Auto-Submit: Kyle Horimoto <khorimoto@chromium.org> Reviewed-by: Regan Hsu <hsuregan@chromium.org> Commit-Queue: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/branch-heads/4472@{#18} Cr-Branched-From: 3d60439-refs/heads/master@{#870763}
chromium · Apr 12, 2021 · efe7feb · efe7feb
1 parent 2a4ba6d
commit efe7feb
Show file tree

Hide file tree

Showing 8 changed files with 142 additions and 28 deletions.
diff --git a/ash/public/cpp/test/test_system_tray_client.cc b/ash/public/cpp/test/test_system_tray_client.cc
@@ -73,7 +73,9 @@ void TestSystemTrayClient::ShowNetworkCreate(const std::string& type) {}
 
 void TestSystemTrayClient::ShowSettingsCellularSetup(bool show_psim_flow) {}
 
-void TestSystemTrayClient::ShowSettingsSimUnlock() {}
+void TestSystemTrayClient::ShowSettingsSimUnlock() {
+  ++show_sim_unlock_settings_count_;
+}
 
 void TestSystemTrayClient::ShowThirdPartyVpnCreate(
     const std::string& extension_id) {}

diff --git a/ash/public/cpp/test/test_system_tray_client.h b/ash/public/cpp/test/test_system_tray_client.h
@@ -72,12 +72,17 @@ class ASH_PUBLIC_EXPORT TestSystemTrayClient : public SystemTrayClient {
     return show_wifi_sync_settings_count_;
   }
 
+  int show_sim_unlock_settings_count() const {
+    return show_sim_unlock_settings_count_;
+  }
+
  private:
   int show_bluetooth_settings_count_ = 0;
   int show_multi_device_setup_count_ = 0;
   int show_connected_devices_settings_count_ = 0;
   int show_os_settings_privacy_and_security_count_ = 0;
   int show_wifi_sync_settings_count_ = 0;
+  int show_sim_unlock_settings_count_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(TestSystemTrayClient);
 };

diff --git a/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc b/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc
@@ -180,6 +180,8 @@ void ChromeBrowserMainExtraPartsAsh::PreProfileInit() {
   session_controller_client_->Init();
 
   system_tray_client_ = std::make_unique<SystemTrayClient>();
+  network_connect_delegate_->SetSystemTrayClient(system_tray_client_.get());
+
   tablet_mode_page_behavior_ = std::make_unique<TabletModePageBehavior>();
   vpn_list_forwarder_ = std::make_unique<VpnListForwarder>();
 

diff --git a/chrome/browser/ui/ash/network/network_connect_delegate_chromeos.cc b/chrome/browser/ui/ash/network/network_connect_delegate_chromeos.cc
@@ -72,3 +72,8 @@ void NetworkConnectDelegateChromeOS::ShowMobileActivationError(
     const std::string& network_id) {
   network_state_notifier_->ShowMobileActivationErrorForGuid(network_id);
 }
+
+void NetworkConnectDelegateChromeOS::SetSystemTrayClient(
+    ash::SystemTrayClient* system_tray_client) {
+  network_state_notifier_->set_system_tray_client(system_tray_client);
+}
diff --git a/chrome/browser/ui/ash/network/network_connect_delegate_chromeos.h b/chrome/browser/ui/ash/network/network_connect_delegate_chromeos.h
@@ -11,6 +11,10 @@
 #include "base/macros.h"
 #include "chromeos/network/network_connect.h"
 
+namespace ash {
+class SystemTrayClient;
+}  // namespace ash
+
 namespace chromeos {
 class NetworkStateNotifier;
 }
@@ -30,6 +34,8 @@ class NetworkConnectDelegateChromeOS
                                const std::string& network_id) override;
   void ShowMobileActivationError(const std::string& network_id) override;
 
+  void SetSystemTrayClient(ash::SystemTrayClient* system_tray_client);
+
  private:
   std::unique_ptr<chromeos::NetworkStateNotifier> network_state_notifier_;
 

diff --git a/chrome/browser/ui/ash/network/network_state_notifier.cc b/chrome/browser/ui/ash/network/network_state_notifier.cc
@@ -36,6 +36,9 @@ const int kMinTimeBetweenOutOfCreditsNotifySeconds = 10 * 60;
 const char kNotifierNetwork[] = "ash.network";
 const char kNotifierNetworkError[] = "ash.network.error";
 
+// TODO(b:184776317): Use string from service_constants.h
+const char kErrorSimLocked[] = "sim-locked";
+
 // Ignore in-progress error.
 bool ShillErrorIsIgnored(const std::string& shill_error) {
   if (shill_error == shill::kErrorResultInProgress)
@@ -136,6 +139,14 @@ const NetworkState* GetNetworkStateForGuid(const std::string& guid) {
                             ->GetNetworkStateFromGuid(guid);
 }
 
+bool IsSimLockConnectionFailure(const std::string& connection_error_name,
+                                const NetworkState* network_state) {
+  if (connection_error_name == NetworkConnectionHandler::kErrorSimLocked)
+    return true;
+
+  return network_state && network_state->GetError() == kErrorSimLocked;
+}
+
 }  // namespace
 
 const char NetworkStateNotifier::kNetworkConnectNotificationId[] =
@@ -520,11 +531,19 @@ void NetworkStateNotifier::ShowConnectErrorNotification(
   std::string network_type =
       GetStringFromDictionary(shill_properties, shill::kTypeProperty);
 
+  base::RepeatingClosure on_click;
+  if (IsSimLockConnectionFailure(error_name, network)) {
+    on_click = base::BindRepeating(&NetworkStateNotifier::ShowSimUnlockSettings,
+                                   weak_ptr_factory_.GetWeakPtr());
+  } else {
+    on_click = base::BindRepeating(&NetworkStateNotifier::ShowNetworkSettings,
+                                   weak_ptr_factory_.GetWeakPtr(), guid);
+  }
+
   ShowErrorNotification(
       NetworkPathId(service_path), kNetworkConnectNotificationId, network_type,
       l10n_util::GetStringUTF16(IDS_NETWORK_CONNECTION_ERROR_TITLE), error_msg,
-      base::BindRepeating(&NetworkStateNotifier::ShowNetworkSettings,
-                          weak_ptr_factory_.GetWeakPtr(), guid));
+      std::move(on_click));
 }
 
 void NetworkStateNotifier::ShowVpnDisconnectedNotification(VpnDetails* vpn) {
@@ -540,7 +559,7 @@ void NetworkStateNotifier::ShowVpnDisconnectedNotification(VpnDetails* vpn) {
 }
 
 void NetworkStateNotifier::ShowNetworkSettings(const std::string& network_id) {
-  if (!SystemTrayClient::Get())
+  if (!system_tray_client_)
     return;
   const NetworkState* network = GetNetworkStateForGuid(network_id);
   if (!network)
@@ -553,12 +572,20 @@ void NetworkStateNotifier::ShowNetworkSettings(const std::string& network_id) {
   if (!NetworkTypePattern::Primitive(network->type())
            .MatchesPattern(NetworkTypePattern::Mobile()) &&
       shill_error::IsConfigurationError(error)) {
-    SystemTrayClient::Get()->ShowNetworkConfigure(network_id);
+    system_tray_client_->ShowNetworkConfigure(network_id);
   } else {
-    SystemTrayClient::Get()->ShowNetworkSettings(network_id);
+    system_tray_client_->ShowNetworkSettings(network_id);
   }
 }
 
+void NetworkStateNotifier::ShowSimUnlockSettings() {
+  if (!system_tray_client_)
+    return;
+
+  NET_LOG(USER) << "Opening SIM unlock settings";
+  system_tray_client_->ShowSettingsSimUnlock();
+}
+
 void NetworkStateNotifier::ShowCarrierAccountDetail(
     const std::string& network_id) {
   NetworkConnect::Get()->ShowCarrierAccountDetail(network_id);

diff --git a/chrome/browser/ui/ash/network/network_state_notifier.h b/chrome/browser/ui/ash/network/network_state_notifier.h
@@ -19,6 +19,10 @@
 #include "chromeos/network/network_connection_observer.h"
 #include "chromeos/network/network_state_handler_observer.h"
 
+namespace ash {
+class SystemTrayClient;
+}  // namespace ash
+
 namespace chromeos {
 
 class NetworkState;
@@ -48,11 +52,17 @@ class NetworkStateNotifier : public NetworkConnectionObserver,
   // Show a mobile activation error notification.
   void ShowMobileActivationErrorForGuid(const std::string& guid);
 
+  void set_system_tray_client(ash::SystemTrayClient* system_tray_client) {
+    system_tray_client_ = system_tray_client;
+  }
+
   static const char kNetworkConnectNotificationId[];
   static const char kNetworkActivateNotificationId[];
   static const char kNetworkOutOfCreditsNotificationId[];
 
  private:
+  friend class NetworkStateNotifierTest;
+
   struct VpnDetails {
     VpnDetails(const std::string& guid, const std::string& name)
         : guid(guid), name(name) {}
@@ -97,10 +107,13 @@ class NetworkStateNotifier : public NetworkConnectionObserver,
 
   // Shows the network settings for |network_id|.
   void ShowNetworkSettings(const std::string& network_id);
+  void ShowSimUnlockSettings();
 
   // Shows the carrier account detail page for |network_id|.
   void ShowCarrierAccountDetail(const std::string& network_id);
 
+  ash::SystemTrayClient* system_tray_client_ = nullptr;
+
   // The details of the connected VPN network if any, otherwise null.
   // Used for displaying the VPN disconnected notification.
   std::unique_ptr<VpnDetails> connected_vpn_;

diff --git a/chrome/browser/ui/ash/network/network_state_notifier_unittest.cc b/chrome/browser/ui/ash/network/network_state_notifier_unittest.cc
@@ -6,7 +6,9 @@
 
 #include <memory>
 
+#include "ash/public/cpp/test/test_system_tray_client.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "chrome/browser/notifications/notification_display_service_tester.h"
 #include "chrome/browser/notifications/system_notification_helper.h"
@@ -22,20 +24,17 @@
 #include "ui/message_center/public/cpp/notification.h"
 
 namespace chromeos {
-namespace test {
-
 namespace {
 
-const char kWiFi1ServicePath[] = "/service/wifi1";
 const char kWiFi1Guid[] = "wifi1_guid";
-
-}  // namespace
+const char kCellular1Guid[] = "cellular1_guid";
 
 class NetworkConnectTestDelegate : public NetworkConnect::Delegate {
  public:
-  NetworkConnectTestDelegate()
-      : network_state_notifier_(new NetworkStateNotifier()) {}
-  ~NetworkConnectTestDelegate() override {}
+  NetworkConnectTestDelegate(
+      std::unique_ptr<NetworkStateNotifier> network_state_notifier)
+      : network_state_notifier_(std::move(network_state_notifier)) {}
+  ~NetworkConnectTestDelegate() override = default;
 
   // NetworkConnect::Delegate
   void ShowNetworkConfigure(const std::string& network_id) override {}
@@ -58,18 +57,26 @@ class NetworkConnectTestDelegate : public NetworkConnect::Delegate {
   DISALLOW_COPY_AND_ASSIGN(NetworkConnectTestDelegate);
 };
 
+}  // namespace
+
 class NetworkStateNotifierTest : public BrowserWithTestWindowTest {
  public:
-  NetworkStateNotifierTest() {}
-  ~NetworkStateNotifierTest() override {}
+  NetworkStateNotifierTest() = default;
+  ~NetworkStateNotifierTest() override = default;
 
   void SetUp() override {
     BrowserWithTestWindowTest::SetUp();
     shill_clients::InitializeFakes();
     SetupDefaultShillState();
     NetworkHandler::Initialize();
     base::RunLoop().RunUntilIdle();
-    network_connect_delegate_ = std::make_unique<NetworkConnectTestDelegate>();
+
+    auto notifier = std::make_unique<NetworkStateNotifier>();
+    notifier->set_system_tray_client(&test_system_tray_client_);
+
+    network_connect_delegate_ =
+        std::make_unique<NetworkConnectTestDelegate>(std::move(notifier));
+
     NetworkConnect::Initialize(network_connect_delegate_.get());
   }
 
@@ -83,40 +90,69 @@ class NetworkStateNotifierTest : public BrowserWithTestWindowTest {
 
  protected:
   void SetupDefaultShillState() {
-    base::RunLoop().RunUntilIdle();
     ShillDeviceClient::TestInterface* device_test =
         ShillDeviceClient::Get()->GetTestInterface();
     device_test->ClearDevices();
-    device_test->AddDevice("/device/stub_wifi_device1", shill::kTypeWifi,
-                           "stub_wifi_device1");
-    device_test->AddDevice("/device/stub_cellular_device1",
-                           shill::kTypeCellular, "stub_cellular_device1");
 
     ShillServiceClient::TestInterface* service_test =
         ShillServiceClient::Get()->GetTestInterface();
     service_test->ClearServices();
-    const bool add_to_visible = true;
-    // Create a wifi network and set to online.
+
+    // Set up Wi-Fi device, and add a single network with a passphrase failure.
+    const char kWiFi1ServicePath[] = "/service/wifi1";
+    device_test->AddDevice("/device/stub_wifi_device1", shill::kTypeWifi,
+                           "stub_wifi_device1");
     service_test->AddService(kWiFi1ServicePath, kWiFi1Guid, "wifi1",
-                             shill::kTypeWifi, shill::kStateIdle,
-                             add_to_visible);
+                             shill::kTypeWifi, shill::kStateIdle, true);
     service_test->SetServiceProperty(kWiFi1ServicePath,
                                      shill::kSecurityClassProperty,
                                      base::Value(shill::kSecurityWep));
     service_test->SetServiceProperty(
         kWiFi1ServicePath, shill::kConnectableProperty, base::Value(true));
     service_test->SetServiceProperty(
         kWiFi1ServicePath, shill::kPassphraseProperty, base::Value("failure"));
+
+    // Set up Cellular device, and add a single locked network.
+    const char kCellularDevicePath[] = "/device/cellular1";
+    const char kCellular1ServicePath[] = "/service/cellular1";
+    const char kCellular1Iccid[] = "iccid";
+    device_test->AddDevice(kCellularDevicePath, shill::kTypeCellular,
+                           "stub_cellular_device1");
+    service_test->AddService(kCellular1ServicePath, kCellular1Guid, "cellular1",
+                             shill::kTypeCellular, shill::kStateIdle, true);
+    service_test->SetServiceProperty(kCellular1ServicePath,
+                                     shill::kIccidProperty,
+                                     base::Value(kCellular1Iccid));
+    service_test->SetServiceProperty(
+        kCellular1ServicePath, shill::kActivationStateProperty,
+        base::Value(shill::kActivationStateActivated));
+    base::Value sim_lock_status(base::Value::Type::DICTIONARY);
+    sim_lock_status.SetKey(shill::kSIMLockTypeProperty,
+                           base::Value(shill::kSIMLockPin));
+    device_test->SetDeviceProperty(
+        kCellularDevicePath, shill::kSIMLockStatusProperty,
+        std::move(sim_lock_status), /*notify_changed=*/true);
+    base::Value::ListStorage sim_slot_infos;
+    base::Value slot_info_item(base::Value::Type::DICTIONARY);
+    slot_info_item.SetKey(shill::kSIMSlotInfoICCID,
+                          base::Value(kCellular1Iccid));
+    slot_info_item.SetBoolKey(shill::kSIMSlotInfoPrimary, true);
+    sim_slot_infos.push_back(std::move(slot_info_item));
+    device_test->SetDeviceProperty(
+        kCellularDevicePath, shill::kSIMSlotInfoProperty,
+        base::Value(sim_slot_infos), /*notify_changed=*/true);
+
     base::RunLoop().RunUntilIdle();
   }
 
+  ash::TestSystemTrayClient test_system_tray_client_;
   std::unique_ptr<NetworkConnectTestDelegate> network_connect_delegate_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NetworkStateNotifierTest);
 };
 
-TEST_F(NetworkStateNotifierTest, ConnectionFailure) {
+TEST_F(NetworkStateNotifierTest, WiFiConnectionFailure) {
   TestingBrowserProcess::GetGlobal()->SetSystemNotificationHelper(
       std::make_unique<SystemNotificationHelper>());
   NotificationDisplayServiceTester tester(nullptr /* profile */);
@@ -127,5 +163,23 @@ TEST_F(NetworkStateNotifierTest, ConnectionFailure) {
       NetworkStateNotifier::kNetworkConnectNotificationId));
 }
 
-}  // namespace test
+TEST_F(NetworkStateNotifierTest, CellularLockedSimConnectionFailure) {
+  TestingBrowserProcess::GetGlobal()->SetSystemNotificationHelper(
+      std::make_unique<SystemNotificationHelper>());
+  NotificationDisplayServiceTester tester(nullptr /* profile */);
+  NetworkConnect::Get()->ConnectToNetworkId(kCellular1Guid);
+  base::RunLoop().RunUntilIdle();
+
+  // Failure should spawn a notification.
+  base::Optional<message_center::Notification> notification =
+      tester.GetNotification(
+          NetworkStateNotifier::kNetworkConnectNotificationId);
+  EXPECT_TRUE(notification);
+
+  // Clicking the notification should open SIM unlock settings.
+  notification->delegate()->Click(/*button_index=*/base::nullopt,
+                                  /*reply=*/base::nullopt);
+  EXPECT_EQ(1, test_system_tray_client_.show_sim_unlock_settings_count());
+}
+
 }  // namespace chromeos