diff options
author | Andreas Gohr <andi@splitbrain.org> | 2021-08-14 11:06:31 +0200 |
---|---|---|
committer | Andreas Gohr <andi@splitbrain.org> | 2021-08-14 11:43:40 +0200 |
commit | bd5391243cdd92d9e0abee5ba1a506170b18d072 (patch) | |
tree | 8cbe9f3e5722a3da7294a081d755f3e711f4c527 | |
parent | 1e519eb5339e02a75ee7aba2a1a5555f09ff8818 (diff) | |
download | dokuwiki-bd5391243cdd92d9e0abee5ba1a506170b18d072.tar.gz dokuwiki-bd5391243cdd92d9e0abee5ba1a506170b18d072.zip |
:fire: fix the calculation of file permissons
Our config allows to set the values for `dmode` and `fmode` to allow
users to explicitly define which permissions directories and files
should have.
To avoid unnessary chmod operations, we check the current umask to
compare what permissions files and directories would get witout our
intervention. If the result is already what the user wants, no chmods
will happen later on. Otherwise we set new configs called `dperm` and
`fperm` which will be used in chmod ops. This is done in
`init_creationmodes()`
When we created new directories, we used to pass the original `dmode`
config to `mkdir()`. The system will then apply the umask to that
`dmode`.
This means the resulting directory will *always* have different
permissions than `dmode`, *always* requiring a chmod operation.
That's silly.
**Breaking Change:** This patch removes the passing of `dmode` as
second parameter to all `mkdir` calls, making it default to `0700`
which is also what we test against in `init_creationmodes()`.
Plugins not relying on our `io_*` functions and do create their own
directories and which currenlty pass `dmode` to it need to be
adjusted to remove that second parameter.
Users may want to reapply the proper file permissions to their data
folder.
**Revert:** In 9fdcc8fcd87114ca59a1764a84d213a53c655c8c @movatica
introduced a change to `init_creationmodes()` that compared the umask
against `fmode` instead of `0666`. I merged it because it looked logical
when compared to the code for directories which compared against `dmode`
as described above. However we do not pass `fmode` to any file creation
methods (that's not possible).
The result is that all changes made in the `fmode` setting resulted
in the wrong permissions for newly created files as first reported in
https://forum.dokuwiki.org/d/19463-setting-fmode-not-working-as-intended
I'm unsure about the orginal motivation behind @movatica's change. The
"fix" however, is wrong.
**Tests:** This patch introduces an integration test that will check the
actual results of directory and file creations under various umask,
`dmode` and `fmode` settings.
-rw-r--r-- | _test/tests/inc/init_creationmodes.test.php | 109 | ||||
-rw-r--r-- | inc/Search/Indexer.php | 2 | ||||
-rw-r--r-- | inc/Subscriptions/BulkSubscriptionSender.php | 2 | ||||
-rw-r--r-- | inc/init.php | 8 | ||||
-rw-r--r-- | inc/io.php | 4 |
5 files changed, 117 insertions, 8 deletions
diff --git a/_test/tests/inc/init_creationmodes.test.php b/_test/tests/inc/init_creationmodes.test.php new file mode 100644 index 000000000..04e2e9d41 --- /dev/null +++ b/_test/tests/inc/init_creationmodes.test.php @@ -0,0 +1,109 @@ +<?php + +class init_creationmodes_test extends DokuWikiTest +{ + + protected $oldumask; + protected $dir; + protected $file; + + /** @inheritDoc */ + public static function setUpBeforeClass(): void + { + parent::setUpBeforeClass(); + if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') { + self::markTestSkipped('Permission checks skipped on Windows'); + } + } + + /** + * set up the file and directory we use for testing + */ + protected function init() + { + $this->dir = getCacheName('dir', '.creationmode_test'); + $this->file = getCacheName('bar', '.creationmode_test'); + } + + /** @inheritDoc */ + public function setUp(): void + { + parent::setUp(); + + if (!isset($this->dir)) $this->init(); + $this->oldumask = umask(); + } + + /** @inheritDoc */ + protected function tearDown(): void + { + umask($this->oldumask); + + chmod($this->dir, 0777); + rmdir($this->dir); + + chmod($this->file, 0777); + unlink($this->file); + + parent::tearDown(); + + } + + /** + * @return Generator|string[] + * @see testFilemodes + */ + public function provideFilemodes() + { + $umasks = [0000, 0022, 0002, 0007]; + $fmodes = [0777, 0666, 0644, 0640, 0664, 0660]; + $dmodes = [0777, 0775, 0755, 0750, 0770, 0700]; + + foreach ($umasks as $umask) { + foreach ($dmodes as $dmode) { + foreach ($fmodes as $fmode) { + yield [$umask, $dmode, $fmode]; + } + } + } + } + + /** + * @dataProvider provideFilemodes + */ + public function testFilemodes($umask, $dmode, $fmode) + { + global $conf; + + // setup + $conf['dmode'] = $dmode; + $conf['fmode'] = $fmode; + umask($umask); + + // create + init_creationmodes(); + io_mkdir_p($this->dir); + io_saveFile($this->file, 'test'); + + // get actual values (removing the status bits) + clearstatcache(); + $dperm = fileperms($this->dir) - 0x4000; + $fperm = fileperms($this->file) - 0x8000; + + + $this->assertSame($dmode, $dperm, + sprintf( + 'dir had %04o, expected %04o with umask %04o (fperm: %04o)', + $dperm, $dmode, $umask, $conf['dperm'] + ) + ); + $this->assertSame($fmode, $fperm, + sprintf( + 'file had %04o, expected %04o with umask %04o (fperm: %04o)', + $fperm, $fmode, $umask, $conf['fperm'] + ) + ); + } + +} + diff --git a/inc/Search/Indexer.php b/inc/Search/Indexer.php index ae3b12c57..5c7a09a4c 100644 --- a/inc/Search/Indexer.php +++ b/inc/Search/Indexer.php @@ -927,7 +927,7 @@ class Indexer { $status = true; $run = 0; $lock = $conf['lockdir'].'/_indexer.lock'; - while (!@mkdir($lock, $conf['dmode'])) { + while (!@mkdir($lock)) { usleep(50); if(is_dir($lock) && time()-@filemtime($lock) > 60*5){ // looks like a stale lock - remove it diff --git a/inc/Subscriptions/BulkSubscriptionSender.php b/inc/Subscriptions/BulkSubscriptionSender.php index 7341cd0a5..d37c1e000 100644 --- a/inc/Subscriptions/BulkSubscriptionSender.php +++ b/inc/Subscriptions/BulkSubscriptionSender.php @@ -152,7 +152,7 @@ class BulkSubscriptionSender extends SubscriptionSender } // try creating the lock directory - if (!@mkdir($lock, $conf['dmode'])) { + if (!@mkdir($lock)) { return false; } diff --git a/inc/init.php b/inc/init.php index 059d7060a..774fac9c8 100644 --- a/inc/init.php +++ b/inc/init.php @@ -412,12 +412,12 @@ function init_creationmodes(){ // check what is set automatically by the system on file creation // and set the fperm param if it's not what we want - $auto_fmode = $conf['fmode'] & ~$umask; + $auto_fmode = 0666 & ~$umask; if($auto_fmode != $conf['fmode']) $conf['fperm'] = $conf['fmode']; - // check what is set automatically by the system on file creation - // and set the dperm param if it's not what we want - $auto_dmode = $conf['dmode'] & ~$umask; + // check what is set automatically by the system on directory creation + // and set the dperm param if it's not what we want. + $auto_dmode = 0777 & ~$umask; if($auto_dmode != $conf['dmode']) $conf['dperm'] = $conf['dmode']; } diff --git a/inc/io.php b/inc/io.php index 18eff6858..72831a2da 100644 --- a/inc/io.php +++ b/inc/io.php @@ -408,7 +408,7 @@ function io_lock($file){ do { //waited longer than 3 seconds? -> stale lock if ((time() - $timeStart) > 3) break; - $locked = @mkdir($lockDir, $conf['dmode']); + $locked = @mkdir($lockDir); if($locked){ if($conf['dperm']) chmod($lockDir, $conf['dperm']); break; @@ -504,7 +504,7 @@ function io_mkdir_p($target){ if (file_exists($target) && !is_dir($target)) return 0; //recursion if (io_mkdir_p(substr($target,0,strrpos($target,'/')))){ - $ret = @mkdir($target,$conf['dmode']); // crawl back up & create dir tree + $ret = @mkdir($target); // crawl back up & create dir tree if($ret && !empty($conf['dperm'])) chmod($target, $conf['dperm']); return $ret; } |