Standalone install - check upload directory is writable #29354

ufundo · 2024-02-09T18:37:55Z

Before

Installing Standalone using the composer starter template method fails unexpectedly if the civicrm.files directory doesn't exist or isn't writeable.

After

We check the directory is writeable before starting the installer (in the same way as we do already for templates_c).

Technical Details

I've added this as Standalone specific, but is there any reason we don't do this for other CMSes?

It's particularly relevant for Standalone because the warning that the directory isn't writeable causes a crash before we have installed the SessionHandler. See https://lab.civicrm.org/dev/core/-/issues/4988

civibot · 2024-02-09T18:37:58Z

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

➡️ PR Demo site (can be rebuilt with the command test this please).
📖 Review standards
🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

civibot · 2024-02-09T18:38:01Z

The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/4988

artfulrobot

Hi @ufundo I've made some comments here based on a read-through of the code; I've not tried running it yet.

Thanks for the work! It's interesting to learn about setup plugins - new to me!

setup/plugins/installFiles/StandaloneCivicrmFilesPath.civi-setup.php

artfulrobot · 2024-02-12T08:24:11Z

setup/plugins/installFiles/StandaloneCivicrmFilesPath.civi-setup.php

+    else {
+      $e->addInfo('system', 'civicrmFilesPathWritable', sprintf('The civicrm files dir "%s" can be created.', $civicrmFilesDirectory));
+    }


This else hits cases where the sentence is not true, e.g.

There is no dir defined in config.

The dir already exists

Suggest just removing it?

