Implement store_perms & store_path_perms to specify file permissions

[HorlogeSkynet]

Hello 👋

Summary : This patch implements two new functions, allowing to pass file permissions to store* functions.

Rationale : Software configuration files usually contain secrets that should not be world-readable (default umask on most of operating systems).

As Rust does not support optional parameters, I've opted for fully-separate functions, while keeping diff simple by renaming current store_path function to (internal) do_store.

A test case has been added with UNIX file permissions.

Feel free to edit this branch as needed.

Bye 🙏

[Zykino]

Hello, thanks for the interest! I did some comments on the PR.

I am not sure this is good to include into confy: This change does not work on Windows. It also expect an understanding of Unix permissions.
Also users of this crates can trivially wrap this logic since they either give the configuration path or can retrieve it with get_configuration_file_path. This can permit them to think about how to handle permissions on each OS.

[HorlogeSkynet]

Hello, thanks for the interest! I did some comments on the PR.

Thanks for your quick review !

I am not sure this is good to include into confy: This change does not work on Windows.

set_permissions is not UNIX-specific. According to documentation it also corresponds to a low-level function on Windows.

It also expect an understanding of Unix permissions.

Yes, but previous (and simple) API will be still there if one doesn't know what they must/should set 🙂

Also users of this crates can trivially wrap this logic since they either give the configuration path or can retrieve it with get_configuration_file_path. This can permit them to think about how to handle permissions on each OS.

Yes, but we lose atomicity of saving configuration/secrets under the correct permissions (there is a time-window where data are written on disk with potentially too broad permissions, which is not the case when fchmod is directly used on file descriptor).

---

Bye 👋

[deg4uss3r]

Hello and thank you for this PR, overall I do like this change as it is non-breaking and adds some functionality into this API that makes sense.

My one follow up is if you could also write a Windows platform test that does the same as the unix one and checks the readonly metadata on a config file so they don't get out-of-sync.

[HorlogeSkynet]

Thanks for your message @deg4uss3r, I'll try to add such a test case soon™. Bye 👋

[HorlogeSkynet]

Hey @deg4uss3r, I've added a test_store_path_perms_readonly test case which deals with std::fs::Permissions::readonly (which is cross-platform). Tell me what you think about that ! 🙏

[deg4uss3r]

Approving the changes now with the additional tests added, the PR is not specific to UNIX (my initial concern as well) and does not break previous APIs the additional file permissions/metadata is an optional different path, nor does this change the file permissions on a newly created file using the old path it's all the same as before.

Thank you for this addition!

[deg4uss3r]

@Zykino Let me know if this addresses your original concerns if not happy to discuss more before merging.

[Zykino]

Sorry I wanted to help with an early review and wasn’t aware that on github that would require me to follow through. I have a little nit… but won’t block on that.

I’m a bit surprised by the fact that you can set a file to readonly and then write to it. But apparently rust (or the OS?) authorize it, since this property is checked at the moment we open the file and not at the call to write. See this playground if I’m not clear.

[deg4uss3r]

Merged, thank you!