[KDE/Mac] Review Request 120431: Fix and future-proof Dr Konqi security methods on Bugzilla

Thomas Lübking thomas.luebking at gmail.com
Sun Oct 5 12:17:16 UTC 2014 

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


Not familiar with the codebase, sorry - just some codingstyle nazi remarks (in case you feel bored ;-)


drkonqi/bugzillalib.h
<https://git.reviewboard.kde.org/r/120431/#comment47362>

    Nitpick:
    Coding style -> blanks between braces and content.
    
    As a name suggestion:
    "CookieSecurity" etc. to indicate relation?



drkonqi/bugzillalib.h
<https://git.reviewboard.kde.org/r/120431/#comment47363>

    inline ... () const { ...



drkonqi/bugzillalib.h
<https://git.reviewboard.kde.org/r/120431/#comment47364>

    "lookupServerVersion" ?



drkonqi/bugzillalib.h
<https://git.reviewboard.kde.org/r/120431/#comment47365>

    "serverVersionFound" ?
    
    It should probably align the slots name and "bugzilla" is this library



drkonqi/bugzillalib.h
<https://git.reviewboard.kde.org/r/120431/#comment47366>

    blanks



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47367>

    superfluous whitespace between function and brace.



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47370>

    static int compareVersions(.)



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47368>

    "++part" (yes, i know: gdb will optimize that for you...)



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47369>

    same line as closing brace
    if () {
    } else {
    }



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47371>

    blanks



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47372>

    const QString



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47373>

    lowercase "ok" - capitals are usually used for macros



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47374>

    ++part



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47375>

    or
    if (++part >= nVersionParts)
    
    =)



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47377>

    const int



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47376>

    ++n



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47378>

    kFatal()



drkonqi/bugzillalib.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47379>

    I DO NO KNOW, but maybe kDebug(1410) for DrKonqui?



drkonqi/reportassistantpages_bugzilla.cpp
<https://git.reviewboard.kde.org/r/120431/#comment47380>

    personally i'd not embrace this because it looks as if some || was missing.


- Thomas Lübking


On Okt. 5, 2014, 4:27 vorm., Ian Wadham wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120431/
> -----------------------------------------------------------
> 
> (Updated Okt. 5, 2014, 4:27 vorm.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Runtime and Ben Cooksley.
> 
> 
> Bugs: 337742
>     http://bugs.kde.org/show_bug.cgi?id=337742
> 
> 
> Repository: kde-runtime
> 
> 
> Description
> -------
> 
> When bugs.kde.org changed over to Bugzilla 4.4.5 in July 2014, the security method used by Bugzilla changed from cookies to tokens that had to be supplied as parameters with every secure remote-procedure call. Further changes to security methods have been announced by Bugzilla and are documented for unstable 4.5.x versions of Bugzilla software. Tokens will be deprecated and then discontinued. When this happens, Dr Konqi will need to supply a user-login name and a password with every secure remote-procedure call. Furthermore, the traditional "User.login" call presently used by Dr Konqi will be deprecated and discontinued.
> 
> This patch fixes the tokens problem, which has given rise to several bug reports https://bugs.kde.org/show_bug.cgi?id=337742 and duplicates. It also provides for automatic switching to passwords-only security as and when the Bugzilla version changes again. This uses
> a general data-driven approach which can be easily updated, ahead of time, next time Bugzilla announces a change that affects Dr Konqi, whether it be in security methods or some other feature.
> 
> NOTES:
> 1. This patch is intended to be forward-portable to Frameworks/KF5, but I work on Apple OS X, where it is not yet possible to run Frameworks/KF5 and do the porting and testing. So could someone else please do it?
> 2. Another Review Request https://git.reviewboard.kde.org/r/120376/ addresses the tokens issue only, but it should be reviewed and shipped as a matter of urgency, both in KDE 4 and Frameworks, the next bug-fixing release for KDE 4.14 being due for tagging on Thursday, 9 October. That will leave more time for this review (120431) of my more long-term and more general patch.
> 3. The passwords-only part of my patch is currently storing the password in clear. Suggestions re encryption are welcomed --- or the code could be changed to make use of KWalletD mandatory (but that might not be fully portable to all platforms).
> 4. When the Bugzilla call "User.login" is discontinued, some re-sequencing of the flow of KAssistantDialog pages will be needed. I have not attempted to do that at this stage. Probably the entry of the user name and password should be delayed until the report has been accepted by the Dr Konqi logic and it is just about to be sent to bugs.kde.org or attached to an existing bug report.
> 
> REFERENCES:
> http://www.bugzilla.org/docs/
> http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService.html#LOGGING_IN Bugzilla 4.5.x (future) API doco re security
> http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService.html#LOGGING_IN Bugzilla 4.4.5 (current) API doco re security
> http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService/User.html#login User.login will be DEPRECATED in 4.5.x
> 
> 
> Diffs
> -----
> 
>   drkonqi/bugzillalib.h 570169b 
>   drkonqi/bugzillalib.cpp f74753c 
>   drkonqi/reportassistantpages_bugzilla.h b7af5b8 
>   drkonqi/reportassistantpages_bugzilla.cpp 22183f0 
> 
> Diff: https://git.reviewboard.kde.org/r/120431/diff/
> 
> 
> Testing
> -------
> 
> Used the bugstest.kde.org database and KDE 4 master on KDE/kde-runtime repository.
> 
> Tested a range of version numbers (see commented-out test data) against a range of 5 or 6 hypothetical and real Bugzilla versions at which things could or will change. This was to test the basic version-checking and feature-choosing algorithm.
> 
> Tested submitting both full reports and attached reports, using both the token method and the passwords-only method.
> 
> Also tested with KWalletD supplying the username and password on Dr Konqi's login dialog.
> 
> 
> Thanks,
> 
> Ian Wadham
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20141005/8b71c92b/attachment-0001.html>

More information about the kde-mac mailing list