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

David Faure faure at kde.org
Sun Mar 12 15:49:18 UTC 2017 

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




src/ioslaves/file/CMakeLists.txt (line 24)
<https://git.reviewboard.kde.org/r/129983/#comment68473>

    move it to a separate line, since kauth is unix only:
    
    if (UNIX)
        target_link_libraries(kio_file KF5::KAuth)
    endif()
    
    It's additive, so no problem to call it twice.



src/ioslaves/file/CMakeLists.txt (line 37)
<https://git.reviewboard.kde.org/r/129983/#comment68472>

    if (UNIX)
       add_subdirectory(kauth)
    endif()
    
    Apparently kauth doesn't exist on Windows.



src/ioslaves/file/file.h (line 107)
<https://git.reviewboard.kde.org/r/129983/#comment68457>

    keep all member variables together -> move down with the others, and call it mIsRoot for consistency.



src/ioslaves/file/file.cpp (line 1379)
<https://git.reviewboard.kde.org/r/129983/#comment68459>

    move declaration to where it's first used, this is C++, not C.



src/ioslaves/file/file.cpp (line 1381)
<https://git.reviewboard.kde.org/r/129983/#comment68460>

    use .insert() rather than [], to avoid the creation of a temporary (details in Effective C++ book)



src/ioslaves/file/file.cpp (line 1387)
<https://git.reviewboard.kde.org/r/129983/#comment68458>

    remove trailing whitespace (repeats)



src/ioslaves/file/file.cpp (line 1391)
<https://git.reviewboard.kde.org/r/129983/#comment68461>

    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 ;)



src/ioslaves/file/file.cpp (line 1419)
<https://git.reviewboard.kde.org/r/129983/#comment68462>

    1) this method should be marked const
    2) would it be possible to pass the enum type instead of int here? (then you can remove the "default:" clause)



src/ioslaves/file/file.cpp (line 1425)
<https://git.reviewboard.kde.org/r/129983/#comment68464>

    Use i18n around strings that will be shown to the user, so that they can get translated.



src/ioslaves/file/file.cpp (line 1435)
<https://git.reviewboard.kde.org/r/129983/#comment68463>

    not useful, it is a default-constructed QString already; remove this line.



src/ioslaves/file/file.cpp (line 1443)
<https://git.reviewboard.kde.org/r/129983/#comment68466>

    i18n around each translatable string



src/ioslaves/file/file.cpp (line 1444)
<https://git.reviewboard.kde.org/r/129983/#comment68465>

    "? true : false" is as useful as adding 0 to a number or multiplying a number by 1 ;) Remove.



src/ioslaves/file/kauth/helper.h (line 1)
<https://git.reviewboard.kde.org/r/129983/#comment68467>

    add copyright header (with you as copyright holder, and with a LGPL v2+ license, or even better, v2+v3+e.V.)



src/ioslaves/file/kauth/helper.cpp (line 1)
<https://git.reviewboard.kde.org/r/129983/#comment68468>

    same comment about missing copyright header



src/ioslaves/file/kauth/helper.cpp (line 14)
<https://git.reviewboard.kde.org/r/129983/#comment68474>

    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.



src/ioslaves/file/kauth/helper.cpp (line 25)
<https://git.reviewboard.kde.org/r/129983/#comment68471>

    use .constData() instead of .data() to avoid a detach



src/ioslaves/file/kauth/helper.cpp (line 26)
<https://git.reviewboard.kde.org/r/129983/#comment68469>

    This statement does nothing, the method would return anyway immediately afterwards. Is there missing error handling here? Shouldn't this return a bool?



src/ioslaves/file/kauth/helper.cpp (line 36)
<https://git.reviewboard.kde.org/r/129983/#comment68470>

    same as above


- David Faure


On March 9, 2017, 3:57 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 9, 2017, 3:57 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/20170312/92a2cf03/attachment-0001.html>

More information about the Kde-frameworks-devel mailing list