Match Container's User and Group to Host #49962

ObliviousHarmony · 2023-04-20T16:59:03Z

What?

This pull request changes some of the permissions for wp-env containers in order to ensure parity with the host operating system.

Why?

By using the host's user and group we are able to make sure the host and containers are all acting as the same owner for files and folders. This ensures files created by any of them are readable and writeable by the rest. For instance, wp rewrite structure '/%postname%/' --hard in the cli container will create a .htaccess file that the wordpress container is able to read.

Closes #22515, #45592, #49373, and #28201

How?

We add the current host user to the wordpress and tests-wordpress containers using the same uid and gid from the host. We also use the APACHE_RUN_USER and APACHE_RUN_GROUP environment variables so that the web server will run as the host user too. Lastly, when using wp-env run we use the host user that we created on the container.

Thanks to #42826 we can be sure that the entire contents of our WordPress installation is mounted. This is good because it means we don't have to change the permissions of any files or folders to get this to work. The end result is that on operating systems where the distinction matters, the owner of these files is the same everywhere.

Testing Instructions

Since CI is using Linux, successfully passing the tests that use wp-env is a good indicator that nothing has broken.
Run npm run env start locally and confirm that it builds the container. If you already have one, make sure to use npm run env start --update so that the new Dockerfile is used.
Run npm run env run wordpress ls -la and confirm that the files are owned by your current user. On macOS the ownership seemed oddly flaky at times, but, it doesn't matter because of the way the mounting works. I'm not sure if it does this on Linux, but, someone can test 😄
Run npm run env run cli ls -la and confirm that the files are still owned by your current user. The name will be missing, but, the raw uid is a good indicator of the difference made here.
Run npm run env run wordpress ls -la wp-content/plugins/gutenberg and note that your current user owns all of the files and folders here.
Log into wp-admin in the container and install a plugin via the UI. It should work.
Create a wp-cli.yml file in /var/www/html with the content:

apache_modules:
  - mod_rewrite

Run npm run env run cli wp rewrite structure '/%postname%/' --hard. Use npm run env run cli ls -la to confirm that a .htaccess file was created as expected.
Visit a page in the container and verify that pretty permalink are enabled and working.
Upload some media and verify that it works as expected.

Given the opportunity for this PR to go wrong, I would suggest trying anything else that you can think of!

Instead of running the web service as `root` and the CLI service as `33:33`, we should be running them as the host user. By doing this we ensure ownership parity in mounted folders on platforms where having different owners would result in permission issues.

ObliviousHarmony · 2023-04-20T18:42:08Z

Hey @defunctl, @Luc45, and @eliot-akira! Given the wide reach of this pull request, I was wondering if you folks might take a look at this and try it out locally? I think this will solve all of our existing permission problems in a minimally invasive way.

ObliviousHarmony · 2023-04-20T18:58:26Z

packages/env/lib/build-docker-compose-config.js

+	const hostUser = os.userInfo();
+	const uid = ( hostUser.uid === -1 ? 0 : hostUser.uid ).toString();
+	const gid = ( hostUser.gid === -1 ? 0 : hostUser.gid ).toString();


As per Node's documentation, on Windows the uid and gid is -1. I think this only happens with a process.platform of win32. If you're using WSL you will realistically be running commands inside the container, so, this is more of an edge-case where they have Node installed on Windows.

The question I have is whether or not this breaks anything. I'm curious about what kind of permission magic takes place when going mounting from Windows -> Docker instead of WSL -> Docker. If it works anything like macOS, I would wager that having everything owned by root doesn't matter.

packages/env/lib/build-docker-compose-config.js

packages/env/lib/commands/run.js

noahtallen · 2023-04-20T23:04:14Z

I noticed this wp-content and wp-config.php are under different users and groups. Not sure if that's a problem!

-rw-r--r--   1 www-data  www-data  5868 Apr 20 22:52 wp-config.php
drwxr-xr-x   6 root      root       192 Feb 17 20:22 wp-content

