Review Request 124140: Ported over to Netty and added SSL support

Albert Vaca Cintora albertvaka at gmail.com
Mon Jun 22 04:24:46 UTC 2015 

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


I've tested it in two phones and it worked at the beginning, but it breaks easily. One of the things I've done that caused problems is: turn the phone WiFi off, unpair, and turn the WiFi on again (so the other device doesn't know that we have unpaired). After that, they started misbehaving: it was not possible to pair them again, they disappeared and reappeared intermittently from the list of available devices of each other, etc. I've seen some crashes in the logcat, this is a stack trace from one of them: http://pastebin.com/4KfpnsYm

You should do some testing (manual if it can't be automated) to make sure that all this edge cases work correctly and the devices are always doing what they should do. Debugging this will definitely take you more time than the time you have before the mid-term evaluation, but I don't think it makes sense to start working on the Qt side until this is not perfect or we will just carry over the same bugs. 

Also, the code needs a lot of clean up and you will also need to polish a bit the user interface (see my comments below).


build.gradle (lines 55 - 56)
<https://git.reviewboard.kde.org/r/124140/#comment55954>

    Nice that you decided to go with Netty. Is the other package that you changed a dependency of Netty? It was there originally for the SFTP plugin, have you tested that the new version is still compatible and the SFTP plugin still works?



res/layout/activity_pair.xml (lines 28 - 59)
<https://git.reviewboard.kde.org/r/124140/#comment55950>

    This info is not user friendly at all:
    - When not using SSL it doesn't make any sense to display it.
    - Most users don't care about this and/or don't know what to do with that information. For that reason it should not be in the main interface.
    
    My proposal for improvement: Put it in an options menu (three dots) named "Encryption info" or something like this. If there is no SSL it should say so: "The other device doesn't use a recent version of KDE Connect, using the legacy encryption method.". If there is SSL available, show the key hashes.



res/xml/general_preferences.xml (line 14)
<https://git.reviewboard.kde.org/r/124140/#comment55949>

    This shouldn't be an option, we can decide for the user. SSL is better and there should no reason to want to disable it.



src/org/kde/kdeconnect/Backends/LanBackend/LanLink.java (line 49)
<https://git.reviewboard.kde.org/r/124140/#comment55955>

    I like how minimalistic the changes needed in this class are :)



src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java 
<https://git.reviewboard.kde.org/r/124140/#comment55956>

    You probably need to enable keepalive here as well, as we used to do with mina.



src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java (line 230)
<https://git.reviewboard.kde.org/r/124140/#comment55953>

    This won't allow you to receive packets longer than 8192 bytes! The previous max size was 512*1024 (512kb), you can keep that value.



src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java (line 371)
<https://git.reviewboard.kde.org/r/124140/#comment55957>

    Same thing with the 8192 size limit.



src/org/kde/kdeconnect/Device.java (lines 313 - 314)
<https://git.reviewboard.kde.org/r/124140/#comment55951>

    I liked your previous approach with the PairingHandler much better: each type of link could store the information it needed in the Handler without having to put it inside device like this. 
    
    Also with the handlers every device could use different pairing mechanisms, which can be very useful:
    - We could implement tests with a fake pairing handler.  
    - A Bluetooth link could rely on the pairing made by Bluetooth, and not ask the user to re-pair in KDE Connect.
    
    I think you should have finished the "refactor" task before starting to implement this, because without refactoring the pairing now the code is much dirtier here... before we can merge this, I would like to see the pairing-related things refactored out of this class.



src/org/kde/kdeconnect/NetworkPackage.java (line 209)
<https://git.reviewboard.kde.org/r/124140/#comment55952>

    Instead of adding a new field, use the protocolVersion: increase the protocol version number in this release, and check if the version is greater than the old version to enable SSL. 
    
    I think we are making a change big enough to bump the protocol version number.


- Albert Vaca Cintora


On jun. 21, 2015, 5:41 a.m., Vineet Garg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124140/
> -----------------------------------------------------------
> 
> (Updated jun. 21, 2015, 5:41 a.m.)
> 
> 
> Review request for kdeconnect and Albert Vaca Cintora.
> 
> 
> Repository: kdeconnect-android
> 
> 
> Description
> -------
> 
> * Since MINA was tying hands in adding SSL, ported over Netty which has a good, clean and easy interface and a larger user and developer base.
> * Added support to setup links on SSL
> * Links automatically removed if wrong certificate is sent
> * Shows keys based on hash of certificate to check right certificates are received
> * Added a preference to use SSL, as it is experienced to cause high CPU usage on devices.
> * Corrected unit tests, LanLinkProvider is removed as it is nearly impossible to write it with current design. Will find a way
> 
> 
> Diffs
> -----
> 
>   build.gradle 6520c1c 
>   res/layout/activity_pair.xml 6b73edc 
>   res/values/strings.xml c128342 
>   res/xml/general_preferences.xml 82ab0ca 
>   src/org/kde/kdeconnect/Backends/BaseLink.java 579a7af 
>   src/org/kde/kdeconnect/Backends/LanBackend/LanLink.java 5994142 
>   src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java ae26958 
>   src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackLink.java add92f8 
>   src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackLinkProvider.java bd9c41b 
>   src/org/kde/kdeconnect/BackgroundService.java 5e3d8c2 
>   src/org/kde/kdeconnect/Device.java 3262aef 
>   src/org/kde/kdeconnect/Helpers/SecurityHelpers/RsaHelper.java PRE-CREATION 
>   src/org/kde/kdeconnect/Helpers/SecurityHelpers/SslHelper.java PRE-CREATION 
>   src/org/kde/kdeconnect/NetworkPackage.java f62b5d0 
>   src/org/kde/kdeconnect/UserInterface/MainSettingsActivity.java 1b0e230 
>   src/org/kde/kdeconnect/UserInterface/PairActivity.java 7a45751 
>   tests/org/kde/kdeconnect/DeviceTest.java 5d3383d 
>   tests/org/kde/kdeconnect/LanLinkProviderTest.java fe7863a 
>   tests/org/kde/kdeconnect/LanLinkTest.java d3d94c9 
>   tests/org/kde/kdeconnect/NetworkPackageTest.java a21114e 
> 
> Diff: https://git.reviewboard.kde.org/r/124140/diff/
> 
> 
> Testing
> -------
> 
> Tesed on some device with where both supports SSL, also with PC where ssl is not supported, working fine. Need a little bit more testing with more devices.
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20150622/6d9d0aab/attachment-0001.html>

More information about the KDEConnect mailing list