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

Consider file permissions when writing configuration in system tests #43466

Merged
merged 21 commits into from
Jun 7, 2024

Conversation

muhme
Copy link
Contributor

@muhme muhme commented May 13, 2024

Pull Request for Issue #43465.

Summary of Changes

  • Change configuration.php file mode to 644, write the file and restores the original file permissions in Cypress system test case Installation.cy.js
  • updated system tests README.md

This change is developed for branch 4.4-dev and should be merged in all other branches.

Testing Instructions

It was tested on new cloned 4.4-dev branch with the patch, running installation twice on macOS 14.4.1 (Sonoma), Windows 11 Pro and inside Docker container with Debian GNU/Linux 11 (bullseye).

  • installation runs without errors == test passed
  • configuration.php parameters are set and file mask is read-only for all (444)

Actual result BEFORE applying this Pull Request

Installation fails on macOS and Windows

Expected result AFTER applying this Pull Request

Installation step passed on macOS and Windows

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

muhme added 3 commits May 13, 2024 14:38
Fix for issue joomla#43465 'Cypress System Tests fail when writing configuration.php'
. remember the original file permission
. set 644
. write file
. restore original file permission

additional:
. writing file to ${Cypress.env('cmsPath')}/configuration.php` and no more to 'configuration.php'
. error handle file is not existing
tests/System/README.md Outdated Show resolved Hide resolved
tests/System/README.md Outdated Show resolved Hide resolved
tests/System/README.md Outdated Show resolved Hide resolved
of course, thank you for checking

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@laoneo laoneo changed the title Fix issue 43465 Consider file permissions when writing configuration in system tests May 14, 2024
muhme added 2 commits May 14, 2024 18:51
deleted failure handler for readFile as it is not needed, tested with chmod 0, Cypress fails with clear reason:

	CypressError: `cy.readFile("./configuration.php")` failed while trying to read the file at the following path:
	`.../43465/joomla-cms/configuration.php`
	The following error occurred:
	> "EACCES: permission denied, open '/Users/hlu/Desktop/no_backup/43465/joomla-cms/configuration.php'"
muhme and others added 3 commits May 15, 2024 16:02
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
Chaining the then()-calls for a not so deeply nested code source
looks catchy - thank Allon for the recommendation
@laoneo
Copy link
Member

laoneo commented May 17, 2024

There is a javascript error which prevents the system tests from running.

- deleted console.log statements
- used const for never changing value
- refactored file mask to not use bitwise operation '&'
@muhme
Copy link
Contributor Author

muhme commented May 17, 2024

There is a javascript error which prevents the system tests from running.

fixed the lint:js errors and commited

@alikon
Copy link
Contributor

alikon commented May 18, 2024

i was able to reproduce the issue on w10+laragon
image

with pr

image

@alikon
Copy link
Contributor

alikon commented May 18, 2024

I have tested this item ✅ successfully on 3856d5b


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43466.

@laoneo laoneo self-assigned this May 18, 2024
@laoneo
Copy link
Member

laoneo commented May 21, 2024

I would like to merge this one, but drone still fails. @muhme can you have a look? Looks like there is an issue with the API tests.

@muhme
Copy link
Contributor Author

muhme commented May 21, 2024

I would like to merge this one, but drone still fails. @muhme can you have a look? Looks like there is an issue with the API tests.

I saw drone failed at the weekend, but I can't do anything with it, it looks unspecific with many failures, like the API failures. I tried to reproduce locally with fresh new clone, applied the PR, but running all system tests locally w/o problems.

At the moment there is a different picture, perhaps drone is started again? It doesn't come to phpmin-system-mysql step. I do not know the phan errors and do not know how to see npm errors in /tmp/npm-cache/_logs/2024-05-21T05_40_38_520Z-debug-0.log. sorry

@alikon
Copy link
Contributor

alikon commented May 21, 2024

I tried to reproduce locally with fresh new clone, applied the PR, but running all system tests locally w/o problems

same here too

@laoneo
Copy link
Member

laoneo commented May 22, 2024

You can check the artifacts, perhaps they give a clue.
image

@muhme
Copy link
Contributor Author

muhme commented May 23, 2024

You can check the artifacts, perhaps they give a clue.

checked some of the screenshot artifacts, just like with the detailed Cypress logs, unspecific with many errors, which in my opinion have nothing to do with this PR, unfortunately I can't help here, we need someone who can log into drone to check this

@muhme
Copy link
Contributor Author

muhme commented May 31, 2024

I was able to force drone to run (by update PR with changes in the main branch) and i am wondering why the same 40 tests fail again. Since these are the API and mail related tests, the reason must be that setting the parameters in configuration.php does not work - I will check this again on Linux.

@laoneo
Copy link
Member

laoneo commented Jun 1, 2024

I started drone a couple of times before. The only reason I can see, is that there is some special file permission handling or that Joomla is not on the same location as the code which is served in the webserver
https://github.com/joomla/joomla-cms/blob/4.4-dev/tests/System/drone-system-run.sh#L14

@muhme
Copy link
Contributor Author

muhme commented Jun 1, 2024

I found one problem on (hardware based) Ubuntu for me:

  • configuration.php is created by apache with user www-data and mode 444
  • i am running cypress install as user hlu and i am therefore not allowed to change the file permission, nor to write the file

This is usually not a problem in Linux Docker containers, as the user is mostly root inside.

Therefore, the original problem occurs not only under macOS and Windows, but also under Linux as a non-root user.

Running the installation as root aka with sudo is possible with Cypress, but needs additional Cypress installation for rootuser and additional space of 653 MB installed in root/cache/Cypress.

sudo npx cypress run --spec 'tests/System/integration/install/Installation.cy.js'

To prevent the additional Cypress installation for root user we can use actual users Cypress installation cache, e.g.

sudo CYPRESS_CACHE_FOLDER=$HOME/.cache/Cypress npx cypress run --spec 'tests/System/integration/install/Installation.cy.js'

Should we change to sudo?

I assume drone works before this PR because it runs in the Docker container as the root user, which can overwrite read-only files.

And it is not OK that the new changeFilePermissions() does not throw an error if it does not work, this should be able to be fixed.

And I think the same is for drone with this PR, getFilePermissions() or changeFilePermissions() fails silently and parameters in configuration.php are not configured. This is the needed to fix.

muhme added 2 commits June 4, 2024 15:28
Working with the code when fighting with the drone shows that a `chmod`
was already implemented in `writeFile()`. Following changes with this commit:
- Only using `chmod` method synchronously
- Replaced setting directory mode to setting file mode before writing
- Setting file mode only if the file exists
- Having final file mode as parameter with default 0o444
- Using 0o444 as default file mode and not hard-wired 0o777
- The methods `getFilePermissions()` and `changeFilePermissions()` created for this PR earlier are deleted.

Enhancement of the `tests/System/README.md` for troubleshooting three-user-problem in having
Cypress running user, web server running user and `root` user.

This commitment has been extensively tested in various combinations. Every test contains:
- Checking error before
- Doing the patch
- Running installation twice and running overall test suite

Tests are:
- macOS 14.5 Sonoma, local with apache & Cypress same user, branch 4.4-dev
  - error before `> EACCES: permission denied, open './configuration.php'`
- Docker, one container with joomla and one container with Cypress, using `root` users inside containers
  - no error before, but `configuration.php` is 777
  - after the patch `configuration.php` is 444 inside container and shown 644 on host
  - tested four times, branches 4.4-dev, 5.1-dev, 5.2-dev and 6.0-dev
- Ubuntu 24.04 LTS local installation, one non-root users running Cypress and
  another non-root user running Apache, branch 4.4-dev
  - error before `> EACCES: permission denied, open './configuration.php'`
  - need to use `sudo` and need to set `umask 0`, see troubleshooting
- Windows 11 Pro, Laragon with Cmder, branch 4.4-dev
  - error before `> EPERM: operation not permitted, open 'C:\laragon\www\joomla-cms\configuration.php'`

All tests are successful:
- running `Installation.cy.js` twice, checking `configuration.php` 444 and params are set
- running complete system test suite without errors
- corrected mistake task writeFile was used with cmsPath + 'configuration.php'
- extended writeFile to set process umask 0
  - to prevent the 3-user-problem == no need to set umask 0 in sudo anymore

This commitment has been tested in various combinations. Every test contains:
- Checking error before
- Doing the patch
- Running Installation.cy.js only and running overall test suite

Tests are:
- Docker environment with drone images, root running Cypress and www-data running Apache, branch 4.4-dev
  - no error before, but /tests/www/cmysql/configuration.php has 777
- Ubuntu 24.04 LTS local installation, one non-root user running Cypress and
  another non-root user running Apache, branch 4.4-dev
  - error before `> EACCES: permission denied, open './configuration.php'`
  - need to use `sudo`, see troubleshooting (umask 0 is no more needed)
- Windows 11 Pro, Laragon with Cmder, branch 4.4-dev
  - error before `> EPERM: operation not permitted, open 'C:\laragon\www\joomla-cms\configuration.php'`
  - found out that on the second run cy.exec('rm configuration.php') does not work under Windows
    - deleted file manually and i will create an issue afterwards to avoid enlarging this one
- macOS 14.5 Sonoma, local with apache & Cypress same user, branch 4.4-dev
  - error before `> EACCES: permission denied, open './configuration.php'`

All tests are successful:
- running `Installation.cy.js`, checking `configuration.php` 444 and params are set
- running complete system test suite without errors
@laoneo
Copy link
Member

laoneo commented Jun 6, 2024

Looks good now, restarted drone for another test run. If all is good, I'm goig to merge it.

@laoneo laoneo merged commit 6c45033 into joomla:4.4-dev Jun 7, 2024
3 checks passed
@laoneo
Copy link
Member

laoneo commented Jun 7, 2024

Thank you very much, this one was a lot of work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants