[needsbackport] apply an alternative fix for #20826

Partly reverts c9e7616, which removed the synchronizatiion of credential requests (eg. in a project that has multiple layers from the same postgresql database without credentials) and led to multiple concurrent requests for the same credentials. Some of which were silently discarded, when events processed in the dialogs exec() event loop tried to reinvoke the dialog and caused invalid layers. Authentications caused by network requests can still cause this. The credential cache is now guarded by a separate mutex.
qgis · Mar 23, 2019 · 2af3535 · 2af3535
1 parent 78b5678
commit 2af3535
Show file tree

Hide file tree

Showing 13 changed files with 86 additions and 76 deletions.
diff --git a/python/core/auto_generated/qgscredentials.sip.in b/python/core/auto_generated/qgscredentials.sip.in
@@ -20,6 +20,9 @@ credential creator function.
 
 QGIS application uses QgsCredentialDialog class for displaying a dialog to the user.
 
+Caller can use the mutex to synchronize authentications to avoid requesting
+credentials for the same resource several times.
+
 Object deletes itself when it's not needed anymore. Children should use
 signal destroyed() to be notified of the deletion
 %End
@@ -68,27 +71,27 @@ combinations are used with this method.
 retrieves instance
 %End
 
- void lock() /Deprecated/;
+    void lock();
 %Docstring
 Lock the instance against access from multiple threads. This does not really lock access to get/put methds,
 it will just prevent other threads to lock the instance and continue the execution. When the class is used
 from non-GUI threads, they should call lock() before the get/put calls to avoid race conditions.
 
-.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
+.. versionadded:: 2.4
 %End
 
- void unlock() /Deprecated/;
+    void unlock();
 %Docstring
 Unlock the instance after being locked.
 
-.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
+.. versionadded:: 2.4
 %End
 
- QMutex *mutex() /Deprecated/;
+    QMutex *mutex();
 %Docstring
 Returns pointer to mutex
 
-.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
+.. versionadded:: 2.4
 %End
 
   protected:

diff --git a/src/app/qgisapp.cpp b/src/app/qgisapp.cpp
@@ -13956,35 +13956,30 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe
 
   for ( ;; )
   {
-    bool ok;
-
-    {
-      ok = QgsCredentials::instance()->get(
-             QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
-             username, password,
-             tr( "Proxy authentication required" ) );
-    }
+    bool ok = QgsCredentials::instance()->get(
+                QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
+                username, password,
+                tr( "Proxy authentication required" ) );
     if ( !ok )
       return;
 
     if ( auth->user() != username || ( password != auth->password() && !password.isNull() ) )
+    {
+      QgsCredentials::instance()->put(
+        QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
+        username, password
+      );
       break;
-
-    // credentials didn't change - stored ones probably wrong? clear password and retry
+    }
+    else
     {
+      // credentials didn't change - stored ones probably wrong? clear password and retry
       QgsCredentials::instance()->put(
         QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
         username, QString() );
     }
   }
 
-  {
-    QgsCredentials::instance()->put(
-      QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
-      username, password
-    );
-  }
-
   auth->setUser( username );
   auth->setPassword( password );
 }

diff --git a/src/app/qgsappauthrequesthandler.cpp b/src/app/qgsappauthrequesthandler.cpp
@@ -20,10 +20,13 @@
 #include "qgsauthmanager.h"
 #include "qgisapp.h"
 #include "qgscredentials.h"
