MkdirTask behaves the same as "mkdir" Linux command and respects POSIX ACL #591

sustmi · 2016-10-10T22:27:54Z

This PR makes MkdirTask create directories with the same permissions as Linux mkdir command. Also, it sets correct permissions when using POSIX Access Control Lists.

It is possibly a BC break, but I think that it is worth it as the new behaviour is more intuitive.

Test-cases explaining the old and the new behaviour for ACL:

1. a) Linux shell (ACL enabled):

$ setfacl --default --modify user::rwX .
$ getfacl .
# file: .
# owner: me
# group: me
user::rwx
group::rwx
other::r-x
default:user::rwx
default:user:me:rwx
default:group::rwx
default:mask::rwx
default:other::r-x

$ umask 0077
$ mkdir dir1
$ mkdir --mode 0777 dir2

$ ls -l
drwxrwxr-x+ 2 me me 4096 Oct 10 23:41 dir1
drwxrwxrwx+ 2 me me 4096 Oct 10 23:41 dir2

$ getfacl dir1
# file: dir1
# owner: me
# group: me
user::rwx
user:me:rwx
group::rwx
mask::rwx
other::r-x
default:user::rwx
default:user:me:rwx
default:group::rwx
default:mask::rwx
default:other::r-x

Notice that umask is completely ignored when ACL is enabled. Directory dir1 has rwxrwxr-x permissions because that is the default setting of the parent directory.

1. b) MkdirTask before this PR (ACL enabled):

build.xml:

...
  <mkdir dir="dir1" />
  <mkdir dir="dir2" mode="0777" />
...

shell:

$ umask 0077
$ php phing

$ ls -l
drwx------+ 2 me me 4096 Oct 10 23.57 dir1
drwxrwxr-x+ 2 me me 4096 Oct 10 23.57 dir2

$ getfacl dir1
# file: dir1
# owner: me
# group: me
user::rwx
user:me:rwx         #effective:---
group::rwx          #effective:---
mask::---
other::---
default:user::rwx
default:user:me:rwx
default:group::rwx
default:mask::rwx
default:other::r-x

Notice that dir1 has wrong permissions. Old MkdirTask calls mkdir('dir1', 0700) because it masks 0777 with the original umask (see the original MkdirTask::__construct()).
Also, you can see that in this case old MkdirTask incorrectly sets ACL mask to --- because ACL mask equals to group permissions when ACL is enabled.

1. c) MkdirTask after this PR (ACL enabled):

build.xml:

...
  <mkdir dir="dir1" />
  <mkdir dir="dir2" mode="0777" />
...

shell:

$ umask 0077
$ php phing

$ ls -l
drwxrwxr-x+ 2 me me 4096 Oct 11 00.01 dir1
drwxrwxrwx+ 2 me me 4096 Oct 11 00.01 dir2

$ getfacl dir1
# file: dir1
# owner: me
# group: me
user::rwx
user:me:rwx
group::rwx
mask::rwx
other::r-x
default:user::rwx
default:user:me:rwx
default:group::rwx
default:mask::rwx
default:other::r-x

You can see that the new MkdirTask creates all permissions the same as Linux mkdir command.

Test-cases explaining the old and the new behaviour when creating parent directories:

2. a) Linux shell (no ACL):

$ umask 0077
$ mkdir dir1
$ mkdir --mode 0777 dir2
$ mkdir --parents --mode 0777 dir3/dir4

$ ls -l
drwx------.   2 me me  4096 Oct 10 22:50 dir1
drwxrwxrwx.   2 me me  4096 Oct 10 22:51 dir2
drwx------.   2 me me  4096 Oct 10 22:51 dir3

$ ls -l dir3
drwxrwxrwx.   2 me me  4096 Oct 10 22:51 dir4

Notice that dir2 is created with 0777 permissions even though umask is set to 0077. That is because umask is ignored when --mode option is used.
Also notice that dir3 has default permissions and not 0777 as specified in --mode option.

2. b) MkdirTask before this PR (no ACL):

build.xml:

...
  <mkdir dir="dir1" />
  <mkdir dir="dir2" mode="0777" />
  <mkdir dir="dir3/dir4" mode="0777" />
...

shell:

$ umask 0077
$ php phing

$ ls -l
drwx------. 2 me me 4096 Oct 10 23:32 dir1
drwxrwxrwx. 2 me me 4096 Oct 10 23:32 dir2
drwxrwxrwx. 3 me me 4096 Oct 10 23:32 dir3

$ ls -l dir3
drwxrwxrwx. 2 me me 4096 Ocr 10 23.32 dir4

Notice that dir3 is not created with default permissions but with those specified in mode attribute which is not the same behaviour as in case of Linux mkdir command.

2. c) MkdirTask after this PR (no ACL):

build.xml:

...
  <mkdir dir="dir1" />
  <mkdir dir="dir2" mode="0777" />
  <mkdir dir="dir3/dir4" mode="0777" />
...

shell:

$ umask 0077
$ php phing

$ ls -l
drwx------. 2 me me 4096 Oct 10 23:36 dir1
drwxrwxrwx. 2 me me 4096 Oct 10 23:36 dir2
drwx------. 3 me me 4096 Oct 10 23:36 dir3

$ ls -l dir3
drwxrwxrwx. 2 me me 4096 Oct 10 23:36 dir4

MkdirTask now behaves the same as Linux mkdir command (ie. only last child directory has the permissions specified in mode attribute).

sustmi · 2016-10-10T22:37:29Z

I might rewrite the test-cases as proper PHPUnit tests if you agree with the changes in the PR.

mrook · 2016-10-20T12:19:35Z

I like the idea, but this is indeed a BC break - which makes it a nice candidate for the 3.0 release. So, could you:

rebase this PR to the 3.0 branch
add / rewrite test cases
update the documentation where necessary

Thanks!

sustmi · 2017-04-10T19:18:11Z

Hi @mrook . I did not have much time to write the tests lately but I would like to finish the PR now.
Which branch should I rebase it to unstable-3.0 or future-4.0?

mrook · 2017-04-10T20:23:53Z

@sustmi that would be great, please rebase against master.

sustmi · 2017-04-16T16:32:54Z

@mrook: I added tests and rebased the PR on master.
The tests for POSIX ACL are skipped on Travis CI. It probably does not support ACL (does not have setfacl command).
Also, I had to add one extra commit that fixes ChmodTaskTest to properly clean-up testing tmp directory.
BTW: Should the documentation be updated too?

sustmi · 2017-04-16T16:48:56Z

Ok, I managed to enable ACL support in .travis.yml by installing acl system package so the ACL tests are no longer skipped.

mrook · 2017-11-22T14:51:44Z

@siad007 could you review this one?

siad007

I think we schould add the whole acl stuff to the documentation.

mrook · 2017-11-26T13:44:41Z

Yeah you're probably right. @sustmi can you make that happen?

stale · 2018-06-16T15:36:50Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2018-08-15T16:28:59Z

This issue has been automatically closed because it has not had recent activity. Thank you for your contributions.

siad007 · 2019-08-10T09:06:29Z

I reopen this ticket. Related to #1088 - but we should think of windows support here.

sustmi · 2019-10-01T16:58:04Z

Sorry for leaving this PR stale. Are you still interested in getting this merged?
I changed my job and in the current one we do not use Phing so I do not really need this any more. But if there is somebody who will profit from this, we can try to get it done.

@siad007 : Do you have any idea what should be documented?
I think I cannot "add the whole acl stuff to the documentation". 😉

Should I add some info to the documentation of MkdirTask or some general documentation of ACL too?

sustmi · 2019-10-02T23:51:28Z

I updated the MkdirTaskTest to skip scenarios that require non-zero umask (since Windows has umask hard-coded to zero).

sustmi · 2019-10-03T00:09:36Z

  [phpunit] testParentDirectoriesHaveDefaultPermissions with data set #0 FAILED
  [phpunit] Failed asserting that file mode of "C:\projects\phing\test/etc/tasks/system/tmp/a/b" is 707

Well, it looks like Windows may not even support chmod().

sustmi · 2019-10-03T00:26:08Z

Ok, the chmod() on Windows is really "crooked".
See this test script:

<?php

function chmodTest($file, $mode) {
    chmod($file, $mode);
    
    clearstatcache();
    var_dump(sprintf('expected: %03o, actual: %03o', $mode, fileperms($file)));
}

$file = __DIR__ . '/test.txt';

touch($file);

chmodTest($file, 0000);
chmodTest($file, 0777);
chmodTest($file, 0707);
chmodTest($file, 0700);
chmodTest($file, 0070);
chmodTest($file, 0007);

