Added dialog to set execute permission for executable file when trying to run it.
ClosedPublic

Authored by mdlubakowski on Jul 17 2019, 2:30 PM.

Details

Summary

Currently if you try to run executable file for which you don't have execute permission, KRun will either open it in some default program, or prompt user to choose one. It would make sense that, if a file can be normally run with +x permission, user should be asked if he likes to automatically make it executable and run it. If user chooses "Open" option, KRun falls back to default behavior.
This QoL change makes it easier to just download & run things like AppImage files.
Obviously, running executables downloaded from the internet may not be safe, so a warning is included in the prompt.

Here is how it looks:

Test Plan
  1. Obtain two executable files (e.g. AppImage), set +x on one and -x on another.
  2. Try to run both files. The one with +x will run normally, the other one will show the prompt.

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D22510
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16041
Build 16059: arc lint + arc unit
mdlubakowski created this revision.Jul 17 2019, 2:30 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 17 2019, 2:30 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mdlubakowski requested review of this revision.Jul 17 2019, 2:30 PM

One of the commits did not upload

If the use case you're targeting is specifically AppImage files, it might make sense to coordinate with upstream, where they're working on ways to improve this: https://github.com/AppImage/appimaged/issues/30. There's a small helper called AppImageLauncher that can do this automatically, so I'm not sure it makes sense to use KRun for it. @probono?

mdlubakowski edited the summary of this revision. (Show Details)Jul 17 2019, 2:35 PM
mdlubakowski added a reviewer: Frameworks.

Doesn't AppImageLauncher take care of this? When I download an AppImage, I click on it in Dolphin and I get prompted whether to integrate it or run once.

mdlubakowski added a comment.EditedJul 17 2019, 3:02 PM

If the use case you're targeting is specifically AppImage files, it might make sense to coordinate with upstream, where they're working on ways to improve this: https://github.com/AppImage/appimaged/issues/30. There's a small helper called AppImageLauncher that can do this automatically, so I'm not sure it makes sense to use KRun for it. @probono?

My personal target was both ELFs and AppImages, since by default they just open the dialog to choose how to open them, but this patch should benefit anyone who uses executables that inherit from application/x-erxecutable etc.

Doesn't AppImageLauncher take care of this? When I download an AppImage, I click on it in Dolphin and I get prompted whether to integrate it or run once.

I must admit, I didn't know about this. It would have made my life easier. I was a little annoyed about the default behavior and decided to change it.

Very much appreciate this. Thank you.

This is nothing AppImage specific. It should handle all sorts of ELF files, which AppImages are just a special case of.

Things like AppImageLauncher are really just workarounds because functionality like this has been missing natively from the system so far.

dfaure requested changes to this revision.Sun, Aug 25, 10:17 AM
dfaure added inline comments.
src/widgets/krun.cpp
1187

This can be removed now that isExecutableFile returns true for desktop files.

1451

is *an* executable

(otherwise this sentence is very confusing)

1452

What happens for desktop files? Won't this go into this code, before going into the existing code to make desktop files executable?

1466

Only local files can be executed anyway, so you could use file.setPermissions(QFile::ExeUser | file.permissions()) like makeFileExecutable does for desktop files.

Actually maybe you could reuse more of that code?

This revision now requires changes to proceed.Sun, Aug 25, 10:17 AM

Incorporated feedback

mdlubakowski marked 4 inline comments as done.Thu, Aug 29, 11:25 AM

First of all, sorry for the mess I made here with Arcanist. My SSD died recently and I lost original repository with those changes, so I used arc patch to pull it back, updated the code and pushed here with arc diff --update. I'm not sure if it affects anything though, I tested on fresh repository and it can be patched with this revision without problem.
In response to the feedback, I ditched my original dialog - it is now shared with that from makeServiceExecutable helper function (the one that shows for .desktop files), and its code is moved to showUntrustedProgramWarning method. Code for setting executable bit is also moved, from makeFileExecutable to setExecuteBit.
Desktop and script files also show another type of dialog, one which ask user whether to open or execute the file, but I decided not to use it here with executables since I don't think anyone needs to open executables with external programs.

src/widgets/krun.cpp
1187

isExecutableFile will return false if the file doesn't have +x bit, which will result in prompt not being show, so I dont think this should be removed.

