Add restrict_permissions to FileLoggerBuilder#51
Conversation
|
Thank you for your PR! I'll review it tomorrow. |
sile
left a comment
There was a problem hiding this comment.
Thanks again. This PR looks basically good but I left a comment to reduce code complexity a bit.
src/permissions.rs
Outdated
| /// | ||
| /// This ensures the log files are not world-readable. | ||
| #[cfg(unix)] | ||
| pub fn restrict_file_permissions(file: File) -> io::Result<File> { |
There was a problem hiding this comment.
How about aligning function signatures between Unix and Windows versions?
For example, I think we could do as follows:
#[cfg(unix)]
pub fn restrict_file_permissions<P: AsRef<Path>>(_path: P, file: File) -> io::Result<File> {
...abbrev...
Ok(file)
}
#[cfg(windows)]
pub fn restrict_file_permissions<P: AsRef<Path>>(path: P, file: _File) -> io::Result<File> {
...abbrev...
Ok(file)
}By this change, the cfg branches in src/file.rs can be removed.
There was a problem hiding this comment.
In addition to the following comment, I thought it could be better to add another implementation for platforms other than Unix and Windows as follows:
#[cfg(not(any(unix, windows)))]
pub fn restrict_file_permissions<P: AsRef<Path>>(path: P, file: _File) -> io::Result<File> {
Err(io::Error::new(io::ErrorKind::Other, "`restrict_permissions` feature is only available on Unix or Windows"))
}By this addition, users, who even don't use Unix or Windows, could build sloggers without a "no such function" error.
There was a problem hiding this comment.
Yep good call. I've implemented both of these suggestions.
Pull Request Test Coverage Report for Build 1414790960Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
sile
left a comment
There was a problem hiding this comment.
Thank you for your swift action. LGTM.
|
Thank you for the quick review and merge! |
Resolves #17
This PR adds a new method to
FileLoggerBuilderwhich restricts log file permissions such that they are only readable by the file owner.This corresponds to:
600on Unix systems.600) on Windows systems.This is done specifically as a restriction rather than setting an arbitrary umask to ensure it worked across both Unix and Windows platforms.