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

Handle folder permissions in wp-env #22515

Closed
talldan opened this issue May 21, 2020 · 27 comments · Fixed by #49962
Closed

Handle folder permissions in wp-env #22515

talldan opened this issue May 21, 2020 · 27 comments · Fixed by #49962
Assignees
Labels
[Status] In Progress Tracking issues with work in progress [Tool] Env /packages/env [Type] Enhancement A suggestion for improvement.

Comments

@talldan
Copy link
Contributor

talldan commented May 21, 2020

Is your feature request related to a problem? Please describe.
In some recent merged PRs, there's been a requirement to change the permissions of certain folders to be writable when using wp-env, in particular when using travis.

The way this is handled should be consistent, and there may also be a requirement to handle other folders (e.g. all of the upload folder).

It'd also be good to get an understanding of why this is required in certain environments.

Previous examples:

Describe the solution you'd like
Open to suggestions, but the code should be made consistent as a first improvement.

@noahtallen
Copy link
Member

note: blocking some tests here: #22454 (comment)

@dd32
Copy link
Member

dd32 commented Jul 14, 2020

It'd also be good to get an understanding of why this is required in certain environments.

This happens because wp-env assumes that the user id's between systems will be consistent, ie. that the www-data user is the same ID on the host AND container, but also kind of assumes that it's being RUN AS www-data on the host system (which it never should be).

An example of this is https://github.com/WordPress/gutenberg/blob/master/packages/env/lib/wordpress.js#L36-L48 where it chown's folders to www-data on the host system, but that user might not match the ID within the container. #20403 is another example of that.

I was looking into this yesterday, and I have a few ideas that I hashed out, one of which worked, I just need to explore it further (I was testing in an interactive GitHub action, so limited patching ability)
The "simplest" option, since Docker doesn't support ID remapping in the needed way, is to alter the www-data user within the container to match the ID of the owner of the files being mounted. That can be done by a) a new image that usermod -u $UID www-data && groupmod -g $GID www-data or b) overriding/mounting a new docker entrypoint.

