Cancel client auth requests when not promptable.

content/browser/ssl/ssl_client_auth_handler.cc

Index: content/browser/ssl/ssl_client_auth_handler.cc
diff --git a/content/browser/ssl/ssl_client_auth_handler.cc b/content/browser/ssl/ssl_client_auth_handler.cc
index d8edc5df7f588a9946728d2cf3169d7b7ece8112..91ae68445c316384d2f63acc5c5c0f39b74300be 100644
--- a/content/browser/ssl/ssl_client_auth_handler.cc
+++ b/content/browser/ssl/ssl_client_auth_handler.cc
@@ -7,8 +7,11 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/client_certificate_delegate.h"
 #include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/resource_request_info.h"
+#include "content/public/browser/web_contents.h"
 #include "net/cert/x509_certificate.h"
 #include "net/ssl/client_cert_store.h"
 #include "net/url_request/url_request.h"
@@ -17,26 +20,53 @@ namespace content {
 
 namespace {
 
-void CertificateSelectedOnUIThread(
-    const SSLClientAuthHandler::CertificateCallback& io_thread_callback,
-    net::X509Certificate* cert) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+class ClientCertificateDelegateImpl : public ClientCertificateDelegate {
+ public:
+  ClientCertificateDelegateImpl(

[mmenke, 2015/02/10 17:19:11]

explicit

[davidben, 2015/02/10 20:28:49]

Done.

+      const base::WeakPtr<SSLClientAuthHandler>& handler)
+      : handler_(handler) {}
+
+  // ClientCertificateDelegate implementation:
+  void ContinueWithCertificate(net::X509Certificate* cert) override {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&SSLClientAuthHandler::ContinueWithCertificate, handler_,
+                   make_scoped_refptr(cert)));
+  }
 
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(io_thread_callback, make_scoped_refptr(cert)));
-}
+  void CancelCertificateSelection() override {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&SSLClientAuthHandler::CancelCertificateSelection,
+                   handler_));
+  }
+
+ private:
+  base::WeakPtr<SSLClientAuthHandler> handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(ClientCertificateDelegateImpl);
+};
 
 void SelectCertificateOnUIThread(
     int render_process_host_id,
     int render_frame_host_id,
     net::SSLCertRequestInfo* cert_request_info,
-    const SSLClientAuthHandler::CertificateCallback& io_thread_callback) {
+    const base::WeakPtr<SSLClientAuthHandler>& handler) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  scoped_ptr<ClientCertificateDelegate> delegate(
+      new ClientCertificateDelegateImpl(handler));
+
+  RenderFrameHost* rfh =
+      RenderFrameHost::FromID(render_process_host_id, render_frame_host_id);
+  WebContents* web_contents = WebContents::FromRenderFrameHost(rfh);
+  if (!web_contents) {

[mmenke, 2015/02/10 17:19:10]

Is there a reason for moving this logic here?  Just wondering if there's a
reason an embedder would want to do anything different in this case.

[davidben, 2015/02/10 20:28:49]

Partly so this can be tested within content. Partly so the API surface can be
WebContents, which is cleaner (and gets us closer to moving it to
WebContentsDelegate). Partly because I don't think there's any reason an
embedder would want to change it, yeah. I feel how a SharedWorker interacts with
network prompts like this (which WebContents we map it to, etc) is a web
platform decision, and so it's part of content.

+    delegate->CancelCertificateSelection();
+    return;
+  }
+
   GetContentClient()->browser()->SelectClientCertificate(
-      render_process_host_id, render_frame_host_id, cert_request_info,
-      base::Bind(&CertificateSelectedOnUIThread, io_thread_callback));
+      web_contents, cert_request_info, delegate.Pass());
 }
 
 }  // namespace
@@ -88,10 +118,10 @@ SSLClientAuthHandler::SSLClientAuthHandler(
     scoped_ptr<net::ClientCertStore> client_cert_store,
     net::URLRequest* request,
     net::SSLCertRequestInfo* cert_request_info,
-    const SSLClientAuthHandler::CertificateCallback& callback)
+    SSLClientAuthHandler::Delegate* delegate)
     : request_(request),
       cert_request_info_(cert_request_info),
