MOSIP-30573: test case for pkcs11 file

[nagendra0721]

Summary by CodeRabbit

• Tests
  • Enhanced testing infrastructure with inline mocking for improved isolation.
  • Added extensive unit tests covering PKCS#11 and offline keystore behaviors (generation, storage, retrieval, deletion, provider handling, caching, and error scenarios).
  • Expanded negative-path and exception verification across keystore APIs.
  • Updated partner certificate test data (organization name change).

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

[coderabbitai]

Walkthrough

Added a test-only dependency and three new comprehensive test classes (PKCS11KeyStoreImplTest, OLKeyStoreImplTest, PartnerCertManagerControllerTest update) exercising keystore providers, key/certificate operations, error paths, and provider stubbing; plus a small test-data change in PartnerCertManagerControllerTest.

Changes

Cohort / File(s)	Summary
Dependency Management kernel/kernel-keymanager-service/pom.xml	Added test dependency org.mockito:mockito-inline with version ${mockito.core.version} and scope test.
PKCS11 HSM Keystore Tests kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/PKCS11KeyStoreImplTest.java	New comprehensive JUnit test class with multiple nested test providers/SPIs and helpers covering asymmetric/EC/symmetric key generation, storage, retrieval, deletion, certificate generation/storage, provider reload/fallback, caching, and many failure scenarios.
Offline Keystore Tests kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/OLKeyStoreImplTest.java	New JUnit test class covering OLKeyStoreImpl constructors, all keystore operations, extensive negative/error-path tests, EC-curve cases, and provider name assertions.
Partner Certificate Test data kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/partnercertservice/test/controller/PartnerCertManagerControllerTest.java	Test data change: organizationName in testUploadPartnerCertificate_Success changed from "IITB" to "Mosip".

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• PKCS11KeyStoreImplTest contains dense test infrastructure (multiple nested provider/SPIs, reflection use, many scenarios) requiring careful review.
• OLKeyStoreImplTest has many negative-path assertions to verify error codes/messages.
• Verify mockito-inline addition doesn't conflict with existing mocking setup.

Possibly related PRs

• MOSIP-30573: add test coverage #444 — Related test edits in kernel-keymanager-service and changes to PartnerCertManagerControllerTest.

Suggested reviewers

• mahammedtaheer

Poem

🐰 In testbeds where the keyrings play,
I hop and stub the night away,
Providers mocked and certs boxed neat,
Keys generated, kept, and beat,
Small change of name — Mosip's the way!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

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.
Title check	❓ Inconclusive	The title refers to only one aspect of the changeset (PKCS11 test case) but omits the significant additions of OLKeyStoreImpl tests, mockito-inline dependency, and a minor data change in PartnerCertManager tests.	Consider a more comprehensive title that encompasses the main changes, such as 'Add comprehensive HSM keystore tests and mockito-inline dependency' or clarify if PKCS11 is the primary focus.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ 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: 1

🧹 Nitpick comments (4)

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/PKCS11KeyStoreImplTest.java (3)

87-107: Consider thread-safety implications of Security provider manipulation.

The test modifies the global Security provider registry (lines 89-90, 104), which could cause issues if tests run in parallel. While the tearDown method properly cleans up, any test failure that prevents tearDown execution could affect subsequent tests.

Consider documenting that these tests should run serially, or explore using JUnit 5's @Execution(ExecutionMode.SAME_THREAD) annotation if upgrading from JUnit 4.

---

550-862: Reduce visibility of nested test helper classes.

All nested helper classes (TestProvider, NoSecureRandomProvider, FaultyProvider, TestKeyStoreSpi, TestRSAKeyPairGeneratorSpi, TestECKeyPairGeneratorSpi, TestAesKeyGeneratorSpi, TestSecureRandomSpi) are declared as public static. Since these classes are only used within this test file, they should use package-private (default) visibility to avoid unnecessary exposure.

Apply this pattern to all nested classes:

-	public static class TestProvider extends Provider {
+	static class TestProvider extends Provider {

---

234-241: Heavy reflection usage creates brittle tests.

The tests extensively use ReflectionTestUtils to access and manipulate private fields and methods of PKCS11KeyStoreImpl. While this provides thorough coverage of internal behavior, it tightly couples tests to implementation details, making them fragile during refactoring.

Consider balancing coverage with maintainability:

• Focus on testing public API behavior where possible
• Reserve reflection-based tests for critical internal logic that must be verified
• Document why reflection is necessary in complex test cases

Also applies to: 248-253, 267-268, 276-278, 286-288, 296-297, 313-314, 331-332, 348-349, 365-366, 374-375, 380-388, 393-402, 407-425, 430-439, 444-461, 470-471, 480-481, 489-490

kernel/kernel-keymanager-service/pom.xml (1)

149-154: Consider upgrading to Mockito 5.x to simplify dependencies.

Mockito 5.20.0 (latest stable, Sep 2025) includes inline mocking by default out of the box, making the separate mockito-inline artifact unnecessary. The project currently uses Mockito 3.4.3 from 2020, so upgrading would eliminate the redundant dependency and modernize the codebase. However, this is a major version bump and requires verification of breaking changes and compatibility with existing tests before proceeding.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c0adde and 1c1703d.

📒 Files selected for processing (3)

• kernel/kernel-keymanager-service/pom.xml (1 hunks)
• kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/PKCS11KeyStoreImplTest.java (1 hunks)
• kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/partnercertservice/test/controller/PartnerCertManagerControllerTest.java (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/PKCS11KeyStoreImplTest.java (2)

kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/pkcs/PKCS11KeyStoreImpl.java (1)

• PKCS11KeyStoreImpl (62-656)

kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java (1)

• CertificateUtility (46-289)

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

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/partnercertservice/test/controller/PartnerCertManagerControllerTest.java (1)

531-531: LGTM! Test data now consistent with certificate content.

The organizationName change from "IITB" to "Mosip" aligns with the certificate's organization field and matches the usage pattern in other tests (e.g., Line 259).

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/PKCS11KeyStoreImplTest.java (2)

549-550: Deprecated Provider constructor usage is acceptable for test purposes.

The nested provider classes use the deprecated Provider(String, double, String) constructor (lines 554, 578, 600), which is suppressed via @SuppressWarnings("deprecation"). This is acceptable for test scaffolding where the deprecated API provides the simplest way to create custom providers.

Also applies to: 573-575, 595-597

---

117-491: Excellent test coverage and structure.

The test suite provides comprehensive coverage of PKCS11KeyStoreImpl functionality, including:

• Key generation and storage (RSA, EC, symmetric)
• Key retrieval and caching
• Certificate handling
• Provider reload logic
• Error scenarios and exception handling
• Edge cases (blank passwords, missing providers, etc.)

The test naming convention ("should...") and use of helper methods enhance readability and maintainability.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/OLKeyStoreImplTest.java (2)

37-56: SpringRunner + plain @Mock fields add complexity without clear benefit

You’re using @RunWith(SpringRunner.class) and @Mock fields, but there’s no Spring test config (@SpringBootTest, @ContextConfiguration, @MockBean, etc.) and no Mockito initialization (MockitoJUnitRunner, MockitoRule, MockitoAnnotations.initMocks(this)). In this setup the @Mock fields are likely just null, and OLKeyStoreImpl ignores them anyway (it always throws), so the annotations and custom runner don’t buy much.

Consider simplifying by either:

• Dropping SpringRunner and initializing Mockito properly, or
• Dropping the @Mock annotations (passing null is enough here since args are unused), and using the default JUnit runner.

This will make the tests’ intent and dependencies clearer.

---

58-71: Constructor tests currently use tautological assertions

assertThat(instance, is(instance)); will always pass and doesn’t verify anything beyond “constructor did not throw” (which JUnit already guarantees if the test completes). If you want a stronger check, you could assert something meaningful (e.g., non-null instance or a specific property), or simply omit the assertion and rely on the absence of exceptions.

This is purely a readability/clarity tweak; behavior is correct as-is.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c1703d and 46046ff.

📒 Files selected for processing (1)

• kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/OLKeyStoreImplTest.java (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/OLKeyStoreImplTest.java (1)

kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/offline/OLKeyStoreImpl.java (1)

• OLKeyStoreImpl (28-176)

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

kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanager/hsm/test/OLKeyStoreImplTest.java (1)

73-299: Offline keystore exception behavior is well covered and matches OLKeyStoreImpl

All the operation tests (getAllAlias, getKey, getAsymmetricKey, getPrivateKey, getPublicKey, getCertificate, getSymmetricKey, deleteKey, generateAndStoreAsymmetricKey (both overloads), generateAndStoreSymmetricKey, storeCertificate) correctly expect KeystoreProcessingException and, in the detail tests, assert errorCode and errorText against KeymanagerErrorCode.OFFLINE_KEYSTORE_ACCESS_ERROR. This aligns with the production implementation in OLKeyStoreImpl (kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/offline/OLKeyStoreImpl.java, lines 27–175), which always throws that error for these methods and returns KEYSTORE_TYPE_OFFLINE for the provider name.

The coverage across normal and parameter edge cases (null aliases/params, EC curve overload) looks consistent and should guard against regressions in the offline keystore behavior.