Add KAuth support to delete operation
AbandonedPublic

Authored by chinmoyr on Jun 12 2017, 4:45 PM.

Details

Reviewers
elvisangelaccio
Group Reviewers
Frameworks
Summary

This patch makes it possible to delete root owned files and folders.
To avoid any accidental deletion a warning is shown informing the user about the write-protection. The warnings can either be from KIO::JobUIDelegate or from file ioslave because there arise two cases while deleting with escalated privilege.

Case 1: The parent dir of the item(s) to be deleted is write-protected
In this case the warning will be shown from JobUIDelegate. Since this will be a privileged operation, the warning dialog won't have the "Don't show again" checkbox. Following the warning the polkit authentication dialog will be shown if the user has not authenticated himself otherwise if the delete was performed within the threshold time the file will be deleted imediately.

Case 2: The parent isn't write-protected but some of the children are
In this case the regular warning is shown from JobUIDelegate and ioslave performs the delete operation. It continues till a root owned item is encountered. At this point either the polkit authentication dialog is shown or the warning from the ioslave.

Depends on D6197

Test Plan

use D6199 to test the changes

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
chinmoyr created this revision.Jun 12 2017, 4:45 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 12 2017, 4:45 PM
chinmoyr edited the test plan for this revision. (Show Details)Jun 12 2017, 5:09 PM
elvisangelaccio requested changes to this revision.Jun 17 2017, 8:52 AM
elvisangelaccio added inline comments.
src/ioslaves/file/kauth/file.actions
3

Let's improve this description, we should at least warn the the operation cannot be reverted.

This revision now requires changes to proceed.Jun 17 2017, 8:52 AM
chinmoyr updated this revision to Diff 15516.Jun 17 2017, 11:28 AM
chinmoyr edited edge metadata.
chinmoyr updated this revision to Diff 15517.Jun 17 2017, 11:38 AM
davidedmundson added inline comments.
src/ioslaves/file/kauth/file.actions
6

We don't want session persistence

It would mean if you prompt the user to delete a file, any rogue plugin in Dolphin can now remove any file without prompts.

src/ioslaves/file/kauth/helper.cpp
46

Rather than replying with Success when you fail, with your own structure use setError (not errorCode) and setErrorDescription

src/ioslaves/file/kauth/file.actions
6

We don't want session persistence

KAuth is actually misleading here, see D6250

(TL;DR on polkit-1 systems, there is no such thing as session persistence)

chinmoyr updated this revision to Diff 15625.Jun 20 2017, 8:51 AM
chinmoyr marked 2 inline comments as done.

Added unit tests and addressed minor issues.

chinmoyr updated this revision to Diff 15732.Jun 22 2017, 9:25 AM

minor fixes

src/ioslaves/file/kauth/file.actions
6

It would mean if you prompt the user to delete a file, any rogue plugin in Dolphin can now remove any file without prompts.

There are additional prompts as well. To be precise the ioslave shows a warning berfore proceeding if there is persistence and if folder is read-only then there is also a warning from dolphin without the "dont show again" checkbox.

There are additional prompts as well.

Those prompts are in the client code.
If there was a malicous agent hiding in a dolphin plugin, it would just be making a direct call to the helper and skip all that.

If there was a malicous agent hiding in a dolphin plugin, it would just be making a direct call to the helper and skip all that.

Till now I was not aware of what a dolphin plugin can or cannot do. I assumed there would be some kind of restriction. I was wrong , my bad.
Now the ioslave can also show a warning but right now it does so only for a particular case, i.e, when only some of the items being deleted are in read-only location but for deletion in a read-only folder we rely on the warning from jobuidelegate. So, right now the only thing I find sensible to do is to make ioslave show warning for both the cases above. By removing persistence the user will have to enter password every time he deletes something which is somewhat annoying. Getting a warning everytime is annoying as well but it is only matter of a click. So to some extent it is like the sudo command. I would also like to point out that my plan, right from the start, was to use the warning from ioslave in 'copy' and 'del' method. As for the rest, mkdir, symlink, rename etc, I was to rely on the warning from the client. I believe I may have to rethink my approach. What will you suggest?

dfaure added a subscriber: dfaure.Jul 1 2017, 12:11 PM

OK now I see the purpose of the Helper class. Strange that it appeared in the previous commit, but OK.

autotests/deletejobtest.cpp
119 ↗(On Diff #15732)

100s is too much, please remove a zero.

122 ↗(On Diff #15732)

Make it const so that first() doesn't detach.

fvogt added a subscriber: fvogt.Jan 12 2018, 1:27 PM
chinmoyr abandoned this revision.Feb 9 2018, 4:28 PM