D9885: Retrieve OAuth2 token with HTTP socket

[Daniel Vrátil]

dvratil requested changes to this revision.
dvratil added a comment.
This revision now requires changes to proceed.


  Generally looks good. I tested it locally and it works nicely. Thanks a lot for the patch! The new user-facing strings are a bit unfortunate since that means we can't land this in Applications/17.12 branch, but I'd like to get this to users before 18.04. Probably we will have to ship the `QStringLiteral` version to stable branch and make it only translatable in master.

INLINE COMMENTS

> authwidget.cpp:112
> +        d->connection->setParent(this);
> +        connect(d->connection, &QTcpSocket::readyRead, d, &AuthWidgetPrivate::socketReady);
> +        d->server->close();

You may want to handle the `QTcpSocket::error()` signal in case there's some problem, in which case `readyRead` would not be emitted, so that the OAuth flow does not get stuck but shows an error instead.

> authwidget.cpp:114
> +        d->server->close();
> +        delete d->server;
> +    });

You may want to use `deleteLater()` here instead of `delete` - it's not ideal to delete the emitter from its signal handler, since when code execution returns back to the emitter, it might try to access its `this` pointer which would be deleted at this point.

> davidk wrote in authwidget_p.cpp:259
> I don't think it will crash. If line.size() is unequal 3, the remaining checks will be skipped (lazy evaluation). Those will only be checked, if line.size() == 3.
> Or did I missunderstand your comment?

You are correct, I think Laurent just didn't notice the initial size check.

> authwidget_p.cpp:259
>  
> -    if (!isGoogleHost(url)) {
> +    const QStringList line = QString::fromLatin1(data).split(QLatin1Char(' '));
> +    if (line.size() != 3 || line.at(0) != QStringLiteral("GET") || !line.at(2).startsWith(QStringLiteral("HTTP/1.1"))) {

You can use `QByteArray::split()` and continue operating on `QByteArray` below and only convert the value on `line.at(1)` to `QString` when building the `QUrl`

> authwidget_p.cpp:261
> +    if (line.size() != 3 || line.at(0) != QStringLiteral("GET") || !line.at(2).startsWith(QStringLiteral("HTTP/1.1"))) {
> +        qCDebug(KGAPIDebug) << QStringLiteral("Token response invalid");
> +        emitError(InvalidResponse, QStringLiteral("Token response invalid"));

Please add `qCDebug(KGAPIRaw)` that logs the content of the received data for easier debugging in case there's an error.

> authwidget_p.cpp:262
> +        qCDebug(KGAPIDebug) << QStringLiteral("Token response invalid");
> +        emitError(InvalidResponse, QStringLiteral("Token response invalid"));
>          return;

Please use `tr()` instead of `QStringLiteral` here, this is a user-facing message so it needs to be localized.

> authwidget_p.cpp:277
> +            qCDebug(KGAPIDebug) << QStringLiteral("Could not extract token from HTTP answer");
> +            emitError(InvalidResponse, QStringLiteral("Could not extract token from HTTP answer"));
>          }

`tr()` instead of `QStringLiteral`

> authwidget_p.h:67-69
> +    QTcpServer *server;
> +    int serverPort;
> +    QTcpSocket *connection;

Initialize the new members here to default values

REPOSITORY
  R477 KGAPI Library

REVISION DETAIL
  https://phabricator.kde.org/D9885

To: davidk, mlaurent, dvratil, #kde_pim
Cc: dvasin, winterz, vkrause, mlaurent, knauss, dvratil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20180117/68e1c4ca/attachment.html>