Add wrappers to fix permission checks

Tagging, this would still need a forward port/reroll to apply to 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!

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.

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

Steps to reproduce in drupal-7.20 (swap apache with your website user):

1. Download the drupal tarball, extract it.
2. Do whatever you need to do in settings.php
3. In sites/default,
   1. mkdir files
   2. chmod 700 files
   3. setfacl -m user:apache:rwx files/
   4. setfacl -d -m user:apache:rwx files/
4. Install Drupal (minimal)
5. Download and install IMCE
6. Open the file browser from your "My Account" page.

IMCE will fail, claiming that the directory (sites/default/files) is not writable. This is due to file_prepare_directory('public://', FILE_CREATE_DIRECTORY) failing, and is fixed by the patch which modifies file_prepare_directory().

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.

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

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

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

The current steps to reproduce probably won't work I'm afraid. Uploading files—in and of itself—has always worked. The filesystem ACLs do really allow you to write the file, so if you try to do it, it will work.

The problem is when Drupal does its heuristic to determine, "will I be able to write this file if the user tries to do it?" That test is currently broken, so the answer is "no" even though it should be "yes" (thanks to the ACL).

The last submitted patch, drupal-writable-executable-fix-7.22.patch, failed testing.