Refactor scrobbler authentication code

Fix a crash when authentication is cancelled

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() {
 

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_) {
-  if (!server->Listen()) {
+    server_ = new LocalRedirectServer(https, this);
-    AuthError(server->error());
+    if (!server_->Listen()) {
-    delete server;
+      AuthError(server_->error());
-    return;
+      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 (server_->error().isEmpty()) {
-  if (!QUrlQuery(url).queryItemValue("error").isEmpty()) {
+    QUrl url = server_->request_url();
-    AuthError(QUrlQuery(url).queryItemValue("error"));
+    if (url.isValid()) {
-    return;
+      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()) {
+  else {
-    AuthError("Redirect missing token code!");
+    AuthError(server_->error());
-    return;
   }
 
-  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;

src/scrobbler/listenbrainzscrobbler.h

@@ -26,7 +26,6 @@
 
 #include <QtGlobal>
 #include <QObject>
-#include <QNetworkReply>
 #include <QList>
 #include <QVariant>
 #include <QByteArray>
@@ -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<quint64>);
@@ -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 RequestSession(const QUrl &url, const QString &token);
-  void AuthError(QString error);
+  void AuthError(const QString &error);
-  void Error(QString error, QVariant debug = QVariant());
+  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_;

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;

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_) {
-  if (!server->Listen()) {
+    server_ = new LocalRedirectServer(https, this);
-    AuthError(server->error());
+    if (!server_->Listen()) {
-    delete server;
+      AuthError(server_->error());
-    return;
+      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();
+    if (server_) {
-    server->deleteLater();
+      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()) {
+  if (server_->error().isEmpty()) {
-    AuthError(server->error());
+    QUrl url = server_->request_url();
-    return;
+    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();
+  server_->close();
-  if (!url.isValid()) {
+  server_->deleteLater();
-    AuthError(tr("Received invalid reply from web browser. Try the HTTPS option, or use another browser like Chromium or Chrome."));
+  server_ = nullptr;
-    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);
 
 }
 
-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:

src/scrobbler/scrobblingapi20.h

@@ -26,7 +26,6 @@
 
 #include <QtGlobal>
 #include <QObject>
-#include <QNetworkReply>
 #include <QList>
 #include <QVariant>
 #include <QByteArray>
@@ -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<quint64>);
@@ -130,11 +131,11 @@ class ScrobblingAPI20 : public ScrobblerService {
   QNetworkReply *CreateRequest(const ParamList &request_params);
   QByteArray GetReplyData(QNetworkReply *reply);
 
-  void RequestSession(QString token);
+  void RequestSession(const QString &token);
-  void AuthError(QString error);
+  void AuthError(const QString &error);
   void SendSingleScrobble(ScrobblerCacheItem *item);
-  void Error(QString error, QVariant debug = QVariant());
+  void Error(const QString &error, const QVariant &debug = QVariant());
-  QString ErrorString(ScrobbleErrorCode error) const;
+  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_;