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);
    }
  }
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

colan’s picture

Status: Active » Needs review

I haven't produced a patch file yet. Some feedback would be nice first!

greg.1.anderson’s picture

Status: Needs review » Needs work

This 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).

moshe weitzman’s picture

That 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.

colan’s picture

To quash the overlap, I suggested that Security Review handle the security policy checking, while we handle the setting of permissions over here.

greg.1.anderson’s picture

I 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.

colan’s picture

I don't think you need to iterate every file in the drupal root. Use chmod / chown with -R option...

As 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. ;)

greg.1.anderson’s picture

I 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.

moshe weitzman’s picture

I don't think cross platform is possible here so i'd be fine with drush_shell_exec() or drush_op('system', 'chmod ....'). either way.

colan’s picture

Assigned: Unassigned » colan

Hopefully I'll get to this soon. :)

greg.1.anderson’s picture

n.b. Recently (sometime after #8) drush_op('system', ... was deprecated in favor of the new function, drush_op_system. See comments in drush.inc.

jmlane’s picture

Subscribing 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.

carole’s picture

When 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.

greg.1.anderson’s picture

Created #1156862: Handle file system permissions correctly on Windows.. The 'fix permissions' command described here should use the new functions suggested there.

colan’s picture

Assigned: colan » Unassigned

I'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. ;)

rfay’s picture

Drush 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 files
  • drush 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

jonhattan’s picture

All 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

jonhattan’s picture

greg.1.anderson’s picture

I 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.

greg.1.anderson’s picture

Good blog post on this subject: http://randyfay.com/node/114

bibo’s picture

I'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).

colan’s picture

Here's an actual patch! Other than some minor changes to my initial post, it now takes user and group arguments.

Still needs the following:

  1. Reworking to divide files into categories as described in #2.
  2. Validation check for relevant OS.
  3. Probably some other stuff. :)
colan’s picture

Status: Needs work » Needs review
FileSize
8.16 KB

This took longer than I thought, but here it is.

  1. We can't support Windows here until #1156862: Handle file system permissions correctly on Windows. gets anywhere.
  2. I went with chmod() instead of drupal_chmod() because drupal_chmod() requires a full bootstrap, which won't get anywhere if the permissions are off. This subcommand should work with as little overhead as possible in order to set things to a usable state.
colan’s picture

There is now far less ambiguity as it forces callers to explicitly state the the web user and the group.

greg.1.anderson’s picture

Status: Needs review » Needs work

Thanks 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.

jonhattan’s picture

I'd prefer to drush_shell_exec('sudo chown...') instead of sudo drush perms .... This way per-user defined config, aliases, ssh keys,... will continue working.

colan’s picture

Assigned: Unassigned » colan

Thanks for the feedback folks! I'll try to find some more time to get this nailed down.

greg.1.anderson’s picture

I 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 the sudo has been left out of Drush, and per-user config has remained something that the individual user must solve independently.

colan’s picture

Status: Needs work » Needs review
FileSize
8.18 KB
  • It's now necessary to state the user owner, the group owner and the web user (for ownership of the files directory) as arguments.
  • Permissions for each type of file/directory can be passed in as options if one wishes something different from the defaults.
greg.1.anderson’s picture

Status: Needs review » Needs work

This 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.

moshe weitzman’s picture

  1. yes, needs a test
  2. needs to support --simulate. not sure if --simulate should be equivalent to a --dry-run but a way to preview changes before executing them is vital. in fact, i think of this as an 'audit' feature.
  3. Let's implement the complete hook for just a bit more delight.
  4. Lets go through the options and note which have optional values. You would use an array as value. See examples/commands.html for detail.
Anonymous’s picture

just 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.

JordanMagnuson’s picture

This 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).

JordanMagnuson’s picture

Tried #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

Owen Barton’s picture

Thanks 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.

greg.1.anderson’s picture

I 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 and find 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.

Owen Barton’s picture

I think that it should be rare for these commands to be run by the php process.

Just 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 and find would, I am sure, be many times faster than iterating over every file as in #28.

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).

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.

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.

greg.1.anderson’s picture

I 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.

Owen Barton’s picture

I 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.

greg.1.anderson’s picture

Perhaps 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.

greg.1.anderson’s picture

FWIIW, There is a script at http://drupal.org/node/244924#comment-6499054 that uses 'find' as suggested in #34

greg.1.anderson’s picture