good point! have added the conditions (I feel like it could be a useful notice for user to see that it's creating the directory where they want it/somewhere sensible)

setup/plugins/installFiles/StandaloneCivicrmFilesPath.civi-setup.php

artfulrobot · 2024-02-12T08:31:21Z

setup/plugins/installFiles/StandaloneCivicrmFilesPath.civi-setup.php

+      Civi\Setup::log()->info('[StandaloneCivicrmFilesPath.civi-setup.php] mkdir "{path}"', [
+        'path' => $civicrmFilesDirectory,
+      ]);
+      mkdir($civicrmFilesDirectory, 0777, TRUE);


0777 is very open; it makes the uploaded data world-read-writable. If the ancestor dirs do not provide protection against access this could mean some other process (e.g. another website on same server) could write files in the civi upload dir.

Also, I note that the following line calls FileUtil which does the same thing- we don't need that call.

To my eye, the parameter here looks like a formality necessitated by setting $recursive=TRUE. 0777 is the PHP default, and it's still subject to umask. It's consistent with how CreateTemplateCompilePath does it.

Due to umask handling, mkdir(...0777...) doesn't have the same meaning as chmod(...0777...) (in FileUtil::makeWebWritable()):

mkdir() speaks softly: "I'm fine with anything up to 0777, but the local system might prefer to change it via umask."

chmod() speaks loudly: "Set this thing to 0777. It will be public!"

The part that's being overly prescriptive is makeWebWritable(). Fixing that would kinda be a different feature, tho, wouldn't it? (e.g. "configurable permissioning"?)

Full disclosure: I just blindly copied from CreateTemplateCompilePath ...

If the webserver is able to create the file, can we assume it's the file owner, and so 700 or 770 be ok?

Or is this also called by CLI install, in which case you might be running as a different user. Still in this case 770 and make sure you have a shared group seems better place to get to?

Good point @totten re mkdir respecting umask.

I think we just ditch the 2nd call that applies 0777 and we're done.

(@ufundo) Or is this also called by CLI install, in which case you might be running as a different user.

Quite right.

(@totten) The part that's being overly prescriptive is makeWebWritable(). Fixing that would kinda be a different feature, tho, wouldn't it? (e.g. "configurable permissioning"?)

(@ufundo) Still in this case 770 and make sure you have a shared group seems better place to get to?

(@artfulrobot) I think we just ditch the 2nd call that applies 0777 and we're done.

Ugh, messy area.

Unfortunately, in the wild, 0770, 0700, and inherited-umask are just as likely to leave a broken environment. Even if the person knows about the console-user/web-user split, their primary-group is usually mismatched (myuser:staff or myuser:myuser not myuser:www-data not myuser:apache), and they usually don't have any suitable policy/inheritance mechanism (like sticky bits or umasks or setfacl). And even if they did, it's hard to -preconfigure enough nuance to distinguish "gid/perms for new code files" (re:unzip) and "gid/perms for new data files" (re:cv core:install).

The thing about 0777 for initializing data-folders is.... it just works. It's not awful (in the common contexts of a dedicated web-server or personal workstation or container), but it's not good either (wrt defense-in-depth or consistently-mutually access). For a professional admin, it's usually not what you want. But it's simple; it's a universally recognized quantity; and you can tune post-install.

IMHO:

The KISS thing is basically what you have already (call makeWebWritable()).

A bonus thing would be an option in the install-model ($model->dataDirMode). Teach makeWebWritable() to abide. The web-based installer and CLI-installer can set this differently. Folks doing system-automation can tune it.

Another bonus thing would be to write more detailed detection/training/advice. ("Your source-code is owned by a different user? Do X..." or "Your server has a user named www-data? Do Y...") You would probably target this first as a status-check (which applies to post-install as well as system-restores, migrations, etc).

Kiss

Makes sense.

Thanks both.

On that basis I'd be quite keen to leave the makeWebWritable for now.. its consistent with the templates_c behaviour and if anyone comes along to improve that they'll pick up this occurence?

ufundo · 2024-02-13T16:52:38Z

I think the fails are spurious... retest this please? 🙂

totten · 2024-02-13T21:36:21Z

@ufundo If you look at the latest test results, the "Console" shows this:

ParseError: syntax error, unexpected token ";", expecting ")" in Civi\Setup::init() (line 23 of /home/jenkins/bknix-dfl/build/core-29354-7jo84/web/sites/all/modules/civicrm/setup/plugins/installFiles/StandaloneCivicrmFilesPath.civi-setup.php).

ufundo · 2024-02-14T10:28:51Z

Oops sorry 🤦 should be fixed now

ufundo · 2024-02-22T23:20:37Z

@totten or @artfulrobot - I think this could be mergeable now?

artfulrobot · 2024-03-06T14:20:13Z

I did this to test:

git clone https://github.com/civicrm/civicrm-standalone /var/www/example.com
cd /var/www/example.com

Edited the composer file:

Change the require entry for core to be like:

 "civicrm/civicrm-core": "dev-standalone-install-upload-directory",

Add this section:

 "repositories": [{"type": "vcs","url": "https://github.com/ufundo/civicrm-core.git"}],

Then

composer install

Then navigate to the web installer, enter database details (create database first). I then saw this:

I needed to:

chmod a+rw data
mkdir web/upload; chmod a+rw web/upload or chmod a+rw web

Then the installer continued.

artfulrobot · 2024-03-06T14:29:28Z

Merging based on tests - I think the behaviour I documented is what's supposed to happen.

Thanks @ufundo

civibot bot added the master label Feb 9, 2024

ufundo mentioned this pull request Feb 9, 2024

dev/core#4988 Standalone - use regular php session during install #29352

Merged

ufundo force-pushed the standalone-install-upload-directory branch from 35937c0 to 8909112 Compare February 10, 2024 12:03

ufundo changed the title ~~dev/core#4988 Standalone install - ensure upload directory is writable, then load standaloneusers early~~ dev/core#4988 Standalone install - check upload directory is writable Feb 10, 2024

ufundo changed the title ~~dev/core#4988 Standalone install - check upload directory is writable~~ Standalone install - check upload directory is writable Feb 10, 2024

ufundo mentioned this pull request Feb 10, 2024

dev/core#4988 enable standaloneusers extension early in Standalone install #29362

Closed

ufundo force-pushed the standalone-install-upload-directory branch from 8909112 to bcab1ef Compare February 10, 2024 12:25

ufundo marked this pull request as ready for review February 11, 2024 19:47

artfulrobot reviewed Feb 12, 2024

View reviewed changes

ufundo force-pushed the standalone-install-upload-directory branch 2 times, most recently from e23e886 to 340d5e6 Compare February 12, 2024 11:05

check upload directory writable for standalone install

82418bf

ufundo force-pushed the standalone-install-upload-directory branch from 340d5e6 to 82418bf Compare February 14, 2024 10:27

artfulrobot merged commit 0d6ea26 into civicrm:master Mar 6, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standalone install - check upload directory is writable #29354

Standalone install - check upload directory is writable #29354

ufundo commented Feb 9, 2024 •

edited

Loading

civibot bot commented Feb 9, 2024

civibot bot commented Feb 9, 2024

artfulrobot left a comment

artfulrobot Feb 12, 2024

ufundo Feb 12, 2024

artfulrobot Feb 12, 2024

totten Feb 12, 2024

ufundo Feb 12, 2024

ufundo Feb 12, 2024

artfulrobot Feb 12, 2024

totten Feb 12, 2024

artfulrobot Feb 12, 2024

ufundo Feb 13, 2024

ufundo commented Feb 13, 2024

totten commented Feb 13, 2024

ufundo commented Feb 14, 2024

ufundo commented Feb 22, 2024

artfulrobot commented Mar 6, 2024 •

edited

Loading

artfulrobot commented Mar 6, 2024

Standalone install - check upload directory is writable #29354

Standalone install - check upload directory is writable #29354

Conversation

ufundo commented Feb 9, 2024 • edited Loading

Before

After

Technical Details

civibot bot commented Feb 9, 2024

civibot bot commented Feb 9, 2024

artfulrobot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ufundo commented Feb 13, 2024

totten commented Feb 13, 2024

ufundo commented Feb 14, 2024

ufundo commented Feb 22, 2024

artfulrobot commented Mar 6, 2024 • edited Loading

artfulrobot commented Mar 6, 2024

ufundo commented Feb 9, 2024 •

edited

Loading

artfulrobot commented Mar 6, 2024 •

edited

Loading