Skip to content

Commit

Permalink
OAuth2: avoid infinite loop when refreshing token from non-main thread
Browse files Browse the repository at this point in the history
If a worker thread (such as a repaint one) causes a refresh of the token
(and if the main thread also tries to call QgsAuthOAuth2Method::updateNetworkRequest(),
but I'm not sure if this is needed), then the network events fail to be
delivered to the temporary event loop. To fix that, use QgsBlockingNetworkRequest

Stack when infinite loop occurs:

```
Thread 7 (Thread 0x7f430bce7700 (LWP 1572)):
0  0x00007f4356c0780d in poll () at ../sysdeps/unix/syscall-template.S:84
1  0x00007f434d89a38c in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
2  0x00007f434d89a49c in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
3  0x00007f43577b429f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /opt/qt59/lib/libQt5Core.so.5
4  0x00007f435775d13a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /opt/qt59/lib/libQt5Core.so.5
5  0x00007f430f3144b0 in QgsAuthOAuth2Method::updateNetworkRequest (this=0x229ddf0, request=..., authcfg=..., dataprovider=...) at /home/even/qgis/QGIS/src/auth/oauth2/qgsauthoauth2method.cpp:179
6  0x00007f4359132590 in QgsAuthManager::updateNetworkRequest (this=0x2287fd0, request=..., authcfg=..., dataprovider=...) at /home/even/qgis/QGIS/src/core/auth/qgsauthmanager.cpp:1473
7  0x00007f435948e200 in QgsBlockingNetworkRequest::doRequest (this=0x7f430bcdfb20, method=QgsBlockingNetworkRequest::Get, request=..., forceRefresh=true, feedback=0x0)
    at /home/even/qgis/QGIS/src/core/qgsblockingnetworkrequest.cpp:125
8  0x00007f435948d6c5 in QgsBlockingNetworkRequest::get (this=0x7f430bcdfb20, request=..., forceRefresh=true, feedback=0x0) at /home/even/qgis/QGIS/src/core/qgsblockingnetworkrequest.cpp:59
9  0x00007f435955a259 in QgsCPLHTTPFetchOverrider::callback (pszURL=0x7f42fc03ab40 "http://localhost:9200/poly/_mapping?pretty", papszOptions=0x0, pfnWrite=0x0, pWriteArg=0x0, pUserData=0x7f430bce06b0)
    at /home/even/qgis/QGIS/src/core/qgscplhttpfetchoverrider.cpp:139
[...]
16 0x00007f4354f142f8 in GDALDatasetGetLayerByName (hDS=0x7f42fc0404a0, pszName=0x7f42fc03aa78 "poly") at gdaldataset.cpp:4379
17 0x00007f43593dbff1 in QgsOgrFeatureIterator::QgsOgrFeatureIterator (this=0x7f42fc039f10, source=0x83ab160, ownSource=false, request=..., transaction=0x0)
    at /home/even/qgis/QGIS/src/core/providers/ogr/qgsogrfeatureiterator.cpp:96
18 0x00007f43593df539 in QgsOgrFeatureSource::getFeatures (this=0x83ab160, request=...) at /home/even/qgis/QGIS/src/core/providers/ogr/qgsogrfeatureiterator.cpp:615
19 0x00007f4359926503 in QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator (this=0x7f42fc0383d0, source=0x820ed60, ownSource=false, request=...)
    at /home/even/qgis/QGIS/src/core/qgsvectorlayerfeatureiterator.cpp:300
20 0x00007f4359925237 in QgsVectorLayerFeatureSource::getFeatures (this=0x820ed60, request=...) at /home/even/qgis/QGIS/src/core/qgsvectorlayerfeatureiterator.cpp:99
21 0x00007f435993ed73 in QgsVectorLayerRenderer::render (this=0x83aaf40) at /home/even/qgis/QGIS/src/core/qgsvectorlayerrenderer.cpp:305
22 0x00007f435969231a in QgsMapRendererCustomPainterJob::doRender (this=0x82c7010) at /home/even/qgis/QGIS/src/core/qgsmaprenderercustompainterjob.cpp:317
23 0x00007f4359691d9a in QgsMapRendererCustomPainterJob::staticRender (self=0x82c7010) at /home/even/qgis/QGIS/src/core/qgsmaprenderercustompainterjob.cpp:267
24 0x00007f4359695551 in QtConcurrent::StoredFunctorCall1<void, void (*)(QgsMapRendererCustomPainterJob*), QgsMapRendererCustomPainterJob*>::runFunctor (this=0x82c2cf0)
    at /opt/qt59/include/QtConcurrent/qtconcurrentstoredfunctioncall.h:432
25 0x00007f4359692f69 in QtConcurrent::RunFunctionTask<void>::run (this=0x82c2cf0) at /opt/qt59/include/QtConcurrent/qtconcurrentrunbase.h:136
26 0x00007f435757b943 in ?? () from /opt/qt59/lib/libQt5Core.so.5
27 0x00007f435757f659 in ?? () from /opt/qt59/lib/libQt5Core.so.5
28 0x00007f434ed276ba in start_thread (arg=0x7f430bce7700) at pthread_create.c:333
29 0x00007f4356c134dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Thread 1 (Thread 0x7f4326e23640 (LWP 27535)):
0  syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
1  0x00007f43575767b5 in QBasicMutex::lockInternal() () from /opt/qt59/lib/libQt5Core.so.5
2  0x00007f4357576872 in QMutex::lock() () from /opt/qt59/lib/libQt5Core.so.5
3  0x0000000000418dc3 in QMutexLocker::QMutexLocker (this=0x7ffe17546870, m=0x229de28) at /opt/qt59/include/QtCore/qmutex.h:200
--Type <RET> for more, q to quit, c to continue without paging--
4  0x00007f430f313dd6 in QgsAuthOAuth2Method::updateNetworkRequest (this=0x229ddf0, request=..., authcfg=..., dataprovider=...) at /home/even/qgis/QGIS/src/auth/oauth2/qgsauthoauth2method.cpp:111
5  0x00007f4359132590 in QgsAuthManager::updateNetworkRequest (this=0x2287fd0, request=..., authcfg=..., dataprovider=...) at /home/even/qgis/QGIS/src/core/auth/qgsauthmanager.cpp:1473
6  0x00007f435948e200 in QgsBlockingNetworkRequest::doRequest (this=0x7ffe17546da0, method=QgsBlockingNetworkRequest::Get, request=..., forceRefresh=true, feedback=0x0)
    at /home/even/qgis/QGIS/src/core/qgsblockingnetworkrequest.cpp:125
7  0x00007f435948d6c5 in QgsBlockingNetworkRequest::get (this=0x7ffe17546da0, request=..., forceRefresh=true, feedback=0x0) at /home/even/qgis/QGIS/src/core/qgsblockingnetworkrequest.cpp:59
[...]
18 0x00007f4355536d8d in OGRElasticLayer::GetExtent (this=0x8198340, psExtent=0x820ea50, bForce=1) at ogr_elastic.h:190
19 0x00007f4355685f24 in OGR_L_GetExtent (hLayer=0x8198340, psExtent=0x820ea50, bForce=1) at ogrlayer.cpp:313
20 0x00007f43593b6264 in QgsOgrLayer::GetExtent (this=0x7fe63f0, psExtent=0x820ea50, bForce=true) at /home/even/qgis/QGIS/src/core/providers/ogr/qgsogrprovider.cpp:6302
21 0x00007f435938bce1 in QgsOgrProvider::extent (this=0x80dfa20) at /home/even/qgis/QGIS/src/core/providers/ogr/qgsogrprovider.cpp:1424
22 0x00007f43598cfe67 in QgsVectorLayer::extent (this=0x8191430) at /home/even/qgis/QGIS/src/core/qgsvectorlayer.cpp:851
23 0x00007f43596bbdc7 in QgsMapSettings::fullExtent (this=0x22997e8) at /home/even/qgis/QGIS/src/core/qgsmapsettings.cpp:610
24 0x00007f435bdd8a91 in QgsMapCanvas::fullExtent (this=0x22997b0) at /home/even/qgis/QGIS/src/gui/qgsmapcanvas.cpp:1063
25 0x00007f435be0a818 in QgsMapOverviewCanvas::updateFullExtent (this=0x338d8e0) at /home/even/qgis/QGIS/src/gui/qgsmapoverviewcanvas.cpp:256
26 0x00007f435be0a709 in QgsMapOverviewCanvas::setLayers (this=0x338d8e0, layers=...) at /home/even/qgis/QGIS/src/gui/qgsmapoverviewcanvas.cpp:245
[...]
128 0x0000000000416180 in main (argc=1, argv=0x7ffe1754c468) at /home/even/qgis/QGIS/src/app/main.cpp:1637
```

(cherry picked from commit b86cc93)
  • Loading branch information
rouault authored and nyalldawson committed Nov 20, 2020
1 parent 46067e5 commit 33d079e
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 24 deletions.
25 changes: 1 addition & 24 deletions src/auth/oauth2/qgsauthoauth2method.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -157,30 +157,7 @@ bool QgsAuthOAuth2Method::updateNetworkRequest( QNetworkRequest &request, const
QgsMessageLog::logMessage( msg, AUTH_METHOD_KEY, Qgis::MessageLevel::Info );

// Try to get a refresh token first
// go into local event loop and wait for a fired refresh-related slot
QEventLoop rloop( nullptr );
connect( o2, &QgsO2::refreshFinished, &rloop, &QEventLoop::quit );

// add single shot timer to quit refresh after an allotted timeout
// this should keep the local event loop from blocking forever
QTimer r_timer( nullptr );
int r_reqtimeout = o2->oauth2config()->requestTimeout() * 1000;
r_timer.setInterval( r_reqtimeout );
r_timer.setSingleShot( true );
connect( &r_timer, &QTimer::timeout, &rloop, &QEventLoop::quit );
r_timer.start();

// Asynchronously attempt the refresh
// TODO: This already has a timed reply setup in O2 base class (and in QgsNetworkAccessManager!)
// May need to address this or app crashes will occur!
o2->refresh();

// block request update until asynchronous linking loop is quit
rloop.exec();
if ( r_timer.isActive() )
{
r_timer.stop();
}
o2->refreshSynchronous();

// refresh result should set o2 to (un)linked
}
Expand Down
83 changes: 83 additions & 0 deletions src/auth/oauth2/qgso2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,11 @@
#include "qgsauthoauth2config.h"
#include "qgslogger.h"
#include "qgsnetworkaccessmanager.h"
#include "qgsblockingnetworkrequest.h"

#include <QDir>
#include <QJsonDocument>
#include <QJsonObject>
#include <QSettings>
#include <QUrl>
#include <QUrlQuery>
Expand Down Expand Up @@ -348,3 +351,83 @@ QNetworkAccessManager *QgsO2::getManager()
{
return QgsNetworkAccessManager::instance();
}

/// Parse JSON data into a QVariantMap
static QVariantMap parseTokenResponse( const QByteArray &data )
{
QJsonParseError err;
QJsonDocument doc = QJsonDocument::fromJson( data, &err );
if ( err.error != QJsonParseError::NoError )
{
qWarning() << "parseTokenResponse: Failed to parse token response due to err:" << err.errorString();
return QVariantMap();
}

if ( !doc.isObject() )
{
qWarning() << "parseTokenResponse: Token response is not an object";
return QVariantMap();
}

return doc.object().toVariantMap();
}

// Code adapted from O2::refresh(), but using QgsBlockingNetworkRequest
void QgsO2::refreshSynchronous()
{
qDebug() << "O2::refresh: Token: ..." << refreshToken().right( 7 );

if ( refreshToken().isEmpty() )
{
qWarning() << "O2::refresh: No refresh token";
onRefreshError( QNetworkReply::AuthenticationRequiredError );
return;
}
if ( refreshTokenUrl_.isEmpty() )
{
qWarning() << "O2::refresh: Refresh token URL not set";
onRefreshError( QNetworkReply::AuthenticationRequiredError );
return;
}

QNetworkRequest refreshRequest( refreshTokenUrl_ );
refreshRequest.setHeader( QNetworkRequest::ContentTypeHeader, O2_MIME_TYPE_XFORM );
QMap<QString, QString> parameters;
parameters.insert( O2_OAUTH2_CLIENT_ID, clientId_ );
parameters.insert( O2_OAUTH2_CLIENT_SECRET, clientSecret_ );
parameters.insert( O2_OAUTH2_REFRESH_TOKEN, refreshToken() );
parameters.insert( O2_OAUTH2_GRANT_TYPE, O2_OAUTH2_REFRESH_TOKEN );

QByteArray data = buildRequestBody( parameters );

QgsBlockingNetworkRequest blockingRequest;
QgsBlockingNetworkRequest::ErrorCode errCode = blockingRequest.post( refreshRequest, data, true );
if ( errCode == QgsBlockingNetworkRequest::NoError )
{
QByteArray reply = blockingRequest.reply().content();
QVariantMap tokens = parseTokenResponse( reply );
if ( tokens.contains( QStringLiteral( "error" ) ) )
{
qDebug() << " Error refreshing token" << tokens.value( QStringLiteral( "error" ) ).toMap().value( QStringLiteral( "message" ) ).toString().toLocal8Bit().constData();
unlink();
}
else
{
setToken( tokens.value( O2_OAUTH2_ACCESS_TOKEN ).toString() );
setExpires( QDateTime::currentMSecsSinceEpoch() / 1000 + tokens.value( O2_OAUTH2_EXPIRES_IN ).toInt() );
const QString refreshToken = tokens.value( O2_OAUTH2_REFRESH_TOKEN ).toString();
if ( !refreshToken.isEmpty() )
setRefreshToken( refreshToken );
setLinked( true );
qDebug() << " New token expires in" << expires() << "seconds";
emit linkingSucceeded();
}
emit refreshFinished( QNetworkReply::NoError );
}
else
{
unlink();
qDebug() << "O2::onRefreshFinished: Error" << blockingRequest.errorMessage();
emit refreshFinished( blockingRequest.reply().error() );
}
}
4 changes: 4 additions & 0 deletions src/auth/oauth2/qgso2.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ class QgsO2: public O2
//! Store oauth2 state to a random value when called
void setState( const QString &value );

//! Refresh token in a synchronous way
void refreshSynchronous();

public slots:

//! Clear all properties
Expand Down Expand Up @@ -101,6 +104,7 @@ class QgsO2: public O2
QString state_;
QgsAuthOAuth2Config *mOAuth2Config;
bool mIsLocalHost = false;

static QString O2_OAUTH2_STATE;
};

Expand Down

0 comments on commit 33d079e

Please sign in to comment.