modules: set permissions based on package configuration (#11337)

Previously, module files were not set with the same permissions as the package installation.  For world-readable packages, this would not cause a problem.  For group readable packages, it does:

```
packages:
  mypackage:
    permissions:
      group: mygroup
      read: group
      write: group
```

In this case, the modulefile is unreadable by members of the group other than the one who installed it.  Add logic to the modulefile writers to set the permissions based on the configuration in `packages.yaml`

5 files changed, 99 insertions, 52 deletions

diff --git a/lib/spack/spack/hooks/permissions_setters.py b/lib/spack/spack/hooks/permissions_setters.py
index 9ada697ad8..10d75906e2 100644
--- a/lib/spack/spack/hooks/permissions_setters.py
+++ b/lib/spack/spack/hooks/permissions_setters.py
@@ -4,54 +4,18 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 import os
-import stat
-
-from llnl.util.filesystem import chmod_x, chgrp
-
-from spack.package_prefs import get_package_permissions, get_package_group
-from spack.package_prefs import get_package_dir_permissions
-from spack.error import SpackError
-
-
-def forall_files(path, fn, args, dir_args=None):
-    """Apply function to all files in directory, with file as first arg.
-
-    Does not apply to the root dir. Does not apply to links"""
-    # os.walk explicitly set not to follow links
-    for root, dirs, files in os.walk(path, followlinks=False):
-        for d in dirs:
-            if not os.path.islink(os.path.join(root, d)):
-                if dir_args:
-                    fn(os.path.join(root, d), *dir_args)
-                else:
-                    fn(os.path.join(root, d), *args)
-        for f in files:
-            if not os.path.islink(os.path.join(root, f)):
-                fn(os.path.join(root, f), *args)
-
-
-def chmod_real_entries(path, perms):
-    # Don't follow links so we don't change things outside the prefix
-    if not os.path.islink(path):
-        mode = os.stat(path).st_mode
-        perms |= mode & (stat.S_ISUID | stat.S_ISGID | stat.S_ISVTX)
-        if perms & stat.S_ISUID and perms & stat.S_IWGRP:
-            raise InvalidPermissionsError(
-                'Attempting to set suid with world writable')
-        chmod_x(path, perms)
+import spack.util.file_permissions as fp
 
 
 def post_install(spec):
     if not spec.external:
-        perms = get_package_permissions(spec)
-        dir_perms = get_package_dir_permissions(spec)
-        group = get_package_group(spec)
-
-        forall_files(spec.prefix, chmod_real_entries, [perms], [dir_perms])
-
-        if group:
-            forall_files(spec.prefix, chgrp, [group])
-
-
-class InvalidPermissionsError(SpackError):
-    """Error class for invalid permission setters"""
+        fp.set_permissions_by_spec(spec.prefix, spec)
+
+        # os.walk explicitly set not to follow links
+        for root, dirs, files in os.walk(spec.prefix, followlinks=False):
+            for d in dirs:
+                if not os.path.islink(os.path.join(root, d)):
+                    fp.set_permissions_by_spec(os.path.join(root, d), spec)
+            for f in files:
+                if not os.path.islink(os.path.join(root, f)):
+                    fp.set_permissions_by_spec(os.path.join(root, f), spec)
diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py
index 62df108b50..9185e6bfc9 100644
--- a/lib/spack/spack/modules/common.py
+++ b/lib/spack/spack/modules/common.py
@@ -47,6 +47,7 @@ import spack.util.path
 import spack.util.environment
 import spack.error
 import spack.util.spack_yaml as syaml
+import spack.util.file_permissions as fp
 
 #: config section for this file
 configuration = spack.config.get('modules')
@@ -750,6 +751,10 @@ class BaseModuleFileWriter(object):
         with open(self.layout.filename, 'w') as f:
             f.write(text)
 
+        # Set the file permissions of the module to match that of the package
+        if os.path.exists(self.layout.filename):
+            fp.set_permissions_by_spec(self.layout.filename, self.spec)
+
     def remove(self):
         """Deletes the module file."""
         mod_file = self.layout.filename
diff --git a/lib/spack/spack/test/modules/common.py b/lib/spack/spack/test/modules/common.py
index bb61100739..5c32c34840 100644
--- a/lib/spack/spack/test/modules/common.py
+++ b/lib/spack/spack/test/modules/common.py
@@ -3,8 +3,12 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-
+import pytest
+import os
+import stat
+import spack.spec
 import spack.modules.common
+import spack.modules.tcl
 
 
 def test_update_dictionary_extending_list():
@@ -32,3 +36,37 @@ def test_update_dictionary_extending_list():
     assert len(target['foo']) == 4
     assert len(target['bar']) == 4
     assert target['baz'] == 'foobaz'
+
+
+@pytest.fixture()
+def mock_module_filename(monkeypatch, tmpdir):
+    filename = str(tmpdir.join('module'))
+    monkeypatch.setattr(spack.modules.common.BaseFileLayout,
+                        'filename',
+                        filename)
+
+    yield filename
+
+
+@pytest.fixture()
+def mock_package_perms(monkeypatch):
+    perms = stat.S_IRGRP | stat.S_IWGRP
+    monkeypatch.setattr(spack.package_prefs,
+                        'get_package_permissions',
+                        lambda spec: perms)
+
+    yield perms
+
+
+def test_modules_written_with_proper_permissions(mock_module_filename,
+                                                 mock_package_perms,
+                                                 mock_packages, config):
+    spec = spack.spec.Spec('mpileaks').concretized()
+
+    # The code tested is common to all module types, but has to be tested from
+    # one. TCL picked at random
+    generator = spack.modules.tcl.TclModulefileWriter(spec)
+    generator.write()
+
+    assert mock_package_perms & os.stat(
+        mock_module_filename).st_mode == mock_package_perms
diff --git a/lib/spack/spack/test/permissions.py b/lib/spack/spack/test/permissions.py
index 223b3e8319..b233c7a0fa 100644
--- a/lib/spack/spack/test/permissions.py
+++ b/lib/spack/spack/test/permissions.py
@@ -7,8 +7,8 @@ import os
 import pytest
 import stat
 
-from spack.hooks.permissions_setters import (
-    chmod_real_entries, InvalidPermissionsError
+from spack.util.file_permissions import (
+    set_permissions, InvalidPermissionsError
 )
 import llnl.util.filesystem as fs
 
@@ -20,7 +20,7 @@ def test_chmod_real_entries_ignores_suid_sgid(tmpdir):
     mode = os.stat(path).st_mode  # adds a high bit we aren't concerned with
 
     perms = stat.S_IRWXU
-    chmod_real_entries(path, perms)
+    set_permissions(path, perms)
 
     assert os.stat(path).st_mode == mode | perms & ~stat.S_IXUSR
 
@@ -32,4 +32,4 @@ def test_chmod_rejects_group_writable_suid(tmpdir):
 
     perms = stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO
     with pytest.raises(InvalidPermissionsError):
-        chmod_real_entries(path, perms)
+        set_permissions(path, perms)
diff --git a/lib/spack/spack/util/file_permissions.py b/lib/spack/spack/util/file_permissions.py
new file mode 100644
index 0000000000..c732aad0ab
--- /dev/null
+++ b/lib/spack/spack/util/file_permissions.py
@@ -0,0 +1,40 @@
+# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+import os
+import stat as st
+import llnl.util.filesystem as fs
+import spack.package_prefs as pp
+from spack.error import SpackError
+
+
+def set_permissions_by_spec(path, spec):
+    # Get permissions for spec
+    if os.path.isdir(path):
+        perms = pp.get_package_dir_permissions(spec)
+    else:
+        perms = pp.get_package_permissions(spec)
+    group = pp.get_package_group(spec)
+
+    set_permissions(path, perms, group)
+
+
+def set_permissions(path, perms, group=None):
+    # Preserve higher-order bits of file permissions
+    perms |= os.stat(path).st_mode & (st.S_ISUID | st.S_ISGID | st.S_ISVTX)
+
+    # Do not let users create world writable suid binaries
+    if perms & st.S_ISUID and perms & st.S_IWGRP:
+        raise InvalidPermissionsError(
+            "Attepting to set suid with world writable")
+
+    fs.chmod_x(path, perms)
+
+    if group:
+        fs.chgrp(path, group)
+
+
+class InvalidPermissionsError(SpackError):
+    """Error class for invalid permission setters"""