search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.47k stars 4.18k forks source link

Handle folder permissions in wp-env #22515

Closed talldan closed 1 year ago

talldan commented 4 years ago

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 commented 4 years ago

note: blocking some tests here: https://github.com/WordPress/gutenberg/pull/22454#issuecomment-633154131

dd32 commented 4 years ago

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. https://github.com/WordPress/gutenberg/pull/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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

dd32 commented 4 years ago

Not fully tested, but https://github.com/WordPress/gutenberg/compare/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 commented 4 years ago

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

dd32 commented 4 years ago

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: https://github.com/WordPress/wpdev-docker-images/pull/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 commented 4 years ago

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 commented 4 years ago

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 commented 2 years ago

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:

https://github.com/WordPress/gutenberg/blob/ace064391f2ce2d96c30a0c8761a5d2a985b9c5b/packages/env/lib/wordpress.js#L27-L33

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 commented 2 years ago

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 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

westonruter commented 2 years ago

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 commented 2 years ago

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: https://github.com/WordPress/gutenberg/blob/1b9fe3868a1481dc39f3e49ede11ce6f1615d2d7/packages/env/lib/commands/start.js#L144

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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/WordPress/gutenberg/pull/40864

westonruter commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/WordPress/gutenberg/pull/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 commented 1 year ago

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 commented 1 year ago

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:

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:

As a result of the above changes:

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 commented 1 year ago

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: https://github.com/WordPress/gutenberg/issues/45592#issuecomment-1504224581 The prevailing thought was to use fixuid in our dockerfile. Any thoughts about how that connects to this issue?

ObliviousHarmony commented 1 year ago

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:

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 :smile:

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.