[D7] Add wrappers to fix permission checks

Thanks @mjorlitzky, at a glance this looks pretty good. This needs to be fixed in 8.x first then backported to 7.x per the backport policy.

The 8.x patch from #944582-56: ./sites/default/files directory permission check is incorrect during install AND status report (affects Docker on Windows) will need to be rerolled against 8.x minus the installer changes.

Instructions for rerolling patches.

Tagging, this would still need a forward port/reroll to apply to 8.x.

i will reroll this patch against 8.x

We worked on this in the Drupal Spring London with Pasive and joates. We basically took the patch provided for D7 in this issue summary and ported it to D8 up to date. Thanks to Joates to guide us through the process!

Code looks great!

Many things look wrong in there.

First, the original post mentions that Drupal is broken when ACLs are in use, but doesn't mention why checking the execute bit (which has nothing to do with ACLs) would fix anything. Other then parsing the ACLs manually, the only way to check if you can write a file in a given directory is to try to write it, and that's what #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs actually tried to do.

Second, using drupal_realpath() without checking the return value is *really wrong*. Remote stream wrappers do not implement ->realpath(), so this is going to return FALSE.

Third, I don't see at all why this is necessary, because both is_executable() and is_writable() are basically wrappers around stat() and as a consequence they *actually work* with stream wrappers (as long as they implement ->stat()).

Finally, I doubt is_executable() on a directory returns what you expect it to return on Windows.

ty for the feedback.

possible alternative to drupal_realpath() (deprecated) is discussed here..
#1201024: drupal_realpath() should describe when it should be used

[edit] in the case of a remote resource ... [/edit]
returning FALSE is certainly not useful, should probably at least test for this & log an error when it occurs.

Hi Damien and Joates. Thank you guys for the feedback. As far as I can tell at this point this issue should have never existed? Would you suggest another alterantive or we'll just rely on the fact that drupal does not support ACLs full stop.

Please let me know as I'm new at drupal issues so sincerely I don't know how to proceed from here.

Cheers!

Juan B

From reading Damien's response, I think the first thing to do here is to provide detailed, reproducable steps on how to get Drupal to fail when it should work.

Thanks, I'll try this on D8 and get back with the results here.

in response to #10 Posted by mjorlitzky..

it would be more constructive if we can all agree that this is an 8.x issue now,
there is no 8.x release of IMCE

Revised guidelines to re-create the bug:

1. build a clean Drupal 8 development environment
2. In sites/default,
   • chown -R nobody files
   • chmod -R 700 files
   • setfacl -R -m user:apache:rwx files/
   • setfacl -Rd -m user:apache:rwx files/
3. Install Drupal 8
4. Add a new Article (uploading an image should fail).

Notes:

• change apache in step 2. for the username of your httpd server.
• step 4. needs to be confirmed or an alternative method proposed.

[edit]
// 28-Feb-2013
Modified the instructions to implement full excluded file access to the entire files/ sub-tree (user = nobody)
and added recursive ACL permissions as Drupal needs to store file uploads in several sub-folder locations.
[/edit]

Ok, I see what's happening. PHP has a special case for local files in which it uses access(2) or the equivalent Windows call. This supports ACLs. If called on a stream-wrapper, it defaults to a (very complicated) process based on the stat(2) structure returned by the stream-wrapper implementation.

More precisely it checks part of the .mode structure depending on:

