Review Request 124312: Initial SSL implementation

Sat Jul 11 00:00:38 UTC 2015

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124312/#review82345
-----------------------------------------------------------

core/backends/lan/downloadjob.cpp (line 46)
<https://git.reviewboard.kde.org/r/124312/#comment56718>

    Why do we offer the option to not use SSL?

core/backends/lan/downloadjob.cpp (line 47)
<https://git.reviewboard.kde.org/r/124312/#comment56717>

    useless debug info?

core/backends/lan/landevicelink.cpp (line 112)
<https://git.reviewboard.kde.org/r/124312/#comment56719>

    useless debug info.

core/backends/lan/lanlinkprovider.h (line 61)
<https://git.reviewboard.kde.org/r/124312/#comment56720>

    const &

core/backends/lan/lanlinkprovider.cpp (line 54)
<https://git.reviewboard.kde.org/r/124312/#comment56721>

    Is it leaking?

core/backends/lan/lanlinkprovider.cpp (line 117)
<https://git.reviewboard.kde.org/r/124312/#comment56722>

    ?

core/backends/lan/lanlinkprovider.cpp (line 125)
<https://git.reviewboard.kde.org/r/124312/#comment56726>

    NetworkPackages are usually instantiated using the stack rather than heap. Why's the change?

core/backends/lan/lanlinkprovider.cpp (line 162)
<https://git.reviewboard.kde.org/r/124312/#comment56723>

    ?

core/backends/lan/lanlinkprovider.cpp (line 181)
<https://git.reviewboard.kde.org/r/124312/#comment56724>

    I don't understand what you mean here.

core/backends/lan/lanlinkprovider.cpp (line 248)
<https://git.reviewboard.kde.org/r/124312/#comment56730>

    Opening brace goes on the next line.

core/backends/lan/lanlinkprovider.cpp (line 297)
<https://git.reviewboard.kde.org/r/124312/#comment56727>

    use take()

core/backends/lan/lanlinkprovider.cpp (line 307)
<https://git.reviewboard.kde.org/r/124312/#comment56728>

    why did you remove the qCDebug part?

core/backends/lan/lanpairinghandler.cpp (line 25)
<https://git.reviewboard.kde.org/r/124312/#comment56731>

    Opening brace, on every method, on the next line.

core/backends/lan/lanpairinghandler.cpp (line 38)
<https://git.reviewboard.kde.org/r/124312/#comment56729>

    Just use QString, no const&

core/backends/lan/lanpairinghandler.cpp (line 76)
<https://git.reviewboard.kde.org/r/124312/#comment56732>

    The success variable is unused.

core/backends/lan/server.h (line 28)
<https://git.reviewboard.kde.org/r/124312/#comment56733>

    Fix trailing spaces (all over the patch)

core/backends/lan/server.cpp (line 47)
<https://git.reviewboard.kde.org/r/124312/#comment56735>

    isEmpty()

core/backends/lan/server.cpp (line 57)
<https://git.reviewboard.kde.org/r/124312/#comment56734>

    !isEmpty()

core/backends/lan/uploadjob.cpp (line 28)
<https://git.reviewboard.kde.org/r/124312/#comment56736>

    const &

core/backends/linkprovider.h (line 38)
<https://git.reviewboard.kde.org/r/124312/#comment56714>

    Move protected between public and private. Also, why is it there? To save the setter?

core/backends/linkprovider.h (line 52)
<https://git.reviewboard.kde.org/r/124312/#comment56705>

    It should be pairingHandler();

    Also it's odd that it's public, with this all plugins will be able to access it.

core/backends/pairinghandler.h (line 34)
<https://git.reviewboard.kde.org/r/124312/#comment56715>

    Looks like you could be using signals & slots.

core/backends/pairinghandler.h (line 35)
<https://git.reviewboard.kde.org/r/124312/#comment56716>

    const NetworkPackage &np

core/daemon.cpp (line 136)
<https://git.reviewboard.kde.org/r/124312/#comment56703>

    These changes are unrelated to the patch.

core/device.h (line 159)
<https://git.reviewboard.kde.org/r/124312/#comment56704>

    Commit that right away so it's out of the way.

core/device.cpp (line 213)
<https://git.reviewboard.kde.org/r/124312/#comment56706>

    Why do we need to /try/ with all of the device links?

core/device.cpp (line 285)
<https://git.reviewboard.kde.org/r/124312/#comment56707>

    remove debug, doesn't look too useful on production.

