Make network authentication request handling thread safe, avoid

nyalldawson · nyalldawson · commit 8ee2e793f8cc · 2019-02-01T21:44:53.000+11:00
races and crashes when auth requests occur in non-main thread

Like the recent SSL error handling, this commit abstracts out
the network authentication handling into a thread safe approach.
diff --git a/python/core/auto_generated/qgsnetworkaccessmanager.sip.in b/python/core/auto_generated/qgsnetworkaccessmanager.sip.in
@@ -268,6 +268,22 @@ This signal is propagated to the main thread QgsNetworkAccessManager instance, s
 only to connect to the main thread's signal in order to receive notifications about requests
 created in any thread.
 
+.. versionadded:: 3.6
+%End
+
+    void requestRequiresAuth( int requestId, const QString &realm );
+%Docstring
+Emitted when a network request prompts an authentication request.
+
+The ``requestId`` argument reflects the unique ID identifying the original request which the authentication relates to.
+
+This signal is propagated to the main thread QgsNetworkAccessManager instance, so it is necessary
+only to connect to the main thread's signal in order to receive notifications about authentication requests
+from any thread.
+
+This signal is for debugging and logging purposes only, and cannot be used to respond to the
+requests. See QgsNetworkAuthenticationHandler for details on how to handle authentication requests.
+
 .. versionadded:: 3.6
 %End
 
@@ -276,12 +292,15 @@ created in any thread.
 %Docstring
 Emitted when a network request encounters SSL ``errors``.
 
-The ``requestId`` argument reflects the unique ID identifying the original request which the progress report relates to.
+The ``requestId`` argument reflects the unique ID identifying the original request which the SSL error relates to.
 
 This signal is propagated to the main thread QgsNetworkAccessManager instance, so it is necessary
 only to connect to the main thread's signal in order to receive notifications about SSL errors
 from any thread.
 
+This signal is for debugging and logging purposes only, and cannot be used to respond to the errors.
+See QgsSslErrorHandler for details on how to handle SSL errors and potentially ignore them.
+
 .. versionadded:: 3.6
 %End
 
@@ -296,6 +315,7 @@ from any thread.
     void requestTimedOut( QNetworkReply * );
 
 
+
   protected:
     virtual QNetworkReply *createRequest( QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *outgoingData = 0 );
 
diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt
@@ -8,6 +8,7 @@ SET(QGIS_APP_SRCS
   qgisappstylesheet.cpp
   qgsabout.cpp
   qgsalignrasterdialog.cpp
+  qgsappauthrequesthandler.cpp
   qgsappbrowserproviders.cpp
   qgsapplayertreeviewmenuprovider.cpp
   qgsappwindowmanager.cpp
diff --git a/src/app/qgisapp.cpp b/src/app/qgisapp.cpp
@@ -142,6 +142,7 @@ Q_GUI_EXPORT extern int qt_defaultDpiX();
 #include "qgisplugin.h"
 #include "qgsabout.h"
 #include "qgsalignrasterdialog.h"
+#include "qgsappauthrequesthandler.h"
 #include "qgsappbrowserproviders.h"
 #include "qgsapplayertreeviewmenuprovider.h"
 #include "qgsapplication.h"
@@ -13883,85 +13884,18 @@ void QgisApp::namSetup()
 {
   QgsNetworkAccessManager *nam = QgsNetworkAccessManager::instance();
 
-  connect( nam, &QNetworkAccessManager::authenticationRequired,
-           this, &QgisApp::namAuthenticationRequired );
-
   connect( nam, &QNetworkAccessManager::proxyAuthenticationRequired,
            this, &QgisApp::namProxyAuthenticationRequired );
 
   connect( nam, qgis::overload< QgsNetworkRequestParameters >::of( &QgsNetworkAccessManager::requestTimedOut ),
            this, &QgisApp::namRequestTimedOut );
 
+  nam->setAuthHandler( qgis::make_unique<QgsAppAuthRequestHandler>() );
 #ifndef QT_NO_SSL
   nam->setSslErrorHandler( qgis::make_unique<QgsAppSslErrorHandler>() );
 #endif
 }
 
-void QgisApp::namAuthenticationRequired( QNetworkReply *inReply, QAuthenticator *auth )
-{
-  QPointer<QNetworkReply> reply( inReply );
-  Q_ASSERT( qApp->thread() == QThread::currentThread() );
-
-  QString username = auth->user();
-  QString password = auth->password();
-
-  if ( username.isEmpty() && password.isEmpty() && reply->request().hasRawHeader( "Authorization" ) )
-  {
-    QByteArray header( reply->request().rawHeader( "Authorization" ) );
-    if ( header.startsWith( "Basic " ) )
-    {
-      QByteArray auth( QByteArray::fromBase64( header.mid( 6 ) ) );
-      int pos = auth.indexOf( ':' );
-      if ( pos >= 0 )
-      {
-        username = auth.left( pos );
-        password = auth.mid( pos + 1 );
-      }
-    }
-  }
-
-  for ( ;; )
-  {
-    bool ok;
-
-    {
-      QMutexLocker lock( QgsCredentials::instance()->mutex() );
-      ok = QgsCredentials::instance()->get(
-             QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
-             username, password,
-             tr( "Authentication required" ) );
-    }
-    if ( !ok )
-      return;
-
-    if ( reply.isNull() || reply->isFinished() )
-      return;
-
-    if ( auth->user() != username || ( password != auth->password() && !password.isNull() ) )
-      break;
-
-    // credentials didn't change - stored ones probably wrong? clear password and retry
-    {
-      QMutexLocker lock( QgsCredentials::instance()->mutex() );
-      QgsCredentials::instance()->put(
-        QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
-        username, QString() );
-    }
-  }
-
-  // save credentials
-  {
-    QMutexLocker lock( QgsCredentials::instance()->mutex() );
-    QgsCredentials::instance()->put(
-      QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
-      username, password
-    );
-  }
-
-  auth->setUser( username );
-  auth->setPassword( password );
-}
-
 void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthenticator *auth )
 {
   QgsSettings settings;
diff --git a/src/app/qgisapp.h b/src/app/qgisapp.h
@@ -879,7 +879,6 @@ class APP_EXPORT QgisApp : public QMainWindow, private Ui::MainWindow
     void setAppStyleSheet( const QString &stylesheet );
 
     //! request credentials for network manager
-    void namAuthenticationRequired( QNetworkReply *reply, QAuthenticator *auth );
     void namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthenticator *auth );
     void namRequestTimedOut( const QgsNetworkRequestParameters &request );
 
