pluckyswan commented 1 week ago

Description

Generate a SHA3-512 hash for orders and results instead of using hashCode. Added unit tests for the new method generateHash.

Issue

https://github.com/CDCgov/trusted-intermediary/issues/824

Checklist

[x] I have added tests to cover my changes
[x] I have added logging where useful (with appropriate log level)

Note: You may remove items that are not applicable

github-actions[bot] commented 1 week ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

**🎫 Ticket compliance analysis ✅** **[824](https://github.com/CDCgov/trusted-intermediary/issues/824) - Fully compliant** Fully compliant requirements: - Implement a secure, non-reversible hash for storing message metadata.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Error Handling
The generateHash method returns an empty string when an exception occurs, which might not be an ideal error handling strategy.

github-actions[bot] commented 1 week ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Improve error handling in the hash generation to prevent silent failures ___ Add a fallback mechanism or throw an exception when the `generateHash` method fails to compute the hash due to a `NoSuchAlgorithmException`, instead of returning an empty string. [etor/src/main/java/gov/hhs/cdc/trustedintermediary/etor/orders/SendOrderUseCase.java [68-70]](https://github.com/CDCgov/trusted-intermediary/pull/1594/files#diff-574c5640083d573a99fb3caced5d4ba24531dea4d65d5eb0486858657b63859dR68-R70) ```diff catch (NoSuchAlgorithmException e) { logger.logError("Algorithm does not exist!", e); - return ""; + throw new RuntimeException("Failed to compute hash", e); } ``` Suggestion importance[1-10]: 8 Why: The suggestion to throw an exception instead of returning an empty string when encountering a NoSuchAlgorithmException is crucial for robust error handling and avoiding silent failures in the system.	8
	Modify the test to ensure different object states are used to validate hash uniqueness ___ Ensure that the test 'generateHash generates unique hash for the same object' uses different object states for `mockOrder` and `mockOrder2` to validate the uniqueness of the hash properly. [etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/orders/SendOrderUseCaseTest.groovy [145-146]](https://github.com/CDCgov/trusted-intermediary/pull/1594/files#diff-56696e8718f522c7926a514339b630853370a926d4ca74c4911c87ccd7585493R145-R146) ```diff def mockOrder = Mock(Order) +mockOrder.someProperty = 'value1' def mockOrder2 = Mock(Order) +mockOrder2.someProperty = 'value2' ``` Suggestion importance[1-10]: 7 Why: This suggestion is valid as it ensures that the test case for generating unique hashes uses different states for each object, which is crucial for accurately testing the hash function's ability to generate unique hashes for different objects.	7
	Ensure the `Order` object's string representation is suitable for hash generation ___ Ensure that the `Order` class has a properly overridden `toString` method that uniquely represents its state, as it is used for generating the hash in `generateHash`. [etor/src/main/java/gov/hhs/cdc/trustedintermediary/etor/orders/SendOrderUseCase.java [64]](https://github.com/CDCgov/trusted-intermediary/pull/1594/files#diff-574c5640083d573a99fb3caced5d4ba24531dea4d65d5eb0486858657b63859dR64-R64) ```diff -byte[] objBytes = order.toString().getBytes(StandardCharsets.UTF_8); +byte[] objBytes = order.customToStringMethod().getBytes(StandardCharsets.UTF_8); ``` Suggestion importance[1-10]: 6 Why: This suggestion is relevant as it ensures the `Order` class's `toString` method provides a unique representation for hash generation. However, it suggests using a non-existent method, which reduces its score.	6
General	Implement the test case to verify error handling and logging when hashing fails ___ Complete the implementation of the commented-out test case 'generateHash logs error when object cannot be hashed' to ensure proper error handling and logging. [etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/orders/SendOrderUseCaseTest.groovy [157-167]](https://github.com/CDCgov/trusted-intermediary/pull/1594/files#diff-56696e8718f522c7926a514339b630853370a926d4ca74c4911c87ccd7585493R157-R167) ```diff -// Need to check failure edge case -// def "generateHash logs error when object cannot be hashed"() { +def "generateHash logs error when object cannot be hashed"() { + given: + def mockOrder = Mock(Order) + when: + String mockHash = SendOrderUseCase.getInstance().generateHash(mockOrder) + then: + mockHash == "" + 1 * mockLogger.logError("Algorithm does not exist!", _) +} ``` Suggestion importance[1-10]: 7 Why: Completing the implementation of the commented-out test case is important for ensuring that error handling and logging are correctly implemented when the hash generation fails. This suggestion effectively addresses a commented-out section, pushing for its completion.	7

pluckyswan commented 1 week ago

@halprin Two questions: 1. Is MessageDigest safe for multi-threading (i, ii)? 2. There are instances of hashCode elsewhere in the repo that are overridden. Do these need to be swapped out too?

pluckyswan commented 1 week ago

/review

github-actions[bot] commented 1 week ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

**🎫 Ticket compliance analysis ✅** **[824](https://github.com/CDCgov/trusted-intermediary/issues/824) - Fully compliant** Fully compliant requirements: - Implement a secure, non-reversible hash for storing metadata.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ No major issues detected

halprin commented 1 week ago

@halprin Two questions: 1. Is MessageDigest safe for multi-threading (i, ii)? 2. There are instances of hashCode elsewhere in the repo that are overridden. Do these need to be swapped out too?

An instance of MessageDigest is not thread safe. This means the object you get back from calling MessageDigest.getInstance("whatever") should not be used in multiple threads. And you aren't using it in multiple threads. Each call to HashHelper#generateHash could be on a separate thread but it creates its own instance of MessageDigest, so there is no shared instance of MessageDigest and we're all good.

Can you give me examples of what you're talking about? Is it something like HttpEndpoint.java where we override public int hashCode()?

pluckyswan commented 1 week ago

@halprin Two questions: 1. Is MessageDigest safe for multi-threading (i, ii)? 2. There are instances of hashCode elsewhere in the repo that are overridden. Do these need to be swapped out too?

An instance of MessageDigest is not thread safe. This means the object you get back from calling MessageDigest.getInstance("whatever") should not be used in multiple threads. And you aren't using it in multiple threads. Each call to HashHelper#generateHash could be on a separate thread but it creates its own instance of MessageDigest, so there is no shared instance of MessageDigest and we're all good.

Can you give me examples of what you're talking about? Is it something like HttpEndpoint.java where we override public int hashCode()?

MessageLink and HttpEndpoint both have hashCode() that is overrode. Just wanted to double check it those don't need altering too.

halprin commented 1 week ago

@halprin Two questions: 1. Is MessageDigest safe for multi-threading (i, ii)? 2. There are instances of hashCode elsewhere in the repo that are overridden. Do these need to be swapped out too?

An instance of MessageDigest is not thread safe. This means the object you get back from calling MessageDigest.getInstance("whatever") should not be used in multiple threads. And you aren't using it in multiple threads. Each call to HashHelper#generateHash could be on a separate thread but it creates its own instance of MessageDigest, so there is no shared instance of MessageDigest and we're all good. Can you give me examples of what you're talking about? Is it something like HttpEndpoint.java where we override public int hashCode()?

MessageLink and HttpEndpoint both have hashCode() that is overrode. Just wanted to double check it those don't need altering too.

Those do not need to be changed. The way hashCode works today in the context of how it is normally used (comparing whether two objects are equal, IIRC) is totally fine. But subsequently using hashCode for storing in a database where the original data is sensitive is perhaps not the best and why we are moving away from using hashCode in this context.