[ES-2790] Compute Public Key Hash & PATCH Client Management API #1618
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Md-Humair-KK <[email protected]>
Signed-off-by: Md-Humair-KK <[email protected]>
Signed-off-by: Md-Humair-KK <[email protected]>
Signed-off-by: Md-Humair-KK <[email protected]>
Signed-off-by: Md-Humair-KK <[email protected]>
Signed-off-by: Md-Humair-KK <[email protected]>
Signed-off-by: Md-Humair-KK <[email protected]>
WalkthroughAdds support for an encryption public key (encPublicKey) and its hash across model, persistence, API, service logic, PATCH endpoint, docs, migration scripts, tests, and Postman collections to allow partial updates and management of client encryption keys. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as ClientManagementController
participant Service as ClientManagementService
participant Util as IdentityProviderUtil
participant Repo as ClientDetailRepository
participant DB as Database
Client->>Controller: PATCH /client-mgmt/oidc-client/{clientId} (ClientDetailPatchRequest)
Controller->>Service: patchClient(clientId, patchRequest)
Service->>Repo: findByClientId(clientId)
Repo->>DB: SELECT client_detail
DB-->>Repo: ClientDetail
Repo-->>Service: ClientDetail
alt encPublicKey provided (non-null)
Service->>Util: computePublicKeyHash(encPublicKey)
Util-->>Service: encPublicKeyHash
Service->>Service: set encPublicKey & encPublicKeyHash
else encPublicKey explicitly null
Service->>Service: clear encPublicKey & encPublicKeyHash
end
Service->>Service: apply other partial updates and update timestamp
Service->>Repo: save(updated ClientDetail)
Repo->>DB: UPDATE client_detail
DB-->>Repo: success
Repo-->>Service: saved ClientDetail
Service-->>Controller: ClientDetailResponse
Controller-->>Client: ResponseWrapper(ClientDetailResponse)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. 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: 4
🤖 Fix all issues with AI agents
In
`@client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java`:
- Around line 328-412: The patch loses the original client name when
objectMapper.readValue fails: capture the raw existingName before parsing in
buildClient, and if parsing throws, populate existingNameMap with a single entry
mapping Constants.NONE_LANG_KEY to the original existingName (if non-null)
instead of leaving it empty; then proceed to derive clientName and
clientNameLangMap and call getClientNameLanguageMapAsJsonString as before so the
original name is preserved when only clientNameLangMap is provided.
In `@db_scripts/mosip_esignet/ddl/esignet-client_detail.sql`:
- Around line 28-29: The enc_public_key column is defined as varchar(1024) here
but other schemas use varchar(2048), which risks truncation; update the DDL
(column enc_public_key) and any related upgrade/migration scripts and test
schema definitions to use varchar(2048) consistently, and verify any indexes,
length checks, or hashing logic that reference enc_public_key length (e.g.,
enc_public_key_hash generation) are adjusted accordingly to avoid mismatches.
In `@db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql`:
- Around line 105-107: The ALTER TABLE in the upgrade script adds enc_public_key
as varchar(1024) which is smaller than the target/test schema; update the
migration to use the intended length (change enc_public_key to varchar(2048)) so
it matches the target DDL and tests, and ensure any related DDL/tests
referencing enc_public_key size are updated to 2048 as well; keep
enc_public_key_hash as-is unless tests require a different length.
In `@docs/esignet-openapi.yaml`:
- Around line 1453-1456: The PATCH schema currently omits validation constraints
(enums, uniqueItems, minItems, date-time format) for fields like requestTime and
request; update the PATCH schema to reuse the same property constraints as the
PUT/POST schemas so supplied fields are validated identically — either $ref the
existing PUT/POST property schemas or copy the exact constraints (including enum
lists, uniqueItems/minItems, and format: date-time) into PATCH properties (or
use allOf with the PUT/POST schema) so any provided field must meet the same
rules; apply the same change for the other affected schema block referenced in
the comment.
🧹 Nitpick comments (6)
client-management-service-impl/src/test/java/io/mosip/esignet/ClientDetailRepositoryTest.java (1)
293-319: Assert encPublicKeyHash values to fully validate persistence.
Right now only null/non-null is verified. Capture expected hashes and assert equality after fetch/update.🧪 Suggested test tightening
@@ - clientDetail.setEncPublicKeyHash(UUID.randomUUID().toString()); + String encPublicKeyHash = UUID.randomUUID().toString(); + clientDetail.setEncPublicKeyHash(encPublicKeyHash); @@ - Assertions.assertEquals("DUMMY ENC PUBLIC KEY", result.get().getEncPublicKey()); + Assertions.assertEquals("DUMMY ENC PUBLIC KEY", result.get().getEncPublicKey()); + Assertions.assertEquals(encPublicKeyHash, result.get().getEncPublicKeyHash()); @@ - clientDetail.setEncPublicKeyHash(UUID.randomUUID().toString()); + String updatedEncPublicKeyHash = UUID.randomUUID().toString(); + clientDetail.setEncPublicKeyHash(updatedEncPublicKeyHash); @@ - Assertions.assertEquals("UPDATED ENC PUBLIC KEY", result.get().getEncPublicKey()); + Assertions.assertEquals("UPDATED ENC PUBLIC KEY", result.get().getEncPublicKey()); + Assertions.assertEquals(updatedEncPublicKeyHash, result.get().getEncPublicKeyHash());Also applies to: 347-378
postman-collection/eSignet.postman_collection.json (2)
277-280: Remove duplicate alg/use assignments to reduce confusion.♻️ Suggested cleanup
- "// Add alg and use values to client public key", - "publicKey_jwk.alg = \"RS256\";", - "publicKey_jwk.use = \"sig\";", - "",
379-445: PATCH item is useful; consider normalizingencPublicKeyeven when reused.
Ifencryption_public_keyalready exists but lacksalg/use, the PATCH call can fail validation. Reasserting these fields each time keeps runs deterministic.🛠️ Suggested tweak
- "const existingEncPublicKey = pm.environment.get(\"encryption_public_key\");", - "if (!existingEncPublicKey || existingEncPublicKey === '' || existingEncPublicKey === 'null' || existingEncPublicKey === '{}') {", + "const existingEncPublicKey = pm.environment.get(\"encryption_public_key\");", + "let enc_publicKey_jwk;", + "if (!existingEncPublicKey || existingEncPublicKey === '' || existingEncPublicKey === 'null' || existingEncPublicKey === '{}') {", " enc_kp = pmlib.rs.KEYUTIL.generateKeypair(\"RSA\", 2048);", " enc_privateKey_jwk = pmlib.rs.KEYUTIL.getJWK(enc_kp.prvKeyObj);", " enc_publicKey_jwk = pmlib.rs.KEYUTIL.getJWK(enc_kp.pubKeyObj);", @@ - " pm.environment.set(\"encryption_private_key\", JSON.stringify(enc_privateKey_jwk));", - " pm.environment.set(\"encryption_public_key\", JSON.stringify(enc_publicKey_jwk));", - "}", + " pm.environment.set(\"encryption_private_key\", JSON.stringify(enc_privateKey_jwk));", + "} else {", + " enc_publicKey_jwk = JSON.parse(existingEncPublicKey);", + "}", + "enc_publicKey_jwk.alg = \"RSA-OAEP-256\";", + "enc_publicKey_jwk.use = \"enc\";", + "pm.environment.set(\"encryption_public_key\", JSON.stringify(enc_publicKey_jwk));",client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java (3)
343-364: Consider using ArgumentCaptor to verify patched entity state.The test verifies the response but doesn't confirm that the
ClientDetailentity passed tosave()has the expected patched values. UsingArgumentCaptorwould strengthen this test.♻️ Suggested enhancement
+ `@Captor` + ArgumentCaptor<ClientDetail> clientDetailCaptor; + `@Test` public void patchClient_withValidClientId_thenPass() throws EsignetException { ClientDetail clientDetail = createMockClientDetail("client_id_v1"); Mockito.when(clientDetailRepository.findById("client_id_v1")).thenReturn(Optional.of(clientDetail)); ClientDetailPatchRequest patchRequest = new ClientDetailPatchRequest(); patchRequest.setClientName("updated_client_name"); patchRequest.setLogoUri("http://service.com/new_logo.png"); patchRequest.setRedirectUris(Arrays.asList("http://service.com/callback")); patchRequest.setStatus("ACTIVE"); ClientDetail savedEntity = new ClientDetail(); savedEntity.setId("client_id_v1"); savedEntity.setStatus("ACTIVE"); - Mockito.when(clientDetailRepository.save(Mockito.any(ClientDetail.class))).thenReturn(savedEntity); + Mockito.when(clientDetailRepository.save(clientDetailCaptor.capture())).thenReturn(savedEntity); ClientDetailResponse response = clientManagementService.patchClient("client_id_v1", patchRequest); Assertions.assertNotNull(response); Assertions.assertEquals("client_id_v1", response.getClientId()); Assertions.assertEquals("ACTIVE", response.getStatus()); + + ClientDetail captured = clientDetailCaptor.getValue(); + Assertions.assertEquals("http://service.com/new_logo.png", captured.getLogoUri()); }
366-379: Consider usingassertThrowsfor exception testing (optional).While the try-catch pattern is consistent with existing tests in this file, JUnit 5's
assertThrowsprovides cleaner assertion syntax and ensures the test fails if the exception is not thrown.♻️ Modern JUnit 5 approach
`@Test` public void patchClient_withNonExistingClientId_thenFail() { Mockito.when(clientDetailRepository.findById("non_existing_client")).thenReturn(Optional.empty()); ClientDetailPatchRequest patchRequest = new ClientDetailPatchRequest(); patchRequest.setClientName("updated_name"); - try { - clientManagementService.patchClient("non_existing_client", patchRequest); - Assertions.fail(); - } catch (EsignetException ex) { - Assertions.assertEquals(ErrorConstants.INVALID_CLIENT_ID, ex.getErrorCode()); - } + EsignetException ex = Assertions.assertThrows(EsignetException.class, + () -> clientManagementService.patchClient("non_existing_client", patchRequest)); + Assertions.assertEquals(ErrorConstants.INVALID_CLIENT_ID, ex.getErrorCode()); }
701-718: Consider verifying that the timestamp is actually updated.The comment states "only timestamp should be updated," but the test doesn't verify this. Using
ArgumentCaptorto check thatupdatedTimesis set (and is recent) would strengthen this test.♻️ Suggested verification
`@Test` public void patchClient_withEmptyPatchRequest_thenOnlyUpdateTimestamp() throws EsignetException { ClientDetail clientDetail = createMockClientDetail("client_id_v1"); + LocalDateTime originalTimestamp = LocalDateTime.now().minusDays(1); + clientDetail.setUpdatedTimes(originalTimestamp); Mockito.when(clientDetailRepository.findById("client_id_v1")).thenReturn(Optional.of(clientDetail)); ClientDetailPatchRequest patchRequest = new ClientDetailPatchRequest(); // All fields are null - only timestamp should be updated ClientDetail savedEntity = new ClientDetail(); savedEntity.setId("client_id_v1"); savedEntity.setStatus("ACTIVE"); - Mockito.when(clientDetailRepository.save(Mockito.any(ClientDetail.class))).thenReturn(savedEntity); + ArgumentCaptor<ClientDetail> captor = ArgumentCaptor.forClass(ClientDetail.class); + Mockito.when(clientDetailRepository.save(captor.capture())).thenReturn(savedEntity); ClientDetailResponse response = clientManagementService.patchClient("client_id_v1", patchRequest); Assertions.assertNotNull(response); Assertions.assertEquals("client_id_v1", response.getClientId()); + Assertions.assertTrue(captor.getValue().getUpdatedTimes().isAfter(originalTimestamp)); }
Comment on lines
+328
to
+412
| /** | ||
| * Build client detail entity for PATCH update operation. | ||
| * @param clientId The client ID to update | ||
| * @param patchRequest The patch request containing fields to update | ||
| * @return Updated ClientDetail entity | ||
| */ | ||
| public ClientDetail buildClient(String clientId, ClientDetailPatchRequest patchRequest) { | ||
| Optional<ClientDetail> result = clientDetailRepository.findById(clientId); | ||
| if (result.isEmpty()) { | ||
| log.error("Invalid Client Id : {}", ErrorConstants.INVALID_CLIENT_ID); | ||
| throw new EsignetException(ErrorConstants.INVALID_CLIENT_ID); | ||
| } | ||
|
|
||
| ClientDetail clientDetail = result.get(); | ||
|
|
||
| // Apply partial updates - only non-null fields | ||
| if (patchRequest.getLogoUri() != null) { | ||
| clientDetail.setLogoUri(patchRequest.getLogoUri()); | ||
| } | ||
|
|
||
| if (patchRequest.getRedirectUris() != null) { | ||
| patchRequest.getRedirectUris().removeAll(NULL); | ||
| clientDetail.setRedirectUris(JSONArray.toJSONString(patchRequest.getRedirectUris())); | ||
| } | ||
|
|
||
| if (patchRequest.getUserClaims() != null) { | ||
| patchRequest.getUserClaims().removeAll(NULL); | ||
| clientDetail.setClaims(JSONArray.toJSONString(patchRequest.getUserClaims())); | ||
| } | ||
|
|
||
| if (patchRequest.getAuthContextRefs() != null) { | ||
| patchRequest.getAuthContextRefs().removeAll(NULL); | ||
| clientDetail.setAcrValues(JSONArray.toJSONString(patchRequest.getAuthContextRefs())); | ||
| } | ||
|
|
||
| if (patchRequest.getStatus() != null) { | ||
| clientDetail.setStatus(patchRequest.getStatus()); | ||
| } | ||
|
|
||
| if (patchRequest.getGrantTypes() != null) { | ||
| patchRequest.getGrantTypes().removeAll(NULL); | ||
| clientDetail.setGrantTypes(JSONArray.toJSONString(patchRequest.getGrantTypes())); | ||
| } | ||
|
|
||
| if (patchRequest.getClientAuthMethods() != null) { | ||
| patchRequest.getClientAuthMethods().removeAll(NULL); | ||
| clientDetail.setClientAuthMethods(JSONArray.toJSONString(patchRequest.getClientAuthMethods())); | ||
| } | ||
|
|
||
| // Handle client name update | ||
| if (patchRequest.getClientName() != null || patchRequest.getClientNameLangMap() != null) { | ||
| String existingName = clientDetail.getName(); | ||
| Map<String, String> existingNameMap = new HashMap<>(); | ||
| try { | ||
| if (existingName != null) { | ||
| existingNameMap = objectMapper.readValue(existingName, new TypeReference<Map<String, String>>() {}); | ||
| } | ||
| } catch (Exception e) { | ||
| log.warn("Failed to parse existing client name as JSON, using empty map"); | ||
| } | ||
|
|
||
| String clientName = patchRequest.getClientName() != null ? | ||
| patchRequest.getClientName() : | ||
| existingNameMap.getOrDefault(Constants.NONE_LANG_KEY, ""); | ||
|
|
||
| Map<String, String> clientNameLangMap = patchRequest.getClientNameLangMap() != null ? | ||
| patchRequest.getClientNameLangMap() : | ||
| existingNameMap; | ||
|
|
||
| clientDetail.setName(getClientNameLanguageMapAsJsonString(clientNameLangMap, clientName)); | ||
| } | ||
|
|
||
| if (patchRequest.getAdditionalConfig() != null) { | ||
| clientDetail.setAdditionalConfig(patchRequest.getAdditionalConfig()); | ||
| } | ||
|
|
||
| // Handle enc_public_key update - only if provided and not empty | ||
| if (patchRequest.getEncPublicKey() != null && !patchRequest.getEncPublicKey().isEmpty()) { | ||
| clientDetail.setEncPublicKey(IdentityProviderUtil.getJWKString(patchRequest.getEncPublicKey())); | ||
| clientDetail.setEncPublicKeyHash(identityProviderUtil.computePublicKeyHash(patchRequest.getEncPublicKey())); | ||
| } | ||
|
|
||
| clientDetail.setUpdatedtimes(LocalDateTime.now(ZoneId.of("UTC"))); | ||
| return clientDetail; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java | sed -n '328,412p'Repository: mosip/esignet
Length of output: 4530
🏁 Script executed:
# Check Constants.NONE_LANG_KEY definition
grep -r "NONE_LANG_KEY" client-management-service-impl/src/main/java/ -nRepository: mosip/esignet
Length of output: 603
🏁 Script executed:
# Check getClientNameLanguageMapAsJsonString implementation
grep -A 20 "getClientNameLanguageMapAsJsonString" client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java -nRepository: mosip/esignet
Length of output: 4363
🏁 Script executed:
# Check ClientDetailPatchRequest to understand the fields
find client-management-service-impl/src -name "ClientDetailPatchRequest.java" -exec cat {} \;Repository: mosip/esignet
Length of output: 39
Preserve original client name when JSON parsing fails during PATCH updates.
When the stored name is invalid JSON and the patch request only provides clientNameLangMap without clientName, the current logic falls back to an empty string for the @none language key, permanently losing the original name. Store the original name as a fallback when JSON parsing fails.
Suggested fix
Map<String, String> existingNameMap = new HashMap<>();
+ boolean parseFailed = false;
try {
if (existingName != null) {
existingNameMap = objectMapper.readValue(existingName, new TypeReference<Map<String, String>>() {});
}
} catch (Exception e) {
- log.warn("Failed to parse existing client name as JSON, using empty map");
+ parseFailed = true;
+ log.warn("Failed to parse existing client name as JSON, using `@none` fallback");
}
+ if (parseFailed && existingName != null) {
+ existingNameMap.put(Constants.NONE_LANG_KEY, existingName);
+ }
+
String clientName = patchRequest.getClientName() != null ?
patchRequest.getClientName() :
existingNameMap.getOrDefault(Constants.NONE_LANG_KEY, "");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Build client detail entity for PATCH update operation. | |
| * @param clientId The client ID to update | |
| * @param patchRequest The patch request containing fields to update | |
| * @return Updated ClientDetail entity | |
| */ | |
| public ClientDetail buildClient(String clientId, ClientDetailPatchRequest patchRequest) { | |
| Optional<ClientDetail> result = clientDetailRepository.findById(clientId); | |
| if (result.isEmpty()) { | |
| log.error("Invalid Client Id : {}", ErrorConstants.INVALID_CLIENT_ID); | |
| throw new EsignetException(ErrorConstants.INVALID_CLIENT_ID); | |
| } | |
| ClientDetail clientDetail = result.get(); | |
| // Apply partial updates - only non-null fields | |
| if (patchRequest.getLogoUri() != null) { | |
| clientDetail.setLogoUri(patchRequest.getLogoUri()); | |
| } | |
| if (patchRequest.getRedirectUris() != null) { | |
| patchRequest.getRedirectUris().removeAll(NULL); | |
| clientDetail.setRedirectUris(JSONArray.toJSONString(patchRequest.getRedirectUris())); | |
| } | |
| if (patchRequest.getUserClaims() != null) { | |
| patchRequest.getUserClaims().removeAll(NULL); | |
| clientDetail.setClaims(JSONArray.toJSONString(patchRequest.getUserClaims())); | |
| } | |
| if (patchRequest.getAuthContextRefs() != null) { | |
| patchRequest.getAuthContextRefs().removeAll(NULL); | |
| clientDetail.setAcrValues(JSONArray.toJSONString(patchRequest.getAuthContextRefs())); | |
| } | |
| if (patchRequest.getStatus() != null) { | |
| clientDetail.setStatus(patchRequest.getStatus()); | |
| } | |
| if (patchRequest.getGrantTypes() != null) { | |
| patchRequest.getGrantTypes().removeAll(NULL); | |
| clientDetail.setGrantTypes(JSONArray.toJSONString(patchRequest.getGrantTypes())); | |
| } | |
| if (patchRequest.getClientAuthMethods() != null) { | |
| patchRequest.getClientAuthMethods().removeAll(NULL); | |
| clientDetail.setClientAuthMethods(JSONArray.toJSONString(patchRequest.getClientAuthMethods())); | |
| } | |
| // Handle client name update | |
| if (patchRequest.getClientName() != null || patchRequest.getClientNameLangMap() != null) { | |
| String existingName = clientDetail.getName(); | |
| Map<String, String> existingNameMap = new HashMap<>(); | |
| try { | |
| if (existingName != null) { | |
| existingNameMap = objectMapper.readValue(existingName, new TypeReference<Map<String, String>>() {}); | |
| } | |
| } catch (Exception e) { | |
| log.warn("Failed to parse existing client name as JSON, using empty map"); | |
| } | |
| String clientName = patchRequest.getClientName() != null ? | |
| patchRequest.getClientName() : | |
| existingNameMap.getOrDefault(Constants.NONE_LANG_KEY, ""); | |
| Map<String, String> clientNameLangMap = patchRequest.getClientNameLangMap() != null ? | |
| patchRequest.getClientNameLangMap() : | |
| existingNameMap; | |
| clientDetail.setName(getClientNameLanguageMapAsJsonString(clientNameLangMap, clientName)); | |
| } | |
| if (patchRequest.getAdditionalConfig() != null) { | |
| clientDetail.setAdditionalConfig(patchRequest.getAdditionalConfig()); | |
| } | |
| // Handle enc_public_key update - only if provided and not empty | |
| if (patchRequest.getEncPublicKey() != null && !patchRequest.getEncPublicKey().isEmpty()) { | |
| clientDetail.setEncPublicKey(IdentityProviderUtil.getJWKString(patchRequest.getEncPublicKey())); | |
| clientDetail.setEncPublicKeyHash(identityProviderUtil.computePublicKeyHash(patchRequest.getEncPublicKey())); | |
| } | |
| clientDetail.setUpdatedtimes(LocalDateTime.now(ZoneId.of("UTC"))); | |
| return clientDetail; | |
| } | |
| /** | |
| * Build client detail entity for PATCH update operation. | |
| * `@param` clientId The client ID to update | |
| * `@param` patchRequest The patch request containing fields to update | |
| * `@return` Updated ClientDetail entity | |
| */ | |
| public ClientDetail buildClient(String clientId, ClientDetailPatchRequest patchRequest) { | |
| Optional<ClientDetail> result = clientDetailRepository.findById(clientId); | |
| if (result.isEmpty()) { | |
| log.error("Invalid Client Id : {}", ErrorConstants.INVALID_CLIENT_ID); | |
| throw new EsignetException(ErrorConstants.INVALID_CLIENT_ID); | |
| } | |
| ClientDetail clientDetail = result.get(); | |
| // Apply partial updates - only non-null fields | |
| if (patchRequest.getLogoUri() != null) { | |
| clientDetail.setLogoUri(patchRequest.getLogoUri()); | |
| } | |
| if (patchRequest.getRedirectUris() != null) { | |
| patchRequest.getRedirectUris().removeAll(NULL); | |
| clientDetail.setRedirectUris(JSONArray.toJSONString(patchRequest.getRedirectUris())); | |
| } | |
| if (patchRequest.getUserClaims() != null) { | |
| patchRequest.getUserClaims().removeAll(NULL); | |
| clientDetail.setClaims(JSONArray.toJSONString(patchRequest.getUserClaims())); | |
| } | |
| if (patchRequest.getAuthContextRefs() != null) { | |
| patchRequest.getAuthContextRefs().removeAll(NULL); | |
| clientDetail.setAcrValues(JSONArray.toJSONString(patchRequest.getAuthContextRefs())); | |
| } | |
| if (patchRequest.getStatus() != null) { | |
| clientDetail.setStatus(patchRequest.getStatus()); | |
| } | |
| if (patchRequest.getGrantTypes() != null) { | |
| patchRequest.getGrantTypes().removeAll(NULL); | |
| clientDetail.setGrantTypes(JSONArray.toJSONString(patchRequest.getGrantTypes())); | |
| } | |
| if (patchRequest.getClientAuthMethods() != null) { | |
| patchRequest.getClientAuthMethods().removeAll(NULL); | |
| clientDetail.setClientAuthMethods(JSONArray.toJSONString(patchRequest.getClientAuthMethods())); | |
| } | |
| // Handle client name update | |
| if (patchRequest.getClientName() != null || patchRequest.getClientNameLangMap() != null) { | |
| String existingName = clientDetail.getName(); | |
| Map<String, String> existingNameMap = new HashMap<>(); | |
| boolean parseFailed = false; | |
| try { | |
| if (existingName != null) { | |
| existingNameMap = objectMapper.readValue(existingName, new TypeReference<Map<String, String>>() {}); | |
| } | |
| } catch (Exception e) { | |
| parseFailed = true; | |
| log.warn("Failed to parse existing client name as JSON, using `@none` fallback"); | |
| } | |
| if (parseFailed && existingName != null) { | |
| existingNameMap.put(Constants.NONE_LANG_KEY, existingName); | |
| } | |
| String clientName = patchRequest.getClientName() != null ? | |
| patchRequest.getClientName() : | |
| existingNameMap.getOrDefault(Constants.NONE_LANG_KEY, ""); | |
| Map<String, String> clientNameLangMap = patchRequest.getClientNameLangMap() != null ? | |
| patchRequest.getClientNameLangMap() : | |
| existingNameMap; | |
| clientDetail.setName(getClientNameLanguageMapAsJsonString(clientNameLangMap, clientName)); | |
| } | |
| if (patchRequest.getAdditionalConfig() != null) { | |
| clientDetail.setAdditionalConfig(patchRequest.getAdditionalConfig()); | |
| } | |
| // Handle enc_public_key update - only if provided and not empty | |
| if (patchRequest.getEncPublicKey() != null && !patchRequest.getEncPublicKey().isEmpty()) { | |
| clientDetail.setEncPublicKey(IdentityProviderUtil.getJWKString(patchRequest.getEncPublicKey())); | |
| clientDetail.setEncPublicKeyHash(identityProviderUtil.computePublicKeyHash(patchRequest.getEncPublicKey())); | |
| } | |
| clientDetail.setUpdatedtimes(LocalDateTime.now(ZoneId.of("UTC"))); | |
| return clientDetail; | |
| } |
🤖 Prompt for AI Agents
In
`@client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java`
around lines 328 - 412, The patch loses the original client name when
objectMapper.readValue fails: capture the raw existingName before parsing in
buildClient, and if parsing throws, populate existingNameMap with a single entry
mapping Constants.NONE_LANG_KEY to the original existingName (if non-null)
instead of leaving it empty; then proceed to derive clientName and
clientNameLangMap and call getClientNameLanguageMapAsJsonString as before so the
original name is preserved when only clientNameLangMap is provided.
Comment on lines
+28
to
+29
| enc_public_key varchar(1024), | ||
| enc_public_key_hash varchar(128), |
There was a problem hiding this comment.
Align enc_public_key length across schemas to avoid truncation.
Line 28 defines enc_public_key as varchar(1024), but test schemas use varchar(2048). This inconsistency can mask truncation bugs in production. Please standardize the length across the DDL, upgrade script, and test schemas (consider 2048 if that’s the intended max).
🔧 Proposed fix (if 2048 is the intended size)
- enc_public_key varchar(1024),
+ enc_public_key varchar(2048),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| enc_public_key varchar(1024), | |
| enc_public_key_hash varchar(128), | |
| enc_public_key varchar(2048), | |
| enc_public_key_hash varchar(128), |
🤖 Prompt for AI Agents
In `@db_scripts/mosip_esignet/ddl/esignet-client_detail.sql` around lines 28 - 29,
The enc_public_key column is defined as varchar(1024) here but other schemas use
varchar(2048), which risks truncation; update the DDL (column enc_public_key)
and any related upgrade/migration scripts and test schema definitions to use
varchar(2048) consistently, and verify any indexes, length checks, or hashing
logic that reference enc_public_key length (e.g., enc_public_key_hash
generation) are adjusted accordingly to avoid mismatches.
Comment on lines
+105
to
+107
| -- Add enc_public_key and enc_public_key_hash columns to store encryption public key and computed hash in JWK format | ||
| ALTER TABLE client_detail ADD COLUMN IF NOT EXISTS enc_public_key varchar(1024); | ||
| ALTER TABLE client_detail ADD COLUMN IF NOT EXISTS enc_public_key_hash varchar(128); |
There was a problem hiding this comment.
Keep upgrade column length in sync with target schema.
Line 106 sets enc_public_key to varchar(1024), but the test schemas use varchar(2048). This mismatch can lead to production truncation that tests won’t catch. Please align the upgrade script with the intended final size (and update the DDL/tests accordingly).
🔧 Proposed fix (if 2048 is the intended size)
-ALTER TABLE client_detail ADD COLUMN IF NOT EXISTS enc_public_key varchar(1024);
+ALTER TABLE client_detail ADD COLUMN IF NOT EXISTS enc_public_key varchar(2048);🤖 Prompt for AI Agents
In `@db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql` around lines
105 - 107, The ALTER TABLE in the upgrade script adds enc_public_key as
varchar(1024) which is smaller than the target/test schema; update the migration
to use the intended length (change enc_public_key to varchar(2048)) so it
matches the target DDL and tests, and ensure any related DDL/tests referencing
enc_public_key size are updated to 2048 as well; keep enc_public_key_hash as-is
unless tests require a different length.
Comment on lines
+1453
to
+1456
| requestTime: | ||
| type: string | ||
| description: Current date and time when the request is sent | ||
| request: |
There was a problem hiding this comment.
Mirror PUT/POST validation constraints in the PATCH schema
PATCH accepts the same fields but drops key constraints (enums, uniqueItems, minItems, date-time format). This weakens the contract and can mislead integrators about accepted values. Suggest reusing the same constraints as the PUT/POST schemas when a field is supplied.
🔧 Proposed doc fix
requestTime:
type: string
+ format: date-time
description: Current date and time when the request is sent
...
redirectUris:
type: array
description: Valid list of callback URIs.
minItems: 1
+ uniqueItems: true
items:
type: string
format: uri
userClaims:
type: array
description: Allowed user info claims.
+ minItems: 1
items:
type: string
+ enum:
+ - name
+ - given_name
+ - middle_name
+ - preferred_username
+ - nickname
+ - gender
+ - birthdate
+ - email
+ - phone_number
+ - picture
+ - address
authContextRefs:
type: array
description: Authentication Context Class Reference values.
+ minItems: 1
+ uniqueItems: true
items:
type: string
+ enum:
+ - 'mosip:idp:acr:static-code'
+ - 'mosip:idp:acr:generated-code'
+ - 'mosip:idp:acr:linked-wallet'
+ - 'mosip:idp:acr:biometrics'
+ - 'mosip:idp:acr:knowledge'
+ - 'mosip:idp:acr:password'
+ - 'mosip:idp:acr:id-token'
grantTypes:
type: array
description: Form of Authorization Grant.
+ minItems: 1
items:
- type: string
+ const: authorization_code
clientAuthMethods:
type: array
description: Auth method supported for token endpoint.
+ minItems: 1
items:
- type: string
+ const: private_key_jwtAlso applies to: 1479-1504
🤖 Prompt for AI Agents
In `@docs/esignet-openapi.yaml` around lines 1453 - 1456, The PATCH schema
currently omits validation constraints (enums, uniqueItems, minItems, date-time
format) for fields like requestTime and request; update the PATCH schema to
reuse the same property constraints as the PUT/POST schemas so supplied fields
are validated identically — either $ref the existing PUT/POST property schemas
or copy the exact constraints (including enum lists, uniqueItems/minItems, and
format: date-time) into PATCH properties (or use allOf with the PUT/POST schema)
so any provided field must meet the same rules; apply the same change for the
other affected schema block referenced in the comment.
Signed-off-by: Md-Humair-KK <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.