Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

manage.py maintenance_mode on/off changes permissions of state file #172

Closed
amstilp opened this issue Jan 22, 2024 · 12 comments
Closed

manage.py maintenance_mode on/off changes permissions of state file #172

amstilp opened this issue Jan 22, 2024 · 12 comments
Assignees
Labels
bug Something isn't working

Comments

@amstilp
Copy link

amstilp commented Jan 22, 2024

Python version
3.8.10

Django version
4.2.8

Package version
0.21.1

Current behavior (bug description)
After running python manage.py maintenance_mode on or python manage.py maintenance_mode off, the permissions of the config/settings/maintenance_mode_state.txt file are changed from -rw-r--r-- to -rw-------. With our setup, the maintenance mode file cannot be ready by apache and causes our site to throw server/500 errors when trying to load it.

This is new behavior, I think introduced by #162.

Expected behavior
The permissions of the maintenance_mode_state.txt file remain the same when running python manage.py maintenance_mode on/off.

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@amstilp amstilp added the bug Something isn't working label Jan 22, 2024
@amstilp
Copy link
Author

amstilp commented Jan 22, 2024

Note that we can fix this by manually running chmod to change the permissions of maintenance_mode_state.txt back to what we need them to be, but it took us a while to figure out what was going on, and it seems like the package shouldn't be changing existing permissions.

@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Jan 22, 2024

@amstilp thank you for reporting this.

Yes, much probably the issue has been introduced by #162.

This is the function that writes the file atomically and that should also set the right permissions:
https://github.com/fabiocaccamo/python-fsutil/blob/main/fsutil/__init__.py#L1340

If you can figure out where the problem is in the function above that would be great, alternatively I'll debug as soon as I can.

@amstilp
Copy link
Author

amstilp commented Jan 22, 2024

Thanks for looking into it!

Yeah, I see what you're saying - _write_file_atomic calls _set_permissions. I wonder if it would work better to read the original permissions first, replace the file, and then set them after copying the file.

def _write_file_atomic(
    path: PathIn,
    content: str,
    *,
    append: bool = False,
    encoding: str = "utf-8",
) -> None:
    path = _get_path(path)
    mode = "a" if append else "w"
    if append:
        content = read_file(path, encoding=encoding) + content
    dirpath, _ = split_filepath(path)
    try:
        with tempfile.NamedTemporaryFile(
            mode=mode,
            dir=dirpath,
            delete=True,
            encoding=encoding,
        ) as file:
            file.write(content)
            file.flush()
            os.fsync(file.fileno())
            temp_path = file.name
            ### Changes start here ###
            path_exists = exists(path)
            if path_exists:
                permissions = get_permissions(path))
            os.replace(temp_path, path)
            os.set_permissions(path, permissions)
            ### Changes end here ###
    except FileNotFoundError:
        # success - the NamedTemporaryFile has not been able
        # to remove the temp file on __exit__ because the temp file
        # has replaced atomically the file at path.
        pass

I'm happy to try it in a pull request and see if it works on our servers.

@fabiocaccamo
Copy link
Owner

@amstilp could you try to install python-fsutil from main branch and let me know if the last commit fixes the issue?

@amstilp
Copy link
Author

amstilp commented Jan 23, 2024

That fixed it! Thanks for the quick update!

@fabiocaccamo
Copy link
Owner

@amstilp good news, thanks for the feedback! I'm going to publish a patch release.

@fabiocaccamo
Copy link
Owner

@amstilp fixed in 0.21.1 version.

amstilp added a commit to UW-GAC/primed-django that referenced this issue Jan 24, 2024
This update fixes a bug where the permissions of the maintenance
mode state file are not properly maintained (due to the implementation
in python-fsutil). v0.21.1 of django maintenance mode requires the
version of python-fsutil where this bug is fixed. See
fabiocaccamo/django-maintenance-mode#172 for info.
amstilp added a commit to UW-GAC/gregor-django that referenced this issue Jan 24, 2024
This update fixes a bug where the permissions of the maintenance
mode state file are not properly maintained (due to the implementation
in python-fsutil). v0.21.1 of django maintenance mode requires the
version of python-fsutil where this bug is fixed. See
fabiocaccamo/django-maintenance-mode#172 for info.
amstilp added a commit to UW-GAC/gregor-django that referenced this issue Jan 24, 2024
This update fixes a bug where the permissions of the maintenance
mode state file are not properly maintained (due to the implementation
in python-fsutil). v0.21.1 of django maintenance mode requires the
version of python-fsutil where this bug is fixed. See
fabiocaccamo/django-maintenance-mode#172 for info.
@amstilp
Copy link
Author

amstilp commented Jan 24, 2024

Awesome, thanks. I've updated to the new version!

@TurtleCode84
Copy link

I ran into related issues when intially running manage.py maintenance_mode on/off on a fresh installation of the package, the initial permissions of the newly created maintenance_mode_state.txt are -rw-------, causing Apache server errors as described by the OP. chmod fixes it, but it seems like a similar fix to the OP's problem.

I was also running into a problem where permission was being denied to create a tmpXXXXXXX file (in the same directory as the state file) whenever I tried to use the maintenance-mode/on or maintenance-mode/off URLs to toggle maintenance mode. Not sure if this is an issue with my own system's perms or an actual bug, but I did want to mention it just in case it was relevant.

@fabiocaccamo
Copy link
Owner

@TurtleCode84 when the state file is created, it has some "default" permissions, only in subsequent overwrites the state file inherits permissions from the existing state file.

Some improvements that could be done by this package are:

  • when creating the file, set the file permissions like the directory permissions
  • alternatively add a new setting for specifying the permissions to set on the state file

@amstilp @reitermarkus what do you think about this?

@amstilp
Copy link
Author

amstilp commented Jan 29, 2024

Either of those options makes sense to me. I'm not sure it's worth having a setting for the permissions, since you can pretty easily change them as a one-time step and then they will remain going forward.

@fabiocaccamo
Copy link
Owner

@amstilp I agree, thinking about it, it doesn't make much sense to give the possibility to decide the permissions, but it makes more sense to inherit the permissions from the directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

3 participants