Allow read-only access to file cache (when needed) (#29693)

* Allow read-only access to file cache (when needed)
* Tweaked and added unit tests
* Skip test_cache_init_entry_fails for windows

2 files changed, 47 insertions, 2 deletions

diff --git a/lib/spack/spack/test/util/file_cache.py b/lib/spack/spack/test/util/file_cache.py
index 988e243835..106a411b69 100644
--- a/lib/spack/spack/test/util/file_cache.py
+++ b/lib/spack/spack/test/util/file_cache.py
@@ -5,10 +5,13 @@
 
 """Test Spack's FileCache."""
 import os
+import sys
 
 import pytest
 
-from spack.util.file_cache import FileCache
+import llnl.util.filesystem as fs
+
+from spack.util.file_cache import CacheError, FileCache
 
 
 @pytest.fixture()
@@ -55,3 +58,39 @@ def test_write_and_remove_cache_file(file_cache):
     # After removal both the file and the lock file should not exist
     assert not os.path.exists(file_cache.cache_path('test.yaml'))
     assert not os.path.exists(file_cache._lock_path('test.yaml'))
+
+
+@pytest.mark.skipif(sys.platform == 'win32',
+                    reason="Not supported on Windows (yet)")
+def test_cache_init_entry_fails(file_cache):
+    """Test init_entry failures."""
+    relpath = fs.join_path('test-dir', 'read-only-file.txt')
+    cachefile = file_cache.cache_path(relpath)
+    fs.touchp(cachefile)
+
+    # Ensure directory causes exception
+    with pytest.raises(CacheError, match='not a file'):
+        file_cache.init_entry(os.path.dirname(relpath))
+
+    # Ensure non-readable file causes exception
+    os.chmod(cachefile, 0o200)
+    with pytest.raises(CacheError, match='Cannot access cache file'):
+        file_cache.init_entry(relpath)
+
+    # Ensure read-only parent causes exception
+    relpath = fs.join_path('test-dir', 'another-file.txxt')
+    cachefile = file_cache.cache_path(relpath)
+    os.chmod(os.path.dirname(cachefile), 0o400)
+    with pytest.raises(CacheError, match='Cannot access cache dir'):
+        file_cache.init_entry(relpath)
+
+
+def test_cache_write_readonly_cache_fails(file_cache):
+    """Test writing a read-only cached file."""
+    filename = 'read-only-file.txt'
+    path = file_cache.cache_path(filename)
+    fs.touch(path)
+    os.chmod(path, 0o400)
+
+    with pytest.raises(CacheError, match='Insufficient permissions to write'):
+        file_cache.write_transaction(filename)
diff --git a/lib/spack/spack/util/file_cache.py b/lib/spack/spack/util/file_cache.py
index f9436ace73..cf5ed8310e 100644
--- a/lib/spack/spack/util/file_cache.py
+++ b/lib/spack/spack/util/file_cache.py
@@ -81,7 +81,7 @@ class FileCache(object):
             if not os.path.isfile(cache_path):
                 raise CacheError("Cache file is not a file: %s" % cache_path)
 
-            if not os.access(cache_path, os.R_OK | os.W_OK):
+            if not os.access(cache_path, os.R_OK):
                 raise CacheError("Cannot access cache file: %s" % cache_path)
         else:
             # if the file is hierarchical, make parent directories
@@ -118,6 +118,12 @@ class FileCache(object):
         moves the file into place on top of the old file atomically.
 
         """
+        filename = self.cache_path(key)
+        if os.path.exists(filename) and not os.access(filename, os.W_OK):
+            raise CacheError(
+                "Insufficient permissions to write to file cache at {0}"
+                .format(filename))
+
         # TODO: this nested context manager adds a lot of complexity and
         # TODO: is pretty hard to reason about in llnl.util.lock. At some
         # TODO: point we should just replace it with functions and simplify