Also, curious if this code is necessary any more:

gutenberg/packages/env/lib/wordpress.js

Lines 175 to 181 in 4dbc1a6

    
           async function createUploadsDir( corePath ) { 
        
           	// Ensure the tests uploads folder is writeable for travis, 
        
           	// creating the folder if necessary. 
        
           	const uploadPath = path.join( corePath, 'wp-content/uploads' ); 
        
           	await fs.mkdir( uploadPath, { recursive: true } ); 
        
           	await fs.chmod( uploadPath, 0o0767 ); 
        
           }

So far this is working fine on macOS, but that's expected 😅

I'll try to test on Windows as well.

packages/env/CHANGELOG.md

ObliviousHarmony · 2023-04-20T23:58:45Z

I noticed this wp-content and wp-config.php are under different users and groups. Not sure if that's a problem!

This is probably because the "wordpress" image sets the permissions on that file to www-data. I might suggest trying npm run env destroy and then npm run env start if you haven't before this PR. It's worth noting though that the permissions on macOS are a bit strange. I saw it jump from root to my user for, as far as I can tell, absolutely no reason. I think it may have happened after a subsequent wp-env start because it did a git pull and maybe changed the permissions? I have absolutely no idea, but, because of the way the permissions work in macOS, it doesn't really matter.

These problems shouldn't happen on Linux because it uses bind to mount the volume and has unified ownership and permissions across the containers from the host.

Also, curious if this code is necessary any more

It probably isn't, actually. Let's wait for some further verification on whether or not this pull request works, and if it does, we can take a look at other things like this that may not be necessary anymore.

noahtallen · 2023-04-21T00:10:16Z

I might suggest trying npm run env destroy and then npm run env start if you haven't before this PR

I hadn't done that. Potentially worth mentioning in the changelog if it's necessary

Luc45 · 2023-04-21T00:11:59Z

I haven't tested this yet, but the logic seems sound. This is similar to what fixuid does. One thing that it does and I'm not seeing here is something like sudo chown -R $MAPPED_USER /var/www

ObliviousHarmony · 2023-04-21T00:21:23Z

Thanks @Luc45, fixuid works by changing the ownership of all files owned by a given user to another user. In our case, /var/www/html is a mapped directory from the host. These files are created by wp-env and are already owned by the host user. Thanks to that, we don't need to change the ownership, just the user's container.

ObliviousHarmony · 2023-04-21T00:28:08Z

I hadn't done that. Potentially worth mentioning in the changelog if it's necessary

I think it might be necessary on macOS @noahtallen, but, I'm not sure if it is elsewhere. On macOS the permissions in the containers are unique to them, so, they'll retain whatever permissions they had prior. With Linux, since the permissions are unified, they probably won't need to make any changes.

Assuming that it works in Linux as-is without regenerating the environment, I don't think folks need to do anything. If regenerating works, we should test a pre-PR start re-ran with start here and see if that breaks anything. As long as everything works before and after I don't think it matters what it looks like on macOS.

Luc45 · 2023-04-21T00:29:36Z

Sounds good, I'll take it for a spin (tentatively tomorrow) and report back

ObliviousHarmony · 2023-04-21T19:20:41Z

I used npm pack to try this fix out in our GitHub workflows. This workflow run is for a PR that has this fix. Under the Load docker images and start containers with COT enabled. header is some log output that I added:

Web Permissions 

ℹ Starting 'ls -la' on the tests-wordpress container. 

