Skip to content
Permalink
Browse files

[needsbackport] apply an alternative fix for #20826 & #21582

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.

(cherry picked from commit 2af3535)
  • Loading branch information
jef-n committed Mar 3, 2019
1 parent bd0667c commit e26c4bf36906dcf056caf520e5eab5d2c86db5b7
@@ -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:

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 );
}
@@ -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 );
}
@@ -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 )
@@ -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();
}


@@ -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;
};


@@ -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
@@ -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();
@@ -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;
}
@@ -700,7 +700,7 @@ void QgsGrass::loadMapsetSearchPath()

void QgsGrass::setMapsetSearchPathWatcher()
{
QgsDebugMsg( "etered" );
QgsDebugMsg( "entered." );
if ( mMapsetSearchPathWatcher )
{
delete mMapsetSearchPathWatcher;
@@ -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() )
@@ -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 )

0 comments on commit e26c4bf

Please sign in to comment.
You can’t perform that action at this time.