Add 'fix permissions' task

Solid plan. I was planning on making this a separate module but it seems that this is more or less what tasks_extra is for.

+1 for creating such a script/task.

One downside of using drush for this is the risk of priviledge escalation.

If the fix-permissions drush command is called as root, can we be sure that no other code where the aegir user can write to is used?
In most current installations the aegir user can modify the drush code ... or add a drush module with e.g. a pre hook.

So any code called via sudo should be only writable by root. My vote would be for a compact standalone script php of shell without any includes.

What about the script from #1737266: The directory sites/drupal7.d7.local/files is not writable.

I've taken the script example from https://www.drupal.org/node/244924, and opened a pull request in one of the git repo's tracking it.

Instructions:
Copy the fix-permissions.sh script from this patch to /usr/local/bin/fix-permissions.sh (especially a place where the aegir user should have NO write access).

Add to sudo with `visudo -f /etc/sudoers.d/aegir`

aegir ALL = (root) NOPASSWD: /usr/local/bin/fix-permissions.sh

Usage:
sudo /usr/local/bin/fix-permissions.sh --drupal_path=/var/aegir/platforms/drupal-8.0.x --drupal_user=aegir

About adding a task ... this script works on a platform ... so a platform task would make the most sense.

We could eventually include this in the Debian package install but only after thorough review.

