D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure added inline comments. INLINE COMMENTS > broulik wrote in applicationlauncherjob.cpp:155 > With this it starts to look as hard to follow as KRun :) Not even close :-) (OpenUrlJob will be more complicated, somewhere in between this one and KRun...) REPOSITORY R241 KIO BRANCH 2020_04_UntrustedProgramHandler REVISION DETAIL https://phabricator.kde.org/D29153 To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
This revision was automatically updated to reflect the committed changes. Closed by commit R241:f0ae038490e6: Move handling of untrusted programs to ApplicationLauncherJob. (authored by dfaure). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29153?vs=81173=81472 REVISION DETAIL https://phabricator.kde.org/D29153 AFFECTED FILES autotests/applicationlauncherjobtest.cpp autotests/applicationlauncherjobtest.h src/core/CMakeLists.txt src/core/jobuidelegateextension.h src/core/untrustedprogramhandlerinterface.cpp src/core/untrustedprogramhandlerinterface.h src/gui/applicationlauncherjob.cpp src/gui/applicationlauncherjob.h src/widgets/CMakeLists.txt src/widgets/jobuidelegate.cpp src/widgets/krun.cpp src/widgets/krun_p.h src/widgets/widgetsuntrustedprogramhandler.cpp src/widgets/widgetsuntrustedprogramhandler.h tests/kruntest.cpp To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
broulik accepted this revision. broulik added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > applicationlauncherjob.cpp:155 > +} > +proceedAfterSecurityChecks(); > +} With this it starts to look as hard to follow as KRun :) REPOSITORY R241 KIO BRANCH 2020_04_UntrustedProgramHandler REVISION DETAIL https://phabricator.kde.org/D29153 To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure added a comment. @broulik ping? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29153 To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure added a task: T11549: KIO: remove unneeded QWidget dependencies to set parent windows or display errors. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29153 To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure added a comment. > The question is how that will work in conjunction with KNotificationJobUiDelegate? In principle we could also make it emit a KNotification with buttons We would need a KIO::NotificationJobUiDelegate subclass of KNotificationJobUiDelegate in KIOGui, which also implements UntrustedProgramHandlerInterface. For KF5, its constructor would register itself using setDefaultUntrustedProgramHandler, and the destructor would use that same method to reset it to nullptr (to avoid leaving a dangling pointer). Alternatively it could do just like KIO::JobUiDelegate and register a singleton. In KF6, it'll inherit the handler interface and it'll just be autodetected by qobject_cast on the job's delegate (or we could even already do this as the primary way to find the handler, with fallback to checking the singleton --- this is all because I can't change what KIO::JobUiDelegate inherits from). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29153 To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure updated this revision to Diff 81173. dfaure added a comment. Make UntrustedProgramHandlerInterface async. This required a nasty QEventLoop for KRun though, since it has a sync API. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29153?vs=81158=81173 BRANCH 2020_04_UntrustedProgramHandler REVISION DETAIL https://phabricator.kde.org/D29153 AFFECTED FILES autotests/applicationlauncherjobtest.cpp autotests/applicationlauncherjobtest.h src/core/CMakeLists.txt src/core/jobuidelegateextension.h src/core/untrustedprogramhandlerinterface.cpp src/core/untrustedprogramhandlerinterface.h src/gui/applicationlauncherjob.cpp src/gui/applicationlauncherjob.h src/widgets/CMakeLists.txt src/widgets/jobuidelegate.cpp src/widgets/krun.cpp src/widgets/krun_p.h src/widgets/widgetsuntrustedprogramhandler.cpp src/widgets/widgetsuntrustedprogramhandler.h tests/kruntest.cpp To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure added inline comments. INLINE COMMENTS > broulik wrote in untrustedprogramhandlerinterface.h:79 > I was wondering if this should be done async? Nested event loops are quite a > problem when QML is involved. I don't see a nested event loop in makeServiceFileExecutable. I guess your comment was for the main method, showUntrustedProgramWarning? Indeed we could make that one async, if I turn this interface into a QObject and add a signal. Can do. This kind of turns it into a job, but not really, we don't need this bit to have its own delegate etc. I think a signal is enough? > broulik wrote in applicationlauncherjob.cpp:117 > You know I'm not a fan of jobs suddenly blocking on IO :) You know I'm not a fan of network mounts, exactly for this reason In my opinion it's a crazy requirement to say that we are not allowed to use QFile or QFileInfo anywhere anymore, because of network mounts. What's your suggestion? I'm not even aware of an asynchronous (but still portable) equivalent. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29153 To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
broulik added a comment.
The question is how that will work in conjunction with
`KNotificationJobUiDelegate`? In principle we could also make it emit a
`KNotification` with buttons
INLINE COMMENTS
> untrustedprogramhandlerinterface.h:79
> + */
> +bool makeServiceFileExecutable(const QString , QString
> );
> +
I was wondering if this should be done async? Nested event loops are quite a
problem when QML is involved.
> applicationlauncherjob.cpp:117
> +// but that gets rid of the command line arguments.
> +QString program =
> QFileInfo(d->m_service->exec()).canonicalFilePath();
> +if (program.isEmpty()) { // e.g. due to command line arguments
You know I'm not a fan of jobs suddenly blocking on IO :)
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D29153
To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure updated this revision to Diff 81158. dfaure added a comment. Also set a KIO::JobUiDelegate in KRun itself (which simplifies its code) REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29153?vs=8=81158 BRANCH 2020_04_UntrustedProgramHandler REVISION DETAIL https://phabricator.kde.org/D29153 AFFECTED FILES autotests/applicationlauncherjobtest.cpp autotests/applicationlauncherjobtest.h src/core/CMakeLists.txt src/core/jobuidelegateextension.h src/core/untrustedprogramhandlerinterface.cpp src/core/untrustedprogramhandlerinterface.h src/gui/applicationlauncherjob.cpp src/gui/applicationlauncherjob.h src/widgets/CMakeLists.txt src/widgets/jobuidelegate.cpp src/widgets/krun.cpp src/widgets/krun_p.h src/widgets/widgetsuntrustedprogramhandler.cpp src/widgets/widgetsuntrustedprogramhandler.h tests/kruntest.cpp To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure added a reviewer: mdlubakowski. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29153 To: dfaure, ahmadsamir, broulik, ngraham, mdlubakowski Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure updated this revision to Diff 8. dfaure added a comment. Fix error message box after the user clicks Cancel. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29153?vs=81108=8 BRANCH 2020_04_UntrustedProgramHandler REVISION DETAIL https://phabricator.kde.org/D29153 AFFECTED FILES autotests/applicationlauncherjobtest.cpp autotests/applicationlauncherjobtest.h src/core/CMakeLists.txt src/core/jobuidelegateextension.h src/core/untrustedprogramhandlerinterface.cpp src/core/untrustedprogramhandlerinterface.h src/gui/applicationlauncherjob.cpp src/gui/applicationlauncherjob.h src/widgets/CMakeLists.txt src/widgets/jobuidelegate.cpp src/widgets/krun.cpp src/widgets/widgetsuntrustedprogramhandler.cpp src/widgets/widgetsuntrustedprogramhandler.h tests/kruntest.cpp To: dfaure, ahmadsamir, broulik, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure updated this revision to Diff 81108. dfaure added a comment. improve docu, add test in kruntest REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29153?vs=81101=81108 BRANCH 2020_04_UntrustedProgramHandler REVISION DETAIL https://phabricator.kde.org/D29153 AFFECTED FILES autotests/applicationlauncherjobtest.cpp autotests/applicationlauncherjobtest.h src/core/CMakeLists.txt src/core/jobuidelegateextension.h src/core/untrustedprogramhandlerinterface.cpp src/core/untrustedprogramhandlerinterface.h src/gui/applicationlauncherjob.cpp src/gui/applicationlauncherjob.h src/widgets/CMakeLists.txt src/widgets/jobuidelegate.cpp src/widgets/krun.cpp src/widgets/widgetsuntrustedprogramhandler.cpp src/widgets/widgetsuntrustedprogramhandler.h tests/kruntest.cpp To: dfaure, ahmadsamir, broulik, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29153: Move handling of untrusted programs to ApplicationLauncherJob.
dfaure created this revision. dfaure added reviewers: ahmadsamir, broulik, ngraham. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. dfaure requested review of this revision. REVISION SUMMARY This was still in KRun so the porting to ApplicationLauncherJob was actually losing that feature along the way. Move KRun's handling of untrusted programs to a separate class and provide it via an interface used by ApplicationLauncherJob and implemented in KIOWidgets. Ideally KIOWidget's JobUiDelegate class would implement it, but that's an exported class so it would be BIC. So for KF5, the JobUiDelegate registers the handler into kiogui using an internal global setter, and in KF6 JobUiDelegate itself can implement that interface. The benefit of this approach is that the application code stays the same: job->setUiDelegate(new KIO::JobUiDelegate); We'll have to update all the "new KDialogUiDelegate" to the above line instead, where using ApplicationLauncherJob. TEST PLAN Unittest + starting a non-executable desktop file and a non-executable copy of dolphin, in dolphin. REPOSITORY R241 KIO BRANCH 2020_04_UntrustedProgramHandler REVISION DETAIL https://phabricator.kde.org/D29153 AFFECTED FILES autotests/applicationlauncherjobtest.cpp autotests/applicationlauncherjobtest.h src/core/CMakeLists.txt src/core/jobuidelegateextension.h src/core/untrustedprogramhandlerinterface.cpp src/core/untrustedprogramhandlerinterface.h src/gui/applicationlauncherjob.cpp src/gui/applicationlauncherjob.h src/widgets/CMakeLists.txt src/widgets/jobuidelegate.cpp src/widgets/krun.cpp src/widgets/widgetsuntrustedprogramhandler.cpp src/widgets/widgetsuntrustedprogramhandler.h To: dfaure, ahmadsamir, broulik, ngraham Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
