From: Phillip Wood Date: Tue, 03 May 2022 14:12:30 GMT Subject: Re: [PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo Message-ID: In-Reply-To: <20220503065442.95699-4-carenas@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 > Signed-off-by: Carlo Marcelo Arenas Belón > --- > 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 > '