Creating 36f129a9d109274d16f01b89c88bf54c_tests-wordpress_run ... 
Creating 36f129a9d109274d16f01b89c88bf54c_tests-wordpress_run ... done
total 252
drwxr-xr-x  5 runner runner  4096 Apr 21 19:11 .
drwxr-xr-x  1 root   root    4096 Nov 15 04:13 ..
-rw-r--r--  1 runner runner   405 Apr 21 19:11 index.php
-rw-r--r--  1 runner runner 19915 Apr 21 19:11 license.txt
-rw-r--r--  1 runner runner  7402 Apr 21 19:11 readme.html
-rw-r--r--  1 runner runner  7205 Apr 21 19:11 wp-activate.php
drwxr-xr-x  9 runner runner  4096 Apr 21 19:11 wp-admin
-rw-r--r--  1 runner runner   351 Apr 21 19:11 wp-blog-header.php
-rw-r--r--  1 runner runner    32 Apr 21 19:03 wp-cli.yml
-rw-r--r--  1 runner runner  2338 Apr 21 19:11 wp-comments-post.php
-rw-r--r--  1 runner runner  3013 Apr 21 19:11 wp-config-sample.php
-rw-r--r--  1 runner runner  6019 Apr 21 19:11 wp-config.php
drwxr-xr-x  5 runner runner  4096 Apr 21 19:11 wp-content
-rw-r--r--  1 runner runner  5536 Apr 21 19:11 wp-cron.php
drwxr-xr-x 28 runner runner 12288 Apr 21 19:11 wp-includes
-rw-r--r--  1 runner runner  2502 Apr 21 19:11 wp-links-opml.php
-rw-r--r--  1 runner runner  3792 Apr 21 19:11 wp-load.php
-rw-r--r--  1 runner runner 49330 Apr 21 19:11 wp-login.php
-rw-r--r--  1 runner runner  8541 Apr 21 19:11 wp-mail.php
-rw-r--r--  1 runner runner 24993 Apr 21 19:11 wp-settings.php
-rw-r--r--  1 runner runner 34350 Apr 21 19:11 wp-signup.php
-rw-r--r--  1 runner runner  4889 Apr 21 19:11 wp-trackback.php
-rw-r--r--  1 runner runner  3238 Apr 21 19:11 xmlrpc.php
✔ Ran `ls -la` in 'tests-wordpress'. (in 1s 495ms)
CLI Permissions 

ℹ Starting 'ls -la' on the tests-cli container. 

Creating 36f129a9d109274d16f01b89c88bf54c_tests-cli_run ... 
Creating 36f129a9d109274d16f01b89c88bf54c_tests-cli_run ... done
total 252
drwxr-xr-x    5 1001     122           4096 Apr 21 19:11 .
drwxr-xr-x    1 root     root          4096 Nov 12 08:36 ..
-rw-r--r--    1 1001     122            405 Apr 21 19:11 index.php
-rw-r--r--    1 1001     122          19915 Apr 21 19:11 license.txt
-rw-r--r--    1 1001     122           7402 Apr 21 19:11 readme.html
-rw-r--r--    1 1001     122           7205 Apr 21 19:11 wp-activate.php
drwxr-xr-x    9 1001     122           4096 Apr 21 19:11 wp-admin
-rw-r--r--    1 1001     122            351 Apr 21 19:11 wp-blog-header.php
-rw-r--r--    1 1001     122             32 Apr 21 19:03 wp-cli.yml
-rw-r--r--    1 1001     122           2338 Apr 21 19:11 wp-comments-post.php
-rw-r--r--    1 1001     122           3013 Apr 21 19:11 wp-config-sample.php
-rw-r--r--    1 1001     122           6019 Apr 21 19:11 wp-config.php
drwxr-xr-x    5 1001     122           4096 Apr 21 19:11 wp-content
-rw-r--r--    1 1001     122           5536 Apr 21 19:11 wp-cron.php
drwxr-xr-x   28 1001     122          12288 Apr 21 19:11 wp-includes
-rw-r--r--    1 1001     122           2502 Apr 21 19:11 wp-links-opml.php
-rw-r--r--    1 1001     122           3792 Apr 21 19:11 wp-load.php
-rw-r--r--    1 1001     122          49330 Apr 21 19:11 wp-login.php
-rw-r--r--    1 1001     122           8541 Apr 21 19:11 wp-mail.php
-rw-r--r--    1 1001     122          24993 Apr 21 19:11 wp-settings.php
-rw-r--r--    1 1001     122          34350 Apr 21 19:11 wp-signup.php
-rw-r--r--    1 1001     122           4889 Apr 21 19:11 wp-trackback.php
-rw-r--r--    1 1001     122           3238 Apr 21 19:11 xmlrpc.php
✔ Ran `ls -la` in 'tests-cli'. (in 2s 299ms)

It looks like the changes in this PR had the desired effect. There was no longer a need to change any permissions and everything worked exactly as expected. None of the permission oddities we see on macOS are present and everything has exactly what we expected it to.

ObliviousHarmony · 2023-04-21T20:01:52Z

I set up WSL2 on my Windows machine and tested this out there. I confirmed that on trunk I receive permission issues when running wp plugin install woocommerce. After checking out this pull request I am able to run the same command and it installs WooCommerce without issue.

I've also confirmed that uploads operate the same way. Now that I've got this environment set up, I'll come back and try out removing the permission change for uploads. This might be it!

With the changes in this pull request we no longer need to do this.

noahtallen · 2023-04-21T20:14:22Z

I think it might be necessary on macOS @noahtallen, but, I'm not sure if it is elsewhere. On macOS the permissions in the containers are unique to them, so, they'll retain whatever permissions they had prior. With Linux, since the permissions are unified, they probably won't need to make any changes.

I mean, wp-env still seemed to work ok without destroying it, but maybe I should stress test it more :P

This will ensure things like `wp plugin install` have a directory to use as a cache. Anything else can now refer to the home directory for the same purpose.

Some of our `docker-compose run` actions weren't removing the container after being executed. This was breaking `wp-env destroy` becuase of the volumes that were in-use.

ObliviousHarmony · 2023-04-25T21:26:54Z

Thank you again for such comprehensive feedback @eliot-akira. This has given me a great deal of confidence that we can release this change without destroying everyone's environment. It looks like we will just need folks to destroy their current environment. Once they've done that, it will work as-expected.

noahtallen

I think the phpunit test failures could potentially be related: https://github.com/WordPress/gutenberg/actions/runs/4802349358/jobs/8545703892?pr=49962#step:11:183

Or at the very least, the changes in the PR might be uncovering an issue. I feel like these were also flaky in the PR last week, and the phpunit ones normally shouldn't be that flaky 🤔

packages/env/CHANGELOG.md

noahtallen · 2023-04-27T18:47:25Z

packages/env/README.md


 ```sh
 wp-env start --debug
 ```

-`wp-env` will output its config which includes `dockerComposeConfigPath`.
+`wp-env` will output its config which includes `dockerComposeConfigPath`:


This doc section is actually out of date now that we have npx install-path! We could probably change the entire thing to say that you can just cd $(npx wp-env install-path) and then see all the generated config files

Co-authored-by: Noah Allen <noahtallen@gmail.com>

ObliviousHarmony · 2023-04-27T21:16:37Z

Some documentation thoughts @noahtallen:

Remove the permission error information since this PR solves the problem.
Expand on the install-path documentation to clarify that it is also the location for the Docker files.
Remove the section made unnecessary by the install-path command since it is already documented as such.

Debian stretch was just removed from deb.debian.org and moved to the archive repository. This commit changes the sources so that, on stretch, the apt-get update won't break.

noahtallen · 2023-04-27T23:09:39Z

packages/env/lib/init-config.js