Assigned: colan » greg.1.anderson

Reworking this per recent comments.

greg.1.anderson’s picture

Updated help text. Edit: Updated help text again for patch in #64, below.

Set appropriate ownership and permissions of files and directories within a
Drupal web directory.

Examples:
 drush core-permissions                    Set permissions with "www-admin" as  
 www-admin:www-data                        the user owner and "www-data" as the 
                                           group owner.  "Files" and "Private"  
                                           will be writable by the group        
                                           "www-data"; other files will be      
                                           writable by the owner and world      
                                           readable. Suitable for use in a      
                                           typical site running on a dedicated  
                                           server. The web server user / php    
                                           process MUST be a member of the      
                                           group www-data.                      
 drush perms www-admin:www-data www-data   Set permissions with "www-admin" as  
 --strict                                  the user owner and "www-data" as the 
                                           group owner of the code files, and   
                                           "www-data" as both the owner and     
                                           group of "Files" and "Private".      
                                           Other users not in the www-data      
                                           group will not be able to read any   
                                           files. Suitable for use in a typical 
                                           site running on a shared server. The 
                                           web server user / php process MUST   
                                           be a member of the group "www-data". 
 drush perms bob:devs www-data:devs --lax  Set permissions with "bob" as the    
                                           user owner and "devs" as the group   
                                           owner.  "Files" and "Private" will   
                                           be world-writable.  Members of group 
                                           "devs" will be able to write to all  
                                           files (except settings.php). Other   
                                           files will be world-readable.        
                                           Suitable for use in a typical        
                                           development environment. The web     
                                           server user / php process SHOULD NOT 
                                           be a member of the group "devs".     
 drush perms bob www-data                  Set permissions with "bob" as the    
                                           user and group owner of code files.  
                                           Data files will be owned by          
                                           "www-data". Other files will be      
                                           world-readable.                      
 sudo chown -R www-admin . && drush perms  Only run unpriviledged commands from 
 --skip-set-owner www-data                 Drush.                               
 drush perms --sudo=all                    Instruct Drush to call sudo before   
 www-admin:www-data                        executing priviledged commands.      
                                           n.b. This is preferable to using     
                                           `sudo drush ...`, which gives all    
                                           enabled contrib modules an           
                                           opportunity to run arbitrary code as 
                                           the superuser.                       
 drush perms --pipe www-admin:www-data >   Generate a script and run it via     
 perms.sh && chmod +x perms.sh && sudo     sudo. Even more secure than the      
 ./perms.sh                                --sudo option.                       


Arguments:
 code owner:group                          The user and group that will own     
                                           most directories and files.  For     
                                           security reasons, it is recommended  
                                           that this user should be neither     
                                           "root" (the superuser) nor the web   
                                           user (e.g. "www-data" or "apache").  
                                           Optional; if not specified, forces   
                                           --skip-set-owner and sets            
                                           permissions only.                    
 data owner:group                          The user and group that will own the 
                                           "Files" and "Private" directories    
                                           and files.  Optional; default is to  
                                           use the code owner and group.        


