Extracted public key data in getPublicKey API [MOSIP-22993] #1284
Conversation
Signed-off-by: GOKULRAJ136 <[email protected]>
WalkthroughRefactors public key response parsing in SyncConfigDetailsServiceImpl from TypeReference-based deserialization to manual JsonNode parsing. Extracts certificate, issuedAt, and expiryAt fields, constructs X.509 certificate, computes PublicKey, converts to PEM format with 64-character line wrapping, and returns enhanced PublicKeyResponse with additional metadata. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service
participant JsonParser as JSON Parser
participant CertFactory as Certificate Factory
participant PEMConverter as PEM Converter
rect rgb(200, 220, 255)
Note over Service: Old Flow (TypeReference)
Client->>Service: getPublicKey()
Service->>JsonParser: deserialize(response, TypeReference)
JsonParser-->>Service: PublicKeyResponse
Service-->>Client: return PublicKeyResponse
end
rect rgb(220, 240, 200)
Note over Service: New Flow (Manual Parsing)
Client->>Service: getPublicKey()
Service->>JsonParser: parse as JsonNode
JsonParser-->>Service: JsonNode tree
Service->>Service: extract certificate, issuedAt, expiryAt
Service->>CertFactory: buildX509Certificate()
CertFactory-->>Service: X509Certificate
Service->>Service: computePublicKey()
Service->>PEMConverter: convertToPEM(publicKey)
PEMConverter-->>Service: PEM formatted string (64-char wrapped)
Service->>Service: buildPublicKeyResponse(enriched fields)
Service-->>Client: return enhanced PublicKeyResponse
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
...ervice/src/main/java/io/mosip/kernel/syncdata/service/impl/SyncConfigDetailsServiceImpl.java
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
admin/kernel-syncdata-service/src/main/java/io/mosip/kernel/syncdata/service/impl/SyncConfigDetailsServiceImpl.java(6 hunks)
🔇 Additional comments (1)
admin/kernel-syncdata-service/src/main/java/io/mosip/kernel/syncdata/service/impl/SyncConfigDetailsServiceImpl.java (1)
68-72: PEM formatting helper and constants look correctThe
BEGIN_KEY/END_KEYconstants combined withconvertToPEM(PublicKey)produce a standard PUBLIC KEY PEM with 64‑character line wrapping; the implementation is straightforward and appropriate.Also applies to: 373-383
| try { | ||
| publicKeyResponseMapped = objectMapper.readValue(publicKeyResponseEntity.getBody(), | ||
| new TypeReference<ResponseWrapper<PublicKeyResponse<String>>>() { | ||
| }); | ||
|
|
||
| publicKeyResponseMapped.getResponse().setProfile(environment.getActiveProfiles()[0]); | ||
| LOGGER.debug("Public key fetched successfully for applicationId: {}", applicationId); | ||
| return publicKeyResponseMapped.getResponse(); | ||
| } catch (IOException | NullPointerException e) { | ||
| JsonNode root = objectMapper.readTree(publicKeyResponseEntity.getBody()); | ||
| String certificatePem = root.path("response").path("certificate").asText(); | ||
| String issuedAt = root.path("response").path("issuedAt").asText(); | ||
| String expiryAt = root.path("response").path("expiryAt").asText(); | ||
|
|
||
| CertificateFactory certificateFactory = CertificateFactory.getInstance("X.509"); | ||
| ByteArrayInputStream certStream = new ByteArrayInputStream(certificatePem.getBytes(StandardCharsets.UTF_8)); | ||
| X509Certificate certificate = (X509Certificate) certificateFactory.generateCertificate(certStream); | ||
| PublicKey publicKey = certificate.getPublicKey(); | ||
|
|
||
| String publicKeyPEM = convertToPEM(publicKey); | ||
|
|
||
| OffsetDateTime issued = OffsetDateTime.parse(issuedAt); | ||
| OffsetDateTime expiry = OffsetDateTime.parse(expiryAt); | ||
|
|
||
| PublicKeyResponse<String> response = new PublicKeyResponse<>(); | ||
| response.setPublicKey(publicKeyPEM); | ||
| response.setLastSyncTime(DateUtils2.getUTCCurrentDateTimeString()); | ||
| response.setIssuedAt(issued.toLocalDateTime()); | ||
| response.setExpiryAt(expiry.toLocalDateTime()); | ||
| response.setProfile(environment.getActiveProfiles()[0]); | ||
|
|
||
| return response; | ||
|
|
||
| } catch (Exception e) { |
There was a problem hiding this comment.
🧩 Analysis chain
Handle missing response fields and active profiles more defensively
The new parsing flow is directionally good, but a few robustness points are worth tightening:
environment.getActiveProfiles()[0](Line 266) will throwArrayIndexOutOfBoundsExceptionif no active profiles are set (which is valid in Spring). Consider a safe fallback, e.g. checking length and defaulting to"default"or a configured value.certificate,issuedAt, andexpiryAtare taken viaasText()(Lines 247–249), so missing/blank fields only surface later as generic parse errors (certificate factory orOffsetDateTime.parse). It would be clearer to explicitly validate these fields (null/empty/format) and throw a more specific exception (e.g.SyncInvalidArgumentExceptionwith details) before attempting parsing.- In the catch block (Lines 270–274) you log only
e.getMessage(). UsingLOGGER.error("Failed to parse public key response", e);would retain the stack trace and make production debugging easier. - Behavioural change:
lastSyncTimeandprofileare now derived locally (DateUtils2.getUTCCurrentDateTimeString()and environment profile) instead of being taken from the upstream response. Please double‑check that this changed semantics is acceptable for existing consumers ofgetPublicKey.
Please verify these changes against existing registration‑client flows (especially ones relying on profile and lastSyncTime) and adjust the error handling as needed.
I need to verify the specific concerns in this review comment by examining the actual code. Let me gather detailed information from the repository.
---
🏁 Script executed:
```shell
#!/bin/bash
# Retrieve the full method implementation around lines 245-270
fd -e java "SyncConfigDetailsServiceImpl" --exec cat {} \; | head -400 | tail -160
Length of output: 7165
🏁 Script executed:
#!/bin/bash
# Search for getActiveProfiles usage in the codebase to understand safety patterns
rg -nC5 "getActiveProfiles" --type=javaLength of output: 5302
🏁 Script executed:
#!/bin/bash
# Look for the convertToPEM method and BEGIN_KEY/END_KEY constants
fd -e java "SyncConfigDetailsServiceImpl" --exec cat {} \; | grep -A 20 "convertToPEM\|BEGIN_KEY"Length of output: 2077
🏁 Script executed:
#!/bin/bash
# Search for calls to getPublicKey to understand downstream consumers
rg -nP '\bgetPublicKey\s*\(' --type=java -C3Length of output: 43012
Let me verify one more critical aspect—whether the behavioral change for profile and lastSyncTime represents a breaking change:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if profile was previously populated from response or if this is a new addition
rg -nC8 "setProfile|getProfile" --type=java admin/kernel-syncdata-service/src/main/java/io/mosip/kernel/syncdata/
Length of output: 2924
🏁 Script executed:
#!/bin/bash
# Inspect full getPublicKey method to see what was there before (context lines)
fd -e java "SyncConfigDetailsServiceImpl" --exec cat {} \; | grep -B50 "try {" | grep -A50 "getPublicKey"Length of output: 6990
🏁 Script executed:
#!/bin/bash
# Check the test setup to confirm if profile was being set from response before
cat admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncConfigDetailsServiceImplTest.java | grep -A 30 "public void getPublicKey"Length of output: 4388
Add defensive null-check for getActiveProfiles() and improve error logging
Your implementation correctly extracts certificate details and derives the public key, but two issues need attention:
-
Line 266:
environment.getActiveProfiles()[0]lacks safety check. If no active profiles are set (valid in Spring), this throwsArrayIndexOutOfBoundsException. Other parts of the codebase usegetActiveProfiles().length == 0 ? getDefaultProfiles() : getActiveProfiles()(seeadmin-service/AuditUtil.java:201). Apply the same pattern here or provide a fallback (e.g.,"default"). -
Line 271: Logging discards stack trace. Change
LOGGER.error("Failed to parse public key response: {}", e.getMessage())toLOGGER.error("Failed to parse public key response", e)to preserve the full stack trace for production debugging. -
lastSyncTime and profile derivation: These are now set locally instead of from the response. Note that
SyncDataControllerline 136 overwriteslastSyncTimeanyway, so the service-level value is not used; however, verify that clients have no other dependencies on theprofilefield being immutable or response-sourced.
🤖 Prompt for AI Agents
In
admin/kernel-syncdata-service/src/main/java/io/mosip/kernel/syncdata/service/impl/SyncConfigDetailsServiceImpl.java
around lines 245 to 270, add a defensive check for
environment.getActiveProfiles() before indexing [0] (use the pattern used in
admin-service/AuditUtil.java:201: use getActiveProfiles().length == 0 ?
getDefaultProfiles() : getActiveProfiles() or fall back to "default") and assign
profile from that safe value; change the error log inside the catch to
LOGGER.error("Failed to parse public key response", e) so the full stacktrace is
logged; also confirm whether lastSyncTime/profile should remain set here or left
for SyncDataController (update or document accordingly).
2 participants
Summary by CodeRabbit