Fix ONC password filtering in about:policy.

Fix ONC password filtering in about:policy.

BUG=chromium:138227
R=joaodasilva@chromium.org, pneubeck@chromium.org


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150784

[Mattias Nissler (ping if slow), 2012-08-08 15:40:08]

Joao, can you review?

[Joao da Silva, 2012-08-09 07:29:33]

lgtm

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
File chrome/browser/policy/configuration_policy_handler_chromeos.cc (right):

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:127: {
onc::kWiFi, onc::wifi::kPassphrase },
Suggestion: align the 2nd and 3rd column entries?

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:138:
static_cast<const char*>(NULL)));
I'm not sure if the memory bots won't complain about reading uninitialized
values; maybe add explicit NULLs?

[pneubeck (no reviews), 2012-08-09 07:59:20]

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
File chrome/browser/policy/configuration_policy_handler_chromeos.cc (right):

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:5: #include
"chrome/browser/policy/configuration_policy_handler_chromeos.h"
Nit:
btw. the header is missing at least a forward declaration of DictionaryValue.
base/values.h should be included explicitly in .cc.
'base' namespace could be added to references to types/functions of values.h

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:119:
std::vector<std::string> filtered_settings;
Unused? If so, also remove the comment above.

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:121: static const
char* kFilteredSettings[][kMaxComponents] = {
Add a comment, that these are the components of a dictionary path which are to
be garbled. Each entry must be NULL-terminated.

Overall, the terms "filter" and "strip" are not used correctly, IMO. Instead the
sensitive data is "garbled"/"hidden"/"starred"...

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:141:
network_dict->Set(key, Value::CreateStringValue(kPlaceholder));
Maybe create the StringValue only once.

[Mattias Nissler (ping if slow), 2012-08-09 09:09:14]

PTAL

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
File chrome/browser/policy/configuration_policy_handler_chromeos.cc (right):

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:119:
std::vector<std::string> filtered_settings;
On 2012/08/09 07:59:20, pneubeck1 wrote:
> Unused? If so, also remove the comment above.

Done.

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:121: static const
char* kFilteredSettings[][kMaxComponents] = {
On 2012/08/09 07:59:20, pneubeck1 wrote:
> Add a comment, that these are the components of a dictionary path which are to
> be garbled. Each entry must be NULL-terminated.
> 
> Overall, the terms "filter" and "strip" are not used correctly, IMO. Instead
the
> sensitive data is "garbled"/"hidden"/"starred"...

Done.

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:127: {
onc::kWiFi, onc::wifi::kPassphrase },
On 2012/08/09 07:29:33, Joao da Silva wrote:
> Suggestion: align the 2nd and 3rd column entries?

That would suggest the entries correspond to each other when in fact they're
not.

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:138:
static_cast<const char*>(NULL)));
On 2012/08/09 07:29:33, Joao da Silva wrote:
> I'm not sure if the memory bots won't complain about reading uninitialized
> values; maybe add explicit NULLs?

The C++ standard says that missing elements in array initializers are
initialized to zeros.

http://codereview.chromium.org/10823234/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:141:
network_dict->Set(key, Value::CreateStringValue(kPlaceholder));
On 2012/08/09 07:59:20, pneubeck1 wrote:
> Maybe create the StringValue only once.

No, we need fresh copies for each Set()

[Joao da Silva, 2012-08-09 09:24:52]

Still lgtm, with a couple of nits.

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler_chromeos.cc (right):

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:9: #include
<vector>
Nit: <algorithm> and <vector> not used anymore

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:92:
DictionaryValue* config_dict =
Nit: base::

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:93:
static_cast<DictionaryValue*>(json_value.get());
Nit: base::

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:103:
MaskSensitiveValues(static_cast<DictionaryValue*>(*network_entry));
Nit: base::

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:113: return
Value::CreateStringValue(json_string);
Nit: base::

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:118:
DictionaryValue* network_dict) {
Nit: base::

[Mattias Nissler (ping if slow), 2012-08-09 11:32:42]

Fixed all the nits, I'll tick the box.

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler_chromeos.cc (right):

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:9: #include
<vector>
On 2012/08/09 09:24:52, Joao da Silva wrote:
> Nit: <algorithm> and <vector> not used anymore

Done.

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:92:
DictionaryValue* config_dict =
On 2012/08/09 09:24:52, Joao da Silva wrote:
> Nit: base::

Done.

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:93:
static_cast<DictionaryValue*>(json_value.get());
On 2012/08/09 09:24:52, Joao da Silva wrote:
> Nit: base::

Done.

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:103:
MaskSensitiveValues(static_cast<DictionaryValue*>(*network_entry));
On 2012/08/09 09:24:52, Joao da Silva wrote:
> Nit: base::

Done.

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:113: return
Value::CreateStringValue(json_string);
On 2012/08/09 09:24:52, Joao da Silva wrote:
> Nit: base::

Done.

http://codereview.chromium.org/10823234/diff/6001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:118:
DictionaryValue* network_dict) {
On 2012/08/09 09:24:52, Joao da Silva wrote:
> Nit: base::

Done.

[commit-bot: I haz the power, 2012-08-09 11:32:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/10823234/1003

[commit-bot: I haz the power, 2012-08-09 13:13:12]

Change committed as 150784