added multiuser support and associated tests #192

jahrules · 2023-02-02T16:52:28Z

new option FileLock(multiuser = True) (defaults to False):

results in Lock File being created with rw-rw-rw- permissions so that all users on the system can use the lockfile.

jahrules · 2023-02-20T18:25:45Z

flake8 doesnt like the import filemode in test_filelock.py that I added to check the filemode with this addition. Aside from that; all tests should pass now.

Apologies for the gap from the original PR to these fixes. It was some time before I could access a windows box to debug.

jahrules · 2023-02-26T01:01:54Z

I wanted to post perhaps a few clarifications.

This PR addresses requests noted in the following issues:
#112
#70

As mentioned in issue #112 I updated the variable name from mode to flags - which is a finding that I made independently before finding it mentioned in the aforementioned issue.

gaborbernat

Add changelog entry here https://github.com/tox-dev/py-filelock/blob/main/docs/changelog.rst

gaborbernat · 2023-03-13T16:57:46Z

src/filelock/_api.py

+        self,
+        lock_file: str | os.PathLike[Any],
+        timeout: float = -1,
+        multi_user: bool = False,


I'm not sure about this interface. Should we just expose access_control instead and allow them to set it to whatever they want?

I'm open to this idea. My only concern is that when setting permissions for "group" or "all" you also have to set os.umask(0) so a change to that interface will also require extra checking for the requested permissions to set the umask if necessary

I'm alright to make that check 👍

jahrules · 2023-03-13T20:41:06Z

I have some additional information on this. The default to mode in os.open is 511 (0o777 in octal). So the r/w bits are set by default. The only reason that the default mode doesn't make the files globally r/w is the lack of the os.umask call. So, even with exposing the mode directly, I believe we need a way for users to specify if they intend the lock to be multiuser or not. Maybe a solution would to both allow the user to specify their own mode and specify if multi-user is intended but this may lead to more confusion. Open to thoughts on this.

…

On Mon, Mar 13, 2023, 3:48 PM Bernát Gábor ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/filelock/_api.py <#192 (comment)>: > @@ -39,14 +39,20 @@ def __exit__( class BaseFileLock(ABC, contextlib.ContextDecorator): """Abstract base class for a file lock object.""" - def __init__(self, lock_file: str | os.PathLike[Any], timeout: float = -1) -> None: + def __init__( + self, + lock_file: str | os.PathLike[Any], + timeout: float = -1, + multi_user: bool = False, I'm alright to make that check 👍 — Reply to this email directly, view it on GitHub <#192 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APCURVAHRHBMU3UFCN3PX2DW352XVANCNFSM6AAAAAAUPIPPVM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

jahrules · 2023-03-13T20:43:02Z

Another potential solution would be to make the filelock default mode 0o700 but I'm not comfortable making the call on if we should deviate from the os open default or not.

…

On Mon, Mar 13, 2023, 4:40 PM Greg Taeger ***@***.***> wrote: I have some additional information on this. The default to mode in os.open is 511 (0o777 in octal). So the r/w bits are set by default. The only reason that the default mode doesn't make the files globally r/w is the lack of the os.umask call. So, even with exposing the mode directly, I believe we need a way for users to specify if they intend the lock to be multiuser or not. Maybe a solution would to both allow the user to specify their own mode and specify if multi-user is intended but this may lead to more confusion. Open to thoughts on this. On Mon, Mar 13, 2023, 3:48 PM Bernát Gábor ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/filelock/_api.py > <#192 (comment)>: > > > @@ -39,14 +39,20 @@ def __exit__( > class BaseFileLock(ABC, contextlib.ContextDecorator): > """Abstract base class for a file lock object.""" > > - def __init__(self, lock_file: str | os.PathLike[Any], timeout: float = -1) -> None: > + def __init__( > + self, > + lock_file: str | os.PathLike[Any], > + timeout: float = -1, > + multi_user: bool = False, > > I'm alright to make that check 👍 > > — > Reply to this email directly, view it on GitHub > <#192 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/APCURVAHRHBMU3UFCN3PX2DW352XVANCNFSM6AAAAAAUPIPPVM> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

gaborbernat · 2023-03-14T01:46:35Z

