Permit reading UMASK from environment

setunnoticed commented 4 years ago

We welcome all PR’s though this doesn’t guarantee it will be accepted.

Description:

Extend the init script to optionally read UMASK from the environment.

Benefits of this PR and context:

This allows multiple systems (sonarr, radarr, etc) to access similar files by allowing common group access so they do not have to run as matching users.

How Has This Been Tested?

This same technique is used by Radarr and Sonarr docker images.

Source / References:

LinuxServer-CI commented 4 years ago

I am a bot, here are the test results for this PR: https://lsio-ci.ams3.digitaloceanspaces.com/lspipepr/nzbget/v21.0-pkg-6f10660d-pr-93/index.html https://lsio-ci.ams3.digitaloceanspaces.com/lspipepr/nzbget/v21.0-pkg-6f10660d-pr-93/shellcheck-result.xml

thelamer commented 4 years ago

NZBGet actually has this in the WebUI (Settings => Security => Umask) and you can set it in your config file (nzbget.conf)

# umask-mode (set via shell) is used in this case.
UMask=1000

On the fence here as ENV vars are useful, but this overrides their known default behavior for modifying the Umask of downloaded files so leaning towards no.

setunnoticed commented 4 years ago

NZBGet actually has this in the WebUI (Settings => Security => Umask) and you can set it in your config file (nzbget.conf)

I took a look at my current configuration. It is set to 1002. To test, I found an nzb which results in a directory plus contents (the sort that I've been struggling with) and achieved these results:

umask 022 (linuxserver/docker-nzbget default) + Settings / Security / Umask: 1002 Result: drwxr-sr-x 2 nzbget media

umask 002 (suggested modification in this PR) + Settings / Security / Umask: 1002 Result: drwxrwsr-x 2 nzbget media

On the fence here as ENV vars are useful, but this overrides their known default behavior for modifying the Umask of downloaded files so leaning towards no.

I'm not sure why, but I'm not seeing the settings/security/umask applied as consistently as the umask used right before the daemon is started.

thelamer commented 4 years ago

I don't understand the comparison why are you not setting the in app setting to 1022 and not changing any sys level Umask?

setunnoticed commented 4 years ago

I don't understand the comparison why are you not setting the in app setting to 1022 and not changing any sys level Umask?

To be clear, I want the group write bit to be set which I understand to be umask 002:

❯ umask 002
❯ touch 002.txt
❯ umask 022
❯ touch 022.txt
❯ ls -l
total 0
-rw-rw-r-- 1 root root 0 Jan 16 18:00 002.txt
-rw-r--r-- 1 root root 0 Jan 16 18:00 022.txt

When I only set 002 in the app setting, then I run into files/folders missing the group write bit. When both the sys level umask and app setting are set to 002, then the modes are correct. That is the comparison that I'm trying to make.

The app setting not applying correctly (??) is the motivation behind setting the sys level umask with the environment.

CHBMB commented 4 years ago

The app setting not applying correctly (??) is the motivation behind setting the sys level umask with the environment.

If there's an upstream issue, shouldn't that be reported and fixed at source rather than applying fixes to the container.

setunnoticed commented 4 years ago

The app setting not applying correctly (??) is the motivation behind setting the sys level umask with the environment.

If there's an upstream issue, shouldn't that be reported and fixed at source rather than applying fixes to the container.

I agree, so I've dug in further to see if this is an upstream bug.

I see this in daemon/nzbget/nzbget.cpp (apparently the only umask() call):

#ifndef WIN32
    if (m_options->GetUMask() < 01000)
    {
        /* set newly created file permissions */
        umask(m_options->GetUMask());
    }
#endif

NZBGet does not set the umask when the parameter is set >= 1000 (and this is implied in the app setting page where it says "Empty value or value "1000" disables the setting of umask-mode; current umask-mode (set via shell) is used in this case.")

I see now that my exact problem can be fixed either by a correct 002 umask in the app (1022 failed, though I meant it to set the sticky bit) OR by setting the system umask.

Since the docker image does already set a umask and NZBGet alludes to setting the value either natively or from the environment, I still think there's some value in exposing it through an environmental variable. But, at the end of the day, I'm for whatever best fits the project.

nemchik commented 4 years ago

Sorry, ignore my crazy review mess.

nemchik commented 4 years ago

This is now resolved by https://github.com/linuxserver/docker-nzbget/blob/master/README.md#umask-for-running-applications

We've included this in all of our base images.

linuxserver / docker-nzbget