afero
afero copied to clipboard
spf13
File permissions don't get checked by Open
It's possible to use the memmap fs to create a file, chmod it to 0, and then open it without a problem. When using memmap to simulate a filesystem in unit tests, this behaviour is surprising.
Adding this to memmap.go's MemMapFs.open() just before the final return does the trick
if mem.GetFileInfo(f).Mode().Perm()&0444 == 0 {
return nil, &os.PathError{Op: "open", Path: name, Err: os.ErrPermission}
}
but we also need a m.Chmod(0666) in Create, which then matches the documentation for os.Create() : https://golang.org/pkg/os/#Create
After applying only these changes, the afero_test fails because of some more permission problems in tempfile and tempdir creation I haven't tracked down yet.
I just ran into this one myself, was really surprised by the behavior, I thought I had a bug in my code.
Is this is still an issue on current version?
I tried this: https://play.golang.org/p/LIY--dJzmrS
Am I missing something?
How is it possibile to test for wrong permissions?
This is ~easier~ harder than it sounds - there's lots of corner cases. If MemMapFs is going to check ownership and modes, which user and group should it assume the caller is? Should there be a way to change the calling uid and gid (this would MemTestFs a lot more complicated)? If your code runs as root, a mode of 0 still doesn't prevent your program from reading a file or directory - should MemTestFs account for that too? Handling nested directory permissions correctly will also be super hard. There's probably lots more that I'm not thinking of.
If you want specific MemMapFs calls to fail in tests it might be better to introduce a helper type which embeds a MemMapFs and implements afero.Fs, and use that as the test files system. This could then intercept specific calls and paths that should return an error (passing any other calls through to the embedded MemMapFs). This is likely going to be simpler and would provide exact control over what should fail and how.
@mjs, do you mean "harder than it sounds"? Can you elaborate on what makes directory permissions "super hard"?
I think there are some reasonable and unsurprising shortcuts that can be made to obviate most of the corner cases:
- Assume the caller's uid and gid match those on the file.
- Don't worry about root gotchyas.
The test framework you describe sounds to me messier than a basic permissions implementation in MemMapFs.
@mjs, do you mean "harder than it sounds"?
Yes sorry, it was late for me when I wrote this. I meant to say "not as easy as it sounds".
Can you elaborate on what makes directory permissions "super hard"?
Well to do it right MemMapFs would need to walk up from the root and check access on all parent directories, and then check file permissions (for file opens). "Super hard" was perhaps too strong - "non trivial" is a better choice.
I think there are some reasonable and unsurprising shortcuts that can be made to obviate most of the corner cases:
- Assume the caller's uid and gud match those on the file.
Ok but this is weird. On a real filesystem, no-one but root can generally chown a file so we're going to pretend that the caller is root for chown, and an arbitrary and potentially variable user for most other operations. At least with the current implementation it behaves as if you're root all the time which is easier to think about.
- Don't worry about root gotchyas.
These shortcuts will lead to situations where tests pass but code will fail in production. We already have this now but MemMapFs will operate in even more surprising ways with these special rules.
These special cases would need to be clearly documented and will be harder to hold in a developer's head.
The test framework you describe sounds to me messier than a basic permissions implementation in
MemMapFs.
I guess that's a matter of opinion. It would be small and very simple, and could end up as part of afero so it only needs to be written once.
If you want specific MemMapFs calls to fail in tests it might be better to introduce a helper type which embeds a MemMapFs and implements afero.Fs, and use that as the test files system. This could then intercept specific calls and paths that should return an error (passing any other calls through to the embedded MemMapFs). This is likely going to be simpler and would provide exact control over what should fail and how.
@mjs how would this implementation look like? Not being able to check permission errors on file operations reduces coverage by a significant percentage for a number of applications.
Being relatively new to Go and Afero, is there a way of testing permissions with Afero, maybe without MemMapFs?
@everdrone I believe this is what @mjs is suggesting:
type MockFs struct {
afero.MemMapFs
}
func (m *MockFs) Open(name string) (afero.File, error) {
if name == "/some/path/name" {
return nil, fs.ErrPermission
}
return m.MemMapFs.Open(name)
}
This worked for me in my tests. If it's something you need to do a lot of, you can abstract it further so that "/some/path/name" isn't hard coded.
See https://eli.thegreenplace.net/2020/embedding-in-go-part-1-structs-in-structs/ for detail on struct embedding if you're not familiar.
This is exactly what I've done in a codebase I work on (not open source so can't share easily). I took it a little further so that the test wrapper filesystem was fairly generic, implementing all the Fs methods with the ability to fail for specific paths and operations. That's just a small matter of typing :)
In each test you set the filesystem used by the package to a MockFs instance (or whatever you call it - I used ErrorFs) which wraps a MemMapFs instead of using MemMapFs directly, setting the paths and operations you want to fail for that test.
Just ran into this issue myself and found this convo while researching it. Perhaps the majority of the problem here could be resolved by adding something to the documentation for MemMapFs that explicitly states that all permissions are treated as root. That way at least consumers have a chance to avoid the confusion.
