streamich / memfs

JavaScript file system utilities
http://streamich.github.io/memfs/
Apache License 2.0
1.76k stars 129 forks source link

Permissions support #1009

Closed wilcoxmd closed 1 month ago

wilcoxmd commented 8 months ago

Hi! What level of support does memfs have for permissions today?

I'm migrating from mock-fs, which allowed me to create read-only files in tests with something like

mockFs({
  fileName: mockFs.file({ mode: 0o444 }); 
})

In memfs, it seems the read-only permissions aren't respected, i.e. this doesn't throw an error like I'd expect:

vol.fromJSON({
  fileName: 'some existing content',
});

vol.chmodSync('fileName', 0o444);
vol.writeFileSync('fileName', 'new content'); // doesn't throw

Should I expect this to work, or perhaps am I doing something incorrect?

Thanks!

G-Rath commented 8 months ago

As far as I know we don't have any special support for permissions, which feels right given they're arguably more of an OS concern.

wilcoxmd commented 8 months ago

they're arguably more of an OS concern

That's fair. My use case for checking this in a test is to make sure that errors are properly bubbling up in a CLI. I suppose I can work around this by mocking fs.writeFile for this particular test and having it throw.

G-Rath commented 8 months ago

Yeah that's what I do in my tests for those kind of cases - there's also been discussion and work in the past to add support for readonly and writeonly modes for both unionfs and memfs, which I think would be a useful addition but they got stalled due to age.

wilcoxmd commented 8 months ago

Got it. Yea, I agree they would be helpful additions. For now I can workaround this, so I'll close the issue. Thanks for the quick responses!

BadIdeaException commented 2 months ago

they're arguably more of an OS concern

I am inclined to disagree with this, even though I will concede they're kind of straddling the fence. Besides the somewhat academic question of OS vs FS concern, though, there is also a very practical one about permissions not being supported:

One of the major use cases for this library is test mocking. Not supporting permissions makes a major source of bugs inaccessible to those tests. And yes, we can mock the respective method, but in that case, what is the point of using a mock fs in the first place? In addition, it requires to know which exact fs method will be used, thereby leaking implementation details.

I would love to see this reopened and implemented. I can see from the code that permissions are already tracked, and from doing some simple tryouts, can see that there is (incomplete) support for them:

const FILE = '/test.txt';
fs.writeFileSync(FILE, 'foo', { mode: 0 });
fs.readFileSync(FILE); // EACCES
G-Rath commented 2 months ago

@BadIdeaException I get your point and I'm not against PRs for this - my main concern is that historically this sort of thing has had differences between Windows and Linux so I don't want to commit to maintaining a bunch of code for archiving 1:1 parity across different OSs and different versions of Node.

I'm not saying that will happen, which is why I say PRs are welcome, I'm just flagging it as something people should be prepared to handle and such if/when it comes up.

I will leave this closed for now but feel free to open a strawperson PR showing me how it could be done without a lot of overhead and we can go from there 🙂

BadIdeaException commented 2 months ago

Understood. I've been looking through your code to see where it could be implemented (that's how I knew you were already tracking permissions). I'll keep playing around with it a bit and see if I can come up with something. 🙂

As far as maintenance goes: I'm not aware that the POSIX permission system, which is what is modeled by the Node fs interface, has changed for a long time. If, however, different Node versions had bugs in the past, arguably those shouldn't be reproduced anyways. For dealing with MS Windows quirks, I'm definitely the wrong person - I turned my back on that years ago. But maybe somebody else will step up for that.

github-actions[bot] commented 1 month ago

:tada: This issue has been resolved in version 4.12.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: