Rejecting networks/certificates individually from ONC during validation.

Rejecting networks/certificates individually from ONC during validation.

Cleaned up ONC validator unit tests to scale better with more tests.

BUG=162801
TEST=Adapted the validator unit test.


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

[pneubeck (no reviews), 2012-12-10 13:24:51]

First part of rejecting network/certificates individually.
I will update the LoadOncNetworks function and the CertificateImporter in a
separate CL.

(This CL is based on https://codereview.chromium.org/11469026/)

[Mattias Nissler (ping if slow), 2012-12-13 09:45:18]

https://codereview.chromium.org/11428078/diff/16001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator_unittest.cc (right):

https://codereview.chromium.org/11428078/diff/16001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator_unittest.cc:69: struct OncParams {
migt want to hide this is in an anonymous namespace.

https://codereview.chromium.org/11428078/diff/16001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator_unittest.cc:73: :
location(location_of_object),
How about renaming all these "location" variables to "filename"? That would make
it immediately obvious what it is :)

https://codereview.chromium.org/11428078/diff/16001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator_unittest.cc:159:
scoped_ptr<base::DictionaryValue> LoadObject(const std::string &name) {
Rename to LoadDictionaryFromFile maybe?

[pneubeck (no reviews), 2012-12-13 12:50:02]

Addressed Mattias' comments.

https://codereview.chromium.org/11428078/diff/16001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator_unittest.cc (right):

https://codereview.chromium.org/11428078/diff/16001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator_unittest.cc:69: struct OncParams {
On 2012/12/13 09:45:18, Mattias Nissler wrote:
> migt want to hide this is in an anonymous namespace.

Done.

https://codereview.chromium.org/11428078/diff/16001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator_unittest.cc:73: :
location(location_of_object),
On 2012/12/13 09:45:18, Mattias Nissler wrote:
> How about renaming all these "location" variables to "filename"? That would
make
> it immediately obvious what it is :)

Because it's either a filename or a key in the dictionary shared over several
test cases.

I added a comment here and at each test case instantiation.

https://codereview.chromium.org/11428078/diff/16001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator_unittest.cc:159:
scoped_ptr<base::DictionaryValue> LoadObject(const std::string &name) {
On 2012/12/13 09:45:18, Mattias Nissler wrote:
> Rename to LoadDictionaryFromFile maybe?
Then "name" could be misinterpreted as the filename the data is loaded from.

Changed to GetDictionaryFromTestFile and added a comment. Is that clearer?

[stevenjb, 2012-12-14 01:06:36]

https://codereview.chromium.org/11428078/diff/26001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/11428078/diff/26001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:193: *nested_error =
nested_error_in_current_array;
It seems odd that nested_error is only sometimes set. This should either set it
to false in an else clause, or we should document that it doesn't always set it
(but I much prefer the former).

[Greg Spencer (Chromium), 2012-12-14 18:15:11]

lgtm

[pneubeck (no reviews), 2012-12-14 18:55:45]

Addressed Steven's comment.

https://codereview.chromium.org/11428078/diff/26001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/11428078/diff/26001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:193: *nested_error =
nested_error_in_current_array;
On 2012/12/14 01:06:36, stevenjb (chromium) wrote:
> It seems odd that nested_error is only sometimes set. This should either set
it
> to false in an else clause, or we should document that it doesn't always set
it
> (but I much prefer the former).

That's currently the behavior of all other methods in Validator and Mapper.
Clarified that |error| or |nested_error| is _only_ set if an error occurred.

The original reason was:
- The caller has to initialize the variable in anyway.
- This way, an error variable can be used for several methods in sequence and
the result is the OR of all errors.
(see e.g. Mapper::MapArray)

If we change it here, we must/should change all methods.

[stevenjb, 2012-12-14 20:21:39]

lgtm

https://codereview.chromium.org/11428078/diff/26001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/11428078/diff/26001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:193: *nested_error =
nested_error_in_current_array;
On 2012/12/14 18:55:46, pneubeck wrote:
> On 2012/12/14 01:06:36, stevenjb (chromium) wrote:
> > It seems odd that nested_error is only sometimes set. This should either set
> it
> > to false in an else clause, or we should document that it doesn't always set
> it
> > (but I much prefer the former).
> 
> That's currently the behavior of all other methods in Validator and Mapper.
> Clarified that |error| or |nested_error| is _only_ set if an error occurred.
> 
> The original reason was:
> - The caller has to initialize the variable in anyway.
> - This way, an error variable can be used for several methods in sequence and
> the result is the OR of all errors.
> (see e.g. Mapper::MapArray)
> 
> If we change it here, we must/should change all methods.
OK, as long as the comment is clear that's fine.

[commit-bot: I haz the power, 2012-12-16 21:43:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/11428078/33001

[commit-bot: I haz the power, 2012-12-17 00:32:40]

Change committed as 173404