Re: [PATCH v2 3/3] t: add tests for safe.directory when running with sudo

[Junio C Hamano <gitster@pobox.com>]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> If we do :
>
>   [0] login as regular user || sudo to root || login as root

Among these three, the last one is equivalent to "sudo and then
unset the environment", right?

As many installations no longer allow direct "login" as "root",
clarifying how users can get the behaviour for the third column
would help our documentation, and that is the reason behind the
question.  IOW, this is merely a simple yes-or-no question to
sanity-check our mutual understanding, and no need to get overly
defensive about being asked.

>   [1] % mkdir -p root/r
>   [2] % git init root/r
>   [3] % cd root/r && git status
>
>   step \ type | regular user | sudo to root | root
> --------------------------------------------------
>             1 |    work      |     work     | work
>   before    2 |    work      |     work     | work
>             3 |    work      |     work     | work
> ---------------------------------------------------
>             1 |    work      |     work     | work
>    after    2 |    work      |     work     | work
>             3 |    work      |     fail     | work

So the only difference is that in a repository created by a user who
did "sudo mkdir; sudo git init".  It used to be that the same user
can read the repository with "sudo git status" (because we did not
care about how we become 'root', we only saw the owner of the repo
and the current euid).  Now, such an access is no longer allowed.

And a workaround is to use the third-column behaviour, i.e. either
login as root (which is probably too cumbersome as a step in a
typical "make && make test && make install" sequence where at least
the last step need to be done as a privileged user) or use "sudo"
and drop the SUDO_UID environment, for which, the documentation was
added in this series.

But I do not see what relevance the above has to the argument you
were making, against "if you start these tests as 'root' (instead of
starting as an ordinary user), some tests may succeed but for a
wrong reason, and some tests may fail because they are not prepared
for it; it is wrong to mark only the latter with prerequisite and
not the former".  The change in the behaviour we see above is for
those who start as an ordinary user and uses "sudo" without dropping
SUDO_UID.  How is it relevant to allow those who start the test as
'root' (not an ordinary user) to try that?  Tests done under such
condition will see 'root' in euid, SUDO_UID, and st.st_uid, so there
is no way for them to detect any mismatch and behave differently,
so the transition from "it used to work" to "now it is forbidden"
is not even tested.

> and rejects the repository because it is NOT owned by that id (it was created
> by root anyway, even if there is no way for git to know that it was done
> at a different time and with a different session, and therefore the SUDO_UID
> variable it is honoring could be considered irrelevant in the current context.
>
> in the documentation patch (which I think would be better to squash with the
> fix) I explain what to do as a workaround, and I expect this use case to be
> less common than the currently broken one (so a net positive)

Both of these two paragraphs speak truth, but there is no relevance
to, and it does not justify, your "I disagree, and think that the
fact ... proves my point".

For example, this is the 'setup' step.

> +test_expect_success SUDO 'setup' '
> +	sudo rm -rf root &&
> +	mkdir -p root/r &&
> +	sudo chown root root &&
> +	(
> +		cd root/r &&
> +		git init
> +	)
> +'

If the test was started by an ordinary user, root/r is owned by the
user who is not 'root'.  If the test was started by 'root',
everything is owned by 'root'.  Either way, 'root' is owned by
'root'.  In such a repository, we see this test:

+test_expect_success SUDO 'sudo git status as original owner' '
+	(
+		cd root/r &&
+		git status &&
+		sudo git status
+	)
+'

The behaviour we are trying to ensure is that, even though root/r is
owned by non-root, accessing it with "git status" as the original
user and "git status" as root work, as long as if you used "sudo" in
the second "git status", so that "git status" can take SUDO_UID into
account.  The test is making sure that our "pay attention to
SUDO_UID" mechanism has not been broken by future changes.

If we start this test as 'root', we cannot test for that.  The setup
step made everything owned by 'root', and we go there as 'root' and
run "git status", which should succceed, but with "sudo git status",
even we broke SUDO_UID mechanism and compared euid with st.st_uid,
we'll allow an access.

So the test may succeed but it succeeds for a wrong reason even
after we break the mechanism added by this series.