`manage.py maintenance_mode on/off` changes permissions of state file about django-maintenance-mode HOT 12 CLOSED

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

from django-maintenance-mode.

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.

from django-maintenance-mode.

@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.

from django-maintenance-mode.

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.

from django-maintenance-mode.

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

from django-maintenance-mode.

That fixed it! Thanks for the quick update!

from django-maintenance-mode.

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

from django-maintenance-mode.

@amstilp fixed in 0.21.1 version.

from django-maintenance-mode.

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.

from django-maintenance-mode.

@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?

from django-maintenance-mode.

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.

from django-maintenance-mode.

@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.

from django-maintenance-mode.