Enable modifying root-owned files in Dolphin
Needs ReviewPublic

Authored by chinmoyr on Aug 27 2017, 1:02 PM.

Details

Reviewers
dfaure
emmanuelp
Group Reviewers
Dolphin
Maniphest Tasks
T6561: Polkit support in KIO
Summary

This patch adds PrivilegeExecution flag to KIO jobs in dolphin thus enabling them to perform file operation with elevated privileges.
Depends on D7563 for context menu to show properly.

TODO:

  • Rename action for multiple selections in a read-only folder is disabled because mass rename uses CopyJob in a loop and doing it in a read-only folder will trigger multiple prompts. Enable it again.
  • Trashing from read-only folder shows error. Fix it.

Diff Detail

Repository
R318 Dolphin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
chinmoyr created this revision.Aug 27 2017, 1:02 PM
Restricted Application added a subscriber: Konqueror. · View Herald TranscriptAug 27 2017, 1:02 PM

Looks good, much simpler than the initial patch! (thanks to D7563)

src/dolphinmainwindow.cpp
1323–1334

Why not exploit D7563 also for renaming?

dfaure added inline comments.Sep 3 2017, 8:12 AM
src/dolphinmainwindow.cpp
1323–1334

Yes it would make more sense for this to be in KFileItemListProperties.

Remember that the file dialog, too, supports most file operations. So you should test what happens there.

emmanuelp added inline comments.Sep 3 2017, 9:45 AM
src/panels/folders/folderspanel.cpp
260

Why is the privileged execution opt-in instead of opt-out?

A KIO user usually doesn't care how KIO handles the requested operations, because KIO claims to be resource and network transparent. Thus, if KIO is able to perform privileged operations when needed, it should do it by default unless explicitly forbidden by the client.

Additionally, opt-in requires you to patch all KIO users (incl. unknown 3rd-party users), whereas opt-out only requires patching in some special cases (if there is any?).

dfaure edited edge metadata.Sep 3 2017, 10:04 AM

It's opt-in because it should only happen for user-triggered operations.
An application copying some config file in the background (or any other internal file operation) shouldn't bother the user with prompts.

In case of Dolphin yes it means modifying many lines, but in general, there are also many many jobs triggered by apps.

emmanuelp edited edge metadata.Sep 6 2017, 9:55 PM

An application copying some config file in the background (or any other internal file operation) shouldn't bother the user with prompts.

The same happens e.g. when overwriting an existing file in the background.
Such applications explicitly handle such cases to avoid prompts (e.g. by setting the overwrite flag or by prohibiting any user interaction).

In case of Dolphin yes it means modifying many lines

I was generally speaking. I think that the opt-in approach will lead to annoying inconsistencies (even within an application e.g. when someone forgets to add the KIO::PrivilegeExecution flag in newly added code).
Some applications like Dolphin will allow privileged operations while others (e.g. Plasma, Krusader, ...) don't (if not patched). This is bad from a user point of view.

I would like to add one point. With an opt-out flag we will have to patch every corner case of every application that uses KIO::Job. Whereas with PrivilegeExecution flag this transition can be slow and will be less error prone.

dfaure added a comment.Sep 9 2017, 6:15 PM

Hmm. Emmanuel might have a point.
Looking at KIO jobs usage, I see a lot that are user-triggered and would benefit from privilege-execution support.
All of KIO's filewidgets library (filedialog etc.) obviously qualifies. KTextEditor for editing root-owned files, KParts for vieweing them, and then the same in most apps that use KIO to open and/or save files.

One exception is KUrlCompletion where popups are annoying, but that's just a candidate for an opt-out flag.

One thing however is that only one or two people tested this feature at this point, so I'm not 100% sure of the side effects on all apps (more precisely: finding out in which case we don't want privilege-execution to be available).
Maybe we can have it in committed "on by default" (opt-out) now, to test it heavily for 3 weeks (before the next KF5 release) so we can re-evaluate before the release (and turn it off by default if we find out it should be).

I would suggest keeping it off by default and if we don't encounter any inconsistencies then we can turn it on for good. I believe this way we would have much less surprises or is it the surprises we are seeking?

Indeed the advantage of opt-out is that if there are bugs, we will find them.
If we go the opt-out way, we could maybe ask Neon unstable users to help testing the feature before the next Frameworks release.

So how do you suggest I proceed? Add an extra flag and comment out the current one or replace it altogether?

Add both flag values, so that the current patches still work and don't need to be changed again if we toggle the default.

mati865 added a subscriber: mati865.Jun 3 2018, 1:52 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptJun 3 2018, 1:52 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
ngraham added a subscriber: ngraham.Jun 3 2018, 2:53 PM