Make client certificate store thread safe

If the users manipulate the API from the UI, we need to read from the
UI thread as well.

Change-Id: I8af787a357954cff4fbdd94bcf27b880fb6aecb4
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

7 files changed, 71 insertions, 35 deletions

diff --git a/src/core/api/qwebengineclientcertificatestore.cpp b/src/core/api/qwebengineclientcertificatestore.cpp
index 850dd16d7..40412dfd8 100644
--- a/src/core/api/qwebengineclientcertificatestore.cpp
+++ b/src/core/api/qwebengineclientcertificatestore.cpp
@@ -112,7 +112,7 @@ void QWebEngineClientCertificateStore::add(const QSslCertificate &certificate, c
 QList<QWebEngineClientCertificateStore::Entry> QWebEngineClientCertificateStore::toList() const
 {
     QList<Entry> certificateList;
-    for (auto data : qAsConst(d_ptr->addedCerts)) {
+    for (auto data : qAsConst(d_ptr->extraCerts)) {
         Entry entry;
         entry.certificate = data->certificate;
         entry.privateKey = data->key;
@@ -123,21 +123,12 @@ QList<QWebEngineClientCertificateStore::Entry> QWebEngineClientCertificateStore:
 
 /*!
     Deletes all the instances of the client certificate in the in-memory client certificate store
-    that matches the certificate in the \a entry.
+    that matches the certificate \a certificate.
 */
 
-void QWebEngineClientCertificateStore::remove(Entry entry)
+void QWebEngineClientCertificateStore::remove(const QSslCertificate &certificate)
 {
-    auto it = d_ptr->addedCerts.begin();
-    while (it != d_ptr->addedCerts.end()) {
-        auto *overrideData = *it;
-        if (entry.certificate.toDer() == overrideData->certificate.toDer()) {
-            d_ptr->deletedCerts.append(overrideData);
-            it = d_ptr->addedCerts.erase(it);
-            continue;
-        }
-        ++it;
-    }
+    d_ptr->remove(certificate);
 }
 
 /*!
@@ -146,8 +137,7 @@ void QWebEngineClientCertificateStore::remove(Entry entry)
 
 void QWebEngineClientCertificateStore::clear()
 {
-    d_ptr->deletedCerts.append(d_ptr->addedCerts);
-    d_ptr->addedCerts.clear();
+    d_ptr->clear();
 }
 
 #endif // QT_CONFIG(ssl)
diff --git a/src/core/api/qwebengineclientcertificatestore.h b/src/core/api/qwebengineclientcertificatestore.h
index c0bd66e2b..0000299a2 100644
--- a/src/core/api/qwebengineclientcertificatestore.h
+++ b/src/core/api/qwebengineclientcertificatestore.h
@@ -67,7 +67,7 @@ public:
     static QWebEngineClientCertificateStore *getInstance();
     void add(const QSslCertificate &certificate, const QSslKey &privateKey);
     QList<Entry> toList() const;
-    void remove(Entry entry);
+    void remove(const QSslCertificate &certificate);
     void clear();
 
 private:
diff --git a/src/core/net/client_cert_override.cpp b/src/core/net/client_cert_override.cpp
index caf6adad7..cbcbb03b8 100644
--- a/src/core/net/client_cert_override.cpp
+++ b/src/core/net/client_cert_override.cpp
@@ -43,6 +43,7 @@
 #include "base/bind_helpers.h"
 #include "base/task/post_task.h"
 #include "base/callback_forward.h"
+#include "content/public/browser/browser_task_traits.h"
 #include "net/ssl/client_cert_store.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/ssl/ssl_private_key.h"
@@ -106,32 +107,58 @@ ClientCertOverrideStore::ClientCertOverrideStore()
 
 ClientCertOverrideStore::~ClientCertOverrideStore() = default;
 
-void ClientCertOverrideStore::GetClientCerts(const net::SSLCertRequestInfo &cert_request_info,
-                                             const ClientCertListCallback &callback)
-{
 #if QT_CONFIG(ssl)
+// static
+net::ClientCertIdentityList ClientCertOverrideStore::GetClientCertsOnUIThread(const net::SSLCertRequestInfo &cert_request_info)
+{
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     QWebEngineClientCertificateStore *clientCertificateStore = QWebEngineClientCertificateStore::getInstance();
-    const auto &clientCertOverrideData = clientCertificateStore->d_ptr->addedCerts;
+    const auto &clientCertOverrideData = clientCertificateStore->d_ptr->extraCerts;
     // Look for certificates in memory store
     for (int i = 0; i < clientCertOverrideData.length(); i++) {
         scoped_refptr<net::X509Certificate> cert = clientCertOverrideData[i]->certPtr;
         if (cert != NULL && cert->IsIssuedByEncoded(cert_request_info.cert_authorities)) {
             net::ClientCertIdentityList selected_identities;
             selected_identities.push_back(std::make_unique<ClientCertIdentityOverride>(cert, clientCertOverrideData[i]->keyPtr));
-            callback.Run(std::move(selected_identities));
-            return;
+            return selected_identities;
         }
     }
-#endif // QT_CONFIG(ssl)
+    return net::ClientCertIdentityList();
+}
 
-    // Continue with native cert store if matching certificate is not found in memory
-    if (m_nativeStore) {
+void ClientCertOverrideStore::GetClientCertsReturn(const net::SSLCertRequestInfo &cert_request_info,
+                                                   const ClientCertListCallback &callback,
+                                                   net::ClientCertIdentityList &&result)
+{
+    // Continue with native cert store if matching certificatse were not found in memory
+    if (result.empty() && m_nativeStore)
         m_nativeStore->GetClientCerts(cert_request_info, callback);
+    else
+        callback.Run(std::move(result));
+}
+
+#endif // QT_CONFIG(ssl)
+
+void ClientCertOverrideStore::GetClientCerts(const net::SSLCertRequestInfo &cert_request_info,
+                                             const ClientCertListCallback &callback)
+{
+#if QT_CONFIG(ssl)
+    // Access the user-provided data from the UI thread, but return on whatever thread this is.
+    if (base::PostTaskWithTraitsAndReplyWithResult(
+            FROM_HERE, { content::BrowserThread::UI },
+            base::BindOnce(&GetClientCertsOnUIThread, base::ConstRef(cert_request_info)),
+            base::BindOnce(&ClientCertOverrideStore::GetClientCertsReturn,
+                           base::Unretained(this), base::ConstRef(cert_request_info), callback))
+       ) {
         return;
     }
+#endif // QT_CONFIG(ssl)
 
-    callback.Run(net::ClientCertIdentityList());
-    return;
+    // Continue with native cert store if we failed to post task
+    if (m_nativeStore)
+        m_nativeStore->GetClientCerts(cert_request_info, callback);
+    else
+        callback.Run(net::ClientCertIdentityList());
 }
 
 // static
diff --git a/src/core/net/client_cert_override.h b/src/core/net/client_cert_override.h
index 4f13c3116..04c3ce66d 100644
--- a/src/core/net/client_cert_override.h
+++ b/src/core/net/client_cert_override.h
@@ -59,6 +59,10 @@ public:
                         const ClientCertListCallback &callback) override;
 private:
     static std::unique_ptr<net::ClientCertStore> createNativeStore();
+    static net::ClientCertIdentityList GetClientCertsOnUIThread(const net::SSLCertRequestInfo &request);
+    void GetClientCertsReturn(const net::SSLCertRequestInfo &cert_request_info,
+                              const ClientCertListCallback &callback,
+                              net::ClientCertIdentityList &&result);
     std::unique_ptr<net::ClientCertStore> m_nativeStore;
 };
 
diff --git a/src/core/net/client_cert_store_data.cpp b/src/core/net/client_cert_store_data.cpp
index ae4deed1c..d1018a063 100644
--- a/src/core/net/client_cert_store_data.cpp
+++ b/src/core/net/client_cert_store_data.cpp
@@ -138,12 +138,27 @@ void ClientCertificateStoreData::add(const QSslCertificate &certificate, const Q
     data->certPtr = net::X509Certificate::CreateFromBytes(certInBytes.data(), certInBytes.length());
     data->key = privateKey;
     data->certificate = certificate;
-    addedCerts.append(data);
+    extraCerts.append(data);
 }
 
-ClientCertificateStoreData::~ClientCertificateStoreData()
+void ClientCertificateStoreData::remove(const QSslCertificate &certificate)
 {
-    qDeleteAll(deletedCerts);
+    auto it = extraCerts.begin();
+    while (it != extraCerts.end()) {
+        const QtWebEngineCore::ClientCertificateStoreData::Entry *overrideData = *it;
+        if (certificate.toDer() == overrideData->certificate.toDer()) {
+            it = extraCerts.erase(it);
+            delete overrideData;
+            continue;
+        }
+        ++it;
+    }
+}
+
+void ClientCertificateStoreData::clear()
+{
+    qDeleteAll(extraCerts);
+    extraCerts.clear();
 }
 
 } // namespace QtWebEngineCore
diff --git a/src/core/net/client_cert_store_data.h b/src/core/net/client_cert_store_data.h
index 41dc1f8ec..7f83f4b60 100644
--- a/src/core/net/client_cert_store_data.h
+++ b/src/core/net/client_cert_store_data.h
@@ -46,7 +46,7 @@
 #if QT_CONFIG(ssl)
 #include "base/memory/ref_counted.h"
 
-#include <QtCore/qlist.h>
+#include <QtCore/qvector.h>
 #include <QtNetwork/qsslcertificate.h>
 #include <QtNetwork/qsslkey.h>
 
@@ -65,11 +65,11 @@ struct ClientCertificateStoreData {
         scoped_refptr<net::SSLPrivateKey> keyPtr;
     };
 
-    ~ClientCertificateStoreData();
     void add(const QSslCertificate &certificate, const QSslKey &privateKey);
+    void remove(const QSslCertificate &certificate);
+    void clear();
 
-    QList<Entry*> addedCerts;
-    QList<Entry*> deletedCerts;
+    QVector<Entry*> extraCerts;
 };
 
 } // namespace QtWebEngineCore
diff --git a/tests/auto/core/qwebengineclientcertificatestore/tst_qwebengineclientcertificatestore.cpp b/tests/auto/core/qwebengineclientcertificatestore/tst_qwebengineclientcertificatestore.cpp
index 5c4d82f46..fe2f71418 100644
--- a/tests/auto/core/qwebengineclientcertificatestore/tst_qwebengineclientcertificatestore.cpp
+++ b/tests/auto/core/qwebengineclientcertificatestore/tst_qwebengineclientcertificatestore.cpp
@@ -85,7 +85,7 @@ void tst_QWebEngineClientCertificateStore::removeAndClearCertificates()
 
     // Remove one certificate from in-memory store
     auto list = QWebEngineClientCertificateStore::getInstance()->toList();
-    QWebEngineClientCertificateStore::getInstance()->remove(list[0]);
+    QWebEngineClientCertificateStore::getInstance()->remove(list[0].certificate);
     QCOMPARE(1, QWebEngineClientCertificateStore::getInstance()->toList().length());
 
     // Remove all certificates in-memory store