Set file permissions accoding to umask

[funkey]

Zarr uses temporary files that are moved in place to write meta-files and chunks. As a side effect, the created containers are only readable by the user who created them (as is default for temporary files). A better approach might be to set the file permissions after the move according to the users umask.

[alimanfoo]

Thanks @funkey.

Do you know if there a reliable way to determine what the current user's umask is? I found something like this, but don't know if it's the best way, and involves temporarily obliterating the umask just to find out what it is, which seems a little awkward:

import os
umask = os.umask(0)
os.umask(umask)

Is this something that can be done once and cached somewhere, or would we need to query the umask before every time you want to change a file's permissions?

Also wondering if file permissions should be surfaced as an argument to the DirectoryStore constructor, so users can control explicitly if they want to?

[funkey]

I think this is the only way to get the umask, this seems to go back to the POSIX standard (see man umask).

Caching should be fine, I don't think the umask of a process can be changed from the outside.

Having file permissions as an optional argument to DirectoryStore sounds like a good idea.

[alimanfoo]

Cool. How about adding in a chmod argument to the DirectoryStore constructor. If None (default) then files will be set to permissions based on the user's umask. If False then permissions will not be changed (current behaviour). If some other value then file permissions will be changed to that value, assuming the value is a numeric mode that can be passed to os.chmod(). Thoughts?

[funkey]

That sounds very reasonable to me!

[alimanfoo]

Great, thanks @funkey. A PR would be welcome if you have the time, otherwise we can label this as "help wanted".

[jakirkham]

What if we just used a normal file (instead of a temp file)? Would that solve this issue?

[funkey]

Actually, yes. That might be the easiest solution.

[alimanfoo]

How would you generate a name for that file?

…

On Thu, 8 Nov 2018, 18:28 Jan Funke ***@***.*** wrote: Actually, yes. That might be the easiest solution. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#325 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq8QrfNwj4V3MrzCWjF6m6AcA110zVNks5utHfngaJpZM4YTd8Z> .

[jakirkham]

Meaning the randomized part? We could use a UUID.

[alimanfoo]

So the benefit of using a normal file is (1) no need to query current umask, and (2) no need to chmod?

I.e., in the default case (file should be created according to current umask) would you generate a temporary file name using UUID, then use the built-in open() function to open it? (Then use os.replace() to move it into place when writing is finished successfully, or os.remove() if any errors occur.)

In the case where you want to allow the user to provide an explicit file mode, would you open the file with the lower-level os.open() function? Or open with built-in open() then os.chmod() if write succeeds? Or do we not worry about supporting this now (it's beyond the scope of the original issue)?

[alimanfoo]

Thinking about this has actually reminded me that we might want to reconsider the strategy of using a different randomly-named temporary file for each atomic write. I've tried to write up the issue in #328. It's somewhat orthogonal to the issue here of file permissions, however if we are considering changes to the way files are created then #328 is also relevant.

[jakirkham]

So the benefit of using a normal file is (1) no need to query current umask, and (2) no need to chmod?

That's right.

I.e., in the default case (file should be created according to current umask) would you generate a temporary file name using UUID, then use the built-in open() function to open it? (Then use os.replace() to move it into place when writing is finished successfully, or os.remove() if any errors occur.)

Sounds like a good plan to me.

In the case where you want to allow the user to provide an explicit file mode, would you open the file with the lower-level os.open() function? Or open with built-in open() then os.chmod() if write succeeds? Or do we not worry about supporting this now (it's beyond the scope of the original issue)?

Wouldn't worry about this yet. This basically hasn't come up AFAICT. Except of course because temp files are a little unusual. Moving away from temp files should eliminate the need to think about this.

[eddienko]

This is bothering me a lot, since the directory storage created by Zarr cannot be shared between members of the group. See my quick fix in the PR, does that seem reasonable?

[mcherkassky]

+1 can this be included this in the next release?

[manolinux]

+1 In my opinion, this is absolutely necessary in UNIX enviroments, should be released ASAP ...

[manolinux]

Just in case someone needs it:

I've solved this in a dirty way ... The problem comes with tempfile__mkstemp_inner, that creates the temp file with 0600 permissions. I've set up this to use current mask (mine takes into account group is 0007):

def _mkstemp_inner(dir, pre, suf, flags, output_type):
    """Code common to mkstemp, TemporaryFile, and NamedTemporaryFile."""

    names = tempfile._get_candidate_names()
    if output_type is bytes:
        names = map(tempfile._os.fsencode, names)

    for seq in range(tempfile.TMP_MAX):
        name = next(names)
        file = tempfile._os.path.join(dir, pre + name + suf)
        try:
            umask = os.umask(0)
            os.umask(umask)
            fd = tempfile._os.open(file, flags, 0o666 & ~umask)
        except FileExistsError:
            continue    # try again
        except PermissionError:
            # This exception is thrown when a directory with the chosen name
            # already exists on windows.
            if (tempfile._os.name == 'nt' and tempfile._os.path.isdir(dir) and
                tempfile._os.access(dir, tempfile._os.W_OK)):
                continue
            else:
                raise
        return (fd, tempfile._os.path.abspath(file))

    raise tempfile.FileExistsError(tempfile._errno.EEXIST,
                          "No usable temporary file name found")

tempfile._mkstemp_inner = _mkstemp_inner

[d-v-b]

+1 A fix to this would be greatly appreciated.

[adcroft]

+1 We also ran into this limitation just recently. A fix seems necessary to be able share data as expected @raphaeldussin

[raphaeldussin]

+1 with @d-v-b and @adcroft

this seems like a blocking issue for collaborative work