core/device.cpp (line 474)
<https://git.reviewboard.kde.org/r/124312/#comment56708>

    Remove the QString() constructor, it's not correct. Is it because toPem returns a QByteArray?

core/kdeconnectconfig.cpp (line 36)
<https://git.reviewboard.kde.org/r/124312/#comment56709>

    `#include <QSslCertificate>`

    Keep the style of the rest of incles.

core/kdeconnectconfig.cpp (line 45)
<https://git.reviewboard.kde.org/r/124312/#comment56710>

    Again, get out of the way.

core/kdeconnectconfig.cpp (line 120)
<https://git.reviewboard.kde.org/r/124312/#comment56712>

    Isn't QSsl able to generate a certificate? It would be cool if we could drop the dependency...

core/kdeconnectconfig.cpp (line 134)
<https://git.reviewboard.kde.org/r/124312/#comment56711>

    beep!

core/networkpackage.cpp (line 50)
<https://git.reviewboard.kde.org/r/124312/#comment56713>

    Use initializers.

plugins/sftp/kdeconnect_sftp.json (line 62)
<https://git.reviewboard.kde.org/r/124312/#comment56737>

    drop.

tests/testsocketlinereader.cpp (line 82)
<https://git.reviewboard.kde.org/r/124312/#comment56738>

    Are you sure this cast is needed?

Maybe you could go a bit further with the testing? :)

- Aleix Pol Gonzalez

On July 10, 2015, 12:42 a.m., Vineet Garg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124312/
> -----------------------------------------------------------
> 
> (Updated July 10, 2015, 12:42 a.m.)
> 
> 
> Review request for kdeconnect.
> 
> 
> Repository: kdeconnect-kde
> 
> 
> Description
> -------
> 
> * Similar to Android version, this SSL implementation supports old version also.
> * Certificate is generated when public key is generated using QCA, but used as QSslCertificate because QSslSocket don't like QCA certificate.
> * A new server is written overriding QTcpServer for setting socket descriptor to QSslSocket.
> * QSslSocket works as plain socket to transfer identity package and then either uses encrypted mode or normal mode according to the received package.
> * If connetion is to be on SSL, new link is only added after successful handshake.
> * Certificate is embedded into identity package on start of encrypted mode and then set in device using it.
> * PairingHandler is added like in Android version so that each LinkProvider manage pairing according to its own rules.
> 
> P.S. : Uploaded the patch with some debug statements, will comment out them.
> 
> 
> Diffs
> -----
> 
>   core/CMakeLists.txt dd8fedb 
>   core/backends/lan/CMakeLists.txt 7c5be38 
>   core/backends/lan/downloadjob.h eeeab07 
>   core/backends/lan/downloadjob.cpp bba2e98 
>   core/backends/lan/landevicelink.h 7d31881 
>   core/backends/lan/landevicelink.cpp 0987057 
>   core/backends/lan/lanlinkprovider.h b379434 
>   core/backends/lan/lanlinkprovider.cpp f5d2b22 
>   core/backends/lan/lanpairinghandler.h PRE-CREATION 
>   core/backends/lan/lanpairinghandler.cpp PRE-CREATION 
>   core/backends/lan/server.h PRE-CREATION 
>   core/backends/lan/server.cpp PRE-CREATION 
>   core/backends/lan/socketlinereader.h b3be55a 
>   core/backends/lan/socketlinereader.cpp a6bd85e 
>   core/backends/lan/uploadjob.h 12b6f7e 
>   core/backends/lan/uploadjob.cpp b0d35fc 
>   core/backends/linkprovider.h f4fc309 
>   core/backends/pairinghandler.h PRE-CREATION 
>   core/backends/pairinghandler.cpp PRE-CREATION 
>   core/daemon.h 674ca01 
>   core/daemon.cpp af4c67b 
>   core/device.h 03f528c 
>   core/device.cpp c2dc609 
>   core/kdeconnectconfig.h af6c6df 
>   core/kdeconnectconfig.cpp 919505c 
>   core/networkpackage.h 6a0bf9c 
>   core/networkpackage.cpp 64cfab7 
>   plugins/sftp/kdeconnect_sftp.json 08a89fb 
>   tests/CMakeLists.txt b00a574 
>   tests/testsocketlinereader.cpp 77a9b4b 
> 
> Diff: https://git.reviewboard.kde.org/r/124312/diff/
> 
> 
> Testing
> -------
> 
> * Working fine, tested using connection to Android devices.
> * Pairing handler needs to be tested using two LinkProvider, working fine with one, but there be some problems with two LinkProviders simultaneouly.
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20150711/01af745a/attachment-0001.html>