+
 #include <QAuthenticator>
 
 void QgsAppAuthRequestHandler::handleAuthRequest( QNetworkReply *reply, QAuthenticator *auth )
 {
+  Q_ASSERT( qApp->thread() == QThread::currentThread() );
+
   QString username = auth->user();
   QString password = auth->password();
 
@@ -52,20 +55,23 @@ void QgsAppAuthRequestHandler::handleAuthRequest( QNetworkReply *reply, QAuthent
       return;
 
     if ( auth->user() != username || ( password != auth->password() && !password.isNull() ) )
+    {
+      // save credentials
+      QgsCredentials::instance()->put(
+        QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
+        username, password
+      );
       break;
-
-    // credentials didn't change - stored ones probably wrong? clear password and retry
-    QgsCredentials::instance()->put(
-      QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
-      username, QString() );
+    }
+    else
+    {
+      // credentials didn't change - stored ones probably wrong? clear password and retry
+      QgsCredentials::instance()->put(
+        QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
+        username, QString() );
+    }
   }
 
-  // save credentials
-  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/qgsgeometryvalidationdock.cpp b/src/app/qgsgeometryvalidationdock.cpp
@@ -289,18 +289,14 @@ void QgsGeometryValidationDock::onCurrentLayerChanged( QgsMapLayer *layer )
 
 void QgsGeometryValidationDock::onLayerEditingStatusChanged()
 {
-  bool enabled = false;
   if ( mCurrentLayer && mCurrentLayer->isSpatial() && mCurrentLayer->isEditable() )
   {
     const QList<QgsGeometryCheckFactory *> topologyCheckFactories = QgsAnalysis::instance()->geometryCheckRegistry()->geometryCheckFactories( mCurrentLayer, QgsGeometryCheck::LayerCheck, QgsGeometryCheck::Flag::AvailableInValidation );
     const QStringList activeChecks = mCurrentLayer->geometryOptions()->geometryChecks();
     for ( const QgsGeometryCheckFactory *factory : topologyCheckFactories )
     {
       if ( activeChecks.contains( factory->id() ) )
-      {
-        enabled = true;
         break;
-      }
     }
   }
 }

diff --git a/src/core/auth/qgsauthmanager.cpp b/src/core/auth/qgsauthmanager.cpp
@@ -3203,9 +3203,8 @@ bool QgsAuthManager::masterPasswordInput()
 
   if ( ! ok )
   {
-    QgsCredentials *creds = QgsCredentials::instance();
     pass.clear();
-    ok = creds->getMasterPassword( pass, masterPasswordHashInDatabase() );
+    ok = QgsCredentials::instance()->getMasterPassword( pass, masterPasswordHashInDatabase() );
   }
 
   if ( ok && !pass.isEmpty() && mMasterPass != pass )

diff --git a/src/core/qgscredentials.cpp b/src/core/qgscredentials.cpp
@@ -40,27 +40,23 @@ QgsCredentials *QgsCredentials::instance()
 
 bool QgsCredentials::get( const QString &realm, QString &username, QString &password, const QString &message )
 {
-  QMutexLocker locker( &mMutex );
-  if ( mCredentialCache.contains( realm ) )
   {
-    QPair<QString, QString> credentials = mCredentialCache.take( realm );
-    locker.unlock();
-    username = credentials.first;
-    password = credentials.second;
-#if 0 // don't leak credentials on log
-    QgsDebugMsg( QStringLiteral( "retrieved realm:%1 username:%2 password:%3" ).arg( realm, username, password ) );
-#endif
-
-    if ( !password.isNull() )
-      return true;
+    QMutexLocker locker( &mCacheMutex );
+    if ( mCredentialCache.contains( realm ) )
+    {
+      QPair<QString, QString> credentials = mCredentialCache.take( realm );
+      username = credentials.first;
+      password = credentials.second;
+      QgsDebugMsg( QStringLiteral( "retrieved realm:%1 username:%2" ).arg( realm, username ) );
+
+      if ( !password.isNull() )
+        return true;
+    }
   }
-  locker.unlock();
 
   if ( request( realm, username, password, message ) )
   {
-#if 0 // don't leak credentials on log
-    QgsDebugMsg( QStringLiteral( "requested realm:%1 username:%2 password:%3" ).arg( realm, username, password ) );
-#endif
+    QgsDebugMsg( QStringLiteral( "requested realm:%1 username:%2" ).arg( realm, username ) );
     return true;
   }
   else
@@ -72,10 +68,8 @@ bool QgsCredentials::get( const QString &realm, QString &username, QString &pass
 
 void QgsCredentials::put( const QString &realm, const QString &username, const QString &password )
 {
-#if 0 // don't leak credentials on log
-  QgsDebugMsg( QStringLiteral( "inserting realm:%1 username:%2 password:%3" ).arg( realm, username, password ) );
-#endif
-  QMutexLocker locker( &mMutex );
+  QMutexLocker locker( &mCacheMutex );
+  QgsDebugMsg( QStringLiteral( "inserting realm:%1 username:%2" ).arg( realm, username ) );
   mCredentialCache.insert( realm, QPair<QString, QString>( username, password ) );
 }
 
@@ -91,12 +85,12 @@ bool QgsCredentials::getMasterPassword( QString &password, bool stored )
 
 void QgsCredentials::lock()
 {
-  mMutex.lock();
+  mAuthMutex.lock();
 }
 
 void QgsCredentials::unlock()
 {
-  mMutex.unlock();
+  mAuthMutex.unlock();
 }
 
 

diff --git a/src/core/qgscredentials.h b/src/core/qgscredentials.h
@@ -35,6 +35,9 @@
 
  * QGIS application uses QgsCredentialDialog class for displaying a dialog to the user.
 
+ * Caller can use the mutex to synchronize authentications to avoid requesting
+ * credentials for the same resource several times.
+
  * Object deletes itself when it's not needed anymore. Children should use
  * signal destroyed() to be notified of the deletion
 */
@@ -84,25 +87,21 @@ class CORE_EXPORT QgsCredentials
      * Lock the instance against access from multiple threads. This does not really lock access to get/put methds,
      * it will just prevent other threads to lock the instance and continue the execution. When the class is used
      * from non-GUI threads, they should call lock() before the get/put calls to avoid race conditions.
-     *
-     * \deprecated since QGIS 3.4 - mutex locking is automatically handled
+     * \since QGIS 2.4
      */
-    Q_DECL_DEPRECATED void lock() SIP_DEPRECATED;
+    void lock();
 
     /**
      * Unlock the instance after being locked.
-     * \deprecated since QGIS 3.4 - mutex locking is automatically handled
+     * \since QGIS 2.4
      */
-    Q_DECL_DEPRECATED void unlock() SIP_DEPRECATED;
+    void unlock();
 
     /**
      * Returns pointer to mutex
-     * \deprecated since QGIS 3.4 - mutex locking is automatically handled
+     * \since QGIS 2.4
      */
-    Q_DECL_DEPRECATED QMutex *mutex() SIP_DEPRECATED
-    {
-      return &mMutex;
-    }
+    QMutex *mutex() { return &mAuthMutex; }
 
   protected:
 
@@ -133,7 +132,11 @@ class CORE_EXPORT QgsCredentials
     //! Pointer to the credential instance
     static QgsCredentials *sInstance;
 
-    QMutex mMutex;
+    //! Mutex to synchronize authentications
+    QMutex mAuthMutex;
+
+    //! Mutex to guard the cache
+    QMutex mCacheMutex;
 };
 
 

diff --git a/src/core/qgsnetworkaccessmanager.cpp b/src/core/qgsnetworkaccessmanager.cpp
@@ -414,6 +414,7 @@ void QgsNetworkAccessManager::onAuthRequired( QNetworkReply *reply, QAuthenticat
   // in main thread this will trigger auth handler immediately and return once the request is satisfied,
   // while in worker thread the signal will be queued (and return immediately) -- hence the need to lock the thread in the next block
   emit authRequestOccurred( reply, auth );
+
   if ( this != sMainNAM )
   {
     // lock thread and wait till error is handled. If we return from this slot now, then the reply will resume

diff --git a/src/gui/qgscredentialdialog.cpp b/src/gui/qgscredentialdialog.cpp
@@ -59,7 +59,7 @@ bool QgsCredentialDialog::request( const QString &realm, QString &username, QStr
   {
     QgsDebugMsg( QStringLiteral( "emitting signal" ) );
     emit credentialsRequested( realm, &username, &password, message, &ok );
-    QgsDebugMsg( QStringLiteral( "signal returned %1 (username=%2, password=%3)" ).arg( ok ? "true" : "false", username, password ) );
+    QgsDebugMsg( QStringLiteral( "signal returned %1 (username=%2)" ).arg( ok ? "true" : "false", username ) );
   }
   else
   {
@@ -81,7 +81,9 @@ void QgsCredentialDialog::requestCredentials( const QString &realm, QString *use
   labelMessage->setText( message );
   labelMessage->setHidden( message.isEmpty() );
 
-  if ( !leUsername->text().isEmpty() )
+  if ( leUsername->text().isEmpty() )
+    leUsername->setFocus();
+  else
     lePassword->setFocus();
 
   QWidget *activeWindow = qApp->activeWindow();

diff --git a/src/providers/db2/qgsdb2provider.cpp b/src/providers/db2/qgsdb2provider.cpp
@@ -235,6 +235,7 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
   db.setPort( port.toInt() );
   bool connected = false;
   int i = 0;
+  QgsCredentials::instance()->lock();
   while ( !connected && i < 3 )
   {
     i++;
@@ -248,6 +249,7 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
       {
         errMsg = QStringLiteral( "Cancel clicked" );
         QgsDebugMsg( errMsg );
+        QgsCredentials::instance()->unlock();
         break;
       }
     }
@@ -289,6 +291,7 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
   {
     QgsCredentials::instance()->put( databaseName, userName, password );
   }
+  QgsCredentials::instance()->unlock();
 
   return db;
 }

diff --git a/src/providers/grass/qgsgrass.cpp b/src/providers/grass/qgsgrass.cpp
@@ -700,7 +700,7 @@ void QgsGrass::loadMapsetSearchPath()
 
 void QgsGrass::setMapsetSearchPathWatcher()
 {
-  QgsDebugMsg( "etered" );
+  QgsDebugMsg( "entered." );
   if ( mMapsetSearchPathWatcher )
   {
     delete mMapsetSearchPathWatcher;

diff --git a/src/providers/oracle/qgsoracleconn.cpp b/src/providers/oracle/qgsoracleconn.cpp
@@ -96,6 +96,8 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri )
   QgsDebugMsg( QStringLiteral( "Connecting with options: " ) + options );
   if ( !mDatabase.open() )
   {
+    QgsCredentials::instance()->lock();
+
     while ( !mDatabase.open() )
     {
       bool ok = QgsCredentials::instance()->get( realm, username, password, mDatabase.lastError().text() );
@@ -125,6 +127,8 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri )
 
     if ( mDatabase.isOpen() )
       QgsCredentials::instance()->put( realm, username, password );
+
+    QgsCredentials::instance()->unlock();
   }
 
   if ( !mDatabase.isOpen() )

diff --git a/src/providers/postgres/qgspostgresconn.cpp b/src/providers/postgres/qgspostgresconn.cpp
@@ -272,6 +272,8 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s
     QString username = uri.username();
     QString password = uri.password();
 
+    QgsCredentials::instance()->lock();
+
     int i = 0;
     while ( PQstatus() != CONNECTION_OK && i < 5 )
     {
@@ -296,6 +298,8 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s
 
     if ( PQstatus() == CONNECTION_OK )
       QgsCredentials::instance()->put( conninfo, username, password );
+
+    QgsCredentials::instance()->unlock();
   }
 
   if ( PQstatus() != CONNECTION_OK )