Re: [PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- May 3, 2022, 14:12 UTC
- Message-ID
- <af4dd56d-f301-9c3d-2fc3-929a24596e82@gmail.com>
- In-Reply-To
- <20220503065442.95699-4-carenas@gmail.com>
Hi Carlo
On 03/05/2022 07:54, Carlo Marcelo Arenas Belón wrote:
Show 34 quoted lines
> 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"'
Show 22 quoted lines
> + 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.
Show 12 quoted lines
> + 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?
Show 31 quoted lines
> +# 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.
Show 16 quoted lines
> +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
Show 16 quoted lines
> + 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 > '