diff options
45 files changed, 1026 insertions, 367 deletions
diff --git a/android_webview/browser/aw_content_browser_client.cc b/android_webview/browser/aw_content_browser_client.cc index fb89fc6..a4db9971 100644 --- a/android_webview/browser/aw_content_browser_client.cc +++ b/android_webview/browser/aw_content_browser_client.cc @@ -28,6 +28,7 @@ #include "content/public/browser/browser_message_filter.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/child_process_security_policy.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/permission_type.h" #include "content/public/browser/render_process_host.h" #include "content/public/browser/render_view_host.h" @@ -368,17 +369,13 @@ void AwContentBrowserClient::AllowCertificateError( } void AwContentBrowserClient::SelectClientCertificate( - int render_process_id, - int render_frame_id, - net::SSLCertRequestInfo* cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback) { + content::WebContents* web_contents, + net::SSLCertRequestInfo* cert_request_info, + scoped_ptr<content::ClientCertificateDelegate> delegate) { AwContentsClientBridgeBase* client = - AwContentsClientBridgeBase::FromID(render_process_id, render_frame_id); - if (client) { - client->SelectClientCertificate(cert_request_info, callback); - } else { - callback.Run(NULL); - } + AwContentsClientBridgeBase::FromWebContents(web_contents); + if (client) + client->SelectClientCertificate(cert_request_info, delegate.Pass()); } void AwContentBrowserClient::RequestPermission( diff --git a/android_webview/browser/aw_content_browser_client.h b/android_webview/browser/aw_content_browser_client.h index cae1bf3..c61c2c4 100644 --- a/android_webview/browser/aw_content_browser_client.h +++ b/android_webview/browser/aw_content_browser_client.h @@ -102,10 +102,9 @@ class AwContentBrowserClient : public content::ContentBrowserClient { const base::Callback<void(bool)>& callback, content::CertificateRequestResultType* result) override; void SelectClientCertificate( - int render_process_id, - int render_frame_id, + content::WebContents* web_contents, net::SSLCertRequestInfo* cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback) override; + scoped_ptr<content::ClientCertificateDelegate> delegate) override; void RequestPermission( content::PermissionType permission, content::WebContents* web_contents, diff --git a/android_webview/browser/aw_contents_client_bridge_base.h b/android_webview/browser/aw_contents_client_bridge_base.h index a24aa4b..265004e 100644 --- a/android_webview/browser/aw_contents_client_bridge_base.h +++ b/android_webview/browser/aw_contents_client_bridge_base.h @@ -5,13 +5,14 @@ #ifndef ANDROID_WEBVIEW_BROWSER_AW_CONTENTS_CLIENT_BRIDGE_BASE_H_ #define ANDROID_WEBVIEW_BROWSER_AW_CONTENTS_CLIENT_BRIDGE_BASE_H_ -#include "base/callback_forward.h" +#include "base/memory/scoped_ptr.h" #include "base/supports_user_data.h" #include "content/public/browser/javascript_dialog_manager.h" class GURL; namespace content { +class ClientCertificateDelegate; class WebContents; } @@ -29,8 +30,6 @@ namespace android_webview { // native/ from browser/ layer. class AwContentsClientBridgeBase { public: - typedef base::Callback<void(net::X509Certificate*)> SelectCertificateCallback; - // Adds the handler to the UserData registry. static void Associate(content::WebContents* web_contents, AwContentsClientBridgeBase* handler); @@ -48,7 +47,7 @@ class AwContentsClientBridgeBase { bool* cancel_request) = 0; virtual void SelectClientCertificate( net::SSLCertRequestInfo* cert_request_info, - const SelectCertificateCallback& callback) = 0; + scoped_ptr<content::ClientCertificateDelegate> delegate) = 0; virtual void RunJavaScriptDialog( content::JavaScriptMessageType message_type, diff --git a/android_webview/native/aw_contents_client_bridge.cc b/android_webview/native/aw_contents_client_bridge.cc index 9d68603..c54b8a1 100644 --- a/android_webview/native/aw_contents_client_bridge.cc +++ b/android_webview/native/aw_contents_client_bridge.cc @@ -10,7 +10,9 @@ #include "base/android/jni_array.h" #include "base/android/jni_string.h" #include "base/callback_helpers.h" +#include "base/macros.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/render_process_host.h" #include "content/public/browser/render_view_host.h" #include "content/public/browser/web_contents.h" @@ -58,11 +60,18 @@ AwContentsClientBridge::~AwContentsClientBridge() { JNIEnv* env = AttachCurrentThread(); ScopedJavaLocalRef<jobject> obj = java_ref_.get(env); - if (obj.is_null()) - return; - // Clear the weak reference from the java peer to the native object since - // it is possible that java object lifetime can exceed the AwContens. - Java_AwContentsClientBridge_setNativeContentsClientBridge(env, obj.obj(), 0); + if (!obj.is_null()) { + // Clear the weak reference from the java peer to the native object since + // it is possible that java object lifetime can exceed the AwContens. + Java_AwContentsClientBridge_setNativeContentsClientBridge(env, obj.obj(), + 0); + } + + for (IDMap<content::ClientCertificateDelegate>::iterator iter( + &pending_client_cert_request_delegates_); + !iter.IsAtEnd(); iter.Advance()) { + delete iter.GetCurrentValue(); + } } void AwContentsClientBridge::AllowCertificateError( @@ -114,13 +123,13 @@ void AwContentsClientBridge::ProceedSslError(JNIEnv* env, jobject obj, // This method is inspired by SelectClientCertificate() in // chrome/browser/ui/android/ssl_client_certificate_request.cc void AwContentsClientBridge::SelectClientCertificate( - net::SSLCertRequestInfo* cert_request_info, - const SelectCertificateCallback& callback) { + net::SSLCertRequestInfo* cert_request_info, + scoped_ptr<content::ClientCertificateDelegate> delegate) { DCHECK_CURRENTLY_ON(BrowserThread::UI); // Add the callback to id map. - int request_id = pending_client_cert_request_callbacks_.Add( - new SelectCertificateCallback(callback)); + int request_id = + pending_client_cert_request_delegates_.Add(delegate.release()); // Make sure callback is run on error. base::ScopedClosureRunner guard(base::Bind( &AwContentsClientBridge::HandleErrorInClientCertificateResponse, @@ -196,19 +205,24 @@ void AwContentsClientBridge::ProvideClientCertificateResponse( jobject private_key_ref) { DCHECK_CURRENTLY_ON(BrowserThread::UI); - SelectCertificateCallback* callback = - pending_client_cert_request_callbacks_.Lookup(request_id); - DCHECK(callback); + content::ClientCertificateDelegate* delegate = + pending_client_cert_request_delegates_.Lookup(request_id); + DCHECK(delegate); + + if (encoded_chain_ref == NULL || private_key_ref == NULL) { + LOG(ERROR) << "No client certificate selected"; + pending_client_cert_request_delegates_.Remove(request_id); + delegate->ContinueWithCertificate(nullptr); + delete delegate; + return; + } // Make sure callback is run on error. base::ScopedClosureRunner guard(base::Bind( &AwContentsClientBridge::HandleErrorInClientCertificateResponse, base::Unretained(this), request_id)); - if (encoded_chain_ref == NULL || private_key_ref == NULL) { - LOG(ERROR) << "Client certificate request cancelled"; - return; - } + // Convert the encoded chain to a vector of strings. std::vector<std::string> encoded_chain_strings; if (encoded_chain_ref) { @@ -236,20 +250,20 @@ void AwContentsClientBridge::ProvideClientCertificateResponse( return; } + // Release the guard and |pending_client_cert_request_delegates_| references + // to |delegate|. + pending_client_cert_request_delegates_.Remove(request_id); + ignore_result(guard.Release()); + // RecordClientCertificateKey() must be called on the I/O thread, - // before the callback is called with the selected certificate on + // before the delegate is called with the selected certificate on // the UI thread. content::BrowserThread::PostTaskAndReply( - content::BrowserThread::IO, - FROM_HERE, - base::Bind(&RecordClientCertificateKey, - client_cert, + content::BrowserThread::IO, FROM_HERE, + base::Bind(&RecordClientCertificateKey, client_cert, base::Passed(&private_key)), - base::Bind(*callback, client_cert)); - pending_client_cert_request_callbacks_.Remove(request_id); - - // Release the guard. - ignore_result(guard.Release()); + base::Bind(&content::ClientCertificateDelegate::ContinueWithCertificate, + base::Owned(delegate), client_cert)); } void AwContentsClientBridge::RunJavaScriptDialog( @@ -377,10 +391,11 @@ void AwContentsClientBridge::CancelJsResult(JNIEnv*, jobject, int id) { // Use to cleanup if there is an error in client certificate response. void AwContentsClientBridge::HandleErrorInClientCertificateResponse( int request_id) { - SelectCertificateCallback* callback = - pending_client_cert_request_callbacks_.Lookup(request_id); - callback->Run(nullptr); - pending_client_cert_request_callbacks_.Remove(request_id); + content::ClientCertificateDelegate* delegate = + pending_client_cert_request_delegates_.Lookup(request_id); + pending_client_cert_request_delegates_.Remove(request_id); + + delete delegate; } bool RegisterAwContentsClientBridge(JNIEnv* env) { diff --git a/android_webview/native/aw_contents_client_bridge.h b/android_webview/native/aw_contents_client_bridge.h index 2a5caba..a2bde4b 100644 --- a/android_webview/native/aw_contents_client_bridge.h +++ b/android_webview/native/aw_contents_client_bridge.h @@ -40,7 +40,7 @@ class AwContentsClientBridge : public AwContentsClientBridgeBase { bool* cancel_request) override; void SelectClientCertificate( net::SSLCertRequestInfo* cert_request_info, - const SelectCertificateCallback& callback) override; + scoped_ptr<content::ClientCertificateDelegate> delegate) override; void RunJavaScriptDialog( content::JavaScriptMessageType message_type, @@ -73,8 +73,10 @@ class AwContentsClientBridge : public AwContentsClientBridgeBase { IDMap<CertErrorCallback, IDMapOwnPointer> pending_cert_error_callbacks_; IDMap<content::JavaScriptDialogManager::DialogClosedCallback, IDMapOwnPointer> pending_js_dialog_callbacks_; - IDMap<SelectCertificateCallback, IDMapOwnPointer> - pending_client_cert_request_callbacks_; + // |pending_client_cert_request_delegates_| owns its pointers, but IDMap + // doesn't provide Release, so ownership is managed manually. + IDMap<content::ClientCertificateDelegate> + pending_client_cert_request_delegates_; }; bool RegisterAwContentsClientBridge(JNIEnv* env); diff --git a/android_webview/native/aw_contents_client_bridge_unittest.cc b/android_webview/native/aw_contents_client_bridge_unittest.cc index b301f91..4b59144 100644 --- a/android_webview/native/aw_contents_client_bridge_unittest.cc +++ b/android_webview/native/aw_contents_client_bridge_unittest.cc @@ -8,8 +8,10 @@ #include "base/android/jni_array.h" #include "base/android/scoped_java_ref.h" #include "base/bind.h" +#include "base/macros.h" #include "base/memory/scoped_ptr.h" #include "base/run_loop.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/test/test_browser_thread_bundle.h" #include "jni/MockAwContentsClientBridge_jni.h" #include "net/android/net_jni_registrar.h" @@ -33,9 +35,6 @@ namespace { // Tests the android_webview contents client bridge. class AwContentsClientBridgeTest : public Test { public: - typedef AwContentsClientBridge::SelectCertificateCallback - SelectCertificateCallback; - AwContentsClientBridgeTest() { } // Callback method called when a cert is selected. @@ -53,6 +52,24 @@ class AwContentsClientBridgeTest : public Test { JNIEnv* env_; }; +class TestClientCertificateDelegate + : public content::ClientCertificateDelegate { + public: + explicit TestClientCertificateDelegate(AwContentsClientBridgeTest* test) + : test_(test) {} + + // content::ClientCertificateDelegate. + void ContinueWithCertificate(net::X509Certificate* cert) override { + test_->CertSelected(cert); + test_ = nullptr; + } + + private: + AwContentsClientBridgeTest* test_; + + DISALLOW_COPY_AND_ASSIGN(TestClientCertificateDelegate); +}; + } // namespace void AwContentsClientBridgeTest::SetUp() { @@ -90,9 +107,7 @@ void AwContentsClientBridgeTest::TestCertType(SSLClientCertType type, cert_request_info_->cert_key_types.push_back(type); bridge_->SelectClientCertificate( cert_request_info_.get(), - base::Bind( - &AwContentsClientBridgeTest::CertSelected, - base::Unretained(static_cast<AwContentsClientBridgeTest*>(this)))); + make_scoped_ptr(new TestClientCertificateDelegate(this))); base::RunLoop().RunUntilIdle(); EXPECT_EQ(0, cert_selected_callbacks_); ScopedJavaLocalRef<jobjectArray> key_types = @@ -112,10 +127,8 @@ TEST_F(AwContentsClientBridgeTest, // Call SelectClientCertificate to create a callback id that mock java object // can call on. bridge_->SelectClientCertificate( - cert_request_info_.get(), - base::Bind( - &AwContentsClientBridgeTest::CertSelected, - base::Unretained(static_cast<AwContentsClientBridgeTest*>(this)))); + cert_request_info_.get(), + make_scoped_ptr(new TestClientCertificateDelegate(this))); bridge_->ProvideClientCertificateResponse(env_, jbridge_.obj(), Java_MockAwContentsClientBridge_getRequestId(env_, jbridge_.obj()), Java_MockAwContentsClientBridge_createTestCertChain( @@ -133,10 +146,8 @@ TEST_F(AwContentsClientBridgeTest, // Call SelectClientCertificate to create a callback id that mock java object // can call on. bridge_->SelectClientCertificate( - cert_request_info_.get(), - base::Bind( - &AwContentsClientBridgeTest::CertSelected, - base::Unretained(static_cast<AwContentsClientBridgeTest*>(this)))); + cert_request_info_.get(), + make_scoped_ptr(new TestClientCertificateDelegate(this))); int requestId = Java_MockAwContentsClientBridge_getRequestId(env_, jbridge_.obj()); bridge_->ProvideClientCertificateResponse(env_, jbridge_.obj(), diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc index a92c059..a8c7829 100644 --- a/chrome/browser/chrome_content_browser_client.cc +++ b/chrome/browser/chrome_content_browser_client.cc @@ -116,6 +116,7 @@ #include "content/public/browser/browser_url_handler.h" #include "content/public/browser/child_process_data.h" #include "content/public/browser/child_process_security_policy.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/permission_type.h" #include "content/public/browser/render_frame_host.h" #include "content/public/browser/render_process_host.h" @@ -1861,21 +1862,11 @@ void ChromeContentBrowserClient::AllowCertificateError( } void ChromeContentBrowserClient::SelectClientCertificate( - int render_process_id, - int render_frame_id, + content::WebContents* web_contents, net::SSLCertRequestInfo* cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback) { - content::RenderFrameHost* rfh = content::RenderFrameHost::FromID( - render_process_id, render_frame_id); - WebContents* tab = WebContents::FromRenderFrameHost(rfh); - if (!tab) { - // TODO(davidben): This makes the request hang, but returning no certificate - // also breaks. It should abort the request. See https://crbug.com/417092 - return; - } - + scoped_ptr<content::ClientCertificateDelegate> delegate) { prerender::PrerenderContents* prerender_contents = - prerender::PrerenderContents::FromWebContents(tab); + prerender::PrerenderContents::FromWebContents(web_contents); if (prerender_contents) { prerender_contents->Destroy( prerender::FINAL_STATUS_SSL_CLIENT_CERTIFICATE_REQUESTED); @@ -1887,7 +1878,8 @@ void ChromeContentBrowserClient::SelectClientCertificate( << "Invalid URL string: https://" << cert_request_info->host_and_port.ToString(); - Profile* profile = Profile::FromBrowserContext(tab->GetBrowserContext()); + Profile* profile = + Profile::FromBrowserContext(web_contents->GetBrowserContext()); scoped_ptr<base::Value> filter = profile->GetHostContentSettingsMap()->GetWebsiteSetting( requesting_url, @@ -1907,7 +1899,7 @@ void ChromeContentBrowserClient::SelectClientCertificate( for (size_t i = 0; i < all_client_certs.size(); ++i) { if (CertMatchesFilter(*all_client_certs[i].get(), *filter_dict)) { // Use the first certificate that is matched by the filter. - callback.Run(all_client_certs[i].get()); + delegate->ContinueWithCertificate(all_client_certs[i].get()); return; } } @@ -1916,7 +1908,8 @@ void ChromeContentBrowserClient::SelectClientCertificate( } } - chrome::ShowSSLClientCertificateSelector(tab, cert_request_info, callback); + chrome::ShowSSLClientCertificateSelector(web_contents, cert_request_info, + delegate.Pass()); } void ChromeContentBrowserClient::AddCertificate( diff --git a/chrome/browser/chrome_content_browser_client.h b/chrome/browser/chrome_content_browser_client.h index 9166286..8e28784 100644 --- a/chrome/browser/chrome_content_browser_client.h +++ b/chrome/browser/chrome_content_browser_client.h @@ -166,10 +166,9 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient { const base::Callback<void(bool)>& callback, content::CertificateRequestResultType* request) override; void SelectClientCertificate( - int render_process_id, - int render_frame_id, + content::WebContents* web_contents, net::SSLCertRequestInfo* cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback) override; + scoped_ptr<content::ClientCertificateDelegate> delegate) override; void AddCertificate(net::CertificateMimeType cert_type, const void* cert_data, size_t cert_size, diff --git a/chrome/browser/extensions/background_xhr_browsertest.cc b/chrome/browser/extensions/background_xhr_browsertest.cc new file mode 100644 index 0000000..6ff6deb --- /dev/null +++ b/chrome/browser/extensions/background_xhr_browsertest.cc @@ -0,0 +1,82 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/bind.h" +#include "base/memory/scoped_ptr.h" +#include "base/run_loop.h" +#include "chrome/browser/extensions/extension_browsertest.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/profiles/profile_io_data.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/test/base/ui_test_utils.h" +#include "content/public/browser/browser_thread.h" +#include "extensions/common/extension.h" +#include "extensions/test/result_catcher.h" +#include "net/base/escape.h" +#include "net/base/url_util.h" +#include "net/ssl/client_cert_store.h" +#include "net/test/spawned_test_server/spawned_test_server.h" +#include "url/gurl.h" + +namespace { + +scoped_ptr<net::ClientCertStore> CreateNullCertStore() { + return nullptr; +} + +void InstallNullCertStoreFactoryOnIOThread( + content::ResourceContext* resource_context) { + ProfileIOData::FromResourceContext(resource_context) + ->set_client_cert_store_factory_for_testing( + base::Bind(&CreateNullCertStore)); +} + +} // namespace + +class BackgroundXhrTest : public ExtensionBrowserTest { + protected: + void RunTest(const std::string& path, const GURL& url) { + const extensions::Extension* extension = + LoadExtension(test_data_dir_.AppendASCII("background_xhr")); + ASSERT_TRUE(extension); + + extensions::ResultCatcher catcher; + GURL test_url = net::AppendQueryParameter(extension->GetResourceURL(path), + "url", url.spec()); + ui_test_utils::NavigateToURL(browser(), test_url); + ASSERT_TRUE(catcher.GetNextResult()); + } +}; + +// Test that fetching a URL using TLS client auth doesn't crash, hang, or +// prompt. +IN_PROC_BROWSER_TEST_F(BackgroundXhrTest, TlsClientAuth) { + // Install a null ClientCertStore so the client auth prompt isn't bypassed due + // to the system certificate store returning no certificates. + base::RunLoop loop; + content::BrowserThread::PostTaskAndReply( + content::BrowserThread::IO, FROM_HERE, + base::Bind(&InstallNullCertStoreFactoryOnIOThread, + browser()->profile()->GetResourceContext()), + loop.QuitClosure()); + loop.Run(); + + // Launch HTTPS server. + net::SpawnedTestServer::SSLOptions ssl_options; + ssl_options.request_client_certificate = true; + net::SpawnedTestServer https_server( + net::SpawnedTestServer::TYPE_HTTPS, ssl_options, + base::FilePath(FILE_PATH_LITERAL("content/test/data"))); + ASSERT_TRUE(https_server.Start()); + + ASSERT_NO_FATAL_FAILURE( + RunTest("test_tls_client_auth.html", https_server.GetURL(""))); +} + +// Test that fetching a URL using HTTP auth doesn't crash, hang, or prompt. +IN_PROC_BROWSER_TEST_F(BackgroundXhrTest, HttpAuth) { + ASSERT_TRUE(test_server()->Start()); + ASSERT_NO_FATAL_FAILURE( + RunTest("test_http_auth.html", test_server()->GetURL("auth-basic"))); +} diff --git a/chrome/browser/ssl/ssl_client_auth_observer.cc b/chrome/browser/ssl/ssl_client_auth_observer.cc index edc63bb..acbdddd0 100644 --- a/chrome/browser/ssl/ssl_client_auth_observer.cc +++ b/chrome/browser/ssl/ssl_client_auth_observer.cc @@ -10,6 +10,7 @@ #include "base/logging.h" #include "chrome/browser/chrome_notification_types.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/notification_service.h" #include "net/cert/x509_certificate.h" #include "net/ssl/ssl_cert_request_info.h" @@ -21,10 +22,10 @@ typedef std::pair<net::SSLCertRequestInfo*, net::X509Certificate*> CertDetails; SSLClientAuthObserver::SSLClientAuthObserver( const content::BrowserContext* browser_context, const scoped_refptr<net::SSLCertRequestInfo>& cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback) + scoped_ptr<content::ClientCertificateDelegate> delegate) : browser_context_(browser_context), cert_request_info_(cert_request_info), - callback_(callback) { + delegate_(delegate.Pass()) { } SSLClientAuthObserver::~SSLClientAuthObserver() { @@ -32,10 +33,11 @@ SSLClientAuthObserver::~SSLClientAuthObserver() { void SSLClientAuthObserver::CertificateSelected( net::X509Certificate* certificate) { - if (callback_.is_null()) + if (!delegate_) return; - // Stop listening right away so we don't get our own notification. + // Stop listening now that the delegate has been resolved. This is also to + // avoid getting a self-notification. StopObserving(); CertDetails details; @@ -47,8 +49,17 @@ void SSLClientAuthObserver::CertificateSelected( content::Source<content::BrowserContext>(browser_context_), content::Details<CertDetails>(&details)); - callback_.Run(certificate); - callback_.Reset(); + delegate_->ContinueWithCertificate(certificate); + delegate_.reset(); +} + +void SSLClientAuthObserver::CancelCertificateSelection() { + if (!delegate_) + return; + + // Stop observing now that the delegate has been resolved. + StopObserving(); + delegate_.reset(); } void SSLClientAuthObserver::Observe( @@ -67,8 +78,8 @@ void SSLClientAuthObserver::Observe( DVLOG(1) << this << " got matching notification and selecting cert " << cert_details->second; StopObserving(); - callback_.Run(cert_details->second); - callback_.Reset(); + delegate_->ContinueWithCertificate(cert_details->second); + delegate_.reset(); OnCertSelectedByNotification(); } diff --git a/chrome/browser/ssl/ssl_client_auth_observer.h b/chrome/browser/ssl/ssl_client_auth_observer.h index 62e3fde..ee26134 100644 --- a/chrome/browser/ssl/ssl_client_auth_observer.h +++ b/chrome/browser/ssl/ssl_client_auth_observer.h @@ -7,6 +7,7 @@ #include "base/callback.h" #include "base/memory/ref_counted.h" +#include "base/memory/scoped_ptr.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" @@ -17,24 +18,32 @@ class X509Certificate; namespace content { class BrowserContext; +class ClientCertificateDelegate; } +// SSLClientAuthObserver is a base class that wraps a +// ClientCertificateDelegate. It links client certificate selection dialogs +// attached to the same BrowserContext. When CertificateSelected is called via +// one of them, the rest simulate the same action. class SSLClientAuthObserver : public content::NotificationObserver { public: SSLClientAuthObserver( const content::BrowserContext* browser_context, const scoped_refptr<net::SSLCertRequestInfo>& cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback); + scoped_ptr<content::ClientCertificateDelegate> delegate); ~SSLClientAuthObserver() override; // UI should implement this to close the dialog. virtual void OnCertSelectedByNotification() = 0; - // Send content the certificate. Can also call with NULL if the user - // cancelled. Derived classes must use this instead of caching the callback - // and calling it directly. + // Continues the request with a certificate. Can also call with NULL to + // continue with no certificate. Derived classes must use this instead of + // caching the delegate and calling it directly. void CertificateSelected(net::X509Certificate* cert); + // Cancels the certificate selection and aborts the request. + void CancelCertificateSelection(); + // Begins observing notifications from other SSLClientAuthHandler instances. // If another instance chooses a cert for a matching SSLCertRequestInfo, we // will also use the same cert and OnCertSelectedByNotification will be called @@ -57,7 +66,7 @@ class SSLClientAuthObserver : public content::NotificationObserver { const content::BrowserContext* browser_context_; scoped_refptr<net::SSLCertRequestInfo> cert_request_info_; - base::Callback<void(net::X509Certificate*)> callback_; + scoped_ptr<content::ClientCertificateDelegate> delegate_; content::NotificationRegistrar notification_registrar_; DISALLOW_COPY_AND_ASSIGN(SSLClientAuthObserver); diff --git a/chrome/browser/ssl/ssl_client_auth_requestor_mock.cc b/chrome/browser/ssl/ssl_client_auth_requestor_mock.cc index 6f63c32..ef114b4 100644 --- a/chrome/browser/ssl/ssl_client_auth_requestor_mock.cc +++ b/chrome/browser/ssl/ssl_client_auth_requestor_mock.cc @@ -4,9 +4,38 @@ #include "chrome/browser/ssl/ssl_client_auth_requestor_mock.h" +#include "base/macros.h" +#include "content/public/browser/client_certificate_delegate.h" #include "net/ssl/ssl_cert_request_info.h" #include "net/url_request/url_request.h" +namespace { + +class FakeClientCertificateDelegate + : public content::ClientCertificateDelegate { + public: + explicit FakeClientCertificateDelegate(SSLClientAuthRequestorMock* requestor) + : requestor_(requestor) {} + + ~FakeClientCertificateDelegate() override { + if (requestor_) + requestor_->CancelCertificateSelection(); + } + + // content::ClientCertificateDelegate implementation: + void ContinueWithCertificate(net::X509Certificate* cert) override { + requestor_->CertificateSelected(cert); + requestor_ = nullptr; + } + + private: + scoped_refptr<SSLClientAuthRequestorMock> requestor_; + + DISALLOW_COPY_AND_ASSIGN(FakeClientCertificateDelegate); +}; + +} // namespace + SSLClientAuthRequestorMock::SSLClientAuthRequestorMock( net::URLRequest* request, const scoped_refptr<net::SSLCertRequestInfo>& cert_request_info) @@ -14,3 +43,8 @@ SSLClientAuthRequestorMock::SSLClientAuthRequestorMock( } SSLClientAuthRequestorMock::~SSLClientAuthRequestorMock() {} + +scoped_ptr<content::ClientCertificateDelegate> +SSLClientAuthRequestorMock::CreateDelegate() { + return make_scoped_ptr(new FakeClientCertificateDelegate(this)); +} diff --git a/chrome/browser/ssl/ssl_client_auth_requestor_mock.h b/chrome/browser/ssl/ssl_client_auth_requestor_mock.h index 7d63a7c..6801d1a 100644 --- a/chrome/browser/ssl/ssl_client_auth_requestor_mock.h +++ b/chrome/browser/ssl/ssl_client_auth_requestor_mock.h @@ -6,8 +6,13 @@ #define CHROME_BROWSER_SSL_SSL_CLIENT_AUTH_REQUESTOR_MOCK_H_ #include "base/memory/ref_counted.h" +#include "base/memory/scoped_ptr.h" #include "testing/gmock/include/gmock/gmock.h" +namespace content { +class ClientCertificateDelegate; +} + namespace net { class HttpNetworkSession; class SSLCertRequestInfo; @@ -22,7 +27,10 @@ class SSLClientAuthRequestorMock net::URLRequest* request, const scoped_refptr<net::SSLCertRequestInfo>& cert_request_info); + scoped_ptr<content::ClientCertificateDelegate> CreateDelegate(); + MOCK_METHOD1(CertificateSelected, void(net::X509Certificate* cert)); + MOCK_METHOD0(CancelCertificateSelection, void()); scoped_refptr<net::SSLCertRequestInfo> cert_request_info_; diff --git a/chrome/browser/ssl/ssl_client_certificate_selector.h b/chrome/browser/ssl/ssl_client_certificate_selector.h index 1a33c05..7c73479 100644 --- a/chrome/browser/ssl/ssl_client_certificate_selector.h +++ b/chrome/browser/ssl/ssl_client_certificate_selector.h @@ -6,8 +6,10 @@ #define CHROME_BROWSER_SSL_SSL_CLIENT_CERTIFICATE_SELECTOR_H_ #include "base/callback_forward.h" +#include "base/memory/scoped_ptr.h" namespace content { +class ClientCertificateDelegate; class WebContents; } @@ -18,17 +20,14 @@ class X509Certificate; namespace chrome { -typedef base::Callback<void(net::X509Certificate*)> SelectCertificateCallback; - // Opens a constrained SSL client certificate selection dialog under |parent|, // offering certificates from |cert_request_info|. When the user has made a -// selection, the dialog will report back to |callback|. |callback| is notified -// when the dialog closes in call cases; if the user cancels the dialog, we call -// with a NULL certificate. +// selection, the dialog will report back to |delegate|. If the dialog is +// closed with no selection, |delegate| will simply be destroyed. void ShowSSLClientCertificateSelector( content::WebContents* contents, net::SSLCertRequestInfo* cert_request_info, - const SelectCertificateCallback& callback); + scoped_ptr<content::ClientCertificateDelegate> delegate); } // namespace chrome diff --git a/chrome/browser/ui/android/ssl_client_certificate_request.cc b/chrome/browser/ui/android/ssl_client_certificate_request.cc index 1acbec9..b09f483 100644 --- a/chrome/browser/ui/android/ssl_client_certificate_request.cc +++ b/chrome/browser/ui/android/ssl_client_certificate_request.cc @@ -9,12 +9,12 @@ #include "base/android/scoped_java_ref.h" #include "base/basictypes.h" #include "base/bind.h" -#include "base/callback_helpers.h" #include "base/compiler_specific.h" #include "base/logging.h" #include "chrome/browser/ssl/ssl_client_certificate_selector.h" #include "chrome/browser/ui/android/window_android_helper.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/client_certificate_delegate.h" #include "crypto/scoped_openssl_types.h" #include "jni/SSLClientCertificateRequest_jni.h" #include "net/android/keystore_openssl.h" @@ -43,19 +43,9 @@ void RecordClientCertificateKey( void StartClientCertificateRequest( const net::SSLCertRequestInfo* cert_request_info, ui::WindowAndroid* window, - const chrome::SelectCertificateCallback& callback) { + scoped_ptr<content::ClientCertificateDelegate> delegate) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - // Ensure that callback(NULL) is posted as a task on the UI thread - // in case of an error. - base::Closure post_task_closure = base::Bind( - base::IgnoreResult(&content::BrowserThread::PostTask), - content::BrowserThread::UI, - FROM_HERE, - base::Bind(callback, scoped_refptr<net::X509Certificate>())); - - base::ScopedClosureRunner guard(post_task_closure); - // Build the |key_types| JNI parameter, as a String[] std::vector<std::string> key_types; for (size_t n = 0; n < cert_request_info->cert_key_types.size(); ++n) { @@ -98,12 +88,8 @@ void StartClientCertificateRequest( base::android::ConvertUTF8ToJavaString( env, cert_request_info->host_and_port.host()); - // Create a copy of the callback on the heap so that its address - // and ownership can be passed through and returned from Java via JNI. - scoped_ptr<chrome::SelectCertificateCallback> request( - new chrome::SelectCertificateCallback(callback)); - - jlong request_id = reinterpret_cast<intptr_t>(request.get()); + // Pass the address of the delegate through to Java. + jlong request_id = reinterpret_cast<intptr_t>(delegate.get()); if (!chrome::android:: Java_SSLClientCertificateRequest_selectClientCertificate( @@ -117,10 +103,8 @@ void StartClientCertificateRequest( return; } - ignore_result(guard.Release()); - // Ownership was transferred to Java. - ignore_result(request.release()); + ignore_result(delegate.release()); } } // namespace @@ -146,18 +130,13 @@ static void OnSystemRequestCompletion( jobject private_key_ref) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - // Take back ownership of the request object. - scoped_ptr<chrome::SelectCertificateCallback> callback( - reinterpret_cast<chrome::SelectCertificateCallback*>(request_id)); - - // Ensure that callback(NULL) is called in case of an error. - base::Closure null_closure = - base::Bind(*callback, scoped_refptr<net::X509Certificate>()); - - base::ScopedClosureRunner guard(null_closure); + // Take back ownership of the delegate object. + scoped_ptr<content::ClientCertificateDelegate> delegate( + reinterpret_cast<content::ClientCertificateDelegate*>(request_id)); if (encoded_chain_ref == NULL || private_key_ref == NULL) { - LOG(ERROR) << "Client certificate request cancelled"; + LOG(ERROR) << "No client certificate selected"; + delegate->ContinueWithCertificate(nullptr); return; } @@ -188,18 +167,15 @@ static void OnSystemRequestCompletion( return; } - ignore_result(guard.Release()); - // RecordClientCertificateKey() must be called on the I/O thread, // before the callback is called with the selected certificate on // the UI thread. content::BrowserThread::PostTaskAndReply( - content::BrowserThread::IO, - FROM_HERE, - base::Bind(&RecordClientCertificateKey, - client_cert, + content::BrowserThread::IO, FROM_HERE, + base::Bind(&RecordClientCertificateKey, client_cert, base::Passed(&private_key)), - base::Bind(*callback, client_cert)); + base::Bind(&content::ClientCertificateDelegate::ContinueWithCertificate, + base::Owned(delegate.release()), client_cert)); } static void NotifyClientCertificatesChanged() { @@ -226,12 +202,12 @@ bool RegisterSSLClientCertificateRequestAndroid(JNIEnv* env) { void ShowSSLClientCertificateSelector( content::WebContents* contents, net::SSLCertRequestInfo* cert_request_info, - const chrome::SelectCertificateCallback& callback) { + scoped_ptr<content::ClientCertificateDelegate> delegate) { ui::WindowAndroid* window = WindowAndroidHelper::FromWebContents(contents)->GetWindowAndroid(); DCHECK(window); DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - StartClientCertificateRequest(cert_request_info, window, callback); + StartClientCertificateRequest(cert_request_info, window, delegate.Pass()); } } // namespace chrome diff --git a/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h b/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h index aacdafe..f1fe2e3 100644 --- a/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h +++ b/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h @@ -18,6 +18,7 @@ namespace content { class BrowserContext; +class ClientCertificateDelegate; } class ConstrainedWindowMac; @@ -41,13 +42,18 @@ class SSLClientAuthObserverCocoaBridge; NSRect oldSheetFrame_; // A copy of the sheet's |autoresizesSubviews| flag to restore on show. BOOL oldResizesSubviews_; + // True if the user dismissed the dialog directly, either via the OK (continue + // the request with a certificate) or Cancel (continue the request with no + // certificate) buttons. + BOOL userResponded_; } @property (readonly, nonatomic) SFChooseIdentityPanel* panel; - (id)initWithBrowserContext:(const content::BrowserContext*)browserContext certRequestInfo:(net::SSLCertRequestInfo*)certRequestInfo - callback:(const chrome::SelectCertificateCallback&)callback; + delegate:(scoped_ptr<content::ClientCertificateDelegate>) + delegate; - (void)displayForWebContents:(content::WebContents*)webContents; - (void)closeWebContentsModalDialog; diff --git a/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm b/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm index a8d7912..54ff891 100644 --- a/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm +++ b/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm @@ -14,7 +14,9 @@ #include "chrome/browser/ssl/ssl_client_auth_observer.h" #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h" #include "chrome/grit/generated_resources.h" +#include "components/web_modal/popup_manager.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/web_contents.h" #include "net/cert/x509_certificate.h" #include "net/cert/x509_util_mac.h" @@ -42,11 +44,12 @@ class SSLClientAuthObserverCocoaBridge : public SSLClientAuthObserver, SSLClientAuthObserverCocoaBridge( const content::BrowserContext* browser_context, net::SSLCertRequestInfo* cert_request_info, - const chrome::SelectCertificateCallback& callback, + scoped_ptr<content::ClientCertificateDelegate> delegate, SSLClientCertificateSelectorCocoa* controller) - : SSLClientAuthObserver(browser_context, cert_request_info, callback), - controller_(controller) { - } + : SSLClientAuthObserver(browser_context, + cert_request_info, + delegate.Pass()), + controller_(controller) {} // SSLClientAuthObserver implementation: void OnCertSelectedByNotification() override { @@ -72,14 +75,22 @@ namespace chrome { void ShowSSLClientCertificateSelector( content::WebContents* contents, net::SSLCertRequestInfo* cert_request_info, - const SelectCertificateCallback& callback) { + scoped_ptr<content::ClientCertificateDelegate> delegate) { DCHECK_CURRENTLY_ON(BrowserThread::UI); + + // Not all WebContentses can show modal dialogs. + // + // TODO(davidben): Move this hook to the WebContentsDelegate and only try to + // show a dialog in Browser's implementation. https://crbug.com/456255 + if (web_modal::PopupManager::FromWebContents(contents) == nullptr) + return; + // The dialog manages its own lifetime. SSLClientCertificateSelectorCocoa* selector = [[SSLClientCertificateSelectorCocoa alloc] initWithBrowserContext:contents->GetBrowserContext() certRequestInfo:cert_request_info - callback:callback]; + delegate:delegate.Pass()]; [selector displayForWebContents:contents]; } @@ -88,13 +99,14 @@ void ShowSSLClientCertificateSelector( @implementation SSLClientCertificateSelectorCocoa - (id)initWithBrowserContext:(const content::BrowserContext*)browserContext - certRequestInfo:(net::SSLCertRequestInfo*)certRequestInfo - callback:(const chrome::SelectCertificateCallback&)callback { + certRequestInfo:(net::SSLCertRequestInfo*)certRequestInfo + delegate:(scoped_ptr<content::ClientCertificateDelegate>) + delegate { DCHECK(browserContext); DCHECK(certRequestInfo); if ((self = [super init])) { observer_.reset(new SSLClientAuthObserverCocoaBridge( - browserContext, certRequestInfo, callback, self)); + browserContext, certRequestInfo, delegate.Pass(), self)); } return self; } @@ -113,12 +125,16 @@ void ShowSSLClientCertificateSelector( NOTREACHED(); } - // Finally, tell the backend which identity (or none) the user selected. - observer_->StopObserving(); - observer_->CertificateSelected(cert); + if (!closePending_) { + // If |closePending_| is already set, |closeSheetWithAnimation:| was called + // already to cancel the selection rather than continue with no + // certificate. Otherwise, tell the backend which identity (or none) the + // user selected. + userResponded_ = YES; + observer_->CertificateSelected(cert); - if (!closePending_) constrainedWindow_->CloseWebContentsModalDialog(); + } } - (void)displayForWebContents:(content::WebContents*)webContents { @@ -184,6 +200,14 @@ void ShowSSLClientCertificateSelector( } - (void)closeSheetWithAnimation:(BOOL)withAnimation { + if (!userResponded_) { + // If the sheet is closed by closing the tab rather than the user explicitly + // hitting Cancel, |closeSheetWithAnimation:| gets called before + // |sheetDidEnd:|. In this case, the selection should be canceled rather + // than continue with no certificate. The |returnCode| parameter to + // |sheetDidEnd:| is the same in both cases. + observer_->CancelCertificateSelection(); + } closePending_ = YES; overlayWindow_.reset(); // Closing the sheet using -[NSApp endSheet:] doesn't work so use the private diff --git a/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm b/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm index a0c71106..2684236 100644 --- a/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm +++ b/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm @@ -8,12 +8,14 @@ #include "base/bind.h" #import "base/mac/mac_util.h" +#include "base/macros.h" #include "chrome/browser/ssl/ssl_client_certificate_selector.h" #include "chrome/browser/ssl/ssl_client_certificate_selector_test.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_commands.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" #include "components/web_modal/web_contents_modal_dialog_manager.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/web_contents.h" #include "content/public/test/test_utils.h" #include "ui/base/cocoa/window_size_constants.h" @@ -22,12 +24,33 @@ using web_modal::WebContentsModalDialogManager; namespace { -void OnCertificateSelected(net::X509Certificate** out_cert, - int* out_count, - net::X509Certificate* cert) { - *out_cert = cert; - ++(*out_count); -} +class TestClientCertificateDelegate + : public content::ClientCertificateDelegate { + public: + // Creates a ClientCertificateDelegate that sets |*destroyed| to true on + // destruction. + explicit TestClientCertificateDelegate(bool* destroyed) + : destroyed_(destroyed) {} + + ~TestClientCertificateDelegate() override { + if (destroyed_ != nullptr) + *destroyed_ = true; + } + + // content::ClientCertificateDelegate. + void ContinueWithCertificate(net::X509Certificate* cert) override { + // TODO(davidben): Add a test which explicitly tests selecting a + // certificate, or selecting no certificate, since closing the dialog + // (normally by closing the tab) is not the same as explicitly selecting no + // certificate. + ADD_FAILURE() << "Certificate selected"; + } + + private: + bool* destroyed_; + + DISALLOW_COPY_AND_ASSIGN(TestClientCertificateDelegate); +}; } // namespace @@ -46,15 +69,13 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorCocoaTest, DISABLED_Basic) { WebContentsModalDialogManager::FromWebContents(web_contents); EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive()); - net::X509Certificate* cert = NULL; - int count = 0; - SSLClientCertificateSelectorCocoa* selector = - [[SSLClientCertificateSelectorCocoa alloc] - initWithBrowserContext:web_contents->GetBrowserContext() - certRequestInfo:auth_requestor_->cert_request_info_.get() - callback:base::Bind(&OnCertificateSelected, - &cert, - &count)]; + bool destroyed = false; + SSLClientCertificateSelectorCocoa* selector = [ + [SSLClientCertificateSelectorCocoa alloc] + initWithBrowserContext:web_contents->GetBrowserContext() + certRequestInfo:auth_requestor_->cert_request_info_.get() + delegate:make_scoped_ptr(new TestClientCertificateDelegate( + &destroyed))]; [selector displayForWebContents:web_contents]; content::RunAllPendingInMessageLoop(); EXPECT_TRUE([selector panel]); @@ -66,19 +87,19 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorCocoaTest, DISABLED_Basic) { content::RunAllPendingInMessageLoop(); EXPECT_FALSE(web_contents_modal_dialog_manager->IsDialogActive()); - EXPECT_EQ(NULL, cert); - EXPECT_EQ(1, count); + EXPECT_TRUE(destroyed); } // Test that switching to another tab correctly hides the sheet. IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorCocoaTest, HideShow) { content::WebContents* web_contents = browser()->tab_strip_model()->GetActiveWebContents(); - SSLClientCertificateSelectorCocoa* selector = - [[SSLClientCertificateSelectorCocoa alloc] - initWithBrowserContext:web_contents->GetBrowserContext() - certRequestInfo:auth_requestor_->cert_request_info_.get() - callback:chrome::SelectCertificateCallback()]; + SSLClientCertificateSelectorCocoa* selector = [ + [SSLClientCertificateSelectorCocoa alloc] + initWithBrowserContext:web_contents->GetBrowserContext() + certRequestInfo:auth_requestor_->cert_request_info_.get() + delegate:make_scoped_ptr( + new TestClientCertificateDelegate(nullptr))]; [selector displayForWebContents:web_contents]; content::RunAllPendingInMessageLoop(); diff --git a/chrome/browser/ui/views/ssl_client_certificate_selector.cc b/chrome/browser/ui/views/ssl_client_certificate_selector.cc index 097195f..72f4a49 100644 --- a/chrome/browser/ui/views/ssl_client_certificate_selector.cc +++ b/chrome/browser/ui/views/ssl_client_certificate_selector.cc @@ -9,7 +9,9 @@ #include "base/logging.h" #include "base/strings/utf_string_conversions.h" #include "chrome/grit/generated_resources.h" +#include "components/web_modal/popup_manager.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/web_contents.h" #include "net/cert/x509_certificate.h" #include "net/ssl/ssl_cert_request_info.h" @@ -23,11 +25,11 @@ SSLClientCertificateSelector::SSLClientCertificateSelector( content::WebContents* web_contents, const scoped_refptr<net::SSLCertRequestInfo>& cert_request_info, - const chrome::SelectCertificateCallback& callback) + scoped_ptr<content::ClientCertificateDelegate> delegate) : CertificateSelector(cert_request_info->client_certs, web_contents), SSLClientAuthObserver(web_contents->GetBrowserContext(), cert_request_info, - callback) { + delegate.Pass()) { DVLOG(1) << __FUNCTION__; } @@ -76,6 +78,15 @@ bool SSLClientCertificateSelector::Accept() { return false; } +bool SSLClientCertificateSelector::Close() { + // By default, closing the dialog calls the Cancel method. However, selecting + // cancel in the UI currently continues the request with no certificate, + // remembering the selection. If the dialog is closed by closing the + // containing tab, the request should abort. + CancelCertificateSelection(); + return true; +} + void SSLClientCertificateSelector::Unlocked(net::X509Certificate* cert) { DVLOG(1) << __FUNCTION__; CertificateSelected(cert); @@ -87,11 +98,19 @@ namespace chrome { void ShowSSLClientCertificateSelector( content::WebContents* contents, net::SSLCertRequestInfo* cert_request_info, - const chrome::SelectCertificateCallback& callback) { + scoped_ptr<content::ClientCertificateDelegate> delegate) { DVLOG(1) << __FUNCTION__ << " " << contents; DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - SSLClientCertificateSelector* selector = - new SSLClientCertificateSelector(contents, cert_request_info, callback); + + // Not all WebContentses can show modal dialogs. + // + // TODO(davidben): Move this hook to the WebContentsDelegate and only try to + // show a dialog in Browser's implementation. https://crbug.com/456255 + if (web_modal::PopupManager::FromWebContents(contents) == nullptr) + return; + + SSLClientCertificateSelector* selector = new SSLClientCertificateSelector( + contents, cert_request_info, delegate.Pass()); selector->Init(); selector->Show(); } diff --git a/chrome/browser/ui/views/ssl_client_certificate_selector.h b/chrome/browser/ui/views/ssl_client_certificate_selector.h index d80fc82..5f9c1e2 100644 --- a/chrome/browser/ui/views/ssl_client_certificate_selector.h +++ b/chrome/browser/ui/views/ssl_client_certificate_selector.h @@ -29,7 +29,7 @@ class SSLClientCertificateSelector : public chrome::CertificateSelector, SSLClientCertificateSelector( content::WebContents* web_contents, const scoped_refptr<net::SSLCertRequestInfo>& cert_request_info, - const chrome::SelectCertificateCallback& callback); + scoped_ptr<content::ClientCertificateDelegate> delegate); ~SSLClientCertificateSelector() override; void Init(); @@ -40,6 +40,7 @@ class SSLClientCertificateSelector : public chrome::CertificateSelector, // chrome::CertificateSelector: bool Cancel() override; bool Accept() override; + bool Close() override; private: // Callback after unlocking certificate slot. diff --git a/chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc b/chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc index 63ee691..2ad2566 100644 --- a/chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc +++ b/chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc @@ -13,6 +13,7 @@ #include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/interactive_test_utils.h" #include "chrome/test/base/ui_test_utils.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/web_contents.h" #include "content/public/test/browser_test_utils.h" #include "net/base/request_priority.h" @@ -86,9 +87,7 @@ class SSLClientCertificateSelectorTest : public InProcessBrowserTest { browser()->tab_strip_model()->GetActiveWebContents()); selector_ = new SSLClientCertificateSelector( browser()->tab_strip_model()->GetActiveWebContents(), - auth_requestor_->cert_request_info_, - base::Bind(&SSLClientAuthRequestorMock::CertificateSelected, - auth_requestor_)); + auth_requestor_->cert_request_info_, auth_requestor_->CreateDelegate()); selector_->Init(); selector_->Show(); @@ -178,15 +177,13 @@ class SSLClientCertificateSelectorMultiTabTest selector_1_ = new SSLClientCertificateSelector( browser()->tab_strip_model()->GetWebContentsAt(1), auth_requestor_1_->cert_request_info_, - base::Bind(&SSLClientAuthRequestorMock::CertificateSelected, - auth_requestor_1_)); + auth_requestor_1_->CreateDelegate()); selector_1_->Init(); selector_1_->Show(); selector_2_ = new SSLClientCertificateSelector( browser()->tab_strip_model()->GetWebContentsAt(2), auth_requestor_2_->cert_request_info_, - base::Bind(&SSLClientAuthRequestorMock::CertificateSelected, - auth_requestor_2_)); + auth_requestor_2_->CreateDelegate()); selector_2_->Init(); selector_2_->Show(); @@ -256,8 +253,7 @@ class SSLClientCertificateSelectorMultiProfileTest selector_1_ = new SSLClientCertificateSelector( browser_1_->tab_strip_model()->GetActiveWebContents(), auth_requestor_1_->cert_request_info_, - base::Bind(&SSLClientAuthRequestorMock::CertificateSelected, - auth_requestor_1_)); + auth_requestor_1_->CreateDelegate()); selector_1_->Init(); selector_1_->Show(); @@ -303,7 +299,7 @@ class SSLClientCertificateSelectorMultiProfileTest IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorTest, MAYBE_SelectNone) { - EXPECT_CALL(*auth_requestor_.get(), CertificateSelected(NULL)); + EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection()); // Let the mock get checked on destruction. } @@ -343,7 +339,7 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiTabTest, Escape) { // Now let the default selection for auth_requestor_ mock get checked on // destruction. - EXPECT_CALL(*auth_requestor_.get(), CertificateSelected(NULL)); + EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection()); } IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiTabTest, SelectSecond) { @@ -371,7 +367,7 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiTabTest, SelectSecond) { // Now let the default selection for auth_requestor_ mock get checked on // destruction. - EXPECT_CALL(*auth_requestor_.get(), CertificateSelected(NULL)); + EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection()); } IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiProfileTest, Escape) { @@ -385,7 +381,7 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiProfileTest, Escape) { // Now let the default selection for auth_requestor_ mock get checked on // destruction. - EXPECT_CALL(*auth_requestor_.get(), CertificateSelected(NULL)); + EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection()); } IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiProfileTest, @@ -401,5 +397,5 @@ IN_PROC_BROWSER_TEST_F(SSLClientCertificateSelectorMultiProfileTest, // Now let the default selection for auth_requestor_ mock get checked on // destruction. - EXPECT_CALL(*auth_requestor_.get(), CertificateSelected(NULL)); + EXPECT_CALL(*auth_requestor_.get(), CancelCertificateSelection()); } diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 50a64b7..9c7ab33 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -218,6 +218,7 @@ 'browser/extensions/background_app_browsertest.cc', 'browser/extensions/background_page_apitest.cc', 'browser/extensions/background_scripts_apitest.cc', + 'browser/extensions/background_xhr_browsertest.cc', 'browser/extensions/browsertest_util_browsertest.cc', 'browser/extensions/chrome_app_api_browsertest.cc', 'browser/extensions/chrome_ui_overrides_browsertest.cc', diff --git a/chrome/test/data/extensions/background_xhr/background.js b/chrome/test/data/extensions/background_xhr/background.js new file mode 100644 index 0000000..52c8e34 --- /dev/null +++ b/chrome/test/data/extensions/background_xhr/background.js @@ -0,0 +1,13 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +chrome.runtime.onMessage.addListener(function(message, sender, sendResponse) { + if (message.type == "xhr") { + var xhr = new XMLHttpRequest(); + xhr.open(message.method, message.url); + xhr.send(); + } else { + console.error("Unknown message: " + JSON.stringify(message)); + } +}); diff --git a/chrome/test/data/extensions/background_xhr/manifest.json b/chrome/test/data/extensions/background_xhr/manifest.json new file mode 100644 index 0000000..70285c6 --- /dev/null +++ b/chrome/test/data/extensions/background_xhr/manifest.json @@ -0,0 +1,9 @@ +{ + "name": "Extension tests which issue XHRs from the background page.", + "version": "1.0", + "manifest_version": 2, + "background": { + "scripts": ["background.js"] + }, + "permissions": ["webRequest", "<all_urls>"] +} diff --git a/chrome/test/data/extensions/background_xhr/test_http_auth.html b/chrome/test/data/extensions/background_xhr/test_http_auth.html new file mode 100644 index 0000000..b496e0f --- /dev/null +++ b/chrome/test/data/extensions/background_xhr/test_http_auth.html @@ -0,0 +1,6 @@ +<!-- + * Copyright 2015 The Chromium Authors. All rights reserved. Use of this + * source code is governed by a BSD-style license that can be found in the + * LICENSE file. +--> +<script src="test_http_auth.js"></script> diff --git a/chrome/test/data/extensions/background_xhr/test_http_auth.js b/chrome/test/data/extensions/background_xhr/test_http_auth.js new file mode 100644 index 0000000..beb26e8 --- /dev/null +++ b/chrome/test/data/extensions/background_xhr/test_http_auth.js @@ -0,0 +1,23 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// TODO(davidben): When URLSearchParams is stable and implemented, switch this +// (and a lot of other test code) to it. https://crbug.com/303152 +var url = decodeURIComponent(/url=([^&]*)/.exec(location.search)[1]); +var filter = {urls: [url], types: ["xmlhttprequest"]}; + +chrome.webRequest.onCompleted.addListener(function(details) { + chrome.test.assertEq(-1, details.tabId); + chrome.test.assertEq(url, details.url); + chrome.test.assertEq("xmlhttprequest", details.type); + chrome.test.assertEq(401, details.statusCode); + + chrome.test.notifyPass(); +}, filter); + +chrome.webRequest.onErrorOccurred.addListener(function(details) { + chrome.test.notifyFail("Request failed"); +}, filter); + +chrome.runtime.sendMessage({type: "xhr", method: "GET", url: url}); diff --git a/chrome/test/data/extensions/background_xhr/test_tls_client_auth.html b/chrome/test/data/extensions/background_xhr/test_tls_client_auth.html new file mode 100644 index 0000000..61edc57 --- /dev/null +++ b/chrome/test/data/extensions/background_xhr/test_tls_client_auth.html @@ -0,0 +1,6 @@ +<!-- + * Copyright 2015 The Chromium Authors. All rights reserved. Use of this + * source code is governed by a BSD-style license that can be found in the + * LICENSE file. +--> +<script src="test_tls_client_auth.js"></script> diff --git a/chrome/test/data/extensions/background_xhr/test_tls_client_auth.js b/chrome/test/data/extensions/background_xhr/test_tls_client_auth.js new file mode 100644 index 0000000..4a48af7 --- /dev/null +++ b/chrome/test/data/extensions/background_xhr/test_tls_client_auth.js @@ -0,0 +1,23 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// TODO(davidben): When URLSearchParams is stable and implemented, switch this +// (and a lot of other test code) to it. https://crbug.com/303152 +var url = decodeURIComponent(/url=([^&]*)/.exec(location.search)[1]); +var filter = {urls: [url], types: ["xmlhttprequest"]}; + +chrome.webRequest.onCompleted.addListener(function(details) { + chrome.test.notifyFail("Request completed unexpectedly"); +}, filter); + +chrome.webRequest.onErrorOccurred.addListener(function(details) { + chrome.test.assertEq(-1, details.tabId); + chrome.test.assertEq(url, details.url); + chrome.test.assertEq("xmlhttprequest", details.type); + chrome.test.assertEq("net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED", details.error); + + chrome.test.notifyPass(); +}, filter); + +chrome.runtime.sendMessage({type: "xhr", method: "GET", url: url}); diff --git a/chromecast/browser/cast_content_browser_client.cc b/chromecast/browser/cast_content_browser_client.cc index 56a8250..9793e68 100644 --- a/chromecast/browser/cast_content_browser_client.cc +++ b/chromecast/browser/cast_content_browser_client.cc @@ -29,6 +29,7 @@ #include "components/network_hints/browser/network_hints_message_filter.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/certificate_request_result_type.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/render_process_host.h" #include "content/public/browser/resource_dispatcher_host.h" #include "content/public/common/content_descriptors.h" @@ -194,16 +195,15 @@ void CastContentBrowserClient::AllowCertificateError( } void CastContentBrowserClient::SelectClientCertificate( - int render_process_id, - int render_view_id, + WebContents* web_contents, net::SSLCertRequestInfo* cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback) { + scoped_ptr<content::ClientCertificateDelegate> delegate) { GURL requesting_url("https://" + cert_request_info->host_and_port.ToString()); if (!requesting_url.is_valid()) { LOG(ERROR) << "Invalid URL string: " << requesting_url.possibly_invalid_spec(); - callback.Run(NULL); + delegate->SelectClientCertificate(nullptr); return; } @@ -214,16 +214,16 @@ void CastContentBrowserClient::SelectClientCertificate( // it, because CastNetworkDelegate is bound to the IO thread. // Subsequently, the callback must then itself be performed back here // on the UI thread. + // + // TODO(davidben): Stop using child ID to identify an app. DCHECK_CURRENTLY_ON(content::BrowserThread::UI); content::BrowserThread::PostTaskAndReplyWithResult( - content::BrowserThread::IO, - FROM_HERE, - base::Bind( - &CastContentBrowserClient::SelectClientCertificateOnIOThread, - base::Unretained(this), - requesting_url, - render_process_id), - callback); + content::BrowserThread::IO, FROM_HERE, + base::Bind(&CastContentBrowserClient::SelectClientCertificateOnIOThread, + base::Unretained(this), requesting_url, + web_contents->GetRenderProcessHost()->GetID()), + base::Bind(&content::ClientCertificateDelegate::ContinueWithCertificate, + delegate.Pass())); } net::X509Certificate* diff --git a/chromecast/browser/cast_content_browser_client.h b/chromecast/browser/cast_content_browser_client.h index ace7286..4184674 100644 --- a/chromecast/browser/cast_content_browser_client.h +++ b/chromecast/browser/cast_content_browser_client.h @@ -68,10 +68,9 @@ class CastContentBrowserClient: public content::ContentBrowserClient { const base::Callback<void(bool)>& callback, content::CertificateRequestResultType* result) override; void SelectClientCertificate( - int render_process_id, - int render_frame_id, + content::WebContents* web_contents, net::SSLCertRequestInfo* cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback) override; + scoped_ptr<content::ClientCertificateDelegate> delegate) override; bool CanCreateWindow( const GURL& opener_url, const GURL& opener_top_level_frame_url, diff --git a/content/browser/loader/resource_loader.cc b/content/browser/loader/resource_loader.cc index b462a68..7bb55f1 100644 --- a/content/browser/loader/resource_loader.cc +++ b/content/browser/loader/resource_loader.cc @@ -311,8 +311,7 @@ void ResourceLoader::OnCertificateRequested( << "OnCertificateRequested called with ssl_client_auth_handler pending"; ssl_client_auth_handler_.reset(new SSLClientAuthHandler( GetRequestInfo()->GetContext()->CreateClientCertStore(), request_.get(), - cert_info, base::Bind(&ResourceLoader::ContinueWithCertificate, - weak_ptr_factory_.GetWeakPtr()))); + cert_info, this)); ssl_client_auth_handler_->SelectCertificate(); } @@ -503,6 +502,18 @@ void ResourceLoader::ContinueSSLRequest() { request_->ContinueDespiteLastError(); } +void ResourceLoader::ContinueWithCertificate(net::X509Certificate* cert) { + DCHECK(ssl_client_auth_handler_); + ssl_client_auth_handler_.reset(); + request_->ContinueWithCertificate(cert); +} + +void ResourceLoader::CancelCertificateSelection() { + DCHECK(ssl_client_auth_handler_); + ssl_client_auth_handler_.reset(); + request_->CancelWithError(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED); +} + void ResourceLoader::Resume() { DCHECK(!is_transferring_); @@ -851,9 +862,4 @@ void ResourceLoader::RecordHistograms() { } } -void ResourceLoader::ContinueWithCertificate(net::X509Certificate* cert) { - ssl_client_auth_handler_.reset(); - request_->ContinueWithCertificate(cert); -} - } // namespace content diff --git a/content/browser/loader/resource_loader.h b/content/browser/loader/resource_loader.h index 92afbd2..bb21509 100644 --- a/content/browser/loader/resource_loader.h +++ b/content/browser/loader/resource_loader.h @@ -5,10 +5,10 @@ #ifndef CONTENT_BROWSER_LOADER_RESOURCE_LOADER_H_ #define CONTENT_BROWSER_LOADER_RESOURCE_LOADER_H_ -#include "base/gtest_prod_util.h" #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "content/browser/loader/resource_handler.h" +#include "content/browser/ssl/ssl_client_auth_handler.h" #include "content/browser/ssl/ssl_error_handler.h" #include "content/common/content_export.h" #include "content/public/browser/resource_controller.h" @@ -23,13 +23,13 @@ namespace content { class ResourceDispatcherHostLoginDelegate; class ResourceLoaderDelegate; class ResourceRequestInfoImpl; -class SSLClientAuthHandler; // This class is responsible for driving the URLRequest (i.e., calling Start, // Read, and servicing events). It has a ResourceHandler, which is typically a // chain of ResourceHandlers, and is the ResourceController for its handler. class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate, public SSLErrorHandler::Delegate, + public SSLClientAuthHandler::Delegate, public ResourceController { public: ResourceLoader(scoped_ptr<net::URLRequest> request, @@ -55,10 +55,6 @@ class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate, void OnUploadProgressACK(); private: - FRIEND_TEST_ALL_PREFIXES(ResourceLoaderTest, ClientCertStoreLookup); - FRIEND_TEST_ALL_PREFIXES(ResourceLoaderTest, ClientCertStoreNull); - FRIEND_TEST_ALL_PREFIXES(ResourceLoaderTest, ClientCertStoreAsyncCancel); - // net::URLRequest::Delegate implementation: void OnReceivedRedirect(net::URLRequest* request, const net::RedirectInfo& redirect_info, @@ -78,6 +74,10 @@ class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate, void CancelSSLRequest(int error, const net::SSLInfo* ssl_info) override; void ContinueSSLRequest() override; + // SSLClientAuthHandler::Delegate implementation. + void ContinueWithCertificate(net::X509Certificate* cert) override; + void CancelCertificateSelection() override; + // ResourceController implementation: void Resume() override; void Cancel() override; @@ -103,7 +103,6 @@ class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate, void ResponseCompleted(); void CallDidFinishLoading(); void RecordHistograms(); - void ContinueWithCertificate(net::X509Certificate* cert); bool is_deferred() const { return deferred_stage_ != DEFERRED_NONE; } diff --git a/content/browser/loader/resource_loader_unittest.cc b/content/browser/loader/resource_loader_unittest.cc index 92241b6..89f8236 100644 --- a/content/browser/loader/resource_loader_unittest.cc +++ b/content/browser/loader/resource_loader_unittest.cc @@ -6,24 +6,31 @@ #include "base/files/file.h" #include "base/files/file_util.h" +#include "base/macros.h" #include "base/message_loop/message_loop_proxy.h" #include "base/run_loop.h" #include "content/browser/browser_thread_impl.h" #include "content/browser/loader/redirect_to_file_resource_handler.h" #include "content/browser/loader/resource_loader_delegate.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/resource_request_info.h" #include "content/public/common/resource_response.h" #include "content/public/test/mock_resource_context.h" +#include "content/public/test/test_browser_context.h" #include "content/public/test/test_browser_thread_bundle.h" +#include "content/public/test/test_renderer_host.h" #include "content/test/test_content_browser_client.h" +#include "content/test/test_web_contents.h" #include "ipc/ipc_message.h" #include "net/base/io_buffer.h" #include "net/base/mock_file_stream.h" +#include "net/base/net_errors.h" #include "net/base/request_priority.h" #include "net/cert/x509_certificate.h" #include "net/ssl/client_cert_store.h" #include "net/ssl/ssl_cert_request_info.h" #include "net/url_request/url_request.h" +#include "net/url_request/url_request_job_factory.h" #include "net/url_request/url_request_job_factory_impl.h" #include "net/url_request/url_request_test_job.h" #include "net/url_request/url_request_test_util.h" @@ -53,7 +60,6 @@ class ClientCertStoreStub : public net::ClientCertStore { int* request_count, std::vector<std::string>* requested_authorities) : response_(response), - async_(false), requested_authorities_(requested_authorities), request_count_(request_count) { requested_authorities_->clear(); @@ -62,9 +68,6 @@ class ClientCertStoreStub : public net::ClientCertStore { ~ClientCertStoreStub() override {} - // Configures whether the certificates are returned asynchronously or not. - void set_async(bool async) { async_ = async; } - // net::ClientCertStore: void GetClientCerts(const net::SSLCertRequestInfo& cert_request_info, net::CertificateList* selected_certs, @@ -73,20 +76,88 @@ class ClientCertStoreStub : public net::ClientCertStore { ++(*request_count_); *selected_certs = response_; - if (async_) { - base::MessageLoop::current()->PostTask(FROM_HERE, callback); - } else { - callback.Run(); - } + callback.Run(); } private: const net::CertificateList response_; - bool async_; std::vector<std::string>* requested_authorities_; int* request_count_; }; +// Client certificate store which destroys its resource loader before the +// asynchronous GetClientCerts callback is called. +class LoaderDestroyingCertStore : public net::ClientCertStore { + public: + // Creates a client certificate store which, when looked up, posts a task to + // reset |loader| and then call the callback. The caller is responsible for + // ensuring the pointers remain valid until the process is complete. + explicit LoaderDestroyingCertStore(scoped_ptr<ResourceLoader>* loader) + : loader_(loader) {} + + // net::ClientCertStore: + void GetClientCerts(const net::SSLCertRequestInfo& cert_request_info, + net::CertificateList* selected_certs, + const base::Closure& callback) override { + // Don't destroy |loader_| while it's on the stack. + base::MessageLoop::current()->PostTask( + FROM_HERE, base::Bind(&LoaderDestroyingCertStore::DoCallback, + base::Unretained(loader_), callback)); + } + + private: + static void DoCallback(scoped_ptr<ResourceLoader>* loader, + const base::Closure& callback) { + loader->reset(); + callback.Run(); + } + + scoped_ptr<ResourceLoader>* loader_; +}; + +// A mock URLRequestJob which simulates an SSL client auth request. +class MockClientCertURLRequestJob : public net::URLRequestTestJob { + public: + MockClientCertURLRequestJob(net::URLRequest* request, + net::NetworkDelegate* network_delegate) + : net::URLRequestTestJob(request, network_delegate) {} + + static std::vector<std::string> test_authorities() { + return std::vector<std::string>(1, "dummy"); + } + + // net::URLRequestTestJob: + void Start() override { + scoped_refptr<net::SSLCertRequestInfo> cert_request_info( + new net::SSLCertRequestInfo); + cert_request_info->cert_authorities = test_authorities(); + base::MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&MockClientCertURLRequestJob::NotifyCertificateRequested, + this, cert_request_info)); + } + + void ContinueWithCertificate(net::X509Certificate* cert) override { + net::URLRequestTestJob::Start(); + } + + private: + ~MockClientCertURLRequestJob() override {} + + DISALLOW_COPY_AND_ASSIGN(MockClientCertURLRequestJob); +}; + +class MockClientCertJobProtocolHandler + : public net::URLRequestJobFactory::ProtocolHandler { + public: + // URLRequestJobFactory::ProtocolHandler implementation: + net::URLRequestJob* MaybeCreateJob( + net::URLRequest* request, + net::NetworkDelegate* network_delegate) const override { + return new MockClientCertURLRequestJob(request, network_delegate); + } +}; + // Arbitrary read buffer size. const int kReadBufSize = 1024; @@ -239,25 +310,28 @@ class SelectCertificateBrowserClient : public TestContentBrowserClient { SelectCertificateBrowserClient() : call_count_(0) {} void SelectClientCertificate( - int render_process_id, - int render_view_id, + WebContents* web_contents, net::SSLCertRequestInfo* cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback) override { + scoped_ptr<ClientCertificateDelegate> delegate) override { ++call_count_; passed_certs_ = cert_request_info->client_certs; + delegate_ = delegate.Pass(); } - int call_count() { - return call_count_; - } + int call_count() { return call_count_; } + net::CertificateList passed_certs() { return passed_certs_; } - net::CertificateList passed_certs() { - return passed_certs_; + void ContinueWithCertificate(net::X509Certificate* cert) { + delegate_->ContinueWithCertificate(cert); + delegate_.reset(); } + void CancelCertificateSelection() { delegate_.reset(); } + private: net::CertificateList passed_certs_; int call_count_; + scoped_ptr<ClientCertificateDelegate> delegate_; }; class ResourceContextStub : public MockResourceContext { @@ -297,19 +371,19 @@ class ResourceLoaderTest : public testing::Test, resource_context_(&test_url_request_context_), raw_ptr_resource_handler_(NULL), raw_ptr_to_request_(NULL) { - job_factory_.SetProtocolHandler( - "test", net::URLRequestTestJob::CreateProtocolHandler()); test_url_request_context_.set_job_factory(&job_factory_); } - GURL test_url() const { - return net::URLRequestTestJob::test_url_1(); - } + GURL test_url() const { return net::URLRequestTestJob::test_url_1(); } std::string test_data() const { return net::URLRequestTestJob::test_data_1(); } + virtual net::URLRequestJobFactory::ProtocolHandler* CreateProtocolHandler() { + return net::URLRequestTestJob::CreateProtocolHandler(); + } + virtual scoped_ptr<ResourceHandler> WrapResourceHandler( scoped_ptr<ResourceHandlerStub> leaf_handler, net::URLRequest* request) { @@ -317,8 +391,14 @@ class ResourceLoaderTest : public testing::Test, } void SetUp() override { - const int kRenderProcessId = 1; - const int kRenderViewId = 2; + job_factory_.SetProtocolHandler("test", CreateProtocolHandler()); + + browser_context_.reset(new TestBrowserContext()); + scoped_refptr<SiteInstance> site_instance = + SiteInstance::Create(browser_context_.get()); + web_contents_.reset( + TestWebContents::Create(browser_context_.get(), site_instance.get())); + RenderFrameHost* rfh = web_contents_->GetMainFrame(); scoped_ptr<net::URLRequest> request( resource_context_.GetRequestContext()->CreateRequest( @@ -327,16 +407,12 @@ class ResourceLoaderTest : public testing::Test, NULL /* delegate */, NULL /* cookie_store */)); raw_ptr_to_request_ = request.get(); - ResourceRequestInfo::AllocateForTesting(request.get(), - RESOURCE_TYPE_MAIN_FRAME, - &resource_context_, - kRenderProcessId, - kRenderViewId, - MSG_ROUTING_NONE, - true, // is_main_frame - false, // parent_is_main_frame - true, // allow_download - false); // is_async + ResourceRequestInfo::AllocateForTesting( + request.get(), RESOURCE_TYPE_MAIN_FRAME, &resource_context_, + rfh->GetProcess()->GetID(), rfh->GetRenderViewHost()->GetRoutingID(), + rfh->GetRoutingID(), true /* is_main_frame */, + false /* parent_is_main_frame */, true /* allow_download */, + false /* is_async */); scoped_ptr<ResourceHandlerStub> resource_handler( new ResourceHandlerStub(request.get())); raw_ptr_resource_handler_ = resource_handler.get(); @@ -346,6 +422,14 @@ class ResourceLoaderTest : public testing::Test, this)); } + void TearDown() override { + // Destroy the WebContents and pump the event loop before destroying + // |rvh_test_enabler_| and |thread_bundle_|. This lets asynchronous cleanup + // tasks complete. + web_contents_.reset(); + base::RunLoop().RunUntilIdle(); + } + // ResourceLoaderDelegate: ResourceDispatcherHostLoginDelegate* CreateLoginDelegate( ResourceLoader* loader, @@ -362,11 +446,14 @@ class ResourceLoaderTest : public testing::Test, void DidReceiveResponse(ResourceLoader* loader) override {} void DidFinishLoading(ResourceLoader* loader) override {} - content::TestBrowserThreadBundle thread_bundle_; + TestBrowserThreadBundle thread_bundle_; + RenderViewHostTestEnabler rvh_test_enabler_; net::URLRequestJobFactoryImpl job_factory_; net::TestURLRequestContext test_url_request_context_; ResourceContextStub resource_context_; + scoped_ptr<TestBrowserContext> browser_context_; + scoped_ptr<TestWebContents> web_contents_; // The ResourceLoader owns the URLRequest and the ResourceHandler. ResourceHandlerStub* raw_ptr_resource_handler_; @@ -374,11 +461,15 @@ class ResourceLoaderTest : public testing::Test, scoped_ptr<ResourceLoader> loader_; }; -// Verifies if a call to net::UrlRequest::Delegate::OnCertificateRequested() -// causes client cert store to be queried for certificates and if the returned -// certificates are correctly passed to the content browser client for -// selection. -TEST_F(ResourceLoaderTest, ClientCertStoreLookup) { +class ClientCertResourceLoaderTest : public ResourceLoaderTest { + protected: + net::URLRequestJobFactory::ProtocolHandler* CreateProtocolHandler() override { + return new MockClientCertJobProtocolHandler; + } +}; + +// Tests that client certificates are requested with ClientCertStore lookup. +TEST_F(ClientCertResourceLoaderTest, WithStoreLookup) { // Set up the test client cert store. int store_request_count; std::vector<std::string> store_requested_authorities; @@ -388,91 +479,125 @@ TEST_F(ResourceLoaderTest, ClientCertStoreLookup) { dummy_certs, &store_request_count, &store_requested_authorities)); resource_context_.SetClientCertStore(test_store.Pass()); - // Prepare a dummy certificate request. - scoped_refptr<net::SSLCertRequestInfo> cert_request_info( - new net::SSLCertRequestInfo()); - std::vector<std::string> dummy_authority(1, "dummy"); - cert_request_info->cert_authorities = dummy_authority; - // Plug in test content browser client. SelectCertificateBrowserClient test_client; ContentBrowserClient* old_client = SetBrowserClientForTesting(&test_client); - // Everything is set up. Trigger the resource loader certificate request event - // and run the message loop. - loader_->OnCertificateRequested(raw_ptr_to_request_, cert_request_info.get()); + // Start the request and wait for it to pause. + loader_->StartRequest(); base::RunLoop().RunUntilIdle(); - // Restore the original content browser client. - SetBrowserClientForTesting(old_client); + EXPECT_FALSE(raw_ptr_resource_handler_->received_response_completed()); // Check if the test store was queried against correct |cert_authorities|. EXPECT_EQ(1, store_request_count); - EXPECT_EQ(dummy_authority, store_requested_authorities); + EXPECT_EQ(MockClientCertURLRequestJob::test_authorities(), + store_requested_authorities); // Check if the retrieved certificates were passed to the content browser // client. EXPECT_EQ(1, test_client.call_count()); EXPECT_EQ(dummy_certs, test_client.passed_certs()); -} -// Verifies if a call to net::URLRequest::Delegate::OnCertificateRequested() -// on a platform with a NULL client cert store still calls the content browser -// client for selection. -TEST_F(ResourceLoaderTest, ClientCertStoreNull) { - // Prepare a dummy certificate request. - scoped_refptr<net::SSLCertRequestInfo> cert_request_info( - new net::SSLCertRequestInfo()); - std::vector<std::string> dummy_authority(1, "dummy"); - cert_request_info->cert_authorities = dummy_authority; + // Continue the request. + test_client.ContinueWithCertificate(dummy_certs[0].get()); + base::RunLoop().RunUntilIdle(); + EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed()); + EXPECT_EQ(net::OK, raw_ptr_resource_handler_->status().error()); + + // Restore the original content browser client. + SetBrowserClientForTesting(old_client); +} +// Tests that client certificates are requested on a platform with NULL +// ClientCertStore. +TEST_F(ClientCertResourceLoaderTest, WithNullStore) { // Plug in test content browser client. SelectCertificateBrowserClient test_client; ContentBrowserClient* old_client = SetBrowserClientForTesting(&test_client); - // Everything is set up. Trigger the resource loader certificate request event - // and run the message loop. - loader_->OnCertificateRequested(raw_ptr_to_request_, cert_request_info.get()); + // Start the request and wait for it to pause. + loader_->StartRequest(); base::RunLoop().RunUntilIdle(); + // Check if the SelectClientCertificate was called on the content browser + // client. + EXPECT_EQ(1, test_client.call_count()); + EXPECT_EQ(net::CertificateList(), test_client.passed_certs()); + + // Continue the request. + scoped_refptr<net::X509Certificate> cert( + new net::X509Certificate("test", "test", base::Time(), base::Time())); + test_client.ContinueWithCertificate(cert.get()); + base::RunLoop().RunUntilIdle(); + EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed()); + EXPECT_EQ(net::OK, raw_ptr_resource_handler_->status().error()); + // Restore the original content browser client. SetBrowserClientForTesting(old_client); +} + +// Tests that the ContentBrowserClient may cancel a certificate request. +TEST_F(ClientCertResourceLoaderTest, CancelSelection) { + // Plug in test content browser client. + SelectCertificateBrowserClient test_client; + ContentBrowserClient* old_client = SetBrowserClientForTesting(&test_client); + + // Start the request and wait for it to pause. + loader_->StartRequest(); + base::RunLoop().RunUntilIdle(); // Check if the SelectClientCertificate was called on the content browser // client. EXPECT_EQ(1, test_client.call_count()); EXPECT_EQ(net::CertificateList(), test_client.passed_certs()); + + // Cancel the request. + test_client.CancelCertificateSelection(); + base::RunLoop().RunUntilIdle(); + EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed()); + EXPECT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED, + raw_ptr_resource_handler_->status().error()); + + // Restore the original content browser client. + SetBrowserClientForTesting(old_client); } -TEST_F(ResourceLoaderTest, ClientCertStoreAsyncCancel) { - // Set up the test client cert store. - int store_request_count; - std::vector<std::string> store_requested_authorities; - scoped_ptr<ClientCertStoreStub> test_store( - new ClientCertStoreStub(net::CertificateList(), &store_request_count, - &store_requested_authorities)); - test_store->set_async(true); - EXPECT_EQ(0, store_request_count); - resource_context_.SetClientCertStore(test_store.Pass()); +// Verifies that requests without WebContents attached abort. +TEST_F(ClientCertResourceLoaderTest, NoWebContents) { + // Destroy the WebContents before starting the request. + web_contents_.reset(); - // Prepare a dummy certificate request. - scoped_refptr<net::SSLCertRequestInfo> cert_request_info( - new net::SSLCertRequestInfo()); - std::vector<std::string> dummy_authority(1, "dummy"); - cert_request_info->cert_authorities = dummy_authority; + // Plug in test content browser client. + SelectCertificateBrowserClient test_client; + ContentBrowserClient* old_client = SetBrowserClientForTesting(&test_client); - // Everything is set up. Trigger the resource loader certificate request - // event. - loader_->OnCertificateRequested(raw_ptr_to_request_, cert_request_info.get()); + // Start the request and wait for it to pause. + loader_->StartRequest(); + base::RunLoop().RunUntilIdle(); - // Check if the test store was queried against correct |cert_authorities|. - EXPECT_EQ(1, store_request_count); - EXPECT_EQ(dummy_authority, store_requested_authorities); + // Check that SelectClientCertificate wasn't called and the request aborted. + EXPECT_EQ(0, test_client.call_count()); + EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed()); + EXPECT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED, + raw_ptr_resource_handler_->status().error()); + + // Restore the original content browser client. + SetBrowserClientForTesting(old_client); +} + +// Verifies that ClientCertStore's callback doesn't crash if called after the +// loader is destroyed. +TEST_F(ClientCertResourceLoaderTest, StoreAsyncCancel) { + scoped_ptr<LoaderDestroyingCertStore> test_store( + new LoaderDestroyingCertStore(&loader_)); + resource_context_.SetClientCertStore(test_store.Pass()); - // Cancel the request before the store calls the callback. - loader_.reset(); + loader_->StartRequest(); + base::RunLoop().RunUntilIdle(); + EXPECT_FALSE(loader_); - // Pump the event loop. There shouldn't be a crash when the callback is run. + // Pump the event loop to ensure nothing asynchronous crashes either. base::RunLoop().RunUntilIdle(); } diff --git a/content/browser/shared_worker/worker_browsertest.cc b/content/browser/shared_worker/worker_browsertest.cc index 14100b9..3a0593b 100644 --- a/content/browser/shared_worker/worker_browsertest.cc +++ b/content/browser/shared_worker/worker_browsertest.cc @@ -11,6 +11,7 @@ #include "base/sys_info.h" #include "base/test/test_timeouts.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/common/content_paths.h" #include "content/public/test/browser_test_utils.h" #include "content/public/test/content_browser_test.h" @@ -19,15 +20,45 @@ #include "content/shell/browser/shell.h" #include "content/shell/browser/shell_content_browser_client.h" #include "content/shell/browser/shell_resource_dispatcher_host_delegate.h" +#include "net/base/escape.h" #include "net/base/test_data_directory.h" #include "net/test/spawned_test_server/spawned_test_server.h" #include "url/gurl.h" namespace content { +namespace { + +bool SupportsSharedWorker() { +#if defined(OS_ANDROID) + // SharedWorkers are not enabled on Android. https://crbug.com/154571 + // + // TODO(davidben): Move other SharedWorker exclusions from + // build/android/pylib/gtest/filter/ inline. + return false; +#else + return true; +#endif +} + +} // namespace + class WorkerTest : public ContentBrowserTest { public: - WorkerTest() {} + WorkerTest() : select_certificate_count_(0) {} + + void SetUpOnMainThread() override { + ShellContentBrowserClient::Get()->set_select_client_certificate_callback( + base::Bind(&WorkerTest::OnSelectClientCertificate, + base::Unretained(this))); + } + + void TearDownOnMainThread() override { + ShellContentBrowserClient::Get()->set_select_client_certificate_callback( + base::Closure()); + } + + int select_certificate_count() const { return select_certificate_count_; } GURL GetTestURL(const std::string& test_case, const std::string& query) { base::FilePath test_file_path = GetTestFilePath( @@ -64,6 +95,11 @@ class WorkerTest : public ContentBrowserTest { shell()->LoadURL(url); runner->Run(); } + + private: + void OnSelectClientCertificate() { select_certificate_count_++; } + + int select_certificate_count_; }; IN_PROC_BROWSER_TEST_F(WorkerTest, SingleWorker) { @@ -102,14 +138,55 @@ IN_PROC_BROWSER_TEST_F(WorkerTest, WorkerHttpAuth) { NavigateAndWaitForAuth(url); } -// Make sure that auth dialog is displayed from shared worker context. +// Make sure that HTTP auth dialog is displayed from shared worker context. // http://crbug.com/33344 +// +// TODO(davidben): HTTP auth dialogs are no longer displayed on shared workers, +// but this test only tests that the delegate is called. Move handling the +// WebContentsless case from chrome/ to content/ and adjust the test +// accordingly. IN_PROC_BROWSER_TEST_F(WorkerTest, SharedWorkerHttpAuth) { ASSERT_TRUE(test_server()->Start()); GURL url = test_server()->GetURL("files/workers/shared_worker_auth.html"); NavigateAndWaitForAuth(url); } +// Tests that TLS client auth prompts for normal workers. +IN_PROC_BROWSER_TEST_F(WorkerTest, WorkerTlsClientAuth) { + // Launch HTTPS server. + net::SpawnedTestServer::SSLOptions ssl_options; + ssl_options.request_client_certificate = true; + net::SpawnedTestServer https_server( + net::SpawnedTestServer::TYPE_HTTPS, ssl_options, + base::FilePath(FILE_PATH_LITERAL("content/test/data"))); + ASSERT_TRUE(https_server.Start()); + + RunTest("worker_tls_client_auth.html", + "url=" + + net::EscapeQueryParamValue(https_server.GetURL("").spec(), true)); + EXPECT_EQ(1, select_certificate_count()); +} + +// Tests that TLS client auth does not prompt for a shared worker; shared +// workers are not associated with a WebContents. +IN_PROC_BROWSER_TEST_F(WorkerTest, SharedWorkerTlsClientAuth) { + if (!SupportsSharedWorker()) + return; + + // Launch HTTPS server. + net::SpawnedTestServer::SSLOptions ssl_options; + ssl_options.request_client_certificate = true; + net::SpawnedTestServer https_server( + net::SpawnedTestServer::TYPE_HTTPS, ssl_options, + base::FilePath(FILE_PATH_LITERAL("content/test/data"))); + ASSERT_TRUE(https_server.Start()); + + RunTest("worker_tls_client_auth.html", + "shared=true&url=" + + net::EscapeQueryParamValue(https_server.GetURL("").spec(), true)); + EXPECT_EQ(0, select_certificate_count()); +} + IN_PROC_BROWSER_TEST_F(WorkerTest, WebSocketSharedWorker) { // Launch WebSocket server. net::SpawnedTestServer ws_server(net::SpawnedTestServer::TYPE_WS, diff --git a/content/browser/ssl/ssl_client_auth_handler.cc b/content/browser/ssl/ssl_client_auth_handler.cc index d8edc5d..e528643 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,56 @@ namespace content { namespace { -void CertificateSelectedOnUIThread( - const SSLClientAuthHandler::CertificateCallback& io_thread_callback, - net::X509Certificate* cert) { - DCHECK_CURRENTLY_ON(BrowserThread::UI); +class ClientCertificateDelegateImpl : public ClientCertificateDelegate { + public: + explicit ClientCertificateDelegateImpl( + const base::WeakPtr<SSLClientAuthHandler>& handler) + : handler_(handler), continue_called_(false) {} + + ~ClientCertificateDelegateImpl() override { + if (!continue_called_) { + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&SSLClientAuthHandler::CancelCertificateSelection, + handler_)); + } + } - BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, - base::Bind(io_thread_callback, make_scoped_refptr(cert))); -} + // ClientCertificateDelegate implementation: + void ContinueWithCertificate(net::X509Certificate* cert) override { + DCHECK(!continue_called_); + continue_called_ = true; + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&SSLClientAuthHandler::ContinueWithCertificate, handler_, + make_scoped_refptr(cert))); + } + + private: + base::WeakPtr<SSLClientAuthHandler> handler_; + bool continue_called_; + + 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) + 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 +121,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 +142,41 @@ void SSLClientAuthHandler::SelectCertificate() { core_->GetClientCerts(); } +// static +void SSLClientAuthHandler::ContinueWithCertificate( + const base::WeakPtr<SSLClientAuthHandler>& handler, + net::X509Certificate* cert) { + if (handler) + handler->delegate_->ContinueWithCertificate(cert); +} + +// static +void SSLClientAuthHandler::CancelCertificateSelection( + const base::WeakPtr<SSLClientAuthHandler>& handler) { + if (handler) + handler->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); + // + // TODO(davidben): The WebContents-less check on the UI thread should come + // before checking ClientCertStore; ClientCertStore itself should probably + // be handled by the embedder (https://crbug.com/394131), especially since + // this doesn't work on Android (https://crbug.com/345641). + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&SSLClientAuthHandler::ContinueWithCertificate, + weak_factory_.GetWeakPtr(), + scoped_refptr<net::X509Certificate>())); return; } @@ -128,7 +185,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 +196,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 diff --git a/content/browser/ssl/ssl_client_auth_handler.h b/content/browser/ssl/ssl_client_auth_handler.h index 7597687..b9c2447 100644 --- a/content/browser/ssl/ssl_client_auth_handler.h +++ b/content/browser/ssl/ssl_client_auth_handler.h @@ -7,8 +7,10 @@ #include "base/basictypes.h" #include "base/callback.h" +#include "base/macros.h" #include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" +#include "content/common/content_export.h" #include "content/public/browser/browser_thread.h" #include "net/ssl/ssl_cert_request_info.h" @@ -23,30 +25,56 @@ namespace content { // This class handles the approval and selection of a certificate for SSL client // authentication by the user. Should only be used on the IO thread. If the // SSLClientAuthHandler is destroyed before the certificate is selected, the -// selection is canceled and the callback never called. +// selection is canceled and the delegate never called. class SSLClientAuthHandler { public: - using CertificateCallback = base::Callback<void(net::X509Certificate*)>; + // Delegate interface for SSLClientAuthHandler. Method implementations may + // delete the handler when called. + class CONTENT_EXPORT Delegate { + public: + Delegate() {} + // Called to continue the request with |cert|. |cert| may be nullptr. + virtual void ContinueWithCertificate(net::X509Certificate* cert) = 0; + + // Called to cancel the certificate selection and abort the request. + virtual void CancelCertificateSelection() = 0; + + protected: + virtual ~Delegate() {} + + private: + DISALLOW_COPY_AND_ASSIGN(Delegate); + }; + + // Creates a new SSLClientAuthHandler. The caller ensures that the handler + // does not outlive |request| or |delegate|. SSLClientAuthHandler(scoped_ptr<net::ClientCertStore> client_cert_store, net::URLRequest* request, net::SSLCertRequestInfo* cert_request_info, - const CertificateCallback& callback); + Delegate* delegate); ~SSLClientAuthHandler(); // Selects a certificate and resumes the URL request with that certificate. void SelectCertificate(); + // Called to continue the request associated with |handler| using |cert|. This + // is static to avoid deleting |handler| while it is on the stack. + static void ContinueWithCertificate( + const base::WeakPtr<SSLClientAuthHandler>& handler, + net::X509Certificate* cert); + + // Called to abort the request associated with |handler|. This is static to + // avoid deleting |handler| while it is on the stack. + static void CancelCertificateSelection( + const base::WeakPtr<SSLClientAuthHandler>& handler); + private: class Core; // Called when |core_| is done retrieving the cert list. void DidGetClientCerts(); - // Called when the user has selected a cert. If the user chose to continue - // with no certificate, |cert| is NULL. - void CertificateSelected(net::X509Certificate* cert); - // A reference-counted core so the ClientCertStore may outlive // SSLClientAuthHandler if the handler is destroyed while an operation on the // ClientCertStore is in progress. @@ -58,8 +86,8 @@ class SSLClientAuthHandler { // The certs to choose from. scoped_refptr<net::SSLCertRequestInfo> cert_request_info_; - // The callback to call when the certificate is selected. - CertificateCallback callback_; + // The delegate to call back with the result. + Delegate* delegate_; base::WeakPtrFactory<SSLClientAuthHandler> weak_factory_; diff --git a/content/content_browser.gypi b/content/content_browser.gypi index 06b1149..fcfc728 100644 --- a/content/content_browser.gypi +++ b/content/content_browser.gypi @@ -92,6 +92,7 @@ 'public/browser/certificate_request_result_type.h', 'public/browser/child_process_data.h', 'public/browser/child_process_security_policy.h', + 'public/browser/client_certificate_delegate.h', 'public/browser/color_chooser.h', 'public/browser/content_browser_client.cc', 'public/browser/content_browser_client.h', diff --git a/content/public/browser/client_certificate_delegate.h b/content/public/browser/client_certificate_delegate.h new file mode 100644 index 0000000..d957fd9 --- /dev/null +++ b/content/public/browser/client_certificate_delegate.h @@ -0,0 +1,35 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CONTENT_PUBLIC_BROWSER_CLIENT_CERTIFICATE_DELEGATE_H_ +#define CONTENT_PUBLIC_BROWSER_CLIENT_CERTIFICATE_DELEGATE_H_ + +#include "base/macros.h" + +namespace net { +class X509Certificate; +} + +namespace content { + +// A delegate interface for selecting a client certificate for use with a +// network request. If the delegate is destroyed without calling +// ContinueWithCertificate, the certificate request will be aborted. +class ClientCertificateDelegate { + public: + ClientCertificateDelegate() {} + virtual ~ClientCertificateDelegate() {} + + // Continue the request with |cert|. |cert| may be nullptr to continue without + // supplying a certificate. This decision will be remembered for future + // requests to the domain. + virtual void ContinueWithCertificate(net::X509Certificate* cert) = 0; + + private: + DISALLOW_COPY_AND_ASSIGN(ClientCertificateDelegate); +}; + +} // namespace content + +#endif // CONTENT_PUBLIC_BROWSER_CLIENT_CERTIFICATE_DELEGATE_H_ diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc index 96c076a..1ce7105 100644 --- a/content/public/browser/content_browser_client.cc +++ b/content/public/browser/content_browser_client.cc @@ -5,6 +5,7 @@ #include "content/public/browser/content_browser_client.h" #include "base/files/file_path.h" +#include "content/public/browser/client_certificate_delegate.h" #include "ui/gfx/image/image_skia.h" #include "url/gurl.h" @@ -177,11 +178,9 @@ QuotaPermissionContext* ContentBrowserClient::CreateQuotaPermissionContext() { } void ContentBrowserClient::SelectClientCertificate( - int render_process_id, - int render_frame_id, + WebContents* web_contents, net::SSLCertRequestInfo* cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback) { - callback.Run(nullptr); + scoped_ptr<ClientCertificateDelegate> delegate) { } net::URLRequestContext* ContentBrowserClient::OverrideRequestContextForURL( diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h index c5c3942..3195973 100644 --- a/content/public/browser/content_browser_client.h +++ b/content/public/browser/content_browser_client.h @@ -89,6 +89,7 @@ class BrowserMainParts; class BrowserPluginGuestDelegate; class BrowserPpapiHost; class BrowserURLHandler; +class ClientCertificateDelegate; class DevToolsManagerDelegate; class ExternalVideoSurfaceContainer; class LocationProvider; @@ -400,14 +401,14 @@ class CONTENT_EXPORT ContentBrowserClient { const base::Callback<void(bool)>& callback, CertificateRequestResultType* result) {} - // Selects a SSL client certificate and returns it to the |callback|. If no - // certificate was selected nullptr is returned to the |callback|. Note: - // |callback| may be called synchronously or asynchronously. + // Selects a SSL client certificate and returns it to the |delegate|. Note: + // |delegate| may be called synchronously or asynchronously. + // + // TODO(davidben): Move this hook to WebContentsDelegate. virtual void SelectClientCertificate( - int render_process_id, - int render_frame_id, + WebContents* web_contents, net::SSLCertRequestInfo* cert_request_info, - const base::Callback<void(net::X509Certificate*)>& callback); + scoped_ptr<ClientCertificateDelegate> delegate); // Adds a new installable certificate or private key. // Typically used to install an X.509 user certificate. diff --git a/content/shell/browser/shell_content_browser_client.cc b/content/shell/browser/shell_content_browser_client.cc index 927fdd7..90cc70c 100644 --- a/content/shell/browser/shell_content_browser_client.cc +++ b/content/shell/browser/shell_content_browser_client.cc @@ -10,6 +10,7 @@ #include "base/files/file_util.h" #include "base/path_service.h" #include "base/strings/utf_string_conversions.h" +#include "content/public/browser/client_certificate_delegate.h" #include "content/public/browser/page_navigator.h" #include "content/public/browser/render_process_host.h" #include "content/public/browser/resource_dispatcher_host.h" @@ -287,6 +288,14 @@ ShellContentBrowserClient::CreateQuotaPermissionContext() { return new ShellQuotaPermissionContext(); } +void ShellContentBrowserClient::SelectClientCertificate( + WebContents* web_contents, + net::SSLCertRequestInfo* cert_request_info, + scoped_ptr<ClientCertificateDelegate> delegate) { + if (!select_client_certificate_callback_.is_null()) + select_client_certificate_callback_.Run(); +} + void ShellContentBrowserClient::RequestPermission( PermissionType permission, WebContents* web_contents, diff --git a/content/shell/browser/shell_content_browser_client.h b/content/shell/browser/shell_content_browser_client.h index e78062f..6360f2c 100644 --- a/content/shell/browser/shell_content_browser_client.h +++ b/content/shell/browser/shell_content_browser_client.h @@ -54,6 +54,10 @@ class ShellContentBrowserClient : public ContentBrowserClient { WebContentsViewDelegate* GetWebContentsViewDelegate( WebContents* web_contents) override; QuotaPermissionContext* CreateQuotaPermissionContext() override; + void SelectClientCertificate( + WebContents* web_contents, + net::SSLCertRequestInfo* cert_request_info, + scoped_ptr<ClientCertificateDelegate> delegate) override; void RequestPermission( PermissionType permission, WebContents* web_contents, @@ -94,6 +98,12 @@ class ShellContentBrowserClient : public ContentBrowserClient { return shell_browser_main_parts_; } + // Used for content_browsertests. + void set_select_client_certificate_callback( + base::Closure select_client_certificate_callback) { + select_client_certificate_callback_ = select_client_certificate_callback; + } + private: ShellBrowserContext* ShellBrowserContextForBrowserContext( BrowserContext* content_browser_context); @@ -106,6 +116,8 @@ class ShellContentBrowserClient : public ContentBrowserClient { base::ScopedFD v8_snapshot_fd_; #endif + base::Closure select_client_certificate_callback_; + ShellBrowserMainParts* shell_browser_main_parts_; }; diff --git a/content/test/data/workers/worker_common.js b/content/test/data/workers/worker_common.js index 675de0d..03cc7f6 100644 --- a/content/test/data/workers/worker_common.js +++ b/content/test/data/workers/worker_common.js @@ -1,3 +1,7 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + // Track the number of clients for this worker - tests can use this to ensure // that shared workers are actually shared, not distinct. var num_clients = 0; @@ -29,5 +33,11 @@ onmessage = function(evt) { } catch (ex) { postMessage(ex); } + } else if (/tls-client-auth.+/.test(evt.data)) { + try { + importScripts(evt.data.substr(16)); + } catch (ex) { + } + postMessage("done"); } } diff --git a/content/test/data/workers/worker_tls_client_auth.html b/content/test/data/workers/worker_tls_client_auth.html new file mode 100644 index 0000000..02260cc --- /dev/null +++ b/content/test/data/workers/worker_tls_client_auth.html @@ -0,0 +1,25 @@ +<html> + +<head> +<title>Worker TLS Client Auth Test</title> + +<script src="worker_utils.js"></script> + +<script> + +// TODO(davidben): When URLSearchParams is stable and implemented, switch this +// (and a lot of other test code) to it. https://crbug.com/303152 +var url = decodeURIComponent(/url=([^&]*)/.exec(location.search)[1]); +var worker = getWorker("worker_common.js"); +worker.onmessage = function(ev) { + if (ev.data == "done") + onSuccess(); +}; +worker.postMessage("tls-client-auth " + url); +</script> +</head> + +<body> +<div id=statusPanel></div> +</body> +</html> diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc index 8351029..f84fca4 100644 --- a/net/url_request/url_request.cc +++ b/net/url_request/url_request.cc @@ -157,7 +157,7 @@ void URLRequest::Delegate::OnAuthRequired(URLRequest* request, void URLRequest::Delegate::OnCertificateRequested( URLRequest* request, SSLCertRequestInfo* cert_request_info) { - request->Cancel(); + request->CancelWithError(ERR_SSL_CLIENT_AUTH_CERT_NEEDED); } void URLRequest::Delegate::OnSSLCertificateError(URLRequest* request, |