From a9accb7d85ebce008575de89f078a6ea7663ff48 Mon Sep 17 00:00:00 2001 From: Jonas Kvinge Date: Thu, 14 Nov 2019 21:07:30 +0100 Subject: [PATCH] Refactor scrobbler authentication code Fix a crash when authentication is cancelled --- src/internet/localredirectserver.cpp | 4 +- src/scrobbler/listenbrainzscrobbler.cpp | 61 +++++++++++++------- src/scrobbler/listenbrainzscrobbler.h | 12 ++-- src/scrobbler/scrobblerservice.h | 2 +- src/scrobbler/scrobblingapi20.cpp | 76 +++++++++++++++---------- src/scrobbler/scrobblingapi20.h | 14 +++-- 6 files changed, 104 insertions(+), 65 deletions(-) diff --git a/src/internet/localredirectserver.cpp b/src/internet/localredirectserver.cpp index 7ed63cf60..367aefa0f 100644 --- a/src/internet/localredirectserver.cpp +++ b/src/internet/localredirectserver.cpp @@ -52,7 +52,9 @@ LocalRedirectServer::LocalRedirectServer(const bool https, QObject *parent) socket_(nullptr) {} -LocalRedirectServer::~LocalRedirectServer() {} +LocalRedirectServer::~LocalRedirectServer() { + if (isListening()) close(); +} bool LocalRedirectServer::GenerateCertificate() { diff --git a/src/scrobbler/listenbrainzscrobbler.cpp b/src/scrobbler/listenbrainzscrobbler.cpp index 378880ee9..bf49c82f4 100644 --- a/src/scrobbler/listenbrainzscrobbler.cpp +++ b/src/scrobbler/listenbrainzscrobbler.cpp @@ -71,9 +71,11 @@ ListenBrainzScrobbler::ListenBrainzScrobbler(Application *app, QObject *parent) app_(app), network_(new NetworkAccessManager(this)), cache_(new ScrobblerCache(kCacheFile, this)), + server_(nullptr), enabled_(false), expires_in_(-1), - submitted_(false) { + submitted_(false), + timestamp_(0) { ReloadSettings(); LoadSession(); @@ -123,16 +125,19 @@ void ListenBrainzScrobbler::Logout() { void ListenBrainzScrobbler::Authenticate(const bool https) { - LocalRedirectServer *server = new LocalRedirectServer(https, this); - if (!server->Listen()) { - AuthError(server->error()); - delete server; - return; + if (!server_) { + server_ = new LocalRedirectServer(https, this); + if (!server_->Listen()) { + AuthError(server_->error()); + delete server_; + server_ = nullptr; + return; + } + connect(server_, SIGNAL(Finished()), this, SLOT(RedirectArrived())); } - NewClosure(server, SIGNAL(Finished()), this, &ListenBrainzScrobbler::RedirectArrived, server); QUrl redirect_url(kRedirectUrl); - redirect_url.setPort(server->url().port()); + redirect_url.setPort(server_->url().port()); QUrlQuery url_query; url_query.addQueryItem("response_type", "code"); @@ -151,25 +156,39 @@ void ListenBrainzScrobbler::Authenticate(const bool https) { } -void ListenBrainzScrobbler::RedirectArrived(LocalRedirectServer *server) { +void ListenBrainzScrobbler::RedirectArrived() { - server->deleteLater(); + if (!server_) return; - QUrl url = server->request_url(); - if (!QUrlQuery(url).queryItemValue("error").isEmpty()) { - AuthError(QUrlQuery(url).queryItemValue("error")); - return; + if (server_->error().isEmpty()) { + QUrl url = server_->request_url(); + if (url.isValid()) { + QUrlQuery url_query(url); + if (url_query.hasQueryItem("error")) { + AuthError(QUrlQuery(url).queryItemValue("error")); + } + else if (url_query.hasQueryItem("code")) { + RequestSession(url, url_query.queryItemValue("code")); + } + else { + AuthError(tr("Redirect missing token code!")); + } + } + else { + AuthError(tr("Received invalid reply from web browser.")); + } } - if (QUrlQuery(url).queryItemValue("code").isEmpty()) { - AuthError("Redirect missing token code!"); - return; + else { + AuthError(server_->error()); } - RequestSession(url, QUrlQuery(url).queryItemValue("code").toUtf8()); + server_->close(); + server_->deleteLater(); + server_ = nullptr; } -void ListenBrainzScrobbler::RequestSession(QUrl url, QString token) { +void ListenBrainzScrobbler::RequestSession(const QUrl &url, const QString &token) { QUrl session_url(kAuthTokenUrl); QUrlQuery url_query; @@ -525,11 +544,11 @@ void ListenBrainzScrobbler::ScrobbleRequestFinished(QNetworkReply *reply, QList< } -void ListenBrainzScrobbler::AuthError(QString error) { +void ListenBrainzScrobbler::AuthError(const QString &error) { emit AuthenticationComplete(false, error); } -void ListenBrainzScrobbler::Error(QString error, QVariant debug) { +void ListenBrainzScrobbler::Error(const QString &error, const QVariant &debug) { qLog(Error) << "ListenBrainz:" << error; if (debug.isValid()) qLog(Debug) << debug; diff --git a/src/scrobbler/listenbrainzscrobbler.h b/src/scrobbler/listenbrainzscrobbler.h index ee0246126..52c8df829 100644 --- a/src/scrobbler/listenbrainzscrobbler.h +++ b/src/scrobbler/listenbrainzscrobbler.h @@ -26,7 +26,6 @@ #include #include -#include #include #include #include @@ -37,6 +36,8 @@ #include "scrobblerservice.h" #include "scrobblercache.h" +class QNetworkReply; + class Application; class NetworkAccessManager; class LocalRedirectServer; @@ -75,7 +76,7 @@ class ListenBrainzScrobbler : public ScrobblerService { void WriteCache() { cache_->WriteCache(); } private slots: - void RedirectArrived(LocalRedirectServer *server); + void RedirectArrived(); void AuthenticateReplyFinished(QNetworkReply *reply); void UpdateNowPlayingRequestFinished(QNetworkReply *reply); void ScrobbleRequestFinished(QNetworkReply *reply, QList); @@ -84,9 +85,9 @@ class ListenBrainzScrobbler : public ScrobblerService { QNetworkReply *CreateRequest(const QUrl &url, const QJsonDocument &json_doc); QByteArray GetReplyData(QNetworkReply *reply); - void RequestSession(QUrl url, QString token); - void AuthError(QString error); - void Error(QString error, QVariant debug = QVariant()); + void RequestSession(const QUrl &url, const QString &token); + void AuthError(const QString &error); + void Error(const QString &error, const QVariant &debug = QVariant()); void DoSubmit(); static const char *kAuthUrl; @@ -101,6 +102,7 @@ class ListenBrainzScrobbler : public ScrobblerService { Application *app_; NetworkAccessManager *network_; ScrobblerCache *cache_; + LocalRedirectServer *server_; bool enabled_; QString user_token_; QString access_token_; diff --git a/src/scrobbler/scrobblerservice.h b/src/scrobbler/scrobblerservice.h index 838600859..7902c196b 100644 --- a/src/scrobbler/scrobblerservice.h +++ b/src/scrobbler/scrobblerservice.h @@ -52,7 +52,7 @@ class ScrobblerService : public QObject { virtual void ClearPlaying() = 0; virtual void Scrobble(const Song &song) = 0; virtual void Love() {} - virtual void Error(QString error, QVariant debug = QVariant()) = 0; + virtual void Error(const QString &error, const QVariant &debug = QVariant()) = 0; virtual void DoSubmit() = 0; virtual void Submitted() = 0; diff --git a/src/scrobbler/scrobblingapi20.cpp b/src/scrobbler/scrobblingapi20.cpp index 9112aa5eb..20e10d66b 100644 --- a/src/scrobbler/scrobblingapi20.cpp +++ b/src/scrobbler/scrobblingapi20.cpp @@ -72,9 +72,11 @@ ScrobblingAPI20::ScrobblingAPI20(const QString &name, const QString &settings_gr api_url_(api_url), batch_(batch), app_(app), + server_(nullptr), enabled_(false), subscriber_(false), - submitted_(false) {} + submitted_(false), + timestamp_(0) {} ScrobblingAPI20::~ScrobblingAPI20() {} @@ -121,16 +123,19 @@ void ScrobblingAPI20::Logout() { void ScrobblingAPI20::Authenticate(const bool https) { - LocalRedirectServer *server = new LocalRedirectServer(https, this); - if (!server->Listen()) { - AuthError(server->error()); - delete server; - return; + if (!server_) { + server_ = new LocalRedirectServer(https, this); + if (!server_->Listen()) { + AuthError(server_->error()); + delete server_; + server_ = nullptr; + return; + } + connect(server_, SIGNAL(Finished()), this, SLOT(RedirectArrived())); } - NewClosure(server, SIGNAL(Finished()), this, &ScrobblingAPI20::RedirectArrived, server); QUrlQuery redirect_url_query; - const QString port = QString::number(server->url().port()); + const QString port = QString::number(server_->url().port()); redirect_url_query.addQueryItem("port", port); if (https) redirect_url_query.addQueryItem("https", QString("1")); QUrl redirect_url(kRedirectUrl); @@ -160,8 +165,11 @@ void ScrobblingAPI20::Authenticate(const bool https) { QApplication::clipboard()->setText(url.toString()); break; case QMessageBox::Cancel: - server->close(); - server->deleteLater(); + if (server_) { + server_->close(); + server_->deleteLater(); + server_ = nullptr; + } emit AuthenticationComplete(false); break; default: @@ -170,31 +178,37 @@ void ScrobblingAPI20::Authenticate(const bool https) { } -void ScrobblingAPI20::RedirectArrived(LocalRedirectServer *server) { +void ScrobblingAPI20::RedirectArrived() { - server->deleteLater(); + if (!server_) return; - if (!server->error().isEmpty()) { - AuthError(server->error()); - return; + if (server_->error().isEmpty()) { + QUrl url = server_->request_url(); + if (url.isValid()) { + QUrlQuery url_query(url); + if (url_query.hasQueryItem("token")) { + QString token = url_query.queryItemValue("token").toUtf8(); + RequestSession(token); + } + else { + AuthError(tr("Invalid reply from web browser. Missing token.")); + } + } + else { + AuthError(tr("Received invalid reply from web browser. Try the HTTPS option, or use another browser like Chromium or Chrome.")); + } + } + else { + AuthError(server_->error()); } - QUrl url = server->request_url(); - if (!url.isValid()) { - AuthError(tr("Received invalid reply from web browser. Try the HTTPS option, or use another browser like Chromium or Chrome.")); - return; - } - QUrlQuery url_query(url); - QString token = url_query.queryItemValue("token").toUtf8(); - if (token.isEmpty()) { - AuthError(tr("Invalid reply from web browser. Missing token.")); - return; - } - RequestSession(token); + server_->close(); + server_->deleteLater(); + server_ = nullptr; } -void ScrobblingAPI20::RequestSession(QString token) { +void ScrobblingAPI20::RequestSession(const QString &token) { QUrl session_url(api_url_); QUrlQuery session_url_query; @@ -906,18 +920,18 @@ void ScrobblingAPI20::LoveRequestFinished(QNetworkReply *reply) { } -void ScrobblingAPI20::AuthError(QString error) { +void ScrobblingAPI20::AuthError(const QString &error) { emit AuthenticationComplete(false, error); } -void ScrobblingAPI20::Error(QString error, QVariant debug) { +void ScrobblingAPI20::Error(const QString &error, const QVariant &debug) { qLog(Error) << name_ << error; if (debug.isValid()) qLog(Debug) << debug; } -QString ScrobblingAPI20::ErrorString(ScrobbleErrorCode error) const { +QString ScrobblingAPI20::ErrorString(const ScrobbleErrorCode error) const { switch (error) { case ScrobbleErrorCode::InvalidService: diff --git a/src/scrobbler/scrobblingapi20.h b/src/scrobbler/scrobblingapi20.h index d995a4f33..4508e1602 100644 --- a/src/scrobbler/scrobblingapi20.h +++ b/src/scrobbler/scrobblingapi20.h @@ -26,7 +26,6 @@ #include #include -#include #include #include #include @@ -36,6 +35,8 @@ #include "scrobblerservice.h" #include "scrobblercache.h" +class QNetworkReply; + class Application; class NetworkAccessManager; class LocalRedirectServer; @@ -81,7 +82,7 @@ class ScrobblingAPI20 : public ScrobblerService { void WriteCache() { cache()->WriteCache(); } private slots: - void RedirectArrived(LocalRedirectServer *server); + void RedirectArrived(); void AuthenticateReplyFinished(QNetworkReply *reply); void UpdateNowPlayingRequestFinished(QNetworkReply *reply); void ScrobbleRequestFinished(QNetworkReply *reply, QList); @@ -130,11 +131,11 @@ class ScrobblingAPI20 : public ScrobblerService { QNetworkReply *CreateRequest(const ParamList &request_params); QByteArray GetReplyData(QNetworkReply *reply); - void RequestSession(QString token); - void AuthError(QString error); + void RequestSession(const QString &token); + void AuthError(const QString &error); void SendSingleScrobble(ScrobblerCacheItem *item); - void Error(QString error, QVariant debug = QVariant()); - QString ErrorString(ScrobbleErrorCode error) const; + void Error(const QString &error, const QVariant &debug = QVariant()); + QString ErrorString(const ScrobbleErrorCode error) const; void DoSubmit(); QString name_; @@ -144,6 +145,7 @@ class ScrobblingAPI20 : public ScrobblerService { bool batch_; Application *app_; + LocalRedirectServer *server_; bool enabled_; bool https_;