Make permission check more lenient. #163
Conversation
Instead of enforcing an arbitrary permission policy, check permissions against current umask. If permissions are changed from the default, this might be a hint that something shady is going on. Resolves MichaelAquilina#152.
| @@ -179,15 +179,21 @@ function check_venv() | |||
| file_owner="$(/usr/bin/stat -f %u "$venv_path")" | |||
| file_permissions="$(/usr/bin/stat -f %OLp "$venv_path")" | |||
| fi | |||
| if [[ -d "$venv_path" ]]; then | |||
| default_permissions=777 | |||
There was a problem hiding this comment.
There might be something I'm missing here - so please correct me if I am wrong
But this seems to change the plugin to require the activated file to be executable, writeable and readable by everyone on the system?
There was a problem hiding this comment.
Ah wait - you're specifically checking if its a directory here? I'm a bit lost about what we are covering here then?
There was a problem hiding this comment.
This code is to figure out what the default permissions of the file would be:
- If it's a directory, e.g. a
.venvfolder, the default permissions are777 & ~umask - If it's a regular file, e.g. a
Pipfile, the default permissions are666 & ~umask
So I'm checking for whether it's a directory to figure out what octal value I need to bit-wise AND with umask.
There was a problem hiding this comment.
@MichaelAquilina : Would you be willing to accept an updated/rebased version of this PR? I'm been getting hit with this quite often recently when working with a poetry based env. A lot of the poetry commands seem to rewrite poetry.lock, causing its permission to be reverted to defaults. Since those 664 on my system (Ubuntu 20.04 Desktop), I get this warning every time after the file has been touched :(
There was a problem hiding this comment.
Any update on this? I've also run into the poetry.lock file permissions issue.
Edit: For anyone else who gets here: It appears that poetry respects the permissions of the parent directory, so if you change your project folder to 644 or stricter it seems to work.
There was a problem hiding this comment.
Side note: I've been using my patch for quite a while, and it's much better, but I've still been running into the problem occasionally (not sure why and don't care enough to track down what's going on), so I'm inclined to just remove the check in my fork entirely... still think it's a useful check in the case where $venv_path is an actual venv folder but for me, it never is.
Triggers repeatedly when using the plugin in conjunction with poetry (see [0]). [0]: MichaelAquilina#163
Instead of enforcing an arbitrary permission policy, check permissions against current umask.
If permissions are changed from the default, this might be a hint that something shady is going on.
Resolves #152.