patch, 5 partstweak t1304 ACL tests so they work on Solaris

The operaton of setfacl and the interaction of mode bits with ACL seem to differ on Solaris and Linux. The current t1304 tests fail on Solaris 7 and 10.

The following patch series seems to fix the tests on Solaris 10. Solaris 7 still fails t1304.3 for some unknown reason. I used the script below to do some testing.

On Linux it prints out:

  Initialized empty Git repository in /var/tmp/test_acl2/.git/
  [master (root-commit) 22176cb] test commit
   1 files changed, 1 insertions(+), 0 deletions(-)
   create mode 100644 foo.txt
  -r--r-----+ 1 casey NRL7240 29 Mar 15 11:47 .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec
  # file: .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx	#effective:r--
  group::---
  mask::r--
  other::---

  -r--r-----+ 1 casey NRL7240 225 Mar 15 11:47 .git/objects/pack/pack-0611d0bcfb48f3db9f0d51334a035289272dbb2c.pack
  # file: .git/objects/pack/pack-0611d0bcfb48f3db9f0d51334a035289272dbb2c.pack
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx	#effective:r--
  group::---
  mask::r--
  other::---

On Solaris 10 it prints out:

  Initialized empty Git repository in /var/tmp/test_acl2/.git/
  [master (root-commit) 5e4c0c6] test commit
   1 files changed, 1 insertions(+), 0 deletions(-)
   create mode 100644 foo.txt
  -r--------+  1 casey    NRL7240       29 Mar 15 11:49 .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec

  # file: .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx		#effective:r--
  group::---		#effective:---
  mask:r--
  other:---
  -r--------+  1 casey    NRL7240      225 Mar 15 11:49 .git/objects/pack/pack-fedf57238f710f6b713019fe02fddab7a63702d0.pack

  # file: .git/objects/pack/pack-fedf57238f710f6b713019fe02fddab7a63702d0.pack
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx		#effective:r--
  group::---		#effective:---
  mask:r--
  other:---

On Solaris 7 it prints out:

  Initialized empty Git repository in /var/tmp/test_acl2/.git/
  [master (root-commit) ca62c6f] test commit
   1 files changed, 1 insertions(+), 0 deletions(-)
   create mode 100644 foo.txt
  -r--------+  1 casey    NRL7240       29 Mar 15 10:51 .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec

  # file: .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx		#effective:r--
  group::---		#effective:---
  mask:r--
  other:---
  -r--------+  1 casey    NRL7240      225 Mar 15 10:51 .git/objects/pack/pack-dc814256703ad9c01e6dd1080c7151f664c6382a.pack

  # file: .git/objects/pack/pack-dc814256703ad9c01e6dd1080c7151f664c6382a.pack
  # owner: casey
  # group: NRL7240
  user::r--
  user:display:rwx		#effective:---
  group::---		#effective:---
  mask:---
  other:---

So, on Solaris 7, the pack file incorrectly has effective permissions of '---' for the 'display' user. For some reason the mask ACL was not inherited from the parent directory.

When the patch series is applied on top of 7aba6185, which introduced these tests, Linux still correctly fails t1304.3, but t1304.2 now passes. This is ok since t1304.2 was only failing because git gave object files more permissive file permissions than what were requested, not because the ACL's were clobbered. Of course, when applied on top of master, all tests pass on Linux, and now on Solaris 10.

--->8--- test_acl.sh --->8--- #!/bin/sh

alt_user=display

testcase=${1:-'1'}

case "$testcase" in
   1) test_dir=/var/tmp/test_acl1 ;;
   2) test_dir=/var/tmp/test_acl2
      GIT_EXEC_PATH=`pwd`
      PATH=`pwd`:$PATH
      GITPERLLIB=`pwd`/perl/blib/lib
      export GIT_EXEC_PATH PATH GITPERLLIB
      ;;
   *)
      echo 1>&2 "Usage: $0 [1|2]"
      exit 1
      ;;
esac

