Skip to content

Commit f7d47f0

Browse files
jef-njef-n
committedMar 29, 2019
[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)
·
final-3_4_15final-3_4_7
1 parent ebb18d5 commit f7d47f0

File tree

10 files changed

+68
-61
lines changed

10 files changed

+68
-61
lines changed
 

‎python/core/auto_generated/qgscredentials.sip.in

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@ credential creator function.
2020

2121
QGIS application uses QgsCredentialDialog class for displaying a dialog to the user.
2222

23+
Caller can use the mutex to synchronize authentications to avoid requesting
24+
credentials for the same resource several times.
25+
2326
Object deletes itself when it's not needed anymore. Children should use
2427
signal destroyed() to be notified of the deletion
2528
%End
@@ -68,27 +71,27 @@ combinations are used with this method.
6871
retrieves instance
6972
%End
7073

71-
void lock() /Deprecated/;
74+
void lock();
7275
%Docstring
7376
Lock the instance against access from multiple threads. This does not really lock access to get/put methds,
7477
it will just prevent other threads to lock the instance and continue the execution. When the class is used
7578
from non-GUI threads, they should call lock() before the get/put calls to avoid race conditions.
7679

77-
.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
80+
.. versionadded:: 2.4
7881
%End
7982

80-
void unlock() /Deprecated/;
83+
void unlock();
8184
%Docstring
8285
Unlock the instance after being locked.
8386

84-
.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
87+
.. versionadded:: 2.4
8588
%End
8689

87-
QMutex *mutex() /Deprecated/;
90+
QMutex *mutex();
8891
%Docstring
8992
Returns pointer to mutex
9093

91-
.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
94+
.. versionadded:: 2.4
9295
%End
9396

9497
protected:

‎src/app/qgisapp.cpp

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13755,35 +13755,30 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe
1375513755

1375613756
for ( ;; )
1375713757
{
13758-
bool ok;
13759-
13760-
{
13761-
ok = QgsCredentials::instance()->get(
13762-
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
13763-
username, password,
13764-
tr( "Proxy authentication required" ) );
13765-
}
13758+
bool ok = QgsCredentials::instance()->get(
13759+
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
13760+
username, password,
13761+
tr( "Proxy authentication required" ) );
1376613762
if ( !ok )
1376713763
return;
1376813764

1376913765
if ( auth->user() != username || ( password != auth->password() && !password.isNull() ) )
13766+
{
13767+
QgsCredentials::instance()->put(
13768+
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
13769+
username, password
13770+
);
1377013771
break;
13771-
13772-
// credentials didn't change - stored ones probably wrong? clear password and retry
13772+
}
13773+
else
1377313774
{
13775+
// credentials didn't change - stored ones probably wrong? clear password and retry
1377413776
QgsCredentials::instance()->put(
1377513777
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
1377613778
username, QString() );
1377713779
}
1377813780
}
1377913781

13780-
{
13781-
QgsCredentials::instance()->put(
13782-
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
13783-
username, password
13784-
);
13785-
}
13786-
1378713782
auth->setUser( username );
1378813783
auth->setPassword( password );
1378913784
}

‎src/core/auth/qgsauthmanager.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3205,9 +3205,8 @@ bool QgsAuthManager::masterPasswordInput()
32053205

32063206
if ( ! ok )
32073207
{
3208-
QgsCredentials *creds = QgsCredentials::instance();
32093208
pass.clear();
3210-
ok = creds->getMasterPassword( pass, masterPasswordHashInDatabase() );
3209+
ok = QgsCredentials::instance()->getMasterPassword( pass, masterPasswordHashInDatabase() );
32113210
}
32123211

32133212
if ( ok && !pass.isEmpty() && mMasterPass != pass )

‎src/core/qgscredentials.cpp

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -40,27 +40,23 @@ QgsCredentials *QgsCredentials::instance()
4040

