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 @@ def init_entry(self, key): 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 @@ def write_transaction(self, key): 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