Introduce NetworkState::is_captive_portal()

For c/b/ui/webui/chromeos/login/network_state_informer.cc
TBR=nkostylov@chromium.org
BUG=450364

Review URL: https://codereview.chromium.org/873713004

Cr-Commit-Position: refs/heads/master@{#313311}

7 files changed, 131 insertions, 30 deletions

diff --git a/chrome/browser/chromeos/net/network_portal_detector_impl.cc b/chrome/browser/chromeos/net/network_portal_detector_impl.cc
index 3dbe501..c83cc0d 100644
--- a/chrome/browser/chromeos/net/network_portal_detector_impl.cc
+++ b/chrome/browser/chromeos/net/network_portal_detector_impl.cc
@@ -96,7 +96,7 @@ void RecordDiscrepancyWithShill(
           NetworkPortalDetectorImpl::kSessionShillOnlineHistogram,
           status,
           NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
-    } else if (network->connection_state() == shill::kStatePortal) {
+    } else if (network->is_captive_portal()) {
       UMA_HISTOGRAM_ENUMERATION(
           NetworkPortalDetectorImpl::kSessionShillPortalHistogram,
           status,
@@ -113,7 +113,7 @@ void RecordDiscrepancyWithShill(
           NetworkPortalDetectorImpl::kOobeShillOnlineHistogram,
           status,
           NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
-    } else if (network->connection_state() == shill::kStatePortal) {
+    } else if (network->is_captive_portal()) {
       UMA_HISTOGRAM_ENUMERATION(
           NetworkPortalDetectorImpl::kOobeShillPortalHistogram,
           status,
@@ -480,7 +480,7 @@ void NetworkPortalDetectorImpl::OnAttemptCompleted(
   // if the default network is in portal state.
   if (result != captive_portal::RESULT_NO_RESPONSE &&
       DBusThreadManager::Get()->GetShillProfileClient()->GetTestInterface() &&
-      network && network->connection_state() == shill::kStatePortal) {
+      network && network->is_captive_portal()) {
     result = captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL;
     response_code = 200;
   }
@@ -502,8 +502,7 @@ void NetworkPortalDetectorImpl::OnAttemptCompleted(
     case captive_portal::RESULT_NO_RESPONSE:
       if (state.response_code == net::HTTP_PROXY_AUTHENTICATION_REQUIRED) {
         state.status = CAPTIVE_PORTAL_STATUS_PROXY_AUTH_REQUIRED;
-      } else if (network &&
-                 (network->connection_state() == shill::kStatePortal)) {
+      } else if (network && network->is_captive_portal()) {
         // Take into account shill's detection results.
         state.status = CAPTIVE_PORTAL_STATUS_PORTAL;
       } else {
@@ -618,17 +617,15 @@ void NetworkPortalDetectorImpl::RecordDetectionStats(
       NOTREACHED();
       break;
     case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_OFFLINE:
-      if (network->connection_state() == shill::kStateOnline ||
-          network->connection_state() == shill::kStatePortal) {
+      if (network->IsConnectedState())
         RecordDiscrepancyWithShill(network, status);
-      }
       break;
     case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE:
       if (network->connection_state() != shill::kStateOnline)
         RecordDiscrepancyWithShill(network, status);
       break;
     case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL:
-      if (network->connection_state() != shill::kStatePortal)
+      if (!network->is_captive_portal())
         RecordDiscrepancyWithShill(network, status);
       break;
     case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PROXY_AUTH_REQUIRED:
diff --git a/chrome/browser/ui/webui/chromeos/login/network_state_informer.cc b/chrome/browser/ui/webui/chromeos/login/network_state_informer.cc
index ad93d44..af577ff 100644
--- a/chrome/browser/ui/webui/chromeos/login/network_state_informer.cc
+++ b/chrome/browser/ui/webui/chromeos/login/network_state_informer.cc
@@ -70,14 +70,14 @@ NetworkStateInformer::State GetStateForDefaultNetwork() {
     }
     if (status == NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL ||
         (status == NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_UNKNOWN &&
-         network->connection_state() == shill::kStatePortal))
+         network->is_captive_portal()))
       return NetworkStateInformer::CAPTIVE_PORTAL;
   } else {
     if (NetworkState::StateIsConnecting(network->connection_state()))
       return NetworkStateInformer::CONNECTING;
     if (network->connection_state() == shill::kStateOnline)
       return NetworkStateInformer::ONLINE;
-    if (network->connection_state() == shill::kStatePortal)
+    if (network->is_captive_portal())
       return NetworkStateInformer::CAPTIVE_PORTAL;
   }
   return NetworkStateInformer::OFFLINE;
diff --git a/chromeos/network/network_state.cc b/chromeos/network/network_state.cc
index bd0f0d7..62c7019 100644
--- a/chromeos/network/network_state.cc
+++ b/chromeos/network/network_state.cc
@@ -29,6 +29,47 @@ bool ConvertListValueToStringVector(const base::ListValue& string_list,
   return true;
 }
 
+bool IsCaptivePortalState(const base::DictionaryValue& properties, bool log) {
+  std::string state;
+  properties.GetStringWithoutPathExpansion(shill::kStateProperty, &state);
+  if (state != shill::kStatePortal)
+    return false;
+  std::string portal_detection_phase, portal_detection_status;
+  if (!properties.GetStringWithoutPathExpansion(
+          shill::kPortalDetectionFailedPhaseProperty,
+          &portal_detection_phase) ||
+      !properties.GetStringWithoutPathExpansion(
+          shill::kPortalDetectionFailedStatusProperty,
+          &portal_detection_status)) {
+    // If Shill (or a stub) has not set PortalDetectionFailedStatus
+    // or PortalDetectionFailedPhase, assume we are in captive portal state.
+    return true;
+  }
+
+  // Shill reports the phase in which it determined that the device is behind a
+  // captive portal. We only want to rely only on incorrect content being
+  // returned and ignore other reasons.
+  bool is_captive_portal =
+      portal_detection_phase == shill::kPortalDetectionPhaseContent &&
+      portal_detection_status == shill::kPortalDetectionStatusFailure;
+
+  if (log) {
+    std::string name;
+    properties.GetStringWithoutPathExpansion(shill::kNameProperty, &name);
+    if (name.empty())
+      properties.GetStringWithoutPathExpansion(shill::kSSIDProperty, &name);
+    if (!is_captive_portal) {
+      NET_LOG(EVENT) << "State is 'portal' but not in captive portal state:"
+                     << " name=" << name << " phase=" << portal_detection_phase
+                     << " status=" << portal_detection_status;
+    } else {
+      NET_LOG(EVENT) << "Network is in captive portal state: " << name;
+    }
+  }
+
+  return is_captive_portal;
+}
+
 }  // namespace
 
 namespace chromeos {
@@ -36,8 +77,9 @@ namespace chromeos {
 NetworkState::NetworkState(const std::string& path)
     : ManagedState(MANAGED_TYPE_NETWORK, path),
       visible_(false),
-      connectable_(false),
       prefix_length_(0),
+      connectable_(false),
+      is_captive_portal_(false),
       signal_strength_(0),
       cellular_out_of_credits_(false) {
 }
@@ -141,9 +183,13 @@ bool NetworkState::InitialPropertiesReceived(
   // SignalStrength > 0.
   if ((type() == shill::kTypeWifi || type() == shill::kTypeWimax) &&
       visible() && signal_strength_ <= 0) {
-      signal_strength_ = 1;
+    signal_strength_ = 1;
   }
 
+  // Any change to connection state will trigger a complete property update,
+  // so we update is_captive_portal_ here.
+  is_captive_portal_ = IsCaptivePortalState(properties, true /* log */);
+
   // Ensure that the network has a valid name.
   return UpdateName(properties);
 }
@@ -182,9 +228,8 @@ void NetworkState::GetStateProperties(base::DictionaryValue* dictionary) const {
 
   // Mobile properties
   if (NetworkTypePattern::Mobile().MatchesType(type())) {
-    dictionary->SetStringWithoutPathExpansion(
-        shill::kNetworkTechnologyProperty,
-        network_technology());
+    dictionary->SetStringWithoutPathExpansion(shill::kNetworkTechnologyProperty,
+                                              network_technology());
     dictionary->SetStringWithoutPathExpansion(shill::kActivationStateProperty,
                                               activation_state());
     dictionary->SetStringWithoutPathExpansion(shill::kRoamingStateProperty,
@@ -196,8 +241,8 @@ void NetworkState::GetStateProperties(base::DictionaryValue* dictionary) const {
 
 void NetworkState::IPConfigPropertiesChanged(
     const base::DictionaryValue& properties) {
-  for (base::DictionaryValue::Iterator iter(properties);
-       !iter.IsAtEnd(); iter.Advance()) {
+  for (base::DictionaryValue::Iterator iter(properties); !iter.IsAtEnd();
+       iter.Advance()) {
     std::string key = iter.key();
     const base::Value& value = iter.value();
 
@@ -262,7 +307,7 @@ bool NetworkState::IsInProfile() const {
 
 bool NetworkState::IsPrivate() const {
   return !profile_path_.empty() &&
-      profile_path_ != NetworkProfileHandler::GetSharedProfilePath();
+         profile_path_ != NetworkProfileHandler::GetSharedProfilePath();
 }
 
 std::string NetworkState::GetDnsServersAsString() const {
@@ -281,7 +326,7 @@ std::string NetworkState::GetNetmask() const {
 
 std::string NetworkState::GetSpecifier() const {
   if (!update_received()) {
-    NET_LOG(ERROR) << "GetSpecifier called before update: " <<  path();
+    NET_LOG(ERROR) << "GetSpecifier called before update: " << path();
     return std::string();
   }
   if (type() == shill::kTypeWifi)
@@ -320,6 +365,12 @@ bool NetworkState::StateIsConnecting(const std::string& connection_state) {
 }
 
 // static
+bool NetworkState::NetworkStateIsCaptivePortal(
+    const base::DictionaryValue& shill_properties) {
+  return IsCaptivePortalState(shill_properties, false /* log */);
+}
+
+// static
 bool NetworkState::ErrorIsValid(const std::string& error) {
   // Shill uses "Unknown" to indicate an unset or cleared error state.
   return !error.empty() && error != kErrorUnknown;
diff --git a/chromeos/network/network_state.h b/chromeos/network/network_state.h
index 403daf7..e8e6561 100644
--- a/chromeos/network/network_state.h
+++ b/chromeos/network/network_state.h
@@ -76,6 +76,7 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
 
   // Wireless property accessors
   bool connectable() const { return connectable_; }
+  bool is_captive_portal() const { return is_captive_portal_; }
   int signal_strength() const { return signal_strength_; }
 
   // Wifi property accessors
@@ -116,9 +117,11 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   // Set the GUID. Called exclusively by NetworkStateHandler.
   void SetGuid(const std::string& guid);
 
-  // Helpers (used e.g. when a state or error is cached)
+  // 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 NetworkStateIsCaptivePortal(
+      const base::DictionaryValue& shill_properties);
   static bool ErrorIsValid(const std::string& error);
 
  private:
@@ -143,7 +146,6 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   std::string connection_state_;
   std::string profile_path_;
   std::vector<uint8_t> raw_ssid_;  // Unknown encoding. Not necessarily UTF-8.
-  bool connectable_;
 
   // Reflects the current Shill Service.Error property. This might get cleared
   // by Shill shortly after a failure.
@@ -163,6 +165,8 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   GURL web_proxy_auto_discovery_url_;
 
   // Wireless properties, used for icons and Connect logic.
+  bool connectable_;
+  bool is_captive_portal_;
   int signal_strength_;
 
   // Cellular properties, used for icons, Connect, and Activation.
diff --git a/chromeos/network/network_state_handler.cc b/chromeos/network/network_state_handler.cc
index 9e2e83c..c5b77fc 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -29,10 +29,12 @@ namespace chromeos {
 namespace {
 
 bool ConnectionStateChanged(NetworkState* network,
-                            const std::string& prev_connection_state) {
-  return (network->connection_state() != prev_connection_state) &&
-         (network->connection_state() != shill::kStateIdle ||
-          !prev_connection_state.empty());
+                            const std::string& prev_connection_state,
+                            bool prev_is_captive_portal) {
+  return ((network->connection_state() != prev_connection_state) &&
+          !((network->connection_state() == shill::kStateIdle) &&
+            prev_connection_state.empty())) ||
+         (network->is_captive_portal() != prev_is_captive_portal);
 }
 
 std::string GetManagedStateLogType(const ManagedState* state) {
@@ -519,6 +521,7 @@ void NetworkStateHandler::UpdateNetworkStateProperties(
   DCHECK(network);
   bool network_property_updated = false;
   std::string prev_connection_state = network->connection_state();
+  bool prev_is_captive_portal = network->is_captive_portal();
   for (base::DictionaryValue::Iterator iter(properties);
        !iter.IsAtEnd(); iter.Advance()) {
     if (network->PropertyChanged(iter.key(), iter.value()))
@@ -531,8 +534,10 @@ void NetworkStateHandler::UpdateNetworkStateProperties(
   // Notify observers of NetworkState changes.
   if (network_property_updated || network->update_requested()) {
     // Signal connection state changed after all properties have been updated.
-    if (ConnectionStateChanged(network, prev_connection_state))
+    if (ConnectionStateChanged(network, prev_connection_state,
+                               prev_is_captive_portal)) {
       OnNetworkConnectionStateChanged(network);
+    }
     NET_LOG_EVENT("NetworkPropertiesUpdated", GetLogName(network));
     NotifyNetworkPropertiesUpdated(network);
   }
@@ -548,6 +553,7 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
   if (!network)
     return;
   std::string prev_connection_state = network->connection_state();
+  bool prev_is_captive_portal = network->is_captive_portal();
   std::string prev_profile_path = network->profile_path();
   changed |= network->PropertyChanged(key, value);
   if (!changed)
@@ -555,7 +561,8 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
 
   if (key == shill::kStateProperty || key == shill::kVisibleProperty) {
     network_list_sorted_ = false;
-    if (ConnectionStateChanged(network, prev_connection_state)) {
+    if (ConnectionStateChanged(network, prev_connection_state,
+                               prev_is_captive_portal)) {
       OnNetworkConnectionStateChanged(network);
       // If the connection state changes, other properties such as IPConfig
       // may have changed, so request a full update.
diff --git a/chromeos/network/network_state_unittest.cc b/chromeos/network/network_state_unittest.cc
index 37a3763..aa636d7 100644
--- a/chromeos/network/network_state_unittest.cc
+++ b/chromeos/network/network_state_unittest.cc
@@ -171,4 +171,46 @@ TEST_F(NetworkStateTest, SsidHexMultipleUpdates) {
   EXPECT_TRUE(SetStringProperty(shill::kWifiHexSsid, wifi_hex));
 }
 
+TEST_F(NetworkStateTest, CaptivePortalState) {
+  std::string network_name = "test";
+  EXPECT_TRUE(SetStringProperty(shill::kTypeProperty, shill::kTypeWifi));
+  EXPECT_TRUE(SetStringProperty(shill::kNameProperty, network_name));
+  std::string hex_ssid =
+      base::HexEncode(network_name.c_str(), network_name.length());
+  EXPECT_TRUE(SetStringProperty(shill::kWifiHexSsid, hex_ssid));
+
+  // State != portal -> is_captive_portal == false
+  EXPECT_TRUE(SetStringProperty(shill::kStateProperty, shill::kStateReady));
+  SignalInitialPropertiesReceived();
+  EXPECT_FALSE(network_state_.is_captive_portal());
+
+  // State == portal, kPortalDetection* not set -> is_captive_portal = true
+  EXPECT_TRUE(SetStringProperty(shill::kStateProperty, shill::kStatePortal));
+  SignalInitialPropertiesReceived();
+  EXPECT_TRUE(network_state_.is_captive_portal());
+
+  // Set kPortalDetectionFailed* properties to states that should not trigger
+  // is_captive_portal.
+  SetStringProperty(shill::kPortalDetectionFailedPhaseProperty,
+                    shill::kPortalDetectionPhaseUnknown);
+  SetStringProperty(shill::kPortalDetectionFailedStatusProperty,
+                    shill::kPortalDetectionStatusTimeout);
+  SignalInitialPropertiesReceived();
+  EXPECT_FALSE(network_state_.is_captive_portal());
+
+  // Set just the phase property to the expected captive portal state.
+  // is_captive_portal should still be false.
+  SetStringProperty(shill::kPortalDetectionFailedPhaseProperty,
+                    shill::kPortalDetectionPhaseContent);
+  SignalInitialPropertiesReceived();
+  EXPECT_FALSE(network_state_.is_captive_portal());
+
+  // Set the status property to the expected captive portal state property.
+  // is_captive_portal should now be true.
+  SetStringProperty(shill::kPortalDetectionFailedStatusProperty,
+                    shill::kPortalDetectionStatusFailure);
+  SignalInitialPropertiesReceived();
+  EXPECT_TRUE(network_state_.is_captive_portal());
+}
+
 }  // namespace chromeos
diff --git a/chromeos/network/onc/onc_translator_shill_to_onc.cc b/chromeos/network/onc/onc_translator_shill_to_onc.cc
index f497421..3de5f77 100644
--- a/chromeos/network/onc/onc_translator_shill_to_onc.cc
+++ b/chromeos/network/onc/onc_translator_shill_to_onc.cc
@@ -434,8 +434,8 @@ void ShillToONCTranslator::TranslateNetworkWithState() {
     }
     onc_object_->SetStringWithoutPathExpansion(
         ::onc::network_config::kConnectionState, onc_state);
-    // Only set 'RestrictedConnectivity' if true.
-    if (state == shill::kStatePortal) {
+    // Only set 'RestrictedConnectivity' if captive portal state is true.
+    if (NetworkState::NetworkStateIsCaptivePortal(*shill_dictionary_)) {
       onc_object_->SetBooleanWithoutPathExpansion(
           ::onc::network_config::kRestrictedConnectivity, true);
     }