Add a --chmod option to set file mode on uploaded files#1506
Add a --chmod option to set file mode on uploaded files#1506svenstaro merged 10 commits intosvenstaro:masterfrom
Conversation
svenstaro
left a comment
svenstaro
left a comment
There was a problem hiding this comment.
Hey, this is excellent! Make sure to also update the README, please.
tests/upload_files.rs
Outdated
| #[cfg(unix)] | ||
| #[rstest] | ||
| #[case(server(&["-u"]), 0o600)] | ||
| #[case(server(&["-u", "--chmod", "660"]), 0o660)] | ||
| #[case(server(&["-u", "--chmod", "644"]), 0o644)] | ||
| fn chmod(#[case] server: TestServer, #[case] expected_mode: u32) -> Result<(), Error> { |
There was a problem hiding this comment.
Seems like a good test but please add a doc comment explaining the intention. I'd also love to see a second test that tests the chmod error in case you don't have permissions for the chmod but I recognize it might be hard. If you don't find an easy way to test that, it's fine.
There was a problem hiding this comment.
Yes, it's hard because the file owner can always chmod the file.
src/args.rs
Outdated
| long = "chmod", | ||
| value_parser(parse_file_mode), | ||
| env = "MINISERVE_CHMOD", | ||
| default_value = "0600" |
There was a problem hiding this comment.
So far, the default behavior was that we uploaded with default permissions of the underlying system. This changes it and that's a problem. If this option is not set, we shouldn't also apply permissions that differ from the system's default.
There was a problem hiding this comment.
So far, the default behavior was that we uploaded with default permissions of the underlying system.
This isn't the case since 413a63a. The file is created with 0600 since then (and it is a problem for me, thus this pr).
I can change it default to what umask says.
There was a problem hiding this comment.
Oh crap. Well, looks like no one noticed. Might as well leave your change as it is, then.
There was a problem hiding this comment.
I've implemented this. It's pretty simple, but I don't know how to write the test (I can't use the utility function to determine the default file mode).
Co-authored-by: Sven-Hendrik Haase <[email protected]>
|
The CI failure looks legit. Wanna look into it? |
Linux wants u32 and MacOS wants u16, and u16 is enough for our purpose.
|
I hope it works on all three platforms this time... |
|
This looks great! Thanks! Merging as-is. Sorry for taking some time on this. |
2 participants
see also #124.
I haven't looked into tests / docs yet. It works for me so I send it first to get early feedback.