This is regarding the GSOC idea https://community.kde.org/GSoC/2017/Ideas#Project:_Polkit_support_in_KIO.

This patch intends to demonstrate one possible approach to provide polkit support in kio. Here its only for the delete operation. This is based on the patch in task https://phabricator.kde.org/T5070.

The approach is as follows;
1. Whenever file ioslave gets access denied error it calls the method execWithRoot with the action that requires priviledge, the path of items
upon which action needs to be performed and a warning ID as arguments.
2. execWithRoot then executes the KAuth::Action org.kde.kio.file.execute.
3. This Kauth::Action has its Persistence set too 'session'. This means that after authentication the restrictions are dropped for a while, for
about 5 minutes. This is similar to the behaviour of sudo command.
4. During this time we can perform any action as a privileged user without any authentication. So to prevent any mishap i added a warning box which
would popup before performing any action(only during this period).
5. After the said time interval the root privileges are droped and calling execWithRoot should show the usual authentication dialog.

Testing Done

Screenshots

Files

Issues

2

24

1

All Issues:27

Description

From

Last Updated

I'm not convinced, what's the point of requestroot? Why don't you let it call the action right away?

For a single file calling action right away might work but doing this for multiple files will show the authentication dialog everytime before file is deleted. This is something we don't want.
One way to solve this is to store all the paths and delete them in helper but this approach might cause problem while porting FileProtocol::copy.

doing this for multiple files will show the authentication dialog everytime before file is deleted.

This should not happen, sounds like a bug in KAuth? With Persistence=session the authorization is supposed to last until the user logs out, according to https://api.kde.org/frameworks/kauth/html/namespaceKAuth.html

Added Files:

Description:

This is regarding the GSOC idea https://community.kde.org/GSoC/2017/Ideas#Project:_Polkit_support_in_KIO.

This patch intends to demonstrate one possible approach to provide polkit support in kio. Here its only for the delete operation. This is based on the patch in task https://phabricator.kde.org/T5070.

The approach is as follows;

~

The file ioslave gets three methods, /getRootPermission, execWithRoot and unsetRoot/ with a variable /isRoot/ to store the persistance. The helper gets two actions, /org.kde.kio.file.requestroot/ and /org.kde.kio.file.execute/.

~

When an action encounters access denied error the method "execWithRoot" is called with the action you want to perform and the path of objects upon which you want to perform the action as arguments. This method then calls "getRootPermission" for authorisation purpose. Upon successfull authorisation this will then go on performing the desired action as privileged user. Once the job is finished "unsetRoot" is called.

~

For authorisation a call to "org.kde.kio.file.requestroot" will be made. This action has its "Policy" set to "auth_admin" so as to prompt for password every time its called. And the action "org.kde.kio.file.execute" has its "Policy" set to "yes" so that it can carry out the desired action as a priviledged user without asking for authentication.

~

~

As for the deletion of files and directories are concerned, the authentication dialog will pop up only once i.e, for the first file/directory that needs requires a priviledged user to delete them. If there are more files which only priviledge users can delete then they will be deleted straightaway without asking for authentication. This is decided by the truth of variable "isRoot". Once the delete job is finished "isRoot" is set to false. In short once the job has started and authentication's been done, the root access will persist and once the job is finished the root access will reset.

~

1. Whenever file ioslave gets access denied error it calls the method execWithRoot with the action that requires priviledge, the path of items

~

upon which action needs to be performed and a warning ID as arguments.

can you explain the meaning of the isRoot member to me? It sounds like "I am authorized to be root", but here it's set while in "auth required" state, sounds like we're not root yet?
(I know nothing about polkit/kauth but at least this code should make sense to me ;)

The name isRoot is misleading so I changed it to mPriviledgeOpStarted. It is used to show that the priviledged operation has started and will be set to false once the job ends. This along with kauthStatus are used to decide whether to show warning dialog or not.

The "execute" method in KAuth is similar to sudo command. The priviledge persist for about 5 minutes. It is during this time we need to show the warning. Now when deleting multiple files we can either delete them using one job(deleteRecursive) or multiple jobs. So to determine whether the call to execWithRoot was from the same job or not and show the warning accordingly I used the aforementioned members.

So my logic here is,
- Prepare the arguments
- If kauth status is Auth Required status(i.e. execWithRoot is called for the first time) then set mPriviledgeOpStarted to true.
- If kauth status is Authorized status (i.e. priviledges persists) and mPriviledgeOpStarted is false(this is a different job) then set it to true.
- If the user entered the correct password or chose continue proceed with the action else halt.

Why is this using a QueuedConnection? This prevents any sort of error handling because this method returns too early.

Testing that invokeMethod worked is only half the story. If the QueuedConnection can be removed, you can use a return argument of type bool for error handling?
Or better, a KIO error code, so we can give the exact error message (I see that we didn't have "directory not empty" for rmdir, but if this is extended to other actions then it will be useful to have).

In fact, instead of invokeMethod this could be a set of if(method == "...")? Seems simpler to me, but I have no strong opinion.

I'm not sure if we should show the warning dialog at the kio_file level. This would result in a double dialog when deleting files with Shift+Del from file managers. What about making jobuidelegate.cpp smarter? It could show a more "scary" message if the file that's being deleted is write-protected.

In my system when i use Shift+del there is no warning dialog (few months back dolphin used to show the dialog but strangly it doesn't shows now) so this case didn't occured to me.

If I am not wrong jobuidelegate's warning is shown before the job executes and kauth's authentication dialog is shown only after job executes. But its just the opposite we want. So if there's a way for slave to notify the job of starting of a priviledged action then there should be no problem in making jobuidelegate smarter.
(But the thing is at this very moment i can't think of or find any possible way)

You probably checked the "don't show again" checkbox ;) The jobuidelegate's dialog is shown by default, so we must assume the user will see it => we need a way to prevent showing the dialog twice. You are right that jobuidelegate is run before kio_file is involved. I was thinking of checking the permissions from jobuidelegate: can you check if QFileInfo would do the trick? (e.g. QFileInfo::isWritable()?)

I hope you mean the warning dialog because if the user is not authorised then there will be two dialogs.

I was thinking of checking the permissions from jobuidelegate: can you check if QFileInfo would do the trick?

Checking permissions beforehand and then showing the dangerous warning is definitely a good idea. Now QFileInfo::isWritable will work only when deleteJob is started inside a write-protected location. Suppose if its a regular folder inside some regular location but containing a write-protected subfolder then the check's result will be useless.
One thing we can do is stat the contents and as soon we find a write-protected location we can abort the stat'ing and show our special warning.

I don't see the advantage of using a single kauth action. This way you are generating only one "generic" polkit action that you can either enable or disable. Instead if we used one polkit action per operation (del, rmdir, etc.) we could allow a more fine-grained control. For example, a sysadmin could decide that the users can create files in write protected locations, but they cannot delete existing files.

Some operation may use more than one polkit action like Delete. If we use one polkit action per operation then we will have to provide credentials for every polkit action that operation might use. Though we can add necessary code for this but it will only complicate the matter.

we could allow a more fine-grained control

Does this involve editing the policy file directly or just writing a config file? In later case we can provide control over execution of actions within the ioslave. 'execWithRoot' can perform a check prior to execution of the polkit action.

Some operation may use more than one polkit action like Delete. If we use one polkit action per operation then we will have to provide credentials for every polkit action that operation might use.

But only if the actions are different, right? Otherwise we should be fine within the 5 minutes threshold after the first authentication. Can you show some concrete examples where this issue would happen?

Does this involve editing the policy file directly or just writing a config file?

Have a look at polkit rules. For example, one could write a rule that says "org.kde.kio.file.copy is allowed, org.kde.kio.file.del is not." and it would be very cool if we could actually support this.

But only if the actions are different, right? Otherwise we should be fine within the 5 minutes threshold after the first authentication. Can you show some concrete examples where this issue would happen?

Delteing a non-empty directory
In this operation 'del' and 'rmdir' actions are used. In this case we will have to authenticate for both the actions. And after the first authentication, doing a similar operation will show the warning dialog twice.

Copy operation can also be considered.
This operation will consist of six actions which are file_open, sendfile, read, write, file_close, chown. Although all the actions will not be used at once usage of atleast two can be expected. Here also we will have to authenticate ouself atleast twice just for copying a single file which is very annoying.

it would be very cool if we could actually support this.

Indeed its very cool and useful,but still, we will have all these problems.

One possible solution can be using the annotate tag. Polkit doc mentions its used for annotating an action with a key/value pair. If the value is org.freedesktop.policykit.imply then the subject if authorized for an action with this annotation is also authorized for any action specified by the annotation.
So what I did is added this line <annotate key="org.freedesktop.policykit.imply">org.kde.kio.file.rmdir</annotate> after the defaults tag of del action. After this when I tried deleting a non-empty folder I only got one auth dialog. One more thing I noticed is, with this line in place, when I deleted a single file deleting an empty folder afterwards only showed one warning even though the action was different and was executing for this first time.

I'm still not sure about this so I might be completely wrong as well. If you have any knowledge of this please let me know if I got everything or anything right. And if what I have mentioned is indeed correct then we will have to add the support for annotate tag to kauth policy generator.

The imply annotation sounds very promising, might be exactly what we are looking for. Unfortunately I don't know more details about it, at the moment.

Another solution (possibly simpler) could be a middle ground between "only one generic action" and "one action per operation". For example, we could have a org.kde.kio.file.delete action that handles everything about deletion: single file, empty folder, non-empty folder, etc.

Merging actions sounds good. I have updated the patch as per your suggestion. We can merge some other action as well, like,
file_open, sendfile, read, write, file_close --> to copy
symlink, put, mkdir ---> to create_something
chown, chmod ---> to change_something or perhaps we can leave them
what do you say?

Another solution (possibly simpler) could be a middle ground between "only one generic action" and "one action per operation".

It is only a part of solution. Say, if a user wants to perform a privileged copy after a privileged delete then he must be shown a warning instead of authentication dialog no? For this to happen we need some way to authorize the copy action without any interaction as soon as delete is authorized and I now strongly believe imply annotation is what we need.
Maybe its meant for this very purpose only. The use case mentioned in the doc - A typical use of this annotation is when defining an UI shell with a single lock button that should unlock multiple actions from distinct mechanisms.

Right, the two things are not mutually exclusive. We could define meta actions like "delete" or "create" but also use the imply annotation if there is a specific need for it. The latest diff looks good to me now, please update the proposal accordingly :)