/*
Output on Windows 10, NTFS patition, php-7.3.10-Win32-VC15-x64:

string(29) "expected: 000, actual: 100444"
string(29) "expected: 777, actual: 100666"
string(29) "expected: 707, actual: 100666"
string(29) "expected: 700, actual: 100666"
string(29) "expected: 070, actual: 100444"
string(29) "expected: 007, actual: 100444"
*/

sustmi · 2019-10-03T00:37:22Z

I guess all the current tests in MkdirTaskTest can be skipped on Windows because they depend on POSIX permissions and those really do not work as expected in PHP on Windows.

…dows

sustmi · 2019-10-03T01:28:58Z

Hooray! Tests pass both on Linux and Windows.
Tomorrow I will try to look at the code that uses mkdirs() to see if the new behavior could break anything.

codecov-io · 2019-10-03T01:32:21Z

Codecov Report

Merging #591 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #591      +/-   ##
============================================
+ Coverage     52.42%   52.48%   +0.05%     
- Complexity     9306     9307       +1     
============================================
  Files           476      476              
  Lines         22705    22704       -1     
============================================
+ Hits          11903    11916      +13     
+ Misses        10802    10788      -14

Impacted Files	Coverage Δ	Complexity Δ
classes/phing/tasks/system/MkdirTask.php	`63.63% <ø> (-4.37%)`	`8.00 <0.00> (-1.00)`
classes/phing/system/io/FileSystem.php	`46.26% <100.00%> (+0.38%)`	`102.00 <3.00> (+2.00)`
classes/phing/system/io/PhingFile.php	`83.24% <100.00%> (ø)`	`92.00 <12.00> (ø)`
classes/phing/tasks/system/ChmodTask.php	`71.18% <0.00%> (+3.38%)`	`26.00% <0.00%> (ø%)`
classes/phing/tasks/system/DeleteTask.php	`65.35% <0.00%> (+9.44%)`	`68.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d6caf2...f4bcaaf. Read the comment docs.

all things were done

siad007 · 2019-12-25T00:08:28Z

Hooray! Tests pass both on Linux and Windows.
Tomorrow I will try to look at the code that uses mkdirs() to see if the new behavior could break anything.

@sustmi did you find some breaking stuff?

stale · 2020-06-22T00:40:53Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

siad007 · 2020-06-27T20:26:04Z

@sustmi finaly merged - thx for your commits and patience 👍

sustmi · 2020-07-07T17:44:57Z

Thank you for finishing it.
Unfortunatelly, I did not have a time to work on this any more.

mrook added this to the 3.0 milestone Oct 20, 2016

mrook added the needs update label Feb 6, 2017

sustmi force-pushed the mkdir-acl-support branch 2 times, most recently from a6b4356 to 4affafd Compare April 16, 2017 15:00

sustmi changed the title ~~MkdirTask behaves the same as "mkdir" Linux command and respects ACL~~ MkdirTask behaves the same as "mkdir" Linux command and respects POSIX ACL Apr 16, 2017

sustmi changed the title ~~MkdirTask behaves the same as "mkdir" Linux command and respects POSIX ACL~~ [WIP] MkdirTask behaves the same as "mkdir" Linux command and respects POSIX ACL Apr 16, 2017

sustmi force-pushed the mkdir-acl-support branch from 34f30dc to fb3ff04 Compare April 16, 2017 16:46

sustmi changed the title ~~[WIP] MkdirTask behaves the same as "mkdir" Linux command and respects POSIX ACL~~ MkdirTask behaves the same as "mkdir" Linux command and respects POSIX ACL Apr 16, 2017

mrook removed the needs update label Nov 3, 2017

siad007 added the needs documentation label Nov 26, 2017

siad007 requested changes Nov 26, 2017

View reviewed changes

mrook modified the milestones: 3.0, 3.0 RC2 Dec 18, 2017

stale bot added the wontfix label Jun 16, 2018

stale bot closed this Aug 15, 2018

siad007 removed the wontfix label Aug 10, 2019

siad007 reopened this Aug 10, 2019

siad007 added the needs windows support label Aug 10, 2019

sustmi added 3 commits October 3, 2019 02:46

Update MkdirTaskTest to use test data that work both on Linux and Win…

941cbea

…dows

properly clean-up even if test did not create any directory

727544f

Last try before I rage quit and go to bed

8de92db

siad007 added 2 commits October 5, 2019 18:04

Merge branch 'master' into mkdir-acl-support

15ad33a

Merge branch 'master' into mkdir-acl-support

6653a81

siad007 modified the milestones: 3.0.0-alpha2, 3.0.0-alpha4 Oct 19, 2019

Merge branch 'master' into mkdir-acl-support

5ece26d

siad007 previously approved these changes Dec 25, 2019

View reviewed changes

siad007 self-requested a review December 25, 2019 00:04

siad007 removed their request for review December 25, 2019 00:06

stale bot added the wontfix label Jun 22, 2020

siad007 removed the wontfix label Jun 27, 2020

siad007 and others added 3 commits June 27, 2020 21:49

Merge branch 'master' into mkdir-acl-support

fbb56a3

Update MkdirTaskTest.php

c4b8d16

Update MkdirTaskTest.php

f4bcaaf

siad007 added enhancement and removed needs update labels Jun 27, 2020

siad007 merged commit 4726e4f into phingofficial:master Jun 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MkdirTask behaves the same as "mkdir" Linux command and respects POSIX ACL #591

MkdirTask behaves the same as "mkdir" Linux command and respects POSIX ACL #591

sustmi commented Oct 10, 2016

sustmi commented Oct 10, 2016

mrook commented Oct 20, 2016

sustmi commented Apr 10, 2017

mrook commented Apr 10, 2017

sustmi commented Apr 16, 2017 •

edited

Loading

sustmi commented Apr 16, 2017

mrook commented Nov 22, 2017

siad007 left a comment

mrook commented Nov 26, 2017

stale bot commented Jun 16, 2018

stale bot commented Aug 15, 2018

siad007 commented Aug 10, 2019

sustmi commented Oct 1, 2019 •

edited

Loading

sustmi commented Oct 2, 2019

sustmi commented Oct 3, 2019

sustmi commented Oct 3, 2019

sustmi commented Oct 3, 2019 •

edited

Loading

sustmi commented Oct 3, 2019

codecov-io commented Oct 3, 2019 •

edited by codecov bot

Loading

siad007 commented Dec 25, 2019

stale bot commented Jun 22, 2020

siad007 commented Jun 27, 2020

sustmi commented Jul 7, 2020

MkdirTask behaves the same as "mkdir" Linux command and respects POSIX ACL #591

MkdirTask behaves the same as "mkdir" Linux command and respects POSIX ACL #591

Conversation

sustmi commented Oct 10, 2016

Test-cases explaining the old and the new behaviour for ACL:

1. a) Linux shell (ACL enabled):

1. b) MkdirTask before this PR (ACL enabled):

build.xml:

shell:

1. c) MkdirTask after this PR (ACL enabled):

build.xml:

shell:

Test-cases explaining the old and the new behaviour when creating parent directories:

2. a) Linux shell (no ACL):

2. b) MkdirTask before this PR (no ACL):

build.xml:

shell:

2. c) MkdirTask after this PR (no ACL):

build.xml:

shell:

sustmi commented Oct 10, 2016

mrook commented Oct 20, 2016

sustmi commented Apr 10, 2017

mrook commented Apr 10, 2017

sustmi commented Apr 16, 2017 • edited Loading

sustmi commented Apr 16, 2017

mrook commented Nov 22, 2017

siad007 left a comment

Choose a reason for hiding this comment

mrook commented Nov 26, 2017

stale bot commented Jun 16, 2018

stale bot commented Aug 15, 2018

siad007 commented Aug 10, 2019

sustmi commented Oct 1, 2019 • edited Loading

sustmi commented Oct 2, 2019

sustmi commented Oct 3, 2019

sustmi commented Oct 3, 2019

sustmi commented Oct 3, 2019 • edited Loading

sustmi commented Oct 3, 2019

codecov-io commented Oct 3, 2019 • edited by codecov bot Loading

Codecov Report

siad007 commented Dec 25, 2019

stale bot commented Jun 22, 2020

siad007 commented Jun 27, 2020

sustmi commented Jul 7, 2020

sustmi commented Apr 16, 2017 •

edited

Loading

sustmi commented Oct 1, 2019 •

edited

Loading

sustmi commented Oct 3, 2019 •

edited

Loading

codecov-io commented Oct 3, 2019 •

edited by codecov bot

Loading