Review Request 129983: [RFC] PoC patch for polkit support in kio.

Tue Mar 14 14:49:00 UTC 2017

> On March 12, 2017, 3:49 p.m., David Faure wrote:
> > src/ioslaves/file/kauth/helper.cpp, line 14
> > <https://git.reviewboard.kde.org/r/129983/diff/3/?file=492381#file492381line14>
> >
> >     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 used invokeMethod for compactness but an if-else construct is indeed simpler

> On March 12, 2017, 3:49 p.m., David Faure wrote:
> > src/ioslaves/file/kauth/helper.cpp, line 26
> > <https://git.reviewboard.kde.org/r/129983/diff/3/?file=492381#file492381line26>
> >
> >     This statement does nothing, the method would return anyway immediately afterwards. Is there missing error handling here? Shouldn't this return a bool?

Error handling was supposed to be there but somehow I messed up.

> On March 12, 2017, 3:49 p.m., David Faure wrote:
> > src/ioslaves/file/file.cpp, line 1391
> > <https://git.reviewboard.kde.org/r/129983/diff/3/?file=492376#file492376line1391>
> >
> >     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.

> On March 12, 2017, 3:49 p.m., David Faure wrote:
> > src/ioslaves/file/CMakeLists.txt, line 37
> > <https://git.reviewboard.kde.org/r/129983/diff/3/?file=492374#file492374line37>
> >
> >     if (UNIX)
> >        add_subdirectory(kauth)
> >     endif()
> >     
> >     Apparently kauth doesn't exist on Windows.

Shall I move all the kauth specific code to file_unix ?

- Chinmoy Ranjan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129983/#review102823
-----------------------------------------------------------

On March 14, 2017, 2:48 p.m., Chinmoy Ranjan Pradhan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129983/
> -----------------------------------------------------------
> 
> (Updated March 14, 2017, 2:48 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Elvis Angelaccio.
> 
> 
> Repository: kio
> 
> 
> 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;
> 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.
> 
> 
> Diffs
> -----
> 
>   src/ioslaves/file/CMakeLists.txt b9132ce 
>   src/ioslaves/file/file.h 109ea80 
>   src/ioslaves/file/file.cpp eaf6c88 
>   src/ioslaves/file/file_unix.cpp 82eb11a 
>   src/ioslaves/file/kauth/CMakeLists.txt PRE-CREATION 
>   src/ioslaves/file/kauth/file.actions PRE-CREATION 
>   src/ioslaves/file/kauth/helper.h PRE-CREATION 
>   src/ioslaves/file/kauth/helper.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129983/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> warning dialog
>   https://git.reviewboard.kde.org/media/uploaded/files/2017/03/09/d42570e8-aedf-4c02-801e-362a68755c2c__polkit_integration.png
> 
> 
> Thanks,
> 
> Chinmoy Ranjan Pradhan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170314/22380788/attachment-0001.html>