Re: [PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Carlo

On 03/05/2022 07:54, Carlo Marcelo Arenas Belón wrote:
> Add a support library that provides one function that can be used
> to run a "scriplet" of commands through sudo and that has an
> optional parameter (currently unused) to indicate which shell to
> use to do so.
> 
> Add additional negative tests as suggested by Junio and that use
> new workspace that is owned by root.
> 
> Note that in order to be able to call `test_must_fail sudo git status`
> or an equivalent, test_must_fail will need to be enhanced or be able
> to run under sudo, so fixing that has been punted, since the only
> protection it affords is for `git status` not crashing, and that is
> covered already by other tests.
> 
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>   t/lib-sudo.sh                  | 13 +++++++
>   t/t0034-root-safe-directory.sh | 70 +++++++++++++++++++++++++++++++++-
>   2 files changed, 81 insertions(+), 2 deletions(-)
>   create mode 100644 t/lib-sudo.sh
> 
> diff --git a/t/lib-sudo.sh b/t/lib-sudo.sh
> new file mode 100644
> index 00000000000..9ebb30fc82b
> --- /dev/null
> +++ b/t/lib-sudo.sh
> @@ -0,0 +1,13 @@
> +# Helpers for running git commands under sudo.
> +
> +# Runs a scriplet passed through stdin under sudo.
> +run_with_sudo () {
> +	local ret
> +	local SH=${1-"$TEST_SHELL_PATH"}

What use do you envisage for this? It would be simpler just to use 
$TEST_SHELL_PATH directly below

> +	local RUN="$HOME/$$.sh"

Can we used a fixed name for the script? That would make things simpler 
especially debugging as one would know what file to look for. Also using 
$TEST_DIRECTORY rather than $HOME would make it clear where the file 
ends up.

> +	write_script "$RUN" "$SH"
> +	sudo "$SH" -c "\"$RUN\""

I think using write_script means we can just do 'sudo "$RUN"'

> +	ret=$?
> +	rm -f "$RUN" > +	return $ret
> +}
> diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh
> index dd659aed4e1..a68e1d7602b 100755
> --- a/t/t0034-root-safe-directory.sh
> +++ b/t/t0034-root-safe-directory.sh
> @@ -3,6 +3,7 @@
>   test_description='verify safe.directory checks while running as root'
>   
>   . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-sudo.sh
>   
>   if [ "$IKNOWWHATIAMDOING" != "YES" ]
>   then
> @@ -10,6 +11,12 @@ then
>   	test_done
>   fi
>   
> +if ! test_have_prereq NOT_ROOT
> +then
> +	skip_all="No, you don't; these tests can't run as root"

I think the message would be friendlier without the "No, you don't" and 
just said that the tests cannot be run as root.

> +	test_done
> +fi
> +
>   # this prerequisite should be added to all the tests, it not only prevents
>   # the test from failing but also warms up any authentication cache sudo
>   # might need to avoid asking for a password
> @@ -40,8 +47,67 @@ test_expect_success SUDO 'sudo git status as original owner' '
>   	)
>   '
>   
> -# this MUST be always the last test, if used more than once, the next
> -# test should do a full setup again.

Why is the comment being changed? If you want the shorter version at the 
end of this patch can't we just use that wording in patch 1?


> +# this destroys the test environment used above
> +test_expect_success SUDO 'cleanup regression' '
> +	sudo rm -rf root
> +'
> +
> +if ! test_have_prereq SUDO
> +then
> +	skip_all="You need sudo to root for all remaining tests"
> +	test_done
> +fi
> +
> +test_expect_success SUDO 'setup root owned repository' '
> +	sudo mkdir -p root/p &&
> +	sudo git init root/p
> +'
> +
> +test_expect_success 'cannot access if owned by root' '
> +	(
> +		cd root/p &&
> +		test_must_fail git status
> +	)
> +'
> +
> +test_expect_success SUDO 'cannot access with sudo' '
> +	(
> +		# TODO: test_must_fail needs additional functionality
> +		# 6a67c759489 blocks its use with sudo
> +		cd root/p &&
> +		! sudo git status
> +	)
> +'

I think Junio suggested that this should work and showed it was simple 
to make it work. It seems funny that if sudo is started as root it does 
not work.

> +test_expect_success SUDO 'can access using a workaround' '
> +	# run sudo twice
> +	(
> +		cd root/p &&
> +		run_with_sudo <<-END
> +			sudo git status
> +		END
> +	) &&
> +	# provide explicit GIT_DIR
> +	(
> +		cd root/p &&
> +		run_with_sudo <<-END
> +			GIT_DIR=.git &&
> +			GIT_WORK_TREE=. &&
> +			export GIT_DIR GIT_WORK_TREE &&
> +			git status

I'm confused by this. Does this mean we don't do the ownership checks if 
GIT_DIR and or GIT_WORK_TREE are set in the environment?


Thanks for working on this

Best Wishes

Phillip


> +		END
> +	) &&
> +	# discard SUDO_UID
> +	(
> +		cd root/p &&
> +		run_with_sudo <<-END
> +			unset SUDO_UID &&
> +			git status
> +		END
> +	)
> +'
> +
> +# this MUST be always the last test
>   test_expect_success SUDO 'cleanup' '
>   	sudo rm -rf root
>   '