Skip authorize.php step if webroot files are owned by the httpd user

Sorry for the brevity. It's late, gotta get up early.

-J

+++ modules/update/update.manager.inc	25 Oct 2009 12:20:43 -0000
@@ -418,14 +418,30 @@ function update_manager_confirm_update_f
+    if (fileowner($project_real_location) == fileowner(conf_path())) {
+      module_load_include('inc', 'update', 'update.authorize');
+      $filetransfer = new FileTransferLocal(DRUPAL_ROOT);
+      update_authorize_run_update($filetransfer, $updates);
+    }
+    // Otherwise, go through the regular workflow to prompt for FTP/SSH
+    // credentials and invoke update_authorize_run_update() indirectly with
+    // whatever FileTransfer object authorize.php creates for us.
+    else {
+      system_run_authorized('update_authorize_run_update', drupal_get_path('module', 'update') . '/update.authorize.inc', array($updates));
+    }

Shouldn't we check for is_writable here? It could be owned by someone else but 777.

+++ modules/update/update.manager.inc	25 Oct 2009 12:20:43 -0000
@@ -565,13 +581,29 @@ function update_manager_install_form_sub
+  if (fileowner($project_real_location) == fileowner(conf_path())) {
+    module_load_include('inc', 'update', 'update.authorize');
+    $filetransfer = new FileTransferLocal(DRUPAL_ROOT);
+    call_user_func_array('update_authorize_run_install', array_merge(array($filetransfer), $arguments));
+  }

This is sorta a cut & paste job, isn't it? Any better way to do this?

+++ includes/filetransfer/local.inc	25 Oct 2009 10:28:45 -0000
@@ -0,0 +1,86 @@
+  protected function removeDirectoryJailed($directory) {

Should this use a RecursiveDirectoryIterator? (it is only like this in the FTP Wrapper class because RecursiveDirectoryIterators don't work properly there).

I'm on crack. Are you, too?

Okay, A and B make sense to me (although I'd put a @see in the code comments for maintainability).

RecursiveDirectoryIterator is there exactly for this purpose, I think we should use it. It is easier to read (IMO) but also less code and the way it is "supposed to be done". Using the php4 method doesn't really buy us anything.

If we change this, I'd say RTBC.

The advantage of RecursiveDirectoryIterator and friends, as I see it, is to centralize the ugly parts of the code into the setup for the loop so that the actual logic of the loop is cleaner. It also makes it easier to make the body of the loop interchangeable using various OO techniques such as the visitor pattern. If your loop is simple, then it's frequently unnecessary. I use scandir() all the time, still.

RecursiveDirectoryIterator here wouldn't give us a great deal, I think, aside from consistency with a foreach/iterator approach (DBTNG is all about the iterators) and some utility routines such as $dir->isDot() to replace if ($entry == '.' || $entry == '..'). I'd have to see the implementation to say for sure which I would find "cleaner", which is admittedly subjective. I'd probably lean toward the iterator approach, but that's me. (Now, if we were using splFile and friends as well I'd absolutely say we should be using the full iterator suite. SPL has extensive file handling niceness that we don't use as often as we perhaps should. sdboyer could talk to SplFile more than I can.)

As long as I'm here, this makes no sense to me at all:

+ static function factory($jail, $settings) {
+ return new FileTransferLocal($jail);
+ }

Factories are supposed to be centralized points of contact so that one system can request "some implementation" of another system. Why have a trivial factory per implementation? Besides, static methods suck for inheritance. :-)

Yeah, of course it is subjective, both work. One is:

* Shorter in bytes and commands
* Probably faster (not that it matters much).
* Meant for doing exactly this

The other is:

* Old.

re: the silly factory.

My original plan was different than this, but this is sorta a casualty of participating slightly outside the Drupal eco system. I agree, we could keep our factory in Drupal land if wanted to.

However, we need a factory of some sort because an anonymous array of settings is not a good thing to pass to a constructor (@see the new hook_theme_* for the opposite of good). Currently, the factories deal with setting defaults and instantiating themselves. In some cases though (like FileTransferFTP::factory) it actually is more like a "normal" factory in that it is figuring out the subclass.

In this case, it's just following the expected interface, and because this is a "pseudo" FileTransfer anyway (99% of other ones would require some settings), I'm fine with it.

The alternatives:
* Pass $settings to a constructor (bad)
* Have the factory in Drupal - requires that each implementation register a separate function and handle through that. That function would have logic which really belongs in the class most likely. This is redundant and harder to maintain, the class knows what its defaults, etc should be and how to find child classes.

Anyway, this issue is really not about changing the whole design pattern, so let's try to stay off that one until later if it needs discussion.

Thanks,
J

Looks good to me!

Although one small point, if $file->isDir() returns false, I'm pretty sure that means $file->isFile() is going to return true.

-J

Nice of you to post it, but yeah, helper function not desired in this case, and any refactoring not worth it. I was only concerned, not saying we shouldn't. Duped comment is no biggie.

It struck me as odd that none of the functions in local.inc are documented, but dww pointed out that these are in the base class, and the common pattern is not to document overridden methods in derived classes, which is true.

We also discussed whether or not this warrants a warning, either on this screen or in the status messages list that basically says "Your host is an idiot and should not have stuff set up this way." But in the end, figured that if someone is in such a situation, there's really nothing that they can do it about it, and probably neither can their host or they would've set it up properly in the first place. :P

It's concerning me to change this much code without any changes in the tests. I've committed this to HEAD for now to keep things moving, but is there any way of automated testing this kind of stuff, or not really since it's so host-specific?

Aw, crap. I committed 12 instead of 10. :( I had it right the first time and then went and committed the button case fix one since it was easier. Anyway, fixed it. I think.

Can you guys check to make sure this got through properly?

Questions:

1. Does this mean that "administer software updates" is now one of the most dangerous permissions on a Drupal site? As far as I can tell, before this patch, that permission didn't let you do anything too dangerous - unless you also happened to know the server login credentials, in which case it doesn't matter :) Whereas with this patch, it seems like on most popular shared hosting providers (Bluehost, Dreamhost, whatever) this permission is now pretty much equivalent to "execute arbitrary PHP code".

   If so, we need to document the permission description much better. (It would be a separate issue, but before filing one I want to make sure I understand the situation correctly.)

2. Is checking the fileowner() really good enough? It seems like in most cases, it would be, but are there any server configurations in which the webserver-created file could wind up with more permissive group ownership that the sites/default directory (or perhaps even wind up world-writable), even though they have the same owner? I remember now that this is the kind of thing I was concerned about when thinking about #418302: Copy default.settings.php to settings.php during install IFF webserver owns files (FTP on shared hosting) and the other issues that led up to that.

   If that is a concern, we need to change something here. And if not, it would be really great to have the code comment here expanded significantly to explain exactly why this check is sufficient and safe (and ideally even have that check broken out into a separate function that returns a boolean). So either way, I'm tentatively putting this back to needs work.

Very belated reply :)

For 1, yes, my concern was not with the permission being given a "dangerous" name (that certainly makes sense), but rather that it breaks other assumptions Drupal 7 has about when and how arbitrary PHP code execution is allowed via the UI - e.g., by someone who stumbles across a computer where an administrator has left themselves logged in. We may wind up deciding it's just a documentation issue, but we'll see. I've created an issue for that here: #932110: On some servers, the Update Manager allows administrators to directly execute arbitrary code even without the PHP module

For 2, having thought about it more, I agree with you that there are probably no (realistically secure) server setups under which this could be a problem.