Execute ClientCertResolver on network disconnection.

Execute ClientCertResolver on network disconnection.

The previous behavior was that the certificate resolver was executed
only in a few circumstances: directly after login, when a certificate is
installed/removed, when the network list changes. This means that if a
certificate expired and none of the above happened before a next network
connection is initiated, then the network configuration in Network
Manager would still contain the certificate. This, in turn, means that
the managed users won't see the "Get new certificate" dialog (the Wi-Fi
profile setup dialog will appear instead).

So the solution is to trigger the certificate resolver when the network
disconnection happens, so that the expired certificate is removed before
the next connect attempt.

BUG=551380
Committed: https://crrev.com/98181cd7a5d2745589fe47e63e8926b32b7f1832
Cr-Commit-Position: refs/heads/master@{#360110}

[emaxx, 2015-11-16 20:27:08]

emaxx@chromium.org changed reviewers:
+ stevenjb@chromium.org

[emaxx, 2015-11-16 20:27:09]

Steven, PTAL.

[stevenjb, 2015-11-17 00:45:22]

https://codereview.chromium.org/1443043002/diff/20001/chromeos/network/client...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/1443043002/diff/20001/chromeos/network/client...
chromeos/network/client_cert_resolver_unittest.cc:55: test_clock_(NULL),
s/NULL/nullptr throughout

https://codereview.chromium.org/1443043002/diff/20001/chromeos/network/client...
chromeos/network/client_cert_resolver_unittest.cc:147:
client_cert_resolver_->SetClockForTesting(make_scoped_ptr(test_clock_));
This is dangerous. client_cert_resolver_ now owns test_clock_, even though this
class hangs on to a pointer to it.

Either:
a) The test should own the clock, pass a raw * to ClientCertResolver (which
would also store it as a raw *), then call
client_cert_resolver_->SetClockForTesting(nullptr) when it destroys it
b) ClientCertResolver should have a test_clock() accessor and this class
shouldn't store the pointer.

a) is probably simplest.

[emaxx, 2015-11-17 12:44:07]

https://codereview.chromium.org/1443043002/diff/20001/chromeos/network/client...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/1443043002/diff/20001/chromeos/network/client...
chromeos/network/client_cert_resolver_unittest.cc:55: test_clock_(NULL),
On 2015/11/17 00:45:22, stevenjb wrote:
> s/NULL/nullptr throughout

Done.

https://codereview.chromium.org/1443043002/diff/20001/chromeos/network/client...
chromeos/network/client_cert_resolver_unittest.cc:147:
client_cert_resolver_->SetClockForTesting(make_scoped_ptr(test_clock_));
On 2015/11/17 00:45:22, stevenjb wrote:
> This is dangerous. client_cert_resolver_ now owns test_clock_, even though
this
> class hangs on to a pointer to it.
> 
> Either:
> a) The test should own the clock, pass a raw * to ClientCertResolver (which
> would also store it as a raw *), then call
> client_cert_resolver_->SetClockForTesting(nullptr) when it destroys it
> b) ClientCertResolver should have a test_clock() accessor and this class
> shouldn't store the pointer.
> 
> a) is probably simplest.

Implemented a) now. Though this looks pretty similar to the previous version:
basically, there's one thing having a pointer to another one with the lifetime
managed somewhere else.

[stevenjb, 2015-11-17 17:53:52]

lgtm

https://codereview.chromium.org/1443043002/diff/20001/chromeos/network/client...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/1443043002/diff/20001/chromeos/network/client...
chromeos/network/client_cert_resolver_unittest.cc:147:
client_cert_resolver_->SetClockForTesting(make_scoped_ptr(test_clock_));
On 2015/11/17 12:44:07, emaxx wrote:
> On 2015/11/17 00:45:22, stevenjb wrote:
> > This is dangerous. client_cert_resolver_ now owns test_clock_, even though
> this
> > class hangs on to a pointer to it.
> > 
> > Either:
> > a) The test should own the clock, pass a raw * to ClientCertResolver (which
> > would also store it as a raw *), then call
> > client_cert_resolver_->SetClockForTesting(nullptr) when it destroys it
> > b) ClientCertResolver should have a test_clock() accessor and this class
> > shouldn't store the pointer.
> > 
> > a) is probably simplest.
> 
> Implemented a) now. Though this looks pretty similar to the previous version:
> basically, there's one thing having a pointer to another one with the lifetime
> managed somewhere else.

I acknowledge it's a subtle distinction, but I think this is easier to read.
Before I had to look at ClientCertResolver to confirm that it is OK for this
class to hold onto a raw * after passing a scoped_ptr to the class. Now the
lifetime of both client_cert_resolver_ and test_clock_ is wholly controlled by
this class.

[emaxx, 2015-11-17 17:59:47]

The CQ bit was checked by emaxx@chromium.org

[commit-bot: I haz the power, 2015-11-17 18:02:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1443043002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1443043002/40001

[commit-bot: I haz the power, 2015-11-17 18:08:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-17 18:09:01]

Patchset 3 (id:??) landed as
https://crrev.com/98181cd7a5d2745589fe47e63e8926b32b7f1832
Cr-Commit-Position: refs/heads/master@{#360110}