Right now Drupal is unusable with filesystem (POSIX) ACLs. It's been broken for years. This was previously #944582: ./sites/default/files directory permission check is incorrect during install AND status report and #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs which are/were doomed to failure.

Guidelines to reproduce:

  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.
Files: 
CommentFileSizeAuthor
#27 drupal-writable-executable-fix-7.25.patch6.26 KBmjorlitzky
#26 drupal-writable-executable-fix-7.24.patch6.29 KBmjorlitzky
#25 drupal-writable-executable-fix-7.23.patch5.69 KBmjorlitzky
#23 drupal-writable-executable-fix-7.22.patch5.69 KBmjorlitzky
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-writable-executable-fix-7.22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 issue #1899126 (Screenshot from 2013-02-28).png318.78 KBjoates
#17 issue #1899126 (Screenshot from 2013-02-28 15:37:45).png318.78 KBjoates
#4 drupalcore-add_wrappers_to_fix_permission_checks-1899126-04.patch7.6 KBjuan.brein
PASSED: [[SimpleTest]]: [MySQL] 52,391 pass(es).
[ View ]
drupal-writable-executable-fix-7.19.patch5.69 KBmjorlitzky
PASSED: [[SimpleTest]]: [MySQL] 39,751 pass(es).
[ View ]

Comments

Version:7.x-dev» 8.x-dev
Issue tags:+Novice, +Needs manual testing, +needs backport to D7

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 will need to be rerolled against 8.x minus the installer changes.

Instructions for rerolling patches.

Issue tags:+Needs reroll

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

Assigned:Unassigned» joates
Status:Needs review» Needs work

i will reroll this patch against 8.x

Assigned:joates» juan.brein
Status:Needs work» Needs review
StatusFileSize
new7.6 KB
PASSED: [[SimpleTest]]: [MySQL] 52,391 pass(es).
[ View ]

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!

Status:Needs review» Reviewed & tested by the community

Code looks great!

Status:Reviewed & tested by the community» Needs work

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.

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

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

Assigned:Unassigned» juan.brein
Priority:Normal» Major
Status:Needs review» Needs work
Issue tags:+Novice, +Needs reroll

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]

You don't really need IMCE to reproduce the issue in 8.x (if it exists). Just call file_prepare_directory('public://', FILE_CREATE_DIRECTORY) somewhere and see if it works. IMCE is just the reason why this is a complete show-stopper for us.

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.

Status:Needs review» Needs work

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);
  }
}
?>

Assigned:juan.brein» Unassigned
Priority:Major» Normal
Status:Needs work» Needs review
Issue tags:-Novice, -Needs reroll
StatusFileSize
new318.78 KB

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.

Status:Needs work» Needs review
StatusFileSize
new318.78 KB

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

Assigned:juan.brein» Unassigned
Priority:Major» Normal
Status:Needs work» Needs review
Issue tags:-Novice, -Needs reroll

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

Issue summary:View changes

Updated issue summary.

ok

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

StatusFileSize
new5.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-writable-executable-fix-7.22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Updated patch for 7.22.

Status:Needs review» Needs work

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

StatusFileSize
new5.69 KB

Updated patch for 7.23.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
StatusFileSize
new6.29 KB

Updated patch for 7.24.

StatusFileSize
new6.26 KB

Updated patch for 7.25.

The patch for 7.25 applies cleanly to 7.26.