Fix bogus directoryPerm 744 (Use 700 instead)

[glensc]

The 0744 directory permission is insane:

[root@d0b3bff7d8f0 test1]# mkdir test1
[root@d0b3bff7d8f0 test1]# chmod 744 test1
[root@d0b3bff7d8f0 test1]# ls -ld test1
drwxr--r-- 2 root root 4096 Nov 20 10:32 test1
[root@d0b3bff7d8f0 test1]#

it should be either 755 (all enter and readdir) or 711 (all enter, but no readdir), or 700 (nobody enter but owner), because read permission on a directory without execute bit is totally useless, you can't read directory if you can't enter it and to enter you need the execute bit.

for the change to be backward compatible, I decided to go with 700, but I really think the "public" was the intention here, i.e should be changed to 755 in 2.x

[glensc]

2.x does not have such property, it has visibility convertor, so, all good, but not really, found still one place that's incorrect:

• Fix private dir permission default for fromArray call flysystem#1234

[KeitelDOG]

I had the same problem, if you're saving a file, visibility is OK. But with directory, you end up not having access on remote server's www-data user for example. League SftpAdapter did not take the directoryPerm in config into consideration in createDir(). When hacking vendor code to put directoryPerm = 0755 everything is ok. So I was about to extends the FilesystemServiceProvider to replace the SftpAdapter with an extended one to avoid vendor hack.

Hopefully there is a version 2.x that convert permission for directories from visibility value. Gonna try that.

@glensc does version 2.x is stable enough for production?

[KeitelDOG]

Unfortunately, Laravel does not seem to support yet flysystem 2.x which is required to handle flysystem-sftp 2.x . I'm using Laravel, so it's not an option.

[KeitelDOG]

I have a great solution to this.

The problem is that createDirectory() method in SftpAdapter does not make use of the $config parameter to use the directoryPerm if available.

     /**
     * @var int
     */
    protected $directoryPerm = 0744;

    /**
     * @inheritdoc
     */
    public function createDir($dirname, Config $config)
    {
        $connection = $this->getConnection();

        if (! $connection->mkdir($dirname, $this->directoryPerm, true)) {
            return false;
        }

        return ['path' => $dirname];
    }

You can see that mkdir uses $this->directoryPerm without using the config like other functions. So it's kinda like a bug.

THE SOLUTION for this is to use the accessor method called setDirectoryPerm() and to set it manually each time like:

$filesystem = Storage::disk('sftp');
$filesystem->getDriver()->getAdapter()->setDirectoryPerm(0755);
$filesystem->put('dir1/dir2/test.txt', 'Hello World');

[glensc]

@KeitelDOG I don't know about 2.x: ask maintainers. Why this trivial change is still not merged: ask maintainers.

For me the workaround was just to set directoryPerm for SftpAdapter constructor:

        $configuration = [
            'host' => $options['ssh']['host'],
            'port' => $options['ssh']['port'],
            'username' => $options['ssh']['auth_username'],
            'password' => $options['ssh']['auth_password'],
            'privateKey' => $options['ssh']['auth_priv_keyfile'],
            'root' => $options['remote_dir'] ?: '/',
            'directoryPerm' => 0755,
            'timeout' => 10,
            'hostFingerprint' => $options['ssh']['server_fingerprint'],
        ];

        $mountConfig = [
            'ssh' => new Filesystem(new SftpAdapter($configuration)),
            'local' => new Filesystem(new Local('/')),
        ];
        $mountManager = new MountManager($mountConfig);

[frankdejonge]

I tend to respond quicker when people are nice and refrain from using terms like “insane”. Please remind yourself this is an open source project with one active maintainer who has a dayjob and a life.

[KeitelDOG]

I don't think switching from 744 to 700 is a fix, it's just a preference. I was expecting instead that createDirectory() method make use of $config parameter like:

/**
     * @inheritdoc
     */
    public function createDir($dirname, Config $config)
    {
        $connection = $this->getConnection();
        
        // 2 PROBLEMS (not using the $config before and now
        // Example 1: get directory perm from config if any, if not use directoryPerm class attribute;
        $directoryPerm = $config->get('directoryPerm') || $this->directoryPerm;
        // But the problem here is that, if directoryPerm is changed in controller, then config would always replace it

        // Example 2: get directory from config only if directoryPerm attributes has not been changed in controller
        // assuming an attribute for default dir perm
        // $this->defaultDirPerm = 0744;
        // $this->directoryPerm = null;
        if ($this->directoryPerm) {
            $directoryPerm = $this->directoryPerm;
        } else if ($config->get('directoryPerm')) {
            $directoryPerm = $config->get('directoryPerm');
        } else {
            $directoryPerm = $this->defaultDirPerm;
        }
        // The problem with that is that I have the feeling that it might break SFTP standard implementations
       // but it should make make use of both $config and $this->directoryPerm (maybe outside of this method) for more flexibility;

        if (! $connection->mkdir($dirname, $directoryPerm, true)) {
            return false;
        }

        return ['path' => $dirname];
    }

[glensc]

@KeitelDOG 744 to identical to 700 in behavior, having read permission without execute permission is totally useless for directories, and to show the intent it should be changed to 700 to preserve backward compatibility. it's all described in PR body.

what you seem to want is a new feature request: having createDir take permission from $config parameter. remember, you came to this PR and trying to fit your problem to this PR fix, it should be in the opposite direction: you describe the problem, and (some) PR tries to solve it.

so, dr;tl: file a feature request!

[KeitelDOG]

@glensc I understand what you mean. Although 700 would preserve backward compatibility on most use cases, but with some processes that may have just read or scanning contents info on the directory, while 744 would make it possible, 700 could crash it or make contents not available for reading anymore. That's why I don't really mind changing the 744.

But I totally agree with you on the PR approach. I just discussed it here, and if I receive feedback on what to take into consideration, then I would open an issue and do a PR for it. But anyway I found a good way around it, and version 1.x won't stay long anyway.

[glensc]

@KeitelDOG you are wrong, you can not "read" directory without "x"-bit! so 700 and 744 are identical in sense of how they behave.

[glensc]

The same fix for 2.x was accepted::

• https://github.com/thephpleague/flysystem/pull/1234/files

would be symmetrical to fix 1.x as well.

given that 700 and 744 are identical:

you can not "read" directory without "x"-bit! so 700 and 744 are identical in sense of how they behave.

• Fix bogus directoryPerm 744 (Use 700 instead) #107 (comment)

this is not a breaking change, but describes better the intent. 744 may leave user to believe the directory can be accessed by "group" or "other".