Another option was to alter the upstream wordpress docker image to allow executing as the needed user (making use of it's APACHE_RUN_AS var), but that's not as ideal/easy, as it may require creating a new user anyway, and that user may already exist using the specified ID.

The other option is that the WordPress mount and everything under it could just be chmod 777'd and deal with the mismatch between file ownerships on the parent system later. That's super not-ideal, and limits the ability to have tests that alter filesystems be realistic.

@noahtallen
Copy link
Member

The other option is that the WordPress mount and everything under it could just be chmod 777'd

Unfortunately, we have many examples of this too. :( Even chmod 777 on the whole mount would be an improvement to scattering it around everywhere.

The "simplest" option, since Docker doesn't support ID remapping in the needed way, is to alter the www-data user within the container

This sounds pretty great! Is it possible to modify this user without having to change images (which seems like a large undertaking, particularly since multiple images are used)? For example, the user property in docker-compose: https://docs.docker.com/compose/compose-file/#domainname-hostname-ipc-mac_address-privileged-read_only-shm_size-stdin_open-tty-user-working_dir. Though I don't know what exactly that does (I'm fairly inexperienced with Docker!)

@dd32
Copy link
Member

dd32 commented Jul 15, 2020

This sounds pretty great! Is it possible to modify this user without having to change images (which seems like a large undertaking, particularly since multiple images are used)?

Thankfully with Docker you don't have to completely change the upstream images, creating a "new local image" from the upstream image is a few extra lines of code, effectively you can run a few extra CLI commands between the upstreams commands and the upstreams execution point. It does however add a few seconds (and an extra 2 temporary docker container builds) to the bootup process.

The other option is that you change the entrypoint, the "default command" that executes when you start a docket container, to be a wrapper script that's effectively custom-commands && run default upstream image entrypoint which is a little faster than a local image, but not as futureproof (ie. if upstream were to change the default entry point, we'd have to update to reference the new file location). It's also a little more fragile for when you use a docker run command since the default command will probably have been overridden.

I'm looking at the first option today :)

For example, the user property in docker-compose: https://docs.docker.com/compose/compose-file/#domainname-hostname-ipc-mac_address-privileged-read_only-shm_size-stdin_open-tty-user-working_dir. Though I don't know what exactly that does

Unfortunately the user property doesn't actually help a lot here, it simply defines what userNAME commands run as by default inside the container, and since there's no guaranteed HostUserID:ContainerUserID mapping, they sometimes work but othertimes don't.
ie. When the wordpress:cli image runs as user 33:33 (which is www-data:www-data on most debian systems I believe) it's actually running as the xfs:xfs user/group on the alpine subsystem - the X Font Server user - which isn't a huge issue, since we only really care about the filesystem permissions in that case, but for example $HOME would be /etc/X11/fs and it might have extra unexpected privileges or that user ID may not even exist in a future image (Why is there an X Font Server user in the first place on a headless image?!)

I'm fairly inexperienced with Docker!

I wouldn't call myself an expert, but I've run into these problems myself and researched such things a lot!

@noahtallen
Copy link
Member

Thank you for investigating! I look forward to learning more about it

@dd32
Copy link
Member

dd32 commented Jul 16, 2020

Not fully tested, but master...dd32:fix/wp-env-users is basically what I was thinking of.

It's working for me in my testing on linux/mac, I suspect it might not work for Windows systems - I'd need to spin up a Windows box to try.

@noahtallen
Copy link
Member

Nice! Do you think similar changes would be needed for the phpunit or composer services?

@dd32
Copy link
Member

dd32 commented Jul 16, 2020

Do you think similar changes would be needed for the phpunit or composer services?

If the proposed changes work over other environments, then it would make sense to also add it to the other containers.
I wasn't able to test the phpunit/composer containers properly, so I haven't included those here yet (Also why I haven't PR'd it yet)

The composer container runs as root within the container currently, and is alpine based, so the changes here should apply to that container without issue, it looks like running docker run --rm -ti -u www-data composer works as expected so I don't see why it wouldn't work.

I hadn't dug too deep into the phpunit container, but it's not as straight forward I don't think, due to some workarounds already in there: WordPress/wpdev-docker-images#16

It may make more sense to standardise on what's done there, running as a wp_php user within the container rather than juggling as I've done here to change the www-data user, but effectively it's the same thing.
I think for phpunit could even just add {PHP_FPM_UID: localUser, PHP_FPM_GID: localGroup } to the environment param and have it work.
It's also pointed out the -o param for usermod/groupmod which removes the need to change the IDs of existing users, making the changes even simpler (ie Removing the 4 EXISTING_USER/EXISTING_GROUP lines)

@noahtallen
Copy link
Member

I think for phpunit could even just add {PHP_FPM_UID: localUser, PHP_FPM_GID: localGroup } to the environment param and have it work.

Nice. I think theoretically change away from wordpressdevelop/phpunit if we need to. As far as I know, we only use that image to get a phpunit binary included. All test libs/wp sources come from elsewhere

@ajlende
Copy link
Contributor

ajlende commented Aug 10, 2020

Found this issue when I was having permissions issues when using rootless docker on Linux. It may be an edge-case that only needs documentation, but it would be great if the solution for this issue also worked with rootless docker. (And I can help test that scenario when a PR becomes available)

@westonruter
Copy link
Member

ie. When the wordpress:cli image runs as user 33:33 (which is www-data:www-data on most debian systems I believe) it's actually running as the xfs:xfs user/group on the alpine subsystem - the X Font Server user - which isn't a huge issue, since we only really care about the filesystem permissions in that case

I'm trying to get wp-env working on a Linux environment and I'm facing this issue as well, although the issue seems more severe than is being described here. Namely, the files aren't even readable by the xfs user, let alone writable. When I run npm run wp-env start I get:

> gutenberg@13.1.0-rc.1 wp-env .../repos/gutenberg
> wp-env "start"

✖ Error while running docker-compose command.
Creating 9989d161c2b5e3a6f07afd784767de21_cli_run ... 
Creating 9989d161c2b5e3a6f07afd784767de21_cli_run ... done
Error: This does not seem to be a WordPress installation.
Pass --path=`path/to/wordpress` or run `wp core download`.

It seems when running docker-compose like here:

async function checkDatabaseConnection( { dockerComposeConfigPath, debug } ) {
await dockerCompose.run( 'cli', 'wp db check', {
config: dockerComposeConfigPath,
commandOptions: [ '--rm' ],
log: debug,
} );
}

It is running as the xfs user. Nevertheless, if I shell into the wordpress container (and am then root) I can see the file permissions appear to be overly locked down to not have the read bit set for all:

gutenberg$ docker exec -it 0e071513d5c3 ls -laF
total 232
drwxr-x---  6   610350    89939  4096 Apr 22 04:33 ./
drwxr-xr-x  1 root     root      4096 Apr 20 11:18 ../
drwxr-x---  8   610350    89939  4096 Apr 22 21:53 .git/
-rw-r-----  1   610350    89939   405 Apr 22 04:32 index.php
-rw-r-----  1   610350    89939 19915 Apr 22 04:32 license.txt
-rw-r-----  1   610350    89939  7401 Apr 22 04:32 readme.html
-rw-r-----  1   610350    89939  7165 Apr 22 04:32 wp-activate.php
drwxr-x---  9   610350    89939  4096 Apr 22 04:32 wp-admin/
-rw-r-----  1   610350    89939   351 Apr 22 04:32 wp-blog-header.php
-rw-r-----  1   610350    89939  2338 Apr 22 04:32 wp-comments-post.php
-rw-r-----  1   610350    89939  3001 Apr 22 04:32 wp-config-sample.php
-rw-r--r--  1 www-data www-data  5584 Apr 22 04:33 wp-config.php
drwxr-x---  5   610350    89939  4096 Apr 22 04:32 wp-content/
-rw-r-----  1   610350    89939  3939 Apr 22 04:32 wp-cron.php
drwxr-x--- 26   610350    89939 12288 Apr 22 04:32 wp-includes/
-rw-r-----  1   610350    89939  2494 Apr 22 04:32 wp-links-opml.php
-rw-r-----  1   610350    89939  3973 Apr 22 04:32 wp-load.php
-rw-r-----  1   610350    89939 48429 Apr 22 04:32 wp-login.php
-rw-r-----  1   610350    89939  8577 Apr 22 04:32 wp-mail.php
-rw-r-----  1   610350    89939 23706 Apr 22 04:32 wp-settings.php
-rw-r-----  1   610350    89939 32051 Apr 22 04:32 wp-signup.php
-rw-r-----  1   610350    89939  4748 Apr 22 04:32 wp-trackback.php
-rw-r-----  1   610350    89939  3236 Apr 22 04:32 xmlrpc.php

The result is that when WP-CLI runs its check_wp_version method, this logic fails:

		$wp_exists      = $this->wp_exists();
		$wp_is_readable = $this->wp_is_readable();
		if ( ! $wp_exists || ! $wp_is_readable ) {

Because wp-includes/version.php is not readable.

Is this a problem then with download-sources.js in that it isn't properly setting the right file permissions?

I'm fairly inexperienced with Docker!

Me too!

@noahtallen
Copy link
Member

Is this a problem then with download-sources.js in that it isn't properly setting the right file permissions?

Nice investigation! That could explain why it's not happening to everyone. Depending on your .wp-env config, the default is to just use the WordPress files from the docker image, so wp-env wouldn't be doing anything to obtain those files itself in that scenario. I think Gutenberg uses this: https://github.com/WordPress/WordPress. I think the way that works is:

  • wp-env has git clone WordPress to the wp-env directory for the project.
  • docker compose maps that local directory to /var/www/html, the root of the install, as a volume.

wp-env isn't doing anything to set permissions. I wonder if there's a kind of conflict overriding the permissions in the Development docker image with a local git source?

As a side note, I used wp-env successfully on both Arch and Ubuntu, and it's definitely running on Linux in CI as well. I'm super curious why this issue is not coming up consistently!

@westonruter
Copy link
Member

westonruter commented Apr 22, 2022

I'm trying this with the Gutenberg repo.

I was able to get start to complete by shelling into the wordpress and tests-wordpress containers and doing:

chmod -R 755 .

Once that was done then WP-CLI was able to read the files properly.

So it does seem like insufficient file permissions are indeed the problem. I don't know why either, however.

@noahtallen
Copy link
Member

For reference, here are the permissions on my working copy (on macOS):

$ docker exec -it 93b83f01977a ls -laF

total 212
drwxr-xr-x  24 root     root       768 Apr 22 22:33 ./
drwxr-xr-x   1 root     root      4096 Apr 20 11:18 ../
drwxr-xr-x  14 root     root       448 Apr 22 22:33 .git/
-rw-r--r--   1 root     root       405 Jan  5 21:49 index.php
-rw-r--r--   1 root     root     19915 Jan  5 21:49 license.txt
-rwxrwxrwx   1 root     root      5839 Apr 22 22:33 phpunit-wp-config.php*
-rw-r--r--   1 root     root      7437 Jan  5 21:49 readme.html
-rw-r--r--   1 root     root      7165 Jan  5 21:49 wp-activate.php
drwxr-xr-x 101 www-data www-data  3232 Apr 22 22:33 wp-admin/
-rw-r--r--   1 root     root       351 Jan  5 21:49 wp-blog-header.php
-rw-r--r--   1 root     root      2338 Jan  5 21:49 wp-comments-post.php
-rw-r--r--   1 root     root      3001 Jan  5 21:49 wp-config-sample.php
-rw-r--r--   1 www-data www-data  5881 Apr 15 21:08 wp-config.php
drwxr-xr-x   6 root     root       192 Jan  5 21:50 wp-content/
-rw-r--r--   1 root     root      3939 Jan  5 21:49 wp-cron.php
drwxr-xr-x 242 www-data www-data  7744 Apr 22 22:33 wp-includes/
-rw-r--r--   1 root     root      2496 Jan  5 21:49 wp-links-opml.php
-rw-r--r--   1 www-data www-data  3900 Jan  5 21:49 wp-load.php
-rw-r--r--   1 root     root     47914 Apr 22 22:33 wp-login.php
-rw-r--r--   1 root     root      8582 Jan  5 21:49 wp-mail.php
-rw-r--r--   1 www-data www-data 23025 Jan  5 21:49 wp-settings.php
-rw-r--r--   1 root     root     31959 Jan  5 21:49 wp-signup.php
-rw-r--r--   1 root     root      4747 Jan  5 21:49 wp-trackback.php
-rw-r--r--   1 root     root      3236 Jan  5 21:49 xmlrpc.php

Should we then add chmod -R 755 . as a post-install step for any source? That would also assume that the command could work via docker-compose without shelling in

@westonruter
Copy link
Member

Sounds good to me, but I'm no authority on these matters 😄

@westonruter
Copy link
Member

Why not just go ahead and chmod -R 777 . the whole directory tree? It's not like the file permissions need to be locked down in the container for development purposes, right?

@noahtallen
Copy link
Member

noahtallen commented May 4, 2022

Apologies for the delay, been fairly busy!

I experimented with;

await dockerCompose.exec( 'wordpress', 'chmod -R 777 .', dockerComposeConfig );
await dockerCompose.exec( 'tests-wordpress', 'chmod -R 777 .', dockerComposeConfig );

put right around here:

spinner.text = 'Configuring WordPress.';

Essentially, right after downloading and starting the docker images, but before configuring anything else.

After trying this, it did work correctly -- everything has rwx permissions from ls, but I noticed I now have ~7000 VCS changes. So I'm assuming that this is updating permissions in my Gutenberg checkout, which I think we want to avoid!

@westonruter
Copy link
Member

Oh right. We should avoid touching the execute bit for files because that is tracked by Git. So So I think files should get 666 and directories should get 777:

await dockerCompose.exec( 'wordpress', 'find wordpress tests-wordpress -type d -exec chmod 777 {} +', dockerComposeConfig );
await dockerCompose.exec( 'wordpress', 'find wordpress tests-wordpress -type f -exec chmod 666 {} +', dockerComposeConfig );

Either that, or when cloning the repo the config.fileMode could be disabled:

git config core.filemode false

@noahtallen
Copy link
Member

noahtallen commented May 5, 2022

Toying around with something like this:

const ex = ( container, cmd ) =>
  dockerCompose.exec( container, cmd, dockerComposeConfig );
await Promise.all( [
  ex( 'wordpress', 'find -type d -exec chmod 777 {} +' ),
  ex( 'wordpress', 'find -type f -exec chmod 666 {} +' ),
  ex( 'tests-wordpress', 'find -type d -exec chmod 777 {} +' ),
  ex( 'tests-wordpress', 'find -type f -exec chmod 666 {} +' ),
] );

The performance is really poor. There are a ton of files -- for example, it's doing stuff on node_modules in Gutenberg, and any .git directories.

I'm not loving this approach, partly because of the performance, but also because I don't love the idea of setting permissions on user's directories which wp-env doesn't "own."

So I went back to looking at the download source function, and tried using fs.chmod to set 777 permissions on the local filesystem instead of doing it from docker... but I'm unsure if this fixes the root problem. Do you have a way to test it? I made a draft PR here: #40864

@westonruter
Copy link
Member

The performance is really poor. There are a ton of files -- for example, it's doing stuff on node_modules in Gutenberg, and any .git directories.

Oh, yeah, this should be excluding node_modules and .git.

@westonruter
Copy link
Member

westonruter commented May 17, 2022

Continuing on from https://github.com/WordPress/gutenberg/pull/40864/files#r875265169:

But then again, like you said, this will take awhile to run. What I don't understand is why the Docker user downloading WordPress is not the same user who is actually running the site. If the user was the same, then no permissions changes would be needed at all.

@swissspidy
Copy link
Member

What's a current workaround for this issue?

I am struggling to get wp language core install to work on GitHub Actions, because WP-CLI is unable to create the wp-content/languages directory in wp-env due to lack of permissions. Likewise, installing language packs via WP admin doesn't work either because of that.

It works locally, but not in a GitHub Actions environment.

@noahtallen
Copy link
Member

noahtallen commented Nov 8, 2022

I don't know if it'll help in this instance, but chmod on the impacted directories can help. (I've definitely used that before to get CI working in some cases)

There was also a lot more discussion on this recent PR about this issue: #42867. The general idea is that docker makes this harder than it should be, and that we might need to introduce something like https://github.com/boxboat/fixuid to really solve the issue 🤔

@swissspidy
Copy link
Member

If it helps, this is what I ended up doing to ensure I can install languages with WP-CLI on the tests instance:

WP_ENV_DIR=$(npm run wp-env install-path --silent 2>&1 | head -1)
cd $WP_ENV_DIR
mkdir -p tests-WordPress/wp-content/languages tests-WordPress/wp-content/upgrade
chmod -R 767 tests-WordPress/wp-content/languages tests-WordPress/wp-content/upgrade
cd -
npm run wp-env run tests-cli "wp language core install de_CH de_DE es_ES fr_FR it_IT"

@ObliviousHarmony
Copy link
Contributor

ObliviousHarmony commented Apr 20, 2023

I dove really deep into this issue today. While our wp-env environment works great locally, in CI, there are some permission-related problems that are worked around. I wanted to understand why and have some notes to share:

  • Docker Desktop on Windows and macOS runs the daemon in a secret Linux VM. This has some interesting implications for mounted directories, and as a result, this does not happen on those platforms.
  • Mounted directories on Linux retain the same uid:gid ownership as well as permissions.
    • wordpress and tests-wordpress environments run as root.
    • cli and tests-cli environments run as 33:33.
    • /var/www/html/ in Docker always maps to the host, so, it will always have inherited ownership/permissions.
  • Since the wordpress and tests-wordpress environments run as root there is only really a problem here when using the CLI environments.
  • The cli and tests-cli environments only have read-only access to /var/www/html and everything below.
    • The Docker image provided by WordPress changes a few files to 33:33, but, you can't, for instance, install plugins.

Approaching this as an ownership problem instead of a permission problem might help us reach a better solution that is minimally invasive enough to be implemented in wp-env. I have some thoughts on a solution:

  • We don't need to do anything if process.platform is win32 or darwin because the problem does not occur there.
  • Set the group of all files downloaded by wp-env to 33. This means WordPress itself, the PHPUnit files, and any plugins/themes downloaded.
    • Since wp-env put these files there in the first place I don't think it matters whether or not the user can edit them, right?
    • Do not change the ownership of anything that is mapped using a local path. This includes core, mappings, and plugins.
  • Add group write permissions to all of the files that have had their ownership changed.

As a result of the above changes:

  • The user that ran wp-env still owns the files and can do whatever they want with them.
  • The wordpress and tests-wordpress containers can still do anything they want with the files because they run as root.
  • The www-data user in the cli and tests-cli containers now have read and write permission to all of the files and directories that wp-env provided. The CLI will now be able to manipulate the filesystem like you would expect it to be able to.

I think the only caveat here is that anything mapped using a local path will still have permission issues in the cli and tests-cli containers. This means that any WP-CLI command that would make changes to those files or folders will receive permission errors. This seems like an acceptable edge case and can just be documented in the README.md file if we're concerned about it.


As an aside, currently, any files or folders created by the wordpress or tests-wordpress container have 33:33 ownership since they're created by the web server, right? Does that mean you need sudo privileges when using wp-env destroy to remove those files? I think rimraf prints an error message about it though and they can remove it manually?

What do you think @noahtallen?

@noahtallen
Copy link
Member

Thanks for doing a deep dive! I really appreciate it.

Could we change the user of the CLI environments to deal with the locally mapped file issue? (Including ones managed by wp-env?) One thing which came up recently is that the WordPress source code is a locally mapped directory via git (to better handle setting the correct version). As a result, this makes the surface area for problems fairly large.

There was another great summary of the issue recently here: #45592 (comment) The prevailing thought was to use fixuid in our dockerfile. Any thoughts about how that connects to this issue?

@ObliviousHarmony
Copy link
Contributor

Thank you for linking that issue @noahtallen; there is a lot of great ideas and detail there. One thing in particular that it made apparent is that it's important that files created by the 33:33 owner are writeable by the host user.

The prevailing thought was to use fixuid in our dockerfile. Any thoughts about how that connects to this issue?

I would prefer we don't use any external tools in wp-env. fixuid in particular has not been updated in two years and seems very heavy-handed given that we know specifically what directories we should chown. With that said, given wp-env is a development environment, maybe there is something to be learned from it.


As far as I can tell, this might be the best course of action:

  • Modify the generated Dockerfile to create your current user in the container. Since we can set uid:gid permissions this isn't strictly necessary, but, it'll give us a pretty username:group from ls -la instead of uid:gid.
    • Make sure to also add the user to the sudo group.
  • Set APACHE_RUN_USER and APACHE_RUN_GROUP environment variables in the generated docker-compose. This means that any files created, deleted, or modified by the web server will be accessible on the host.
  • run should set the --user option to the current user. Since we added this user to the sudo group in the web container, they will still have root access.

The end result is that you have uid:gid parity between the host and containers. Since all of the directories are mounted, we should be able to get away with this 😄

I'm going to set aside some time today to poke at this solution and see if it works as well as I expect it to. If so, I'll put up a PR and we can see if it works for folks in the other issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Tool] Env /packages/env [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants