Review Request 128477: Do not delete system relevant files in tests (if we might succeed)

[David Faure]

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


Ship it!




Wow, it never occured to me that someone might run this test as root. I thought it was well known that development should not be done as root ;)
But I can see how it might happen when creating packages with unittests enabled or something.

The patch looks fine to me, not sure why you say "it probably still needs some more work" ?

Unfortunately I see no other way to test files owned by other users. But of course as long as this is tested once, the other tests could change permissions on a temp file to get into "missing permissions" error cases.

- David Faure


On July 18, 2016, 5:10 p.m., Tobias Berner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128477/
> -----------------------------------------------------------
> 
> (Updated July 18, 2016, 5:10 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> Some tests for kio try to move system relevant files&paths with the blind assumption that 
> the permissions to touch these files is not present. 
> The files are 
> - /etc/passwd
> - /etc/cups
> - /etc
> - /boot
> [sic!].
> 
> 
> Check that the process does not actually have the rights to touch system
> relevant files when running the
> - TestTrash::trashDirectoryOwnedByRoot
> - TestTrash::trashFileOwnedByRoot
> - JobTest::moveFileNoPermissions
> - JobTest::moveDirectoryNoPermissions
> tests -- and bail out of them if so.
> 
> 
> This patch probably still needs some more work [maybe I also missed another naughty test?],
> and I welcome every kind of input on it (apart from the straw man *don't run tests as root* ;) ).
> 
> 
> Diffs
> -----
> 
>   autotests/jobtest.cpp 579c507 
>   src/ioslaves/trash/tests/testtrash.cpp c71df13 
> 
> Diff: https://git.reviewboard.kde.org/r/128477/diff/
> 
> 
> Testing
> -------
> 
> Without patch:
> - enjoying two hours of restoring a system without /etc & /boot
> 
> With patch:
> - grep 'must not' Testing/Temporary/LastTest.log.tmp
>      SKIP   : TestTrash::trashFileOwnedByRoot() Test must not be run by root.
>      SKIP   : TestTrash::trashDirectoryOwnedByRoot() Test must not be run by root.
>      SKIP   : JobTest::moveFileNoPermissions() Test must not be run by root.
>      SKIP   : JobTest::moveDirectoryNoPermissions() Test must not be run by root.
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160718/3491b7fc/attachment.html>