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

BUG: Default ACLs ignored when adding new file #1369

Open
3 tasks done
Ian2020 opened this issue Feb 29, 2024 · 9 comments
Open
3 tasks done

BUG: Default ACLs ignored when adding new file #1369

Ian2020 opened this issue Feb 29, 2024 · 9 comments
Labels
bug Something isn't working help wanted Extra attention is needed question Further information is requested

Comments

@Ian2020
Copy link

Ian2020 commented Feb 29, 2024

Did you check docs and existing issues?

  • I have read all the docs.
  • I have searched the existing issues.
  • I have searched the existing discussions.

Neovim Version (nvim -v)

0.9.5

Operating System / Version

Fedora 38

Describe the Bug

When I use neo-tree to create a file in a directory with a default access control list (ACL) the permissions of the new file do not respect that ACL. If I create/write a file using plain vim i.e. :w file then the file does respect the ACL.

It looks like permissions on new files are hard-code here(?): https://github.com/nvim-neo-tree/neo-tree.nvim/blob/main/lua/neo-tree/sources/filesystem/lib/fs_actions.lua#L396

Screenshots, Traceback

No response

Steps to Reproduce

  1. mkdir /tmp/blah
  2. Give groups rw perms by default on new files: setfacl -dm g::rw- /tmp/blah
  3. Use neo-tree to create a file in /tmp/blah
  4. ls -al /tmp/blah - observe the new file is missing group:rw. Contrast with permissions of a file created with touch /tmp/blah/new or by :w /tmp/blah/new in nvim.

Expected Behavior

It would be great if neo-tree let the filesystem decide the correct permissions so if a user has custom ACLs etc they are respected.

Your Configuration

-- DO NOT change the paths and don't remove the colorscheme
local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
  vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({ "git", "clone", "--filter=blob:none", "https://github.com/folke/lazy.nvim.git", lazypath, })
end
vim.opt.runtimepath:prepend(lazypath)

-- install plugins
local plugins = {
  "folke/tokyonight.nvim",
  -- add any other plugins here
}

local neotree_config = {
  "nvim-neo-tree/neo-tree.nvim",
  dependencies = { "MunifTanjim/nui.nvim", "nvim-tree/nvim-web-devicons", "nvim-lua/plenary.nvim" },
  cmd = { "Neotree" },
  keys = {
    { "<Leader>e", "<Cmd>Neotree<CR>" }, -- change or remove this line if relevant.
  },
  opts = {
    -- Your config here
    -- ...
  },
}

table.insert(plugins, neotree_config)
require("lazy").setup(plugins, {
  root = root .. "/plugins",
})

vim.cmd.colorscheme("tokyonight")
-- add anything else here
@Ian2020 Ian2020 added the bug Something isn't working label Feb 29, 2024
@pysan3
Copy link
Collaborator

pysan3 commented Mar 12, 2024

Yes indeed, the file permission for new files are hardcoded.

I'm not very familiar with ACL but is there a way to access the value in lua (especially neovim lua)?

@cseickel
Since @bwpge has done a great job with #1353, shall we use vim.cmd.write(utils.escape_path_for_cmd(path)) to use the full power of vim instead?

@pysan3 pysan3 added help wanted Extra attention is needed question Further information is requested labels Mar 12, 2024
@cseickel
Copy link
Contributor

cseickel commented Mar 12, 2024

Since @bwpge has done a great job with #1353, shall we use vim.cmd.write(utils.escape_path_for_cmd(path)) to use the full power of vim instead?

Does it matter in this case? It doesn't seem like there is a bug to fix regarding creating new files on Windows.

Regarding the ACL, I wonder if we even need to set the ACL at all. What happens if we don't set it explicitly?

@bwpge
Copy link
Contributor

bwpge commented Mar 12, 2024

Just did a bit of digging and it seems that fs_open requires setting these explicitly (see libuv: fs_open), used here:

local open_mode = loop.constants.O_CREAT
+ loop.constants.O_WRONLY
+ loop.constants.O_TRUNC
local fd = loop.fs_open(destination, open_mode, 420)

I think maybe we could just use io.open here as basically a drop in replacement, but I'm just guessing and I'm not sure how nicely this plays with vim.loop. The following test script creates the file with correct permissions:

local file = io.open("foo.txt", "w")
if file then
    file:close()
end

@pysan3
Copy link
Collaborator

pysan3 commented Mar 12, 2024

Since @bwpge has done a great job with #1353, shall we use vim.cmd.write(utils.escape_path_for_cmd(path)) to use the full power of vim instead?

Does it matter in this case? It doesn't seem like there is a bug to fix regarding creating new files on Windows.

@cseickel
I meant that we were previously too afraid to pass lua string representation of filepath to a vim command because it creates such a mess ESPECIALLY on Windows.
Thanks to @bwpge this is not the case anymore, and I thought we can trust vim.cmd.write("a-lua-string") enough to use it as a substitution for vim.loop.fs_open.

io.open

@bwpge Does it work when parent directories do not exist? I mean if we still need to call vim.loop.fs_mkdir (and AFAIK this also needs hardcoded permissions), io.open unfortunately does not solve the whole problem whereas we have :w ++p in vimscript.

@bwpge
Copy link
Contributor

bwpge commented Mar 12, 2024

@bwpge Does it work when parent directories do not exist? I mean if we still need to call vim.loop.fs_mkdir (and AFAIK this also needs hardcoded permissions), io.open unfortunately does not solve the whole problem.

Ah ok yes I see, right above my linked section LOL. No io.open would not handle directories. Could we use something like vim.fn.mkdir("...", "p") for this? Might simplify the create_all_parents function as well.

@bwpge
Copy link
Contributor

bwpge commented Mar 13, 2024

Did some quick experimenting on WSL, I was able to reproduce the issue with the steps as documented with setfacl.

Using io.open as a drop in replacement to create the file, I could create the files with correct mode (bar.txt below was from the repro steps). However, I'm not sure that the directory has the correct mode -- I'm not super familiar with ACL stuff:

$ lsd -la --tree /tmp/foo

    drwxr-xr-x bwpge bwpge 4.0 KB Tue Mar 12 21:22:37 2024 .
X   .rw-r--r-- bwpge bwpge   0 B  Tue Mar 12 21:21:44 2024 ├── bar.txt  <-- from repro
✓   .rw-rw-r-- bwpge bwpge   0 B  Tue Mar 12 21:22:07 2024 ├── baz.txt
✓   .rw-rw-r-- bwpge bwpge   0 B  Tue Mar 12 21:21:03 2024 ├── foo.txt
??? drwxr--r-x bwpge bwpge 4.0 KB Tue Mar 12 21:22:41 2024 └── nested
✓   .rw-rw-r-- bwpge bwpge   0 B  Tue Mar 12 21:22:41 2024     └── foo2.txt

@Ian2020 can you confirm if this is the expected behavior we want? The default ACL was set with setfacl -dm g::rw- /tmp/foo, and this is using vim.fn.mkdir (e.g. builtin mkdir()).

@Ian2020
Copy link
Author

Ian2020 commented Mar 13, 2024

I think the dir nested is missing group-write permissions:

When you set a default ACL on a directory, the entry you specify will automatically be applied to every new file and directory within it, regardless of who creates them.

From https://www.howtogeek.com/how-to-use-filesystem-acl-on-linux/

If I use the normal systems tools to make another dir in /tmp/foo i.e. running mkdir nested2 I see:

...
drwxrw-r-x+  2 ian  ian    40 Mar 13 11:38 nested2

...so the nested dir does get group:write permissions, on my system at least.

However when I use :w /tmp/foo/nested3/foo.txt from inside neovim to force creation of nested3 it is also missing group-write permission. So I think neovim is also disregarding ACLs on intermediate directories.

A quick look at calls to os_file_mkdir in the neovim codebase shows hardcoded 0755 permissions:

So I could raise a bug with neovim to respect ACLs on intermediate dirs if that will ultimately make it easier to fix here?

@cseickel
Copy link
Contributor

You know, it occurs to me that maybe we should attempt to use shell commands to create files and folders instead of libuv. We do that with rm for deleting things in *nix systems. AFAIK, there's just *nix style and Windows, so not really that many cases to handle.

mkdir -p would certainly be simpler than manually creating all intermediate directories, and then permissions would be passed off to the OS where they belong.

@bwpge
Copy link
Contributor

bwpge commented Mar 15, 2024

You know, it occurs to me that maybe we should attempt to use shell commands to create files and folders instead of libuv. We do that with rm for deleting things in *nix systems. AFAIK, there's just *nix style and Windows, so not really that many cases to handle.

Just throwing my two cents in for whatever it's worth:

For file creation, I would not want to rely on shell commands. Windows usually defaults most stuff to UTF-16 encoding unless you go out of your way to deal with it, which can get super annoying. Lua's io.open is fairly close to the metal, being a thin wrapper around a FILE* and is essentially a drop-in replacement for what we already have implemented, so it would be the easiest to use with the least amount of surprises.

From @Ian2020's findings, I wouldn't hold my breath for Neovim to change the directory creation behavior anytime soon.. so maybe relying on the OS commands to make directories is the right approach if we really want to get it right.

As with all things, Windows can do somewhat the same as POSIX style mkdir -p with md, but has some goofy usage -- for example, md foo/bar doesn't work but md "foo/bar" does and md foo\bar does as well (which lands us back to weird quoting problem territory, which I know @cseickel is completely sick of hearing about lol).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants