MOSIP-30573: test case coverage for zkservice

[nagendra0721]

Summary by CodeRabbit

• Tests
  • Added controller endpoint tests for encryption, decryption, and re-encryption flows.
  • Introduced comprehensive service unit tests covering normal, edge and error paths for key management and cryptographic operations.
  • Expanded signature-related tests to cover multiple signing/verifying flows, key types, trust validation, and diverse error scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds three new/expanded test suites: controller tests for ZKCryptoManager endpoints, comprehensive unit tests for ZKCryptoManager service implementation, and expanded signature service tests with additional trust/certificate and error-path scenarios.

Changes

Cohort / File(s)	Summary
ZKCryptoManager — Controller tests kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerControlerTest.java	New MockMvc-based tests for /zkEncrypt, /zkDecrypt, and /zkReEncryptRandomKey endpoints covering success and validation-failure scenarios; mocks security context and service dependencies; includes nested TestConfig.
ZKCryptoManager — Service tests kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java	New extensive unit test class for ZKCryptoManagerServiceImpl with many scenarios: encryption/decryption, re-encryption with random keys, multiple reference IDs, error/edge cases (null/empty inputs, thumbprint issues), DTO validations, mock setups for keystores/certificates, and helper methods for test data.
Signature service tests (expanded) kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/SignatureServiceTest.java	Tests updated/extended to use KeymanagerUtil for PEM-to-certificate conversion, add EC/ED25519 sign/verify scenarios, additional boundary and error-path tests (invalid inputs, formats, trust validation), and new imports; no public API changes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Areas needing extra attention:
  • ZKCryptoManagerServiceTest: many varied cases and heavy Mockito setup — verify mock behaviors, stubbing consistency, and exception assertions.
  • ZKCryptoManagerControlerTest: ensure MockMvc security context and request/response JSON path assertions match controller contract.
  • SignatureServiceTest: verify certificate construction via KeymanagerUtil and expanded error-path coverage for correctness.

Possibly related PRs

• MOSIP-30573: test case coverage #478 — Modifies signature test suite files overlapping with SignatureServiceTest updates; likely related to the same test-area changes.

Suggested reviewers

• mahammedtaheer

Poem

🐰
I hopped through code and found the keys,
I braided tests beneath the trees,
Encrypt, re-encrypt, then play,
Edge cases caught along the way,
Hooray — the rabbit guards the keys!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately reflects the main change: adding test case coverage for the ZK (Zero-Knowledge) cryptography service, which is the primary focus across all modified files.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerControlerTest.java (2)

35-38: Class name typo and possibly redundant test configuration

The test class name ZKCryptoManagerControlerTest (single l) is inconsistent with ZKCryptoManagerController and may reduce discoverability; consider renaming to ZKCryptoManagerControllerTest.

Also, @WebMvcTest(ZKCryptoManagerController.class) already slices the context around this controller. The additional @ContextConfiguration with a @SpringBootApplication + @ComponentScan TestConfig may be redundant and can pull in more beans than needed, slowing tests and blurring the slice boundary. Unless you need that broader scan, you can likely drop TestConfig and @ContextConfiguration and rely on @WebMvcTest plus the existing @MockBeans.

Also applies to: 156-159

---

52-154: Consider adding authorization negative-path tests

The controller tests cover successful calls and a validation failure for a user with role ZONAL_ADMIN, but there are no tests asserting behavior for unauthorized roles (e.g., missing role or different role leading to 403/401). Adding at least one negative-path test (e.g., @WithMockUser(roles = "OTHER_ROLE") expecting forbidden) would strengthen coverage of the security configuration around these endpoints.

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java (3)

721-745: Tests encoding NullPointerException behavior may lock in undesirable contracts

testZkReEncryptRandomKeyWithNullKeyAliasesInMap and testGetKeyAliasWithNullInMap intentionally drive the implementation into NullPointerException (or accept either NPE or NoUniqueAliasException) when key-alias maps contain null values. That effectively codifies NPEs as acceptable behavior of the service.

If the long-term intention is to harden this code, it would be preferable for the implementation to detect these invalid states and throw domain-specific exceptions (e.g., NoUniqueAliasException / ZKCryptoException) rather than NPEs, and for the tests to assert those instead. Right now these tests will fail if you later improve null handling, which may discourage such fixes.

Also applies to: 1151-1178

---

680-717: Strengthen assertion for multi-reference-id encrypted random key

In testEncryptRandomKeyWithMultipleReferenceIds, the assertion

