Move handling of untrusted programs to ApplicationLauncherJob.
ClosedPublic

Authored by dfaure on Apr 24 2020, 2:33 PM.

Details

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.

Diff Detail

Repository
R241 KIO
Branch
2020_04_UntrustedProgramHandler
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25836
Build 25854: arc lint + arc unit
dfaure created this revision.Apr 24 2020, 2:33 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 24 2020, 2:33 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
dfaure requested review of this revision.Apr 24 2020, 2:33 PM
dfaure updated this revision to Diff 81108.Apr 24 2020, 3:23 PM

improve docu, add test in kruntest

dfaure updated this revision to Diff 81111.Apr 24 2020, 3:38 PM

Fix error message box after the user clicks Cancel.

dfaure updated this revision to Diff 81158.Apr 25 2020, 9:25 AM

Also set a KIO::JobUiDelegate in KRun itself (which simplifies its code)

The question is how that will work in conjunction with KNotificationJobUiDelegate? In principle we could also make it emit a KNotification with buttons

src/core/untrustedprogramhandlerinterface.h
80 ↗(On Diff #81108)

I was wondering if this should be done async? Nested event loops are quite a problem when QML is involved.

src/gui/applicationlauncherjob.cpp
116 ↗(On Diff #81108)

You know I'm not a fan of jobs suddenly blocking on IO :)

dfaure added inline comments.Apr 25 2020, 9:47 AM
src/core/untrustedprogramhandlerinterface.h
80 ↗(On Diff #81108)

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?

src/gui/applicationlauncherjob.cpp
116 ↗(On Diff #81108)

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.

dfaure updated this revision to Diff 81173.Apr 25 2020, 1:37 PM

Make UntrustedProgramHandlerInterface async.

This required a nasty QEventLoop for KRun though, since it has a sync API.

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

broulik accepted this revision.Apr 28 2020, 9:14 AM
broulik added inline comments.
src/gui/applicationlauncherjob.cpp
193 ↗(On Diff #81108)

With this it starts to look as hard to follow as KRun :)

This revision is now accepted and ready to land.Apr 28 2020, 9:14 AM
dfaure added inline comments.Apr 28 2020, 7:44 PM
src/gui/applicationlauncherjob.cpp
193 ↗(On Diff #81108)

Not even close :-)

(OpenUrlJob will be more complicated, somewhere in between this one and KRun...)

This revision was automatically updated to reflect the committed changes.