I believe we need a way for users to specify if they intend the lock to be multiuser or not.

But there's no such thing as multi user. Is just mangles to acces to the lock created. It just happens that a given combination of flags enable multiple users to use the same lock file.

jahrules · 2023-03-14T03:07:32Z

Well yes but it's the combination of flags plus umask. Typical default for umask (at least on unix based OSes) is 0o133 to deny write to group/all. So even though the os.open has a default mode of 0o777; the umask default denies write making it effectively 0o644 which is why "other users" can't use the same lock. So to distill this to the core; I want to ask filelock users "do you want other users to use this lock?" and modify umask to permit this. Modifying mode and umask is pretty advanced for most users which is why Python put that implementation into os.open and didn't expose it in the standard "open". Maybe the best option is to leave mode alone as I think it highly unlikely that users would need granular permissions. The three options I can reasonably see people wanting are "self" locks (as they are today), "group" locks (where the "group" bit is set to write (umask 0o113), and "global" locks (umask 0o111). The use case for the 3rd would be like if an application user and human user both needed to use the lock. The difference between 2 and 3 is, however, minimal.

…

On Mon, Mar 13, 2023, 9:46 PM Bernát Gábor ***@***.***> wrote: I believe we need a way for users to specify if they intend the lock to be multiuser or not. But there's no such thing as multi user. Is just mangles to acces to the lock created. It just happens that a given combination of flags enable multiple users to use the same lock file. — Reply to this email directly, view it on GitHub <#192 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APCURVBZBFTZCMHPF2W4BILW37EYPANCNFSM6AAAAAAUPIPPVM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

gaborbernat · 2023-03-14T14:36:15Z

src/filelock/_unix.py

-            fd = os.open(self._lock_file, open_mode)
+            open_flags = os.O_RDWR | os.O_CREAT | os.O_TRUNC
+            if self._multi_user is True:
+                os.umask(0)


Shouldn't this be in a try/finally, so after creation we restore the previous value? Also do we need this conditionality? Can't we just always set umask to 0 before creation and restore after? Technically if the umask to set to something non zero, the open mode 511 will not be respected. Also this umask thing is repeated across files so any reason can't be a contextmanager in base?

agree completely. I would suggest that by going that route, the default should be 0o644 so that the 'net effect' of this change is that the default behavior remains the same. Let me know if you would prefer something else.

Also, I think that I will set the umask to 0o111 instead of 0; since there is no reason that the lock files should be executable.

Changes in progress.

src/filelock/_api.py

gaborbernat · 2023-03-15T18:25:34Z

src/filelock/_api.py

