Add back prefs::kSSL3Enabled and prefs::kTLS1Enabled, but control

[This CL is abandoned.  Replaced by http://codereview.chromium.org/8402019/ ]

Add back prefs::kSSL3Enabled and prefs::kTLS1Enabled, but control
the preferences with the command-line options via the
CommandLinePrefStore.  This allows us to control the preferences
via the PolicyPrefStores or any other pref store in the future.

R=rsleevi@chromium.org
BUG=93554
TEST=none

[wtc, 2011-08-27 05:46:12]

rsleevi: please review.  This CL is based on two CLs:

your http://codereview.chromium.org/7462008
palmer's http://codereview.chromium.org/7766004

[Ryan Sleevi, 2011-08-27 14:14:42]

LGTM

http://codereview.chromium.org/7776002/diff/1/chrome/browser/net/ssl_config_s...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/7776002/diff/1/chrome/browser/net/ssl_config_s...
chrome/browser/net/ssl_config_service_manager_pref.cc:142: bool ssl3_enabled_;
nit: You can probably simplify things just using a BooleanPrefMember, but that's
up to you if you want to. If you do go that route, you can delete lines 129-30,
165-166, 197-200, 227-234

http://codereview.chromium.org/7776002/diff/1/chrome/browser/net/ssl_config_s...
chrome/browser/net/ssl_config_service_manager_pref.cc:199: if (*pref_name_in ==
prefs::kTLS1Enabled)
nit: else if here and line 201?

[wtc, 2011-08-27 20:27:06]

rsleevi: I made the changes you suggested.  Please review
Patch Set 2.  I have a question below.

http://codereview.chromium.org/7776002/diff/1/chrome/browser/net/ssl_config_s...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/7776002/diff/1/chrome/browser/net/ssl_config_s...
chrome/browser/net/ssl_config_service_manager_pref.cc:142: bool ssl3_enabled_;
Thanks a lot for the suggestion of using a BooleanPrefMember.

I think the two pref_change_registrar_.Add() calls on
lines 161-162 should also be deleted, right?

161 pref_change_registrar_.Add(prefs::kSSL3Enabled, this);
162 pref_change_registrar_.Add(prefs::kTLS1Enabled, this);

[Ryan Sleevi, 2011-08-27 20:30:03]

On 2011/08/27 20:27:06, wtc wrote:
> rsleevi: I made the changes you suggested.  Please review
> Patch Set 2.  I have a question below.
> 
>
http://codereview.chromium.org/7776002/diff/1/chrome/browser/net/ssl_config_s...
> File chrome/browser/net/ssl_config_service_manager_pref.cc (right):
> 
>
http://codereview.chromium.org/7776002/diff/1/chrome/browser/net/ssl_config_s...
> chrome/browser/net/ssl_config_service_manager_pref.cc:142: bool ssl3_enabled_;
> Thanks a lot for the suggestion of using a BooleanPrefMember.
> 
> I think the two pref_change_registrar_.Add() calls on
> lines 161-162 should also be deleted, right?
> 
> 161 pref_change_registrar_.Add(prefs::kSSL3Enabled, this);
> 162 pref_change_registrar_.Add(prefs::kTLS1Enabled, this);

Correct, those are replaced by the .Init() calls on 155-156.

[wtc, 2011-08-27 20:41:02]

Thanks for the confirmation.  Patch Set 3 adds two lines
to command_line_pref_store.cc and reverts the other three
files to r98500.

[Ryan Sleevi, 2011-08-27 20:46:31]

LGTM

[wtc, 2011-08-27 22:14:33]

http://codereview.chromium.org/7776002/diff/6003/chrome/browser/net/ssl_confi...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/7776002/diff/6003/chrome/browser/net/ssl_confi...
chrome/browser/net/ssl_config_service_manager_pref.cc:156:
tls1_enabled_.Init(prefs::kTLS1Enabled, local_state, this);
rsleevi: I just realized that I forgot to ask you an important
question: how do I make sure that ssl3_enabled_ and tls1_enabled_
ignore any preference values the user may have previously
stored in Local State?  Now we want ssl3_enabled_ and
tls1_enabled_ to default to true and can only be reset
by command-line options or group policy in the future.

[Ryan Sleevi, 2011-08-27 22:21:45]

http://codereview.chromium.org/7776002/diff/6003/chrome/browser/net/ssl_confi...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/7776002/diff/6003/chrome/browser/net/ssl_confi...
chrome/browser/net/ssl_config_service_manager_pref.cc:156:
tls1_enabled_.Init(prefs::kTLS1Enabled, local_state, this);
That is a good question. I don't know what the method is for resetting a local
state preference, but I would imagine there is one, considering the about:flags
resets that have happened.

In the worst case, renaming the preferences would work, but that seems a very
heavy-handed approach. It might be best to ask one of the Policy people how this
is dealt with.

[Chris Palmer, 2011-08-31 18:30:52]

I tested this patch out and it didn't violate my expectation, so LGTM. (My
expectation was that --disable-tls1 and --disable-ssl3 would not persist across
launches — that these command line switches apply to the current session only.)

