From 2af35352167257f5f3f73bea8fbee6f5506829bb Mon Sep 17 00:00:00 2001 From: "Juergen E. Fischer" Date: Mon, 4 Mar 2019 00:34:15 +0100 Subject: [PATCH] [needsbackport] apply an alternative fix for #20826 Partly reverts c9e761649, 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. --- .../core/auto_generated/qgscredentials.sip.in | 15 +++++--- src/app/qgisapp.cpp | 29 ++++++-------- src/app/qgsappauthrequesthandler.cpp | 28 ++++++++------ src/app/qgsgeometryvalidationdock.cpp | 4 -- src/core/auth/qgsauthmanager.cpp | 3 +- src/core/qgscredentials.cpp | 38 ++++++++----------- src/core/qgscredentials.h | 25 ++++++------ src/core/qgsnetworkaccessmanager.cpp | 1 + src/gui/qgscredentialdialog.cpp | 6 ++- src/providers/db2/qgsdb2provider.cpp | 3 ++ src/providers/grass/qgsgrass.cpp | 2 +- src/providers/oracle/qgsoracleconn.cpp | 4 ++ src/providers/postgres/qgspostgresconn.cpp | 4 ++ 13 files changed, 86 insertions(+), 76 deletions(-) diff --git a/python/core/auto_generated/qgscredentials.sip.in b/python/core/auto_generated/qgscredentials.sip.in index 35d1447d5aff..ff6c33dc1e9c 100644 --- 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 index 32577a3654c4..5b6ea97b7333 100644 --- 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 index 61387d577b60..82b25cb80a3d 100644 --- a/src/app/qgsappauthrequesthandler.cpp +++ b/src/app/qgsappauthrequesthandler.cpp @@ -20,10 +20,13 @@ #include "qgsauthmanager.h" #include "qgisapp.h" #include "qgscredentials.h" + #include 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 index ab5e96694d07..6f76a63ab9a9 100644 --- a/src/app/qgsgeometryvalidationdock.cpp +++ b/src/app/qgsgeometryvalidationdock.cpp @@ -289,7 +289,6 @@ void QgsGeometryValidationDock::onCurrentLayerChanged( QgsMapLayer *layer ) void QgsGeometryValidationDock::onLayerEditingStatusChanged() { - bool enabled = false; if ( mCurrentLayer && mCurrentLayer->isSpatial() && mCurrentLayer->isEditable() ) { const QList topologyCheckFactories = QgsAnalysis::instance()->geometryCheckRegistry()->geometryCheckFactories( mCurrentLayer, QgsGeometryCheck::LayerCheck, QgsGeometryCheck::Flag::AvailableInValidation ); @@ -297,10 +296,7 @@ void QgsGeometryValidationDock::onLayerEditingStatusChanged() 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 index e69b1cb5d642..7752180dfad3 100644 --- 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 index e39c26be7d1f..91e29845f4fb 100644 --- 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 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 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( 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 index 7b623b141640..9aab8ddc6e78 100644 --- 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 index 3ad9754d778d..75149606f532 100644 --- 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 index 5f28ffcc2049..7e308633691b 100644 --- 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 index 3e28512f11b8..c42c0cba164e 100644 --- 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 index 03fcd401fbd0..63efaa8b848f 100644 --- 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 index 25af0f0ecf78..e9e39ccc66a7 100644 --- 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 index b94fbfc2bfc2..5deee21c82e8 100644 --- 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 )