• If the current user (as returned by getuid(2)) is the same as the .uid in the stat, it uses the "user" part of the mode;
• If current group (as returned by getgid(2)) or the supplementary groups (as returned by getgroups(2) is the same as the .gid in the stat, it uses the "group" part of the mode;
• Else, it falls back to the "others" part of the mode.

So, one way of moving forward here, would be to modify the stat implementation in our local streamwrapper to fake the mode by calling is_readable(), is_writable() and is_executable() manually, and return the proper structure.

Is this related to #1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination?

Re #14: thanks Damien for locating what seems to be the root cause of this..

• removing Novice tag
• priority normal seems more appropriate
• need a patch to "modify the stat implementation in our local streamwrapper" ?

from core/lib/Drupal/Core/StreamWrapper/LocalStream.php, line 472

<?php
public function url_stat($uri, $flags) {
  $this->uri = $uri;
  $path = $this->getLocalPath();
  // Suppress warnings if requested or if the file or directory does not
  // exist. This is consistent with PHP's plain filesystem stream wrapper.
  if ($flags & STREAM_URL_STAT_QUIET || !file_exists($path)) {
    return @stat($path);
  }
  else {
    return stat($path);
  }
}
?>

in response to #9 and #12..

after setting chmod 700 files
you can clearly see from the terminal that my normal user (joates) is group member but cannot gain access.
getfacl shows the ACL permissions (as described in #12)
(screenshot attachment not working?)

on a fresh install of D8 using default configuration (public:// streamwrapper) i am able to upload an image without error.

try again with the screenshot..

link: http://i.imgur.com/G0s2i99.png

afterwards i realised that having www-data as the directory owner is not too helpful :-p

so i did a chown -R joates:joates files cleared cache and tried again to upload the image, and it did indeed FAIL to upload the file
(which sort of feels like causing it to fail when it *should* fail, but maybe i don't understand ACLs correctly -- does setfacl in the top folder apply to every child ?)
Update: even with recursive ACL the file upload still fails.

link: http://i.imgur.com/FesrnJV.png

can someone with superior ACL sysadmin skills to me please confirm that it seems that Drupal is not providing support for ACLs ?
(perhaps due to the situation Damien describes in #14)

and maybe change the title of this issue if that is the case :)

@joates - Thanks for all your work on this issue. Could you please update the issue summary with the steps to reproduce, that way others can update them too and you don't have to keep editing your comments :)

Updated issue summary.

ok

mjorlitzky: Just wanted to say "Thank you". We download this patch regularly, whenever we update Core.

to run the tests ... change the version. (you can change it back)

back to 8.0.x which needs a patch to work on that.

RTBC++

This patch worked for me against the latest 7.38. It solved the problem I reported here: #2509188: The directory public://pictures does not exist or is not writable.

Is this ticket a backport of the mess in this ticket: #944582: ./sites/default/files directory permission check is incorrect during install AND status report (affects Docker on Windows)?

Well, Thank you, @orlitzky! Thanks for the quick response and providing a solution amidst that chaotic ticket. And thank you in advance for your life-long service of re-rolling this patch ;)

Given that no one has successfully reproduced this in Drupal 8 in the last four-and-a-half years, or indicated that it is still a problem in that version, I'm going to go out on a limb and bump this back down to Drupal 7. I've updated the issue description to reflect that.

Personally, I've experienced these errors on my local dev environment and the patch fixes it.

@orlitzky keep fighting the good fight! :-)

Re-uploading @orlitzky's patch from #52, so it'll test with the correct version of Drupal.

That's better! Since this has been thoroughly tested by multiple people for several years now, I'm marking it RTBC.

This almost certainly does affect Drupal 8, since the code in https://api.drupal.org/api/drupal/includes%21file.inc/function/file_prep... and https://api.drupal.org/api/drupal/core%21includes%21file.inc/function/fi... (as well as other places that call is_writable()/is_executable()) is virtually the same. If this isn't an issue in Drupal 8, someone would need to explain specifically why.

I also don't understand why this was split off from #944582: ./sites/default/files directory permission check is incorrect during install AND status report (affects Docker on Windows). Comparing the patches in the two issues, they are almost exactly the same - the primary difference is that the other issue changes the writable/executable checks to use the new system in more places than this one does. I do not understand the rationale for fixing this in only some places but not others, and especially don't see why there should be two open issues that are duplicating each other's work. So I think this could be closed as a duplicate of the issue. (Or alternatively, it could be repurposed as the Drupal 7 backport of that issue, and postponed on getting it fixed in Drupal 8 first.)

Verified that the patch for 5.6 still works with 5.7.

The difference between this ticket and the other is that this one regularly has patches that keep our site up and running. Myself and my colleagues (who run on AFS servers using ACLs) RELY on orlitzky's patches to keep our sites up and running. The other thread seems so caught up with posts that do not understand the issue, and patches that do nothing. Someone running with an ACL-enabled system needs to be actively involved in constructing solutions to this problem.

Also - thank you orlitzky for your tireless efforts, we would be writing our own patches every Drupal release if not for your work.

Thanks, provided patch solves the problem with file permission check in Drupal. So will this patch be applied to Drupal 8.x core? Can anybody else review it and provide for applying?

It appears that the patch for 7.61 no longer applies for 7.63. Any chance this will be updated soon? Appreciated!

EDIT: The patch for 7.61 does work!

I am so happy to find the patch for 7.68, it also works for 7.69 flawlessly. I think we need to buy @orlitzky coffee and cake.

Let’s try this one more time. Marking RTBC.

@orlitzky your previous patch still applied for me. Are there any changes in this one that I should be aware of?

This was added to #3192080: [meta] Priorities for 2021-04-07 release of Drupal 7 that will be copied into the meta issue for the next release. Not sure if it will help moving toward a commit.

Unfortunately per the backport-policy, this needs to go into D9 first:

See: https://www.drupal.org/project/drupal/issues/944582

And the patch there is identical to this one.

This is an example of when the policy makes no sense. As the D9 issue summary says:

This can cause various hard-to-troubleshoot permission-related errors with the files directory, including on Docker environments.

So, why let it persist for the over 160,000 D7 sites?

Anyway, the patch is here for those that want to use it. Hopefully, it will remain on the priority list for the future when D9 catches up.

@orlitzky Thanks for the reroll, but I think that it was not needed, because the patch in #82 still applies. It would be best to concentrate our efforts on the D10 patch so that we can get back to this.