aboutsummaryrefslogtreecommitdiffstatshomepage
diff options
context:
space:
mode:
authorAndreas Gohr <andi@splitbrain.org>2021-08-14 11:06:31 +0200
committerAndreas Gohr <andi@splitbrain.org>2021-08-14 11:43:40 +0200
commitbd5391243cdd92d9e0abee5ba1a506170b18d072 (patch)
tree8cbe9f3e5722a3da7294a081d755f3e711f4c527
parent1e519eb5339e02a75ee7aba2a1a5555f09ff8818 (diff)
downloaddokuwiki-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.php109
-rw-r--r--inc/Search/Indexer.php2
-rw-r--r--inc/Subscriptions/BulkSubscriptionSender.php2
-rw-r--r--inc/init.php8
-rw-r--r--inc/io.php4
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;
}