From 74e063b98061017f960c1b91d469b12f3129d9dd Mon Sep 17 00:00:00 2001 From: Eduardo Gonzalez Date: Wed, 26 Jun 2019 09:30:42 +0100 Subject: [PATCH 1/4] Set file permissions in DirectoryStorage according to umask Fixes #325 --- zarr/storage.py | 14 +++++++++++--- zarr/tests/test_storage.py | 7 +++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/zarr/storage.py b/zarr/storage.py index d30b0da6df..50306e2c3a 100644 --- a/zarr/storage.py +++ b/zarr/storage.py @@ -74,6 +74,14 @@ def _path_to_prefix(path): return prefix +def _NamedTemporaryFileUmask(*args, **kargs): + fdesc = tempfile.NamedTemporaryFile(*args, **kargs) + umask = os.umask(0) + os.umask(umask) + os.chmod(fdesc.name, 0o666 & ~umask) + return fdesc + + def contains_array(store, path=None): """Return True if the store contains an array at the given logical path.""" path = normalize_storage_path(path) @@ -752,9 +760,9 @@ def __setitem__(self, key, value): # write to temporary file temp_path = None try: - with tempfile.NamedTemporaryFile(mode='wb', delete=False, dir=dir_path, - prefix=file_name + '.', - suffix='.partial') as f: + with _NamedTemporaryFileUmask(mode='wb', delete=False, dir=dir_path, + prefix=file_name + '.', + suffix='.partial') as f: temp_path = f.name f.write(value) diff --git a/zarr/tests/test_storage.py b/zarr/tests/test_storage.py index 90461b9db4..20b47e22bf 100644 --- a/zarr/tests/test_storage.py +++ b/zarr/tests/test_storage.py @@ -721,6 +721,13 @@ def test_filesystem_path(self): store['foo'] = b'bar' assert os.path.isdir(path) + # check correct permissions + stat = os.stat(path) + mode = stat.st_mode & 0o666 + umask = os.umask(0) + os.umask(umask) + assert mode == (0o666 & ~umask) + # test behaviour with file path with tempfile.NamedTemporaryFile() as f: with pytest.raises(ValueError): From a5b8f5999c1e302f437f609095da0932235b2e20 Mon Sep 17 00:00:00 2001 From: James Bourbeau Date: Fri, 25 Oct 2019 12:55:40 -0500 Subject: [PATCH 2/4] Use built-in open function --- zarr/storage.py | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/zarr/storage.py b/zarr/storage.py index 42d934511e..8b387ed1f8 100644 --- a/zarr/storage.py +++ b/zarr/storage.py @@ -32,6 +32,7 @@ from os import scandir from pickle import PicklingError from threading import Lock, RLock +import uuid from numcodecs.compat import ensure_bytes, ensure_contiguous_ndarray from numcodecs.registry import codec_registry @@ -82,14 +83,6 @@ def _path_to_prefix(path): return prefix -def _NamedTemporaryFileUmask(*args, **kargs): - fdesc = tempfile.NamedTemporaryFile(*args, **kargs) - umask = os.umask(0) - os.umask(umask) - os.chmod(fdesc.name, 0o666 & ~umask) - return fdesc - - def contains_array(store, path=None): """Return True if the store contains an array at the given logical path.""" path = normalize_storage_path(path) @@ -776,12 +769,11 @@ def __setitem__(self, key, value): raise KeyError(key) # write to temporary file - temp_path = None + # note we're not using tempfile.NamedTemporaryFile to avoid restrictive file permissions + temp_name = file_name + '.' + uuid.uuid4().hex + '.partial' + temp_path = os.path.join(dir_path, temp_name) try: - with _NamedTemporaryFileUmask(mode='wb', delete=False, dir=dir_path, - prefix=file_name + '.', - suffix='.partial') as f: - temp_path = f.name + with open(temp_path, mode='wb') as f: f.write(value) # move temporary file into place @@ -789,7 +781,7 @@ def __setitem__(self, key, value): finally: # clean up if temp file still exists for whatever reason - if temp_path is not None and os.path.exists(temp_path): # pragma: no cover + if os.path.exists(temp_path): # pragma: no cover os.remove(temp_path) def __delitem__(self, key): From 37a00f58a314c35eefcd0afc746bb069e39c7a4c Mon Sep 17 00:00:00 2001 From: James Bourbeau Date: Fri, 25 Oct 2019 12:57:49 -0500 Subject: [PATCH 3/4] Adds link to GH issue --- zarr/tests/test_storage.py | 1 + 1 file changed, 1 insertion(+) diff --git a/zarr/tests/test_storage.py b/zarr/tests/test_storage.py index f648b155c3..a58bae1f58 100644 --- a/zarr/tests/test_storage.py +++ b/zarr/tests/test_storage.py @@ -754,6 +754,7 @@ def test_filesystem_path(self): assert os.path.isdir(path) # check correct permissions + # regression test for https://github.com/zarr-developers/zarr-python/issues/325 stat = os.stat(path) mode = stat.st_mode & 0o666 umask = os.umask(0) From 0de512349f7517b5150d13a981788896b8481e78 Mon Sep 17 00:00:00 2001 From: James Bourbeau Date: Fri, 25 Oct 2019 13:52:23 -0500 Subject: [PATCH 4/4] Adds release notes entry --- docs/release.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/release.rst b/docs/release.rst index f4f99c2e9b..6512fe7351 100644 --- a/docs/release.rst +++ b/docs/release.rst @@ -24,6 +24,9 @@ Upcoming Release * Removed support for Python 2. By :user:`jhamman`; :issue:`393`, :issue:`470`. +* Update ``DirectoryStore`` to create files with more permissive permissions. + By :user:`Eduardo Gonzalez ` and :user:`James Bourbeau `; :issue:`493` + .. _release_2.3.2: 2.3.2