Add unittests for HSTS decoding.

Add unittests for HSTS decoding.

Add end-to-end tests for the decoding of the preloaded transport security
state. Specifically, test that the Chromium code (net::DecodeHSTSPreload) can
extract the data the generator encoded in the preloaded data.

Review-Url: https://codereview.chromium.org/2680933009
Cr-Commit-Position: refs/heads/master@{#458485}
Committed: https://chromium.googlesource.com/chromium/src/+/d369e670383cdee6cb66a4469b36e5308ed00aa2

[martijnc, 2017-02-10 19:10:43]

Patchset #1 (id:1) has been deleted

[martijnc, 2017-02-10 20:58:45]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 20:59:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 22:30:16]

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

[commit-bot: I haz the power, 2017-02-10 22:30:17]

Dry run: This issue passed the CQ dry run.

[martijnc, 2017-02-13 18:27:25]

Description was changed from

==========
Add unittests for HSTS decoding.

BUG=595493
==========

to

==========
Add unittests for HSTS decoding.

Add end-to-end tests for the decoding of the preloaded transport security
state. Specifically, test that the Chromium code (net::DecodeHSTSPreload) can
extract the data the generator encoded in the preloaded data.
==========

[martijnc, 2017-02-13 18:27:26]

martijn@martijnc.be changed reviewers:
+ rsleevi@chromium.org

[martijnc, 2017-03-13 21:56:14]

Patchset #4 (id:80001) has been deleted

[martijnc, 2017-03-13 21:56:18]

Patchset #4 (id:90001) has been deleted

[Ryan Sleevi, 2017-03-17 22:17:38]

So we're in the home stretch!

I avoided the LG on this because it may have the wrong base revision, since it
looks like it overlaps with some of the other CLs. If you want to update that,
and then pass it over to one of the reviewers below, I'll happily stamp the
tests if they're happy with the build files.

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:2017:
TEST_F(TransportSecurityStateTest, DecodePreloadedSingle) {
Would you be willing to add prosaic definitions to each of these just
elaborating on what's being tested? and/or inline documentation?

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:2033: EXPECT_EQ(GURL(""),
pkp_state.report_uri);
Is GURL("") different than GURL()?

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:2045:
SetTransportSecurityStateSourceForTesting(nullptr);
This won't reset if any of the ASSERT_EQ's fail (e.g. line 2034)

Suggestion: Either make the test fixture reset in its dtor or use an RAII helper
to scope it (e.g.

class ScopedTransportSecurityState {
 public:
  explicit Scoped...(...Source* source) {
    SetTransportSecurity...(source);
  }
  ~Scoped...() {
    SetTransportSecurity...(nullptr);
  }
};

Either works, dtor is probably easier and simpler :)

https://codereview.chromium.org/2680933009/diff/120001/net/tools/transport_se...
File net/tools/transport_security_state_generator/BUILD.gn (right):

https://codereview.chromium.org/2680933009/diff/120001/net/tools/transport_se...
net/tools/transport_security_state_generator/BUILD.gn:45:
"../../http/transport_security_state_static_unittest3.json",
So, I'm not sure this is the right or recommended GN style. Could you get one of
the GN folks to review this and offer suggestions? I don't know enough if
there's a GN styleguide.

I suspect brettw or dpranke

[martijnc, 2017-03-18 19:57:01]

Patchset #6 (id:140001) has been deleted

[martijnc, 2017-03-18 20:03:57]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 20:04:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[martijnc, 2017-03-18 20:49:27]

Patchset #6 (id:160001) has been deleted

[martijnc, 2017-03-18 20:58:53]

Patchset #6 (id:180001) has been deleted

[martijnc, 2017-03-18 21:03:25]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 21:03:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[martijnc, 2017-03-18 21:25:35]

The CQ bit was unchecked by martijn@martijnc.be

[martijnc, 2017-03-18 21:28:15]

martijn@martijnc.be changed reviewers:
+ brettw@chromium.org

[martijnc, 2017-03-18 21:36:05]

+brettw: can you take a look at the GN files (see comments)?

Thanks!

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:2017:
TEST_F(TransportSecurityStateTest, DecodePreloadedSingle) {
On 2017/03/17 at 22:17:37, Ryan Sleevi wrote:
> Would you be willing to add prosaic definitions to each of these just
elaborating on what's being tested? and/or inline documentation?

Added a comment to each test explaining what is being tested. The tests itself
are mostly expectations that map almost 1-to-1 to the content of the related
.json files so haven't added inline comments but let me know if I left things
unclear :).

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:2033: EXPECT_EQ(GURL(""),
pkp_state.report_uri);
On 2017/03/17 at 22:17:37, Ryan Sleevi wrote:
> Is GURL("") different than GURL()?

Changed.

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:2045:
SetTransportSecurityStateSourceForTesting(nullptr);
On 2017/03/17 at 22:17:37, Ryan Sleevi wrote:
> This won't reset if any of the ASSERT_EQ's fail (e.g. line 2034)
> 
> Suggestion: Either make the test fixture reset in its dtor or use an RAII
helper to scope it (e.g.
> 
> class ScopedTransportSecurityState {
>  public:
>   explicit Scoped...(...Source* source) {
>     SetTransportSecurity...(source);
>   }
>   ~Scoped...() {
>     SetTransportSecurity...(nullptr);
>   }
> };
> 
> Either works, dtor is probably easier and simpler :)

Moved it to the test harness but placed it in testing::Test::TearDown (I think
that's correct as well?).

https://codereview.chromium.org/2680933009/diff/120001/net/tools/transport_se...
File net/tools/transport_security_state_generator/BUILD.gn (right):

https://codereview.chromium.org/2680933009/diff/120001/net/tools/transport_se...
net/tools/transport_security_state_generator/BUILD.gn:45:
"../../http/transport_security_state_static_unittest3.json",
On 2017/03/17 at 22:17:37, Ryan Sleevi wrote:
> So, I'm not sure this is the right or recommended GN style. Could you get one
of the GN folks to review this and offer suggestions? I don't know enough if
there's a GN styleguide.
> 
> I suspect brettw or dpranke

Updated this to not use relative paths. Also added brettw for a GN review.

[brettw, 2017-03-20 20:42:41]

It's not a hard requirement, but the build files should generally go with the
source files they refer to.

The way I would have done it is:
  in net/tools/transport_se... have the executable with an assert at the top:
  assert(current_toolchain == host_toolchain)

And have the compiled_action_foreach in net/http/BUILD.gn. This should eliminate
most of the path contortions you're doing.

LGTM

https://codereview.chromium.org/2680933009/diff/200001/net/tools/transport_se...
File net/tools/transport_security_state_generator/BUILD.gn (right):

https://codereview.chromium.org/2680933009/diff/200001/net/tools/transport_se...
net/tools/transport_security_state_generator/BUILD.gn:1: # Copyright (c) 2017
The Chromium Authors. All rights reserved.
uber-nit: The current reference copyright block has no "(c)"

[martijnc, 2017-03-20 21:08:41]

Patchset #7 (id:220001) has been deleted

[martijnc, 2017-03-20 21:10:40]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 21:11:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[martijnc, 2017-03-20 21:46:25]

Thanks for the feedback. I've moved the GN configs around like you proposed.

https://codereview.chromium.org/2680933009/diff/200001/net/tools/transport_se...
File net/tools/transport_security_state_generator/BUILD.gn (right):

https://codereview.chromium.org/2680933009/diff/200001/net/tools/transport_se...
net/tools/transport_security_state_generator/BUILD.gn:1: # Copyright (c) 2017
The Chromium Authors. All rights reserved.
On 2017/03/20 at 20:42:41, brettw (plz ping after 24h) wrote:
> uber-nit: The current reference copyright block has no "(c)"

Removed.

[Ryan Sleevi, 2017-03-21 03:24:36]

LGTM % nit

Note: It's good to split rebasing and feedback into separate uploads -
preferably, feedback-then-rebase, because it lets the reviewer review the
delta-diffs between a patchset to see the feedback being addressed :)

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:2045:
SetTransportSecurityStateSourceForTesting(nullptr);
On 2017/03/18 21:36:04, martijnc wrote:
> On 2017/03/17 at 22:17:37, Ryan Sleevi wrote:
> > This won't reset if any of the ASSERT_EQ's fail (e.g. line 2034)
> > 
> > Suggestion: Either make the test fixture reset in its dtor or use an RAII
> helper to scope it (e.g.
> > 
> > class ScopedTransportSecurityState {
> >  public:
> >   explicit Scoped...(...Source* source) {
> >     SetTransportSecurity...(source);
> >   }
> >   ~Scoped...() {
> >     SetTransportSecurity...(nullptr);
> >   }
> > };
> > 
> > Either works, dtor is probably easier and simpler :)
> 
> Moved it to the test harness but placed it in testing::Test::TearDown (I think
> that's correct as well?).

Prefer DTOR -
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...
:)

[martijnc, 2017-03-21 17:05:45]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 17:06:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[martijnc, 2017-03-21 17:19:18]

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/2680933009/diff/120001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:2045:
SetTransportSecurityStateSourceForTesting(nullptr);
On 2017/03/21 at 03:24:36, Ryan Sleevi (conf till 3-24) wrote:
> Prefer DTOR -
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...
:)

