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

>
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
>
> > +# Runs a scriplet passed through stdin under sudo.
> > +run_with_sudo () {
> > +     local ret
> > +     local SH=${1-"$TEST_SHELL_PATH"}
> > +     local RUN="$HOME/$$.sh"
> > +     {
> > +             echo "#!$SH"
> > +             echo "set -e"
> > +             echo ". \"$HOME/env\""
> > +             cat
> > +     } >"$RUN" &&
> > +     chmod +x "$RUN" &&
> > +     sudo "$SH" -c "\"$RUN\""
> > +     ret=$?
> > +     rm -f "$RUN"
> > +     return $ret
> > +}
>
> I wonder if write_script can be used for better readability.  It is
> especially true as I am going to suggest ripping out $HOME/env stuff
> that is not absolutely needed (and its support with this patch looks
> inadequate when we do need one).
>
>         local RUN=$HOME/$$.sh &&
>         write_script "$RUN" "$TEST_SHELL_PATH" &&
>         sudo "$RUN"
>
> or something?

> +# Makes all variables passed as parameters available to the scriplet that
> > +# run under sudo with run_with_sudo
> > +export_to_sudo () {
> > +     while test -n "$1"
> > +     do
> > +             local v
> > +             eval v="\$$1"
> > +             echo "$1=$v" >>"$HOME/env"
> > +             shift
> > +     done
> > +}
>
> Two potential issues:
>
>  - This forces the caller to list _all_ the relevant environment
>    variables that would ever matter, which would not be feasible and
>    would not be maintainable.  For example, by forgetting to export
>    GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS, "git" commands run in the
>    sudo tests change their behaviour.  Whoever writing a new test
>    need to see what obscure GIT_TEST_* thing may affect the test
>    they want to write, and whoever enhancing the test framework to
>    add new GIT_TEST_* knob need to pay attention to the users of
>    export_to_sudo if their new knob need to be exported.
>
>  - I think the assignment to $v under eval is correct, but I am not
>    sure the string accumulated in the $HOME/env file is safe to
>    eval.  We can pass TEST_DIRECTORY via this mechanism, where the
>    value deliberately has a whitespace in it, but if the leading
>    path to our source directory had a single-quote in it, it
>    probably would not work well.  Of course, any variable that has
>    LF in its value would not work without proper quoting.
>
> I think both are not impossible but are hard to do right.  Because I
> do not see anything that absolutely needs the $HOME/env mechanism to
> work in the rest of the tests in this patch, I am inclined to say
> that I'd prefer keeping things simple and only make sure we use the
> right $SHELL to run our script (which write_script may help).

> > +test_expect_success SUDO 'cannot access if owned by root' '
> > +     (
> > +             cd root/p &&
> > +             test_must_fail git status
> > +     )
> > +'
>
> OK, but strictly speaking, we do not need the SUDO prerequisite for
> this one.  It still need it for the test directories prepared in
> previous steps anyway, so perhaps we want one check upfront, just
> like we do for NOT_ROOT?
>
>         if ! test_have_prereq SUDO
>         then
>                 skip_all="You do not seem to have a working 'sudo'"
>                 test_done
>         fi

> > +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
> > +     )
> > +'
>
> OK.  So we cannot by default access root-owned repository by
> default, which is OK.  I wonder what happens if we did "sudo sudo
> git status".  Perhaps the inner sudo will notice that SUDO_UID is
> set in its environment and does not update it to 0?

>       ... goes and checks ...
>         $ sudo sudo sh -c 'echo $SUDO_UID; whoami'
>         0
>         root
>
> So that gives us another workaround, I guess, which might be even
> simpler.

> > +test_expect_success SUDO 'can access using a workaround' '
> > +     # 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
> > +             END
> > +     ) &&
> > +     # discard SUDO_UID
> > +     (
> > +             cd root/p &&
> > +             run_with_sudo <<-END
> > +                     unset SUDO_UID
> > +                     git status
> > +             END
> > +     )
>
>         # double sudo
>         (
>                 cd root/p &&
>                 sudo sudo git status
>         )
>
> I do not know if it is worth adding this third workaround.