Review Request: Improve handling of 802.1x certificates

[Lamarque Vieira Souza]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101275/#review3111
-----------------------------------------------------------



libs/internals/connection.h
<http://git.reviewboard.kde.org/r/101275/#comment2636>

    Please rename this method to saveCertificates and the one below to deleteCertificates to make clear what they are saving and deleting.



libs/internals/settings/802-1x.h
<http://git.reviewboard.kde.org/r/101275/#comment2644>

    addToCertToDelete



libs/internals/settings/802-1x.h
<http://git.reviewboard.kde.org/r/101275/#comment2643>

    removeFromCertToDelete



libs/internals/settings/802-1x.h
<http://git.reviewboard.kde.org/r/101275/#comment2638>

    I thinks certPathAsByteArray sounds better, just my oppinion.



libs/internals/settings/802-1x.h
<http://git.reviewboard.kde.org/r/101275/#comment2639>

    use QString() instead of "".



libs/internals/settings/802-1x.h
<http://git.reviewboard.kde.org/r/101275/#comment2640>

    Why the parameter scope is int instead of enumerate?



libs/internals/settings/802-1x.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2646>

    stick to the code sytle for if { \n } else { \n }



libs/internals/settings/802-1x.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2647>

     code style: move the { below to this line.



libs/internals/settings/802-1x.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2648>

    code style for if () { in this entire method.



libs/ui/security/peapwidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2649>

    code style for if () { \n } else { \n }



libs/ui/security/peapwidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2651>

    code style for if.



libs/ui/security/peapwidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2650>

    code style for if.



libs/ui/security/tlswidget.h
<http://git.reviewboard.kde.org/r/101275/#comment2652>

    wrong indentation in this line and below.



libs/ui/security/tlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2645>

    Stick to the code style: if { \n } else { \n }



libs/ui/security/tlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2653>

    code style for if



libs/ui/security/tlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2656>

    code style for if { \n } else { \n }



libs/ui/security/tlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2641>

    I use QLatin1String for string constants.



libs/ui/security/tlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2654>

    code style for if.



libs/ui/security/ttlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2642>

    Stick to the code style and use: if () { \n
    } else { \n
    }



libs/ui/security/ttlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2655>

    code style for if



libs/ui/security/ttlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2657>

    code stytle for if


- Lamarque Vieira


On May 2, 2011, 2:30 p.m., Ilia Kats wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101275/
> -----------------------------------------------------------
> 
> (Updated May 2, 2011, 2:30 p.m.)
> 
> 
> Review request for Network Management.
> 
> 
> Summary
> -------
> 
> Currently, KDE NM is not compatible with PEM certificates (base64-encoded), which are the most used format. Attached patch makes some improvements in certificate handling, namely:
> until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.
> 
> This is a major change in behavior, so I would like a public review.
> 
> 
> This addresses bug 209673.
>     http://bugs.kde.org/show_bug.cgi?id=209673
> 
> 
> Diffs
> -----
> 
>   libs/internals/connectionpersistence.cpp 1a22c5b 
>   libs/internals/setting.h b43365b 
>   libs/internals/setting.cpp a637855 
>   libs/internals/settings/802-1x.h 971ef41 
>   libs/internals/connection.cpp 701a9e2 
>   CMakeLists.txt 6748cee 
>   backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f 
>   libs/internals/connection.h 60516dc 
>   libs/internals/settings/802-1x.cpp 245507e 
>   libs/internals/settings/802-1xpersistence.cpp b7b9c42 
>   libs/ui/security/eapmethodleap.cpp 199a8b8 
>   libs/ui/security/eapmethodpeapbase.ui 4b9c5fd 
>   libs/ui/security/eapmethodtlsbase.ui 80a5c1f 
>   libs/ui/security/eapmethodttlsbase.ui 4f2e1a9 
>   libs/ui/security/peapwidget.h 51ad781 
>   libs/ui/security/peapwidget.cpp 2fc502b 
>   libs/ui/security/tlswidget.h c71dcf3 
>   libs/ui/security/tlswidget.cpp 1d28636 
>   libs/ui/security/ttlswidget.h b1a9049 
>   libs/ui/security/ttlswidget.cpp c135b19 
>   settings/config/manageconnectionwidget.h 58afc12 
>   settings/config/manageconnectionwidget.cpp 3c0d4dc 
> 
> Diff: http://git.reviewboard.kde.org/r/101275/diff
> 
> 
> Testing
> -------
> 
> Yes, see the bugzilla ticket.
> 
> 
> Thanks,
> 
> Ilia
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110504/7cb5aca9/attachment-0001.htm