diff options
| -rw-r--r-- | content/browser/loader/resource_loader_unittest.cc | 75 | ||||
| -rw-r--r-- | content/browser/ssl/ssl_client_auth_handler.cc | 7 | ||||
| -rw-r--r-- | content/browser/ssl/ssl_client_auth_handler.h | 3 | ||||
| -rw-r--r-- | content/public/browser/content_browser_client.h | 3 |
4 files changed, 46 insertions, 42 deletions
diff --git a/content/browser/loader/resource_loader_unittest.cc b/content/browser/loader/resource_loader_unittest.cc index c9d3398..92241b6 100644 --- a/content/browser/loader/resource_loader_unittest.cc +++ b/content/browser/loader/resource_loader_unittest.cc @@ -40,35 +40,38 @@ namespace { // inspection. class ClientCertStoreStub : public net::ClientCertStore { public: - ClientCertStoreStub(const net::CertificateList& certs) - : response_(certs), async_(false), request_count_(0) {} - - ~ClientCertStoreStub() override {} - - // Configures whether the certificates are returned asynchronously or not. - void set_async(bool async) { async_ = async; } - - // Returns |cert_authorities| field of the certificate request passed in the - // most recent call to GetClientCerts(). + // Creates a new ClientCertStoreStub that returns |response| on query. It + // saves the number of requests and most recently certificate authorities list + // in |requested_authorities| and |request_count|, respectively. The caller is + // responsible for ensuring those pointers outlive the ClientCertStoreStub. + // // TODO(ppi): Make the stub independent from the internal representation of - // SSLCertRequestInfo. For now it seems that we cannot neither save the + // SSLCertRequestInfo. For now it seems that we can neither save the // scoped_refptr<> (since it is never passed to us) nor copy the entire // CertificateRequestInfo (since there is no copy constructor). - std::vector<std::string> requested_authorities() { - return requested_authorities_; + ClientCertStoreStub(const net::CertificateList& response, + int* request_count, + std::vector<std::string>* requested_authorities) + : response_(response), + async_(false), + requested_authorities_(requested_authorities), + request_count_(request_count) { + requested_authorities_->clear(); + *request_count_ = 0; } - // Returns the number of calls to GetClientCerts(). - int request_count() { - return request_count_; - } + ~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, const base::Closure& callback) override { - ++request_count_; - requested_authorities_ = cert_request_info.cert_authorities; + *requested_authorities_ = cert_request_info.cert_authorities; + ++(*request_count_); + *selected_certs = response_; if (async_) { base::MessageLoop::current()->PostTask(FROM_HERE, callback); @@ -80,8 +83,8 @@ class ClientCertStoreStub : public net::ClientCertStore { private: const net::CertificateList response_; bool async_; - int request_count_; - std::vector<std::string> requested_authorities_; + std::vector<std::string>* requested_authorities_; + int* request_count_; }; // Arbitrary read buffer size. @@ -377,15 +380,12 @@ class ResourceLoaderTest : public testing::Test, // selection. TEST_F(ResourceLoaderTest, ClientCertStoreLookup) { // Set up the test client cert store. + int store_request_count; + std::vector<std::string> store_requested_authorities; net::CertificateList dummy_certs(1, scoped_refptr<net::X509Certificate>( new net::X509Certificate("test", "test", base::Time(), base::Time()))); - scoped_ptr<ClientCertStoreStub> test_store( - new ClientCertStoreStub(dummy_certs)); - EXPECT_EQ(0, test_store->request_count()); - - // Ownership of the |test_store| is about to be turned over to ResourceLoader. - // We need to keep raw pointer copies to access these objects later. - ClientCertStoreStub* raw_ptr_to_store = test_store.get(); + scoped_ptr<ClientCertStoreStub> test_store(new ClientCertStoreStub( + dummy_certs, &store_request_count, &store_requested_authorities)); resource_context_.SetClientCertStore(test_store.Pass()); // Prepare a dummy certificate request. @@ -407,8 +407,8 @@ TEST_F(ResourceLoaderTest, ClientCertStoreLookup) { SetBrowserClientForTesting(old_client); // Check if the test store was queried against correct |cert_authorities|. - EXPECT_EQ(1, raw_ptr_to_store->request_count()); - EXPECT_EQ(dummy_authority, raw_ptr_to_store->requested_authorities()); + EXPECT_EQ(1, store_request_count); + EXPECT_EQ(dummy_authority, store_requested_authorities); // Check if the retrieved certificates were passed to the content browser // client. @@ -446,14 +446,13 @@ TEST_F(ResourceLoaderTest, ClientCertStoreNull) { 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())); + new ClientCertStoreStub(net::CertificateList(), &store_request_count, + &store_requested_authorities)); test_store->set_async(true); - EXPECT_EQ(0, test_store->request_count()); - - // Ownership of the |test_store| is about to be turned over to ResourceLoader. - // We need to keep raw pointer copies to access these objects later. - ClientCertStoreStub* raw_ptr_to_store = test_store.get(); + EXPECT_EQ(0, store_request_count); resource_context_.SetClientCertStore(test_store.Pass()); // Prepare a dummy certificate request. @@ -467,8 +466,8 @@ TEST_F(ResourceLoaderTest, ClientCertStoreAsyncCancel) { loader_->OnCertificateRequested(raw_ptr_to_request_, cert_request_info.get()); // Check if the test store was queried against correct |cert_authorities|. - EXPECT_EQ(1, raw_ptr_to_store->request_count()); - EXPECT_EQ(dummy_authority, raw_ptr_to_store->requested_authorities()); + EXPECT_EQ(1, store_request_count); + EXPECT_EQ(dummy_authority, store_requested_authorities); // Cancel the request before the store calls the callback. loader_.reset(); diff --git a/content/browser/ssl/ssl_client_auth_handler.cc b/content/browser/ssl/ssl_client_auth_handler.cc index fe2fac9..d8edc5d 100644 --- a/content/browser/ssl/ssl_client_auth_handler.cc +++ b/content/browser/ssl/ssl_client_auth_handler.cc @@ -56,6 +56,10 @@ class SSLClientAuthHandler::Core : public base::RefCountedThreadSafe<Core> { void GetClientCerts() { if (client_cert_store_) { + // TODO(davidben): This is still a cyclical ownership where + // GetClientCerts' requirement that |client_cert_store_| remains alive + // until the call completes is maintained by the reference held in the + // callback. client_cert_store_->GetClientCerts( *cert_request_info_, &cert_request_info_->client_certs, base::Bind(&SSLClientAuthHandler::Core::DidGetClientCerts, this)); @@ -85,8 +89,7 @@ SSLClientAuthHandler::SSLClientAuthHandler( net::URLRequest* request, net::SSLCertRequestInfo* cert_request_info, const SSLClientAuthHandler::CertificateCallback& callback) - : core_(nullptr), - request_(request), + : request_(request), cert_request_info_(cert_request_info), callback_(callback), weak_factory_(this) { diff --git a/content/browser/ssl/ssl_client_auth_handler.h b/content/browser/ssl/ssl_client_auth_handler.h index 84991ae..7597687 100644 --- a/content/browser/ssl/ssl_client_auth_handler.h +++ b/content/browser/ssl/ssl_client_auth_handler.h @@ -43,7 +43,8 @@ class SSLClientAuthHandler { // Called when |core_| is done retrieving the cert list. void DidGetClientCerts(); - // Called when the user has selected a cert. + // 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 diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h index 61f6a26..9fc6cdd5 100644 --- a/content/public/browser/content_browser_client.h +++ b/content/public/browser/content_browser_client.h @@ -399,7 +399,8 @@ class CONTENT_EXPORT ContentBrowserClient { CertificateRequestResultType* result) {} // Selects a SSL client certificate and returns it to the |callback|. If no - // certificate was selected nullptr is returned to the |callback|. + // certificate was selected nullptr is returned to the |callback|. Note: + // |callback| may be called synchronously or asynchronously. virtual void SelectClientCertificate( int render_process_id, int render_frame_id, |