Identify and repair ONC files with duplicate GUIDs

Identify and repair ONC files with duplicate GUIDs.

The patch identifies network configurations and certificates with duplicate GUIDs and repairs the ONC such that the first occurrence is retained as a unique network configuration or certificate.

BUG=211421
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251748

[kaliamoorthi, 2014-02-17 09:42:50]

PTAL

[pneubeck (no reviews), 2014-02-17 10:11:36]

A few comments but looks mostly good.

nit in the subject: GUID's -> GUIDs.

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:396: error_or_warning_found_ = true;
Log an error (it's always an error and not a warning) like in
FieldExistsAndIsEmpty. I.e.:

LOG(ERROR) << MessageHeader() << "....";

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.h:178: bool
VerifyGuidUniqueAndAddToHash(const base::DictionaryValue& dict,
Hash -> Set ?

Add a comment that explains when it returns true/false.
yes, the above functions are missing comments as well ;-) but for some it's
obvious when they return what.

Instead of "Verify" you could also use something that better indicates the
meaning of the result, like "GuidIsUnique...."

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.h:203: std::set<std::string> network_guids_;
add comments like:

Accumulates all network GUIDs during validation. Used to identify duplicate
GUIDs.

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator_unittest.cc (right):

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator_unittest.cc:342:
std::make_pair(OncParams("duplicate-cert-id",
nit: id -> guid

https://codereview.chromium.org/166903002/diff/1/chromeos/test/data/network/i...
File chromeos/test/data/network/invalid_settings_with_repairs.json (right):

https://codereview.chromium.org/166903002/diff/1/chromeos/test/data/network/i...
chromeos/test/data/network/invalid_settings_with_repairs.json:5: "GUID":
"{485d6076-dd44-6b6d-69787465725f5043}",
use a short GUID so that it's easier to see that the GUIDs are identical.

https://codereview.chromium.org/166903002/diff/1/chromeos/test/data/network/i...
chromeos/test/data/network/invalid_settings_with_repairs.json:25:
"Certificates": [],
can be removed.

https://codereview.chromium.org/166903002/diff/1/chromeos/test/data/network/i...
chromeos/test/data/network/invalid_settings_with_repairs.json:41:
"Certificates": [],
ditto

[kaliamoorthi, 2014-02-17 15:06:49]

PTAL

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:396: error_or_warning_found_ = true;
On 2014/02/17 10:11:36, pneubeck wrote:
> Log an error (it's always an error and not a warning) like in
> FieldExistsAndIsEmpty. I.e.:
> 
> LOG(ERROR) << MessageHeader() << "....";

Done.

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.h:178: bool
VerifyGuidUniqueAndAddToHash(const base::DictionaryValue& dict,
On 2014/02/17 10:11:36, pneubeck wrote:
> Hash -> Set ?
> 
> Add a comment that explains when it returns true/false.
> yes, the above functions are missing comments as well ;-) but for some it's
> obvious when they return what.
> 
> Instead of "Verify" you could also use something that better indicates the
> meaning of the result, like "GuidIsUnique...."

Done.

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.h:203: std::set<std::string> network_guids_;
On 2014/02/17 10:11:36, pneubeck wrote:
> add comments like:
> 
> Accumulates all network GUIDs during validation. Used to identify duplicate
> GUIDs.

Done.

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator_unittest.cc (right):

https://codereview.chromium.org/166903002/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator_unittest.cc:342:
std::make_pair(OncParams("duplicate-cert-id",
On 2014/02/17 10:11:36, pneubeck wrote:
> nit: id -> guid

Done.

https://codereview.chromium.org/166903002/diff/1/chromeos/test/data/network/i...
File chromeos/test/data/network/invalid_settings_with_repairs.json (right):

https://codereview.chromium.org/166903002/diff/1/chromeos/test/data/network/i...
chromeos/test/data/network/invalid_settings_with_repairs.json:5: "GUID":
"{485d6076-dd44-6b6d-69787465725f5043}",
On 2014/02/17 10:11:36, pneubeck wrote:
> use a short GUID so that it's easier to see that the GUIDs are identical.

Done.

https://codereview.chromium.org/166903002/diff/1/chromeos/test/data/network/i...
chromeos/test/data/network/invalid_settings_with_repairs.json:25:
"Certificates": [],
On 2014/02/17 10:11:36, pneubeck wrote:
> can be removed.

Done.

https://codereview.chromium.org/166903002/diff/1/chromeos/test/data/network/i...
chromeos/test/data/network/invalid_settings_with_repairs.json:41:
"Certificates": [],
On 2014/02/17 10:11:36, pneubeck wrote:
> ditto

Done.

[pneubeck (no reviews), 2014-02-17 16:07:40]

lgtm. only a few optional comments.
You should run the linux_chromeos and linux_chromeos_clang bots.

https://codereview.chromium.org/166903002/diff/130001/chromeos/network/onc/on...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/166903002/diff/130001/chromeos/network/onc/on...
chromeos/network/onc/onc_validator.cc:397: LOG(ERROR) << MessageHeader() <<
"Found a duplicate GUID.";
nit:
since the GUID isn't a credential, we can as well log it:
"Found duplicate GUID " << guid;

https://codereview.chromium.org/166903002/diff/130001/chromeos/network/onc/on...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/166903002/diff/130001/chromeos/network/onc/on...
chromeos/network/onc/onc_validator.h:178: // The function checks if the GUID is
unique. It returns true if the GUID is
nit: remove first sentence. Start with "Returns true ..."

https://codereview.chromium.org/166903002/diff/130001/chromeos/network/onc/on...
chromeos/network/onc/onc_validator.h:179: // unique or if the GUID is empty
(error handled elsewhere) and false
empty GUID isn't handled specially in this function. Remove that comment ("or if
... elsewhere)" ).

https://codereview.chromium.org/166903002/diff/130001/chromeos/test/data/netw...
File chromeos/test/data/network/invalid_settings_with_repairs.json (right):

https://codereview.chromium.org/166903002/diff/130001/chromeos/test/data/netw...
chromeos/test/data/network/invalid_settings_with_repairs.json:10: "SSID":
"CrOS_WPA_DlinkN150",
as discussed please also change the SSIDs/passphrases.

[pneubeck (no reviews), 2014-02-17 16:26:20]

https://codereview.chromium.org/166903002/diff/130001/chromeos/network/onc/on...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/166903002/diff/130001/chromeos/network/onc/on...
chromeos/network/onc/onc_validator.h:180: // otherwise. The function also adds
the GUID to a set in order to identify
... or if the field |kGUID| doesn't exist in |dict| ...

[kaliamoorthi, 2014-02-17 17:21:15]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-02-17 17:21:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/166903002/20...

[kaliamoorthi, 2014-02-17 17:22:37]

The CQ bit was unchecked by kaliamoorthi@chromium.org

[kaliamoorthi, 2014-02-18 08:57:51]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-02-18 08:57:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/166903002/20...

[commit-bot: I haz the power, 2014-02-18 11:38:08]

Change committed as 251748