> On July 18, 2016, 11:30 p.m., David Faure wrote: > > 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.
It needs more work, because the test in itself is still basically a russian roulette. My patch merely tries to break the fingers of the players before their turn's up. I find it highly scary/nightmare inducing that there is a testcase that has as 'failure' a destroyed operating system. This cannot meet any quality standards I can think of. And I really find it quite scary that you use the "don't run as root" excuse. Yes I grant you, that running tests with privileges may be wrong, but I enabled them in our package builder to give them a go -- and there you go... However, if running tests as root is wrong, trying to rm /etc or /boot probably contradicts some Geneva convention. I think the proper way would be to make the test to only touch files it itself creates & chowns. Or the test could only try to remember a hardcoded `/tmp/kio_test/file` and `/tmp/kio_test/dir` that have to be created before running the test by the developer for these tests. But it should _never_ opt to _well look at that nice system config file/dir, that is certainly owned by root, let's try to remove that_ . What do you think would it be possible to reimplement the tests in that way? Or would that break the tests? [Please note, this is not an attack on you personally, but on the really scary stuff the tests do]. - Tobias ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128477/#review97577 ----------------------------------------------------------- On July 18, 2016, 7: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, 7: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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