[wtc, 2011-10-09 15:18:51]

http://codereview.chromium.org/7776002/diff/6003/chrome/browser/net/ssl_confi...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/7776002/diff/6003/chrome/browser/net/ssl_confi...
chrome/browser/net/ssl_config_service_manager_pref.cc:156:
tls1_enabled_.Init(prefs::kTLS1Enabled, local_state, this);

rsleevi,palmer: thank you for testing the patch, and I'm sorry
that I didn't get back to this CL until now.

Unfortunately my experiments showed what I feared is true:
if the user's "Local State" file has an entry for the
ssl.ssl3.enabled or ssl.tls1.enabled preference, it is
honored.  Although the command-line options can override
the entries in Local State as expected, this is still bad
because there is no UI to correct an old
ssl.tls1.enabled=false entry in Local State now.

I will need to ask the Policy people if there is a way
to deal with this, or we will need to rename the preferences
(for example, ssl.ssl3.disabled and ssl.tls1.disabled).
Ideally I'd like a way to remove or ignore the old
preference entries in Local State.

[Ryan Sleevi, 2011-10-10 01:37:22]

http://codereview.chromium.org/7776002/diff/6003/chrome/browser/net/ssl_confi...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/7776002/diff/6003/chrome/browser/net/ssl_confi...
chrome/browser/net/ssl_config_service_manager_pref.cc:176: }
wtc: The pattern I've seen is to call prefs->ClearPref(prefs::kTLS1Enabled),
which will clear the existing value from |prefs|, causing it to return the
default (default_config.tls1_enabled)

You can see usage at
http://codesearch.google.com/codesearch#search&q=ClearPref&exact_package=chro...

You should be able to confirm with the policy people the correctness of this.

[wtc, 2011-10-11 16:49:44]

rsleevi: please review Patch Set 5.  It's enough to just
review the diffs between Patch Sets 3 and 5 in the first
two files, and then review the changes to pref_names.{h,cc}
in Patch Set 5 directly.

Thanks for the great suggestion.  mnissler confirmed that's
the right solution.

[Ryan Sleevi, 2011-10-11 22:56:27]

wtc: Glad to hear it worked. The implementation LGTM, and if you've confirmed it
works as expected, fantastic.

I'm not sure, but is it worth writing up a unit test for this?

I don't know how familiar you are with the pref code, and I'm admittedly not too
familiar with it, but I believe you should be able to write a test that uses
PrefServiceMockBuilder (src/chrome/browser/prefs/pref_service_mock_builder.h)
that can combine several TestingPrefStores, using WithCommandLinePrefs() and
WithUserPrefs(), to simulate a user who has an existing local-state pref along
with a command-line backed pref, to ensure everything behaves as expected.

If you would rather do this in a follow-up, could you file a Type=Cleanup bug?

[wtc, 2011-10-12 19:33:45]

rsleevi: thanks for the code review.

Since you brought up the dreaded "unit test" word, the rule says I need
to write one.  I am not familiar with the pref code.  I will need to
deal with it.

[Ryan Sleevi, 2011-10-26 22:30:54]

On 2011/10/12 19:33:45, wtc wrote:
> rsleevi: thanks for the code review.
> 
> Since you brought up the dreaded "unit test" word, the rule says I need
> to write one.  I am not familiar with the pref code.  I will need to
> deal with it.

wtc: Do you need a hand writing a unittest? I can upload a quick one if you're
not yet caught up on the pref code.

[wtc, 2011-10-27 00:00:53]

On 2011/10/26 22:30:54, Ryan Sleevi wrote:
> 
> wtc: Do you need a hand writing a unittest? I can upload a quick one if you're
> not yet caught up on the pref code.

Thank you for offering to help.  I don't have time to study the
pref code to write a unit test, so I put this CL on the backburner.
(This CL is just a cleanup unless we need to control these two
settings with policy.)

If you have written a test, please take over this CL so you can
get the credit.  Thanks.