@@ -203,6 +203,14 @@ function wordpressDockerFileContents( image, config ) {

 	return `FROM ${ image }

+# Update apt sources for archived versions of Debian.


I might also add a comment linking to that stack overflow thread for a bit of extra context

This isn't a bad idea. Given that it's a better source of truth, I've instead linked to the Debian mailing list record of the change.

noahtallen

Alright, let's give this a shot!

westonruter · 2023-04-28T21:12:46Z

Closes #22515, #45592, #49373, and #28201

Only the the first issue got closed when this PR was merged.

eliot-akira · 2023-04-29T19:38:40Z

Amazing work - thank you @ObliviousHarmony, from all of us who will benefit from this improvement.

mrfoxtalbot · 2023-05-22T13:45:33Z

@ObliviousHarmony, could you confirm if this PR should have fixed what @shashwatahalder01 has reported in #50538 ? Thank you!

ObliviousHarmony · 2023-05-22T16:11:42Z

Hi @mrfoxtalbot,

No, this behavior is expected without appropriate configuration. As per the documentation, they will need to add a wp-cli.yml configuration file to an appropriate directory. This is one way of doing it.

ObliviousHarmony added 3 commits April 20, 2023 09:35

Changelog

027a071

Create Group When It Doesn't Exist

cdaf544

ObliviousHarmony marked this pull request as ready for review April 20, 2023 18:22

ObliviousHarmony requested a review from noahtallen as a code owner April 20, 2023 18:22

ObliviousHarmony commented Apr 20, 2023

View reviewed changes

packages/env/lib/build-docker-compose-config.js Show resolved Hide resolved

ObliviousHarmony mentioned this pull request Apr 20, 2023

Support Running Unit Tests With wp-env woocommerce/woocommerce#37799

Closed

noahtallen added [Tool] Env /packages/env [Type] Bug An existing feature does not function as intended labels Apr 20, 2023

noahtallen reviewed Apr 20, 2023

View reviewed changes

packages/env/lib/build-docker-compose-config.js Show resolved Hide resolved

noahtallen reviewed Apr 20, 2023

View reviewed changes

packages/env/lib/commands/run.js Outdated Show resolved Hide resolved

noahtallen reviewed Apr 20, 2023

View reviewed changes

packages/env/CHANGELOG.md Outdated Show resolved Hide resolved

ObliviousHarmony added 3 commits April 20, 2023 16:39

Marked Breaking Change

6c02538

Moved getHostUser() To Dedicated File

97c23c5

Merge branch 'trunk' into try/env-container-ownership

63f1144

Removed Upload Permission Mod

bd9164a

With the changes in this pull request we no longer need to do this.

ObliviousHarmony added 2 commits April 21, 2023 16:07

Added Shared Home Directory

3ffa6f4

This will ensure things like `wp plugin install` have a directory to use as a cache. Anything else can now refer to the home directory for the same purpose.

Fixed Dangling Containers

269b574

Some of our `docker-compose run` actions weren't removing the container after being executed. This was breaking `wp-env destroy` becuase of the volumes that were in-use.

ObliviousHarmony added 2 commits April 25, 2023 14:15

Added Destroy Note To Changelog

8ddef45

Documented getHostUser Windows Handling

d252b8b

noahtallen reviewed Apr 27, 2023

View reviewed changes

Update packages/env/CHANGELOG.md

3867a1d

Co-authored-by: Noah Allen <noahtallen@gmail.com>

Fixed Debian Stretch Repositories

27eaca1

Debian stretch was just removed from deb.debian.org and moved to the archive repository. This commit changes the sources so that, on stretch, the apt-get update won't break.

noahtallen reviewed Apr 27, 2023

View reviewed changes

ObliviousHarmony and others added 5 commits April 27, 2023 16:13

Updated Documentation

9cd128e

Added Debian Stretch Archive List Link

3a4146a

Merge branch 'trunk' into try/env-container-ownership

aab3e3d

Moved CLI Sleep Into Dockerfile

5d9f976

Update CHANGELOG.md

3424a05

noahtallen enabled auto-merge (squash) April 28, 2023 01:38

noahtallen approved these changes Apr 28, 2023

View reviewed changes

noahtallen merged commit 602ba69 into WordPress:trunk Apr 28, 2023

github-actions bot added this to the Gutenberg 15.8 milestone Apr 28, 2023

This was referenced Apr 28, 2023

wp-env: Unable to upload plugins on Linux & Windows, error on media upload. #45592

Closed

wp-env: file permissions issues with rewrites and uploads #28201

Closed

ObliviousHarmony deleted the try/env-container-ownership branch May 1, 2023 17:02

ObliviousHarmony mentioned this pull request May 10, 2023

Update wp-env To 7.0.0 woocommerce/woocommerce#38223

Merged

shashwatahalder01 mentioned this pull request May 11, 2023

[Env Package]: permalink structure update via wp cli #50538

Open

ObliviousHarmony mentioned this pull request May 22, 2023

Fix media uploads for Docker in rootless mode #50608

Closed

lsinger mentioned this pull request May 30, 2023

Code analyzer checkout isn't successful in temp directory woocommerce/woocommerce#37743

Closed

andrewserong mentioned this pull request Jun 15, 2023

wp-env: try to fix failing PHP Github actions #51513

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match Container's User and Group to Host #49962

Match Container's User and Group to Host #49962

ObliviousHarmony commented Apr 20, 2023 •

edited

Loading

ObliviousHarmony commented Apr 20, 2023

ObliviousHarmony Apr 20, 2023

noahtallen commented Apr 20, 2023

ObliviousHarmony commented Apr 20, 2023

noahtallen commented Apr 21, 2023

Luc45 commented Apr 21, 2023

ObliviousHarmony commented Apr 21, 2023

ObliviousHarmony commented Apr 21, 2023

Luc45 commented Apr 21, 2023

ObliviousHarmony commented Apr 21, 2023

ObliviousHarmony commented Apr 21, 2023

noahtallen commented Apr 21, 2023

ObliviousHarmony commented Apr 25, 2023

noahtallen left a comment

noahtallen Apr 27, 2023

ObliviousHarmony commented Apr 27, 2023

noahtallen Apr 27, 2023

ObliviousHarmony Apr 27, 2023

noahtallen left a comment

westonruter commented Apr 28, 2023

eliot-akira commented Apr 29, 2023 •

edited

Loading

mrfoxtalbot commented May 22, 2023

ObliviousHarmony commented May 22, 2023

		@@ -203,6 +203,14 @@ function wordpressDockerFileContents( image, config ) {

		return `FROM ${ image }

		# Update apt sources for archived versions of Debian.

Match Container's User and Group to Host #49962

Match Container's User and Group to Host #49962

Conversation

ObliviousHarmony commented Apr 20, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

ObliviousHarmony commented Apr 20, 2023

ObliviousHarmony Apr 20, 2023

Choose a reason for hiding this comment

noahtallen commented Apr 20, 2023

ObliviousHarmony commented Apr 20, 2023

noahtallen commented Apr 21, 2023

Luc45 commented Apr 21, 2023

ObliviousHarmony commented Apr 21, 2023

ObliviousHarmony commented Apr 21, 2023

Luc45 commented Apr 21, 2023

ObliviousHarmony commented Apr 21, 2023

ObliviousHarmony commented Apr 21, 2023

noahtallen commented Apr 21, 2023

ObliviousHarmony commented Apr 25, 2023

noahtallen left a comment

Choose a reason for hiding this comment

noahtallen Apr 27, 2023

Choose a reason for hiding this comment

ObliviousHarmony commented Apr 27, 2023

noahtallen Apr 27, 2023

Choose a reason for hiding this comment

ObliviousHarmony Apr 27, 2023

Choose a reason for hiding this comment

noahtallen left a comment

Choose a reason for hiding this comment

westonruter commented Apr 28, 2023

eliot-akira commented Apr 29, 2023 • edited Loading

mrfoxtalbot commented May 22, 2023

ObliviousHarmony commented May 22, 2023

ObliviousHarmony commented Apr 20, 2023 •

edited

Loading

eliot-akira commented Apr 29, 2023 •

edited

Loading