Add single file encryption and decryption #17
Conversation
jkmassel
force-pushed
the
add/single-file-encryption-and-decryption
branch
from
034a9fa to
2a6d89b
Compare
jkmassel
force-pushed
the
add/single-file-encryption-and-decryption
branch
from
9d7fee9 to
59a98e6
Compare
| @@ -136,7 +142,7 @@ dependencies = [ | |||
|
|
|||
| [[package]] | |||
| name = "configure" | |||
| version = "0.5.0" | |||
| version = "0.6.1" | |||
There was a problem hiding this comment.
This was an oversight on my part during the Gradle Plugin updates - sorry about that!
There was a problem hiding this comment.
@jkmassel I've reviewed vast majority of the changes and already left quite a few questions for you which may impact how I review the rest of the PR. I am about to jump into a 1:1 and I thought this was a good start for us to discuss the PR. I'll do another full review once we are done with the line comment discussions.
Thank you for the opportunity to review some Rust code, it's quite exciting even if I don't get to work on it yet. I hope you won't regret asking my review with how many questions I have left so far 😿
src/bin.rs
Outdated
| input_file: String, | ||
|
|
||
| #[structopt(short = "o", long = "output-file")] | ||
| output_file: String, |
There was a problem hiding this comment.
Do we have to ask for an output file? Can it be at least optional, so we just add .enc at the end of the input file name when it's not given?
There was a problem hiding this comment.
Addressed in d23064f!
src/lib.rs
Outdated
| std::process::exit(err as i32); | ||
| } | ||
| }, | ||
| None => crate::encryption::generate_key(), |
There was a problem hiding this comment.
This part is a little interesting to me. If we generate a key ourselves and not tell the user what the key is how can they decrypt it after? The answer to that question will change how I review some of the code around it, so I am going to hold off on commenting on those for now.
There was a problem hiding this comment.
This was also addressed in d23064f, because yeah, that made no sense 🙈
|
@oguzkocer This is ready for another review at your convenience! |
There was a problem hiding this comment.
@jkmassel I followed up on the comments and did a partial review of the changes and I think we are good to go. It'd be great if you can check the comment about magic number as I feel it'd be a good change before merging, but it's up to you.
Note that, I didn't do a full review this time around. I think it's a little difficult to follow up on this PR since it's not a priority for either one of us. So, if there is anything to be improved, I think we should just do that in a future PR.
| #[test] | ||
| fn test_encrypted_filename_can_be_derived_from_original_filename_for_files_with_extension() { | ||
| let source = Path::new("/test.json").to_path_buf(); | ||
| let dest = Path::new("/test.json.enc").to_path_buf(); | ||
|
|
||
| assert_eq!(infer_encryption_output_filename(&source), dest) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_encrypted_filename_can_be_derived_from_original_filename_for_files_without_extension() { | ||
| let source = Path::new("/Gemfile").to_path_buf(); | ||
| let dest = Path::new("/Gemfile.enc").to_path_buf(); | ||
|
|
||
| assert_eq!(infer_encryption_output_filename(&source), dest) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_decrypted_filename_can_be_derived_from_encrypted_filename_for_files_with_extension() { | ||
| let source = Path::new("/test.json.enc").to_path_buf(); | ||
| let dest = Path::new("/test.json").to_path_buf(); | ||
|
|
||
| assert_eq!(infer_decryption_output_filename(&source), dest) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_decrypted_filename_can_be_derived_from_original_filename_for_files_without_extension() { | ||
| let source = Path::new("/Gemfile.enc").to_path_buf(); | ||
| let dest = Path::new("/Gemfile").to_path_buf(); | ||
|
|
||
| assert_eq!(infer_decryption_output_filename(&source), dest) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_decrypted_filename_can_be_derived_from_original_filename_for_files_without_extension_or_suffix( | ||
| ) { | ||
| let source = Path::new("/Gemfile").to_path_buf(); | ||
| let dest = Path::new("/Gemfile.decrypted").to_path_buf(); | ||
|
|
||
| assert_eq!(infer_decryption_output_filename(&source), dest) | ||
| } |
There was a problem hiding this comment.
It might be worth making these tests parametrized in the future, since the logic is common. However, I am not 100% sure if it'd be an improvement right now, because we'd probably need to use a macro, and macros are harder to read. 🤔
There was a problem hiding this comment.
Ooh yeah that'd be nice, especially if it's not too magical 😅
| output_file: Option<String>, | ||
| encryption_key_string: Option<String>, | ||
| ) { | ||
| let input_file_path = Path::new(input_file).to_path_buf(); |
There was a problem hiding this comment.
It looks like I missed this in my initial review. Since PathBuf implements From & Into traits for strings, we can do PathBuf::from(input_file) instead. This approach seems to be the preferred one in the documentation.
With Into trait, we don't even need the extra input_file_path variable, we can just pass it into encrypt_single_file like so:
encrypt_single_file(input_file.into(), output_file_path, encryption_key_string)
Path::new().to_path_buf is heavily used within this PR, so we probably don't want to make this change in this PR. However, if you agree that this is a good change, I can open a separate PR addressing similar issues in one go. What do you think @jkmassel?
There was a problem hiding this comment.
Yeah, I'm good with any change that makes things more idiomatic for sure!!
Co-authored-by: Oguz Kocer <oguzkocer@users.noreply.github.com>
This work allows encrypting and decrypting single files, and does some low-level work to improve error messaging.
It also adds tests around the core encryption stuff in order to add more confidence.