Options:
 --audit                                   Change nothing; only show files and  
                                           folders that do not match requested  
                                           permissions.                         
 --code-dirs=<0755>                        Octal permissions for directories    
                                           that need to be read by (but not     
                                           written to) the web server such as   
                                           directories containing PHP files.    
                                           Optional; default is 0755 or 750     
                                           (strict).                            
 --code-files=<0644>                       Octal permissions for files that     
                                           need to be read by (but not written  
                                           to) the web server such as PHP       
                                           files. Optional; default is 0644 or  
                                           0640 (strict).                       
 --code-files-group=<www-data>             The group who will own the code      
                                           (.php) and documentation (.txt)      
                                           files. Alternative to specifying     
                                           owner via the commandline argument;  
                                           allows setting a group without       
                                           setting the user.                    
 --code-files-owner=<www-admin>            The user who will own the code       
                                           (.php) and documentation (.txt)      
                                           files.                               
 --data-dirs=<0775>                        Octal permissions for directories    
                                           that contain user-uploaded and       
                                           Drupal-generated files that need to  
                                           be readable and writable by the php  
                                           process. Optional; default is 0775   
                                           or 0750 (strict).                    
 --data-files=<0664>                       Octal permissions for user-uploaded  
                                           and Drupal-generated files (files in 
                                           the "Files" and "Private"            
                                           directories) that need to be         
                                           readable and writable by the php     
                                           process. Optional; default is 0664   
                                           or 0640 (strict).                    
 --data-files-group=<www-data>             The group who will own the           
                                           user-provided and Drupal-generated   
                                           files.                               
 --data-files-owner=<www-data>             The user who will own the            
                                           user-provided and Drupal-generated   
                                           files.                               
 --dir=<.>                                 Apply permissions changes at the     
                                           specified directory. Optional;       
                                           defaults to Drupal root.             
 --doc-exceptions=<robots.txt>             Filename pattern of files in the     
                                           top-level directory that should NOT  
                                           be treated like documentation, even  
                                           though they match the pattern given  
                                           in --doc-patterns. Optional; default 
                                           is robots.txt                        
 --doc-files=<0400>                        Octal permissions for documentation  
                                           files such as "INSTALL.txt".         
                                           Optional; default is 0400.           
 --doc-patterns=<*.txt>                    Filename pattern of files in the     
                                           top-level directory that should be   
                                           treated like documentation.          
                                           Optional; default is                 
                                           *.txt,quickstart.html                
 --exclude=<%private>                      Comma-separated list of paths to     
                                           directories that should never be     
                                           touched (e.g. network-mounted shared 
                                           folders). Optional.                  
 --files=<%files,%private>                 Comma-separated list of paths to     
                                           files directories. Optional; default 
                                           is "%files,%private".                
 --lax                                     Make writable files writable by any  
                                           user. Optional; synonym for          
                                           --code-files=0664 --code-dirs=0775   
                                           --data-files=0666 --data-dirs=0777   
                                           --doc-files=664.                     
 --no-variables                            In --script mode, do not assign      
                                           variables; use values directly.      
 --not-group-writable                      Changes the defaults for --data-dirs 
                                           and --data-files to be the same as   
                                           the defaults for --code-dirs and     
                                           --code-files, respectively.          
 --not-world-readable                      Prevent users who are not the file   
                                           owner and not in the applicable      
                                           group from accessing files in the    
                                           webroot.  Optional; defaults to      
                                           world-readable, except for           
                                           settings.php.                        
 --pipe                                    Output a script instead of executing 
                                           the commands.                        
 --settings=<%settings>                    Comma-separated list of paths to     
                                           settings files. Optional; default is 
                                           "%settings".                         
 --settings-files=<0440>                   Octal permissions for settings.php   
                                           configuration files. Optional;       
                                           default is 0440.                     
 --skip-set-owner                          Presumes that the owner of the files 
                                           is already correct, and skips        
                                           setting it. Allows execution by      
                                           unpriviledged user.                  
 --strict                                  Optional; synonym for                
                                           --not-world-readable                 
                                           --not-group-writable. Overrides      
                                           --lax.                               
 --sudo=[all]                              Call sudo before commands that set   
                                           file ownership. If --sudo=all is     
                                           specified, then sudo is also used    
                                           before commands that set file        
                                           permissions.                         


Aliases: perms
greg.1.anderson’s picture

FileSize
17.53 KB

Prototype 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.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
26.75 KB

Still 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.

JordanMagnuson’s picture

Wow, 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?

greg.1.anderson’s picture

FileSize
27.98 KB

I forgot to implement --exclude, but it was easy to add.

JordanMagnuson’s picture

Thanks 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:

find: paths must precede expression: sites/default/files
Usage: find [-H] [-L] [-P] [-Olevel] [-D help|tree|search|stat|rates|opt|exec] [path...] [expression]
find: paths must precede expression: sites/default/files
Usage: find [-H] [-L] [-P] [-Olevel] [-D help|tree|search|stat|rates|opt|exec] [path...] [expression]
Permissions change complete.      

And when I run drush perms my-user:www-data --exclude=some/directory I get an extra warning:

find: paths must precede expression: some/directory
Usage: find [-H] [-L] [-P] [-Olevel] [-D help|tree|search|stat|rates|opt|exec] [path...] [expression]
find: paths must precede expression: sites/default/files
Usage: find [-H] [-L] [-P] [-Olevel] [-D help|tree|search|stat|rates|opt|exec] [path...] [expression]
find: paths must precede expression: sites/default/files
Usage: find [-H] [-L] [-P] [-Olevel] [-D help|tree|search|stat|rates|opt|exec] [path...] [expression]
Permissions change complete.                

These warnings don't seem to be interfering with the execution of the script, but I thought you should know.

JordanMagnuson’s picture

I 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).

JordanMagnuson’s picture

--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).

greg.1.anderson’s picture

Status: Needs review » Needs work

Fixed #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.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
31.6 KB

It 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.

JordanMagnuson’s picture

Maybe 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.

greg.1.anderson’s picture

--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.

JordanMagnuson’s picture

Ah, 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):

find /srv/www/pixelscrapper.com/public_html  -path /srv/www/pixelscrapper.com/public_html/sites/default/files -prune -o \( \( \! -group $DEFAULT_GROUP -o \! -user $DEFAULT_OWNER \) -print0 \) | xargs -0r chown $DEFAULT_OWNER:$DEFAULT_GROUP --
find /srv/www/pixelscrapper.com/public_html  -path /srv/www/pixelscrapper.com/public_html/sites/default/files -prune -o \( -type d \! -perm $CODE_DIRS -print0 \) | xargs -0r chmod $CODE_DIRS --
find /srv/www/pixelscrapper.com/public_html -mindepth 2 -path /srv/www/pixelscrapper.com/public_html/sites/default/files -prune -o \( -type f \! -perm $CODE_FILES -print0 \) | xargs -0r chmod $CODE_FILES --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( \( \! -group $FILES_GROUP -o \! -user $FILES_OWNER \) -print0 \) | xargs -0r chown $FILES_OWNER:$FILES_GROUP --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( -type d \! -perm $USER_DIRS -print0 \) | xargs -0r chmod $USER_DIRS --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( -type f \! -perm $USER_FILES -print0 \) | xargs -0r chmod $USER_FILES --
find /srv/www/pixelscrapper.com/public_html -maxdepth 1  \( -type f \! -perm $DOC_FILES \( -path '*.txt' -o -path quickstart.html \) \! -path /srv/www/pixelscrapper.com/public_html/robots.txt -print0 \) | xargs -0r chmod $DOC_FILES --
find /srv/www/pixelscrapper.com/public_html -maxdepth 1  \( -type f \! -perm $CODE_FILES \( -path /srv/www/pixelscrapper.com/public_html/robots.txt -o \! \( -path '*.txt' -o -path quickstart.html \) \) -print0 \) | xargs -0r chmod $CODE_FILES --

Here's the output with --exclude=foo/bar

find /srv/www/pixelscrapper.com/public_html  \( -path /srv/www/pixelscrapper.com/public_html/sites/default/files -o -path /srv/www/pixelscrapper.com/public_html/foo/bar \) -prune -o \( \( \! -group $DEFAULT_GROUP -o \! -user $DEFAULT_OWNER \) -print0 \) | xargs -0r chown $DEFAULT_OWNER:$DEFAULT_GROUP --
find /srv/www/pixelscrapper.com/public_html  \( -path /srv/www/pixelscrapper.com/public_html/sites/default/files -o -path /srv/www/pixelscrapper.com/public_html/foo/bar \) -prune -o \( -type d \! -perm $CODE_DIRS -print0 \) | xargs -0r chmod $CODE_DIRS --
find /srv/www/pixelscrapper.com/public_html -mindepth 2 \( -path /srv/www/pixelscrapper.com/public_html/sites/default/files -o -path /srv/www/pixelscrapper.com/public_html/foo/bar \) -prune -o \( -type f \! -perm $CODE_FILES -print0 \) | xargs -0r chmod $CODE_FILES --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( \( \! -group $FILES_GROUP -o \! -user $FILES_OWNER \) -print0 \) | xargs -0r chown $FILES_OWNER:$FILES_GROUP --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( -type d \! -perm $USER_DIRS -print0 \) | xargs -0r chmod $USER_DIRS --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( -type f \! -perm $USER_FILES -print0 \) | xargs -0r chmod $USER_FILES --
find /srv/www/pixelscrapper.com/public_html -maxdepth 1  \( -type f \! -perm $DOC_FILES \( -path '*.txt' -o -path quickstart.html \) \! -path /srv/www/pixelscrapper.com/public_html/robots.txt -print0 \) | xargs -0r chmod $DOC_FILES --
find /srv/www/pixelscrapper.com/public_html -maxdepth 1  \( -type f \! -perm $CODE_FILES \( -path /srv/www/pixelscrapper.com/public_html/robots.txt -o \! \( -path '*.txt' -o -path quickstart.html \) \) -print0 \) | xargs -0r chmod $CODE_FILES --

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=''

find /srv/www/pixelscrapper.com/public_html  -path /srv/www/pixelscrapper.com/public_html/sites/default/files -prune -o \( \( \! -group $DEFAULT_GROUP -o \! -user $DEFAULT_OWNER \) -print0 \) | xargs -0r chown $DEFAULT_OWNER:$DEFAULT_GROUP --
find /srv/www/pixelscrapper.com/public_html  -path /srv/www/pixelscrapper.com/public_html/sites/default/files -prune -o \( -type d \! -perm $CODE_DIRS -print0 \) | xargs -0r chmod $CODE_DIRS --
find /srv/www/pixelscrapper.com/public_html -mindepth 2 -path /srv/www/pixelscrapper.com/public_html/sites/default/files -prune -o \( -type f \! -perm $CODE_FILES -print0 \) | xargs -0r chmod $CODE_FILES --
find /srv/www/pixelscrapper.com/public_html/   \( \( \! -group $FILES_GROUP -o \! -user $FILES_OWNER \) -print0 \) | xargs -0r chown $FILES_OWNER:$FILES_GROUP --
find /srv/www/pixelscrapper.com/public_html/   \( -type d \! -perm $USER_DIRS -print0 \) | xargs -0r chmod $USER_DIRS --
find /srv/www/pixelscrapper.com/public_html/   \( -type f \! -perm $USER_FILES -print0 \) | xargs -0r chmod $USER_FILES --
find /srv/www/pixelscrapper.com/public_html -maxdepth 1  \( -type f \! -perm $DOC_FILES \( -path '*.txt' -o -path quickstart.html \) \! -path /srv/www/pixelscrapper.com/public_html/robots.txt -print0 \) | xargs -0r chmod $DOC_FILES --
find /srv/www/pixelscrapper.com/public_html -maxdepth 1  \( -type f \! -perm $CODE_FILES \( -path /srv/www/pixelscrapper.com/public_html/robots.txt -o \! \( -path '*.txt' -o -path quickstart.html \) \) -print0 \) | xargs -0r chmod $CODE_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:

find /srv/www/pixelscrapper.com/public_html  \( -path /srv/www/pixelscrapper.com/public_html/sites/default/files -o -path /srv/www/pixelscrapper.com/public_html/misc \) -prune -o \( \( \! -group $DEFAULT_GROUP -o \! -user $DEFAULT_OWNER \) -print0 \) | xargs -0r chown $DEFAULT_OWNER:$DEFAULT_GROUP --
find /srv/www/pixelscrapper.com/public_html  \( -path /srv/www/pixelscrapper.com/public_html/sites/default/files -o -path /srv/www/pixelscrapper.com/public_html/misc \) -prune -o \( -type d \! -perm $CODE_DIRS -print0 \) | xargs -0r chmod $CODE_DIRS --
find /srv/www/pixelscrapper.com/public_html -mindepth 2 \( -path /srv/www/pixelscrapper.com/public_html/sites/default/files -o -path /srv/www/pixelscrapper.com/public_html/misc \) -prune -o \( -type f \! -perm $CODE_FILES -print0 \) | xargs -0r chmod $CODE_FILES --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( \( \! -group $FILES_GROUP -o \! -user $FILES_OWNER \) -print0 \) | xargs -0r chown $FILES_OWNER:$FILES_GROUP --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( -type d \! -perm $USER_DIRS -print0 \) | xargs -0r chmod $USER_DIRS --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( -type f \! -perm $USER_FILES -print0 \) | xargs -0r chmod $USER_FILES --
find /srv/www/pixelscrapper.com/public_html -maxdepth 1  \( -type f \! -perm $DOC_FILES \( -path '*.txt' -o -path quickstart.html \) \! -path /srv/www/pixelscrapper.com/public_html/robots.txt -print0 \) | xargs -0r chmod $DOC_FILES --
find /srv/www/pixelscrapper.com/public_html -maxdepth 1  \( -type f \! -perm $CODE_FILES \( -path /srv/www/pixelscrapper.com/public_html/robots.txt -o \! \( -path '*.txt' -o -path quickstart.html \) \) -print0 \) | xargs -0r chmod $CODE_FILES --
JordanMagnuson’s picture

Also, 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

./perms.sh: line 2: =0440: command not found
./perms.sh: line 3: =0400: command not found
./perms.sh: line 4: =0644: command not found
./perms.sh: line 5: =0755: command not found
./perms.sh: line 6: =0664: command not found
./perms.sh: line 7: =0775: command not found
./perms.sh: line 8: =magjor: command not found
./perms.sh: line 9: =www-data: command not found
./perms.sh: line 10: =magjor: command not found
./perms.sh: line 11: =www-data: command not found
find: `-o' is not the name of an existing group
find: invalid mode `-print0'
find: invalid mode `-print0'
find: `-o' is not the name of an existing group
find: invalid mode `-print0'
find: invalid mode `-print0'
find: invalid mode `('
find: invalid mode `('

Here's perms.sh in its entirety (using the php tag for syntax highlighting convenience):

#!/bin/bash
$SETTINGS='0440'
$DOC_FILES='0400'
$CODE_FILES='0644'
$CODE_DIRS='0755'
$USER_FILES='0664'
$USER_DIRS='0775'
$DEFAULT_OWNER='magjor'
$DEFAULT_GROUP='www-data'
$FILES_OWNER='magjor'
$FILES_GROUP='www-data'
find /srv/www/pixelscrapper.com/public_html  -path /srv/www/pixelscrapper.com/public_html/sites/default/files -prune -o \( \( \! -group $DEFAULT_GROUP -o \! -user $DEFAULT_OWNER \) -print0 \) | xargs -0r chown $DEFAULT_OWNER:$DEFAULT_GROUP --
find /srv/www/pixelscrapper.com/public_html  -path /srv/www/pixelscrapper.com/public_html/sites/default/files -prune -o \( -type d \! -perm $CODE_DIRS -print0 \) | xargs -0r chmod $CODE_DIRS --
find /srv/www/pixelscrapper.com/public_html -mindepth 2 -path /srv/www/pixelscrapper.com/public_html/sites/default/files -prune -o \( -type f \! -perm $CODE_FILES -print0 \) | xargs -0r chmod $CODE_FILES --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( \( \! -group $FILES_GROUP -o \! -user $FILES_OWNER \) -print0 \) | xargs -0r chown $FILES_OWNER:$FILES_GROUP --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( -type d \! -perm $USER_DIRS -print0 \) | xargs -0r chmod $USER_DIRS --
find /srv/www/pixelscrapper.com/public_html/sites/default/files   \( -type f \! -perm $USER_FILES -print0 \) | xargs -0r chmod $USER_FILES --
find /srv/www/pixelscrapper.com/public_html -maxdepth 1  \( -type f \! -perm $DOC_FILES \( -path '*.txt' -o -path quickstart.html \) \! -path /srv/www/pixelscrapper.com/public_html/robots.txt -print0 \) | xargs -0r chmod $DOC_FILES --
find /srv/www/pixelscrapper.com/public_html -maxdepth 1  \( -type f \! -perm $CODE_FILES \( -path /srv/www/pixelscrapper.com/public_html/robots.txt -o \! \( -path '*.txt' -o -path quickstart.html \) \) -print0 \) | xargs -0r chmod $CODE_FILES --
omega8cc’s picture

$SETTINGS='0440' - is it literally like that in the script? You don't declare variables that way in bash..

JordanMagnuson’s picture

That's what I thought. But yes, what I pasted is the exact output from drush perms to perms.sh (minus the php tags).

omega8cc’s picture

It should be just:

SETTINGS=0440
DOC_FILES=0400
CODE_FILES=0644
CODE_DIRS=0755
USER_FILES=0664
USER_DIRS=0775
DEFAULT_OWNER=magjor
DEFAULT_GROUP=www-data
FILES_OWNER=magjor
FILES_GROUP=www-data

Not sure about the rest of the script correctness.

greg.1.anderson’s picture

Here 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.

JordanMagnuson’s picture

That 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...

kalabro’s picture

Tested a little bit and it works. But I get "find: paths must precede expression: ./*/*" line when run it with --sudo option.

greg.1.anderson’s picture

FileSize
53.44 KB

#61 was not caused by --sudo, but instead was caused when --dir=. was used. This patch fixes #60 and #61.

greg.1.anderson’s picture

FileSize
55.82 KB

Greatly improved options and help text.

greg.1.anderson’s picture

Rename to 'core-permissions' / permissions.core.inc.

moshe weitzman’s picture

Status: Needs review » Needs work

Some 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.

greg.1.anderson’s picture

Perhaps 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.

moshe weitzman’s picture

core-chmod works for me. you might want to wait until we resolve the 'where does this command live' question.

kalabro’s picture

@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.

Owen Barton’s picture

I 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).

colan’s picture

It'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. :)

greg.1.anderson’s picture

I 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..

kalabro’s picture

@greg.1.anderson, @mosheWeitzman, could we help you somehow? This new Drush feature is really amazing, but status is still "needs work".

greg.1.anderson’s picture

IMHO, 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.

jonhattan’s picture

I'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.

msonnabaum’s picture

I'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.

greg.1.anderson’s picture

The xargs -r flag is a bit of a problem, but it is solvable. From man xargs on FreeBSD:

     -r      Compatibility with GNU xargs.  The GNU version of xargs runs the
	     utility argument at least once, even if xargs input is empty, and
	     it supports a -r option to inhibit this behavior.	The FreeBSD
	     version of xargs does not run the utility argument on empty
	     input, but it supports the -r option for command-line compatibil-
	     ity with GNU xargs, but the -r option does nothing in the FreeBSD
	     version of xargs.

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). If xargs [...r...] is found (regex is more like xargs \[[^]*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.

JordanMagnuson’s picture

My 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 :)

greg.1.anderson’s picture

Status: Needs work » Needs review

I'd like to commit this by BADcamp. Should I put it in drush extras? I'd still prefer to see it in core.

moshe weitzman’s picture

We 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.

greg.1.anderson’s picture

Status: Needs review » Needs work

Sounds reasonable; I'll just put off committing until after the discussion.

kalabro’s picture

Some problems after testing. I specifically set 777 permissions to files folder to test Drush core-permissions:

`--> l sites/default/
total 28K
drwxrwxrwx 2 marshalkina marshalkina 4.0K Oct 31 14:04 files
-rw-r--r-- 1 marshalkina marshalkina  11K Oct 31 14:02 default.settings.php
-r--r----- 1 marshalkina marshalkina  11K Oct 31 14:04 settings.php

Note that my settings.php has permissions 440.

I try to fix my permissions with drush perms:

`--> drush perms marshalkina www-data:marshalkina
chown: changing ownership of `/vhosts/test2/sites/default/files': Operation not permitted

or

`--> drush perms marshalkina www-data:marshalkina --sudo
chmod: changing permissions of `/vhosts/test2/sites/default/files': Operation not permitted

With or without --sudo work isn't done:

`--> l sites/default/
total 28K
drwxrwxrwx 2 marshalkina marshalkina 4.0K Oct 31 14:04 files
-rw-r--r-- 1 marshalkina marshalkina  11K Oct 31 14:02 default.settings.php
-rw-r--r-- 1 marshalkina marshalkina  11K Oct 31 14:04 settings.php

My settings.php now has 644 permissions, but I didn't want this to happen.

`--> drush perms marshalkina www-data:marshalkina --sudo=all
Permissions change complete. 

--sudo=all fixes all problems.

Is this workflow is correct? I found it quite strange.

greg.1.anderson’s picture

It 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.

greg.1.anderson’s picture

Regarding #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.

kalabro’s picture

continue 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.

JordanMagnuson’s picture

So 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?

greg.1.anderson’s picture

Maybe 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.

vinmassaro’s picture

Version: » 7.x-5.8

Looking forward to seeing this in drush. We are looking to clean up sites/default directory permissions.

greg.1.anderson’s picture

At 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.

greg.1.anderson’s picture

Side 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.

sonicthoughts’s picture

This would be enormously helpful. Note: we have some sites running PHP as the owner of files and others as nobody.

TravisCarden’s picture

A couple of points of feedback:

  1. The script removes the executable bit from shell scripts (like scripts/run-tests.sh), which probably isn't undesirable. Should there be special handling for *.sh files with a --shell-scripts octal permissions option?
  2. The word "privileged" is misspelled (as "priviledged") everywhere in the patch. Attached is a corrected patch.
greg.1.anderson’s picture

Version: 7.x-5.8 » 8.x-6.x-dev
Status: Needs work » Closed (won't fix)
Issue tags: +Needs migration

This 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.

vinmassaro’s picture

Status: Closed (won't fix) » Closed (duplicate)

Switching status to closed (duplicate) since I've reopened this with a pull request here: https://github.com/drush-ops/drush/pull/158