Re: Review Request 120431: Fix and future-proof Dr Konqi security methods on Bugzilla
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120431/ --- (Updated Oct. 3, 2014, 7:03 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and Ben Cooksley. Changes --- The code for setFeaturesForVersion() has been re-written completely, to accommodate any type of feature change, in any number of versions, in a simpler and more rugged way. This should also take care of the issues Jan Kundrát raised. 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 (updated) - 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
Re: Review Request 120431: Fix and future-proof Dr Konqi security methods on Bugzilla
On Sept. 30, 2014, 9:22 a.m., Jan Kundrát wrote:
drkonqi/bugzillalib.h, line 431
https://git.reviewboard.kde.org/r/120431/diff/1/?file=315732#file315732line431
These are never saved on disk, right?
I don't think that this makes much sense given that the rest of the
stack will happily use regular QString/QByteArray/QIODevice for actual
network IO.
These are never saved on disk, right?
No. And if nobody else is worried about this, neither am I. We are not playing
for sheep-stations here.
On Sept. 30, 2014, 9:22 a.m., Jan Kundrát wrote:
drkonqi/bugzillalib.h, line 442
https://git.reviewboard.kde.org/r/120431/diff/1/?file=315732#file315732line442
`const QString `
Fixed.
On Sept. 30, 2014, 9:22 a.m., Jan Kundrát wrote:
drkonqi/bugzillalib.cpp, line 88
https://git.reviewboard.kde.org/r/120431/diff/1/?file=315733#file315733line88
It's customary to use smallCaps for these identifiers
Fixed. The new array of structs is called changes.
On Sept. 30, 2014, 9:22 a.m., Jan Kundrát wrote:
drkonqi/bugzillalib.cpp, line 107
https://git.reviewboard.kde.org/r/120431/diff/1/?file=315733#file315733line107
I would suggest a QLatin1String here, but that's me
Done.
On Sept. 30, 2014, 9:22 a.m., Jan Kundrát wrote:
drkonqi/bugzillalib.cpp, line 115
https://git.reviewboard.kde.org/r/120431/diff/1/?file=315733#file315733line115
10 is a default, so I'm not convinced it's worth stating it manually
Fixed.
On Sept. 30, 2014, 9:22 a.m., Jan Kundrát wrote:
drkonqi/bugzillalib.cpp, line 119
https://git.reviewboard.kde.org/r/120431/diff/1/?file=315733#file315733line119
While this code is correct, I dislike the fact that `currentVersion[3]`
and `part 2` are related together.
Yes, this one is longer, but also safer:
if (part = sizeof(currentVersion)/sizeof(*currentVersion)) {
// ...
The 3 is a const int now and the BugzillaVersion triplet is a typedef. I have
used sizeof() to calculate the number of struct items in BugzillaChange
changes[].
On Sept. 30, 2014, 9:22 a.m., Jan Kundrát wrote:
drkonqi/bugzillalib.cpp, line 154
https://git.reviewboard.kde.org/r/120431/diff/1/?file=315733#file315733line154
It would be much cleaner IMHO if there was a single
map/associative_array/... mapping the versions to feature set. The proposed
way is IMHO quite fragile as the code relies on the fact that the
`security` and `Versions` share the same length, yet this is not checked
anywhere.
The changes[] array is now an array of structs, each containing a version at
which the change takes place and an enum value to label the type of change.
The labels are used later in a switch() where the actual feature codes (e.g.
m_security) get set.
On Sept. 30, 2014, 9:22 a.m., Jan Kundrát wrote:
drkonqi/bugzillalib.cpp, lines 164-173
https://git.reviewboard.kde.org/r/120431/diff/1/?file=315733#file315733line164
switch statement to make it obvious that we're checking an enum and
want to react to each and every option?
Done.
On Sept. 30, 2014, 9:22 a.m., Jan Kundrát wrote:
drkonqi/bugzillalib.cpp, line 459
https://git.reviewboard.kde.org/r/120431/diff/1/?file=315733#file315733line459
What guarantees that there's always result[0]?
Bugzilla? Or are their announcements in question? Anyway, I have added a check
that the result list has count() 0.
On Sept. 30, 2014, 9:22 a.m., Jan Kundrát wrote:
drkonqi/bugzillalib.cpp, line 461
https://git.reviewboard.kde.org/r/120431/diff/1/?file=315733#file315733line461
This leaks authentication data to an on-disk log.
Deleted. Also found another instance and fixed it. BTW, tokens are
intentionally short-lived IIUC. You get a new one on every login.
- Ian
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120431/#review67658
---
On Oct. 3, 2014, 7:03 a.m., Ian Wadham wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120431/
---
(Updated Oct. 3, 2014, 7:03 a.m.)
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
Re: Review Request 120431: Fix and future-proof Dr Konqi security methods on Bugzilla
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120431/ --- (Updated Oct. 3, 2014, 7:52 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime and Ben Cooksley. Changes --- Fix white space and a slip of the pen. 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 (updated) - 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
