Repository: fsspec/filesystem_spec

Issue:
transaction changes default group/guest permissions
Hi,

Saving a file via `fsspec.open()` results in default (macOS) permissions 644.
Saving a file via `fs.transaction, fsopen()` strips group and guest permissions to 600.

Is this intended?

Here's a code snippet:

```
import fsspec

# correct permissions 644
with fsspec.open('correct_permissions.bin', 'wb') as f:
    f.write(b'hello')

# incorrect permissions 600
fs, urlpath = fsspec.core.url_to_fs('incorrect_permissions.bin')
with fs.transaction, fs.open(urlpath, 'wb') as f:
    f.write(b'hello')
```

Thanks!

Hints from issue comments:
@martindurant: Thanks for letting us know, and I am surprised!
Do you know where the default is defined?

The implementation for finalising a transation uses shutil.mv to put the complete file in its required position. Perhaps some reading of what that means will help us to understand what's going on. 

If your temporary directory and the output path on the same storage device?@brandon-smith-xperi: Thanks @martindurant,

- Sorry, I'm not sure where the default is defined.
- I am working on the same storage device.
- I tried a simple `shutil.move()` and it works as expected (preserving 644 permissions).

Are you able to reproduce the issue with the original code snippet?@martindurant: Yes, I do see the same behaviour.

I believe the culprit is `tempfile.mkstemp()` which is used to make the file before committing it. I suppose it isn't unreasonable that explicitly temp files should have different permissions to regular files.@martindurant: https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp 

> The file is readable and writable only by the creating user ID

At commit time, we can change the permission bits, but it would still be good to know where `644` comes from, in case that might be configured differently on some system; also, what all this does on Windows.@brandon-smith-xperi: Maybe something like this is helpful?
https://stackoverflow.com/questions/7150826/how-can-i-get-the-default-file-permissions-in-python

I have no idea what happens on Windows.@martindurant: Ah, it's right then in the definition:
```
os.open(path, flags, mode=511, *, dir_fd=None)
```