4141
bool QgsCredentials::get( const QString &realm, QString &username, QString &password, const QString &message )
4242
{
43-
QMutexLocker locker( &mMutex );
44-
if ( mCredentialCache.contains( realm ) )
4543
{
46-
QPair<QString, QString> credentials = mCredentialCache.take( realm );
47-
locker.unlock();
48-
username = credentials.first;
49-
password = credentials.second;
50-
#if 0 // don't leak credentials on log
51-
QgsDebugMsg( QStringLiteral( "retrieved realm:%1 username:%2 password:%3" ).arg( realm, username, password ) );
52-
#endif
53-
54-
if ( !password.isNull() )
55-
return true;
44+
QMutexLocker locker( &mCacheMutex );
45+
if ( mCredentialCache.contains( realm ) )
46+
{
47+
QPair<QString, QString> credentials = mCredentialCache.take( realm );
48+
username = credentials.first;
49+
password = credentials.second;
50+
QgsDebugMsg( QStringLiteral( "retrieved realm:%1 username:%2" ).arg( realm, username ) );
51+
52+
if ( !password.isNull() )
53+
return true;
54+
}
5655
}
57-
locker.unlock();
5856

5957
if ( request( realm, username, password, message ) )
6058
{
61-
#if 0 // don't leak credentials on log
62-
QgsDebugMsg( QStringLiteral( "requested realm:%1 username:%2 password:%3" ).arg( realm, username, password ) );
63-
#endif
59+
QgsDebugMsg( QStringLiteral( "requested realm:%1 username:%2" ).arg( realm, username ) );
6460
return true;
6561
}
6662
else
@@ -72,10 +68,8 @@ bool QgsCredentials::get( const QString &realm, QString &username, QString &pass
7268

7369
void QgsCredentials::put( const QString &realm, const QString &username, const QString &password )
7470
{
75-
#if 0 // don't leak credentials on log
76-
QgsDebugMsg( QStringLiteral( "inserting realm:%1 username:%2 password:%3" ).arg( realm, username, password ) );
77-
#endif
78-
QMutexLocker locker( &mMutex );
71+
QMutexLocker locker( &mCacheMutex );
72+
QgsDebugMsg( QStringLiteral( "inserting realm:%1 username:%2" ).arg( realm, username ) );
7973
mCredentialCache.insert( realm, QPair<QString, QString>( username, password ) );
8074
}
8175

@@ -91,12 +85,12 @@ bool QgsCredentials::getMasterPassword( QString &password, bool stored )
9185

9286
void QgsCredentials::lock()
9387
{
94-
mMutex.lock();
88+
mAuthMutex.lock();
9589
}
9690

9791
void QgsCredentials::unlock()
9892
{
99-
mMutex.unlock();
93+
mAuthMutex.unlock();
10094
}
10195

10296

‎src/core/qgscredentials.h

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@
3535
3636
* QGIS application uses QgsCredentialDialog class for displaying a dialog to the user.
3737
38+
* Caller can use the mutex to synchronize authentications to avoid requesting
39+
* credentials for the same resource several times.
40+
3841
* Object deletes itself when it's not needed anymore. Children should use
3942
* signal destroyed() to be notified of the deletion
4043
*/
@@ -84,25 +87,21 @@ class CORE_EXPORT QgsCredentials
8487
* Lock the instance against access from multiple threads. This does not really lock access to get/put methds,
8588
* it will just prevent other threads to lock the instance and continue the execution. When the class is used
8689
* from non-GUI threads, they should call lock() before the get/put calls to avoid race conditions.
87-
*
88-
* \deprecated since QGIS 3.4 - mutex locking is automatically handled
90+
* \since QGIS 2.4
8991
*/
90-
Q_DECL_DEPRECATED void lock() SIP_DEPRECATED;
92+
void lock();
9193

9294
/**
9395
* Unlock the instance after being locked.
94-
* \deprecated since QGIS 3.4 - mutex locking is automatically handled
96+
* \since QGIS 2.4
9597
*/
96-
Q_DECL_DEPRECATED void unlock() SIP_DEPRECATED;
98+
void unlock();
9799

98100
/**
99101
* Returns pointer to mutex
100-
* \deprecated since QGIS 3.4 - mutex locking is automatically handled
102+
* \since QGIS 2.4
101103
*/
102-
Q_DECL_DEPRECATED QMutex *mutex() SIP_DEPRECATED
103-
{
104-
return &mMutex;
105-
}
104+
QMutex *mutex() { return &mAuthMutex; }
106105

107106
protected:
108107

@@ -133,7 +132,11 @@ class CORE_EXPORT QgsCredentials
133132
//! Pointer to the credential instance
134133
static QgsCredentials *sInstance;
135134

136-
QMutex mMutex;
135+
//! Mutex to synchronize authentications
136+
QMutex mAuthMutex;
137+
138+
//! Mutex to guard the cache
139+
QMutex mCacheMutex;
137140
};
138141

139142

‎src/gui/qgscredentialdialog.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ bool QgsCredentialDialog::request( const QString &realm, QString &username, QStr
5959
{
6060
QgsDebugMsg( QStringLiteral( "emitting signal" ) );
6161
emit credentialsRequested( realm, &username, &password, message, &ok );
62-
QgsDebugMsg( QStringLiteral( "signal returned %1 (username=%2, password=%3)" ).arg( ok ? "true" : "false", username, password ) );
62+
QgsDebugMsg( QStringLiteral( "signal returned %1 (username=%2)" ).arg( ok ? "true" : "false", username ) );
6363
}
6464
else
6565
{
@@ -81,7 +81,9 @@ void QgsCredentialDialog::requestCredentials( const QString &realm, QString *use
8181
labelMessage->setText( message );
8282
labelMessage->setHidden( message.isEmpty() );
8383

84-
if ( !leUsername->text().isEmpty() )
84+
if ( leUsername->text().isEmpty() )
85+
leUsername->setFocus();
86+
else
8587
lePassword->setFocus();
8688

8789
QWidget *activeWindow = qApp->activeWindow();

‎src/providers/db2/qgsdb2provider.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
201201
db.setPort( port.toInt() );
202202
bool connected = false;
203203
int i = 0;
204+
QgsCredentials::instance()->lock();
204205
while ( !connected && i < 3 )
205206
{
206207
i++;
@@ -214,6 +215,7 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
214215
{
215216
errMsg = QStringLiteral( "Cancel clicked" );
216217
QgsDebugMsg( errMsg );
218+
QgsCredentials::instance()->unlock();
217219
break;
218220
}
219221
}
@@ -255,6 +257,7 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
255257
{
256258
QgsCredentials::instance()->put( databaseName, userName, password );
257259
}
260+
QgsCredentials::instance()->unlock();
258261

259262
return db;
260263
}

‎src/providers/grass/qgsgrass.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -700,7 +700,7 @@ void QgsGrass::loadMapsetSearchPath()
700700

701701
void QgsGrass::setMapsetSearchPathWatcher()
702702
{
703-
QgsDebugMsg( "etered" );
703+
QgsDebugMsg( "entered." );
704704
if ( mMapsetSearchPathWatcher )
705705
{
706706
delete mMapsetSearchPathWatcher;

‎src/providers/oracle/qgsoracleconn.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,8 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri )
9696
QgsDebugMsg( QStringLiteral( "Connecting with options: " ) + options );
9797
if ( !mDatabase.open() )
9898
{
99+
QgsCredentials::instance()->lock();
100+
99101
while ( !mDatabase.open() )
100102
{
101103
bool ok = QgsCredentials::instance()->get( realm, username, password, mDatabase.lastError().text() );
@@ -125,6 +127,8 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri )
125127

126128
if ( mDatabase.isOpen() )
127129
QgsCredentials::instance()->put( realm, username, password );
130+
131+
QgsCredentials::instance()->unlock();
128132
}
129133

130134
if ( !mDatabase.isOpen() )

‎src/providers/postgres/qgspostgresconn.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,8 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s
272272
QString username = uri.username();
273273
QString password = uri.password();
274274

275+
QgsCredentials::instance()->lock();
276+
275277
int i = 0;
276278
while ( PQstatus() != CONNECTION_OK && i < 5 )
277279
{
@@ -296,6 +298,8 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s
296298

297299
if ( PQstatus() == CONNECTION_OK )
298300
QgsCredentials::instance()->put( conninfo, username, password );
301+
302+
QgsCredentials::instance()->unlock();
299303
}
300304

301305
if ( PQstatus() != CONNECTION_OK )

0 commit comments

Comments
 (0)
Please sign in to comment.