umask 077 &&
  rm -rf "$test_dir" && mkdir "$test_dir" && cd "$test_dir" &&
  setfacl -m d:u::rwx,d:g::---,d:o:---,d:m:rwx . &&
  setfacl -m d:u:$alt_user:rwx . &&
  setfacl -m u:$alt_user:rwx,m:rwx . &&
  git init &&
  echo 'this is a test' > foo.txt &&
  git add foo.txt &&
  git commit -m 'test commit' &&
  ls -l .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec &&
  getfacl .git/objects/90/bfcb510602aa11ae53a42dcec18ea39fbd8cec &&
  git gc &&
  ls -l .git/objects/pack/*.pack &&
  getfacl .git/objects/pack/*.pack
--->8---

Brandon Casey (5):
  t/t1304: avoid -d option to setfacl
  t/t1304: set the Default ACL base entries
  t/t1304: use 'test -r' to test readability rather than looking at
    mode bits
  t/t1304: set the mask ACL that is checked in check_perms_and_acl
  t/t1304: make a second colon optional in the mask ACL check

 t/t1304-default-acl.sh |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

From: Brandon Casey <drafnel@gmail.com>

Some platforms (Solaris) have a setfacl whose -d switch works differently than the one on Linux. On Linux, it causes all operations to be applied to the Default ACL. There is a notation for operating on the Default ACL:

   [d[efault]:] [u[ser]:]uid [:perms]

so use it instead of the -d switch.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t1304-default-acl.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

From: Brandon Casey <drafnel@gmail.com>

According to the Linux setfacl man page, in order for an ACL to be valid, the following rules must be satisfied:

   * Whenever an ACL contains any Default ACL entries, the three Default
     ACL base entries (default owner, default group, and default others)
     must also exist.

   * Whenever a Default ACL contains named user entries or named group
     objects, it must also contain a default effective rights mask.

Some implementations of setfacl (Linux) do this automatically when necessary, some (Solaris) do not. Solaris's setfacl croaks when trying to create a default user ACL if the above rules are not satisfied. So, create them before modifying the default user ACL's.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t1304-default-acl.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

From: Brandon Casey <drafnel@gmail.com>

This test was using the group read permission bit as an indicator of the default ACL mask. This behavior is valid on Linux but not on other platforms like Solaris. So, rather than looking at mode bits, just test readability for the user. This, along with the checks for the existence of the ACL's that were set on the parent directories, should be enough.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t1304-default-acl.sh |   15 +--------------
 1 files changed, 1 insertions(+), 14 deletions(-)

Brandon Casey <casey@nrlssc.navy.mil> writes:

Maybe it makes sense to ask for -r--?-----* instead, to make sure we don't give the read bit to anyone. But that's not really important since we're protected by the directory's permission anyway.

On 03/15/2010 12:33 PM, Matthieu Moy wrote:

I'm a little hesitant to add a test on the output from 'ls', since Solaris and Linux produce different output in the presence of ACL's.

-brandon

From: Brandon Casey <drafnel@gmail.com>

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t1304-default-acl.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Brandon Casey <casey@nrlssc.navy.mil> writes:

The patch sounds right, but I don't understand the commit message. You set m:rwx, and check_perms_and_acl expects mask::r--, so it's not exactly what check_perms_and_acl checks.

My understanding is that you set the mask here to enforce the validity of the ACL, but then you may want to just squash this into [PATCH 2/5].

But again, that's bikeshedding, the patch serie looks right to me. Sorry for not making the test right myself ;-).

On 03/15/2010 12:37 PM, Matthieu Moy wrote:

Ah, yeah, it does sound like I'm saying that check_perms_and_acl is checking for the particular mask that I'm setting. I really meant it to read more like: since check_perms_and_acl is checking the 'mask ACL', it should be set appropriately.

I think the ACL is valid according to the rules stated in the Linux man page, but depending on the previously existing mask ACL on the directories, the other ACL's that were set may or may not have any effect. I think on Linux, the setfacl command updates the effective rights mask when other ACL entries are modified. I don't think this happens on Solaris.

If I do this:

  $ cd /var/tmp &&
    mkdir test &&
    setfacl -m d:u::rwx,d:g::---,d:o:---,d:m:rwx test &&
    setfacl -m d:u:guest:rwx test &&
    setfacl -m u:guest:rwx test &&
    getfacl test

On Solaris I get:

  # file: test
  # owner: XXX
  # group: XXX
  user::rwx
  user:guest:rwx                #effective:---
  group::---              #effective:---
  mask:---
  other:---
  default:user::rwx
  default:user:guest:rwx
  default:group::---
  default:mask:rwx
  default:other:---

and on Linux I get:

# file: test # owner: XXX # group: XXX user::rwx user:guest:rwx group::--- mask::rwx other::--- default:user::rwx default:user:guest:rwx default:group::--- default:mask::rwx default:other::---

Notice how the mask entry is different. On Solaris you get --- and user 'guest' effectively has no permissions, while on Linux it has full rwx. So for the test we should set the mask explicitly.

-brandon

From: Brandon Casey <drafnel@gmail.com>

Some implementations of setfacl do not recalculate the effective rights mask when the ACL is modified. So, set the effective rights mask explicitly to ensure that the ACL's that are set on the directories will have effect.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

Here's a new patch with an improved commit message (hopefully).

-brandon

 t/t1304-default-acl.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Brandon Casey <casey@nrlssc.navy.mil> writes:

Yes, I do like it better.

From: Brandon Casey <drafnel@gmail.com>

Solaris only uses one colon in the listing of the ACL mask, Linux uses two, so substitute egrep for grep and make the second colon optional.

The -q option for Solaris 7's /usr/xpg4/bin/egrep does not appear to be implemented, so redirect output to /dev/null.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t1304-default-acl.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)