linuxserver / docker-wikijs

GNU General Public License v3.0
79 stars 14 forks source link

Change /data owner to fix permission issues #8

Closed calefrey closed 3 years ago

calefrey commented 3 years ago


Description:

The /data and /config directories are initially created as root, and the worker user (abc) does not have write permission. PR #4 made abc the owner for /config, but not /data. This fixes that.

Benefits of this PR and context:

Closes #5, which was a permission issue relating to /data.

How Has This Been Tested?

docker build \
  --no-cache \
  --pull \
  -t linuxserver/wikijs:latest .
docker run \
  --name=wikijs \
  -e PUID=1000 \
  -e PGID=1000 \
  -e TZ=Europe/London \
  -p 3000:3000 \
  --rm \
  linuxserver/wikijs

Source / References:

LinuxServer-CI commented 3 years ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/wikijs/2.5.170-pkg-2b027c56-pr-8/index.html https://ci-tests.linuxserver.io/lspipepr/wikijs/2.5.170-pkg-2b027c56-pr-8/shellcheck-result.xml

github-actions[bot] commented 3 years ago

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

thelamer commented 3 years ago

I know this is an old issue, but it seems like this chown should be a run once type thing no? Do we need to chown /data every time the container starts up ?

calefrey commented 3 years ago

There isn't really a way to run a command a single time. The only real options are at build time or at every startup, and build time won't work if you're mounting the /data directory for persistent storage. So we'll just have to live with a few extra milliseconds of startup time.

KasperOnFire commented 3 years ago

Hi, this issue still persists - and the changes here are good, and fixes the issues. +1 for merging

mattkgwhite commented 3 years ago

This PR is completely valid and is 4 months old.

aptalca commented 3 years ago

You have no idea how many issues/PRs we get from users wanting to remove recursive chowns. If the data folder only needs to be owned by abc upon creation, then a non-recursive chown will be sufficient. That's assuming all data inside /data is created and managed by wikijs

mattkgwhite commented 3 years ago

@aptalca there is already precedence in this container to use recursive chowns (see the chown above for /config) any changes to this pattern is outside of the scope for this fix. You also must think of users who already have data in the directory before this change, recursive chown in this instance ensures consistency. There is another major issue with this container, the application runs as a non-root user. This is not suitable in non-posix compliant volumes (such as a cif/nfs share attached to a kubernetes pod by means of a storage driver).

aptalca commented 3 years ago

Just because we do it on one folder doesn't mean we'll do it on all. They have different circumstances.

The app is meant to run as a non-root user, that is the whole reason we allow PUID/PGID mapping. On kubernetes, if you can't get nfs to work (it works for data/media shares, but not recommended for /config due to databases and other things), you can use local or iscsi/block storage

aptalca commented 3 years ago

I don't feel the issue you're discussing here has anything to do with this fix. If that's a concern, it needs another pull request & it can be rebased if this change is merged first.

I'm commenting on the PR. Do you want me to just close it instead?

If the author changes it to a non-recursive chown, it is acceptable by me. I'm also including my assumption, expecting folks to let me know if that's wrong.

I can happily go back to my corner and ignore it. Maybe another team member will take a look.

aptalca commented 3 years ago

This is reasonable. However, as per #5, the setup flow fails. The abc user cannot write to the root-owned /data. So, a solution is required. Which solution would you prefer @aptalca?

I said it twice already: If the author changes it to a non-recursive chown, it is acceptable by me

And by the way, the folder /data is not (meant to be) created by the container. It is meant to be mapped by the end user. If the end user maps an existing folder, presumably created by the end user with the correct permissions, all is well. If the end user maps a non-existent folder as /data, then the docker service creates that folder as root. That is the behavior of the docker service and is the same for all containers, ie. not a bug.

So while I don't necessarily consider this a bug, it is a welcome improvement to make sure the folder /data is owned by the abc user.

Nfs mounts and POSIX compliance are well beyond the scope of this issue ticket and are things I have no interest in tackling.

Anyway, I'm gonna duck out and refer this to the maintainer of the image.

LinuxServer-CI commented 3 years ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/wikijs/2.5.170-pkg-2428a141-pr-8/index.html https://ci-tests.linuxserver.io/lspipepr/wikijs/2.5.170-pkg-2428a141-pr-8/shellcheck-result.xml

github-actions[bot] commented 3 years ago

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

grilix commented 3 years ago

In my case, it looks like the issue arises when mounting a volume (instead of a path), Linux doesn't allow users to write on volumes until permissions explicitly allow them to.

The changes proposed on this PR would make sure the /data directory has the correct permissions after it's mounted, so I think it would help to have this merged.

Reproducing

I was able to reproduce this issue with these steps:

$ docker volume create wiki-config
wiki-config
$ docker volume create wiki-data
wiki-data
$ docker run --rm \
>   -e PUID=1000 \
>   -e PGID=1000 \
>   -p 3000:3000 \
>   -v wiki-config:/config \
>   -v wiki-data:/data \
>   ghcr.io/linuxserver/wikijs:version-2.5.201

Then go to the browser and try to setup the application.

Result

image

Workaround

In the meantime, the permissions can be updated by injecting a script into /etc/cont-init.d/. For example:

$ cat <<EOF > 51-fix-data
#!/usr/bin/with-contenv bash
chown abc:abc /data
EOF
$ chmod +x 51-fix-data
$ $ docker run --rm \
>   -e PUID=1000 \
>   -e PGID=1000 \
>   -p 3000:3000 \
>   -v wiki-config:/config \
>   -v wiki-data:/data \
>   -v `pwd`/51-fix-data:/etc/cont-init.d/51-data \
>   ghcr.io/linuxserver/wikijs:version-2.5.201

Then back on the browser, try again and that should work.

alex-phillips commented 3 years ago

LGTM