[INJICERT-1295] Update mosipid from vcissuance to data provider plugin #130
Conversation
[INJICERT-1295] Update mosipid plugin mode to dataprovider (inji#127) * [INJICERT-1295] Update mosipid plugin mode to dataprovider Signed-off-by: Piyush7034 <[email protected]> * [INJICERT-1295] Update RC to SNAPSHOT for all pom files Signed-off-by: Piyush7034 <[email protected]> --------- Signed-off-by: Piyush7034 <[email protected]>
Signed-off-by: Piyush7034 <[email protected]>
Signed-off-by: Piyush7034 <[email protected]>
WalkthroughBumps multiple plugin versions to 0.6.0-SNAPSHOT, migrates certify artifacts to groupId Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IdaDataProviderPluginImpl
participant TransactionHelper
participant HSM as KeyStore/HSM
participant HelperService
participant IDA as IDA_KYC_Service
participant JWT as JWT_Decoder
Client->>IdaDataProviderPluginImpl: fetchData(identityDetails)
IdaDataProviderPluginImpl->>TransactionHelper: getOAuthTransaction(accessTokenHash)
TransactionHelper-->>IdaDataProviderPluginImpl: OIDCTransaction
IdaDataProviderPluginImpl->>HSM: getSecretKeyFromHSM() (if individualId encrypted)
HSM-->>IdaDataProviderPluginImpl: symmetric_key
IdaDataProviderPluginImpl->>HelperService: getRequestSignature(headers, payload)
HelperService-->>IdaDataProviderPluginImpl: signed_headers
IdaDataProviderPluginImpl->>IDA: POST KYC Exchange Request (signed)
IDA-->>IdaDataProviderPluginImpl: IdaKycExchangeResponse (encryptedKyc)
IdaDataProviderPluginImpl->>JWT: decodeClaimsFromJwt(encryptedKyc)
JWT-->>IdaDataProviderPluginImpl: claims_map
IdaDataProviderPluginImpl-->>Client: JSONObject(claims_map)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mosip-identity-certify-plugin/pom.xml (1)
133-138: Duplicatejackson-annotationsdependency with conflicting versions.The
jackson-annotationsdependency is declared twice:
- Lines 133-138: version
${jackson.version}(2.12.1) withtestscope- Lines 168-172: version
2.15.4with default (compile) scopeThis creates version inconsistency. Either consolidate to a single declaration or update
${jackson.version}property to2.15.4and remove the duplicate.🔧 Suggested fix - Remove the test-scoped duplicate
- <dependency> - <groupId>com.fasterxml.jackson.core</groupId> - <artifactId>jackson-annotations</artifactId> - <version>${jackson.version}</version> - <scope>test</scope> - </dependency>And optionally update the property for consistency:
- <jackson.version>2.12.1</jackson.version> + <jackson.version>2.15.4</jackson.version>Also applies to: 168-172
mock-certify-plugin/pom.xml (1)
65-91: Dependency version mismatch detected: certify-core 0.14.0-SNAPSHOT is not available in public repositories.The esignet libraries (1.6.2) are correctly available in Maven Central. However,
certify-coreversion0.14.0-SNAPSHOT(groupIdio.inji.certify) returns 404 when queried against the Sonatype snapshot repository. The project will fail to build unless this SNAPSHOT is available in a configured private repository or deployed before the build.Ensure the
0.14.0-SNAPSHOTversion of certify-core is available in your Maven build environment (e.g., deployed to a private nexus/artifactory or built locally).
🤖 Fix all issues with AI agents
In
@mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:
- Around line 110-114: The code in IdaDataProviderPluginImpl logs potentially
sensitive KYC payloads (kycExchangeResult.getEncryptedKyc() and decoded claims
via decodeClaimsFromJwt) at info level; change these log statements to avoid
exposing PII by either removing them entirely or lowering to debug/trace and
ensure they are only logged when debug is enabled (e.g., wrap with
log.isDebugEnabled()), and stop logging the decoded claims object (remove
log.info("Jwt Claims: {}", claims)) — reference kycExchangeResult, encryptedKyc,
and decodeClaimsFromJwt to locate and update the statements.
- Around line 222-228: The RequestEntity builder is wrong: replace deprecated
MediaType.APPLICATION_JSON_UTF8 with MediaType.APPLICATION_JSON and set the
Authorization header to the actual token instead of the literal string; obtain
the auth token (e.g., via the AuthTransactionHelper method used elsewhere in
this class) and pass "Bearer " + token as the value for
AUTHORIZATION_HEADER_NAME while keeping the signature header generated by
helperService.getRequestSignature(requestBody) and the rest of the RequestEntity
construction (kycExchangeUrl, relyingPartyId, clientId, requestBody,
SIGNATURE_HEADER_NAME) unchanged.
In @postgres-dataprovider-plugin/pom.xml:
- Line 7: The pom sets the project <version>0.6.0-SNAPSHOT</version> and a
dependency io.inji.certify:certify-core:0.14.0-SNAPSHOT; ensure these SNAPSHOT
artifacts are available in your configured repositories or replace them with
released versions: update the <version> element for the project and/or change
the dependency coordinate in the dependency block (io.inji.certify:certify-core)
to a release version, or add the appropriate snapshot repository entry to your
pom/settings so Maven can resolve the SNAPSHOTs.
🧹 Nitpick comments (7)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/HelperService.java (1)
66-79: Empty catch block silently swallows exceptions; consider usingLocale.forLanguageTag()instead.
- The empty
catch (MissingResourceException ex)block silently ignores errors without any logging, making debugging difficult.- The
new Locale(String)constructor is deprecated since Java 19. Consider usingLocale.forLanguageTag(langCode)orLocale.of(langCode)for better handling.♻️ Suggested improvement
protected List<String> convertLangCodesToISO3LanguageCodes(String[] langCodes) { if(langCodes == null || langCodes.length == 0) return List.of(); return Arrays.stream(langCodes) .map(langCode -> { try { - return StringUtils.isEmpty(langCode) ? null : new Locale(langCode).getISO3Language(); - } catch (MissingResourceException ex) {} - return null; + if (StringUtils.isEmpty(langCode)) { + return null; + } + Locale locale = Locale.forLanguageTag(langCode); + return locale.getISO3Language(); + } catch (MissingResourceException ex) { + log.warn("Unable to convert language code '{}' to ISO3: {}", langCode, ex.getMessage()); + return null; + } }) .filter(Objects::nonNull) .collect(Collectors.toList()); }mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/dto/IdaKycExchangeResponse.java (1)
10-14: LGTM - Simple and appropriate DTO.The DTO is well-structured for its purpose.
For consistency with
IdaKycExchangeRequest, consider adding@JsonInclude(JsonInclude.Include.NON_NULL)annotation, though this is optional given the single-field nature of this response DTO.mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/dto/IdaKycExchangeRequest.java (1)
35-39: Add explicitprivateaccess modifier for consistency.The fields
verifiedConsentedClaimsandunVerifiedConsentedClaimsare missing explicit access modifiers, making them package-private. For consistency with the other fields and proper encapsulation, addprivate:♻️ Suggested fix
/** * User consented verified claims list. */ - List<Map<String, Object>> verifiedConsentedClaims; + private List<Map<String, Object>> verifiedConsentedClaims; /** * User consented unverified claims list. */ - Map<String, Object> unVerifiedConsentedClaims; + private Map<String, Object> unVerifiedConsentedClaims;mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java (4)
46-48: Reminder: Address TODO comments before release.The TODO comments indicate cleanup and unit tests are pending. Ensure these are tracked for completion.
Would you like me to generate unit test scaffolding for this class or open an issue to track this work?
137-139: Hardcoded locale should be configurable.Line 139 hardcodes
"en"as the claims locale. The TODO on line 137 acknowledges this. Consider using a configuration property similar tokycExchangeAcceptedClaims.♻️ Suggested fix
Add a configuration property:
@Value("${mosip.certify.ida.kyc-exchange.claims-locales:en}") private String[] kycExchangeClaimsLocales;Then use it:
- kycExchangeDto.setClaimsLocales(new String[]{"en"}); + kycExchangeDto.setClaimsLocales(kycExchangeClaimsLocales);
243-246: Improve exception handling formatting and specificity.Multiple issues:
- The catch blocks on line 243 are compressed onto a single line, reducing readability.
- Non-
KycExchangeExceptionerrors are logged but then a genericKycExchangeException()is thrown without context.♻️ Suggested fix
- } catch (KycExchangeException e) { throw e; } catch (Exception e) { + } catch (KycExchangeException e) { + throw e; + } catch (Exception e) { log.error("IDA Kyc-exchange failed with clientId : {}", clientId, e); + throw new KycExchangeException(io.mosip.esignet.api.util.ErrorConstants.DATA_EXCHANGE_FAILED); } - throw new KycExchangeException();
287-293: JWT payload decoding lacks validation.The method decodes the JWT payload without verifying signature or validating the token structure. While this may be acceptable if the JWT comes from a trusted internal service (IDA), consider:
- Adding a check that
parts.length >= 2to avoidArrayIndexOutOfBoundsException- Documenting that signature verification is assumed to be handled elsewhere
♻️ Suggested defensive check
private Map<String, Object> decodeClaimsFromJwt(String jwtToken) throws JsonProcessingException { String[] parts = jwtToken.split("\\."); + if (parts.length < 2) { + throw new IllegalArgumentException("Invalid JWT format: expected at least 2 parts"); + } String payload = new String(urlSafeDecoder.decode(parts[1])); Map<String, Object> claims = objectMapper.readValue(payload, Map.class); - return claims; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
mock-certify-plugin/pom.xmlmosip-identity-certify-plugin/pom.xmlmosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/dto/IdaKycExchangeRequest.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/dto/IdaKycExchangeResponse.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/AuthTransactionHelper.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCITransactionHelper.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/HelperService.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.javapostgres-dataprovider-plugin/pom.xmlsunbird-rc-certify-integration-impl/pom.xml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-31T13:55:27.155Z
Learnt from: Piyush7034
Repo: inji/digital-credential-plugins PR: 123
File: postgres-dataprovider-plugin/src/main/java/io/mosip/certify/postgresdataprovider/integration/service/PostgresDataProviderPlugin.java:50-56
Timestamp: 2025-12-31T13:55:27.155Z
Learning: In the digital-credential-plugins repository, different data provider plugins use different error codes based on their data source: PostgresDataProviderPlugin uses "ERROR_FETCHING_DATA_RECORD_FROM_TABLE" for database operations, while MockIdaDataProviderPlugin and MockCSVDataProviderPlugin use "ERROR_FETCHING_IDENTITY_DATA" for identity service and CSV operations. This distinction is intentional to improve debugging clarity.
Applied to files:
postgres-dataprovider-plugin/pom.xmlmosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
🧬 Code graph analysis (4)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/dto/IdaKycExchangeResponse.java (1)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/dto/IdaKycExchangeRequest.java (1)
Data(14-41)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/AuthTransactionHelper.java (1)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaAuditPluginImpl.java (1)
ConditionalOnProperty(33-133)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java (2)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/AuthTransactionHelper.java (1)
Component(28-80)mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCITransactionHelper.java (1)
Component(15-33)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/dto/IdaKycExchangeRequest.java (1)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/dto/IdaKycExchangeResponse.java (1)
Data(10-14)
🪛 ast-grep (0.40.4)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
[warning] 154-154: Cipher in ECB mode is detected. ECB mode produces the same output for the same input each time which allows an attacker to intercept and replay the data. Further, ECB mode does not provide any integrity checking. See https://find-sec-bugs.github.io/bugs.htm#CIPHER_INTEGRITY.
Context: Cipher cipher = Cipher.getInstance(aesECBTransformation);
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(ecb-cipher-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). (3)
- GitHub Check: build-maven-mock-certify-plugin / maven-build
- GitHub Check: build-maven-mosip-identity-certify-plugin / maven-build
- GitHub Check: build-maven-sunbird-rc-esignet-integration-impl / maven-build
🔇 Additional comments (5)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/HelperService.java (2)
29-29: LGTM!The
@ConditionalOnPropertyannotation update aligns with the PR objective to migrate fromvci-plugintodata-provider-plugin.
48-48: No consumer updates needed—this is a backend-specific JWT format configuration.The
includePayloadflag change fromfalsetotrueis intentional and backend-specific to the IDA integration. The plugin consumers (IdaDataProviderPluginImpl and IdaVCIssuancePluginImpl) do not parse or validate the JWT structure; they only forward the signature string as a header to the backend. This differs from the MockVCIssuancePlugin, which intentionally usesincludePayload(false), confirming this is a deliberate backend-specific configuration rather than a plugin-level change requiring consumer updates.Verify that the IDA backend service can process payload-included JWT signatures as expected.
Likely an incorrect or invalid review comment.
sunbird-rc-certify-integration-impl/pom.xml (1)
11-11: LGTM - Consistent version bump and dependency migration.The changes align with the coordinated update across the repository:
- Version bump to
0.6.0-SNAPSHOT- Migration of
certify-integration-apitoio.inji.certifygroupId with version0.14.0-SNAPSHOTAlso applies to: 55-57
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/AuthTransactionHelper.java (1)
30-30: Annotation update is consistent across all IdaDataProviderPluginImpl components.The
@ConditionalOnPropertychange correctly updates AuthTransactionHelper to usedata-provider-plugin=IdaDataProviderPluginImpl, matching HelperService, VCITransactionHelper, and the main IdaDataProviderPluginImpl component. All related helper classes activate together under the data-provider-plugin configuration, ensuring proper bean initialization order and dependency injection.mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCITransactionHelper.java (1)
15-17: LGTM!The
@ConditionalOnPropertyupdate correctly aligns this helper's activation with the newIdaDataProviderPluginImpldata-provider plugin path. This is consistent with the migration from VCI issuance to data provider plugin architecture.
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
Signed-off-by: Piyush7034 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
@mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:
- Around line 250-254: Update the Javadoc comment in IdaDataProviderPluginImpl
for the method that "Set the verfied and unVerified consented claims to
IdaKycExchangeRequest" by correcting the typo "verfied" to "verified" (and
optionally normalize "unVerified" to "unverified") so the summary and parameter
descriptions accurately read "Set the verified and unverified consented claims
to {@link IdaKycExchangeRequest}" and ensure the @param tags for kycExchangeDto
and idaKycExchangeRequest remain intact.
- Line 219: The log statement in IdaDataProviderPluginImpl logs the entire
idaKycExchangeRequest (including sensitive fields like individualId and
kycToken); remove or replace that full-object log. Update the log.info call to
only emit non-sensitive fields (or a safe correlation id) and either omit or
mask sensitive values (e.g., replace individualId and kycToken with masked
versions), or call a dedicated sanitizer/masking helper (e.g.,
maskSensitiveInfoIn(idaKycExchangeRequest)) before logging; do not serialize or
print the raw idaKycExchangeRequest.
- Around line 288-294: The decodeClaimsFromJwt method lacks validation of the
JWT structure and will throw ArrayIndexOutOfBoundsException when jwtToken is
malformed; update decodeClaimsFromJwt to first check jwtToken is not null/blank,
split on '.' and verify parts.length >= 2 (preferably 3 for a full JWT) before
accessing parts[1]; if the structure is invalid, throw a clear
IllegalArgumentException (or wrap into JsonProcessingException) with a
descriptive message, and ensure any decoding errors from urlSafeDecoder are
caught and rethrown as JsonProcessingException so callers get a consistent,
meaningful error.
- Around line 226-229: The Authorization header is being set to the header name
constant (AUTHORIZATION_HEADER_NAME) instead of a token; inject
AuthTransactionHelper into IdaDataProviderPluginImpl (add a private field
annotated with @Autowired) and replace the header value in the request builder
with the actual token obtained from authTransactionHelper.getAuthToken() (or the
appropriate method) when building the request
(.header(AUTHORIZATION_HEADER_NAME, authTransactionHelper.getAuthToken())).
Ensure null/empty token handling if necessary.
- Line 143: The log line in IdaDataProviderPluginImpl that calls log.info("KYC
exchange DTO built: {}", kycExchangeDto) exposes PII (individualId); remove this
raw-object log or replace it by logging a sanitized version: create or derive a
safe DTO/Map that omits or masks individualId (e.g., replace with "<redacted>"
or hash) and log that instead, or log only non-sensitive fields (e.g., status,
timestamp). Update the logging call to reference the sanitized object/name and
ensure the original kycExchangeDto is not serialized into logs.
- Around line 189-197: doKycExchange currently calls
identityDetails.get(ACCESS_TOKEN_HASH).toString() which can NPE if the key is
absent; first retrieve the value into a local (e.g., tokenHash), check for null
(or use containsKey) and throw a clear exception (IllegalArgumentException or
custom) with a meaningful message if missing, otherwise convert toString() and
pass that to vciTransactionHelper.getOAuthTransaction; update references in this
method (doKycExchange, the tokenHash local, and the call to
vciTransactionHelper.getOAuthTransaction) accordingly so null is never
dereferenced and errors are explicit.
🧹 Nitpick comments (2)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java (2)
46-47: Track the TODO items for code cleanup and unit tests.These TODO comments indicate pending work. Consider creating issues to track these tasks to ensure they don't get forgotten.
Would you like me to open issues to track these tasks?
244-247: Improve code readability and exception context.Line 244 combines multiple statements on one line, reducing readability. Additionally, the
KycExchangeException()on line 247 is thrown without any message or context.♻️ Proposed refactor
- } catch (KycExchangeException e) { throw e; } catch (Exception e) { + } catch (KycExchangeException e) { + throw e; + } catch (Exception e) { log.error("IDA Kyc-exchange failed with clientId : {}", clientId, e); } - throw new KycExchangeException(); + throw new KycExchangeException(io.mosip.esignet.api.util.ErrorConstants.DATA_EXCHANGE_FAILED);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-31T13:55:27.155Z
Learnt from: Piyush7034
Repo: inji/digital-credential-plugins PR: 123
File: postgres-dataprovider-plugin/src/main/java/io/mosip/certify/postgresdataprovider/integration/service/PostgresDataProviderPlugin.java:50-56
Timestamp: 2025-12-31T13:55:27.155Z
Learning: In the digital-credential-plugins repository, different data provider plugins use different error codes based on their data source: PostgresDataProviderPlugin uses "ERROR_FETCHING_DATA_RECORD_FROM_TABLE" for database operations, while MockIdaDataProviderPlugin and MockCSVDataProviderPlugin use "ERROR_FETCHING_IDENTITY_DATA" for identity service and CSV operations. This distinction is intentional to improve debugging clarity.
Applied to files:
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
🪛 ast-grep (0.40.4)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
[warning] 155-155: Cipher in ECB mode is detected. ECB mode produces the same output for the same input each time which allows an attacker to intercept and replay the data. Further, ECB mode does not provide any integrity checking. See https://find-sec-bugs.github.io/bugs.htm#CIPHER_INTEGRITY.
Context: Cipher cipher = Cipher.getInstance(aesECBTransformation);
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(ecb-cipher-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). (4)
- GitHub Check: build-maven-mosip-identity-certify-plugin / maven-build
- GitHub Check: build-maven-mock-certify-plugin / maven-build
- GitHub Check: build-maven-sunbird-rc-esignet-integration-impl / maven-build
- GitHub Check: build-maven-postgres-dataprovider-plugin / maven-build
🔇 Additional comments (5)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java (5)
56-107: LGTM!Configuration properties and dependencies are well-organized. The
Base64.Decoderinstance is thread-safe, making the inline initialization appropriate.
109-130: LGTM with minor observation.The implementation handles exceptions appropriately. Note that
encryptedKycis a slightly misleading name since the value is a JWT that gets decoded (not decrypted) indecodeClaimsFromJwt. Consider renaming tokycJwtfor clarity.
148-152: LGTM!The conditional logic for handling encrypted vs. plaintext individual IDs is clear and correct.
166-183: LGTM!Key retrieval logic is sound with appropriate uniqueness validation. Minor: the condition
!currentKeyAliases.isEmpty() && currentKeyAliases.size() == 1could be simplified tocurrentKeyAliases.size() == 1, but this is a trivial optimization.
273-286: LGTM with observation.The
@NotNullannotation on a private method has no runtime effect without explicit validation framework integration. The comment explains the intent well, so the annotation serves as documentation. Logic is correct.
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Piyush7034 <[email protected]>
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:
- Line 143: The log statement in IdaDataProviderPluginImpl is exposing sensitive
data; stop logging raw kycExchangeDto and instead remove or mask PII/security
fields before logging: replace the direct log.info("KYC exchange DTO built: {}",
kycExchangeDto) with code that either logs a sanitized DTO or constructs a safe
message that omits or masks kycExchangeDto.individualId and
kycExchangeDto.kycToken (and any other sensitive fields), ensuring the sanitized
object or string is what's passed to log.info in the same method where the
original statement appears.
- Around line 154-164: The decryptIndividualId method currently uses the
insecure aesECBTransformation (AES/ECB/PKCS5Padding); replace it with an AEAD
mode (e.g., AES/GCM/NoPadding) and update decryption to expect a nonce/IV
prepended to the ciphertext: decode the base64 payload, split a 12-byte IV and
the remainder ciphertext, construct a GCMParameterSpec(128, iv), get
Cipher.getInstance("AES/GCM/NoPadding"), init with DECRYPT_MODE and
getSecretKeyFromHSM() plus the GCMParameterSpec, then doFinal on the ciphertext.
Also update the default configuration value for
mosip.certify.cache.security.algorithm-name to AES/GCM/NoPadding (or document if
ECB is required), and preserve throwing AES_CIPHER_FAILED on errors as before.
- Around line 226-228: Replace the deprecated MediaType constant: in
IdaDataProviderPluginImpl where the request is built (the chain calling
.contentType(...), .header(SIGNATURE_HEADER_NAME,
helperService.getRequestSignature(requestBody)),
.header(AUTHORIZATION_HEADER_NAME, AUTHORIZATION_HEADER_NAME)), change
MediaType.APPLICATION_JSON_UTF8 to MediaType.APPLICATION_JSON; leave the
Authorization header usage as-is since the literal AUTHORIZATION_HEADER_NAME
value is intentional for the IDA integration.
🧹 Nitpick comments (6)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java (6)
80-81: UnusedsecretKeyfield.The
secretKeyfield is injected but never referenced in this class. This appears to be dead code that should be removed.Proposed fix
- @Value("${mosip.certify.authenticator.ida.secret-key}") - private String secretKey; -
107-107: Consider makingurlSafeDecoderstatic final.
Base64.getUrlDecoder()returns a thread-safe singleton decoder, so this can be a static constant to avoid creating an instance field.Proposed fix
- private Base64.Decoder urlSafeDecoder = Base64.getUrlDecoder(); + private static final Base64.Decoder URL_SAFE_DECODER = Base64.getUrlDecoder();
121-127: Use parameterized logging instead of string concatenation.String concatenation in logging statements is evaluated regardless of log level, causing unnecessary overhead. Use parameterized logging with
{}placeholders.Proposed fix
catch (JSONException | JsonProcessingException e) { - log.error("Error occurred during json processing: " + e.getMessage()); + log.error("Error occurred during json processing: {}", e.getMessage()); throw new DataProviderExchangeException("JSON_PARSING_FAILED", e.getMessage()); } catch (Exception e) { - log.error("ERROR_FETCHING_KYC_DATA. " + e.getMessage()); + log.error("ERROR_FETCHING_KYC_DATA: {}", e.getMessage(), e); throw new DataProviderExchangeException("ERROR_FETCHING_KYC_DATA", e.getMessage()); }
185-187: Remove unnecessary semicolon.There's an extra semicolon after the method's closing brace on line 187.
Proposed fix
private byte[] b64Decode(String value) { return urlSafeDecoder.decode(value); - }; + }
244-247: Improve exception handling readability and provide error details.Line 244 has multiple statements condensed, making it hard to read. Additionally, line 247 throws
KycExchangeExceptionwithout any error details which will make debugging difficult.Proposed fix
log.error("Error response received from IDA (Kyc-exchange) with status : {}", responseEntity.getStatusCode()); - } catch (KycExchangeException e) { throw e; } catch (Exception e) { + } catch (KycExchangeException e) { + throw e; + } catch (Exception e) { log.error("IDA Kyc-exchange failed with clientId : {}", clientId, e); } - throw new KycExchangeException(); + throw new KycExchangeException(io.mosip.esignet.api.util.ErrorConstants.DATA_EXCHANGE_FAILED);
273-286:@NotNullannotation has no effect on private method.The
@NotNullannotation from Jakarta Validation is only enforced at public API boundaries with validation enabled. On a private method, it serves only as documentation. Additionally, the intermediate conversion throughobjectMapper.convertValueis unnecessary—you can return the map directly.Proposed fix
- @NotNull // This is added to not return null either return un verified claims map or empty map - private Map<String, Object> getUnVerifiedConsentedClaims(Map<String, JsonNode> acceptedClaimDetails) { - Map<String, JsonNode> unVerifiedConsentedClaims = new HashMap<>(); + private Map<String, Object> getUnVerifiedConsentedClaims(Map<String, JsonNode> acceptedClaimDetails) { + Map<String, Object> unVerifiedConsentedClaims = new HashMap<>(); if(!CollectionUtils.isEmpty(acceptedClaimDetails)) { for(Map.Entry<String, JsonNode> entry : acceptedClaimDetails.entrySet()) { String key = entry.getKey(); JsonNode value = entry.getValue(); if(!key.equals(VERIFIED_CLAIMS)){ - unVerifiedConsentedClaims.put(key,value); + unVerifiedConsentedClaims.put(key, objectMapper.convertValue(value, Object.class)); } } } - return objectMapper.convertValue(unVerifiedConsentedClaims, new TypeReference<>() {}); + return unVerifiedConsentedClaims; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-31T13:55:27.155Z
Learnt from: Piyush7034
Repo: inji/digital-credential-plugins PR: 123
File: postgres-dataprovider-plugin/src/main/java/io/mosip/certify/postgresdataprovider/integration/service/PostgresDataProviderPlugin.java:50-56
Timestamp: 2025-12-31T13:55:27.155Z
Learning: In the digital-credential-plugins repository, different data provider plugins use different error codes based on their data source: PostgresDataProviderPlugin uses "ERROR_FETCHING_DATA_RECORD_FROM_TABLE" for database operations, while MockIdaDataProviderPlugin and MockCSVDataProviderPlugin use "ERROR_FETCHING_IDENTITY_DATA" for identity service and CSV operations. This distinction is intentional to improve debugging clarity.
Applied to files:
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
🧬 Code graph analysis (1)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java (3)
mock-certify-plugin/src/main/java/io/mosip/certify/util/KeyPairAndCertificateExtractor.java (1)
Slf4j(15-47)mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCITransactionHelper.java (1)
Component(15-33)mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/AuthTransactionHelper.java (1)
Component(28-80)
🪛 ast-grep (0.40.4)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
[warning] 155-155: Cipher in ECB mode is detected. ECB mode produces the same output for the same input each time which allows an attacker to intercept and replay the data. Further, ECB mode does not provide any integrity checking. See https://find-sec-bugs.github.io/bugs.htm#CIPHER_INTEGRITY.
Context: Cipher cipher = Cipher.getInstance(aesECBTransformation);
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(ecb-cipher-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). (4)
- GitHub Check: build-maven-mosip-identity-certify-plugin / maven-build
- GitHub Check: build-maven-mock-certify-plugin / maven-build
- GitHub Check: build-maven-postgres-dataprovider-plugin / maven-build
- GitHub Check: build-maven-sunbird-rc-esignet-integration-impl / maven-build
🔇 Additional comments (4)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java (4)
46-47: TODO comments noted.Acknowledged. The cleanup and unit tests are tracked. Consider creating issues to track these tasks.
250-266: LGTM!The claims handling logic correctly separates verified and unverified claims with appropriate null checks.
189-197: Potential NullPointerException ifACCESS_TOKEN_HASHis missing.If
identityDetailsdoesn't contain theACCESS_TOKEN_HASHkey,get()returnsnulland callingtoString()will throw NPE. Add a null check or useObjects.requireNonNullwith a descriptive message.Proposed fix
public KycExchangeResult doKycExchange(Map<String, Object> identityDetails) throws Exception { + Object accessTokenHash = identityDetails.get(ACCESS_TOKEN_HASH); + if (accessTokenHash == null) { + throw new DataProviderExchangeException("MISSING_ACCESS_TOKEN_HASH", "Access token hash is required"); + } OIDCTransaction transaction = vciTransactionHelper - .getOAuthTransaction(identityDetails.get(ACCESS_TOKEN_HASH).toString()); + .getOAuthTransaction(accessTokenHash.toString()); KycExchangeDto kycExchangeDto = buildKycExchangeDto(transaction);⛔ Skipped due to learnings
Learnt from: Piyush7034 Repo: inji/digital-credential-plugins PR: 130 File: mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:189-197 Timestamp: 2026-01-12T17:24:39.707Z Learning: In the mosip-identity-certify-plugin, the `ACCESS_TOKEN_HASH` key is guaranteed to be present in the `identityDetails` map passed to `IdaDataProviderPluginImpl.doKycExchange()` because it is validated/included by the main service before this method is called. No null check is required for this key.Learnt from: Piyush7034 Repo: inji/digital-credential-plugins PR: 130 File: mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:226-229 Timestamp: 2026-01-12T17:25:25.933Z Learning: In the mosip-identity-certify-plugin, when building KYC exchange requests in `IdaDataProviderPluginImpl.kycExchange()`, the Authorization header is intentionally set to the literal string "Authorization" (value of AUTHORIZATION_HEADER_NAME constant) rather than an actual token. This is the correct implementation verified to work with the IDA service.
288-294: Add validation for JWT structure before parsing.Accessing
parts[1]without checking the array length can throwArrayIndexOutOfBoundsExceptionif the input is malformed. Also, consider whether JWT signature verification is required for this use case.Proposed fix
private Map<String, Object> decodeClaimsFromJwt(String jwtToken) throws JsonProcessingException { String[] parts = jwtToken.split("\\."); + if (parts.length < 2) { + throw new IllegalArgumentException("Invalid JWT format: expected at least 2 parts"); + } String payload = new String(urlSafeDecoder.decode(parts[1])); Map<String, Object> claims = objectMapper.readValue(payload, Map.class); return claims; }⛔ Skipped due to learnings
Learnt from: Piyush7034 Repo: inji/digital-credential-plugins PR: 130 File: mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:189-197 Timestamp: 2026-01-12T17:24:39.707Z Learning: In the mosip-identity-certify-plugin, the `ACCESS_TOKEN_HASH` key is guaranteed to be present in the `identityDetails` map passed to `IdaDataProviderPluginImpl.doKycExchange()` because it is validated/included by the main service before this method is called. No null check is required for this key.
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
Signed-off-by: Piyush7034 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/TransactionHelper.java:
- Around line 25-31: getOAuthTransaction currently returns null on a cache miss
which leads to NPEs downstream; modify OIDCTransaction
getOAuthTransaction(String accessTokenHash) so that after calling
cacheManager.getCache(userinfoCache).get(accessTokenHash, OIDCTransaction.class)
you check the returned value and if it is null throw a descriptive exception
(e.g., "cache_miss" or a custom checked exception) instead of returning null;
ensure the exception is thrown from getOAuthTransaction so callers like
IdaDataProviderPluginImpl.doKycExchange() always receive either a non-null
OIDCTransaction or a clear error.
In
@mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCIAuthTransactionHelper.java:
- Around line 54-74: Add a missing AUTH_TOKEN_CACHE_KEY constant and prevent
caching null tokens: declare a static final String AUTH_TOKEN_CACHE_KEY
(matching the value used in the @Cacheable expression) in the class so
@Cacheable(value = AUTH_TOKEN_CACHE, key = "#root.target.AUTH_TOKEN_CACHE_KEY")
resolves, and update getAuthToken() to check the result of
responseEntity.getHeaders().getFirst("authorization") — if it is null or empty
throw a clear exception (or return a non-cached failure) instead of returning
null so a null token is not stored in the cache.
In
@mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:
- Around line 163-172: In getKeyAlias, guard against dbHelper.getKeyAliases(...)
returning a map without KeymanagerConstant.CURRENTKEYALIAS: check that
keyAliasMap is non-null and that
keyAliasMap.get(KeymanagerConstant.CURRENTKEYALIAS) yields a non-null, non-empty
List before calling isEmpty() or size(); if null or size()!=1, log the
count/state (including "null" case) and throw the existing NO_UNIQUE_ALIAS
exception as currently done; update references in getKeyAlias to use the
validated currentKeyAliases variable and keep the same exception flow.
- Around line 235-244: In decodeClaimsFromJwt, replace the single-argument
DataProviderExchangeException thrown for an invalid JWT (currently new
DataProviderExchangeException("Invalid KYC Exchange response.")) with the
two-argument constructor including an explicit error code, e.g., new
DataProviderExchangeException("INVALID_KYC_RESPONSE", "Invalid KYC Exchange
response."), so the exception use matches the other occurrences in this class.
🧹 Nitpick comments (6)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCITransactionHelper.java (1)
15-18: Consider enhancing the deprecation annotation with metadata and Javadoc.The deprecation is appropriate given the migration to the data-provider plugin. For better maintainability, consider adding:
sinceandforRemovalattributes to the annotation- A Javadoc
@deprecatedtag indicating the replacement📝 Suggested improvement
+/** + * @deprecated since 0.6.0, use {@link TransactionHelper} or {@link VCIAuthTransactionHelper} instead. + * This class will be removed in a future release. + */ @Component @ConditionalOnProperty(value = "mosip.certify.integration.vci-plugin", havingValue = "IdaVCIssuancePluginImpl") -@Deprecated +@Deprecated(since = "0.6.0", forRemoval = true) public class VCITransactionHelper {mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/VCIHelperService.java (2)
51-51: Inconsistent use ofb64Encode- local method exists but external is called.Line 51 calls
HelperService.b64Encode(request), but this class defines its ownb64Encodemethod at lines 63-65. Consider using the local method for consistency, or remove the duplicate local method if the external one is intentionally preferred.♻️ Proposed fix - use local method
- jwtSignatureRequestDto.setDataToSign(HelperService.b64Encode(request)); + jwtSignatureRequestDto.setDataToSign(b64Encode(request));
73-77: Empty catch block silently swallowsMissingResourceException.The empty catch block makes debugging difficult when language code conversion fails. Consider logging the exception at debug level.
♻️ Proposed fix
try { return StringUtils.isEmpty(langCode) ? null : new Locale(langCode).getISO3Language(); - } catch (MissingResourceException ex) {} + } catch (MissingResourceException ex) { + log.debug("Could not convert language code '{}' to ISO3: {}", langCode, ex.getMessage()); + } return null;mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java (2)
41-42: Address TODO comments before merging.The TODO comments indicate pending work:
- Clean up code
- Write unit tests
Consider tracking these in a separate issue if they cannot be addressed in this PR.
Would you like me to help generate unit tests for this class or create an issue to track this work?
110-112: Use parameterized logging instead of string concatenation.String concatenation in log statements (
"Error occurred during json processing: " + e.getMessage()) incurs allocation cost even when the log level is disabled.♻️ Proposed fix
- log.error("Error occurred during json processing: " + e.getMessage()); + log.error("Error occurred during json processing: {}", e.getMessage());- log.error("ERROR_FETCHING_KYC_DATA. " + e.getMessage()); + log.error("ERROR_FETCHING_KYC_DATA: {}", e.getMessage());mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaVCIssuancePluginImpl.java (1)
115-115: UseVCIHelperService.getUTCDateTime()instead ofHelperService.getUTCDateTime()for consistency.The class is autowired with
VCIHelperServiceand both methods have identical implementations. UsingVCIHelperService.getUTCDateTime()aligns with the injected dependency and the service's activation profile.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/TransactionHelper.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCIAuthTransactionHelper.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCITransactionHelper.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaVCIssuancePluginImpl.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/VCIHelperService.javamosip-identity-certify-plugin/src/test/java/io/mosip/certify/mosipid/integration/service/IdaVCIssuancePluginImplTest.java
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-12T17:25:25.933Z
Learnt from: Piyush7034
Repo: inji/digital-credential-plugins PR: 130
File: mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:226-229
Timestamp: 2026-01-12T17:25:25.933Z
Learning: In the mosip-identity-certify-plugin, when building KYC exchange requests in `IdaDataProviderPluginImpl.kycExchange()`, the Authorization header is intentionally set to the literal string "Authorization" (value of AUTHORIZATION_HEADER_NAME constant) rather than an actual token. This is the correct implementation verified to work with the IDA service.
Applied to files:
mosip-identity-certify-plugin/src/test/java/io/mosip/certify/mosipid/integration/service/IdaVCIssuancePluginImplTest.java
📚 Learning: 2026-01-12T17:24:39.707Z
Learnt from: Piyush7034
Repo: inji/digital-credential-plugins PR: 130
File: mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:189-197
Timestamp: 2026-01-12T17:24:39.707Z
Learning: In the mosip-identity-certify-plugin, the `ACCESS_TOKEN_HASH` key is guaranteed to be present in the `identityDetails` map passed to `IdaDataProviderPluginImpl.doKycExchange()` because it is validated/included by the main service before this method is called. No null check is required for this key.
Applied to files:
mosip-identity-certify-plugin/src/test/java/io/mosip/certify/mosipid/integration/service/IdaVCIssuancePluginImplTest.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/TransactionHelper.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaVCIssuancePluginImpl.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCIAuthTransactionHelper.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
📚 Learning: 2026-01-12T17:25:25.933Z
Learnt from: Piyush7034
Repo: inji/digital-credential-plugins PR: 130
File: mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:226-229
Timestamp: 2026-01-12T17:25:25.933Z
Learning: In IdaDataProviderPluginImpl.kycExchange() within mosip-identity-certify-plugin, the Authorization header should be set to the literal string "Authorization" (i.e., the value of AUTHORIZATION_HEADER_NAME) rather than a real token. This exact approach has been verified to work with the IDA service. Do not replace with a bearer token or dynamic value in this path.
Applied to files:
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCITransactionHelper.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/TransactionHelper.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaVCIssuancePluginImpl.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCIAuthTransactionHelper.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.javamosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/VCIHelperService.java
📚 Learning: 2025-12-31T13:55:27.155Z
Learnt from: Piyush7034
Repo: inji/digital-credential-plugins PR: 123
File: postgres-dataprovider-plugin/src/main/java/io/mosip/certify/postgresdataprovider/integration/service/PostgresDataProviderPlugin.java:50-56
Timestamp: 2025-12-31T13:55:27.155Z
Learning: In the digital-credential-plugins repository, different data provider plugins use different error codes based on their data source: PostgresDataProviderPlugin uses "ERROR_FETCHING_DATA_RECORD_FROM_TABLE" for database operations, while MockIdaDataProviderPlugin and MockCSVDataProviderPlugin use "ERROR_FETCHING_IDENTITY_DATA" for identity service and CSV operations. This distinction is intentional to improve debugging clarity.
Applied to files:
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
🧬 Code graph analysis (1)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCIAuthTransactionHelper.java (2)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCITransactionHelper.java (1)
Component(15-34)mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaVCIssuancePluginImpl.java (1)
Component(48-220)
🪛 ast-grep (0.40.4)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
[warning] 144-144: Cipher in ECB mode is detected. ECB mode produces the same output for the same input each time which allows an attacker to intercept and replay the data. Further, ECB mode does not provide any integrity checking. See https://find-sec-bugs.github.io/bugs.htm#CIPHER_INTEGRITY.
Context: Cipher cipher = Cipher.getInstance(aesECBTransformation);
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(ecb-cipher-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). (3)
- GitHub Check: build-maven-mosip-identity-certify-plugin / maven-build
- GitHub Check: build-maven-mock-certify-plugin / maven-build
- GitHub Check: build-maven-sunbird-rc-esignet-integration-impl / maven-build
🔇 Additional comments (6)
mosip-identity-certify-plugin/src/test/java/io/mosip/certify/mosipid/integration/service/IdaVCIssuancePluginImplTest.java (1)
60-61: LGTM!The mock type change from
HelperServicetoVCIHelperServicecorrectly aligns with the production code change inIdaVCIssuancePluginImpl, ensuring test-production consistency.mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaVCIssuancePluginImpl.java (2)
51-52: LGTM on deprecation annotation.Marking
IdaVCIssuancePluginImplas deprecated is appropriate since the newIdaDataProviderPluginImplprovides the updated implementation path.
67-68: LGTM on the field type change.Updating the autowired field from
HelperServicetoVCIHelperServiceis consistent with the new helper service structure for VCI plugin flows.mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCIAuthTransactionHelper.java (1)
45-52: Verify secure handling of secret key configuration.The
secretKeyis injected from configuration propertymosip.certify.authenticator.ida.secret-key. Ensure this value is stored securely (e.g., in a secrets manager or encrypted configuration) and not in plain text in configuration files.mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java (2)
212-214: Authorization header correctly set to literal string.The Authorization header is set to the literal string
"Authorization"(the value ofAUTHORIZATION_HEADER_NAME). Based on learnings, this is the correct and verified approach for the IDA service integration.
143-152: Document the cache-specific ECB requirement in code comments.ECB cipher mode is cryptographically weak (CWE-327) and this usage is flagged across the codebase in all cache encryption implementations (IdaDataProviderPluginImpl, IdaVCIssuancePluginImpl, MockVCIssuancePlugin, MDocMockVCIssuancePlugin). While the algorithm is externally configured and documented in README files, add an inline code comment explaining that ECB is required for backward compatibility with existing cache encryption and document any constraints or limitations of this approach for developers maintaining this code.
...tify-plugin/src/main/java/io/mosip/certify/mosipid/integration/helper/TransactionHelper.java
Show resolved
Hide resolved
...ugin/src/main/java/io/mosip/certify/mosipid/integration/helper/VCIAuthTransactionHelper.java
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
...in/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java
Show resolved
Hide resolved
Signed-off-by: Piyush7034 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/VCIHelperService.java:
- Around line 45-55: In getRequestSignature, avoid NPE by checking the result of
signatureService.jwtSign(...) (the JWTSignatureResponseDto named responseDto)
before calling responseDto.getJwtSignedData() — if null, log an error and either
throw a descriptive exception or return null/empty per existing error-handling
convention; also replace the call to HelperService.b64Encode(request) with the
local b64Encode(request) method defined in this class so the local
implementation is used.
🧹 Nitpick comments (3)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/VCIHelperService.java (3)
16-24: Unused imports detected.
StringUtils(line 16) andCollectors(line 24) are imported but not used in this file. Consider removing them to keep the imports clean.♻️ Suggested fix
-import org.springframework.util.StringUtils;-import java.util.stream.Collectors;
27-31: Add deprecation documentation.The class is marked
@Deprecatedbut lacks a Javadoc explaining why it's deprecated and what replacement should be used. This helps consumers understand the migration path.📝 Suggested documentation
@Service @Slf4j @ConditionalOnProperty(value = "mosip.certify.integration.vci-plugin", havingValue = "IdaVCIssuancePluginImpl") @Deprecated +/** + * @deprecated This service is deprecated. Use {@link HelperService} with the data-provider-plugin instead. + */ public class VCIHelperService {
63-65: Unused method - potential dead code.The
b64Encodemethod is defined but never used within this class. ThegetRequestSignaturemethod callsHelperService.b64Encode()instead. Either remove this method or use it consistently within the class.♻️ Option 1: Remove unused method
- protected static String b64Encode(String value) { - return urlSafeEncoder.encodeToString(value.getBytes(StandardCharsets.UTF_8)); - }Also remove the unused
urlSafeEncoderfield and static initializer if this method is removed.♻️ Option 2: Use the local method
- jwtSignatureRequestDto.setDataToSign(HelperService.b64Encode(request)); + jwtSignatureRequestDto.setDataToSign(b64Encode(request));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/VCIHelperService.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-12T17:25:25.933Z
Learnt from: Piyush7034
Repo: inji/digital-credential-plugins PR: 130
File: mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:226-229
Timestamp: 2026-01-12T17:25:25.933Z
Learning: In IdaDataProviderPluginImpl.kycExchange() within mosip-identity-certify-plugin, the Authorization header should be set to the literal string "Authorization" (i.e., the value of AUTHORIZATION_HEADER_NAME) rather than a real token. This exact approach has been verified to work with the IDA service. Do not replace with a bearer token or dynamic value in this path.
Applied to files:
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/VCIHelperService.java
📚 Learning: 2026-01-12T17:24:39.707Z
Learnt from: Piyush7034
Repo: inji/digital-credential-plugins PR: 130
File: mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/IdaDataProviderPluginImpl.java:189-197
Timestamp: 2026-01-12T17:24:39.707Z
Learning: In the mosip-identity-certify-plugin, the `ACCESS_TOKEN_HASH` key is guaranteed to be present in the `identityDetails` map passed to `IdaDataProviderPluginImpl.doKycExchange()` because it is validated/included by the main service before this method is called. No null check is required for this key.
Applied to files:
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/VCIHelperService.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). (3)
- GitHub Check: build-maven-mosip-identity-certify-plugin / maven-build
- GitHub Check: build-maven-mock-certify-plugin / maven-build
- GitHub Check: build-maven-sunbird-rc-esignet-integration-impl / maven-build
🔇 Additional comments (1)
mosip-identity-certify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/VCIHelperService.java (1)
33-43: LGTM on constants and UTC helper method.The constants, static initializer, and
getUTCDateTime()method are correctly implemented. The UTC datetime formatting usingZonedDateTimewith explicitZoneOffset.UTCis the proper approach.Also applies to: 57-61
...tify-plugin/src/main/java/io/mosip/certify/mosipid/integration/service/VCIHelperService.java
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
Chores
New Features
Bug Fixes / Behavior
Deprecated
✏️ Tip: You can customize this high-level summary in your review settings.