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@xxxxxxxxx>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx>
---
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
'