assertTrue(response.getEncryptedRandomKey().contains(".") || response.getEncryptedRandomKey().length() > 0);

will pass for any non-empty string, even if the expected dot separator for multiple keys is missing. If the intended behavior is that multiple reference IDs yield a dot-separated value, you can tighten this to:

assertTrue(response.getEncryptedRandomKey().contains("."));

(or even assert exact structure) so the test actually protects the multi-key formatting contract.

---

110-137: Redundant manual field restoration and tight coupling to private implementation details

Several tests modify private configuration fields via ReflectionTestUtils.setField (e.g., aesGCMTransformation, aesECBTransformation, pubKeyReferenceId, keyAliases) and then manually restore them at the end of the test. Given JUnit 4’s per-test-instance model and the @Before setUp() method that resets these fields before each test, the explicit “restore” steps are redundant and add noise.

Additionally, the heavy reliance on private field names (and magic strings like "REF1,REF2") makes these tests tightly coupled to implementation details, which will make refactoring the service harder.

Consider:

• Dropping the explicit “restore” calls and relying on setUp() for a clean baseline.
• Where feasible, pushing configuration into constructor/setters so that tests can set it via public API rather than reflection. This will reduce brittleness while keeping the same coverage.

Also applies to: 381-403, 507-549, 848-880, 882-922, 1338-1385, 1423-1456

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9da9554 and cb70b3e.

📒 Files selected for processing (2)

• kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerControlerTest.java (1 hunks)
• kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java (4)

kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerConstant.java (1)

• CryptomanagerConstant (10-67)

kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/exception/NoUniqueAliasException.java (1)

• NoUniqueAliasException (12-29)

kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/exception/ZKCryptoException.java (1)

• ZKCryptoException (12-38)

kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/exception/ZKKeyDerivationException.java (1)

• ZKKeyDerivationException (12-38)

⏰ 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

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/SignatureServiceTest.java (1)

7-38: JUnit Jupiter static imports will fail to compile—missing junit-jupiter-api dependency

The code imports org.junit.jupiter.api.Assertions.assertEquals and org.junit.jupiter.api.Assertions.assertThrows, but the pom.xml declares only junit:junit:4.12 (which does not include assertThrows in org.junit.Assert) and does not include junit-jupiter-api as a dependency. Even though junit-vintage-engine is present, it does not provide the Jupiter assertions API.

JUnit 4.13+ provides Assert.assertThrows, and JUnit 5 Jupiter provides org.junit.jupiter.api.Assertions.assertThrows.

Action: Either:

1. Add junit-jupiter-api to test dependencies in pom.xml and statically import org.junit.jupiter.api.Assertions.*.
2. Upgrade to junit:junit:4.13.2 and use org.junit.Assert.assertThrows instead.
3. Rewrite exception assertions using JUnit 4 patterns (try/catch or @Test(expected = ...)).

Without this fix, test compilation will fail.

♻️ Duplicate comments (5)

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/SignatureServiceTest.java (5)

791-805: Exception-path test is good; relies on assertThrows comment above

testJsonParsingError is a solid addition: it checks that malformed JWS/JWT input leads to RequestException with INVALID_VERIFY_INPUT for both v1 and v2 verify.

Note that it depends on org.junit.jupiter.api.Assertions.assertThrows and assertEquals; this shares the same JUnit version concern mentioned for the imports.

---

807-817: Empty data JWS/JWT sign test is correct; JUnit concern applies

testJWSsignEmptyDataException correctly enforces that an empty dataToSign triggers INVALID_INPUT.

Again, this uses assertThrows/assertEquals from JUnit Jupiter; please align with the chosen JUnit version as noted earlier.

---

819-835: SignV2 invalid app and empty data behavior well captured

testSignV2EmptyDataException distinguishes:

• INVALID_APP_ID → SIGN_NOT_ALLOWED.
• Valid app "TEST" but missing data → INVALID_INPUT.

The logic looks consistent. This test also relies on Jupiter assertions, so it’s subject to the same JUnit version alignment comment.

---

875-896: JWT sign v2 negative-paths are appropriate

testJWTSignV2Exception checks:

• Invalid appId → SIGN_NOT_ALLOWED.
• Valid appId but missing data → INVALID_INPUT.
• Non‑JSON payload → INVALID_JSON.

The flow is clear and enforces the expected error codes. Same JUnit Jupiter assertion caveat applies.

---

898-921: JWS sign v2 negative-paths mirror JWT v2 and look consistent

testJWSsignV2Exception mirrors JWT v2 for JWS:

• Invalid appId → SIGN_NOT_ALLOWED.
• Valid appId but missing data → INVALID_INPUT.
• Non‑JSON data with validateJson=true → INVALID_JSON.

This symmetry is good for maintenance. It also shares the same JUnit 5 assertion dependency.

🧹 Nitpick comments (4)

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/SignatureServiceTest.java (4)

57-58: Field injection for KeymanagerUtil is consistent but could be made private

Autowiring KeymanagerUtil matches the rest of the class usage and enables the new trust-validation tests. For consistency with the other fields, consider making it private as well, but that’s purely cosmetic.

---

322-333: Good extra JWS coverage; consider separating scenarios for readability

The added calls in testJwsSign (empty applicationId and ED25519_SIGN) broaden coverage of default app handling and ED25519 keys.

To reduce coupling between scenarios, you might optionally:

• Create a fresh JWSSignatureRequestDto per distinct scenario, or
• Explicitly reset all relevant fields before each sign call.

Not required, but it would make the test intent clearer.

---

654-663: Trust v2 tests are clear; minor collection simplification possible

The new testValidateTrustV2 scenarios verify:

• validateTrust=true without domain → TRUST_NOT_VERIFIED_NO_DOMAIN.
• With domain set and a single certificate → TRUST_CERT_PATH_NOT_VALID.

That’s good coverage. You could simplify the list creation as:

List<Certificate> certificateList =
        Collections.singletonList(keymanagerUtil.convertToCertificate(pemCertificate));

instead of wrapping Collections.singleton(...) in an ArrayList<>, unless mutability is required.

---

752-764: JwsSignV2 ED25519 and default app scenario

The extra calls in testJwsSignV2 give coverage for:

• Empty applicationId with existing KERNEL/SIGN master key.
• ED25519 signing via ED25519_SIGN reference.

Behaviorally this looks good. As with earlier tests, consider separate DTO instances per scenario if you later find these tests hard to maintain.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb70b3e and 6823224.

📒 Files selected for processing (1)

• kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/SignatureServiceTest.java (10 hunks)

⏰ 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 (7)

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/signature/test/service/SignatureServiceTest.java (7)

217-225: Nice additional ED25519 verification path

Extending testJwtVerify to cover the ED25519_SIGN reference increases confidence that the verify flow works across key types. The reuse of existing request objects keeps the test compact and looks fine.

---

395-404: Sign v2 default-application behavior well covered

The new block in testSignv2 that generates a KERNEL/SIGN key and then signs with applicationId set to "" nicely validates the default appId behavior on v2 signing. Assertions on non-null signature and timestamp are appropriate here.

---

483-500: JwtSignV2: default app/ref and header variations well tested

The extra flows in testJwtSignV2 (null applicationId / referenceId, then updated additional headers including kid and aud) give good coverage of:

• Default key selection when identifiers are omitted.
• Robustness to different header combinations.

The assertions on jwtSignedData presence are sufficient for this level.

---

694-708: JwtVerifyV2: default app/ref behavior thoroughly exercised

The added part of testJwtVerifyV2 that:

• Generates a KERNEL/SIGN key,
• Signs with empty app/ref and no certificate chain,
• Verifies with empty applicationId,

nicely validates fallback behavior for v2 verify. The assertions on validity and message are appropriate.

---

766-788: New default appId/refId verify test is useful

testJwtVerifyDefaultAppIDAndRefID cleanly asserts that:

• Signing without explicit app/ref,
• Then verifying without app/ref,

still succeeds (assuming the default KERNEL/SIGN key exists). This is a valuable regression test for defaulting logic.

---

837-872: Raw sign negative-path coverage is strong; also subject to JUnit setup

rawSignException nicely chains multiple scenarios:

• Invalid appId → SIGN_NOT_ALLOWED.
• Valid appId with missing data → INVALID_INPUT.
• Default app (null) with invalid response format → KeymanagerServiceException + INVALID_FORMAT_ERROR.
• Then a successful base64url response path.

The structure and asserted error codes look good. Just ensure the use of assertThrows is supported by your JUnit configuration as already discussed.

---

923-933: JWT verify v2 empty-signature guard is correct

testJWTVerifyV2EmptySignData validates that an empty jwtSignatureData leads to INVALID_INPUT. The behavior is correct; just ensure assertThrows is available under your chosen JUnit version.