Changed.

I had only read the item about SetUp and TearDown in the GTest guide and it
seemed like a more common pattern in the codebase so I went with that. Thanks
for the pointer!

[commit-bot: I haz the power, 2017-03-21 18:20:48]

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

[commit-bot: I haz the power, 2017-03-21 18:20:49]

Dry run: This issue passed the CQ dry run.

[martijnc, 2017-03-21 18:23:37]

The CQ bit was checked by martijn@martijnc.be

[martijnc, 2017-03-21 18:23:37]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2680933009/#ps280001
(title: "Comment rsleevi.")

[commit-bot: I haz the power, 2017-03-21 18:24:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 18:36:50]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1490120617007780,
"parent_rev": "3428b2d3e81169a97f933c7631b8194baf43d66b", "commit_rev":
"d369e670383cdee6cb66a4469b36e5308ed00aa2"}

[commit-bot: I haz the power, 2017-03-21 18:37:37]

Description was changed from

==========
Add unittests for HSTS decoding.

Add end-to-end tests for the decoding of the preloaded transport security
state. Specifically, test that the Chromium code (net::DecodeHSTSPreload) can
extract the data the generator encoded in the preloaded data.
==========

to

==========
Add unittests for HSTS decoding.

Add end-to-end tests for the decoding of the preloaded transport security
state. Specifically, test that the Chromium code (net::DecodeHSTSPreload) can
extract the data the generator encoded in the preloaded data.

Review-Url: https://codereview.chromium.org/2680933009
Cr-Commit-Position: refs/heads/master@{#458485}
Committed:
https://chromium.googlesource.com/chromium/src/+/d369e670383cdee6cb66a4469b36...
==========

[commit-bot: I haz the power, 2017-03-21 18:37:38]

Committed patchset #9 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/d369e670383cdee6cb66a4469b36...