Get ClientCertStore through ResourceContext.

Get ClientCertStore through ResourceContext.

Make ClientCertStore::GetClientCerts asynchronous.

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

[mattm, 2013-10-25 01:32:46]

jam: content/public/browser/, chrome/browser/chrome_content_browser_client*

simonjam: content/browser/loader/

abarth: content/browser/ssl/

wtc: net/

rsleevi: just fyi

This is just a refactoring, shouldn't change behavior. 
(See https://codereview.chromium.org/18121007/ if you're curious how it will be
used, specifically:
https://codereview.chromium.org/18121007/diff/154001/chrome/browser/chromeos/...
and 
https://codereview.chromium.org/18121007/diff/154001/chrome/browser/chrome_co...
)

This can also be considered a follow up to:
https://chromiumcodereview.appspot.com/12035105
It implements the "make it asynchronous" part of the future work Ryan mentioned,
and is one step towards the request pooling idea (the ContentBrowserClient could
keep a single ClientCertStore object instead of creating a new one for each
request).

[jam, 2013-10-25 01:43:10]

https://codereview.chromium.org/42773002/diff/90001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/42773002/diff/90001/content/public/browser/co...
content/public/browser/content_browser_client.h:402: ResourceContext* context);
since this is a property of ResourceContext, this should be on the
ResourceContext interface.

[mattm, 2013-10-25 02:53:46]

https://codereview.chromium.org/42773002/diff/90001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/42773002/diff/90001/content/public/browser/co...
content/public/browser/content_browser_client.h:402: ResourceContext* context);
On 2013/10/25 01:43:10, jam wrote:
> since this is a property of ResourceContext, this should be on the
> ResourceContext interface.

Done.

[jam, 2013-10-25 15:33:57]

lgtm with nits

https://codereview.chromium.org/42773002/diff/320002/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/42773002/diff/320002/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:27: #include
"net/ssl/client_cert_store.h"
nit: not needed in the header

https://codereview.chromium.org/42773002/diff/320002/content/browser/loader/r...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/42773002/diff/320002/content/browser/loader/r...
content/browser/loader/resource_loader.cc:19: #include
"content/public/browser/content_browser_client.h"
nit: not needed anymore

https://codereview.chromium.org/42773002/diff/320002/content/public/browser/r...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/42773002/diff/320002/content/public/browser/r...
content/public/browser/resource_context.h:57: virtual
scoped_ptr<net::ClientCertStore> GetClientCertStore() = 0;
This isn't your fault, but this interface doesn't conform to a convention I had
added after more experience with the content api: every interface that is
implemented by an embedder should have empty implementations. for this method,
do you mind not making this pure virtual, and putting the empty implementation
in resource_context_impl.cc? then you wouldn't need to touch
mock_resource_context.h, shell_browser_context.*, aw_browser_context.cc


nit: can you put this on line 46? that way the interface has two sections, first
being getters, then access checks

[James Simonsen, 2013-10-25 16:30:56]

lgtm for loader/

https://codereview.chromium.org/42773002/diff/320002/content/browser/loader/r...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/42773002/diff/320002/content/browser/loader/r...
content/browser/loader/resource_loader.cc:76: is_transferring_ = false;
Why not put these in the initializer list?

[mattm, 2013-10-25 19:34:35]

https://codereview.chromium.org/42773002/diff/320002/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/42773002/diff/320002/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.h:27: #include
"net/ssl/client_cert_store.h"
On 2013/10/25 15:33:58, jam wrote:
> nit: not needed in the header

Done.

https://codereview.chromium.org/42773002/diff/320002/content/browser/loader/r...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/42773002/diff/320002/content/browser/loader/r...
content/browser/loader/resource_loader.cc:19: #include
"content/public/browser/content_browser_client.h"
On 2013/10/25 15:33:58, jam wrote:
> nit: not needed anymore

Done.

https://codereview.chromium.org/42773002/diff/320002/content/browser/loader/r...
content/browser/loader/resource_loader.cc:76: is_transferring_ = false;
On 2013/10/25 16:30:56, James Simonsen wrote:
> Why not put these in the initializer list?

Done.

