MOSIP-30573: test case coverage #478
Conversation
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
|
Warning Rate limit exceeded@nagendra0721 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughReplaces and adds unit tests across the keymanager service: new Argon2 hash tests with static Argon2Factory mocks and reflection-set parameters; a comprehensive HSMHealthCheckTest; controller tests updated to create CSRs preconditions; new KeymanagerUtil private-key extraction tests; and expanded signature tests including CWT, PDF, provider error paths, and V2 variants. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Argon2Service
participant Argon2Factory
participant Argon2Advanced
participant Util
participant Cache
rect rgb(230,245,255)
Note over Test,Argon2Service: Argon2 hash generation tests (mocked)
end
Test->>Argon2Service: setUp() (reflect params)
Test->>Argon2Factory: mock static getInstance()
Test->>Argon2Service: generateArgon2Hash(password, [salt])
Argon2Service->>Util: validateInputData(password)
Util-->>Argon2Service: validation result
Argon2Service->>Argon2Factory: getInstance()
Argon2Factory-->>Argon2Service: Argon2Advanced(mock)
Argon2Service->>Argon2Advanced: hash(password, salt)
Argon2Advanced-->>Argon2Service: encodedHash
Argon2Service->>Cache: update counter
Cache-->>Argon2Service: ack
Argon2Service-->>Test: return(hash, salt)
sequenceDiagram
participant Test
participant HSMHealthCheck
participant DBHelper
participant KeyStore
participant Health
rect rgb(245,235,230)
Note over Test,HSMHealthCheck: HSM health-check scenarios
end
Test->>HSMHealthCheck: health()
alt cached alias present
HSMHealthCheck->>KeyStore: readKey(cachedAlias)
KeyStore-->>HSMHealthCheck: key
HSMHealthCheck-->>Health: UP / READ_KEY_SUCCESS
else no cached alias
HSMHealthCheck->>DBHelper: getUniqueKeyAlias()
DBHelper-->>HSMHealthCheck: aliasList
alt single alias
HSMHealthCheck->>KeyStore: (encryptEnabled?) encrypt(key) / readKey(alias)
KeyStore-->>HSMHealthCheck: result or exception
HSMHealthCheck-->>Health: UP / ENCRYPT_OPS_SUCCESS or UP / READ_KEY_SUCCESS
else zero/multiple aliases or error
HSMHealthCheck-->>Health: DOWN / error detail
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/controller/KeymanagerControllerTest.java (1)
280-290: Approve changes, but note unused assignment.The addition of master key generation (line 284) properly establishes the precondition for retrieving the certificate chain.
However, line 285 (
keyPairGenRequest.setRequest(keyPairGenRequestDto)) appears to be unnecessary sincekeyPairGenRequestis not used in the subsequent MockMvc request (lines 286-289), which uses hardcoded parameters instead.Consider removing line 285 if it's not needed:
keymanagerService.generateMasterKey("CSR", keyPairGenRequestDto); - keyPairGenRequest.setRequest(keyPairGenRequestDto); mockMvc.perform(get("/getCertificateChain")kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/CoseSignatureServiceTest.java (1)
544-584: Inconsistent test method naming.The method name
testCwtVerifyChecksException(line 545) uses lowercase "Cwt", while the existing methodtestCWTVerifyChecksException(line 466) uses uppercase "CWT". This inconsistency can cause confusion.Apply this diff to align the naming:
- public void testCwtVerifyChecksException() { + public void testCwtVerifyClaimValidationException() {Note: I've also suggested a more descriptive name since this test specifically focuses on claim validation errors rather than generic CWT verification exceptions.
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/health/HSMHealthCheckTest.java (4)
92-112: Consider simplifying the mock setup and extracting the magic number.The test logic is correct, but there are minor improvements possible:
- Line 106: The explicit type witness
<Key>and cast to(SecretKey)are redundant ifgetSymmetricKeyalready returnsSecretKey.- Line 105: The magic number
16(AES key size in bytes) could be extracted as a constant for clarity.Consider this refactor:
+ private static final int AES_KEY_SIZE_BYTES = 16; + @Test public void testHealthUpWhenReadKeySuccess() throws Exception { ReflectionTestUtils.setField(hsmHealthCheck, "healthCheckEnabled", true); ReflectionTestUtils.setField(hsmHealthCheck, "healthCheckEncryptEnabled", false); Map<String, List<KeyAlias>> keyAliasMap = new HashMap<>(); List<KeyAlias> currentKeyAliases = new ArrayList<>(); KeyAlias keyAlias = new KeyAlias(); keyAlias.setAlias("test-alias"); currentKeyAliases.add(keyAlias); keyAliasMap.put(KeymanagerConstant.CURRENTKEYALIAS, currentKeyAliases); when(dbHelper.getKeyAliases(any(), any(), any(LocalDateTime.class))).thenReturn(keyAliasMap); - Key key = new SecretKeySpec(new byte[16], "AES"); - when(keyStore.<Key>getSymmetricKey("test-alias")).thenReturn((SecretKey) key); + SecretKey key = new SecretKeySpec(new byte[AES_KEY_SIZE_BYTES], "AES"); + when(keyStore.getSymmetricKey("test-alias")).thenReturn(key); Mono<Health> healthMono = hsmHealthCheck.health(); Health health = healthMono.block(); assertEquals(Status.UP, health.getStatus()); assertEquals("READ_KEY_SUCCESS", health.getDetails().get("Info: ")); }
114-134: Same optional refactor applies here.The same simplifications suggested in the previous test (extracting the magic number and simplifying the mock setup) would improve this test as well.
157-177: Consider adding error detail assertion.The test correctly causes an encryption failure by using a DES key with AES transformation. However, it only asserts the DOWN status without verifying the error details. Adding an assertion on the error message would make the test more robust and ensure it's failing for the expected reason.
Add an assertion for error details:
Mono<Health> healthMono = hsmHealthCheck.health(); Health health = healthMono.block(); assertEquals(Status.DOWN, health.getStatus()); + // Verify that an error message is present in details + assertTrue(health.getDetails().containsKey("Error: ")); + assertNotNull(health.getDetails().get("Error: "));
179-192: LGTM with minor improvement opportunity.The test correctly validates the cached alias path, which bypasses database lookup. The same magic number extraction suggested earlier would apply here as well (line 185).
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/Argon2HashServiceTest.java (4)
82-93: Reflection-based Argon2 config is brittle to field changesUsing reflection to set
argon2Iterations,argon2Memory, andargon2Parallelismworks but tightly couples the test to private field names and their existence. If these fields are renamed or refactored (e.g., moved behind a config object), the tests will fail at runtime rather than compile time.Consider exposing package-private setters or constructor/config injection for these parameters so tests can configure them without reflection.
162-191: Generated-salt path test provides good coverage; could assert salt characteristicsThis test nicely covers the branch where no salt is provided, existing AES key/counter are present in cache, and the internal counter is updated, while asserting on the encoded hash and
validateInputData/cache interactions.If you want to tighten it further, you could also assert some properties of
response.getSalt()(e.g., non-empty and URL-safe/base64-encoded) rather than only non-null, to catch accidental regressions in salt encoding.
195-218: Provided-salt path test is clear; optionally verify cache is not usedThe test cleanly verifies that when a valid salt is supplied, it is echoed back and the hash is based on that input, while still checking
validateInputDataon the password.As an optional enhancement, you could assert that
saltGenParamsCacheis not interacted with in this scenario (e.g.,verifyNoInteractions(saltGenParamsCache)or similar) to make the “no cache dependency when salt is provided” behavior explicit.
222-245: Fallback test for missing AES key improves robustness; could assert cache initializationThis test usefully covers the case where
CACHE_AES_KEYis absent, ensuring a hash and salt are still produced and input validation is invoked.To make the expected behavior crisper, you might also verify that new AES key/counter values are written to
saltGenParamsCachein this path (e.g.,verify(saltGenParamsCache).put(...)), mirroring the counter update assertion you have in the generated-salt test.kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/util/KeymanagerUtilTest.java (1)
4-6: Standardize on one JUnit assertion approach for consistency.This code mixes JUnit 4 (
@RunWith(SpringRunner.class),org.junit.Test) with JUnit Jupiter'sassertThrowsfromorg.junit.jupiter.api.Assertions. While this works if Jupiter is on the classpath, it can surprise maintainers. Pick one consistent pattern:
- If staying on JUnit 4: use
Assert.assertThrows(available in JUnit 4.13+) instead of importing from Jupiter; this is clearer and easier to migrate later.- If migrating to JUnit 5: use the Jupiter variant, but migrate the runner and test annotations together.
- Document the choice and apply it consistently across this module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/Argon2HashServiceTest.java(4 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/health/HSMHealthCheckTest.java(1 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/controller/KeymanagerControllerTest.java(2 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/util/KeymanagerUtilTest.java(4 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/controller/SignatureControllerTest.java(2 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/CoseSignatureServiceTest.java(2 hunks)kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/SignatureServiceTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/SignatureServiceTest.java (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/exception/SignatureFailureException.java (1)
SignatureFailureException(12-30)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/Argon2HashServiceTest.java (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerConstant.java (1)
CryptomanagerConstant(10-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-kernel / maven-build
🔇 Additional comments (15)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/controller/KeymanagerControllerTest.java (1)
189-209: LGTM! Improved test precondition setup.The test now properly establishes preconditions by generating a CSR with referenceId "test001" before attempting to revoke it. This makes the test more realistic and specific compared to the previous approach.
The flow is correct:
- Generate master key for REGISTRATION (empty referenceId) - establishes the root key
- Generate CSR for "test001" - creates a specific key to be revoked
- Revoke the "test001" key - tests the actual revocation functionality
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/CoseSignatureServiceTest.java (1)
544-584: Good error path coverage for CWT claim validation.The test effectively validates the error handling for:
- Missing claims in verify request (CLAIM_NOT_MATCHED)
- Claims not present in signed token (CLAIM_NOT_FOUND)
- Mismatched claim values (CLAIM_NOT_MATCHED)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/SignatureServiceTest.java (3)
602-630: Good V2 API test coverage for JWT verification.The test properly exercises the V2 JWT signing and verification flow with certificate chain inclusion and hash validation.
632-672: Good V2 API test coverage for JWS signing.The test validates V2 JWS signing with additional headers including standard JWT claims (iss, aud, sub) and custom headers, covering both payload inclusion/exclusion and certificate chain handling.
556-582: I need to examine the SignatureProvider interface and verify the null-handling contract across all implementations.Based on my analysis of the provided script output, I can now generate the rewritten review comment. The verification shows that:
- All four signature provider implementations (PS256, RS256, EC256, Ed25519) follow the same pattern: they catch broad exception types and wrap them in
SignatureFailureException- None of the implementations have explicit null-parameter validation - they rely on NullPointerException from the underlying cryptographic operations
- The tests verify that null inputs result in
SignatureFailureException, which is the documented error handling behavior- All implementations consistently throw
SignatureFailureExceptionwhen errors occurThe tests are actually verifying the public interface contract, not fragile implementation details. The concern about fragility is partially valid (since implementations don't explicitly validate null parameters), but the tests are robust in that they verify the actual behavior across all implementations.
Verify signature provider implementations handle null parameters consistently.
These tests validate that all signature providers (PS256, RS256, EC256, EdDSA) throw
SignatureFailureExceptionwhen invoked with null parameters. Examination of the implementations reveals all four follow the same pattern: catch cryptographic exceptions and wrap them inSignatureFailureException. While the implementations rely on implicit null-handling rather than explicit parameter validation, this behavior is consistent across all providers and the tests verify the documented contract. Consider adding explicit null-parameter validation in the implementations (e.g.,if (privateKey == null) throw new SignatureFailureException(...)) to make the tests less dependent on exception-catching semantics.kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/controller/SignatureControllerTest.java (2)
186-187: Enhanced test coverage with CWT claims.Adding subject and issuer fields to both the sign and verify requests improves test realism and validates the full claim verification flow.
Also applies to: 195-196
277-290: Improved PDF signing test with realistic data.The updates enhance the test quality by:
- Using a proper base64-encoded PDF structure instead of dummy text
- Setting realistic signature coordinates (100, 100, 200, 150)
- Adding reason and password fields for completeness
- Using
isOk()for more specific status assertionAlso applies to: 296-296
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/health/HSMHealthCheckTest.java (5)
1-53: LGTM!The test class setup is well-structured with appropriate mocks and initialization. The use of ReflectionTestUtils to configure private fields is standard practice for Spring Boot testing.
55-62: LGTM!Correctly tests the disabled health check scenario with appropriate assertions.
64-90: LGTM!Both test methods correctly validate the scenarios where no unique key alias is found (empty list and multiple aliases).
136-155: LGTM!Correctly tests exception handling from the keystore, including proper validation of the error message in health details.
1-193: No known security vulnerabilities, but versions are severely outdated.Only one major version of Mockito is supported at a time, and changes are not backported to older versions. The test uses Mockito 3.4.3 (2020), while Mockito 5 now requires Java 11 and is the current supported line.
Reactor 3.1.7.RELEASE dates from 2018. Current stable releases are Reactor Core 3.7.11 as part of the 2024.0.10 Release Train.
Finding: No CVEs detected in official advisories for either version. However, the extreme age (Reactor is 7 years old) carries inherent risk of undiscovered vulnerabilities and compatibility issues with modern Java versions. Plan a dependency upgrade to align with current supported versions: Mockito 5.x and Reactor 3.7.x.
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/Argon2HashServiceTest.java (2)
3-37: Imports align with expanded Argon2 and cache test scopeThe added static assertions/matchers, Argon2, cache2k, and CryptoUtil imports are consistent with the new test scenarios and keep test code concise.
51-62: Good use of additional mocks and injected service implementationMocking
Cache<String, Object>andCryptomanagerUtilsalongside an@InjectMocksCryptomanagerServiceImplis a solid setup to exercisegenerateArgon2Hashwithout touching real cache or key material, while still validating interactions.kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/util/KeymanagerUtilTest.java (1)
68-71: Wiring changes forSignatureUtiland setup look fine.The added
@Autowired SignatureUtilfield and minor adjustment to the@Beforeannotation introduce no behavioral risk and are consistent with the existing Spring test setup.
...er-service/src/test/java/io/mosip/kernel/keymanagerservice/test/util/KeymanagerUtilTest.java
Show resolved
Hide resolved
...nager-service/src/test/java/io/mosip/kernel/signature/test/service/SignatureServiceTest.java
Show resolved
Hide resolved
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.