[INJIWEB-1621] - Refractor: consolidate KeyGeneration, JWTGeneration, Proof Signing Key utilities using factory pattern #994
Conversation
… proof Signing Key utilities using factory pattern - Merge KeyGenerationUtil and JwtGeneratorUtil into SigningKeyUtil - Implement Strategy pattern with algorithm-specific handlers (RS256, ES256, ES256K, ED25519) - Add SigningAlgorithmHandlerFactory for handler management - Create BouncyCastleProviderUtil for centralized provider access - Add comprehensive test suite for all handlers and utilities - Remove deprecated ProofSigningKeyFactory, KeyGenerationUtil, and JwtGeneratorUtil Signed-off-by: Durgesh Konga <kongadurgesh20@gmail.com>
WalkthroughConsolidates JWT and key-generation utilities into a single SigningKeyUtil backed by a SigningAlgorithmHandler strategy factory; removes legacy JwtGeneratorUtil/KeyGenerationUtil/ProofSigningKeyFactory; adds algorithm-specific handlers, a BouncyCastle provider util, and updates services/tests to use the new APIs. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Service as Wallet/Credential Service
participant SigningKeyUtil
participant Handler as SigningAlgorithmHandler
participant BC as BouncyCastleProviderUtil
Note over Service,SigningKeyUtil: Request to create JWT / keypair
Client->>Service: submit request (audience, clientId, cNonce, algorithm)
Service->>SigningKeyUtil: generateJwt(algorithm, audience, clientId, cNonce, keyPair?)
alt New key pair required
SigningKeyUtil->>Handler: generateKeyPair()
Handler->>BC: getProvider() (if handler needs BC)
BC-->>Handler: Provider
Handler-->>SigningKeyUtil: KeyPair
else Key pair from bytes
SigningKeyUtil->>Handler: getKeyFactory() / reconstruct keys
end
SigningKeyUtil->>Handler: createJWK(KeyPair)
Handler-->>SigningKeyUtil: JWK
SigningKeyUtil->>Handler: createSigner(JWK)
Handler-->>SigningKeyUtil: JWSSigner
SigningKeyUtil->>Service: serialized Signed JWT
Service->>Client: return JWT
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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
🤖 Fix all issues with AI Agents
In @src/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.java:
- Around line 15-27: The current static block may not register the BC_PROVIDER
and getProvider() returns the local BC_PROVIDER regardless, causing
inconsistency; change getProvider() to fetch the globally registered provider
via Security.getProvider(BouncyCastleProvider.PROVIDER_NAME), and if that
returns null register BC_PROVIDER (using Security.addProvider(BC_PROVIDER)) and
then return the now-registered provider; this ensures the returned Provider
matches the Security context and references BC_PROVIDER, keeping the static
block and getProvider() behavior consistent.
In @src/main/java/io/mosip/mimoto/util/SigningKeyUtil.java:
- Around line 167-170: The catch block in SigningKeyUtil (catching
NoSuchAlgorithmException | InvalidAlgorithmParameterException |
NoSuchProviderException) logs exception e but rethrows a KeyGenerationException
without preserving the original cause and uses the wrong ENCRYPTION_FAILED code;
update the throw to pass e as the cause into KeyGenerationException (so the
stacktrace is preserved) and replace ENCRYPTION_FAILED with a dedicated
key-generation error code (e.g., KEY_GENERATION_FAILED) defined in your error
codes enum/constant, ensuring the message still includes the algorithm context.
🧹 Nitpick comments (10)
src/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.java (1)
15-19: Consider adding error handling for static initialization.The static block could potentially throw exceptions during class initialization (e.g., if
Security.addProviderfails), which would result in anExceptionInInitializerErrorthat's difficult to debug. While unlikely in practice, consider adding defensive error handling or at least logging.🔎 Suggested improvement
static { - if (Security.getProvider(BouncyCastleProvider.PROVIDER_NAME) == null) { - Security.addProvider(BC_PROVIDER); + try { + if (Security.getProvider(BouncyCastleProvider.PROVIDER_NAME) == null) { + Security.addProvider(BC_PROVIDER); + } + } catch (Exception e) { + // Log error or handle gracefully + throw new ExceptionInInitializerError("Failed to register BouncyCastle provider: " + e.getMessage()); } }src/test/java/io/mosip/mimoto/util/factory/ES256KAlgorithmHandlerTest.java (1)
69-76: Consider improving exception handling for invalid JWK.The test expects
ClassCastExceptionwhen an invalid (non-ECKey) JWK is passed tocreateSigner. While this tests defensive behavior, it relies on implementation details. Consider:
- The handler should validate the JWK type and throw a more specific exception (e.g.,
IllegalArgumentExceptionorJOSEException) with a clear message- Update the test to expect the specific exception
This makes the API contract clearer and the test less brittle.
🔎 Suggested improvement
In
ES256KAlgorithmHandler.createSigner:public JWSSigner createSigner(JWK jwk) throws JOSEException { if (!(jwk instanceof ECKey)) { throw new IllegalArgumentException("JWK must be an ECKey for ES256K algorithm"); } return new ECDSASigner((ECKey) jwk); }Then update the test:
-@Test(expected = ClassCastException.class) +@Test(expected = IllegalArgumentException.class) public void shouldThrowExceptionWhenJWKIsInvalid() throws Exception { // Create invalid JWK (not ECKey) KeyPair rsaKeyPair = new RS256AlgorithmHandler().generateKeyPair(); JWK invalidJwk = new RS256AlgorithmHandler().createJWK(rsaKeyPair); handler.createSigner(invalidJwk); }src/test/java/io/mosip/mimoto/util/factory/ES256AlgorithmHandlerTest.java (1)
24-30: Redundant BouncyCastle provider registration.The
BouncyCastleProviderUtilalready registers the provider in its static initializer. SinceES256AlgorithmHandler.createSigner()usesBouncyCastleProviderUtil.getProvider(), triggering class loading will register the provider automatically. This manual registration is harmless but redundant.🔎 Suggested simplification
@Before public void setUp() { - // Ensure BouncyCastle provider is registered (required for signing operations) - if (Security.getProvider(BouncyCastleProvider.PROVIDER_NAME) == null) { - Security.addProvider(new BouncyCastleProvider()); - } + // Trigger BouncyCastleProviderUtil class loading to ensure provider registration + BouncyCastleProviderUtil.getProvider(); }src/test/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtilTest.java (1)
16-52: Good test coverage with minor redundancy.The tests correctly verify:
- Provider instance is non-null and correct type
- Singleton pattern (same instance on multiple calls)
- Provider is registered with Security
- Provider name matches expected value
Note:
shouldNotReturnNullProvider(lines 47-52) duplicates the null check inshouldReturnProviderInstance(lines 16-22). Consider consolidating.src/main/java/io/mosip/mimoto/util/factory/RS256AlgorithmHandler.java (1)
20-27: Consider upgrading RSA key size from 2048-bit to 3072-bit for long-term credential security.NIST recommends 3072-bit RSA for credentials with multi-year lifetimes. Since identity systems typically issue long-lived credentials, the current 2048-bit key size may not provide sufficient security margin beyond 2030 without periodic re-issuance. Update
RSA_KEY_SIZEto 3072 and document the reasoning if 2048-bit is retained.src/main/java/io/mosip/mimoto/util/factory/ED25519AlgorithmHandler.java (1)
44-52: Consider using proper key specs instead of raw byte extraction.The current implementation extracts the last 32 bytes from encoded keys (lines 48-49), which assumes specific encoding formats (X.509 for public, PKCS#8 for private). This approach is fragile and may break if encoding formats change.
Consider using
EdECPublicKey.getPoint()and proper EdEC key parameters for more robust key material extraction, or at minimum document the encoding format assumptions.Alternative approach
A more robust implementation could use the key interfaces directly:
// For newer JDK versions with full EdEC support EdECPublicKey edECPublicKey = (EdECPublicKey) keyPair.getPublic(); EdECPrivateKey edECPrivateKey = (EdECPrivateKey) keyPair.getPrivate(); // Extract using proper methods if available, or document the encoding assumption byte[] x = edECPublicKey.getPoint().getY().toByteArray(); // May need adjustment byte[] d = edECPrivateKey.getBytes().orElseThrow();If the current byte slicing approach is retained, add a comment explaining the encoding format dependency.
src/test/java/io/mosip/mimoto/util/factory/RS256AlgorithmHandlerTest.java (2)
24-30: Consider using BouncyCastleProviderUtil for consistency.The PR introduces
BouncyCastleProviderUtilto centralize provider access. Using it here would maintain consistency with other algorithm handler tests and the production code.🔎 Suggested refactor
-import org.bouncycastle.jce.provider.BouncyCastleProvider; +import io.mosip.mimoto.util.factory.BouncyCastleProviderUtil; import org.junit.Before; import org.junit.Test; import java.security.KeyFactory; import java.security.KeyPair; -import java.security.Security; ... @Before public void setUp() { - // Ensure BouncyCastle provider is registered (required for signing operations) - if (Security.getProvider(BouncyCastleProvider.PROVIDER_NAME) == null) { - Security.addProvider(new BouncyCastleProvider()); - } + // Ensure BouncyCastle provider is registered + BouncyCastleProviderUtil.ensureRegistered(); }
69-76: Minor: Avoid redundant handler instantiation.Two
ES256AlgorithmHandlerinstances are created when one would suffice.🔎 Suggested fix
@Test(expected = ClassCastException.class) public void shouldThrowExceptionWhenJWKIsInvalid() throws Exception { // Create invalid JWK (not RSAKey) - KeyPair ecKeyPair = new ES256AlgorithmHandler().generateKeyPair(); - JWK invalidJwk = new ES256AlgorithmHandler().createJWK(ecKeyPair); + ES256AlgorithmHandler ecHandler = new ES256AlgorithmHandler(); + KeyPair ecKeyPair = ecHandler.generateKeyPair(); + JWK invalidJwk = ecHandler.createJWK(ecKeyPair); handler.createSigner(invalidJwk); }src/main/java/io/mosip/mimoto/util/SigningKeyUtil.java (2)
35-35: Consider making JWT expiration configurable.The 5-hour expiration is hardcoded. For proof JWTs in OpenID4VCI flows, this may be longer than necessary and could be a security concern. Consider externalizing this as a configuration property.
176-182: Consider formatting the claims builder for readability.The single-line builder chain is hard to scan. Breaking it across lines improves maintainability.
🔎 Suggested formatting
private static JWTClaimsSet createClaims(String clientId, String audience, String cNonce) { long nowSeconds = System.currentTimeMillis() / 1000; Date issuedAt = new Date(nowSeconds * 1000); Date expiresAt = new Date((nowSeconds + JWT_EXPIRATION_SECONDS) * 1000); - return new JWTClaimsSet.Builder().subject(clientId).audience(audience).issuer(clientId).issueTime(issuedAt).expirationTime(expiresAt).claim("nonce", cNonce).build(); + return new JWTClaimsSet.Builder() + .subject(clientId) + .audience(audience) + .issuer(clientId) + .issueTime(issuedAt) + .expirationTime(expiresAt) + .claim("nonce", cNonce) + .build(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.talismanrcsrc/main/java/io/mosip/mimoto/service/impl/CredentialRequestServiceImpl.javasrc/main/java/io/mosip/mimoto/service/impl/KeyPairRetrievalServiceImpl.javasrc/main/java/io/mosip/mimoto/service/impl/WalletPresentationServiceImpl.javasrc/main/java/io/mosip/mimoto/util/JwtGeneratorUtil.javasrc/main/java/io/mosip/mimoto/util/KeyGenerationUtil.javasrc/main/java/io/mosip/mimoto/util/ProofSigningKeyFactory.javasrc/main/java/io/mosip/mimoto/util/SigningKeyUtil.javasrc/main/java/io/mosip/mimoto/util/WalletUtil.javasrc/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.javasrc/main/java/io/mosip/mimoto/util/factory/ED25519AlgorithmHandler.javasrc/main/java/io/mosip/mimoto/util/factory/ES256AlgorithmHandler.javasrc/main/java/io/mosip/mimoto/util/factory/ES256KAlgorithmHandler.javasrc/main/java/io/mosip/mimoto/util/factory/RS256AlgorithmHandler.javasrc/main/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandler.javasrc/main/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandlerFactory.javasrc/test/java/io/mosip/mimoto/service/CredentialRequestServiceTest.javasrc/test/java/io/mosip/mimoto/service/KeyPairServiceTest.javasrc/test/java/io/mosip/mimoto/service/WalletPresentationServiceTest.javasrc/test/java/io/mosip/mimoto/util/EncryptionDecryptionUtilTest.javasrc/test/java/io/mosip/mimoto/util/JwtGeneratorUtilTest.javasrc/test/java/io/mosip/mimoto/util/KeyGenerationUtilTest.javasrc/test/java/io/mosip/mimoto/util/SigningKeyUtilTest.javasrc/test/java/io/mosip/mimoto/util/WalletUtilTest.javasrc/test/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtilTest.javasrc/test/java/io/mosip/mimoto/util/factory/ED25519AlgorithmHandlerTest.javasrc/test/java/io/mosip/mimoto/util/factory/ES256AlgorithmHandlerTest.javasrc/test/java/io/mosip/mimoto/util/factory/ES256KAlgorithmHandlerTest.javasrc/test/java/io/mosip/mimoto/util/factory/RS256AlgorithmHandlerTest.javasrc/test/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandlerFactoryTest.java
💤 Files with no reviewable changes (5)
- src/main/java/io/mosip/mimoto/util/JwtGeneratorUtil.java
- src/test/java/io/mosip/mimoto/util/JwtGeneratorUtilTest.java
- src/test/java/io/mosip/mimoto/util/KeyGenerationUtilTest.java
- src/main/java/io/mosip/mimoto/util/ProofSigningKeyFactory.java
- src/main/java/io/mosip/mimoto/util/KeyGenerationUtil.java
🧰 Additional context used
🧬 Code graph analysis (7)
src/main/java/io/mosip/mimoto/util/factory/RS256AlgorithmHandler.java (1)
src/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.java (1)
BouncyCastleProviderUtil(11-28)
src/test/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtilTest.java (1)
src/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.java (1)
BouncyCastleProviderUtil(11-28)
src/test/java/io/mosip/mimoto/util/SigningKeyUtilTest.java (1)
src/main/java/io/mosip/mimoto/exception/KeyGenerationException.java (1)
KeyGenerationException(3-13)
src/test/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandlerFactoryTest.java (1)
src/main/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandlerFactory.java (1)
SigningAlgorithmHandlerFactory(13-39)
src/main/java/io/mosip/mimoto/util/factory/ES256KAlgorithmHandler.java (1)
src/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.java (1)
BouncyCastleProviderUtil(11-28)
src/main/java/io/mosip/mimoto/util/factory/ES256AlgorithmHandler.java (1)
src/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.java (1)
BouncyCastleProviderUtil(11-28)
src/main/java/io/mosip/mimoto/util/SigningKeyUtil.java (2)
src/main/java/io/mosip/mimoto/exception/KeyGenerationException.java (1)
KeyGenerationException(3-13)src/main/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandlerFactory.java (1)
SigningAlgorithmHandlerFactory(13-39)
🪛 ast-grep (0.40.3)
src/main/java/io/mosip/mimoto/util/factory/ES256KAlgorithmHandler.java
[warning] 27-27: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: KeyPairGenerator.getInstance("EC", PROVIDER)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 35-35: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: KeyFactory.getInstance("EC", PROVIDER)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 27-27: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: KeyPairGenerator.getInstance("EC", PROVIDER)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 35-35: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: KeyFactory.getInstance("EC", PROVIDER)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
⏰ 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-maven-apitest-mimoto / maven-build
🔇 Additional comments (41)
src/test/java/io/mosip/mimoto/util/EncryptionDecryptionUtilTest.java (2)
95-96: Consistent refactoring in test method.The same
SigningKeyUtilreplacements in the test method maintain consistency with the setUp method.
51-52: Code is correct, but KeyGenerationUtil replacement claim is unfounded.The test correctly uses
SigningKeyUtilfor key generation. However,KeyGenerationUtildoes not exist in the repository—there is no replacement occurring. The methodsSigningKeyUtil.generateEncryptionKey(String algorithm, int keySize)(returningSecretKey) andSigningKeyUtil.generateKeyPair(SigningAlgorithm algorithm)(returningKeyPair) are properly defined and used correctly at lines 51-52 and 95-96.Likely an incorrect or invalid review comment.
src/test/java/io/mosip/mimoto/util/WalletUtilTest.java (1)
53-53: LGTM! Consistent with refactoring pattern.The replacement of
KeyGenerationUtilwithSigningKeyUtilfor encryption key generation aligns with the refactoring across other test files.src/main/java/io/mosip/mimoto/service/impl/KeyPairRetrievalServiceImpl.java (2)
10-10: Import updated correctly.The import change from
KeyGenerationUtiltoSigningKeyUtilaligns with the PR's consolidation objectives.
77-77: Verify method signature and behavior equivalence.The method name changed from
getKeyPairFromDBStoredKeystogenerateKeyPairFromBytes, which better describes the operation (generating a KeyPair from byte arrays rather than implying DB access).Please verify that the new method has the same signature and behavior, and that the old method is removed.
src/test/java/io/mosip/mimoto/service/CredentialRequestServiceTest.java (3)
50-50: MockedStatic updated correctly.The static mock target has been properly updated from
KeyGenerationUtiltoSigningKeyUtil, consistent with the refactoring.
63-67: Proper cleanup of mocked static.The
@Aftermethod correctly closes theMockedStaticinstance, preventing test interference. Good practice!
171-171: Verification calls updated consistently.All static method verifications have been consistently updated to reference
SigningKeyUtil.generateKeyPairinstead ofKeyGenerationUtil.generateKeyPair. The test logic correctly validates algorithm selection based on issuer support.Also applies to: 200-200, 228-228, 256-256
src/main/java/io/mosip/mimoto/util/WalletUtil.java (2)
42-42: No action required. TheSigningKeyUtil.generateEncryptionKey("AES", 256)method is properly implemented and tested. Verification confirms the method exists with the correct signature, generates AES-256 keys correctly, and includes appropriate exception handling for unsupported algorithms.
80-80: ProofSigningKey creation method verified.
SigningKeyUtil.createProofSigningKeyexists and correctly generates proof signing keys for all supported algorithms (RS256, ES256, ES256K, ED25519). The method properly wraps key pairs with metadata and is comprehensively tested.src/main/java/io/mosip/mimoto/service/impl/WalletPresentationServiceImpl.java (1)
259-263: No concerns identified. BothSigningKeyUtil.generateJwk()andSigningKeyUtil.createSigner()methods exist with correct signatures and compatible implementations. The refactoring properly delegates to the handler pattern for algorithm-specific behavior.src/main/java/io/mosip/mimoto/service/impl/CredentialRequestServiceImpl.java (1)
58-60: SigningKeyUtil methods verified and properly implemented.The refactoring consolidates cryptographic operations into
SigningKeyUtil:
- Line 58:
generateKeyPair(signingAlgorithm)✓ exists with matching signature- Lines 60, 113:
generateJwt(signingAlgorithm, issuer, clientId, nonce, keyPair)✓ exists with matching signatureBoth methods are public static, properly documented, and all call sites use correct parameter types and order. The consolidation from
KeyGenerationUtilandJwtGeneratorUtilis complete.src/main/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandlerFactory.java (1)
13-38: LGTM! Clean factory pattern implementation.Good use of
EnumMapfor efficient enum-keyed storage. The static initialization ensures thread-safe reads since the map is never modified after class loading. The design correctly relies on handler implementations being stateless (as documented in the interface).src/test/java/io/mosip/mimoto/util/factory/ES256AlgorithmHandlerTest.java (1)
69-76: Test validates correct exception behavior for type mismatch.Good defensive test ensuring
createSignerproperly fails when given an incompatible JWK type (RSA instead of EC).src/main/java/io/mosip/mimoto/util/factory/RS256AlgorithmHandler.java (1)
34-47: Implementation is correct and follows the handler pattern.The handler correctly:
- Creates JWK with RS256 algorithm and SIGNATURE use
- Configures the signer with BouncyCastle provider for consistency
src/test/java/io/mosip/mimoto/service/KeyPairServiceTest.java (1)
11-11: Migration to SigningKeyUtil is consistent and complete.The test file correctly migrates from
KeyGenerationUtiltoSigningKeyUtil, updating all mock references and method calls fromgetKeyPairFromDBStoredKeystogenerateKeyPairFromBytes. The test coverage remains comprehensive across all signing algorithms.Also applies to: 87-94
src/main/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandler.java (1)
20-30: Well-designed strategy interface.The interface correctly abstracts algorithm-specific operations. The
@implNotedocumenting thread-safety and statelessness requirements is helpful for implementers.One consideration:
createJWKperforms unchecked casts internally in implementations (e.g., toECPublicKeyorRSAPublicKey). Consider whether documenting the expected key types in the Javadoc would improve usability.src/main/java/io/mosip/mimoto/util/factory/ES256KAlgorithmHandler.java (2)
26-32: Static analysis warnings are false positives - ignore them.The ast-grep warnings about "Triple DES" and "AES with ECB mode" are incorrect. This code uses
"EC"(Elliptic Curve) for ECDSA key generation, not encryption. The secp256k1 curve correctly requires the BouncyCastle provider as it's not available in the default JCE.
39-52: Correct ES256K implementation using secp256k1.The handler properly:
- Uses
Curve.SECP256K1for the JWK builder- Sets
JWSAlgorithm.ES256K- Configures the signer with BouncyCastle provider
src/main/java/io/mosip/mimoto/util/factory/ES256AlgorithmHandler.java (4)
26-31: LGTM: Key pair generation correctly uses secp256r1 curve.The implementation properly initializes the EC key pair generator with the P-256 curve parameters. The method relies on the default JCA provider for key generation, which is acceptable as BouncyCastle is explicitly used for signing operations (line 49).
34-36: LGTM: KeyFactory retrieval is correct.
39-44: LGTM: JWK creation is correctly configured.The method properly constructs an ECKey with the P-256 curve, ES256 algorithm, and signature key use, matching the key generation parameters.
47-51: LGTM: Signer creation correctly uses BouncyCastle provider.The implementation properly instantiates an ECDSASigner and explicitly configures it to use the BouncyCastle provider for cryptographic operations.
src/test/java/io/mosip/mimoto/service/WalletPresentationServiceTest.java (2)
24-24: LGTM: Import updated to SigningKeyUtil.The import change from JwtGeneratorUtil to SigningKeyUtil aligns with the PR's consolidation objective.
232-236: LGTM: Mock static contexts consistently updated.All MockedStatic blocks and method calls have been systematically updated from
JwtGeneratorUtiltoSigningKeyUtil. The test logic and assertions remain unchanged, confirming this is a clean refactoring.Also applies to: 306-310, 420-421, 448-452, 490-494, 529-534, 626-627, 649-650, 678-682, 733-734, 762-763, 774-778, 813-817, 857-860, 900-904, 947-951, 991-995, 1034-1038
src/test/java/io/mosip/mimoto/util/factory/ED25519AlgorithmHandlerTest.java (5)
22-29: LGTM: Key pair generation test is comprehensive.The test properly validates key pair generation, including non-null checks and algorithm verification.
32-37: LGTM: KeyFactory test is correct.
40-47: LGTM: JWK creation test validates type and algorithm.The test correctly verifies that the JWK is an OctetKeyPair with Ed25519 algorithm.
50-56: LGTM: Signer creation test covers the happy path.
59-65: LGTM: Negative test validates type safety.The test correctly verifies that attempting to create a signer with a mismatched JWK type throws ClassCastException, ensuring type safety in the handler.
src/test/java/io/mosip/mimoto/util/SigningKeyUtilTest.java (7)
23-61: LGTM: Test setup and key generation tests are comprehensive.The test suite properly initializes the BouncyCastle provider and validates key generation for all supported algorithms with correct algorithm assertions.
64-85: LGTM: Key reconstruction tests cover positive and negative cases.The tests properly validate key pair reconstruction from bytes and exception handling for invalid input.
87-98: LGTM: Encryption key generation tests are appropriate.
100-118: LGTM: JWT generation test validates structure and claims.The test properly verifies JWT creation, parsing, and claim extraction including subject, audience, and nonce.
120-127: LGTM: JWK generation test covers the basics.
129-145: LGTM: Signer creation tests validate functionality and type safety.The tests properly cover successful signer creation and verify that mismatched JWK types throw the expected exception.
147-171: LGTM: ProofSigningKey tests provide excellent coverage.The tests validate ProofSigningKey creation for RS256 specifically and comprehensively for all algorithms, ensuring consistency across all supported signing algorithms.
src/main/java/io/mosip/mimoto/util/factory/ED25519AlgorithmHandler.java (2)
55-84: LGTM: Custom JWSSigner implementation is correct.The custom JWSSigner implementation properly uses BouncyCastle's Ed25519Signer for signing operations. Creating a new signer instance on each
sign()call (line 65) ensures thread-safety, though it sacrifices some performance. This is an acceptable trade-off for correctness.
33-41: No action required. The project is configured with Java 21 (bothmaven.compiler.sourceandmaven.compiler.target), which fully supports Ed25519. The Ed25519 algorithm was introduced in Java 15, and Java 21 exceeds this requirement by a significant margin.src/test/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandlerFactoryTest.java (1)
1-55: LGTM!The test suite comprehensively covers the factory behavior - validating correct handler type resolution for all supported algorithms and proper exception handling for null input.
src/main/java/io/mosip/mimoto/util/SigningKeyUtil.java (1)
46-141: Well-structured consolidation of key management operations.The factory-based delegation pattern cleanly abstracts algorithm-specific details. The public API is cohesive and follows the single-responsibility principle for signing key operations.
src/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.java
Show resolved
Hide resolved
…ecurity Check Security.getProvider() before creating new BouncyCastleProvider to ensure BC_PROVIDER matches the registered provider instance. Make class final and add private constructor for utility class pattern. Signed-off-by: Durgesh Konga <kongadurgesh20@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.java (1)
33-35: Consider single-line constructor for consistency.The empty private constructor could be simplified to a single line for consistency with standard utility class patterns.
🔎 Proposed refactor
- private BouncyCastleProviderUtil(){ - - } + private BouncyCastleProviderUtil() {}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.talismanrcsrc/main/java/io/mosip/mimoto/util/SigningKeyUtil.javasrc/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.javasrc/main/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandlerFactory.javasrc/test/java/io/mosip/mimoto/util/WalletUtilTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/java/io/mosip/mimoto/util/WalletUtilTest.java
- src/main/java/io/mosip/mimoto/util/SigningKeyUtil.java
- .talismanrc
- src/main/java/io/mosip/mimoto/util/factory/SigningAlgorithmHandlerFactory.java
⏰ 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). (2)
- GitHub Check: build-mimoto / maven-build
- GitHub Check: build-maven-apitest-mimoto / maven-build
🔇 Additional comments (2)
src/main/java/io/mosip/mimoto/util/factory/BouncyCastleProviderUtil.java (2)
15-23: LGTM! Previous provider registration inconsistency resolved.The static initialization now correctly ensures
BC_PROVIDERalways references a provider that is registered in theSecuritycontext. By reusing an existing provider (line 18) or registering a new one (line 21), this addresses the concern from the previous review where the returned provider could be inconsistent with the global Security context.
29-31: LGTM! Provider access is consistent.The
getProvider()method correctly returns the singletonBC_PROVIDERinstance, which is guaranteed to be registered by the static block logic.
1 participant
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.