Copilot commented on code in PR #2208:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2208#discussion_r3506131069
##########
minifi_rust/extensions/minifi_pgp/features/encrypt_decrypt.feature:
##########
@@ -0,0 +1,54 @@
+@SUPPORTS_WINDOWS
+Feature: Test PGP extension's encryption and decryption capabilities
+
+ Background: The pgp library is successfully built on linux
+
+ Scenario: The pgp library is loaded into minifi
+ Given log property
"logger.org::apache::nifi::minifi::core::extension::ExtensionManager" is set to
"TRACE,stderr"
+ And log property "logger.org::apache::nifi::minifi::core::ClassLoader" is
set to "TRACE,stderr"
+
+ When the MiNiFi instance starts up
+
+ Then the Minifi logs contain the following message: "Registering class
'EncryptContentPGP' at '/minifi_pgp'" in less than 10 seconds
+ And the Minifi logs contain the following message: "Registering class
'DecryptContentPGP' at '/minifi_pgp'" in less than 1 seconds
+ And the Minifi logs contain the following message: "Registering class
'PGPPublicKeyService' at '/minifi_pgp'" in less than 1 seconds
+ And the Minifi logs contain the following message: "Registering class
'PGPPrivateKeyService' at '/minifi_pgp'" in less than 1 seconds
+ And the Minifi logs do not contain errors
+ And the Minifi logs do not contain warnings
+
+ Scenario: Encrypted for Alice but not for Bob
+ Given log property
"logger.minifi_pgp::processors::decrypt_content::DecryptContentPGP" is set to
"TRACE,stderr"
+ And log property
"logger.minigi_pgp::processors::encrypt_content::EncryptContentPGP" is set to
"TRACE,stderr"
Review Comment:
The logger property key has a typo ("minigi_pgp"), so the EncryptContentPGP
logger level will not be applied during this scenario.
##########
minifi_rust/extensions/minifi_pgp/src/processors/decrypt_content/output_attributes.rs:
##########
@@ -0,0 +1,13 @@
+use minifi_native::OutputAttribute;
+
+pub(crate) const LITERAL_DATA_FILENAME: OutputAttribute = OutputAttribute {
+ name: "pgp.literal.data.filename",
+ relationships: &["success"],
+ description: "Filename from decrypted Literal Data (Note that OpenPGP
signatures do not include the formatting octet, the file name, and the date
field of the Literal Data packet in a signature hash; therefore, those fields
are not protected against tampering in a signed document. Therefore a lot of
implementation omit these inherently malleable metadata)",
Review Comment:
The output-attribute description has a grammatical error: "a lot of
implementation omit" should be plural ("implementations"). This string is part
of user-facing documentation/metadata.
##########
minifi_rust/extensions/minifi_pgp/src/processors/decrypt_content/output_attributes.rs:
##########
@@ -0,0 +1,13 @@
+use minifi_native::OutputAttribute;
+
+pub(crate) const LITERAL_DATA_FILENAME: OutputAttribute = OutputAttribute {
+ name: "pgp.literal.data.filename",
+ relationships: &["success"],
+ description: "Filename from decrypted Literal Data (Note that OpenPGP
signatures do not include the formatting octet, the file name, and the date
field of the Literal Data packet in a signature hash; therefore, those fields
are not protected against tampering in a signed document. Therefore a lot of
implementation omit these inherently malleable metadata)",
+};
+
+pub(crate) const LITERAL_DATA_MODIFIED: OutputAttribute = OutputAttribute {
+ name: "pgp.literal.data.modified",
+ relationships: &["success"],
+ description: "Modified Date from decrypted Literal Data (Note that OpenPGP
signatures do not include the formatting octet, the file name, and the date
field of the Literal Data packet in a signature hash; therefore, those fields
are not protected against tampering in a signed document. Therefore a lot of
implementation omit these inherently malleable metadata)",
Review Comment:
The output-attribute description has a grammatical error: "a lot of
implementation omit" should be plural ("implementations"). This string is part
of user-facing documentation/metadata.
##########
minifi_rust/extensions/minifi_pgp/src/processors/decrypt_content/tests.rs:
##########
@@ -0,0 +1,253 @@
+use crate::controller_services::private_key_service::PGPPrivateKeyService;
+use crate::processors::decrypt_content::{DecryptContentPGP, output_attributes};
+use crate::test_utils;
+use crate::test_utils::get_test_message;
+use minifi_native::{
+ ComponentIdentifier, EnableControllerService, FlowFileStreamTransform,
IoState,
+ MockControllerServiceContext, MockLogger, MockProcessContext, Schedule,
+};
+
+#[test]
+fn test_ids() {
+ assert_eq!(
+ DecryptContentPGP::CLASS_NAME,
+ "minifi_pgp::processors::decrypt_content::DecryptContentPGP"
+ );
+ assert_eq!(DecryptContentPGP::GROUP_NAME, "minifi_pgp");
+ assert_eq!(DecryptContentPGP::VERSION, "0.1.0");
+}
+
+#[test]
+fn fails_to_schedule_by_default() {
+ let decrypt_content =
+ DecryptContentPGP::schedule(&MockProcessContext::new(),
&MockLogger::new());
+ assert!(decrypt_content.is_err());
+}
+
+#[test]
+fn schedules_with_password() {
+ let mut context = MockProcessContext::new();
+ context.properties.insert(
+ super::properties::SYMMETRIC_PASSWORD.name.to_string(),
+ "my_secret_password".to_string(),
+ );
+ let decrypt_content = DecryptContentPGP::schedule(&context,
&MockLogger::new());
+ assert!(decrypt_content.is_ok());
+}
+
+#[test]
+fn schedules_with_controller() {
+ let mut context = MockProcessContext::new();
+ context.properties.insert(
+ super::properties::PRIVATE_KEY_SERVICE.name.to_string(),
+ "my_private_key_service".to_string(),
+ );
+ let decrypt_content = DecryptContentPGP::schedule(&context,
&MockLogger::new());
+ assert!(decrypt_content.is_ok());
+}
+
+#[test]
+fn schedule_rejects_invalid_strategy_without_panicking() {
+ let mut context = MockProcessContext::new();
+ context.properties.insert(
+ super::properties::DECRYPTION_STRATEGY.name.to_string(),
+ "NOT_A_STRATEGY".to_string(),
+ );
+ context.properties.insert(
+ super::properties::SYMMETRIC_PASSWORD.name.to_string(),
+ "my_secret_password".to_string(),
+ );
+ // Must return Err, not panic.
+ let result = DecryptContentPGP::schedule(&context, &MockLogger::new());
+ assert!(result.is_err(), "expected schedule to fail on bad strategy");
+}
+
+#[derive(Copy, Clone)]
+struct PrivateKeyData {
+ key_filename: &'static str,
+ passphrase: Option<&'static str>,
+}
+
+impl PrivateKeyData {
+ fn into_controller(self) -> PGPPrivateKeyService {
+ let mut context = MockControllerServiceContext::new();
+ context
+ .properties
+ .insert("Key File",
test_utils::get_test_key_path(self.key_filename));
+
+ if let Some(passphrase) = self.passphrase {
+ context.properties.insert("Key Passphrase", passphrase);
+ }
+
+ PGPPrivateKeyService::enable(&context,
&MockLogger::new()).expect("should enable")
+ }
+}
+
+fn test_decryption(
+ message_file_name: &str,
+ private_key_data: Option<PrivateKeyData>,
+ symmetric_password: Option<&'static str>,
+ expected_result: Result<&[u8], ()>,
+) {
+ let mut processor_context = MockProcessContext::new();
+ if let Some(private_key) = private_key_data {
+ processor_context.controller_services.insert(
+ "my_private_key_service".to_string(),
+ Box::new(private_key.into_controller()),
+ );
+ processor_context.properties.insert(
+ super::properties::PRIVATE_KEY_SERVICE.name.to_string(),
+ "my_private_key_service".to_string(),
+ );
+ }
+ if let Some(symmetric_password) = symmetric_password {
+ processor_context.properties.insert(
+ super::properties::SYMMETRIC_PASSWORD.name.to_string(),
+ symmetric_password.to_string(),
+ );
+ }
+
+ let decrypt_content = DecryptContentPGP::schedule(&processor_context,
&MockLogger::new())
+ .expect("Should schedule without any properties");
Review Comment:
This expectation message is misleading: the test sets controller services
and/or properties before scheduling, so it is not "without any properties".
Keeping the message accurate helps when diagnosing failures.
##########
minifi_rust/extensions/minifi_pgp/src/processors/decrypt_content/tests.rs:
##########
@@ -0,0 +1,253 @@
+use crate::controller_services::private_key_service::PGPPrivateKeyService;
+use crate::processors::decrypt_content::{DecryptContentPGP, output_attributes};
+use crate::test_utils;
+use crate::test_utils::get_test_message;
+use minifi_native::{
+ ComponentIdentifier, EnableControllerService, FlowFileStreamTransform,
IoState,
+ MockControllerServiceContext, MockLogger, MockProcessContext, Schedule,
+};
+
+#[test]
+fn test_ids() {
+ assert_eq!(
+ DecryptContentPGP::CLASS_NAME,
+ "minifi_pgp::processors::decrypt_content::DecryptContentPGP"
+ );
+ assert_eq!(DecryptContentPGP::GROUP_NAME, "minifi_pgp");
+ assert_eq!(DecryptContentPGP::VERSION, "0.1.0");
+}
+
+#[test]
+fn fails_to_schedule_by_default() {
+ let decrypt_content =
+ DecryptContentPGP::schedule(&MockProcessContext::new(),
&MockLogger::new());
+ assert!(decrypt_content.is_err());
+}
+
+#[test]
+fn schedules_with_password() {
+ let mut context = MockProcessContext::new();
+ context.properties.insert(
+ super::properties::SYMMETRIC_PASSWORD.name.to_string(),
+ "my_secret_password".to_string(),
+ );
+ let decrypt_content = DecryptContentPGP::schedule(&context,
&MockLogger::new());
+ assert!(decrypt_content.is_ok());
+}
+
+#[test]
+fn schedules_with_controller() {
+ let mut context = MockProcessContext::new();
+ context.properties.insert(
+ super::properties::PRIVATE_KEY_SERVICE.name.to_string(),
+ "my_private_key_service".to_string(),
+ );
+ let decrypt_content = DecryptContentPGP::schedule(&context,
&MockLogger::new());
+ assert!(decrypt_content.is_ok());
+}
+
+#[test]
+fn schedule_rejects_invalid_strategy_without_panicking() {
+ let mut context = MockProcessContext::new();
+ context.properties.insert(
+ super::properties::DECRYPTION_STRATEGY.name.to_string(),
+ "NOT_A_STRATEGY".to_string(),
+ );
+ context.properties.insert(
+ super::properties::SYMMETRIC_PASSWORD.name.to_string(),
+ "my_secret_password".to_string(),
+ );
+ // Must return Err, not panic.
+ let result = DecryptContentPGP::schedule(&context, &MockLogger::new());
+ assert!(result.is_err(), "expected schedule to fail on bad strategy");
+}
+
+#[derive(Copy, Clone)]
+struct PrivateKeyData {
+ key_filename: &'static str,
+ passphrase: Option<&'static str>,
+}
+
+impl PrivateKeyData {
+ fn into_controller(self) -> PGPPrivateKeyService {
+ let mut context = MockControllerServiceContext::new();
+ context
+ .properties
+ .insert("Key File",
test_utils::get_test_key_path(self.key_filename));
+
+ if let Some(passphrase) = self.passphrase {
+ context.properties.insert("Key Passphrase", passphrase);
+ }
+
+ PGPPrivateKeyService::enable(&context,
&MockLogger::new()).expect("should enable")
+ }
+}
+
+fn test_decryption(
+ message_file_name: &str,
+ private_key_data: Option<PrivateKeyData>,
+ symmetric_password: Option<&'static str>,
+ expected_result: Result<&[u8], ()>,
+) {
+ let mut processor_context = MockProcessContext::new();
+ if let Some(private_key) = private_key_data {
+ processor_context.controller_services.insert(
+ "my_private_key_service".to_string(),
+ Box::new(private_key.into_controller()),
+ );
+ processor_context.properties.insert(
+ super::properties::PRIVATE_KEY_SERVICE.name.to_string(),
+ "my_private_key_service".to_string(),
+ );
+ }
+ if let Some(symmetric_password) = symmetric_password {
+ processor_context.properties.insert(
+ super::properties::SYMMETRIC_PASSWORD.name.to_string(),
+ symmetric_password.to_string(),
+ );
+ }
+
+ let decrypt_content = DecryptContentPGP::schedule(&processor_context,
&MockLogger::new())
+ .expect("Should schedule without any properties");
+ let mut output: Vec<u8> = Vec::new();
+ let mut flow_file_stream =
std::io::Cursor::new(get_test_message(message_file_name));
+ let res = decrypt_content
+ .transform(
+ &processor_context,
+ &mut flow_file_stream,
+ &mut output,
+ &MockLogger::new(),
+ )
+ .expect("Should be able to transform");
+
+ match expected_result {
+ Ok(_result_bytes) => {
+ assert_eq!(
+ res.target_relationship_name(),
+ super::relationships::SUCCESS.name
+ );
+ assert_eq!(res.write_status(), IoState::Ok);
+ let data_modified = res
+ .get_attribute(output_attributes::LITERAL_DATA_MODIFIED.name)
+ .unwrap()
+ .parse::<u64>()
+ .expect("Should be u64");
+ assert!(data_modified > 1770000000000);
+ assert!(data_modified < 1780000000000);
Review Comment:
These hard-coded millisecond bounds are unexplained magic numbers. Keeping
the same check but expressing it as a single range improves readability and
reduces duplication.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