1451

This line is now removed

1452

Moved all the code to KRun::runUrl instead, so it doesnt intercept anything

mdlubakowski marked 5 inline comments as done.Thu, Aug 29, 11:27 AM
dfaure requested changes to this revision.Mon, Sep 2, 12:59 PM
dfaure added inline comments.
src/widgets/krun.cpp
207

_pre-existing) this code will also be triggered when switching virtual desktops, and we don't want a resize then. So this should test for e->spontaneous() and return early if true.

239

Is this line necessary? (I wouldn't think so)

309

rename to file now that it's used for both

362

At this point checking the mimetype again is redundant, it was done on line 328. I guess we need a simple QFileInfo::isExecutable call or wrapper.

1187

Ah. Hmm. I see. This code is confusing.
isFileExecutable will then be true for a non-+x desktop file.

But other than the naming, it makes sense, this code is actually about the edit-or-execute prompt for +x scripts and (any) desktop files.

This revision now requires changes to proceed.Mon, Sep 2, 12:59 PM
  • Reverse change to KRun::isExeutableFile
  • Incorporated feedback
mdlubakowski marked 6 inline comments as done.Mon, Sep 2, 4:42 PM

Reversed my changes to isExecutableFile method as per D23660, since my changes aren't needed anymore. Also added hasExecuteBit method.

  • Removed leftover kfileitem.h include
dfaure accepted this revision.Mon, Sep 2, 5:18 PM

Thanks for the changes. Looks ok to me now, despite some more nitpicks ;)
Do you have a developer account to submit this?

src/widgets/krun.cpp
156

I wonder why this doesn't use isExecutable(mimetype)...

176

This could now be simplified to return file.isExecutable();

This revision is now accepted and ready to land.Mon, Sep 2, 5:18 PM

Thanks for your guidance!
I don't have developer account, so can't land myself.
Should I fix those two other comments before landing?

pino added inline comments.Mon, Sep 2, 5:25 PM
src/widgets/krun.cpp
173

please make this function as file static, so there is no need to expose it as API of the KRun class

310

maybe this function could return the error string on failure as out parameter, so ...

373

... here this dialog can show the error to the user

(also, please drop the exclamation mark from the message)

1452

extra empty line

dfaure added a comment.Mon, Sep 2, 5:28 PM

Pino is right, I hadn't noticed that you added it as API.

Yes please make the requested changes, we'll land it when it's perfect :-)

  • Incorporated feedback
mdlubakowski marked 6 inline comments as done.Mon, Sep 2, 6:26 PM

Fixed according to comments, hope I haven't forget anything this time.
Sorry for the API change, I thought it may make sense to add it like this since isExecutableFile and isExecutable are a part of it.
I also added showing the errorString from setExecuteBit to makeServiceExecutable, since it used similar dialog to KRun::runUrl

mdlubakowski edited the summary of this revision. (Show Details)Mon, Sep 2, 6:36 PM
dfaure added inline comments.Mon, Sep 2, 8:03 PM
src/widgets/krun.cpp
156

Hmm, because isExecutable(mimetype) also returns true for desktop files.

So this would change behaviour for users of this API. (see why we try to limit public API...).

Most users found in https://lxr.kde.org/ident?_i=isExecutableFile would actually see this as a bugfix (they warn about, or forbid, local execution), but
https://lxr.kde.org/source/extragear/utils/krusader/krusader/Panel/panelfunc.cpp
would try to run the desktop file with runCommand which uses KProcess, this will fail.

So... I was just wondering (and hinting at an investigation, not requesting a change), but I have my answer now..

I suggest to revert your last change, and maybe add a comment about why we can't use isExecutable. (Or we could extract a common function...)

  • Reverted change to KRun::isExecutableFile
mdlubakowski marked an inline comment as done.Tue, Sep 3, 10:05 AM
mdlubakowski added inline comments.
src/widgets/krun.cpp
156

I understand, it was a bit careless from me not to check this beforehand. Reverted and added a note.

mdlubakowski marked 2 inline comments as done.Tue, Sep 3, 10:05 AM
dfaure accepted this revision.Tue, Sep 3, 10:05 AM

Thanks! I'll land it.

dfaure closed this revision.Tue, Sep 3, 10:08 AM