Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I started working on a subcommand that fixes permissions. It really helps after running Git commands, where Git generally messes up file permissions in the Drupal installation directory.
Is there any interest in this?
Currently, there are fixed defaults, but these should be command-line options. So you should be able to do something like this in the future:
drush @site perms set --dirs=0775 --files=0664 --settings=0460 --user=www-data --group=devs
Here's the main part of the code. For the rest, see the attached file.
function drush_perms_permissions() {
// Set some defaults.
$perms_dirs = 0775;
$perms_files = 0664;
$perms_settings = 0460;
$owner_user = 'www-data';
$owner_group = 'devs';
// Get the Drupal root.
if (!$drupal_root = drush_get_context('DRUSH_DRUPAL_ROOT')) {
return drush_set_error(
'ERROR_CANNOT_GET_ROOT',
dt('Cannot determine the Drupal root directory.')
);
}
// Create an iteration of files starting at the Drupal root.
$iterator = new RecursiveIteratorIterator(
new RecursiveDirectoryIterator($drupal_root),
RecursiveIteratorIterator::SELF_FIRST
);
// Iterate through all of the files.
foreach ($iterator as $current_file) {
// Check file type. Is it a regular file or a directory?
if($iterator->isFile()) {
// Act differently if this is a settings.php file.
if ($current_file->getFilename() == 'settings.php') {
// Set desired permissions on settings.php files.
chmod($current_file->getPathname(), $perms_settings);
chown($current_file->getPathname(), $owner_user);
chgrp($current_file->getPathname(), $owner_group);
} else {
// Set desired permissions on all other files.
chmod($current_file->getPathname(), $perms_files);
}
} else if ($iterator->isDir()) {
// Set permissions on the directory.
chmod($current_file->getPathname(), $perms_dirs);
}
}
}
Comment | File | Size | Author |
---|---|---|---|
#91 | drush-perm-command-990812-91.patch | 54.51 KB | TravisCarden |
#91 | interdiff.txt | 5.34 KB | TravisCarden |
#64 | 990812-perm-command-64.patch | 56.2 KB | greg.1.anderson |
#63 | 990812-perm-command-63.patch | 55.82 KB | greg.1.anderson |
#62 | 990812-perm-command-62.patch | 53.44 KB | greg.1.anderson |
Comments
Comment #1
colanI haven't produced a patch file yet. Some feedback would be nice first!
Comment #2
greg.1.anderson CreditAttribution: greg.1.anderson commentedThis is pretty cool in concept. I don't think you need to iterate every file in the drupal root. Use chmod / chown with -R option to apply a default value, and then go back and 'fix up' items like settings.php that you want to have different permissions.
The script should divide files into categories:
* not readable or writable by the web server
* readable, but not writable by the web server
* readable and writable by the web server
The options to the function should let the user decide who the owner and group is for each of these category. For example, some might want 'root' to own all files, and 'www' to be the group, and files writable by the web server should be group writable, while others might want 'www-admin' to own the files that are not writable by the webserver, and 'www-data' to own the files that the web server can write. But the upshot is, the caller decides the permission policy, and the script decides which files should be readable or writable based on best practices for Drupal.
The *.txt files at the root of the Drupal directory should not be readable by the webserver.
If moshe doesn't want this in drush core, I'll take it in drush extras (with the above modifications).
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedThat looks pretty awesome and worthy of drush core. I like Greg's suggestions. I’d like to see a validate hook which throws error on OS that is not supported (i.e. Windows). Also, there is some overlap between this and security review module. See #698002: Drush powered security check of file permission setting which was moved to that module. Not a big deal, just want folks to be aware.
Comment #4
colanTo quash the overlap, I suggested that Security Review handle the security policy checking, while we handle the setting of permissions over here.
Comment #5
greg.1.anderson CreditAttribution: greg.1.anderson commentedI like the idea of having the security review module both check and correct file permissions. However, there has been no patch over there in almost a year of discussion, so I'd be in favor of growing a set-only permissions script here. If it matured to the point where it was ready to also do a security review per the above-quoted issue, then it could migrate at that point.
Comment #6
colanAs far as I know, there's no way to do this natively in PHP. The PHP "chmod" command doesn't offer a "-R" like "chmod" does on the command line. (See http://php.net/manual/en/function.chmod.php if you don't believe me!) I thought about using "exec" or "system", but my understanding is that these are frowned upon, and it also felt a little bit too easy. ;)
Comment #7
greg.1.anderson CreditAttribution: greg.1.anderson commentedI was reading your code too fast, and my brain interpreted it as exec('chmod ...'). You are right, your way is better... although the main reason it would be better is that the php native method is more likely to be cross platform, and that is perhaps not so important since Windows is not going to support chmod anyway (except under cygwin, which will also support exec).
Either way, I guess I'm indifferent on that point.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedI don't think cross platform is possible here so i'd be fine with drush_shell_exec() or drush_op('system', 'chmod ....'). either way.
Comment #9
colanHopefully I'll get to this soon. :)
Comment #10
greg.1.anderson CreditAttribution: greg.1.anderson commentedn.b. Recently (sometime after #8)
drush_op('system', ...
was deprecated in favor of the new function, drush_op_system. See comments in drush.inc.Comment #11
jmlane CreditAttribution: jmlane commentedSubscribing as I am interested in the concept of an embedded command in the Drush scripts that set file permissions, considering the Drush documentation does not mention what permission scheme you should be using to get Drupal and Drush to play nice after install.
Comment #12
carole CreditAttribution: carole commentedWhen I run pm-download on Windows 7, Drush creates readonly folders (or perhaps perms changed when I unsucessfully ran dbupdate). Is this going to cause issues running Drupal? Please include Windows considerations in your patch, thanks.
Comment #13
greg.1.anderson CreditAttribution: greg.1.anderson commentedCreated #1156862: Handle file system permissions correctly on Windows.. The 'fix permissions' command described here should use the new functions suggested there.
Comment #14
colanI'm going to unassign this for now. Unfortunately, I won't have a chance to work on this for a while. (i.e. I don't want to stop someone else from getting to it first. ;)
Comment #15
rfayDrush has enormous permissions problems, almost insoluble, and we all have to debug them time after time.
Had a long conversation with msonnabaum about this tonight in IRC.
His favored technique is to add the drush user to the webserver user group (for example, add the user to the www-data group in /etc/group).
grendzy also mentioned setting the group sticky bit on sites/default/files, which also prevents quite a lot of trouble.
However, we still have *loads and loads* of problems.
Essentially any drush command that causes Drupal to create files will create them with the wrong user, and they'll then be inaccessible to the webserver user. Examples of drush commands that can create files via Drupal are enable, uninstall, cc, cron. Classic failures include
drush en backup_migrate
which creates a sites/default/files/backup_migrate directory which the webserver cannot access.drush en css_injector
for the same #fail.drush si
which creates a sites/default/files/styles which the webserver can't put styles into.drush cc all
which may do many things but routinely fails to delete css and js aggregation filesdrush cron where any hook_cron() creates files (and in some cases where it fails accessing them)
msonnabaum suggested that after key commands (cron, si, en, cc) have a post-hook that does a chmod -R ug+rw (or perhaps ugo+rw) on the files directory (and probably the private directory). That would be a major step forward. It's not perfect, but it would solve a LOT of these.
IMO this is a fundamental drush issue that we've been living with (and debugging regularly) for way too long (like the entire history of drush).
Anyway, the technique outlined in this issue could be deployed to do this, I think.
Thanks for listening,
-Randy
Comment #16
jonhattanAll of the points in #15 are easily fixable by using chmod +s, umask 002 for the webserver user and/or ACL defaults... as far as drush implements move as copy-and-delete.
Related issues: #1168812: Respect filesystem permissions, #1190712: `drush up drupal` inconsistency when used by a non privileges user and permissions are not well set
Comment #17
jonhattanFor reference, there's a drupal_chmod
http://api.drupal.org/api/drupal/includes--file.inc/function/drupal_chmod/7
Comment #18
greg.1.anderson CreditAttribution: greg.1.anderson commentedI described my strategy for handling permissions with drush in issue #1169778-#16. Basically, it is easier if the web server does not own the files, but instead uses group permissions where write access is necessary. chmod +s can help out a lot too.
I think some good points are raised in #15, though, and feel that a patch here would be welcome.
Comment #19
greg.1.anderson CreditAttribution: greg.1.anderson commentedGood blog post on this subject: http://randyfay.com/node/114
Comment #20
bibo CreditAttribution: bibo commentedI've encountered weird css issues on too many sites. Eventually I found that "drush cc all" messing permissions is often the cause. It's not so easy to notice because the problem might become visible only weeks after drush setting wrong css/js/tmp permissions. For example during a cron run or when clearing caches the old way.
Also, thanks for the link: http://randyfay.com/node/114 a good article indeed. (Also: following this issue).
Comment #21
colanHere's an actual patch! Other than some minor changes to my initial post, it now takes user and group arguments.
Still needs the following:
Comment #22
colanThis took longer than I thought, but here it is.
Comment #23
colanThere is now far less ambiguity as it forces callers to explicitly state the the web user and the group.
Comment #24
greg.1.anderson CreditAttribution: greg.1.anderson commentedThanks for working on this. Note that in general, it is better to make your Drupal files owned by some user other than the user that the web server runs under. Otherwise, code that the web server runs could just chmod the files before writing to them. I usually make my files chgrp www-data, and chmod g+w on files that the web server should be able to write. See Securing file permissions and ownership. It would be good, though, to allow the caller to decide what permissions schema should be used to secure the site; see #2, above.
Comment #25
jonhattanI'd prefer to
drush_shell_exec('sudo chown...')
instead ofsudo drush perms ...
. This way per-user defined config, aliases, ssh keys,... will continue working.Comment #26
colanThanks for the feedback folks! I'll try to find some more time to get this nailed down.
Comment #27
greg.1.anderson CreditAttribution: greg.1.anderson commentedI don't think that
drush_shell_exec('sudo chown...')
is the right approach. On my production sites, I tend to make the files owned by a special user, www-admin; some users on the system can sudo -u www-admin, but have no access to root. On my dev sites, I tend to make the files owned by my own account, so the sudo is not necessary at all. While these issues could be solved by adding a sudo option to perms (e.g. --sudo for root, --sudo=www-admin for some other user), historically thesudo
has been left out of Drush, and per-user config has remained something that the individual user must solve independently.Comment #28
colanComment #29
greg.1.anderson CreditAttribution: greg.1.anderson commentedThis is getting really close; looks pretty useful. Some comments:
1. I think that the behavior of the arguments are right, but should be named differently. For example, on my system, I make all files group www-data, and chmod g+w those places the web server can write. In this setting, all files are owned by www-admin, so calling the third parameter "web user" is a misnomer. In chmod parlance, 'u' means 'file owner' and 'g' means 'file group' (see man chmod), so "user owner" and "group owner" is a little confusing. I think that the first two arguments should be called "default file owner" and "default file group", and the third argument should be called "file owner for writable files".
2. Per #1, the first two arguments should be required, and the third should be optional, defaulting to the same user as specified by the 'file owner'.
3. --fs-files should be named --writable-files. The code should also make temp and private writable by the webserver, and so on for any other directory that may need to be writable in the future.
4. Default permissions should be:
--docs=0600
--readonly-files=0644
--settings=0440
--writable-files=0664
--readonly-dirs=0755
--writable-dirs=0775
This reflects my bias that the g bit should be used to control write access to files in the site. Optionally, perhaps another flag could be provided that would set the default perms to be group-writable for every default except --settings. This flag could be used by folks who like to make the files that should be writable by the web server to be owned by the web server, and use group-writable for other purposes, such as write-access for an admin or dev group.
5. If possible, the default value for --readonly-dirs should be (($readonly_files & 0444 >>2) | $readonly_files) -- that is, set the x bit wherever the w bit is set in --readonly-files. Same for --writable-dirs.
Beyond these semantic issues, the code looks good. It would be good to have a couple of test cases.
Comment #30
moshe weitzman CreditAttribution: moshe weitzman commentedComment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedjust adding a big +1.
the config-edit command really needs this, so that after we're done editing a file, we can make sure the webserver user can still write to the file.
Comment #32
JordanMagnuson CreditAttribution: JordanMagnuson commentedThis looks great.
Just wanted to say that I think there should be an option to omit specific sub-directories. Specifically I'm thinking of sites/default/files... in my case, I'm using s3fs for this directory (to store all user generated files on Amazon s3), and so I generally want to leave it alone when doing any permission changes (since s3 gets hit for every permission change in that directory).
Comment #33
JordanMagnuson CreditAttribution: JordanMagnuson commentedTried #28, and it seems to be working pretty well so far.
I'm no expert, but from my limited knowledge I agree with Greg on default values, which seem to be more in line with, for instance http://drupal.org/node/244924
Comment #34
Owen Barton CreditAttribution: Owen Barton commentedThanks for working on this - certainly this is a much needed command!
The terminology in this patch seems to presume that the web server and web php processes run as the same user - however this is not the case when a suexec (or suphp and a handful of other variants, including php-fpm configurations) is being used. This is a very common (I would say required) configuration on shared hosts - these range from the typical low-end shared hosts to high end enterprise clusters that are shared by multiple sites. It is worth bearing in mind that in this setup that the web php process may be the same user as the command line (drush) user, or may be a 3rd account.
Reading the patch from the point of view of the web server and web php processes being different users leads to some confusing and inconsistent language. For example "need to be readable and writable by the web server" - obviously this means the php process (we don't really want webservers writing files, it's just that sometimes the user is the same). I am not sure the best way to resolve this, but I think being clear on who the "actor" is in each case would be an improvement.
Also, in Drupal 7 there is the file_chmod_directory and file_chmod_file variables that denote how new directories and uploaded/generated files should be chmodded. I think we should consider using these as defaults for --writable-dir and --writable-file if no others are specified - however, we need to consider that when these are used the owner will often be the php process, rather than the drush user which might lead to incorrect results (having trouble thinking of a scenario!).
The default values are too broad and potentially insecure for many cases (see http://drupal.org/node/203204#comment-1088283 for some examples), but I don't think we can do much better without the user specifying (and knowing!) better ones or using some heuristic to determine these. I described what I think would be the ideal approach in http://drupal.org/node/698002#comment-2531810 although I haven't had time to implement that. That comment also has some alternate naming for the "categories" that might be useful.
I also have some concerns with performance - has anyone tried running this on a large site (10k to 1M files, perhaps)? It would be interesting to see both runtime and memory usage. My gut feeling is that this may be too slow or memory intensive to be feasible on these kind of sites - if this is the case, we may want to look at using a series of find/xargs/chmod commands (along the lines of "find "$dir" -type d ! ( -name CVS -o -name .svn ) ! -perm 711 -print0 | xargs -0r chmod 711 --" - not that this ignore directories with permissions already correctly set). Not sure if that approach is available on Windows though.
Running this as root is pretty risky - how sure are we that no files within the webroot ever get included (in fact, you might include any files in user directories, including ~/.drush, since those may be writable by the site in some cases)? I am not sure how this can practically be secure unless drush core is in a location (e.g. a systemwide root user install) that is not writable by php processes (web or cli, since the cli also runs against live sites). If not, then this opens up a serious attack pathway, particularly as admin (bad) first instinct on finding a compromised site may be to run this command. It strikes me that being able to run this (albeit in a more limited way) as a non-root user might be quite useful. We could also somewhat reduce the risk to root by only running the specific commands that need it with sudo (i.e. chown), rather than all commands.
Comment #35
greg.1.anderson CreditAttribution: greg.1.anderson commentedI agree with most of the comments in #34 (e.g. use 'php process' as a more accurate descriptor than 'web server'), with a couple of exceptions.
I think that it should be rare for these commands to be run by the php process. It should for the most part be run as the Drush user, and furthermore, risky or no, it will often be executed via
sudo drush permissions ...
, since to do otherwise would presume that the files were already owned by the correct user, and only the group and permissions need to be adjusted -- which may indeed be the case for some users, but not always. Perhaps the command should take care to call chown unless an owner is specified (didn't review last patch to see if it maybe already works like this...)(n.b. only root + the owner of a file can change its permissions, and only root + the owner can change the group. The user changing the group must also be a member of the target group. Only root can change the owner of a file.)
When changing permissions manually, I usually use
chmod -R g+w ...
on the directories that must be writable by Drupal. This avoids the need to specify directory and file permissions separately.chmod -R
andfind
would, I am sure, be many times faster than iterating over every file as in #28. It would be a little tricky to also support skipped directories per #32 with this technique, but possible, I think. This would make --simulate easy to support, but -s would no longer be an audit feature, as the output would just be a sequence of chown -R / chmod -R commands.Comment #36
Owen Barton CreditAttribution: Owen Barton commentedJust to clarify - I wasn't suggesting this command would be actually run by the (web) php process, just that if it is run in a context where files that the web php process can write could be included (and hence executed), then this opens up a pretty significant surface for potential privilege escalation attacks. So for example, if a site is misconfigured or already compromised, and an attacker has been able to write a foo.drush.php file somewhere that Drush will find it (or been able to write to drushrc.php etc) then they will gain root access to the system.
This kind of vulnerability could (via a more tangential mechanism) be exploited even if the attacker has much more limited access - for example, if they only have access to write using the php input format to the database, then they could put code in there that detects when Drush is being run, and uses those (command line) privileges to write to drushrc.php a further attack that is triggered when run as root. This might sound unlikely, but attackers/researchers have pulled off far far less likely sounding tricks in the past :)
If this command used a drush core that was owned by root (or another user separate from web sites), and didn't include any files from web user directories, then this particular attack is much less of an issue. Also, running only the specific chown commands (if needed) with sudo and careful argument validation would harden this - not against a writeable drush core, but it seems it would make attacks like the above much harder.
chmod -R (at least versions we have used) does touch every file, even if it already has correct permissions. I think this would be slower than find (which can exclude files with the correct permissions from the list) and also has repercussions for timestamp based incremental backups (the files get backed unnecessarily).
Actually, I think the way the find commands are written (to only find files/directores that don't match the desired permissions) all we would need to do would be to chop off the xargs/chmod part and we would see a list of files that would be effected.
Comment #37
greg.1.anderson CreditAttribution: greg.1.anderson commentedI see. It does sound like 'find' is the way to go. The other advantage of 'find' is that it would be easy to drop in an exclude term to avoid S3 directories, etc.
Regarding sudo, while it is usually not considered appropriate for Drush to call sudo, perhaps this is one instance where it would be better to have Drush handle that, controlled by a commandline option, so that privilege escalations happen at the last moment, when the 'find' command is called. sudo, find and chmod / chown should all work fine on Windows under mingw / msysgit, which I think would be a reasonable enough restriction for this command.
Comment #38
Owen Barton CreditAttribution: Owen Barton commentedI think Drush calling sudo in a controlled manner is better than the entirety of Drush running under sudo. One thing that did occur to me that would eliminate the sudo threat is just outputting the sudo commands (by default, or perhaps just when running --dry-run or a similar option) and paste them yourself into a different terminal, preventing any sneaky attacks where you are accidentally sudoing an attackers command. Also, some systems use su instead of sudo, so this would allow you to paste into a su'ed terminal instead. Obviously you would need to pause or rerun to chmod the files after the chown is complete.
Comment #39
greg.1.anderson CreditAttribution: greg.1.anderson commentedPerhaps the command could be written to generate a script to execute. If the script commands are stored in an array of strings, then at the very end Drush could either exec each one, or just print it if running in --dry-run mode (or special-case -s for this command so that it emits only executable commands). That way, folks who want to review and execute in another terminal could easily do so. The script could contain both the chown and chmod commands, so there wouldn't be a need to run it twice. Cli options to exclude chown or chmod commands could be provided, if desired, so that folks could run chown sudo'ed, but chmod with normal privs, if desired.
Comment #40
greg.1.anderson CreditAttribution: greg.1.anderson commentedFWIIW, There is a script at http://drupal.org/node/244924#comment-6499054 that uses 'find' as suggested in #34
Comment #41
greg.1.anderson CreditAttribution: greg.1.anderson commentedReworking this per recent comments.
Comment #42
greg.1.anderson CreditAttribution: greg.1.anderson commentedUpdated help text. Edit: Updated help text again for patch in #64, below.
Comment #43
greg.1.anderson CreditAttribution: greg.1.anderson commentedPrototype implementation based on 'find' command per #34. So far only does basic chmod on files and dirs from the root; additional heuristics for other folders and file types still need to be added in (see copious TODOs in patch). For all that it is incomplete, this patch is still smooth and super-fast. It shouldn't be too hard to extend it to dynamically drop additional conditions into the various 'find' commands as needed.
Comment #44
greg.1.anderson CreditAttribution: greg.1.anderson commentedStill needs a test, still needs to implement the complete hook, but does just about everything else. Only lightly tested, but seems to do its thing.
These 'find' scripts are really fast. It takes longer to bootstrap the Drupal site to get %files and %private than it does to set the permissions on it. If you're going to change permissions a lot, you can use the --script command to output just the script; if you save that somewhere, you can run it again and again without Drush.
Comment #45
JordanMagnuson CreditAttribution: JordanMagnuson commentedWow, that look great Greg--thanks for all the work!
Briefly scanned #44, and it looks like the option to omit a directory is not yet implemented... correct?
Comment #46
greg.1.anderson CreditAttribution: greg.1.anderson commentedI forgot to implement --exclude, but it was easy to add.
Comment #47
JordanMagnuson CreditAttribution: JordanMagnuson commentedThanks Greg!
Tried out #46, and it seems to be working, though I'm receiving some warning messages.
On a standard run of drush perms my-user:www-data:
And when I run drush perms my-user:www-data --exclude=some/directory I get an extra warning:
These warnings don't seem to be interfering with the execution of the script, but I thought you should know.
Comment #48
JordanMagnuson CreditAttribution: JordanMagnuson commentedI also noticed that the --exclude option is not taken into account when using --audit (audit will show files and directories that are meant to be excluded).
Comment #49
JordanMagnuson CreditAttribution: JordanMagnuson commented--exclude seems to be respecting files, but not directories.
tried messing permissions up, then running drush perms my-user:www-data --exclude=sites/default/files
files inside that directory kept their messed-up permissions, but all directory permissions were changed (fixed).
Comment #50
greg.1.anderson CreditAttribution: greg.1.anderson commentedFixed #47, but not #49. Seems that files simply are not changed at all, as in #46, --exclude is not respected. Will post a new patch when I have more problems worked out.
Comment #51
greg.1.anderson CreditAttribution: greg.1.anderson commentedIt turns out that correctly generating compound
find
commands is more complicated than I thought. The -prune command in particular is a little tricky. I think I've about got it with this patch. Still haven't had time to thoroughly test, but posting it here for early reviewers.Comment #52
JordanMagnuson CreditAttribution: JordanMagnuson commentedMaybe I'm doing something wrong, but --exclude still doesn't seem to be working for me.
drush perms my-user:www-data --exclude=sites/default/files
Files and directories there are still having their permissions changed. Is my syntax correct? (I Also tried absolute path to files directory).
Let me know if I can post anything more helpful.
Comment #53
greg.1.anderson CreditAttribution: greg.1.anderson commented--exclude only affects those find commands that start at $base_dir. You are excluding sites/default/files, but --files defaults to '%files,%private', so perms are set there. I should figure out a good way to intelligently update the default value for --files. For now, add --files='' and it should work. I did not try my suggestion, so no guarentees. If that does not work, run with --script and you'll be able to see what commands Drush is building.
Comment #54
JordanMagnuson CreditAttribution: JordanMagnuson commentedAh, okay, took a look at the --script output, and that helps to understand what's going on.
Here's the base output of drush perms my-user:www-data (minus the variable assignments):
Here's the output with --exclude=foo/bar
So I can see that the files directory is being handled separately after the exclusions. However, adding --files='' doesn't seem to solve my issue.
Here's the output of drush perms my-user:www-data --exclude=sites/default/files --files=''
So that doesn't seem to work.
Furthermore, I can't seem to get --exclude to work at all, even outside of the files directory. For instance, when I try running drush perms my-user:www-data --exclude=misc which outputs a script that looks reasonable to me, the [base_dir]/misc directory is still having permissions changed. Here's the --script output for that one:
Comment #55
JordanMagnuson CreditAttribution: JordanMagnuson commentedAlso, when I try to run the script that is generated via --script, I get errors:
drush perms magjor:www-data --script >perms.sh && chmod +x perms.sh && sudo ./perms.sh
Here's perms.sh in its entirety (using the php tag for syntax highlighting convenience):
Comment #56
omega8cc CreditAttribution: omega8cc commented$SETTINGS='0440'
- is it literally like that in the script? You don't declare variables that way in bash..Comment #57
JordanMagnuson CreditAttribution: JordanMagnuson commentedThat's what I thought. But yes, what I pasted is the exact output from drush perms to perms.sh (minus the php tags).
Comment #58
omega8cc CreditAttribution: omega8cc commentedIt should be just:
Not sure about the rest of the script correctness.
Comment #59
greg.1.anderson CreditAttribution: greg.1.anderson commentedHere is an updated patch with a bunch of (passing!) tests and many bugfixes for the problems described above and quite a few more. Test coverage is pretty good, but not complete; there may still be some bugs. Setting permissions, when combined with the requirement to exclude directories, avoid touching files that are already correct, and minimizing file touches, turns out to be a much more complex problem than originally anticipated.
I did not implement the complete hook because there is a problem here. The arguments for the perm command are "user:group", just like chmod. In order for this form of argument to work with the Drush complete hook, I would need to output a list containing all of the permutations of all users and all groups on the system, which is not desirable. To fix, the arguments could be changed to "user group" instead of "user:group"; however, I think that the usability of the later is much better, since it allows "user" to be defaulted to "user:user" (group name == user name) and generally makes it a lot easier to specify the up-to-four user/group args that may be provided. Extending the Drush complete code to allow independent completion of user:group would be quite a bit of work; I'm not even sure exactly how tractable it would be to do that.
Finally, I think that the option names could use a thorough review. I am currently calling the user:group pairs the "default" owner/group (for most files) and the "user" group/files (for the mostly-user-generated content in %files), but I'm not sure this is best. The words "user" and "settings" etc. are somewhat overloaded. I'll make another pass over it and see if I can improve it, but feedback would be welcome. I updated #42 to contain the latest help text, as output from the patch on this issue.
Comment #60
JordanMagnuson CreditAttribution: JordanMagnuson commentedThat looks great Greg! Thanks for all your work on this--it's obviously turned out to be a very complex issue!
Your last patch has been working brilliantly for me. The only issue I discovered is if I try to exclude a directory within the files directory. Say:
drush perms www-admin:www-data --exclude=sites/default/files/some-folder
Obviously this has gotten terribly complex, and not sure how easy it would be to add this special case. I think the ability to exclude the files directory as a whole is more important (and working for me), though it may be nice to be able to specify a sub-directory there...
Comment #61
kalabroTested a little bit and it works. But I get "find: paths must precede expression: ./*/*" line when run it with --sudo option.
Comment #62
greg.1.anderson CreditAttribution: greg.1.anderson commented#61 was not caused by --sudo, but instead was caused when
--dir=.
was used. This patch fixes #60 and #61.Comment #63
greg.1.anderson CreditAttribution: greg.1.anderson commentedGreatly improved options and help text.
Comment #64
greg.1.anderson CreditAttribution: greg.1.anderson commentedRename to 'core-permissions' / permissions.core.inc.
Comment #65
moshe weitzman CreditAttribution: moshe weitzman commentedSome people are going to confuse core-permissions with drupal's permissions, not file perms. Not sure the best way to clarify that.
Unneeded perms_drush_command() function.
Any chance we can use Drush_UnitTestCase instead of Drush_CommandTestCase?
I'd love input as to whether this goes into core or Drush extras. It is a ton of code, and I'm not too inclined to maintain it myself.
Comment #66
greg.1.anderson CreditAttribution: greg.1.anderson commentedPerhaps
drush core-chmod
would be a better name? This would avoid confusion with Drupal permissions; there is the issue that this command does both chown and chmod operations, but folks could figure that out pretty quickly by reading the command help, and I think it's fairly likely they'd be able to figure out to look at 'chmod' if looking for 'chown'.I might be able to write a couple additional tests that use Drush_UnitTestCase, but the Drush command calls drush_get_option in a bunch of places, plus_drush_core_directory to evaluate %settings, %files, etc. I'd have to do further refactoring of the command implementation to make this possible; I don't think it's worth it.
I'm happy to maintain this code in Drush Core, in the Security Review module, in Drush Extras, or wherever it seems to fit best. I'm partial to putting it in core, but other places mentioned would also work.
Comment #67
moshe weitzman CreditAttribution: moshe weitzman commentedcore-chmod works for me. you might want to wait until we resolve the 'where does this command live' question.
Comment #68
kalabro@greg.1.anderson, I confirm that problem from #61 is fixed. Works like a charm. Thanks a lot!
About naming: I personally think that chmod will be better name because it is Unix-specific stuff.
Comment #69
Owen Barton CreditAttribution: Owen Barton commentedI like using "chmod" in the name here, since this matches the naming of the core variable (which went through the exact same discussion). Chmod as a concept is pretty well known (although not well understood, of course!) and clearly relates to files only. Even though it is not perfectly precise (since this includes chowning) I think it is clearer than a "perm" name given the overlap with Drupal permissions. Something like "fileperms" might be clear also (although I think it feels less intuitive than chmod).
Comment #70
colanIt's possible that "fileperms" could be confused with files specific to the "files" directory. What about combining the two into "chmown"?
In any event, @greg.1.anderson, thanks for taking this over as I really didn't have time to work on it. :)
Comment #71
greg.1.anderson CreditAttribution: greg.1.anderson commentedI still favor core-chmod. One additional thought I had about making this clearer was to alias this command to both chmod and chown. In this scenario, regardless of what alias you call it via, it still may do either chmod or chown operations, depending on the other parameters and options used. This still feels fairly natural and discoverable to me, since this concept could be explained in the first sentence of the help text..
Comment #72
kalabro@greg.1.anderson, @mosheWeitzman, could we help you somehow? This new Drush feature is really amazing, but status is still "needs work".
Comment #73
greg.1.anderson CreditAttribution: greg.1.anderson commentedIMHO, this feature is RTBC except for a trivial name change. The name change is postponed waiting a decision on whether this feature is acceptable for inclusion in Drush core, or should be committed somewhere else. c.f. #66. Moshe is waiting for input from other Drush maintainers.
Comment #74
jonhattanI'm fine with
core-chmod
in drush core. I've not had the chance reviewed the code in detail but it seems to me it's not hard to maintain.Comment #75
msonnabaum CreditAttribution: msonnabaum commentedI'm on the fence about the functionality, but the implementation is pretty damn complex.
It seems like we could come up with a simpler version that supported far fewer options that would be easier to maintain. Options like doc-patterns seem like they'd very rarely get used. I'd much rather wait till someone asks for that specifically before making the code more complex by anticipating the need.
Also, using find and xargs feels like a premature optimization to me. If what we have is bulletproof, then fine, but I'd worry about the compatibility across distros. For example, xargs -0r seems to give me "xargs: illegal option -- r" on OSX, so that's also likely the case on *BSD.
Comment #76
greg.1.anderson CreditAttribution: greg.1.anderson commentedThe xargs -r flag is a bit of a problem, but it is solvable. From man xargs on FreeBSD:
To summarize: -r is required on GNU systems, is optional on some versions of BSD, and prohibited on other versions of BSD, including MacOS. Windows under Mingw also supports the -r flag, but of course DOS and Powershell have neither find nor xargs, and therefore are not supported. (I tried the command under Windows 7, and found it had some minor operational defects, but that the find / xargs commands generated run just fine. I'll fix the Windows problems if we commit this to Drush core.)
We could work around the xargs -r problem by running
xargs --help
once first. (--help causes an error on *BSD, but the help text is still printed, so it's all the same). Ifxargs [...r...]
is found (regex is more likexargs \[[^]*r
), then we have GNU xargs, and must include the -r; otherwise, we leave it off.Now, on to the issue of complexity. There is already a bash script on the page Securing file permissions and ownership that is simpler and more maintainable, and of course it is easy to type
chown -R www-admin:www-data . && chmod -R g+w sites/default/files
and get your permissions to be 90% right in a hurry. If we remove too much functionality from the command, than it becomes no better than the alternatives, and then is not much worth having at all.If the primary concern with complexity is comprehensibility of the command, then I'd be happy to hide the options --settings-files, --settings, --not-world-readable, --not-group-writable, --no-variables, --files, --doc-patterns and --doc-exceptions. This would make the help text a lot shorter, but of course wouldn't do anything about the complexity of the implementation. I admit that this command's implementation is complex -- more so than sql-sync, for example -- but it is similar in implementation and complexity to Drupal 7 render arrays. That is perhaps not the best comparison one could hope for, but I think it's a manageable amount of code and will maintain it.
We can commit it somewhere else if desired, but this is such an oft-requested feature that I think it would be really good to have in core. Most folks should be able to get correct results pretty easily by reading just the first three examples in the help text, so I think it would be used a lot and save a bunch of folks some time.
Comment #77
JordanMagnuson CreditAttribution: JordanMagnuson commentedMy 2 cents from a user's perspective:
1. I think this is a badly needed feature that would make sense to put in drush core. For anyone using git / a staging server / etc, fixing permissions is a really vital concern, and until this script came along, was the single biggest confusion for me when it came to using drush.
2. I think the current implementation works really well, and is easy to understand (from the end user perspective; examples are clear, etc.). I've used the bash script at Securing file permissions and ownership, and I like this drush command much better, due to the added flexibility/power with the various options. Like Greg said, if this command gets dumbed down, then it becomes no better than alternatives.
3. Greg's a hero :)
Again, just my 2 cents. Or 3, I guess :)
Comment #78
greg.1.anderson CreditAttribution: greg.1.anderson commentedI'd like to commit this by BADcamp. Should I put it in drush extras? I'd still prefer to see it in core.
Comment #79
moshe weitzman CreditAttribution: moshe weitzman commentedWe should discuss it at BADCamp. If you want to commit beforehand, you can use a new branch of drush core or commit to Drush extras.
Comment #80
greg.1.anderson CreditAttribution: greg.1.anderson commentedSounds reasonable; I'll just put off committing until after the discussion.
Comment #81
kalabroSome problems after testing. I specifically set 777 permissions to files folder to test Drush core-permissions:
Note that my settings.php has permissions 440.
I try to fix my permissions with drush perms:
or
With or without --sudo work isn't done:
My settings.php now has 644 permissions, but I didn't want this to happen.
--sudo=all fixes all problems.
Is this workflow is correct? I found it quite strange.
Comment #82
greg.1.anderson CreditAttribution: greg.1.anderson commentedIt is expected that you should get 'operation not permitted' for chown without sudo. You must be root to chown. You may chgrp as an unprivileged user if a) you own the file, and b) you are a member of the destination group.
It is expected that you should get 'operation not permitted' for chmod without sudo if there are files that you do not own. n.b. --sudo means 'chown operations only' and --sudo=all means 'both chown and chmod operations'. Run with --pipe to see the script.
I can't explain why settings.php has 644 when ran without --sudo, if it started with 440 before. Seems that the chmod would either work, in which case it should get the desired permissions, or fail, in which case it should keep the perms it had before.
Comment #83
greg.1.anderson CreditAttribution: greg.1.anderson commentedRegarding #81, I think it might be a good idea for this command to print a longer and more detailed error message when 'operation not permitted' results. Permissions issues are clearly confusing for many users.
Comment #84
kalabrocontinue to test: #1828212: Check private files variable 'file_private_path' correctly
One more question: why we have "440 code-owner code-owner" permissions for settings.php? I have to do
chgrp wwwdata settings.php
manually because web-server can't read settings.php.Comment #85
JordanMagnuson CreditAttribution: JordanMagnuson commentedSo I've created a script via --script, which is working great. I'd like to keep it at my site root (/public_html), but lock it down so only root/sudo can run it.
The only issue is that when I run the script, it changes the permissions of the script file itself (perms.sh)... tried generating the script with --exclude=perms.sh, but that doesn't seem to work.
Any thoughts?
Comment #86
greg.1.anderson CreditAttribution: greg.1.anderson commentedMaybe you could keep your script outside of the site root. I like to structure my Drupal sites such that the site root is at /srv/www/mysite.org/htdocs; then I can put stuff unrelated to the files that should be served up by the webserver at /srv/www/mysite.org.
Comment #87
vinmassaro CreditAttribution: vinmassaro commentedLooking forward to seeing this in drush. We are looking to clean up sites/default directory permissions.
Comment #88
greg.1.anderson CreditAttribution: greg.1.anderson commentedAt BADCamp we discussed a list of changes to simplify the set of available options here; once I have time to implement these suggestions, this should be ready to go in.
Comment #89
greg.1.anderson CreditAttribution: greg.1.anderson commentedSide note: one of the suggestions made was to default directory permissions to be calculated from file permissions, by setting the 'x' bit to the value of the corresponding 'r' bit. Puppet also does this; c.f. 644 = 755 For Directories.
Comment #90
sonicthoughts CreditAttribution: sonicthoughts commentedThis would be enormously helpful. Note: we have some sites running PHP as the owner of files and others as nobody.
Comment #91
TravisCarden CreditAttribution: TravisCarden commentedA couple of points of feedback:
*.sh
files with a--shell-scripts
octal permissions option?Comment #92
greg.1.anderson CreditAttribution: greg.1.anderson commentedThis issue was marked
closed (won't fix)
because Drush has moved to Github.If this feature is still desired, you may copy it to our Github project. For best results, create a Pull Request that has been updated for the master branch. Post a link here to the PR, and please also change the status of this issue to
closed (duplicate)
.Please ask support questions on Drupal Answers.
Comment #93
vinmassaro CreditAttribution: vinmassaro commentedSwitching status to closed (duplicate) since I've reopened this with a pull request here: https://github.com/drush-ops/drush/pull/158