> 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 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 > >