@@ -198,6 +199,9 @@ def acquire(
            with self._thread_lock:
                self._lock_counter = max(0, self._lock_counter - 1)
            raise
+        finally:
+            # reset umask to initial value
+            os.umask(previous_umask)


move comment into the finally line

gaborbernat · 2023-03-15T18:26:26Z

src/filelock/_api.py

        """
        Create a new lock object.

        :param lock_file: path to the file
        :param timeout: default timeout when acquiring the lock, in seconds. It will be used as fallback value in
        the acquire method, if no timeout value (``None``) is given. If you want to disable the timeout, set it
        to a negative value. A timeout of 0 means, that there is exactly one attempt to acquire the file lock.
+        : param multiuser: if True, make the lock file permissions rw/rw/rw for all users


mode not multiuser now 👍

gaborbernat · 2023-03-15T18:27:59Z

docs/changelog.rst

@@ -1,5 +1,8 @@
 Changelog
 =========
+v3.9.1 (2023-03-13)


3.10.0 as is feature

gaborbernat · 2023-03-15T19:26:28Z

src/filelock/_api.py

@@ -52,7 +52,7 @@ def __init__(
        :param timeout: default timeout when acquiring the lock, in seconds. It will be used as fallback value in
        the acquire method, if no timeout value (``None``) is given. If you want to disable the timeout, set it
        to a negative value. A timeout of 0 means, that there is exactly one attempt to acquire the file lock.
-        : param multiuser: if True, make the lock file permissions rw/rw/rw for all users
+        : param mode: file permissions for the lockfile to have. default: 0o644 (-rw-r--r--)


remove default, it's already in the header of the method

gaborbernat · 2023-03-15T19:27:58Z

src/filelock/_api.py

        start_time = time.monotonic()
+        previous_umask = os.umask(0)


Shouldn't this try/catch be scopped just to _acquire?

Looking at _acquire; it will never throw a BaseException error, so it should be safe to just wrap self._acquire() with a set and reset.

The try block is probably a candidate for future improvement because it's quite strangely written; but that seems outside of the scope of this PR.

What if you get a keyboardinterrupt; let's use try/finally please.

gaborbernat

Can you rebase on main, and fix merge conflict?

for more information, see https://pre-commit.ci

jahrules · 2023-03-15T20:23:30Z

rebased and fixed all conflicts.

gaborbernat requested changes Mar 12, 2023

View reviewed changes

gaborbernat requested changes Mar 13, 2023

View reviewed changes

gaborbernat requested changes Mar 14, 2023

View reviewed changes

gaborbernat reviewed Mar 15, 2023

View reviewed changes

src/filelock/_api.py Show resolved Hide resolved

gaborbernat reviewed Mar 15, 2023

View reviewed changes

docs/changelog.rst Outdated

@@ -1,5 +1,8 @@

Changelog

=========

v3.9.1 (2023-03-13)

Copy link

Member

gaborbernat Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.10.0 as is feature

gaborbernat reviewed Mar 15, 2023

View reviewed changes

gaborbernat requested changes Mar 15, 2023

View reviewed changes

jahrules and others added 10 commits March 15, 2023 15:59

added multiuser support and associated tests

c0551fe

[pre-commit.ci] auto fixes from pre-commit.com hooks

a5e6517

for more information, see https://pre-commit.ci

fixed a bug in windows File Perms

080b8d7

flake8 fixes

a7bae6f

fixed test for windows condition

98695dd

adding filemode to whitelist

6bb1cff

added changelog as requested

acf338b

updated code based on comments and added tests for os.umask()

ea1e757

updates as requested

31f634a

changes as requested

4259eae

updated os.umask to be inside a try:finally

b4d42bb

gaborbernat approved these changes Mar 15, 2023

View reviewed changes

gaborbernat merged commit d0061e4 into tox-dev:main Mar 15, 2023
27 checks passed

chriskuehl mentioned this pull request Mar 22, 2023

filelock 3.10.0 changes umask to 0 in multithreaded processes, causing world-writable files to be created #204

Closed

hmaarrfk mentioned this pull request Oct 30, 2023

Allow users to subclass FileLock with custom keyword arguments #284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added multiuser support and associated tests #192

added multiuser support and associated tests #192

jahrules commented Feb 2, 2023

jahrules commented Feb 20, 2023

jahrules commented Feb 26, 2023

gaborbernat left a comment

gaborbernat Mar 13, 2023

jahrules Mar 13, 2023

gaborbernat Mar 13, 2023

jahrules commented Mar 13, 2023 via email

jahrules commented Mar 13, 2023 via email

gaborbernat commented Mar 14, 2023

jahrules commented Mar 14, 2023 via email

gaborbernat Mar 14, 2023

jahrules Mar 15, 2023 •

edited

gaborbernat Mar 15, 2023

gaborbernat Mar 15, 2023

gaborbernat Mar 15, 2023

gaborbernat Mar 15, 2023

gaborbernat Mar 15, 2023

jahrules Mar 15, 2023

gaborbernat Mar 15, 2023

gaborbernat left a comment

jahrules commented Mar 15, 2023 •

edited

added multiuser support and associated tests #192

added multiuser support and associated tests #192

Conversation

jahrules commented Feb 2, 2023

jahrules commented Feb 20, 2023

jahrules commented Feb 26, 2023

gaborbernat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahrules commented Mar 13, 2023 via email

jahrules commented Mar 13, 2023 via email

gaborbernat commented Mar 14, 2023

jahrules commented Mar 14, 2023 via email

Choose a reason for hiding this comment

jahrules Mar 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat left a comment

Choose a reason for hiding this comment

jahrules commented Mar 15, 2023 • edited

jahrules Mar 15, 2023 •

edited

jahrules commented Mar 15, 2023 •

edited