Update network_state to handle new portal states

Shill's portal state is being split into three new states:
 - kStateNoConnectivity: no internet connectivity
 - kStateRedirectFound: shill is behind a portal and was able to find
   a redirect URL
 - kStatePortalSuspected: shill suspects that it is behind a portal
   but wasn't able to find a redirect URL
Since these states are communicated to Chrome via D-Bus, we need to
update Chrome to recognize and handle these new states.

Bug: 946866
Change-Id: Ifc933be0368aafb46d8d07d707489c70e3e924d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1586474
Commit-Queue: Matthew Wang <matthewmwang@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659315}

chrome/browser/chromeos/mobile/mobile_activator.cc

@@ -320,9 +320,10 @@ void MobileActivator::StartActivationOTA() {
   }
 
   const NetworkState* default_network = GetDefaultNetwork();
-  bool is_online_or_portal = default_network &&
+  bool is_online_or_portal =
+      default_network &&
       (default_network->connection_state() == shill::kStateOnline ||
-       default_network->connection_state() == shill::kStatePortal);
+       NetworkState::StateIsPortalled(default_network->connection_state()));
   if (!is_online_or_portal)
     ConnectNetwork(network);
 
@@ -454,7 +455,7 @@ void MobileActivator::ReconnectTimedOut() {
 void MobileActivator::ContinueConnecting() {
   const NetworkState* network = GetNetworkState(service_path_);
   if (network && network->IsConnectedState()) {
-    if (network->connection_state() == shill::kStatePortal &&
+    if (NetworkState::StateIsPortalled(network->connection_state()) &&
         network->error() == shill::kErrorDNSLookupFailed) {
       // It isn't an error to be in a restricted pool, but if DNS doesn't work,
       // then we're not getting traffic through at all.  Just disconnect and
@@ -496,10 +497,11 @@ void MobileActivator::RefreshCellularNetworks() {
     // used to route default traffic. Also, note that we can access the
     // payment portal over a cellular network in the portalled state.
     const NetworkState* default_network = GetDefaultNetwork();
-    bool is_online_or_portal = default_network &&
+    bool is_online_or_portal =
+        default_network &&
         (default_network->connection_state() == shill::kStateOnline ||
          (default_network->type() == shill::kTypeCellular &&
-          default_network->connection_state() == shill::kStatePortal));
+          NetworkState::StateIsPortalled(default_network->connection_state())));
     if (waiting && is_online_or_portal) {
       ChangeState(network, post_reconnect_state_, ActivationError::kNone);
     } else if (!waiting && !is_online_or_portal) {
@@ -612,7 +614,7 @@ MobileActivator::PlanActivationState MobileActivator::PickNextOfflineState(
       break;
     case PLAN_ACTIVATION_START:
       if (activation == shill::kActivationStateActivated) {
-        if (network->connection_state() == shill::kStatePortal)
+        if (NetworkState::StateIsPortalled(network->connection_state()))
           new_state = PLAN_ACTIVATION_PAYMENT_PORTAL_LOADING;
         else
           new_state = PLAN_ACTIVATION_DONE;
@@ -689,7 +691,7 @@ MobileActivator::PlanActivationState MobileActivator::PickNextOnlineState(
       }
       break;
     case PLAN_ACTIVATION_RECONNECTING_PAYMENT:
-      if (network->connection_state() != shill::kStatePortal &&
+      if (!NetworkState::StateIsPortalled(network->connection_state()) &&
           activation == shill::kActivationStateActivated)
         // We're not portalled, and we're already activated, so we're online!
         new_state = PLAN_ACTIVATION_DONE;

chrome/browser/chromeos/mobile/mobile_activator_unittest.cc

@@ -155,7 +155,7 @@ TEST_F(MobileActivatorTest, OTAHasNetworkConnection) {
       .WillRepeatedly(Return(&cellular_network_));
   EXPECT_CALL(mobile_activator_, ConnectNetwork(_))
       .Times(0);
-  set_connection_state(shill::kStatePortal);
+  set_connection_state(shill::kStateNoConnectivity);
   set_network_activation_type(shill::kActivationTypeOTA);
   set_network_activation_state(shill::kActivationStateNotActivated);
   mobile_activator_.InvokeStartActivation();
@@ -219,7 +219,7 @@ TEST_F(MobileActivatorTest, OTASPBasicFlowForNewDevices) {
   // We'll sit in this state until we go online as well.
   EXPECT_EQ(MobileActivator::PLAN_ACTIVATION_INITIATING_ACTIVATION,
             mobile_activator_.InvokePickNextState(&cellular_network_));
-  set_connection_state(shill::kStatePortal);
+  set_connection_state(shill::kStateNoConnectivity);
   // After we go online, we go back to START, which acts as a jumping off
   // point for the two types of initial OTASP.
   EXPECT_EQ(MobileActivator::PLAN_ACTIVATION_START,
@@ -233,7 +233,7 @@ TEST_F(MobileActivatorTest, OTASPBasicFlowForNewDevices) {
   EXPECT_EQ(MobileActivator::PLAN_ACTIVATION_TRYING_OTASP,
             mobile_activator_.InvokePickNextState(&cellular_network_));
   set_network_activation_state(shill::kActivationStatePartiallyActivated);
-  set_connection_state(shill::kStatePortal);
+  set_connection_state(shill::kStateNoConnectivity);
   // And when we come back online again and aren't activating, load the portal.
   EXPECT_EQ(MobileActivator::PLAN_ACTIVATION_PAYMENT_PORTAL_LOADING,
             mobile_activator_.InvokePickNextState(&cellular_network_));

chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc

@@ -93,7 +93,7 @@ class NetworkPortalDetectorImplBrowserTest
                              true /* add_to_visible */);
     DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
         dbus::ObjectPath(kWifiServicePath), shill::kStateProperty,
-        base::Value(shill::kStatePortal), base::DoNothing(),
+        base::Value(shill::kStateRedirectFound), base::DoNothing(),
         base::Bind(&ErrorCallbackFunction));
 
     display_service_ = std::make_unique<NotificationDisplayServiceTester>(

chrome/browser/chromeos/net/network_portal_detector_impl_unittest.cc

@@ -229,7 +229,7 @@ class NetworkPortalDetectorImplTest
   void SetBehindPortal(const std::string& service_path) {
     DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
         dbus::ObjectPath(service_path), shill::kStateProperty,
-        base::Value(shill::kStatePortal), base::DoNothing(),
+        base::Value(shill::kStateNoConnectivity), base::DoNothing(),
         base::Bind(&ErrorCallbackFunction));
     base::RunLoop().RunUntilIdle();
   }

chrome/browser/chromeos/policy/app_install_event_log_collector_unittest.cc

@@ -321,7 +321,8 @@ TEST_F(AppInstallEventLogCollectorTest, ConnectivityChanges) {
             delegate()->last_event().event_type());
   EXPECT_FALSE(delegate()->last_event().online());
 
-  SetNetworkState(collector.get(), kWifiServicePath, shill::kStatePortal);
+  SetNetworkState(collector.get(), kWifiServicePath,
+                  shill::kStateNoConnectivity);
   EXPECT_EQ(2, delegate()->add_for_all_count());
 
   SetNetworkState(collector.get(), kWifiServicePath, shill::kStateOnline);

chrome/browser/chromeos/policy/status_collector/device_status_collector.cc

@@ -1378,6 +1378,9 @@ bool DeviceStatusCollector::GetNetworkInterfaces(
       {shill::kStateConfiguration, em::NetworkState::CONFIGURATION},
       {shill::kStateReady, em::NetworkState::READY},
       {shill::kStatePortal, em::NetworkState::PORTAL},
+      {shill::kStateNoConnectivity, em::NetworkState::PORTAL},
+      {shill::kStateRedirectFound, em::NetworkState::PORTAL},
+      {shill::kStatePortalSuspected, em::NetworkState::PORTAL},
       {shill::kStateOffline, em::NetworkState::OFFLINE},
       {shill::kStateOnline, em::NetworkState::ONLINE},
       {shill::kStateDisconnect, em::NetworkState::DISCONNECT},

chrome/browser/chromeos/policy/status_collector/device_status_collector_browsertest.cc

@@ -1970,34 +1970,33 @@ struct FakeNetworkState {
 // by convention shill will not report a signal strength of 0 for a visible
 // network, so we use 1 below.
 static const FakeNetworkState kFakeNetworks[] = {
-  { "offline", "/device/wifi", shill::kTypeWifi, 35, -85,
-    shill::kStateOffline, em::NetworkState::OFFLINE, "", "" },
-  { "ethernet", "/device/ethernet", shill::kTypeEthernet, 0, 0,
-    shill::kStateOnline, em::NetworkState::ONLINE,
-    "192.168.0.1", "8.8.8.8" },
-  { "wifi", "/device/wifi", shill::kTypeWifi, 23, -97, shill::kStatePortal,
-    em::NetworkState::PORTAL, "", "" },
-  { "idle", "/device/cellular1", shill::kTypeCellular, 0, 0, shill::kStateIdle,
-    em::NetworkState::IDLE, "", "" },
-  { "carrier", "/device/cellular1", shill::kTypeCellular, 0, 0,
-    shill::kStateCarrier, em::NetworkState::CARRIER, "", "" },
-  { "association", "/device/cellular1", shill::kTypeCellular, 0, 0,
-    shill::kStateAssociation, em::NetworkState::ASSOCIATION, "", "" },
-  { "config", "/device/cellular1", shill::kTypeCellular, 0, 0,
-    shill::kStateConfiguration, em::NetworkState::CONFIGURATION, "", "" },
-  // Set signal strength for this network to -20, but expected strength to 0
-  // to test that we only report signal_strength for wifi connections.
-  { "ready", "/device/cellular1", shill::kTypeCellular, -20, 0,
-    shill::kStateReady, em::NetworkState::READY, "", "" },
-  { "disconnect", "/device/wifi", shill::kTypeWifi, 1, -119,
-    shill::kStateDisconnect, em::NetworkState::DISCONNECT, "", "" },
-  { "failure", "/device/wifi", shill::kTypeWifi, 1, -119, shill::kStateFailure,
-    em::NetworkState::FAILURE, "", "" },
-  { "activation-failure", "/device/cellular1", shill::kTypeCellular, 0, 0,
-    shill::kStateActivationFailure, em::NetworkState::ACTIVATION_FAILURE,
-    "", "" },
-  { "unknown", "", shill::kTypeWifi, 1, -119, "unknown",
-    em::NetworkState::UNKNOWN, "", "" },
+    {"offline", "/device/wifi", shill::kTypeWifi, 35, -85, shill::kStateOffline,
+     em::NetworkState::OFFLINE, "", ""},
+    {"ethernet", "/device/ethernet", shill::kTypeEthernet, 0, 0,
+     shill::kStateOnline, em::NetworkState::ONLINE, "192.168.0.1", "8.8.8.8"},
+    {"wifi", "/device/wifi", shill::kTypeWifi, 23, -97,
+     shill::kStateNoConnectivity, em::NetworkState::PORTAL, "", ""},
+    {"idle", "/device/cellular1", shill::kTypeCellular, 0, 0, shill::kStateIdle,
+     em::NetworkState::IDLE, "", ""},
+    {"carrier", "/device/cellular1", shill::kTypeCellular, 0, 0,
+     shill::kStateCarrier, em::NetworkState::CARRIER, "", ""},
+    {"association", "/device/cellular1", shill::kTypeCellular, 0, 0,
+     shill::kStateAssociation, em::NetworkState::ASSOCIATION, "", ""},
+    {"config", "/device/cellular1", shill::kTypeCellular, 0, 0,
+     shill::kStateConfiguration, em::NetworkState::CONFIGURATION, "", ""},
+    // Set signal strength for this network to -20, but expected strength to 0
+    // to test that we only report signal_strength for wifi connections.
+    {"ready", "/device/cellular1", shill::kTypeCellular, -20, 0,
+     shill::kStateReady, em::NetworkState::READY, "", ""},
+    {"disconnect", "/device/wifi", shill::kTypeWifi, 1, -119,
+     shill::kStateDisconnect, em::NetworkState::DISCONNECT, "", ""},
+    {"failure", "/device/wifi", shill::kTypeWifi, 1, -119, shill::kStateFailure,
+     em::NetworkState::FAILURE, "", ""},
+    {"activation-failure", "/device/cellular1", shill::kTypeCellular, 0, 0,
+     shill::kStateActivationFailure, em::NetworkState::ACTIVATION_FAILURE, "",
+     ""},
+    {"unknown", "", shill::kTypeWifi, 1, -119, "unknown",
+     em::NetworkState::UNKNOWN, "", ""},
 };
 
 static const FakeNetworkState kUnconfiguredNetwork = {

chromeos/dbus/shill/fake_shill_manager_client.cc

@@ -70,13 +70,19 @@ std::string GetStringValue(const base::Value& dict, const char* key) {
   return value ? value->GetString() : std::string();
 }
 
+bool IsPortalledState(const std::string& state) {
+  return state == shill::kStatePortal || state == shill::kStateNoConnectivity ||
+         state == shill::kStateRedirectFound ||
+         state == shill::kStatePortalSuspected;
+}
+
 int GetStateOrder(const base::Value& dict) {
   std::string state = GetStringValue(dict, shill::kStateProperty);
   if (state == shill::kStateOnline)
     return 1;
   if (state == shill::kStateReady)
     return 2;
-  if (state == shill::kStatePortal)
+  if (IsPortalledState(state))
     return 3;
   if (state == shill::kStateAssociation || state == shill::kStateConfiguration)
     return 4;
@@ -180,13 +186,14 @@ void LogErrorCallback(const std::string& error_name,
 }
 
 bool IsConnectedState(const std::string& state) {
-  return state == shill::kStateOnline || state == shill::kStatePortal ||
+  return state == shill::kStateOnline || IsPortalledState(state) ||
          state == shill::kStateReady;
 }
 
 void UpdatePortaledWifiState(const std::string& service_path) {
   ShillServiceClient::Get()->GetTestInterface()->SetServiceProperty(
-      service_path, shill::kStateProperty, base::Value(shill::kStatePortal));
+      service_path, shill::kStateProperty,
+      base::Value(shill::kStateNoConnectivity));
 }
 
 bool IsCellularTechnology(const std::string& type) {
@@ -731,7 +738,7 @@ void FakeShillManagerClient::SetupDefaultEnvironment() {
   state = GetInitialStateForType(shill::kTypeWifi, &enabled);
   if (state != kTechnologyUnavailable) {
     bool portaled = false;
-    if (state == shill::kStatePortal) {
+    if (IsPortalledState(state)) {
       portaled = true;
       state = shill::kStateIdle;
     }
@@ -1216,7 +1223,7 @@ bool FakeShillManagerClient::SetInitialNetworkState(
     state = kTechnologyInitializing;
   } else if (state_arg == "portal") {
     // Technology is enabled, a service is connected and in Portal state.
-    state = shill::kStatePortal;
+    state = shill::kStateNoConnectivity;
   } else if (state_arg == "active" || state_arg == "activated") {
     // Technology is enabled, a service is connected and Activated.
     state = kNetworkActivated;
@@ -1277,7 +1284,7 @@ std::string FakeShillManagerClient::GetInitialStateForType(
       *enabled = true;
       result = state;
     }
-    if ((state == shill::kStatePortal && type != shill::kTypeWifi) ||
+    if ((IsPortalledState(state) && type != shill::kTypeWifi) ||
         (state == kNetworkActivated && type != shill::kTypeCellular)) {
       LOG(WARNING) << "Invalid state: " << state << " for " << type;
       result = shill::kStateIdle;

chromeos/network/network_state.cc

@@ -39,7 +39,7 @@ std::string GetStringFromDictionary(const base::Value* dict, const char* key) {
 bool IsCaptivePortalState(const base::Value& properties, bool log) {
   std::string state =
       GetStringFromDictionary(&properties, shill::kStateProperty);
-  if (state != shill::kStatePortal)
+  if (!chromeos::NetworkState::StateIsPortalled(state))
     return false;
   if (!properties.FindKey(shill::kPortalDetectionFailedPhaseProperty) ||
       !properties.FindKey(shill::kPortalDetectionFailedStatusProperty)) {
@@ -58,7 +58,9 @@ bool IsCaptivePortalState(const base::Value& properties, bool log) {
   // returned and ignore other reasons.
   bool is_captive_portal =
       portal_detection_phase == shill::kPortalDetectionPhaseContent &&
-      portal_detection_status == shill::kPortalDetectionStatusFailure;
+      (portal_detection_status == shill::kPortalDetectionStatusSuccess ||
+       portal_detection_status == shill::kPortalDetectionStatusFailure ||
+       portal_detection_status == shill::kPortalDetectionStatusRedirect);
 
   if (log) {
     std::string name =
@@ -560,7 +562,7 @@ network_config::mojom::SecurityType NetworkState::GetMojoSecurity() const {
 bool NetworkState::StateIsConnected(const std::string& connection_state) {
   return (connection_state == shill::kStateReady ||
           connection_state == shill::kStateOnline ||
-          connection_state == shill::kStatePortal);
+          StateIsPortalled(connection_state));
 }
 
 // static
@@ -570,6 +572,14 @@ bool NetworkState::StateIsConnecting(const std::string& connection_state) {
           connection_state == shill::kStateCarrier);
 }
 
+// static
+bool NetworkState::StateIsPortalled(const std::string& connection_state) {
+  return (connection_state == shill::kStatePortal ||
+          connection_state == shill::kStateNoConnectivity ||
+          connection_state == shill::kStateRedirectFound ||
+          connection_state == shill::kStatePortalSuspected);
+}
+
 // static
 bool NetworkState::NetworkStateIsCaptivePortal(
     const base::Value& shill_properties) {

chromeos/network/network_state.h

@@ -236,6 +236,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkState : public ManagedState {
   // Helpers (used e.g. when a state, error, or shill dictionary is cached)
   static bool StateIsConnected(const std::string& connection_state);
   static bool StateIsConnecting(const std::string& connection_state);
+  static bool StateIsPortalled(const std::string& connection_state);
   static bool NetworkStateIsCaptivePortal(const base::Value& shill_properties);
   static bool ErrorIsValid(const std::string& error);
   static std::unique_ptr<NetworkState> CreateDefaultCellular(

chromeos/network/network_state_unittest.cc

@@ -172,7 +172,8 @@ TEST_F(NetworkStateTest, CaptivePortalState) {
   EXPECT_FALSE(network_state_.is_captive_portal());
 
   // State == portal, kPortalDetection* not set -> is_captive_portal = true
-  EXPECT_TRUE(SetStringProperty(shill::kStateProperty, shill::kStatePortal));
+  EXPECT_TRUE(
+      SetStringProperty(shill::kStateProperty, shill::kStateNoConnectivity));
   SignalInitialPropertiesReceived();
   EXPECT_TRUE(network_state_.is_captive_portal());
 

components/arc/net/arc_net_host_impl.cc

@@ -232,7 +232,7 @@ arc::mojom::ConnectionStateType TranslateConnectionState(
   if ((state == shill::kStateIdle) || (state == shill::kStateFailure) ||
       (state == ""))
     return arc::mojom::ConnectionStateType::NOT_CONNECTED;
-  if (state == shill::kStatePortal)
+  if (chromeos::NetworkState::StateIsPortalled(state))
     return arc::mojom::ConnectionStateType::PORTAL;
   if (state == shill::kStateOnline)
     return arc::mojom::ConnectionStateType::ONLINE;