https://codereview.chromium.org/42773002/diff/320002/content/public/browser/r...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/42773002/diff/320002/content/public/browser/r...
content/public/browser/resource_context.h:57: virtual
scoped_ptr<net::ClientCertStore> GetClientCertStore() = 0;
On 2013/10/25 15:33:58, jam wrote:
> This isn't your fault, but this interface doesn't conform to a convention I
had
> added after more experience with the content api: every interface that is
> implemented by an embedder should have empty implementations. for this method,
> do you mind not making this pure virtual, and putting the empty implementation
> in resource_context_impl.cc? then you wouldn't need to touch
> mock_resource_context.h, shell_browser_context.*, aw_browser_context.cc

Oh yeah, that's much nicer. Done.

 
> nit: can you put this on line 46? that way the interface has two sections,
first
> being getters, then access checks

Done

[wtc, 2013-10-28 19:41:12]

Patch set 7 LGTM. I reviewed the whole CL.

https://codereview.chromium.org/42773002/diff/750001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/42773002/diff/750001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:717: #if !defined(USE_OPENSSL)

Could you add a comment to explain this? This seems to imply SSL client auth is
not supported if USE_OPENSSL is defined, but that's not the case. So this is a
little confusing to me.

https://codereview.chromium.org/42773002/diff/750001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:718: return
scoped_ptr<net::ClientCertStore>(new net::ClientCertStoreImpl());

This looks more like a "CreateClientCertStore" method rather than a
"GetClientCertStore" method. I find this a little confusing.

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.cc:52: GotClientCerts();

