On April 28, 2022 12:55 PM, Junio C Hamano wrote:
>Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> writes:
>
>> +test_description='verify safe.directory checks while running as root'
>> +
>> +. ./test-lib.sh
>> +
>> +if [ "$IKNOWWHATIAMDOING" != "YES" ]; then
>
>Style.
>
> if test "$IKNOWWHATIAMDOING" != "YES"
> then
>
>> +is_root() {
>> + test -n "$1" && CMD="sudo -n"
>> + test $($CMD id -u) = $(id -u root)
>> +}
>
>Style.
>
> is_root () {
> ... body ..
>
>But more importantly, why do we need this in the first place?
>SANITY prerequisite checks if the user is running as root or non-root---can't we
>use it here?
>
>Or perhaps my reading is wrong? I assumed from its name that it was just "see if
>we are running as user 'root' and return 0 or non-zero to answer", but if it does
>more than that, like priming "sudo", then probably it is misnamed.
>
>> +test_lazy_prereq SUDO '
>> + is_root sudo &&
>> + ! sudo grep -E '^[^#].*secure_path' /etc/sudoers '
/etc/sudoers is not standard although usual. This path should come from a knob somewhere. We can't run this test on our x86 system anyway - no access to root or sudo even though it is installed. Also, /etc/sudoers is typically secured 0600 so the grep will fail even if is_root passes - and I'm worried about the portability of is_root, which is mostly Linux.
>OK.
>
>> +test_lazy_prereq ROOT '
>> + is_root
>> +'
>> +
>> +test_expect_success SUDO 'setup' '
>> + sudo rm -rf root &&
>> + mkdir -p root/r &&
>> + sudo chown root root &&
>> + (
>> + cd root/r &&
>> + git init
>> + )
>> +'
>
>We have a root-owned directory "root" with a subdirectory "r" owned by us. We
>want to be able to use our "root/r" directory as a repository. OK.
>
>The prerequisite allows this test to be started as root, but I do not quite see the
>point. It may pass when started as root, but it is not testing what this test is
>designed to check (i.e. an ordinary user who has repository at root/r can do things
>there).
>
>> +test_expect_success SUDO 'sudo git status as original owner' '
>> + (
>> + cd root/r &&
>> + git status &&
>> + sudo git status
>> + )
>> +'
>
>And the directory can be used by the user under "sudo", too. Good.
>
>The same "this is allowed to run as root, but why?" question applies. If this was
>started by 'root', root, root/r and root/r/.git all are owned by 'root' and we are
>checking if 'root'
>can run 'git status' as 'root' (or 'root' via sudo) there. Such a test may well pass, but
>it is not catching a future regression on the code you wrote for this series.
>
>> +test_expect_success SUDO 'setup root owned repository' '
>> + sudo mkdir -p root/p &&
>> + sudo git init root/p
>> +'
>
>Now we go on to create root owned repository at root/p
>
>> +test_expect_success SUDO,!ROOT 'can access if owned by root' '
>> + (
>> + cd root/p &&
>> + test_must_fail git status
>> + )
>> +'
>
>And as an ordinary user, we fail to access a repository that is owned by a wrong
>person (i.e. root). !ROOT (or SANITY) prereq should be there NOT because the
>test written here would fail if run by root, but because running it as root, even if
>passed, is totally pointless, because we are *not* testing "person A has a
>repository, person B cannot access it" combination.
>
>The other side of the same coin is that the lack of !ROOT (or
>SANITY) prereq in earlier tests I pointed out above misses the point of why we
>have prerequisite mechanism in the first place. It is not to mark a test that fails
>when the precondition is not met. It is to avoid running code that would NOT test
>what we want to test.
>
>The difference is that a test that passes for a wrong reason (e.g. we wanted to see
>of person A can access a repository of their own even when the user identity is
>tentatively switched to 'root'
>via 'sudo'---if person A is 'root', the access will be granted even if the special code
>to handle 'sudo' situation we have is broken) should also be excluded with
>prerequisite.
>
>> +test_expect_success SUDO,!ROOT 'can access with sudo' '
>> + # fail to access using sudo
>> + (
>> + # TODO: test_must_fail missing functionality
>
>Care to explain a bit in the log message or in this comment the reason why we do
>not use test_must_fail but use ! here? Are we over-eager to reject anything non
>"git" be fed, or something?
>
>> + cd root/p &&
>> + ! sudo git status
>> + )
>> +'
>
>The repository is owned by 'root', but because of the 'sudo'
>"support", you cannot access the repository with "sudo git".
>
>The test title needs updating. We expect that the repository cannot be accessed
>under sudo.
>
>> +test_expect_success SUDO 'can access with workaround' '
>
>"workarounds", I think.
>
>> + # provide explicit GIT_DIR
>> + (
>> + cd root/p &&
>> + sudo sh -c "
>> + GIT_DIR=.git GIT_WORK_TREE=. git status
>> + "
>> + ) &&
>> + # discard SUDO_UID
>> + (
>> + cd root/p &&
>> + sudo sh -c "
>> + unset SUDO_UID &&
>> + git status
>> + "
>> + )
>> +'
>
>Again, this lack !ROOT (or SANITY) because tests pass for a wrong reason.
>
>Overall, I like the simplicity and clarity of "do not start this test as 'root'" in the
>previous round much better.
>
>But other than that, the test coverage given by this patch looks quite sensible.
>
>Thanks.
>
>> +
>> +test_expect_success SUDO 'cleanup' '
>> + sudo rm -rf root
>> +'
>> +
>> +test_done
