From 5937127b73a82fc86f6546397373ce9dbaf4e560 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 19 Nov 2019 17:57:41 +0900 Subject: BaseJob: Don't send accessToken if not needed; send again on 401 The first part closes #358; the second part is a workaround for non-standard cases when endpoints without security by the spec turn out to be secured (in particular, the case of authenticating media servers). --- lib/connection.cpp | 9 ++++--- lib/connectiondata.cpp | 12 +++++++++ lib/connectiondata.h | 2 ++ lib/jobs/basejob.cpp | 66 ++++++++++++++++++++++++++++++-------------------- lib/jobs/basejob.h | 8 +++++- 5 files changed, 66 insertions(+), 31 deletions(-) (limited to 'lib') diff --git a/lib/connection.cpp b/lib/connection.cpp index 5ee7e84e..c830557c 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -343,7 +343,8 @@ void Connection::logout() } const auto* job = callApi(); connect(job, &LogoutJob::finished, this, [this, job, syncWasRunning] { - if (job->status().good() || job->error() == BaseJob::ContentAccessError) { + if (job->status().good() || job->error() == BaseJob::Unauthorised + || job->error() == BaseJob::ContentAccessError) { if (d->syncLoopConnection) disconnect(d->syncLoopConnection); d->data->setToken({}); @@ -380,9 +381,9 @@ void Connection::sync(int timeout) }); connect(job, &SyncJob::failure, this, [this, job] { d->syncJob = nullptr; - if (job->error() == BaseJob::ContentAccessError) { - qCWarning(SYNCJOB) << "Sync job failed with ContentAccessError - " - "login expired?"; + if (job->error() == BaseJob::Unauthorised) { + qCWarning(SYNCJOB) + << "Sync job failed with Unauthorised - login expired?"; emit loginError(job->errorString(), job->rawDataSample()); } else emit syncError(job->errorString(), job->rawDataSample()); diff --git a/lib/connectiondata.cpp b/lib/connectiondata.cpp index cbc36420..e2a0f06f 100644 --- a/lib/connectiondata.cpp +++ b/lib/connectiondata.cpp @@ -42,6 +42,7 @@ public: QString lastEvent; QString userId; QString deviceId; + std::vector needToken; mutable unsigned int txnCounter = 0; const qint64 txnBase = QDateTime::currentMSecsSinceEpoch(); @@ -143,6 +144,12 @@ const QString& ConnectionData::deviceId() const { return d->deviceId; } const QString& ConnectionData::userId() const { return d->userId; } +bool ConnectionData::needsToken(const QString& requestName) const +{ + return std::find(d->needToken.cbegin(), d->needToken.cend(), requestName) + != d->needToken.cend(); +} + void ConnectionData::setDeviceId(const QString& deviceId) { d->deviceId = deviceId; @@ -150,6 +157,11 @@ void ConnectionData::setDeviceId(const QString& deviceId) void ConnectionData::setUserId(const QString& userId) { d->userId = userId; } +void ConnectionData::setNeedsToken(const QString& requestName) +{ + d->needToken.push_back(requestName); +} + QString ConnectionData::lastEvent() const { return d->lastEvent; } void ConnectionData::setLastEvent(QString identifier) diff --git a/lib/connectiondata.h b/lib/connectiondata.h index b367c977..000099d1 100644 --- a/lib/connectiondata.h +++ b/lib/connectiondata.h @@ -40,6 +40,7 @@ public: QUrl baseUrl() const; const QString& deviceId() const; const QString& userId() const; + bool needsToken(const QString& requestName) const; QNetworkAccessManager* nam() const; void setBaseUrl(QUrl baseUrl); @@ -50,6 +51,7 @@ public: void setPort(int port); void setDeviceId(const QString& deviceId); void setUserId(const QString& userId); + void setNeedsToken(const QString& requestName); QString lastEvent() const; void setLastEvent(QString identifier); diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 13e65188..7f558685 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -226,8 +226,9 @@ void BaseJob::Private::sendRequest() requestQuery) }; if (!requestHeaders.contains("Content-Type")) req.setHeader(QNetworkRequest::ContentTypeHeader, "application/json"); - req.setRawHeader("Authorization", - QByteArray("Bearer ") + connection->accessToken()); + if (needsToken) + req.setRawHeader("Authorization", + QByteArray("Bearer ") + connection->accessToken()); req.setAttribute(QNetworkRequest::BackgroundRequestAttribute, inBackground); req.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); req.setMaximumRedirectsAllowed(10); @@ -274,6 +275,7 @@ void BaseJob::sendRequest() return; Q_ASSERT(d->connection && status().code == Pending); qCDebug(d->logCat).noquote() << "Making" << d->dumpRequest(); + d->needsToken |= d->connection->needsToken(objectName()); emit aboutToSendRequest(); d->sendRequest(); Q_ASSERT(d->reply); @@ -341,32 +343,32 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) return false; } -BaseJob::Status BaseJob::Status::fromHttpCode(int httpCode, QString msg) +BaseJob::StatusCode BaseJob::Status::fromHttpCode(int httpCode) { + if (httpCode / 10 == 41) // 41x errors + return httpCode == 410 ? IncorrectRequestError : NotFoundError; + switch (httpCode) { + case 401: + return Unauthorised; // clang-format off - return { [httpCode]() -> StatusCode { - if (httpCode / 10 == 41) // 41x errors - return httpCode == 410 ? IncorrectRequestError : NotFoundError; - switch (httpCode) { - case 401: case 403: case 407: - return ContentAccessError; - case 404: - return NotFoundError; - case 400: case 405: case 406: case 426: case 428: case 505: - case 494: // Unofficial nginx "Request header too large" - case 497: // Unofficial nginx "HTTP request sent to HTTPS port" - return IncorrectRequestError; - case 429: - return TooManyRequestsError; - case 501: case 510: - return RequestNotImplementedError; - case 511: - return NetworkAuthRequiredError; - default: - return NetworkError; - } - }(), std::move(msg) }; - // clang-format on + case 403: case 407: // clang-format on + return ContentAccessError; + case 404: + return NotFoundError; + // clang-format off + case 400: case 405: case 406: case 426: case 428: case 505: // clang-format on + case 494: // Unofficial nginx "Request header too large" + case 497: // Unofficial nginx "HTTP request sent to HTTPS port" + return IncorrectRequestError; + case 429: + return TooManyRequestsError; + case 501: case 510: + return RequestNotImplementedError; + case 511: + return NetworkAuthRequiredError; + default: + return NetworkError; + } } QDebug BaseJob::Status::dumpToLog(QDebug dbg) const @@ -496,6 +498,16 @@ void BaseJob::finishJob() d->connection->submit(this); return; } + if (error() == Unauthorised && !d->needsToken + && !d->connection->accessToken().isEmpty()) { + // Rerun with access token (extension of the spec while + // https://github.com/matrix-org/matrix-doc/issues/701 is pending) + d->connection->setNeedsToken(objectName()); + qCWarning(d->logCat) << this << "re-running with authentication"; + emit retryScheduled(d->retriesTaken, 0); + setStatus(Pending); + sendRequest(); + } if ((error() == NetworkError || error() == Timeout) && d->retriesTaken < d->maxRetries) { // TODO: The whole retrying thing should be put to Connection(Manager) @@ -596,6 +608,8 @@ QString BaseJob::statusCaption() const return tr("Network problems"); case TimeoutError: return tr("Request timed out"); + case Unauthorised: + return tr("Unauthorised request"); case ContentAccessError: return tr("Access error"); case NotFoundError: diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index c4da40f5..ecbec74a 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -60,6 +60,7 @@ public: NetworkError = 100, Timeout, TimeoutError = Timeout, + Unauthorised, ContentAccessError, NotFoundError, IncorrectRequest, @@ -113,7 +114,12 @@ public: struct Status { Status(StatusCode c) : code(c) {} Status(int c, QString m) : code(c), message(std::move(m)) {} - static Status fromHttpCode(int httpCode, QString msg = {}); + + static StatusCode fromHttpCode(int httpCode); + static Status fromHttpCode(int httpCode, QString msg) + { + return { fromHttpCode(httpCode), std::move(msg) }; + } bool good() const { return code < ErrorLevel; } QDebug dumpToLog(QDebug dbg) const; -- cgit v1.2.3