Re: [PATCH v2 3/3] t: add tests for safe.directory when running with sudo

[Junio C Hamano <gitster@pobox.com>]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> 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
> +'

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