Review Request: KAuth support in KDM kcontrol module

[Oswald Buddenhagen]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3631/#review5577
-----------------------------------------------------------


> removeDirTree() and installDirTree() don't use QDirIterator anymore (they are now implemented as recursive functions).
>
ohhh, epic fail on my side. i didn't realize that QDirIterator is already recursive. that certainly explains the delaying of the directory removals. :}
but the new code is nicer anyway. :)

> BTW, is this patch affected by the hard feature freeze coming tomorrow?
>
in theory probably. in practice you will have survived one of the most anal reviewers in the community, so nobody will dare to complain if i push it in a bit later. :D



/trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp
<http://reviewboard.kde.org/r/3631/#comment5259>

    it just occurred to me that you can leave out the return value checks here, as the rmdir below will also fail if any of the nested removals fail.
    



/trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp
<http://reviewboard.kde.org/r/3631/#comment5260>

    that's a very good point ... but the implementation is still insecure, because it is subject to symlink attacks. you need to check the permissions once the file is actually open (qfile should automatically use fstat() then). that means that qfile::copy is off-limits.
    
    dario: such a secure copy function should probably go into a helper tool class at some point.



/trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp
<http://reviewboard.kde.org/r/3631/#comment5261>

    this will need to use a manual file copy as well.


- Oswald


On 2010-05-10 18:33:56, Igor Krivenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3631/
> -----------------------------------------------------------
> 
> (Updated 2010-05-10 18:33:56)
> 
> 
> Review request for kdelibs, usability, Dario Freddi, Oswald Buddenhagen, and Frederik Gladhorn.
> 
> 
> Summary
> -------
> 
> This is a patch for KDM kcontrol module.
> It implements Kauth support for changing kdm settings and managing user images. 
> It still doesn't support installation/deletion of themes (including installation via knewstuff).
> As far as I know there were plans to port KNewStuff3 to KAuth, so a more consistent approach is
> to rely on the KNewStuff's integration when it's available.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/CMakeLists.txt 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/background.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/background.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kcmkdm_actions.actions PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-conv.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-conv.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-dlg.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-dlg.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-gen.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-gen.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-shut.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-shut.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-users.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-users.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/main.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/main.cpp 1125102 
> 
> Diff: http://reviewboard.kde.org/r/3631/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Saving KDM settings
>   http://reviewboard.kde.org/r/3631/s/360/
> Managing user images
>   http://reviewboard.kde.org/r/3631/s/361/
> 
> 
> Thanks,
> 
> Igor
> 
>