-      callback_(callback),
+      delegate_(delegate),
       weak_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -109,17 +139,40 @@ void SSLClientAuthHandler::SelectCertificate() {
   core_->GetClientCerts();
 }
 
+// static
+void SSLClientAuthHandler::ContinueWithCertificate(
+    const base::WeakPtr<SSLClientAuthHandler>& handler,
+    net::X509Certificate* cert) {
+  if (!handler)
+    return;
+  Delegate* delegate = handler->delegate_;
+  delegate->ContinueWithCertificate(cert);

[mmenke, 2015/02/10 17:19:10]

nit:  Gere, and below, can we just merge these two lines?

[davidben, 2015/02/10 20:28:50]

Done.

+}
+
+// static
+void SSLClientAuthHandler::CancelCertificateSelection(
+    const base::WeakPtr<SSLClientAuthHandler>& handler) {
+  if (!handler)
+    return;
+  Delegate* delegate = handler->delegate_;
+  delegate->CancelCertificateSelection();
+}
+
 void SSLClientAuthHandler::DidGetClientCerts() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // Note that if |client_cert_store_| is NULL, we intentionally fall through to
-  // DoCertificateSelected. This is for platforms where the client cert matching
-  // is not performed by Chrome. Those platforms handle the cert matching before
-  // showing the dialog.
+  // SelectCertificateOnUIThread. This is for platforms where the client cert
+  // matching is not performed by Chrome. Those platforms handle the cert
+  // matching before showing the dialog.
   if (core_->has_client_cert_store() &&
       cert_request_info_->client_certs.empty()) {
     // No need to query the user if there are no certs to choose from.
-    CertificateSelected(NULL);
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&SSLClientAuthHandler::ContinueWithCertificate,
+                   weak_factory_.GetWeakPtr(),
+                   scoped_refptr<net::X509Certificate>()));

[mmenke, 2015/02/10 17:19:10]

optional:  Worth creating the ClientCertificateDelegateImpl here, and passing it
to the UI thread if needed?  Means the callbacks only go through one location. 
Do make the delegate's threaded-ness kinda weird, though.

[davidben, 2015/02/10 20:28:49]

So, I do think the WebContents check should come before the empty
ClientCertStore check rather than after---I think ClientCertStore should be
something only the embedder knows about to begin with. I'm a little worried
about this breaking sites that request no client auth and expect things to
silently proceed, so I've added a TODO and will do it as a follow-up.

[mmenke, 2015/02/10 20:39:20]

I'm not sure you understood me.  I was suggesting creating the
ClientCertificateDelegateImpl in DidGetClientCerts, and using it instead of
directly calling PostTask.

That having been said, I agree that the order of the checks (And Android's
behavior in general here) seems really odd.

[davidben, 2015/02/10 22:20:52]

Ah. I'm not sure it's that much clearer; I think I'd prefer that object not live
on two threads.

     return;
   }
 
@@ -128,7 +181,10 @@ void SSLClientAuthHandler::DidGetClientCerts() {
   if (!ResourceRequestInfo::ForRequest(request_)->GetAssociatedRenderFrame(
           &render_process_host_id, &render_frame_host_id)) {
     NOTREACHED();
-    CertificateSelected(NULL);
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&SSLClientAuthHandler::CancelCertificateSelection,
+                   weak_factory_.GetWeakPtr()));
     return;
   }
 
@@ -136,16 +192,7 @@ void SSLClientAuthHandler::DidGetClientCerts() {
       BrowserThread::UI, FROM_HERE,
       base::Bind(&SelectCertificateOnUIThread, render_process_host_id,
                  render_frame_host_id, cert_request_info_,
-                 base::Bind(&SSLClientAuthHandler::CertificateSelected,
-                            weak_factory_.GetWeakPtr())));
-}
-
-void SSLClientAuthHandler::CertificateSelected(net::X509Certificate* cert) {
-  DVLOG(1) << this << " DoCertificateSelected " << cert;
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-  callback_.Run(cert);
-  // |this| may be deleted at this point.
+                 weak_factory_.GetWeakPtr()));
 }
 
 }  // namespace content