Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bogus directoryPerm 744 (Use 700 instead) #107

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glensc
Copy link

@glensc glensc commented Nov 20, 2020

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
Copy link
Author

glensc commented Nov 20, 2020

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

@KeitelDOG
Copy link

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
Copy link

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
Copy link

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
Copy link
Author

glensc commented Dec 27, 2020

@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
Copy link
Member

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
Copy link

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
Copy link
Author

glensc commented Dec 29, 2020

@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
Copy link

KeitelDOG commented Dec 29, 2020

@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
Copy link
Author

glensc commented Dec 30, 2020

@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
Copy link
Author

glensc commented Dec 29, 2021

The same fix for 2.x was accepted::

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants