ChromeOS: Implement CaptivePortalAuthenticationIgnoresProxy policy.

chrome/common/pref_names.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/common/pref_names.h"
 
 #include "base/basictypes.h"
 #include "chrome/common/pref_font_webkit_names.h"
 
 namespace prefs {
@@ skipping 821 matching lines @@
 // restore them after a reboot.
 const char kFileSystemProviderMounted[] = "file_system_provider.mounted";
 
 // A boolean pref set to true if the virtual keyboard should be enabled.
 const char kTouchVirtualKeyboardEnabled[] = "ui.touch_virtual_keyboard_enabled";
 
 // An integer pref that controls the wake on wifi features that should be
 // enabled.
 const char kWakeOnWiFiEnabled[] = "settings.internet.wake_on_wifi";
 
+// This is the policy CaptivePortalAuthenticationIgnoresProxy that allows to
+// open Captive Portal authentication pages in a separate window under

[bartfab (slow), 2015/01/14 11:00:07]

Nit 1: s/Captive Portal/captive portal/ (for consistency, you use lowercase
below)
Nit 2: s/under/under a/

[Alexander Alekseev, 2015/01/16 22:51:48]

Done.

+// temporary incognito profile ("signin profile" is used for this purpose),

[bartfab (slow), 2015/01/14 11:00:07]

Do you use the signin profile directly or an incognito profile tied to the
signin profile? In either case, there could be unintended side effects:

1) Incognito profiles are not 100% separated from the underlying profiles.

2) If the same user runs into multiple captive portals in a row (e.g. a user who
roams around a lot), cookies set by one captive portal will still be around when
the user visits the next profile. It may make sense to clean up the profile
whenever captive portal authentication is done.

3) You are using the signin profile as a convenient way to grab a profile not
tied to the current user's settings. What if someone else has the same idea when
implementing a completely unrelated feature? The two features may end up using
the same profile without knowing about each other, potentially leaking data
between their two use cases.

4) The signin profile is not tied to the current user's policy. This may allow
the user to e.g. click through from the captive portal page to so some page that
policy would normally disallow access to.

[Alexander Alekseev, 2015/01/16 22:51:48]

ProfileHelper::GetSigninProfile() returns "profile used to show Signin screen,
Gaia screen, Lock screen (easy unlock, etc...)", which is incognito profile.

This way we are currently using it to show web pages in some special cases,
clearing it in between.
What side-effects do you expect here?
It is already cleared. See
NetworkPortalNotificationController::OnDialogDestroyed();
Signin profile is currently used as "temporary profile" clearing it when needed.
If someone is unaware of this scenario, his new code would break.
Yes. That is why it is disabled by default for enterprise users.

[bartfab (slow), 2015/01/21 10:09:56]

Defense in depth and good security practice: I am not sure what is not fully
separated between regular and icognito profiles right now. More importantly, I
cannot be sure what will and will not be separated in the future. If anything
slips into the sign-in profile, it will persist onto the login screen. This is a
security risk we need to evaluate before landing this feature.
Excellent. Thanks for confirming.
What if the two features try to use temporary profiles concurrently?
Please document this caveat in policy_templates.json. Better be explicit about
this than to cause surprises.

+// which allows to bypass proxy for captive portal authentication.

[bartfab (slow), 2015/01/14 11:00:07]

Nit: s/proxy/the user's proxy/

[Alexander Alekseev, 2015/01/16 22:51:48]

Done.

+const char kCaptivePortalAuthenticationIgnoresProxy[] =
+    "proxy.captive_portal_ignores_proxy";
 #endif  // defined(OS_CHROMEOS)
 
 // The disabled messages in IPC logging.
 const char kIpcDisabledMessages[] = "ipc_log_disabled_messages";
 
 // A boolean pref set to true if a Home button to open the Home pages should be
 // visible on the toolbar.
 const char kShowHomeButton[] = "browser.show_home_button";
 
 // A string value which saves short list of recently user selected encodings
@@ skipping 1451 matching lines @@
 const char kPasswordBubbleNopesCount[] = "password_bubble.nopes";
 
 // Last user's interaction with the password bubble.
 const char kPasswordBubbleLastInteractions[] = "password_bubble.interactions";
 
 // Boolean that indicates whether elevation is needed to recover Chrome upgrade.
 const char kRecoveryComponentNeedsElevation[] =
     "recovery_component.needs_elevation";
 
 }  // namespace prefs