So I would suggest adding this as a PR, if you would like to:
```diff
--- a/fsspec/implementations/local.py
+++ b/fsspec/implementations/local.py
@@ -413,6 +413,10 @@ class LocalFileOpener(io.IOBase):
         if self.autocommit:
             raise RuntimeError("Can only commit if not already set to autocommit")
         shutil.move(self.temp, self.path)
+        try:
+            os.chmod(self.path, 511)
+        except RuntimeError:
+            pass
```
(I'm not actually sure what exception you can get except FileNotFOund, which should never happen here).@brandon-smith-xperi: I think `511 == 0o777`, so that will change the file permissions to 777.@martindurant: You are right in the conversions, but
```
In [ ]: os.open("afile", flags=os.O_CREAT, mode=511)
In [ ]: %ls -l afile
-rwxr-xr-x  1 mdurant  staff  0 12 Mar 10:28 afile*
```
you see no "w" for group or all.

EDIT
```
In [ ]: os.chmod("afile", 511)
In [ ]: %ls -l afile
-rwxrwxrwx  1 mdurant  staff  0 12 Mar 10:28 afile*
```

(so O_CREAT takes prescedence in open(), and we need to find where that is defined!)@martindurant: More info: fopen() in posix says:
> Any created file will have the mode S_IRUSR | S_IWUSR | S_IRGRP |
       S_IWGRP | S_IROTH | S_IWOTH (0666), as modified by the process's
       umask value (see [umask(2)](https://man7.org/linux/man-pages/man2/umask.2.html))

and this is what builtins open() does. This is 0o666 - 0o022 -> 0o655, -rw-r--r-- (minus here means "set to zero if not zero", i.e., && (not x) ).,

os.open(), by contrast, is doing 0o777 - 0o022 where 777 is the default mode and 0o022 is my umask.


TL;DR: should we set to 0o655 as the most likely value?  Please do make a PR to do this.

Solution patch:
diff --git a/fsspec/implementations/local.py b/fsspec/implementations/local.py
index ee2995d..70dfd90 100644
--- a/fsspec/implementations/local.py
+++ b/fsspec/implementations/local.py
@@ -6,6 +6,7 @@ import os.path as osp
 import shutil
 import stat
 import tempfile
+from functools import lru_cache
 
 from fsspec import AbstractFileSystem
 from fsspec.compression import compr
@@ -354,6 +355,19 @@ def trailing_sep(path):
     return path.endswith(os.sep) or (os.altsep is not None and path.endswith(os.altsep))
 
 
+@lru_cache(maxsize=1)
+def get_umask(mask: int = 0o666) -> int:
+    """Get the current umask.
+
+    Follows https://stackoverflow.com/a/44130549 to get the umask.
+    Temporarily sets the umask to the given value, and then resets it to the
+    original value.
+    """
+    value = os.umask(mask)
+    os.umask(value)
+    return value
+
+
 class LocalFileOpener(io.IOBase):
     def __init__(
         self, path, mode, autocommit=True, fs=None, compression=None, **kwargs
@@ -417,6 +431,11 @@ class LocalFileOpener(io.IOBase):
         if self.autocommit:
             raise RuntimeError("Can only commit if not already set to autocommit")
         shutil.move(self.temp, self.path)
+        try:
+            mask = 0o666
+            os.chmod(self.path, mask & ~get_umask(mask))
+        except RuntimeError:
+            pass
 
     def discard(self):
         if self.autocommit:


Test patch:
diff --git a/fsspec/implementations/tests/test_local.py b/fsspec/implementations/tests/test_local.py
index 2820c14..2e4c07c 100644
--- a/fsspec/implementations/tests/test_local.py
+++ b/fsspec/implementations/tests/test_local.py
@@ -15,7 +15,7 @@ import pytest
 import fsspec
 from fsspec import compression
 from fsspec.core import OpenFile, get_fs_token_paths, open_files
-from fsspec.implementations.local import LocalFileSystem, make_path_posix
+from fsspec.implementations.local import LocalFileSystem, get_umask, make_path_posix
 from fsspec.tests.test_utils import WIN
 
 files = {
@@ -508,6 +508,60 @@ def test_commit_discard(tmpdir):
         assert not fs.exists(tmpdir + "/bfile")
 
 
+def test_same_permissions_with_and_without_transaction(tmpdir):
+    tmpdir = str(tmpdir)
+
+    with fsspec.open(tmpdir + "/afile", "wb") as f:
+        f.write(b"data")
+
+    fs, urlpath = fsspec.core.url_to_fs(tmpdir + "/bfile")
+    with fs.transaction, fs.open(urlpath, "wb") as f:
+        f.write(b"data")
+
+    assert fs.info(tmpdir + "/afile")["mode"] == fs.info(tmpdir + "/bfile")["mode"]
+
+
+def test_get_umask_uses_cache():
+    get_umask.cache_clear()  # Clear the cache before the test for testing purposes.
+    get_umask()  # Retrieve the current umask value.
+    get_umask()  # Retrieve the umask again; this should result in a cache hit.
+    assert get_umask.cache_info().hits == 1
+
+
+def test_multiple_file_transaction_use_umask_cache(tmpdir):
+    get_umask.cache_clear()  # Clear the cache before the test for testing purposes.
+    fs = LocalFileSystem()
+    with fs.transaction:
+        with fs.open(tmpdir + "/afile", "wb") as f:
+            f.write(b"data")
+        with fs.open(tmpdir + "/bfile", "wb") as f:
+            f.write(b"data")
+    assert get_umask.cache_info().hits == 1
+
+
+def test_multiple_transactions_use_umask_cache(tmpdir):
+    get_umask.cache_clear()  # Clear the cache before the test for testing purposes.
+    fs = LocalFileSystem()
+    with fs.transaction:
+        with fs.open(tmpdir + "/afile", "wb") as f:
+            f.write(b"data")
+    with fs.transaction:
+        with fs.open(tmpdir + "/bfile", "wb") as f:
+            f.write(b"data")
+    assert get_umask.cache_info().hits == 1
+
+
+def test_multiple_filesystems_use_umask_cache(tmpdir):
+    get_umask.cache_clear()  # Clear the cache before the test for testing purposes.
+    fs1 = LocalFileSystem()
+    fs2 = LocalFileSystem()
+    with fs1.transaction, fs1.open(tmpdir + "/afile", "wb") as f:
+        f.write(b"data")
+    with fs2.transaction, fs2.open(tmpdir + "/bfile", "wb") as f:
+        f.write(b"data")
+    assert get_umask.cache_info().hits == 1
+
+
 def test_make_path_posix():
     cwd = os.getcwd()
     if WIN: