Set file permissions in DirectoryStorage according to umask

[eddienko]

Fixes #325

This is my take on having DirectoryStorage play nice with the local defined umask.

[pep8speaks]

Hello @eddienko! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-22 09:02:17 UTC

[eddienko]

@manolinux I cannot reproduce the error that you are seeing - my understanding is that if you are the owner of a file you can chmod it. Could you add more info about the permissions that you have in those directories or a simple script that replicates the problem?

[manolinux]

No, the problem is that I'm not the owner, but I belong to the owner's group. This is quite normal when using some shared storage (mounted CIFS volume). That's why I cannot chmod (need the owner user, but python is spawned with a group user). To overcome this, the Zarr file should be created with umask-derived permissions at once. tempfile.NamedTemporaryFile creates a file with 600 (security concerns) permissions. Then, proposed commit changes permissions to umask-based. This is going to work perfectly if you are the real owner, but not in the case of group-based permissions Buy anyway, this may be my particular case, don't pay much attention if you consider this a security risk. Kind regards, and go on with the project, seems great! Manuel. El lun., 29 jul. 2019 a las 12:42, Eduardo Gonzalez (< [email protected]>) escribió:

…

@manolinux <https://github.com/manolinux> I cannot reproduce the error that you are seeing - my understanding is that if you are the owner of a file you can chmod it. Could you add more info about the permissions that you have in those directories or a simple script that replicates the problem? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#445?email_source=notifications&email_token=ABET4ZR3WMRQE2KAW5SMPWLQB3CSVA5CNFSM4H3PYYU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3AKIFQ#issuecomment-515941398>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABET4ZVGG4WB5XKNLQDRAP3QB3CSVANCNFSM4H3PYYUQ> .

[eddienko]

Sorry but I tried lots of combinations with different group permissions and users and still cannot reproduce. If anyone can provide a test where this fails I am happy to have a look at it.

[manolinux]

Hi, don't worry anymore about this.

The problem is my configuration of shared CIFS filesystem that maps users to a uid,gid distinct than current user, that's why tempfile.NamedTemporaryFile is unable to chmod.

I have to keep my tempfile._mkstemp_inner patching for going on with Zarr, but your code has then nothing to do with the issue.

Sorry for the inconvenience, and thanks a lot for your support!
Manuel.

[d-v-b]

@jrbourbeau Is there anything blocking this PR? Our group would really love to see this merged, we've had a lot of permissions-related headaches stemming from this issue.

[manolinux]

Hi, It seems there is not a scheduled 2.3.3 for the moment that could fix some of these issues that have been corrected by community. My guess (read something about it) is that efforts are put now in redefining underlying protocols and unification with N5.py. As early adopters of Zarr, I think we're paying a high price; I have had to use some of the PRs by downloading repository from git and applying manually: https://gist.github.com/adam-p/15413fabef6cffecd897 PRs installed this way seem to install ok with python setup install I'm opening an issue requesting a 2.3.3 version, and see if people vote for it. Kind regards! El dom., 29 sept. 2019 a las 22:39, Davis Bennett (<[email protected]>) escribió:

…

@jrbourbeau <https://github.com/jrbourbeau> Is there anything blocking this PR? Our group would really love to see this merged, we've had a lot of permissions-related headaches stemming from this issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#445?email_source=notifications&email_token=ABET4ZTEU52S5CR52TQTS53QMEG6ZA5CNFSM4H3PYYU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD736FEA#issuecomment-536339088>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABET4ZQRE6D6TWVQEUM75RTQMEG6ZANCNFSM4H3PYYUQ> .

[jrbourbeau]

Apologies for the delayed response @d-v-b. I'm not a Zarr maintainer, so I don't control which PRs are merged. Others like @jakirkham and @alimanfoo are more equipped to review the changes in this PR (although please keep in mind they may have very limited bandwidth right now)

That said, it looks like in the original issue, it was proposed that we could move away from using temporary files here to resolve permissions issues while avoiding the need to query the current umask and then chmod (xref #325 (comment) and #325 (comment)). Not sure if that approach that should be taken here, or if the current changes in this PR are generally okay.

[d-v-b]

@jrbourbeau Thanks for the update, sorry for putting any pressure on you.

It would be great to hear from maintainers in this thread. From my perspective, I would consider the current behavior (generating files with permissions not specified by the umask, then forcing users to manually chmod after creating containers) a bug and thus prioritize fixing that before developing the more elegant solution that avoids the temporary files.

[d-v-b]

For me, this PR does not fix permissions problems for attributes.json files when using N5Store. I'll see where that comes from.

[eddienko]

Hi,

I generally agree that this should be considered a bug and should be fixed prior to having a more general solution if there is any. Only thing however is to check that it does not break anything else, though I guess tests should do that.

@d-v-b for me this works fine as well with N5Store:

import zarr
store = zarr.N5Store('/tmp/array.n5')
z = zarr.zeros((10, 10), chunks=(5, 5), store=store, overwrite=True)
z[:] = 42

and

$ umask
022
$ ls -la array.n5
total 8
drwxr-xr-x   5 user  wheel  160  5 Oct 07:35 .
drwxrwxrwt  17 root   wheel  544  5 Oct 07:35 ..
drwxr-xr-x   4 user  wheel  128  5 Oct 07:35 0
drwxr-xr-x   4 user  wheel  128  5 Oct 07:35 1
-rw-r--r--   1 user  wheel  244  5 Oct 07:35 attributes.json

[jakirkham]

It sounds like this is addressed with PR ( #493 ). Would you agree @eddienko? If so, happy to close?