ChromeOS: Implement CaptivePortalAuthenticationIgnoresProxy policy.

ChromeOS: Implement CaptivePortalAuthenticationIgnoresProxy policy.

This CL adds CaptivePortalAuthenticationIgnoresProxy policy.

BUG=456839
TEST=manual

Committed: https://crrev.com/d17ddcb8b5e5dbf9bfe481567c63a23e9cfe0db3
Cr-Commit-Position: refs/heads/master@{#319138}

[Alexander Alekseev, 2014-12-18 19:40:58]

alemate@chromium.org changed reviewers:
+ bartfab@chromium.org, dpolukhin@chromium.org, isherman@chromium.org,
mnissler@chromium.org, ygorshenin@chromium.org

[Alexander Alekseev, 2014-12-18 19:40:59]

Please review:

dpolukhin@ : chrome/browser/chromeos/preferences.cc

ygorshenin@ : chrome/browser/chromeos/net/*

bartfab@, mnissler@: all (*policy* as owners and chrome/browser/chromeos/net/*
for policy usage)

isherman@: tools/metrics/histograms/histograms.xml

[Ilya Sherman, 2014-12-18 22:40:17]

histograms.xml lgtm

[bartfab (slow), 2014-12-19 12:08:54]

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/captive_portal_ignore_proxy_policy_values.h
(right):

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/captive_portal_ignore_proxy_policy_values.h:10: enum
CaptivePortalAuthenticationIgnoresProxyPolicyValues {
Why does this need its own header? Is there no existing place this enum could be
added to?

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:118:
profile->GetPrefs()->IsManagedPreference(
You do not really need to check if this is a managed preference. If a user
really manages to modify the pref's value directly, then so be it, we can just
follow the user's will.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:122: if
(policy == CAPTIVE_PORTAL_AUTH_IGNORE_PROXY_TRUE) {
This would probably be cleaner as a switch statement.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:127:
DCHECK(policy == CAPTIVE_PORTAL_AUTH_IGNORE_PROXY_USER_CONFIGURABLE);
Nit: Use DCHECK_EQ.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:129: }
else if (g_browser_process->platform_part()
Nit: #include "chrome/browser/browser_process_platform_part.h"

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:131:
->GetConsumerManagementService() &&
Can the consumer management service not exist?

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:133:
->browser_policy_connector_chromeos()
Nit: This is indented 4 spaces too many.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:137:
use_incognito_profile = false;
What is this case? Please document it.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/pref...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/pref...
chrome/browser/chromeos/preferences.cc:324: 
Nit: No need for two blank lines.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/818433003/diff/1/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:518: #if
defined(OS_CHROMEOS)
There is a block for Chrome OS-specific policies already that ends in line 492.
You can just add your policy in there.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:521:
base::Value::TYPE_INTEGER },
Since this is actually an int-enum, you should use a ConfigurationPolicyHandler
that checks whether the value is in the right range. However, as mentioned in my
comments for policy_templates.json, I think it would be best to turn this into a
boolean policy anyway.

https://codereview.chromium.org/818433003/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/818433003/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:2313: // Policy
CaptivePortalAuthenticationIgnoresProxy .
Nit 1: s/ ././
Nit 2: The description should briefly state what the setting actually controls,
not just what policy it corresponds to.

https://codereview.chromium.org/818433003/diff/1/chrome/test/data/policy/poli...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/818433003/diff/1/chrome/test/data/policy/poli...
chrome/test/data/policy/policy_test_cases.json:507: "test_policy": {
"IncognitoModeAvailability": 1 },
s/IncognitoModeAvailability/CaptivePortalAuthenticationIgnoresProxy/

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1248: 'name':
'CaptivePortalAuthenticationIgnoresProxy',
We have a group for proxy settings. Please move this policy in there.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1258: 'caption': '''Captive
portal authernication will use current proxy settings''',
Nit: s/authernication/authentication/

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1263: 'caption': '''Captive
portal authernication window will ignore proxy settings''',
Nit: s/authernication/authentication/

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1268: 'caption': '''Users will
be able to configure this parameter in about:flags''',
about:flags is not meant to be a place where you tweak configuration settings.
It is OK to add flags to control features under development but once a feature
ships, any flags should be removed. Making a flag a permanent control goes
against this rule.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1273: 'can_be_recommended':
False,
Your option 2 is semantically very close to a recommended policy. Why not turn
this into a boolean (use proxy / bypass proxy) and allow it to be recommended,
so that the user can override it?

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1279: 'caption': '''Captive
Portal Authentication Ignores Proxy''',
Nit: Please use regular capitalization, not title case.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1282: This policy only takes
effect if proxy is configured (either in domain policy, or by user in
chrome:settings).
Nit 1: s/in domain/through/
Nit 2: s/by/by the/
Nit 3: s|chrome:settings|chrome://settings|

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1284: If you enable this
setting, captive portal authentication page will be displayed in a separate
window ignoring all proxy settings.
Nit 1: s/captive/any captive/
Nit 2: s/page/pages/

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1286: If you disable this
setting, captive portal authentication page will be shown in a (regular) new
browser tab.
Nit 1: s/captive/any captive/
Nit 2: s/page/pages/

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1288: If you set this policy
to 'UserDefined', users would be able to configure this parameter in
about:flags.
As discussed above, this should be handled via recommended policy and should not
use about:flags as a configuration mechanism.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1290: Leaving this policy not
set eaquals to 'False' policy value.''',
Nit 1: s/eaquals/equals/
Nit 2: You can fold this into the disabled case above:

"If you disable this setting or leave it unset, ..."

[Alexander Alekseev, 2014-12-22 22:20:12]

I've added browser test.

Please review.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/captive_portal_ignore_proxy_policy_values.h
(right):

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/captive_portal_ignore_proxy_policy_values.h:10: enum
CaptivePortalAuthenticationIgnoresProxyPolicyValues {
On 2014/12/19 12:08:53, bartfab wrote:
> Why does this need its own header? Is there no existing place this enum could
be
> added to?

The idea was not to include
chrome/browser/chromeos/net/network_portal_notification_controller.h to 
chrome/browser/chromeos/preferences.cc .
It seems to me, that preferences.cc is already overloaded.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:118:
profile->GetPrefs()->IsManagedPreference(
On 2014/12/19 12:08:53, bartfab wrote:
> You do not really need to check if this is a managed preference. If a user
> really manages to modify the pref's value directly, then so be it, we can just
> follow the user's will.

Done.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:122: if
(policy == CAPTIVE_PORTAL_AUTH_IGNORE_PROXY_TRUE) {
On 2014/12/19 12:08:54, bartfab wrote:
> This would probably be cleaner as a switch statement.

Done.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:127:
DCHECK(policy == CAPTIVE_PORTAL_AUTH_IGNORE_PROXY_USER_CONFIGURABLE);
On 2014/12/19 12:08:53, bartfab wrote:
> Nit: Use DCHECK_EQ.

Done.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:129: }
else if (g_browser_process->platform_part()
On 2014/12/19 12:08:53, bartfab wrote:
> Nit: #include "chrome/browser/browser_process_platform_part.h"

Done.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:131:
->GetConsumerManagementService() &&
On 2014/12/19 12:08:53, bartfab wrote:
> Can the consumer management service not exist?

In tests. (Added comment on this.)

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:133:
->browser_policy_connector_chromeos()
On 2014/12/19 12:08:53, bartfab (OOO till 7th January) wrote:
> Nit: This is indented 4 spaces too many.

Done.

(BTW, this the exact result of clang-format.)

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:137:
use_incognito_profile = false;
On 2014/12/19 12:08:53, bartfab wrote:
> What is this case? Please document it.

Done.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/pref...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/818433003/diff/1/chrome/browser/chromeos/pref...
chrome/browser/chromeos/preferences.cc:324: 
On 2014/12/19 12:08:54, bartfab wrote:
> Nit: No need for two blank lines.

Done.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/818433003/diff/1/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:518: #if
defined(OS_CHROMEOS)
On 2014/12/19 12:08:54, bartfab wrote:
> There is a block for Chrome OS-specific policies already that ends in line
492.
> You can just add your policy in there.

Done.

https://codereview.chromium.org/818433003/diff/1/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:521:
base::Value::TYPE_INTEGER },
On 2014/12/19 12:08:54, bartfab (OOO till 7th January) wrote:
> Since this is actually an int-enum, you should use a
ConfigurationPolicyHandler
> that checks whether the value is in the right range. However, as mentioned in
my
> comments for policy_templates.json, I think it would be best to turn this into
a
> boolean policy anyway.

I've added IntRangePolicyHandler for the new preference.

Also, please see my comments below.

https://codereview.chromium.org/818433003/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/818433003/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:2313: // Policy
CaptivePortalAuthenticationIgnoresProxy .
On 2014/12/19 12:08:54, bartfab wrote:
> Nit 1: s/ ././
> Nit 2: The description should briefly state what the setting actually
controls,
> not just what policy it corresponds to.

Done.

https://codereview.chromium.org/818433003/diff/1/chrome/test/data/policy/poli...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/818433003/diff/1/chrome/test/data/policy/poli...
chrome/test/data/policy/policy_test_cases.json:507: "test_policy": {
"IncognitoModeAvailability": 1 },
On 2014/12/19 12:08:54, bartfab wrote:
> s/IncognitoModeAvailability/CaptivePortalAuthenticationIgnoresProxy/

Done.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1248: 'name':
'CaptivePortalAuthenticationIgnoresProxy',
On 2014/12/19 12:08:54, bartfab wrote:
> We have a group for proxy settings. Please move this policy in there.

I've moved it out of this group, because group description ("if this policy is
not set, all settings in the group are ignored") conflicts with this policy
description.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1258: 'caption': '''Captive
portal authernication will use current proxy settings''',
On 2014/12/19 12:08:54, bartfab wrote:
> Nit: s/authernication/authentication/

Done.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1263: 'caption': '''Captive
portal authernication window will ignore proxy settings''',
On 2014/12/19 12:08:54, bartfab wrote:
> Nit: s/authernication/authentication/

Done.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1268: 'caption': '''Users will
be able to configure this parameter in about:flags''',
On 2014/12/19 12:08:54, bartfab wrote:
> about:flags is not meant to be a place where you tweak configuration settings.
> It is OK to add flags to control features under development but once a feature
> ships, any flags should be removed. Making a flag a permanent control goes
> against this rule.

Done.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1273: 'can_be_recommended':
False,
On 2014/12/19 12:08:54, bartfab wrote:
> Your option 2 is semantically very close to a recommended policy. Why not turn
> this into a boolean (use proxy / bypass proxy) and allow it to be recommended,
> so that the user can override it?

As far, as I understand discussion in
https://docs.google.com/a/chromium.org/document/d/1XhwsJU9hoq-Pm73Nwk7w8edM2W...
, we want new behavior (ignore proxy for captive portals) by default for
ordinary users, but old algorithm (enforce proxy) for enrolled devices.

Most of binary policies are actually ternary "Unset/True/False". But this one
doesn't have "Unset" value, so I've created enum here to keep ternary behavior.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1279: 'caption': '''Captive
Portal Authentication Ignores Proxy''',
On 2014/12/19 12:08:54, bartfab wrote:
> Nit: Please use regular capitalization, not title case.

Done.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1282: This policy only takes
effect if proxy is configured (either in domain policy, or by user in
chrome:settings).
On 2014/12/19 12:08:54, bartfab wrote:
> Nit 1: s/in domain/through/
> Nit 2: s/by/by the/
> Nit 3: s|chrome:settings|chrome://settings|

Done.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1284: If you enable this
setting, captive portal authentication page will be displayed in a separate
window ignoring all proxy settings.
On 2014/12/19 12:08:54, bartfab wrote:
> Nit 1: s/captive/any captive/
> Nit 2: s/page/pages/

Done.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1286: If you disable this
setting, captive portal authentication page will be shown in a (regular) new
browser tab.
On 2014/12/19 12:08:54, bartfab wrote:
> Nit 1: s/captive/any captive/
> Nit 2: s/page/pages/

Done.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1288: If you set this policy
to 'UserDefined', users would be able to configure this parameter in
about:flags.
On 2014/12/19 12:08:54, bartfab wrote:
> As discussed above, this should be handled via recommended policy and should
not
> use about:flags as a configuration mechanism.

Done.

https://codereview.chromium.org/818433003/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:1290: Leaving this policy not
set eaquals to 'False' policy value.''',
On 2014/12/19 12:08:54, bartfab wrote:
> Nit 1: s/eaquals/equals/
> Nit 2: You can fold this into the disabled case above:
> 
> "If you disable this setting or leave it unset, ..."

Done.

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

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
(right):

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:234:
Controller::NOTIFICATION_METRIC_COUNT, NULL);
nit: you can use nullptr instead of NULL.

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:287:
CaptivePortalAuthenticationIgnoresProxyFalse) {
Both CaptivePortalAuthenticationIgnoresProxy{False|True} are almost identical,
except expectations on GetDialog() calls and value of
kCaptivePortalAuthenticationIgnoresProxy pref. Could you please get rid of this
duplication?

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:113: const
bool command_line_switch_present =
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"?

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:138: }
else if (g_browser_process->platform_part()
Could you please create a local variable for a result of
GetConsumerManagementService() call?

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:199:
dialog_->Close();
Could you please wrap branch in curly braces?

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

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:132: case
CAPTIVE_PORTAL_AUTH_IGNORE_PROXY_USER_CONFIGURABLE:
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.

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:148:
use_incognito_profile = false;
Why do we need this code branch for consumer-enrolled devices?

https://codereview.chromium.org/818433003/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/818433003/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:2312: #if defined(OS_CHROMEOS)
Please add this to the OS_CHROMEOS block in lines 457--842.

[Alexander Alekseev, 2015-01-05 20:53:01]

alemate@chromium.org changed reviewers:
+ stevenjb@chromium.org

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

+ stevenjb@ : Please review *browser_options*, and
chrome/app/chromeos_strings.grdp

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
(right):

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:234:
Controller::NOTIFICATION_METRIC_COUNT, NULL);
On 2014/12/25 18:07:15, ygorshenin1 wrote:
> nit: you can use nullptr instead of NULL.

Done.

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:287:
CaptivePortalAuthenticationIgnoresProxyFalse) {
On 2014/12/25 18:07:15, ygorshenin1 wrote:
> Both CaptivePortalAuthenticationIgnoresProxy{False|True} are almost identical,
> except expectations on GetDialog() calls and value of
> kCaptivePortalAuthenticationIgnoresProxy pref. Could you please get rid of
this
> duplication?

Done.

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:113: const
bool command_line_switch_present =
On 2014/12/25 18:07:15, ygorshenin1 wrote:
> 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"?

Done.

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:132: case
CAPTIVE_PORTAL_AUTH_IGNORE_PROXY_USER_CONFIGURABLE:
On 2015/01/05 11:33:29, Mattias Nissler wrote:
> 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.

Done.

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:138: }
else if (g_browser_process->platform_part()
On 2014/12/25 18:07:15, ygorshenin1 wrote:
> Could you please create a local variable for a result of
> GetConsumerManagementService() call?

Done.

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:148:
use_incognito_profile = false;
On 2015/01/05 11:33:29, Mattias Nissler wrote:
> Why do we need this code branch for consumer-enrolled devices?

Done.

https://codereview.chromium.org/818433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:199:
dialog_->Close();
On 2014/12/25 18:07:15, ygorshenin1 wrote:
> Could you please wrap branch in curly braces?

Done.

https://codereview.chromium.org/818433003/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/818433003/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:2312: #if defined(OS_CHROMEOS)
On 2015/01/05 11:33:29, Mattias Nissler wrote:
> Please add this to the OS_CHROMEOS block in lines 457--842.

Done.

[Mattias Nissler (ping if slow), 2015-01-06 09:11:53]

policy and prefs bits LGTM.

[bartfab (slow), 2015-01-07 17:38:43]

https://codereview.chromium.org/818433003/diff/60001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/818433003/diff/60001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:5954: <message
name="IDS_OPTIONS_CAPTIVE_PORTAL_BYPASS_PROXY_LABEL" desc="Option in
chrome://settings that allows Captive Portal authorization window to ignore
proxy settings.">
Nit: s/window/pages/

https://codereview.chromium.org/818433003/diff/60001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:5955: Captive portal authorization bypass proxy
Please run this string by the Chrome UI reviewers. At the very least,
s/bypass/bypasses/ but I suspect they will have further comments on the wording.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
(right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:148:
network_portal_detector_->notification_controller_
Nit: #include
"chrome/browser/chromeos/net/network_portal_notification_controller.h"

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:152:
const NetworkPortalWebDialog* GetDialog() const {
Nit: Forward-declare |NetworkPortalWebDialog|.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:225:
virtual ~NetworkPortalDetectorImplBrowserTestIgnoreProxy() {}
Nit 1: Use |override|, not |virtual|.
Nit 2: There is actually no need to override the destructor at all, since you
are leaving it empty.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:230:
command_line->AppendSwitch(
Nit: #include "base/command_line.h"

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:242:
typedef NetworkPortalNotificationController Controller;
Nit: Use using=, not typedef.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:256:
ProfileManager::GetActiveUserProfile()->GetPrefs()->SetBoolean(
Nit 1: #include "chrome/browser/profiles/profile_manager.h"
Nit 2: #include "chrome/browser/profiles/profile.h"
Nit 3: #include "chrome/browser/prefs/pref_service_syncable.h"

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:257:
prefs::kCaptivePortalAuthenticationIgnoresProxy, preference_value);
Nit: #include "chrome/common/pref_names.h"

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:262:
ASSERT_EQ(PortalDetectorStrategy::STRATEGY_ID_SESSION, strategy()->Id());
Nit: Why ASSERT, not EXPECT?

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:265:
ASSERT_FALSE(message_center()->FindVisibleNotificationById(kNotificationId));
Nit: Why ASSERT, not EXPECT?

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:269: //
Check that wifi is marked as behind the portal and that notification
Nit 1: s/wifi/WiFi/
Nit 2: s/the/a/
Nit 3: s/notification/a notification/

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:271:
ASSERT_TRUE(message_center()->FindVisibleNotificationById(kNotificationId));
Nit: Why ASSERT, not EXPECT?

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:272:
ASSERT_EQ(
Nit: Why ASSERT, not EXPECT?

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:279:
ASSERT_TRUE(
Nit: Why ASSERT, not EXPECT?

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:281:
ASSERT_TRUE(action_checker.Check());
Nit: Why ASSERT, not EXPECT?

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:285:
base::RunLoop().RunUntilIdle();
Nit: It is strange to use |base::RunLoop| and
|content::RunAllPendingInMessageLoop()| in the same method. The latter is
preferable in browser tests.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:293: }
Nit: This entire conditional can be collapsed to:

EXPECT_EQ(preference_value, GetDialog());

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:300:
ASSERT_EQ(PortalDetectorStrategy::STRATEGY_ID_LOGIN_SCREEN, strategy()->Id());
Nit: Why ASSERT, not EXPECT?

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:312:
ASSERT_EQ(PortalDetectorStrategy::STRATEGY_ID_LOGIN_SCREEN, strategy()->Id());
Nit: Why ASSERT, not EXPECT?

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:118:
profile->GetPrefs()->GetBoolean(
Nit: #include "base/prefs/pref_service.h"

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:119:
prefs::kCaptivePortalAuthenticationIgnoresProxy))
Nit: #include "chrome/common/pref_names.h"

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:168:
(!ignore_no_network_for_testing_ ||
This conditional makes |ignore_no_network_for_testing_| prevent the dialog from
closing in *all* cases, not just for a specific class of "no network" errors.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:169:
state.status == NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE)) {
If |ignore_no_network_for_testing_| is false (the default), the dialog used to
be always closed. Now, it will not be closed if |state.status ==
NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE|. Is this intentional?

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:326:
prefs::kCaptivePortalAuthenticationIgnoresProxy, true,
Why is the default value |true|? According to policy_templates.json, leaving the
policy unset should be equivalent to |false|.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:663:
chromeos::switches::kEnableCaptivePortalBypassProxy));
AFAICT, this switch does not actually enable bypassing. It enables the *option*
to bypass the proxy. Please rename the switch to make it clear what it actually
enables.

https://codereview.chromium.org/818433003/diff/60001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/818433003/diff/60001/chrome/common/pref_names...
chrome/common/pref_names.cc:842: // This is policy
CaptivePortalAuthenticationIgnoresProxy that allows to open
Nit: s/is/is the/

https://codereview.chromium.org/818433003/diff/60001/chrome/common/pref_names...
chrome/common/pref_names.cc:843: // Captive Portal authentication page in a
separate window under another
Nit: s/page/pages/

https://codereview.chromium.org/818433003/diff/60001/chrome/common/pref_names...
chrome/common/pref_names.cc:844: // profile, which allows to bypass proxy for
captive portal authentication.
What is this "another profile"? Some other user's profile? I presume you mean
some kind of temporary incognito profile. Could you clarify please?

https://codereview.chromium.org/818433003/diff/60001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/818433003/diff/60001/components/policy/resour...
components/policy/resources/policy_templates.json:1261: 'desc': '''<ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will bypass any proxy for
Captive Portal authentication.
Nit 1: This unconditional sentence makes it sound like Chrome will always bypass
the proxy. It would rather say that this policy controls whether Chrome bypasses
the proxy or not.
Nit 2: You should use the placeholder for Google Chrome OS here, not the one for
Google Chrome.

https://codereview.chromium.org/818433003/diff/60001/components/policy/resour...
components/policy/resources/policy_templates.json:1263: This policy only takes
effect if proxy is configured (either through policy, or by the user in
chrome://settings).
Nit 1: s/proxy/a proxy/
Nit 2: What about proxies configured by extensions?

https://codereview.chromium.org/818433003/diff/60001/components/policy/resour...
components/policy/resources/policy_templates.json:1267: If you disable this
setting or leave it unset, any captive portal authentication pages will be shown
in a (regular) new browser tab.''',
Nit: You may want to add something like ", using the current user's proxy."

https://codereview.chromium.org/818433003/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/818433003/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:44983: +  <int value="285"
label="Captive Portal Authentication Ignores Proxy"/>
Please update the capitalization:

s/Captive Portal Authentication Ignores Proxy/Captive portal authentication
ignores proxy/

[stevenjb, 2015-01-07 18:36:30]

stevenjb@chromium.org changed required reviewers:
+ bartfab@chromium.org

[stevenjb, 2015-01-07 18:36:51]

Owner lgtm with bartfab@'s comments addressed

[ygorshenin1, 2015-01-12 10:34:49]

LGTM

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
(right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:303:
IN_PROC_BROWSER_TEST_F(NetworkPortalDetectorImplBrowserTestIgnoreProxy,
nit: instead of two explicitly defined test fixtures you can try to reuse
IN_PROC_BROWSER_TEST_P:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tas...

[Alexander Alekseev, 2015-01-12 18:41:10]

https://codereview.chromium.org/818433003/diff/60001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/818433003/diff/60001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:5954: <message
name="IDS_OPTIONS_CAPTIVE_PORTAL_BYPASS_PROXY_LABEL" desc="Option in
chrome://settings that allows Captive Portal authorization window to ignore
proxy settings.">
On 2015/01/07 17:38:41, bartfab wrote:
> Nit: s/window/pages/

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:5955: Captive portal authorization bypass proxy
On 2015/01/07 17:38:41, bartfab wrote:
> Please run this string by the Chrome UI reviewers. At the very least,
> s/bypass/bypasses/ but I suspect they will have further comments on the
wording.

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
(right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:148:
network_portal_detector_->notification_controller_
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: #include
> "chrome/browser/chromeos/net/network_portal_notification_controller.h"

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:152:
const NetworkPortalWebDialog* GetDialog() const {
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: Forward-declare |NetworkPortalWebDialog|.

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:225:
virtual ~NetworkPortalDetectorImplBrowserTestIgnoreProxy() {}
On 2015/01/07 17:38:42, bartfab wrote:
> Nit 1: Use |override|, not |virtual|.
> Nit 2: There is actually no need to override the destructor at all, since you
> are leaving it empty.

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:230:
command_line->AppendSwitch(
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: #include "base/command_line.h"

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:242:
typedef NetworkPortalNotificationController Controller;
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: Use using=, not typedef.

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:256:
ProfileManager::GetActiveUserProfile()->GetPrefs()->SetBoolean(
On 2015/01/07 17:38:42, bartfab wrote:
> Nit 1: #include "chrome/browser/profiles/profile_manager.h"
> Nit 2: #include "chrome/browser/profiles/profile.h"
> Nit 3: #include "chrome/browser/prefs/pref_service_syncable.h"

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:257:
prefs::kCaptivePortalAuthenticationIgnoresProxy, preference_value);
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: #include "chrome/common/pref_names.h"

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:262:
ASSERT_EQ(PortalDetectorStrategy::STRATEGY_ID_SESSION, strategy()->Id());
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: Why ASSERT, not EXPECT?

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:265:
ASSERT_FALSE(message_center()->FindVisibleNotificationById(kNotificationId));
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: Why ASSERT, not EXPECT?

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:269: //
Check that wifi is marked as behind the portal and that notification
On 2015/01/07 17:38:42, bartfab wrote:
> Nit 1: s/wifi/WiFi/
> Nit 2: s/the/a/
> Nit 3: s/notification/a notification/

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:271:
ASSERT_TRUE(message_center()->FindVisibleNotificationById(kNotificationId));
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: Why ASSERT, not EXPECT?

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:272:
ASSERT_EQ(
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: Why ASSERT, not EXPECT?

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:279:
ASSERT_TRUE(
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: Why ASSERT, not EXPECT?

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:281:
ASSERT_TRUE(action_checker.Check());
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: Why ASSERT, not EXPECT?

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:285:
base::RunLoop().RunUntilIdle();
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: It is strange to use |base::RunLoop| and
> |content::RunAllPendingInMessageLoop()| in the same method. The latter is
> preferable in browser tests.

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:293: }
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: This entire conditional can be collapsed to:
> 
> EXPECT_EQ(preference_value, GetDialog());

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:300:
ASSERT_EQ(PortalDetectorStrategy::STRATEGY_ID_LOGIN_SCREEN, strategy()->Id());
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: Why ASSERT, not EXPECT?

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:303:
IN_PROC_BROWSER_TEST_F(NetworkPortalDetectorImplBrowserTestIgnoreProxy,
On 2015/01/12 10:34:49, ygorshenin1 wrote:
> nit: instead of two explicitly defined test fixtures you can try to reuse
> IN_PROC_BROWSER_TEST_P:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tas...

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:312:
ASSERT_EQ(PortalDetectorStrategy::STRATEGY_ID_LOGIN_SCREEN, strategy()->Id());
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: Why ASSERT, not EXPECT?

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:118:
profile->GetPrefs()->GetBoolean(
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: #include "base/prefs/pref_service.h"

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:119:
prefs::kCaptivePortalAuthenticationIgnoresProxy))
On 2015/01/07 17:38:42, bartfab wrote:
> Nit: #include "chrome/common/pref_names.h"

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:168:
(!ignore_no_network_for_testing_ ||
On 2015/01/07 17:38:42, bartfab wrote:
> This conditional makes |ignore_no_network_for_testing_| prevent the dialog
from
> closing in *all* cases, not just for a specific class of "no network" errors.

This is intentional.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:169:
state.status == NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE)) {
On 2015/01/07 17:38:42, bartfab wrote:
> If |ignore_no_network_for_testing_| is false (the default), the dialog used to
> be always closed. Now, it will not be closed if |state.status ==
> NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE|. Is this intentional?

Yes.
In browser_tests we have very strange condition: network portal is forced to
show, but network state is not changed, so it is usually ONLINE. And in a few
moments after network portal is shown, we receive event "network is online"
which forces portal dialog to close before we check that it exists.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:326:
prefs::kCaptivePortalAuthenticationIgnoresProxy, true,
On 2015/01/07 17:38:43, bartfab wrote:
> Why is the default value |true|? According to policy_templates.json, leaving
the
> policy unset should be equivalent to |false|.

Because this is shared preference, i.e. controlled both by chrome:settings and
by policy.

But we want different defaults: it should default to |true| for non-enterprise
users, but |false| for enterprise.

There is a more detailed description of algorithm in issue.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:663:
chromeos::switches::kEnableCaptivePortalBypassProxy));
On 2015/01/07 17:38:43, bartfab wrote:
> AFAICT, this switch does not actually enable bypassing. It enables the
*option*
> to bypass the proxy. Please rename the switch to make it clear what it
actually
> enables.

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/818433003/diff/60001/chrome/common/pref_names...
chrome/common/pref_names.cc:842: // This is policy
CaptivePortalAuthenticationIgnoresProxy that allows to open
On 2015/01/07 17:38:43, bartfab wrote:
> Nit: s/is/is the/

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/common/pref_names...
chrome/common/pref_names.cc:843: // Captive Portal authentication page in a
separate window under another
On 2015/01/07 17:38:43, bartfab wrote:
> Nit: s/page/pages/

Done.

https://codereview.chromium.org/818433003/diff/60001/chrome/common/pref_names...
chrome/common/pref_names.cc:844: // profile, which allows to bypass proxy for
captive portal authentication.
On 2015/01/07 17:38:43, bartfab wrote:
> What is this "another profile"? Some other user's profile? I presume you mean
> some kind of temporary incognito profile. Could you clarify please?

Done.

https://codereview.chromium.org/818433003/diff/60001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/818433003/diff/60001/components/policy/resour...
components/policy/resources/policy_templates.json:1261: 'desc': '''<ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will bypass any proxy for
Captive Portal authentication.
On 2015/01/07 17:38:43, bartfab wrote:
> Nit 1: This unconditional sentence makes it sound like Chrome will always
bypass
> the proxy. It would rather say that this policy controls whether Chrome
bypasses
> the proxy or not.
> Nit 2: You should use the placeholder for Google Chrome OS here, not the one
for
> Google Chrome.

Done.

https://codereview.chromium.org/818433003/diff/60001/components/policy/resour...
components/policy/resources/policy_templates.json:1263: This policy only takes
effect if proxy is configured (either through policy, or by the user in
chrome://settings).
On 2015/01/07 17:38:43, bartfab wrote:
> Nit 1: s/proxy/a proxy/
> Nit 2: What about proxies configured by extensions?

Done.

https://codereview.chromium.org/818433003/diff/60001/components/policy/resour...
components/policy/resources/policy_templates.json:1267: If you disable this
setting or leave it unset, any captive portal authentication pages will be shown
in a (regular) new browser tab.''',
On 2015/01/07 17:38:43, bartfab wrote:
> Nit: You may want to add something like ", using the current user's proxy."

Done.

https://codereview.chromium.org/818433003/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/818433003/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:44983: +  <int value="285"
label="Captive Portal Authentication Ignores Proxy"/>
On 2015/01/07 17:38:43, bartfab wrote:
> Please update the capitalization:
> 
> s/Captive Portal Authentication Ignores Proxy/Captive portal authentication
> ignores proxy/

Done.

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

Just a few more nits in the code. But I think you should check with security and
privacy whether this use of the signin profile is a good idea.

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:168:
(!ignore_no_network_for_testing_ ||
On 2015/01/12 18:41:09, Alexander Alekseev wrote:
> On 2015/01/07 17:38:42, bartfab wrote:
> > This conditional makes |ignore_no_network_for_testing_| prevent the dialog
> from
> > closing in *all* cases, not just for a specific class of "no network"
errors.
> 
> This is intentional.

It does not seem to match the comment though.

https://codereview.chromium.org/818433003/diff/80001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/818433003/diff/80001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:5955: Captive portal authorization bypasses
proxy
Are we actually sure that we want to expose this as a user-adjustable option at
all? Chrome tends to not expose tunables that are meaningless to the vast
majority of users. A policy makes sense of course - but do we need a setting in
the consumer case?

https://codereview.chromium.org/818433003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
(right):

https://codereview.chromium.org/818433003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:227:
public testing::WithParamInterface<bool> {
Nit: #include "testing/gtest/include/gtest/gtest.h"

https://codereview.chromium.org/818433003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:310: /*
Why is this test case commented out?

https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:843: // open Captive Portal authentication pages in
a separate window under
Nit 1: s/Captive Portal/captive portal/ (for consistency, you use lowercase
below)
Nit 2: s/under/under a/

https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:844: // temporary incognito profile ("signin
profile" is used for this purpose),
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.

https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:845: // which allows to bypass proxy for captive
portal authentication.
Nit: s/proxy/the user's proxy/

https://codereview.chromium.org/818433003/diff/80001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/818433003/diff/80001/components/policy/resour...
components/policy/resources/policy_templates.json:1261: 'desc': '''This policy
allows <ph name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> to bypass any
proxy for Captive Portal authentication.
Nit: s/Captive Portal/captive portal/ (for consistency - you use lowercase
throughout the remainder of the description)

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

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/818433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:168:
(!ignore_no_network_for_testing_ ||
On 2015/01/14 11:00:07, bartfab wrote:
> On 2015/01/12 18:41:09, Alexander Alekseev wrote:
> > On 2015/01/07 17:38:42, bartfab wrote:
> > > This conditional makes |ignore_no_network_for_testing_| prevent the dialog
> > from
> > > closing in *all* cases, not just for a specific class of "no network"
> errors.
> > 
> > This is intentional.
> 
> It does not seem to match the comment though.

Done.

(The comment was really outdated, sorry.)

https://codereview.chromium.org/818433003/diff/80001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/818433003/diff/80001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:5955: Captive portal authorization bypasses
proxy
On 2015/01/14 11:00:07, bartfab wrote:
> Are we actually sure that we want to expose this as a user-adjustable option
at
> all? Chrome tends to not expose tunables that are meaningless to the vast
> majority of users. A policy makes sense of course - but do we need a setting
in
> the consumer case?

That's a good question. But I would leave the decision to UI reviewers.

If captive portal is behind proxy, this setting would allow to log in to portal.
But I agree that this is a very rare and unusual case.

https://codereview.chromium.org/818433003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
(right):

https://codereview.chromium.org/818433003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:227:
public testing::WithParamInterface<bool> {
On 2015/01/14 11:00:07, bartfab wrote:
> Nit: #include "testing/gtest/include/gtest/gtest.h"

Done.

https://codereview.chromium.org/818433003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc:310: /*
On 2015/01/14 11:00:07, bartfab wrote:
> Why is this test case commented out?

It is now covered by parametric test, so it is an outdated code.
I've removed it.

https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:843: // open Captive Portal authentication pages in
a separate window under
On 2015/01/14 11:00:07, bartfab wrote:
> Nit 1: s/Captive Portal/captive portal/ (for consistency, you use lowercase
> below)
> Nit 2: s/under/under a/

Done.

https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:844: // temporary incognito profile ("signin
profile" is used for this purpose),
On 2015/01/14 11:00:07, bartfab wrote:
> Do you use the signin profile directly or an incognito profile tied to the
> signin profile?

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.

> In either case, there could be unintended side effects:
> 
> 1) Incognito profiles are not 100% separated from the underlying profiles.

What side-effects do you expect here?

> 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.

It is already cleared. See
NetworkPortalNotificationController::OnDialogDestroyed();

> 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.

Signin profile is currently used as "temporary profile" clearing it when needed.
If someone is unaware of this scenario, his new code would break.

> 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.

Yes. That is why it is disabled by default for enterprise users.

https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:845: // which allows to bypass proxy for captive
portal authentication.
On 2015/01/14 11:00:07, bartfab wrote:
> Nit: s/proxy/the user's proxy/

Done.

https://codereview.chromium.org/818433003/diff/80001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/818433003/diff/80001/components/policy/resour...
components/policy/resources/policy_templates.json:1261: 'desc': '''This policy
allows <ph name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> to bypass any
proxy for Captive Portal authentication.
On 2015/01/14 11:00:07, bartfab wrote:
> Nit: s/Captive Portal/captive portal/ (for consistency - you use lowercase
> throughout the remainder of the description)

Done.

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

bartfab@chromium.org changed reviewers:
+ wad@chromium.org

[bartfab (slow), 2015-01-21 10:09:57]

+ Will for security insight.

Will, this feature will derive a temporary incognito profile in-session from the
sign-in profile.

Although we use an incognito profile and we scrub it after use, there always is
a non-zero risk of some data crossing the boundary to the sign-in screen or vice
versa. Do you feel that this is a risk we can live with, or should we rethink
the approach before landing this?

https://codereview.chromium.org/818433003/diff/80001/chrome/app/chromeos_stri...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/818433003/diff/80001/chrome/app/chromeos_stri...
chrome/app/chromeos_strings.grdp:5955: Captive portal authorization bypasses
proxy
On 2015/01/16 22:51:48, Alexander Alekseev wrote:
> On 2015/01/14 11:00:07, bartfab wrote:
> > Are we actually sure that we want to expose this as a user-adjustable option
> at
> > all? Chrome tends to not expose tunables that are meaningless to the vast
> > majority of users. A policy makes sense of course - but do we need a setting
> in
> > the consumer case?
> 
> That's a good question. But I would leave the decision to UI reviewers.
> 
> If captive portal is behind proxy, this setting would allow to log in to
portal.
> But I agree that this is a very rare and unusual case.

Let's find out whether the UI folks want a setting for this before we land one
then.

https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names...
chrome/common/pref_names.cc:844: // temporary incognito profile ("signin
profile" is used for this purpose),
On 2015/01/16 22:51:48, Alexander Alekseev wrote:
> On 2015/01/14 11:00:07, bartfab wrote:
> > Do you use the signin profile directly or an incognito profile tied to the
> > signin profile?
> 
> 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.
> 
> > In either case, there could be unintended side effects:
> > 
> > 1) Incognito profiles are not 100% separated from the underlying profiles.
> 
> What side-effects do you expect here?

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.

> 
> > 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.
> 
> It is already cleared. See
> NetworkPortalNotificationController::OnDialogDestroyed();

Excellent. Thanks for confirming.

> 
> > 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.
> 
> Signin profile is currently used as "temporary profile" clearing it when
needed.
> If someone is unaware of this scenario, his new code would break.

What if the two features try to use temporary profiles concurrently?

> 
> > 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.
> 
> Yes. That is why it is disabled by default for enterprise users.
> 

Please document this caveat in policy_templates.json. Better be explicit about
this than to cause surprises.

[Alexander Alekseev, 2015-01-29 19:25:04]

On 2015/01/21 10:09:57, bartfab wrote:
> + Will for security insight.
> 
> Will, this feature will derive a temporary incognito profile in-session from
the
> sign-in profile.
> 
> Although we use an incognito profile and we scrub it after use, there always
is
> a non-zero risk of some data crossing the boundary to the sign-in screen or
vice
> versa. Do you feel that this is a risk we can live with, or should we rethink
> the approach before landing this?
> 
>
https://codereview.chromium.org/818433003/diff/80001/chrome/app/chromeos_stri...
> File chrome/app/chromeos_strings.grdp (right):
> 
>
https://codereview.chromium.org/818433003/diff/80001/chrome/app/chromeos_stri...
> chrome/app/chromeos_strings.grdp:5955: Captive portal authorization bypasses
> proxy
> On 2015/01/16 22:51:48, Alexander Alekseev wrote:
> > On 2015/01/14 11:00:07, bartfab wrote:
> > > Are we actually sure that we want to expose this as a user-adjustable
option
> > at
> > > all? Chrome tends to not expose tunables that are meaningless to the vast
> > > majority of users. A policy makes sense of course - but do we need a
setting
> > in
> > > the consumer case?
> > 
> > That's a good question. But I would leave the decision to UI reviewers.
> > 
> > If captive portal is behind proxy, this setting would allow to log in to
> portal.
> > But I agree that this is a very rare and unusual case.
> 
> Let's find out whether the UI folks want a setting for this before we land one
> then.
> 
>
https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names.cc
> File chrome/common/pref_names.cc (right):
> 
>
https://codereview.chromium.org/818433003/diff/80001/chrome/common/pref_names...
> chrome/common/pref_names.cc:844: // temporary incognito profile ("signin
> profile" is used for this purpose),
> On 2015/01/16 22:51:48, Alexander Alekseev wrote:
> > On 2015/01/14 11:00:07, bartfab wrote:
> > > Do you use the signin profile directly or an incognito profile tied to the
> > > signin profile?
> > 
> > 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.
> > 
> > > In either case, there could be unintended side effects:
> > > 
> > > 1) Incognito profiles are not 100% separated from the underlying profiles.
> > 
> > What side-effects do you expect here?
> 
> 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.
>
> > > 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.
> > 
> > Signin profile is currently used as "temporary profile" clearing it when
> needed.
> > If someone is unaware of this scenario, his new code would break.
> 
> What if the two features try to use temporary profiles concurrently?

I've added closing of captive portal web dialog on lock screen request.

> > > 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.
> > 
> > Yes. That is why it is disabled by default for enterprise users.
> > 
> 
> Please document this caveat in policy_templates.json. Better be explicit about
> this than to cause surprises.

Done.

[bartfab (slow), 2015-02-03 10:44:57]

> > Please document this caveat in policy_templates.json. Better be explicit
about
> > this than to cause surprises.
> 
> Done.

It does not appear to have been done. The documentation in policy_templates.json
does not explain that this feature allows the user to bypass policy.

[Alexander Alekseev, 2015-02-03 14:00:13]

On 2015/02/03 10:44:57, bartfab wrote:
> > > Please document this caveat in policy_templates.json. Better be explicit
> about
> > > this than to cause surprises.
> > 
> > Done.
> 
> It does not appear to have been done. The documentation in
policy_templates.json
> does not explain that this feature allows the user to bypass policy.

If you think that "... ignoring all proxy settings" is not enough, can you
provide the text that seems to you to be a suitable explanation?

[Alexander Alekseev, 2015-02-09 15:59:16]

bartfab@, wad@ - Ping?

[drewry, 2015-02-11 17:03:09]

On 2015/02/09 15:59:16, Alexander Alekseev wrote:
> bartfab@, wad@ - Ping?

I can look at the code later (tonight/tomorrow), but if you guys think this is
the safest, sanest approach, then go for it.  

Ideally, we don't want anything leaking out and we should keep a close eye if we
ship it as suggested.  Do we restart after use prior to actual sign-in? If not,
how likely are we to bring anything with us into the session too?  Is there a
good way to see if/how much might leak?

Is the only other option to create an ephemeral profile to work on this in then
through it away after?

thanks!

[Alexander Alekseev, 2015-02-11 17:51:33]

On 2015/02/11 17:03:09, drewry wrote:
> On 2015/02/09 15:59:16, Alexander Alekseev wrote:
> > bartfab@, wad@ - Ping?
> 
> I can look at the code later (tonight/tomorrow), but if you guys think this is
> the safest, sanest approach, then go for it.

The functionality itself was added here
https://codereview.chromium.org/750153002 .
This CL only enables it for enterprise users.
  

> Ideally, we don't want anything leaking out and we should keep a close eye if
we
> ship it as suggested.  Do we restart after use prior to actual sign-in? If
not,
> how likely are we to bring anything with us into the session too?  Is there a
> good way to see if/how much might leak?

This profile has been historically used for all non-user web requests inside
user session.
As you can see in
https://codereview.chromium.org/750153002/diff/160001/chrome/browser/chromeos...,
it is cleaned after dialog is closed.

The question is: is it safe enough?

 
> Is the only other option to create an ephemeral profile to work on this in
then
> through it away after?

AFAIK, there is no way to create 'abstract ephemeral profile'. It must be a real
user profile, or 'kiosk' profile, or one of several other specially implemented
profiles. To create another profile type we need to add (a lot of?) code to
profiles.

[drewry, 2015-02-11 20:25:37]

On 2015/02/11 17:51:33, Alexander Alekseev wrote:
> On 2015/02/11 17:03:09, drewry wrote:
> > On 2015/02/09 15:59:16, Alexander Alekseev wrote:
> > > bartfab@, wad@ - Ping?
> > 
> > I can look at the code later (tonight/tomorrow), but if you guys think this
is
> > the safest, sanest approach, then go for it.
> 
> The functionality itself was added here
> https://codereview.chromium.org/750153002 .
> This CL only enables it for enterprise users.

Ah thanks!
  
> 
> > Ideally, we don't want anything leaking out and we should keep a close eye
if
> we
> > ship it as suggested.  Do we restart after use prior to actual sign-in? If
> not,
> > how likely are we to bring anything with us into the session too?  Is there
a
> > good way to see if/how much might leak?
> 
> This profile has been historically used for all non-user web requests inside
> user session.
> As you can see in
>
https://codereview.chromium.org/750153002/diff/160001/chrome/browser/chromeos...,
> it is cleaned after dialog is closed.
> 
> The question is: is it safe enough?

If that's the same model we're using throughout, it seems reasonable to use it
here.  If it turns out to not be safe enough, then it will likely impact all
other usage too!


>  
> > Is the only other option to create an ephemeral profile to work on this in
> then
> > through it away after?
> 
> AFAIK, there is no way to create 'abstract ephemeral profile'. It must be a
real
> user profile, or 'kiosk' profile, or one of several other specially
implemented
> profiles. To create another profile type we need to add (a lot of?) code to
> profiles.


Yeah I meant a temporary user profile on ephemeral storage, but the overhead is
killer.

Seems fine to go ahead here, especially since I don't have a deep enough
understanding to be able to do a stronger assessment than you and bartfab.

Thanks!

[bartfab (slow), 2015-02-12 09:42:43]

On 2015/02/03 14:00:13, Alexander Alekseev wrote:
> On 2015/02/03 10:44:57, bartfab wrote:
> > > > Please document this caveat in policy_templates.json. Better be explicit
> > about
> > > > this than to cause surprises.
> > > 
> > > Done.
> > 
> > It does not appear to have been done. The documentation in
> policy_templates.json
> > does not explain that this feature allows the user to bypass policy.
> 
> If you think that "... ignoring all proxy settings" is not enough, can you
> provide the text that seems to you to be a suitable explanation?

I would like to see something along the lines of "... ignoring all policy
settings and restrictions for the current user." After all, it is not just proxy
settings we ignore.

[bartfab (slow), 2015-02-12 09:42:48]

On 2015/02/03 14:00:13, Alexander Alekseev wrote:
> On 2015/02/03 10:44:57, bartfab wrote:
> > > > Please document this caveat in policy_templates.json. Better be explicit
> > about
> > > > this than to cause surprises.
> > > 
> > > Done.
> > 
> > It does not appear to have been done. The documentation in
> policy_templates.json
> > does not explain that this feature allows the user to bypass policy.
> 
> If you think that "... ignoring all proxy settings" is not enough, can you
> provide the text that seems to you to be a suitable explanation?

I would like to see something along the lines of "... ignoring all policy
settings and restrictions for the current user." After all, it is not just proxy
settings we ignore.

[Alexander Alekseev, 2015-02-12 17:57:49]

On 2015/02/12 09:42:48, bartfab wrote:
> On 2015/02/03 14:00:13, Alexander Alekseev wrote:
> > On 2015/02/03 10:44:57, bartfab wrote:
> > > > > Please document this caveat in policy_templates.json. Better be
explicit
> > > about
> > > > > this than to cause surprises.
> > > > 
> > > > Done.
> > > 
> > > It does not appear to have been done. The documentation in
> > policy_templates.json
> > > does not explain that this feature allows the user to bypass policy.
> > 
> > If you think that "... ignoring all proxy settings" is not enough, can you
> > provide the text that seems to you to be a suitable explanation?
> 
> I would like to see something along the lines of "... ignoring all policy
> settings and restrictions for the current user." After all, it is not just
proxy
> settings we ignore.

Done.

[bartfab (slow), 2015-02-16 13:54:33]

policy LGTM

[Alexander Alekseev, 2015-02-16 16:29:21]

alemate@chromium.org changed reviewers:
+ nkostylev@chromium.org
- dpolukhin@chromium.org

[Alexander Alekseev, 2015-02-16 18:13:21]

New patchsets have been uploaded after l-g-t-m from bartfab@chromium.org

[Alexander Alekseev, 2015-02-17 09:21:27]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-02-17 09:22:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/818433003/240001

[commit-bot: I haz the power, 2015-02-17 09:39:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-17 09:39:47]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2015-02-17 12:24:49]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-02-17 12:25:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/818433003/260001

[commit-bot: I haz the power, 2015-02-17 13:07:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-17 13:07:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2015-03-04 17:40:36]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-03-04 17:40:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, stevenjb@chromium.org, mnissler@chromium.org,
ygorshenin@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/818433003/#ps280001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-03-04 17:41:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/818433003/280001

[Alexander Alekseev, 2015-03-04 17:41:40]

The CQ bit was unchecked by alemate@chromium.org

[Alexander Alekseev, 2015-03-04 20:10:19]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-03-04 20:10:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, stevenjb@chromium.org, mnissler@chromium.org,
ygorshenin@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/818433003/#ps300001
(title: "Tests fixed.")

[commit-bot: I haz the power, 2015-03-04 20:11:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/818433003/300001

[Alexander Alekseev, 2015-03-04 20:17:49]

The CQ bit was unchecked by alemate@chromium.org

[Alexander Alekseev, 2015-03-04 20:53:06]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-03-04 20:54:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/818433003/300001

[commit-bot: I haz the power, 2015-03-04 22:15:50]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-03-04 22:16:36]

Patchset 16 (id:??) landed as
https://crrev.com/d17ddcb8b5e5dbf9bfe481567c63a23e9cfe0db3
Cr-Commit-Position: refs/heads/master@{#319138}