diff --git a/src/app/qgsappauthrequesthandler.cpp b/src/app/qgsappauthrequesthandler.cpp
@@ -0,0 +1,82 @@
+/***************************************************************************
+    qgsappauthrequesthandler.cpp
+    ---------------------------
+    begin                : January 2019
+    copyright            : (C) 2019 by Nyall Dawson
+    email                : nyall dot dawson at gmail dot com
+ ***************************************************************************
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ ***************************************************************************/
+
+#include "qgsappauthrequesthandler.h"
+#include "qgslogger.h"
+#include "qgsauthcertutils.h"
+#include "qgsapplication.h"
+#include "qgsauthmanager.h"
+#include "qgisapp.h"
+#include "qgscredentials.h"
+#include <QAuthenticator>
+
+void QgsAppAuthRequestHandler::handleAuthRequest( QNetworkReply *reply, QAuthenticator *auth )
+{
+  QString username = auth->user();
+  QString password = auth->password();
+
+  if ( username.isEmpty() && password.isEmpty() && reply->request().hasRawHeader( "Authorization" ) )
+  {
+    QByteArray header( reply->request().rawHeader( "Authorization" ) );
+    if ( header.startsWith( "Basic " ) )
+    {
+      QByteArray auth( QByteArray::fromBase64( header.mid( 6 ) ) );
+      int pos = auth.indexOf( ':' );
+      if ( pos >= 0 )
+      {
+        username = auth.left( pos );
+        password = auth.mid( pos + 1 );
+      }
+    }
+  }
+
+  for ( ;; )
+  {
+    bool ok;
+
+    {
+      QMutexLocker lock( QgsCredentials::instance()->mutex() );
+      ok = QgsCredentials::instance()->get(
+             QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
+             username, password,
+             QObject::tr( "Authentication required" ) );
+    }
+    if ( !ok )
+      return;
+
+    if ( auth->user() != username || ( password != auth->password() && !password.isNull() ) )
+      break;
+
+    // credentials didn't change - stored ones probably wrong? clear password and retry
+    {
+      QMutexLocker lock( QgsCredentials::instance()->mutex() );
+      QgsCredentials::instance()->put(
+        QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
+        username, QString() );
+    }
+  }
+
+  // save credentials
+  {
+    QMutexLocker lock( QgsCredentials::instance()->mutex() );
+    QgsCredentials::instance()->put(
+      QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
+      username, password
+    );
+  }
+
+  auth->setUser( username );
+  auth->setPassword( password );
+}
diff --git a/src/app/qgsappauthrequesthandler.h b/src/app/qgsappauthrequesthandler.h
@@ -0,0 +1,30 @@
+/***************************************************************************
+    qgsappauthrequesthandler.h
+    -------------------------
+    begin                : January 2019
+    copyright            : (C) 2019 by Nyall Dawson
+    email                : nyall dot dawson at gmail dot com
+ ***************************************************************************
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ ***************************************************************************/
+#ifndef QGSAPPAUTHREQUESTHANDLER_H
+#define QGSAPPAUTHREQUESTHANDLER_H
+
+#include "qgsnetworkaccessmanager.h"
+
+class QgsAppAuthRequestHandler : public QgsNetworkAuthenticationHandler
+{
+
+  public:
+
+    void handleAuthRequest( QNetworkReply *reply, QAuthenticator *auth );
+
+};
+
+
+#endif // QGSAPPAUTHREQUESTHANDLER_H
diff --git a/src/core/qgsnetworkaccessmanager.cpp b/src/core/qgsnetworkaccessmanager.cpp
diff --git a/src/core/qgsnetworkaccessmanager.h b/src/core/qgsnetworkaccessmanager.h
diff --git a/tests/src/core/testqgsnetworkaccessmanager.cpp b/tests/src/core/testqgsnetworkaccessmanager.cpp