A slightly more specific sudo line:
aegir ALL = (root) NOPASSWD: /usr/local/bin/fix-drupal-file-permissions.sh --drupal_path=/var/aegir/platforms/*/ --drupal_user=aegir

And I suggest to name the script fix-drupal-file-permissions.sh to make it's purpose even more explicit.

I assumed the verify task was for fixing permissions too. What is wrong with that?

If we need a script (jikes) for that I agree with @helmo to name it fix-drupal-file-permissions

(a hostmaster user)

I was talking about site file permissions. My site was broken as site verify seemed to fix top level files dir(s) but forgot it's children

Why not ask the site for it's private:// public:// and temp:// and fix those recursive?

private/temp needs to be handled.

And I think we can simplify it to only work on sites ... not the platform.

targeting for the next release

Marking major. This is a sleeping dragon of a problem, one that most people aren't aware of, and one that causes aegir to "look bad".

After a comment from omega8cc, I think this needs to be a part of aegir core's Verify task.

We already are setting permissions on files as much as aegir user is allowed. In fact this is one of the big selling points of aegir: "You don't have to mess with file permissions." ...

I would almost call this a bug. When the system claims to "set permissions, configuration, and ensure the site is in order" but it actually can't properly reset permissions on uploaded files, wouldn't that be a bug?

Let's make this happen...

In BOA we are running global permissions fix daily, for all sites (but also for custom platforms, to make sure people don't try to make their codebases writable in some dangerous way). It helps a bit, but is still not ideal, since once you clone a site, you can't wait until the next day to have all permissions fixed for you. It should be fixed on the fly. Here is the script we run daily. It does many more than permissions fix, and already turned into spaghetti code, so we need to make it modular, but maybe parts of it could offer some hints.

The procedure where the global fix happens, can be seen here: fix_permissions()

Please note that the fix should be run (available as an extra task?) both for sites and platforms.

It is otherwise possible that people create codebase which is not writable by the Aegir user, and it will instead log warnings like:

Could not change permissions of /path/to/platform/sites/all to 755 (chmod to 755 failed on /path/to/platform/sites/all)
Could not change permissions of /path/to/platform/sites to 751 (chmod to 751 failed on /path/to/platform/sites)

We may also need to add a Drush command, which would more easily allow for remote execution.

In case anyone's interested in looking at the code, I worked on a Drush command for fixing permissions a while ago which never got committed. The issue is #990812: Add a "permissions" subcommand to fix/set all file permissions.

Edit: The most recent patch is probably the most useful: https://www.drupal.org/node/990812#comment-7013970.

What about the 'sites/example.com/private' I assume that needs the same commands as used for files.

Hook re-ordering would be nice here ... but would also make it even more complex. The zz_ prefix seems like an easy solution. Otherwise we might have to look for another hook... a post_post hook :) ?

Module weights?

Unfortunately, Drush doesn't have a concept of module weights, unless that's changes in the last year or so. It relies only on the alphabetical order of the extensions/commandfiles.

I just found Allow modules to control the order of hook invocations which you worked on a while ago, but it looks like it may require the listed follow-up? It would be good to get that in.

Running this in a pre-verify hook can be insufficient to leave a properly working site. For example, CiviCRM will delete and recreate some directories during its cache-clear, that occurs late in a post-verify hook. So, moving to a post-hook, and renaming the Drush files and hook implementations to add a "zz_" prefix, are sufficient to ensure our script is run last. Kind of ugly though, so I haven't committed it yet.

That's a really good point, update hooks or drupal cache clearing might do things to files too.

But I'm thinking we might actually need to run this both pre and post verify.

I run into the situation where Aegir verify cannot run because some file is unwritable, so the fixperms would need to be fixed before that.

This needs a good hint to the user that the install script needs to be executed ... for both permission and ownership modules.
If you currently fail do to do this you get a warning in the task log which is not directly obvious.

I've added a drupal_set_message when the modules are enabled...

Bonus points lets it link to the actual docs... but rtbc otherwise

Perhaps implementing a "hook_requirements()" would be helpful, not just for this but other things Aegir needs?

http://cgit.drupalcode.org/hosting_tasks_extra/commit/?id=402925b

I'm running Aegir 3.9 and have enabled the Fix Permissions and Fix Ownership modules and run the bash install scripts but verify is now throwing warning for sites that previously verified.

EDIT:

I see in the Aegir log for one verify task:

Calling hook drush_zz_fix_ownership_post_provision_verify	
sudo: no tty present and no askpass program specified

Worked it out. For whatever reason, running the sudo bash scripts/install.sh in each of these submodules resulted in copying the scripts to /usr/local/bin/ and setting the correct file ownership and permissions, but did not add the corresponding entries to sudoers. So I manually added the following to my sudoers file and now it works as advertised.

aegir ALL=NOPASSWD: /usr/local/bin/fix-drupal-platform-ownership.sh
aegir ALL=NOPASSWD: /usr/local/bin/fix-drupal-site-ownership.sh
aegir ALL=NOPASSWD: /usr/local/bin/fix-drupal-platform-permissions.sh
aegir ALL=NOPASSWD: /usr/local/bin/fix-drupal-site-permissions.sh

We ran into this issue while trying to install Aegir 3.9 manually. Since drush hostmaster-install needs to be run as the aegir user, this user is not allowed to run the hostmaster-7.x-3.9/profiles/hostmaster/modules/aegir/hosting_tasks_extra/fix_ownership/scripts/install.sh and hostmaster-7.x-3.9/profiles/hostmaster/modules/aegir/hosting_tasks_extra/fix_permissions/scripts/install.sh scripts.

As a workaround, we first executed the following commands as root user:

cd /root/
drush dl hostmaster-7.x-3.9
sh /root/hostmaster-7.x-3.9/profiles/hostmaster/modules/aegir/hosting_tasks_extra/fix_ownership/scripts/install.sh
sh /root/hostmaster-7.x-3.9/profiles/hostmaster/modules/aegir/hosting_tasks_extra/fix_permissions/scripts/install.sh
rm -fr /root/hostmaster-7.x-3.9/

Afterwards, you can continue installation:

sudo -u aegir -s -H
drush dl --destination=/var/aegir/.drush provision-7.x-3.9
drush cache-clear drush
drush hostmaster-install
[...]

Have just updated 3.9 to 3.10 and now it looks like verify task fix permissions is changing ownership to www-data instead of aegir, so verify and backup tasks fail.

Suggestions on how to fix?

I have a fresh Aegir 3.10 install using deb packages on Ubuntu 16.04, and I confirm that "Fix Permissions" and "Fix Ownership" make verify hosmaster task fail immediately with:

Calling hook drush_zz_fix_ownership_post_provision_verify	
sudo: no tty present and no askpass program specified	
Returned from hook drush_zz_fix_ownership_post_provision_verify	
Calling hook drush_zz_fix_permissions_post_provision_verify	
sudo: no tty present and no askpass program specified	
Returned from hook drush_zz_fix_permissions_post_provision_verify

I can confirm #43 happening to me as well.

Please don't ask questions in a closed issue ... it doesn't show up normally, and thus doesn't get attention.

The first thing to check here is if the sudo settings have been setup correctly by the install.sh scripts. You should have files like /etc/sudoers.d/fix-drupal-platform-ownership

Please open a new issue if you have those and still see this problem.

@helmo: Thanks for the information.

@pmunch: I created an additional issue for this here: #2961009: Sudo Settings not setup correctly in regards to Fix Drupal Platform Ownership