Skip to content

Commit

Permalink
[needsbackport] apply an alternative fix for #20826 & #21582
Browse files Browse the repository at this point in the history
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 29, 2019
1 parent ebb18d5 commit f7d47f0
Show file tree
Hide file tree
Showing 10 changed files with 68 additions and 61 deletions.
15 changes: 9 additions & 6 deletions python/core/auto_generated/qgscredentials.sip.in
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
29 changes: 12 additions & 17 deletions src/app/qgisapp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13755,35 +13755,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 );
}
Expand Down
3 changes: 1 addition & 2 deletions src/core/auth/qgsauthmanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3205,9 +3205,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 )
Expand Down
38 changes: 16 additions & 22 deletions src/core/qgscredentials.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 ) );
}

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


Expand Down
25 changes: 14 additions & 11 deletions src/core/qgscredentials.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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:

Expand Down Expand Up @@ -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;
};


Expand Down
6 changes: 4 additions & 2 deletions src/gui/qgscredentialdialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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();
Expand Down
3 changes: 3 additions & 0 deletions src/providers/db2/qgsdb2provider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,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++;
Expand All @@ -214,6 +215,7 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
{
errMsg = QStringLiteral( "Cancel clicked" );
QgsDebugMsg( errMsg );
QgsCredentials::instance()->unlock();
break;
}
}
Expand Down Expand Up @@ -255,6 +257,7 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
{
QgsCredentials::instance()->put( databaseName, userName, password );
}
QgsCredentials::instance()->unlock();

return db;
}
Expand Down
2 changes: 1 addition & 1 deletion src/providers/grass/qgsgrass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ void QgsGrass::loadMapsetSearchPath()

void QgsGrass::setMapsetSearchPathWatcher()
{
QgsDebugMsg( "etered" );
QgsDebugMsg( "entered." );
if ( mMapsetSearchPathWatcher )
{
delete mMapsetSearchPathWatcher;
Expand Down
4 changes: 4 additions & 0 deletions src/providers/oracle/qgsoracleconn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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() );
Expand Down Expand Up @@ -125,6 +127,8 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri )

if ( mDatabase.isOpen() )
QgsCredentials::instance()->put( realm, username, password );

QgsCredentials::instance()->unlock();
}

if ( !mDatabase.isOpen() )
Expand Down
4 changes: 4 additions & 0 deletions src/providers/postgres/qgspostgresconn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
{
Expand All @@ -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 )
Expand Down

0 comments on commit f7d47f0

Please sign in to comment.