Nit: I recommend curly braces because the body of the "if" branch spans multiple
lines.

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.cc:72: if (client_cert_store_ &&
cert_request_info_->client_certs.empty()) {

IMPORTANT: if client_cert_store_ is NULL, does not imply
cert_request_info_->client_certs.empty()?

If seems that we should be testing
    if (cert_request_info_->client_certs.empty()) {
or
    if (!client_cert_store_ || cert_request_info_->client_certs.empty()) {

so that we won't fall through if client_cert_store_ is null.

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_client_auth_handler.h (right):

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.h:34:
SSLClientAuthHandler(ResourceContext* context,

It may be better to just pass the ClientCertStore because the only thing we do
with |context| is to call context->GetClientCertStore().

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.h:61: void GotClientCerts();

Nit: our naming convention for this kind of function seems to be
"DidGetClientCerts".

https://codereview.chromium.org/42773002/diff/750001/content/public/browser/r...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/42773002/diff/750001/content/public/browser/r...
content/public/browser/resource_context.h:47: virtual
scoped_ptr<net::ClientCertStore> GetClientCertStore();

It is strange to see this method returns a scoped_ptr whereas the two preceding
methods return a plain pointer.

Why isn't this method an abstract method (= 0)?

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_store.h
File net/ssl/client_cert_store.h (right):

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store.h:27: const base::Closure& callback) = 0;

Nit: If I only look at this interface and its comments, I won't know how the
method works in synchronous and asynchronous modes.

Two things are unclear to me.

1. What happens to selected_certs. In particular, what happens to selected_certs
in asynchronous mode.

2. When will |callback| be called.

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_mac.cc (right):

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_mac.cc:212: return;

Does this change means we can no longer report an error. Is this OK because
reporting an error would not be useful, so to the user an error is
indistinguishable from an empty selected_certs?

Note: This comment also applies to the _win.cc file. (The version of this method
in the _nss.cc file doesn't return false in the original code.)

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.

My comments in this file also apply to the _mac.cc and _win.cc files.

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:23: bool GetClientCertsImpl(CERTCertList*
cert_list,

This function always returns true. It should be changed to return void. This
will avoid the question of why GetClientCerts can ignore the return value of
this function.

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:89: callback.Run();

Nit: it may be a good idea to clear selected_certs explicitly in this case. I
believe we are relying on the caller passing an empty selected_certs to us.

[mattm, 2013-10-28 23:56:15]

https://codereview.chromium.org/42773002/diff/750001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/42773002/diff/750001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:717: #if !defined(USE_OPENSSL)
On 2013/10/28 19:41:12, wtc wrote:
> 
> Could you add a comment to explain this? This seems to imply SSL client auth
is
> not supported if USE_OPENSSL is defined, but that's not the case. So this is a
> little confusing to me.

Done.

https://codereview.chromium.org/42773002/diff/750001/chrome/browser/profiles/...
chrome/browser/profiles/profile_io_data.cc:718: return
scoped_ptr<net::ClientCertStore>(new net::ClientCertStoreImpl());
On 2013/10/28 19:41:12, wtc wrote:
> 
> This looks more like a "CreateClientCertStore" method rather than a
> "GetClientCertStore" method. I find this a little confusing.

Changed to CreateClientCertStore. If we implement the idea about pooling, the
name can be changed, since the return type would also need to be changed at the
same time anyway.

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.cc:52: GotClientCerts();
On 2013/10/28 19:41:12, wtc wrote:
> 
> Nit: I recommend curly braces because the body of the "if" branch spans
multiple
> lines.

Done.

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.cc:72: if (client_cert_store_ &&
cert_request_info_->client_certs.empty()) {
On 2013/10/28 19:41:12, wtc wrote:
> 
> IMPORTANT: if client_cert_store_ is NULL, does not imply
> cert_request_info_->client_certs.empty()?
> 
> If seems that we should be testing
>     if (cert_request_info_->client_certs.empty()) {
> or
>     if (!client_cert_store_ || cert_request_info_->client_certs.empty()) {
> 
> so that we won't fall through if client_cert_store_ is null.

This fallthrough is for android, where there isn't a ClientCertStore, and the
matching is done as part of the android code. Added a comment.

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_client_auth_handler.h (right):

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.h:34:
SSLClientAuthHandler(ResourceContext* context,
On 2013/10/28 19:41:12, wtc wrote:
> 
> It may be better to just pass the ClientCertStore because the only thing we do
> with |context| is to call context->GetClientCertStore().

Done.

https://codereview.chromium.org/42773002/diff/750001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_client_auth_handler.h:61: void GotClientCerts();
On 2013/10/28 19:41:12, wtc wrote:
> 
> Nit: our naming convention for this kind of function seems to be
> "DidGetClientCerts".

Done.

https://codereview.chromium.org/42773002/diff/750001/content/public/browser/r...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/42773002/diff/750001/content/public/browser/r...
content/public/browser/resource_context.h:47: virtual
scoped_ptr<net::ClientCertStore> GetClientCertStore();
On 2013/10/28 19:41:12, wtc wrote:
> 
> It is strange to see this method returns a scoped_ptr whereas the two
preceding
> methods return a plain pointer.

Should be addressed by the name change.

> Why isn't this method an abstract method (= 0)?

John mentioned, the new convention for content interfaces is to provide an empty
implementation instead of pure virtual. The other ones are from before that
convention.

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_store.h
File net/ssl/client_cert_store.h (right):

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store.h:27: const base::Closure& callback) = 0;
On 2013/10/28 19:41:12, wtc wrote:
> 
> Nit: If I only look at this interface and its comments, I won't know how the
> method works in synchronous and asynchronous modes.
> 
> Two things are unclear to me.
> 
> 1. What happens to selected_certs. In particular, what happens to
selected_certs
> in asynchronous mode.
> 
> 2. When will |callback| be called.

Expanded the comment

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_mac.cc (right):

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_mac.cc:212: return;
On 2013/10/28 19:41:12, wtc wrote:
> 
> Does this change means we can no longer report an error. Is this OK because
> reporting an error would not be useful, so to the user an error is
> indistinguishable from an empty selected_certs?
> 
> Note: This comment also applies to the _win.cc file. (The version of this
method
> in the _nss.cc file doesn't return false in the original code.)

Yeah, the UI wasn't actually using the error return code, so I just removed it
to keep things simple.

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/10/28 19:41:12, wtc wrote:
> 
> My comments in this file also apply to the _mac.cc and _win.cc files.

Done.

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:23: bool GetClientCertsImpl(CERTCertList*
cert_list,
On 2013/10/28 19:41:12, wtc wrote:
> 
> This function always returns true. It should be changed to return void. This
> will avoid the question of why GetClientCerts can ignore the return value of
> this function.

Well, the return value is still checked by the unittest which calls
SelectClientCertsForTesting instead of GetClientCerts.

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:89: callback.Run();
On 2013/10/28 19:41:12, wtc wrote:
> 
> Nit: it may be a good idea to clear selected_certs explicitly in this case. I
> believe we are relying on the caller passing an empty selected_certs to us.

Done.

[wtc, 2013-10-29 23:03:11]

Patch set 8 LGTM.

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:23: bool GetClientCertsImpl(CERTCertList*
cert_list,
On 2013/10/28 23:56:16, mattm wrote:
>
> Well, the return value is still checked by the unittest which calls
> SelectClientCertsForTesting instead of GetClientCerts.

I checked all three implementations of GetClientCertsImpl. All of them always
return true. So the return value isn't useful, even if some caller checks it.

https://codereview.chromium.org/42773002/diff/1040001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/42773002/diff/1040001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:716:
ProfileIOData::ResourceContext::CreateClientCertStore() {

"OpenClientCertStore" could also be a good name for this method. What do you
think?

https://codereview.chromium.org/42773002/diff/1040001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/42773002/diff/1040001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:270: // Verifies if a call to
net::UrlRequest::Delegate::OnCertificateRequested()

Typo: UrlRequest => URLRequest

https://codereview.chromium.org/42773002/diff/1040001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:279:
resource_context_.GetRequestContext()));

Nit: this may fit on the previous line.

https://codereview.chromium.org/42773002/diff/1040001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:288: // need to keep a raw
pointer copy to access this objects later.

Typo: this objects => this object

https://codereview.chromium.org/42773002/diff/1040001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store.h (right):

https://codereview.chromium.org/42773002/diff/1040001/net/ssl/client_cert_sto...
net/ssl/client_cert_store.h:25: // The |callback| may be called sychronously.

This implies the memory that |selected_certs| points to must remain valid until
|callback| is called, right?

[mattm, 2013-10-29 23:59:00]

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/42773002/diff/750001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:23: bool GetClientCertsImpl(CERTCertList*
cert_list,
On 2013/10/29 23:03:12, wtc wrote:
> On 2013/10/28 23:56:16, mattm wrote:
> >
> > Well, the return value is still checked by the unittest which calls
> > SelectClientCertsForTesting instead of GetClientCerts.
> 
> I checked all three implementations of GetClientCertsImpl. All of them always
> return true. So the return value isn't useful, even if some caller checks it.

Ah, good point.  Done.

https://codereview.chromium.org/42773002/diff/1040001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/42773002/diff/1040001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:716:
ProfileIOData::ResourceContext::CreateClientCertStore() {
On 2013/10/29 23:03:12, wtc wrote:
> 
> "OpenClientCertStore" could also be a good name for this method. What do you
> think?

That would also work, though to me "open" implies that it is doing something
more than it actually does. I think Create is okay for the current design.

https://codereview.chromium.org/42773002/diff/1040001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/42773002/diff/1040001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:270: // Verifies if a call to
net::UrlRequest::Delegate::OnCertificateRequested()
On 2013/10/29 23:03:12, wtc wrote:
> 
> Typo: UrlRequest => URLRequest

Done.

https://codereview.chromium.org/42773002/diff/1040001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:279:
resource_context_.GetRequestContext()));
On 2013/10/29 23:03:12, wtc wrote:
> 
> Nit: this may fit on the previous line.

Done.

https://codereview.chromium.org/42773002/diff/1040001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:288: // need to keep a raw
pointer copy to access this objects later.
On 2013/10/29 23:03:12, wtc wrote:
> 
> Typo: this objects => this object

Done.

https://codereview.chromium.org/42773002/diff/1040001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store.h (right):

https://codereview.chromium.org/42773002/diff/1040001/net/ssl/client_cert_sto...
net/ssl/client_cert_store.h:25: // The |callback| may be called sychronously.
On 2013/10/29 23:03:12, wtc wrote:
> 
> This implies the memory that |selected_certs| points to must remain valid
until
> |callback| is called, right?

Yeah. I'll update comment to make that explicit too.

[commit-bot: I haz the power, 2013-10-30 00:25:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/42773002/1250015

[commit-bot: I haz the power, 2013-10-30 01:05:47]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-10-30 02:03:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/42773002/1250015

[commit-bot: I haz the power, 2013-10-30 04:46:25]

Change committed as 231750