ChromeOS: Implement CaptivePortalAuthenticationIgnoresProxy policy.

chrome/browser/chromeos/net/network_portal_notification_controller.cc

Index: chrome/browser/chromeos/net/network_portal_notification_controller.cc
diff --git a/chrome/browser/chromeos/net/network_portal_notification_controller.cc b/chrome/browser/chromeos/net/network_portal_notification_controller.cc
index 137c3f2eb61074e657213ec38ca235ad2107ddea..dd6cd7ce9ff32754e32eeca43bece620b3c390a9 100644
--- a/chrome/browser/chromeos/net/network_portal_notification_controller.cc
+++ b/chrome/browser/chromeos/net/network_portal_notification_controller.cc
@@ -13,15 +13,22 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
+#include "base/prefs/pref_service.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_process_platform_part.h"
 #include "chrome/browser/chromeos/mobile/mobile_activator.h"
+#include "chrome/browser/chromeos/net/captive_portal_ignore_proxy_policy_values.h"
 #include "chrome/browser/chromeos/net/network_portal_web_dialog.h"
+#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "chrome/browser/chromeos/policy/consumer_management_service.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/scoped_tabbed_browser_displayer.h"
 #include "chrome/browser/ui/singleton_tabs.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
 #include "chromeos/chromeos_switches.h"
@@ -101,12 +108,50 @@ void NetworkPortalNotificationControllerDelegate::Click() {
       NetworkPortalNotificationController::USER_ACTION_METRIC_CLICKED,
       NetworkPortalNotificationController::USER_ACTION_METRIC_COUNT);
 
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kEnableCaptivePortalBypassProxy)) {
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+
+  const bool command_line_switch_present =

[ygorshenin1, 2014/12/25 18:07:15]

It's not clear from the name of the variable what switch is present. Could you
please rename it to something like "bypass_proxy_switch_present"?

[Alexander Alekseev, 2015/01/05 20:53:03]

Done.

+      CommandLine::ForCurrentProcess()->HasSwitch(
+          chromeos::switches::kEnableCaptivePortalBypassProxy);
+  bool use_incognito_profile = command_line_switch_present;
+
+  // Check if CaptivePortalAuthenticationIgnoresProxy policy is applied.
+  if (profile &&
+      !profile->GetPrefs()
+           ->FindPreference(prefs::kCaptivePortalAuthenticationIgnoresProxy)
+           ->IsDefaultValue()) {
+    const int policy = profile->GetPrefs()->GetInteger(
+        prefs::kCaptivePortalAuthenticationIgnoresProxy);
+    switch (policy) {
+      case CAPTIVE_PORTAL_AUTH_IGNORE_PROXY_TRUE:
+        use_incognito_profile = true;
+        break;
+      case CAPTIVE_PORTAL_AUTH_IGNORE_PROXY_FALSE:
+        use_incognito_profile = false;
+        break;
+      case CAPTIVE_PORTAL_AUTH_IGNORE_PROXY_USER_CONFIGURABLE:

[Mattias Nissler (ping if slow), 2015/01/05 11:33:29]

This is not how policy is designed to work. user-configurable shouldn't be
decided by a separate value, but by whether the pref is controlled by policy or
not. See PrefService::Preference::IsUserModifiable(). Once you make this change,
you'll probably also want to switch the policy type to boolean instead of
integer.

[Alexander Alekseev, 2015/01/05 20:53:03]

Done.

+        break;
+      default:
+        NOTREACHED();
+    }
+    // ConsumerManagementService() might be undefined in tests.
+  } else if (g_browser_process->platform_part()

[ygorshenin1, 2014/12/25 18:07:15]

Could you please create a local variable for a result of
GetConsumerManagementService() call?

[Alexander Alekseev, 2015/01/05 20:53:03]

Done.

+                 ->browser_policy_connector_chromeos()
+                 ->GetConsumerManagementService() &&
+             g_browser_process->platform_part()
+                 ->browser_policy_connector_chromeos()
+                 ->GetConsumerManagementService()
+                 ->GetStatus() ==
+                 policy::ConsumerManagementService::STATUS_ENROLLED) {
+    // Enrolled device defaults to
+    // "CaptivePortalAuthenticationIgnoresProxy=false".
+    use_incognito_profile = false;

[Mattias Nissler (ping if slow), 2015/01/05 11:33:29]

Why do we need this code branch for consumer-enrolled devices?

[Alexander Alekseev, 2015/01/05 20:53:03]

Done.

+  }
+
+  if (use_incognito_profile) {
     if (controller_)
       controller_->ShowDialog();
   } else {
-    Profile* profile = ProfileManager::GetActiveUserProfile();
     if (!profile)
       return;
     chrome::ScopedTabbedBrowserDisplayer displayer(
@@ -132,7 +177,7 @@ const char NetworkPortalNotificationController::kUserActionMetric[] =
     "CaptivePortal.Notification.UserAction";
 
 NetworkPortalNotificationController::NetworkPortalNotificationController()
-    : dialog_(nullptr) {
+    : dialog_(nullptr), ignore_no_network_for_testing_(false) {
 }
 
 NetworkPortalNotificationController::~NetworkPortalNotificationController() {}
@@ -147,7 +192,10 @@ void NetworkPortalNotificationController::OnPortalDetectionCompleted(
       state.status != NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL) {
     last_network_path_.clear();
 
-    if (dialog_)
+    // Do not close dialog in browser tests on "no network" errors.
+    if (dialog_ &&
+        (!ignore_no_network_for_testing_ ||
+         state.status == NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE))
       dialog_->Close();

[ygorshenin1, 2014/12/25 18:07:15]

Could you please wrap branch in curly braces?

[Alexander Alekseev, 2015/01/05 20:53:02]

Done.

 
     CloseNotification();
@@ -207,4 +255,13 @@ void NetworkPortalNotificationController::OnDialogDestroyed(
   }
 }
 
+void NetworkPortalNotificationController::SetIgnoreNoNetworkForTesting() {
+  ignore_no_network_for_testing_ = true;
+}
+
+const NetworkPortalWebDialog*
+NetworkPortalNotificationController::GetDialogForTesting() const {
+  return dialog_;
+}
+
 }  // namespace chromeos