Brush up SsoSession; document Connection::prepareForSso

Although parented to Connection, SsoSession was pretty leaky in that
unsuccessful login attempts didn't delete the object and in some errors
didn't even close the local HTTP socket (though new connections would no
more be accepted). Also, without the documentation it wasn't clear who
owns the object returned by Connection::prepareForSso(). Now it is.

Unfortunately, it's not easy to cover SsoSession with tests. Basically,
it takes a homeserver and a mock "SSO agent" that would check
the SSO URL for validity and then both send the login authorisation
to the homeserver as well as ping the callback given by SsoSession.
Maybe for another time.

3 files changed, 34 insertions, 25 deletions

diff --git a/lib/connection.h b/lib/connection.h
index b4476347..28688cc1 100644
--- a/lib/connection.h
+++ b/lib/connection.h
@@ -443,6 +443,17 @@ public:
                                     std::forward<JobArgTs>(jobArgs)...);
     }
 
+    //! \brief Start a local HTTP server and generate a single sign-on URL
+    //!
+    //! This call does the preparatory steps to carry out single sign-on
+    //! sequence
+    //! \sa https://matrix.org/docs/guides/sso-for-client-developers
+    //! \return A proxy object holding two URLs: one for SSO on the chosen
+    //!         homeserver and another for the local callback address. Normally
+    //!         you won't need the callback URL unless you proxy the response
+    //!         with a custom UI. You do not need to delete the SsoSession
+    //!         object; the Connection that issued it will dispose of it once
+    //!         the login sequence completes (with any outcome).
     Q_INVOKABLE SsoSession* prepareForSso(const QString& initialDeviceName,
                                           const QString& deviceId = {});
 
diff --git a/lib/ssosession.cpp b/lib/ssosession.cpp
index 5f3479b8..93e252cc 100644
--- a/lib/ssosession.cpp
+++ b/lib/ssosession.cpp
@@ -15,10 +15,10 @@ using namespace Quotient;
 
 class SsoSession::Private {
 public:
-    Private(SsoSession* q, const QString& initialDeviceName = {},
-            const QString& deviceId = {}, Connection* connection = nullptr)
-        : initialDeviceName(initialDeviceName)
-        , deviceId(deviceId)
+    Private(SsoSession* q, QString initialDeviceName = {},
+            QString deviceId = {}, Connection* connection = nullptr)
+        : initialDeviceName(std::move(initialDeviceName))
+        , deviceId(std::move(deviceId))
         , connection(connection)
     {
         auto* server = new QTcpServer(q);
@@ -29,7 +29,7 @@ public:
                           .arg(server->serverPort());
         ssoUrl = connection->getUrlForApi<RedirectToSSOJob>(callbackUrl);
 
-        QObject::connect(server, &QTcpServer::newConnection, q, [this, server] {
+        QObject::connect(server, &QTcpServer::newConnection, q, [this, q, server] {
             qCDebug(MAIN) << "SSO callback initiated";
             socket = server->nextPendingConnection();
             server->close();
@@ -43,8 +43,14 @@ public:
             });
             QObject::connect(socket, &QTcpSocket::disconnected, socket,
                              &QTcpSocket::deleteLater);
+            QObject::connect(socket, &QObject::destroyed, q,
+                             &QObject::deleteLater);
         });
+        qCDebug(MAIN) << "SSO session constructed";
     }
+    ~Private() { qCDebug(MAIN) << "SSO session deconstructed"; }
+    Q_DISABLE_COPY_MOVE(Private)
+
     void processCallback();
     void sendHttpResponse(const QByteArray& code, const QByteArray& msg);
     void onError(const QByteArray& code, const QString& errorMsg);
@@ -62,14 +68,7 @@ SsoSession::SsoSession(Connection* connection, const QString& initialDeviceName,
                        const QString& deviceId)
     : QObject(connection)
     , d(makeImpl<Private>(this, initialDeviceName, deviceId, connection))
-{
-    qCDebug(MAIN) << "SSO session constructed";
-}
-
-SsoSession::~SsoSession()
-{
-    qCDebug(MAIN) << "SSO session deconstructed";
-}
+{}
 
 QUrl SsoSession::ssoUrl() const { return d->ssoUrl; }
 
@@ -82,29 +81,29 @@ void SsoSession::Private::processCallback()
     // (see at https://github.com/clementine-player/Clementine/)
     const auto& requestParts = requestData.split(' ');
     if (requestParts.size() < 2 || requestParts[1].isEmpty()) {
-        onError("400 Bad Request", tr("No login token in SSO callback"));
+        onError("400 Bad Request", tr("Malformed single sign-on callback"));
         return;
     }
     const auto& QueryItemName = QStringLiteral("loginToken");
     QUrlQuery query { QUrl(requestParts[1]).query() };
     if (!query.hasQueryItem(QueryItemName)) {
-        onError("400 Bad Request", tr("Malformed single sign-on callback"));
+        onError("400 Bad Request", tr("No login token in SSO callback"));
+        return;
     }
     qCDebug(MAIN) << "Found the token in SSO callback, logging in";
     connection->loginWithToken(query.queryItemValue(QueryItemName).toLatin1(),
                                initialDeviceName, deviceId);
     connect(connection, &Connection::connected, socket, [this] {
-        const QString msg =
-            "The application '" % QCoreApplication::applicationName()
-            % "' has successfully logged in as a user " % connection->userId()
-            % " with device id " % connection->deviceId()
-            % ". This window can be closed. Thank you.\r\n";
+        const auto msg =
+            tr("The application '%1' has successfully logged in as a user %2 "
+               "with device id %3. This window can be closed. Thank you.\r\n")
+                .arg(QCoreApplication::applicationName(), connection->userId(),
+                     connection->deviceId());
         sendHttpResponse("200 OK", msg.toHtmlEscaped().toUtf8());
         socket->disconnectFromHost();
     });
     connect(connection, &Connection::loginError, socket, [this] {
         onError("401 Unauthorised", tr("Login failed"));
-        socket->disconnectFromHost();
     });
 }
 
@@ -128,4 +127,5 @@ void SsoSession::Private::onError(const QByteArray& code,
     // [kitsune] Yeah, I know, dirty. Maybe the "right" way would be to have
     // an intermediate signal but that seems just a fight for purity.
     emit connection->loginError(errorMsg, requestData);
+    socket->disconnectFromHost();
 }
diff --git a/lib/ssosession.h b/lib/ssosession.h
index 0f3fc3b8..e6a3f8fb 100644
--- a/lib/ssosession.h
+++ b/lib/ssosession.h
@@ -8,9 +8,6 @@
 #include <QtCore/QUrl>
 #include <QtCore/QObject>
 
-class QTcpServer;
-class QTcpSocket;
-
 namespace Quotient {
 class Connection;
 
@@ -36,7 +33,8 @@ class QUOTIENT_API SsoSession : public QObject {
 public:
     SsoSession(Connection* connection, const QString& initialDeviceName,
                const QString& deviceId = {});
-    ~SsoSession() override;
+    ~SsoSession() override = default;
+
     QUrl ssoUrl() const;
     QUrl callbackUrl() const;