Repository navigation
Updated client management endpoints to be backward compatible - #2117
Merged
Merged
Conversation
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughUpdates the client-management API to ChangesClient Management v3 API
MOSIP Engine: IDA Models and Biometric Auth
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@esignet-service/internal/clientmgmt/db/query.sql`:
- Around line 45-58: The full-row UPDATE in client_detail is causing
last-writer-wins behavior for Service.PatchClient because it writes every column
from a stale read/merge. Update the query to only set the fields listed in
PatchFields, or add concurrency protection around the read-modify-write flow in
PatchClient using locking or an optimistic version check. Use the existing
update statement in query.sql and the PatchClient flow as the main places to
adjust.
In `@esignet-service/internal/clientmgmt/handler.go`:
- Around line 43-50: The client-mgmt route registration in handler.go only
exposes POST/PUT for the `ProfileOIDC` and `ProfileOAuth` endpoints, while
`getClient` and `patchClient` are still only mounted under
`/client-mgmt/client`, leaving legacy read and patch calls broken. Update the
`mux.Handle` registrations in `handler.go` so `createClient`, `updateClient`,
`getClient`, and `patchClient` are all available through the intended
backward-compatible aliases for `oidc-client` and `oauth-client`, or else
explicitly remove those aliases from the compatibility promise. Keep the fix
localized to the route setup around `wrap(...)` and the profile-specific
handlers.
In `@esignet-service/internal/clientmgmt/jwk_validate.go`:
- Around line 72-83: The isValidURI helper is currently only checking prefixes
and length, which lets malformed values like bare schemes or arbitrary strings
with “://” pass as valid client configuration. Update isValidURI in
jwk_validate.go to parse the value with a real URI parser and validate the
parsed result before returning true, using the existing isValidURI call sites
that feed logoUri and redirectUris. Keep the length guard if needed, but only
accept URIs that successfully parse and have the expected structure for the
allowed schemes.
In `@esignet-service/internal/clientmgmt/model.go`:
- Around line 185-235: In DecodePatchRequest, stop ignoring json.Unmarshal
errors for decoded PATCH fields and return an error whenever a field value is
malformed instead of silently leaving it zero/nil. Apply the same validation
used for encPublicKey to all switch cases in PatchClientRequest decoding
(clientName, redirectUris, userClaims, authContextRefs, grantTypes,
clientAuthMethods, etc.), and reject a null request body up front so
request:null does not produce an empty no-op PatchClientRequest.
In `@esignet-service/internal/clientmgmt/service_test.go`:
- Around line 232-275: The patch tests for client encrypted public key handling
only verify EncPublicKey, so they can miss stale companion data in
db.PatchClientParams. Update TestPatchClient_EncPublicKey and
TestPatchClient_ClearEncPublicKey to also assert the EncPublicKeyHash and
EncPublicKeyCert fields in the mockQuerier.patchFn callback, alongside
EncPublicKey, using the existing PatchClient and PatchClientParams symbols to
verify the hash/cert are populated or cleared consistently.
In `@esignet-service/internal/clientmgmt/service.go`:
- Around line 187-264: PatchClient currently builds db.PatchClientParams from a
merged pre-PATCH snapshot, which can overwrite concurrent updates with stale
values. Update the clientmgmt service flow around mergePatch/ValidatePatch and
the PatchClientParams assembly so only fields present in the request are
written, or guard the read-merge-write with a row lock or optimistic upd_dtimes
check. Keep the existing helpers like normalizePatchStatus, marshalStringSlice,
and marshalAdditionalConfigRaw, but avoid sending unchanged columns back to the
database.
- Around line 228-246: The encPKCert value is not updated when ClientService
updates EncPublicKey, so a new encrypted key can keep the old certificate
attached. In the existing EncPublicKey update block in service.go, either
validate and patch the certificate alongside the new key or clear encPKCert
whenever req.EncPublicKey.Value replaces the prior key. Make the change in the
EncPublicKey handling path using the existing fields, existing.EncPublicKeyCert,
and the validateJWK/marshalJWK flow so the cert stays consistent with the
current key.
- Around line 267-273: PatchClient in service.go should handle duplicate
public-key hash violations the same way CreateClient does instead of surfacing
them as a generic server error. Update the error handling around s.q.PatchClient
so the duplicate hash/unique-constraint case is mapped to invalid_public_key,
while preserving the existing sql.ErrNoRows to ErrClientNotFound and the
fallback wrapped error path for all other failures.
In `@esignet-service/internal/clientmgmt/validate.go`:
- Around line 68-72: The create validation currently skips redirect URI checks
when req.RedirectURIs is empty, which lets authorization_code clients be created
without any redirect URI. Update the create-path validation in validate.go
(around validateRedirectURIs usage) to require at least one redirect URI,
consistent with the PUT contract and the authorization_code grant type. Keep the
existing validateRedirectURIs call for non-empty input, but add an explicit
rejection when req.RedirectURIs is missing or empty in the create flow.
- Around line 187-239: ValidatePatch currently relaxes the profile contract by
skipping Profile-based checks, using minItems=0 for required collections, and
omitting fields.EncPublicKey validation, so update ValidatePatch to enforce the
same profile-driven rules as ValidateUpdate on the merged client state. Reuse
the existing validation helpers in validate.go (especially ValidateUpdate,
validateRedirectURIs, validateClaims, validateACRs, validateGrantTypes,
validateAuthMethods, validateAdditionalConfig, and the enc public key validation
path) and make sure PATCH cannot clear required arrays or bypass OIDC/OAuth
constraints.
In `@esignet-service/internal/engine/mosip/model.go`:
- Around line 122-128: `IdaKycExchangeRequest.ConsentObtained` is modeled as a
slice of strings, but the MOSIP KYC exchange payload expects a boolean field.
Update the `IdaKycExchangeRequest` struct in `model.go` so `ConsentObtained`
uses the correct boolean type, and adjust any builder/serialization code that
populates it to send a true/false consent flag instead of `consentedAttributes`
values. Verify the JSON field name stays `consentObtained` and that the request
produced by `IdaKycExchangeRequest` matches the exchange schema.
In `@esignet-service/internal/engine/mosip/mosip_authn.go`:
- Around line 108-121: The credential parsing in mosip_authn.go is falling
through when none of OTP, password, or biometric is actually accepted, which
lets an auth request proceed with only timestamp. Update the
credential-selection logic in the auth request builder to detect when no valid
OTP/password/biometric was parsed and return a local “missing or invalid
credentials” error instead of continuing. Keep the check tied to the existing
credential handling in the authRequest/B64Decode/json.Unmarshal flow so any
non-string, empty, or otherwise unusable value is rejected before the downstream
IDA call.
In `@esignet-service/postman/README.md`:
- Around line 61-75: The Postman README instruction for `clientMgmtToken` is
incorrect because the collection already prefixes it with `Bearer` in the
`Authorization` header, so users should paste only the raw JWT string. Update
the guidance near the `clientMgmtToken` setup to say it must contain just the
token produced by `scripts/sign-mgmt-token.py`, not a `Bearer <jwt>` value, and
keep the surrounding scope-enforcement instructions tied to
`CLIENT_MGMT_ISSUER_URL`, `CLIENT_MGMT_JWKS_ENDPOINT`, and the `clientMgmtToken`
variable.
In `@esignet-service/scripts/client-mgmt-smoke.sh`:
- Around line 76-194: Add the missing PATCH coverage to run_client_mgmt_smoke so
the client lifecycle exercises the new v3 write path as well. Reuse the existing
client_id, auth_header, and temp-file curl pattern used by the create/get/update
steps, and insert a PATCH call to /client-mgmt/client/{client_id} before the
final cleanup. Validate the PATCH response the same way the other steps do,
using the response fields and a pass/fail message consistent with the existing
smoke checks.
- Line 4: The smoke test script still defaults BASE_URL to the old 8080
endpoint, which conflicts with the updated documented setup. Update the BASE_URL
default in client-mgmt-smoke.sh to match the new local port used by the examples
so the script works standalone without requiring an override. Keep the change
focused on the BASE_URL assignment near the top of the script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f2cb544-68e5-4c8a-97f1-645ae9152e24
📒 Files selected for processing (24)
esignet-service/README.mdesignet-service/internal/clientmgmt/db/db.goesignet-service/internal/clientmgmt/db/models.goesignet-service/internal/clientmgmt/db/querier.goesignet-service/internal/clientmgmt/db/query.sqlesignet-service/internal/clientmgmt/db/query.sql.goesignet-service/internal/clientmgmt/handler.goesignet-service/internal/clientmgmt/jwk_input_test.goesignet-service/internal/clientmgmt/jwk_validate.goesignet-service/internal/clientmgmt/model.goesignet-service/internal/clientmgmt/model_test.goesignet-service/internal/clientmgmt/service.goesignet-service/internal/clientmgmt/service_test.goesignet-service/internal/clientmgmt/validate.goesignet-service/internal/engine/actors.goesignet-service/internal/engine/mosip/model.goesignet-service/internal/engine/mosip/mosip_authn.goesignet-service/make.shesignet-service/postman/README.mdesignet-service/postman/embedder-local.environment.jsonesignet-service/postman/embedder-positive-flow.jsonesignet-service/scripts/client-mgmt-smoke.shesignet-service/scripts/oauth-smoke.shesignet-service/scripts/sign-mgmt-token.py
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)
esignet-service/internal/engine/mosip/mosip_authn.go (1)
220-226: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPopulate the KYC exchange claim fields in the request payload.
consentedAttributesis derived fromrequested.Names, but the outboundIdaKycExchangeRequestonly setsConsentObtained: true. PopulateverifiedConsentedClaims/unVerifiedConsentedClaimshere so filtered attribute requests keep their claim selection at the IDA boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@esignet-service/internal/engine/mosip/mosip_authn.go` around lines 220 - 226, The IdaKycExchangeRequest construction in mosip_authn.go only sets ConsentObtained and drops the derived consent claim data from requested.Names. Update the request payload assembly in the KYC exchange flow to populate verifiedConsentedClaims and unVerifiedConsentedClaims from consentedAttributes before sending the request, keeping the existing transactionID/KycToken handling intact.esignet-service/README.md (1)
104-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe documented default port no longer matches
make.sh.This table says
make.shdefaultsPORTto8088, butmake.shstill initializes it to8080and derivesMOSIP_ESIGNET_HOSTfrom that value. Anyone relying on defaults will start the service on 8080 while the surrounding examples target 8088.Proposed fix
-| `PORT` | `8088` | HTTP listen port (`make.sh` defaults to `8088`) | +| `PORT` | `8080` | HTTP listen port (`make.sh` defaults to `8080`) |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@esignet-service/README.md` at line 104, The default port documentation is out of sync with the actual startup behavior in make.sh. Update the PORT entry in the README table to match the value initialized by make.sh, and ensure any related default-host example for MOSIP_ESIGNET_HOST stays consistent with that same port. Keep the documented defaults aligned with the startup script so users following the README land on the same port as the service.
♻️ Duplicate comments (1)
esignet-service/internal/clientmgmt/validate.go (1)
189-240: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThread
Profileinto PATCH validation before validating the merged state.
validateMergedPatchStatealways requiresClientNameLangMap, butValidateCreaterejects it forProfileOIDC; valid OIDC clients will fail PATCH after merge. The same missing profile context also lets PATCH use the broad ACR/additionalConfig rules instead of the create/update profile contract.Suggested direction
-func ValidatePatch(merged UpdateClientRequest, fields PatchFields) error { +func ValidatePatch(profile Profile, merged UpdateClientRequest, fields PatchFields) error { + acrAllowed := allowedACRAll + if profile == ProfileOIDC { + acrAllowed = allowedACROIDCPut + } ... if fields.AcrValues { - if err := validateACRs(merged.AcrValues, allowedACRAll, 0, 30); err != nil { + if err := validateACRs(merged.AcrValues, acrAllowed, 0, 30); err != nil { return err } } ... - return validateMergedPatchState(merged) + return validateMergedPatchState(profile, merged) } -func validateMergedPatchState(merged UpdateClientRequest) error { +func validateMergedPatchState(profile Profile, merged UpdateClientRequest) error { if err := validateClientName(merged.ClientName); err != nil { return err } - if err := validateClientNameLangMap(merged.ClientNameLangMap, false); err != nil { - return err + if profile == ProfileOIDC { + if merged.ClientNameLangMap != nil || len(merged.AdditionalConfig) > 0 { + return validationErr("invalid_input") + } + } else { + if err := validateClientNameLangMap(merged.ClientNameLangMap, false); err != nil { + return err + } + if profile == ProfileOAuth && len(merged.AdditionalConfig) > 0 { + return validationErr("invalid_input") + } } ... - if err := validateACRs(merged.AcrValues, allowedACRAll, 1, 0); err != nil { + if err := validateACRs(merged.AcrValues, acrAllowed, 1, 0); err != nil { return err } ... - if len(merged.AdditionalConfig) > 0 { + if profile == ProfileClient && len(merged.AdditionalConfig) > 0 { if err := validateAdditionalConfig(merged.AdditionalConfig); err != nil { return err } }Also applies to: 243-284
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@esignet-service/internal/clientmgmt/validate.go` around lines 189 - 240, ValidatePatch currently calls validateMergedPatchState without any profile context, so PATCH validation can incorrectly apply the wrong contract for OIDC versus other profiles. Update ValidatePatch and validateMergedPatchState to accept and thread the client Profile (same one used by ValidateCreate/ValidateUpdate) so the merged-state checks can honor ProfileOIDC rules for ClientNameLangMap, ACR values, and AdditionalConfig. Keep the field-specific validation in ValidatePatch, then pass the profile through before the final merged-state validation so PATCH follows the same profile-aware constraints as create/update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@esignet-service/internal/clientmgmt/handler.go`:
- Around line 191-192: The ErrClientConflict branch in handleUpdateClient is
mapping a stale-write conflict to invalid_input, which makes a retryable
concurrency error look like a validation failure. Update the case handling for
ErrClientConflict in handler.go to return a distinct conflict-style response (or
another non-input error code until the envelope adds one) instead of
writeSpecError with invalid_input, while keeping the existing retry guidance in
the message.
In `@esignet-service/internal/clientmgmt/jwk_validate.go`:
- Around line 97-105: `isValidRedirectURI` currently allows any scheme as long
as `url.Parse` succeeds and `u.Scheme`/`u.Host` are present, which is broader
than the intended HTTP/HTTPS-only contract. Update the validation in
`isValidRedirectURI` to explicitly accept only `http` and `https` schemes, while
still rejecting malformed URLs and empty hosts, and keep the native redirect URI
TODOs deferred until that support is implemented.
In `@esignet-service/scripts/smoke-client-lib.sh`:
- Around line 137-164: The shared curl wrapper in client_mgmt_request currently
has no connect or read timeout, so a dead or half-open endpoint can hang every
management call. Update client_mgmt_request to pass explicit curl timeout
options for both connection establishment and response waiting, and keep the
existing success/error handling and output capture intact. Make the change in
the client_mgmt_request helper so all oauth-smoke.sh management requests inherit
the timeout behavior automatically.
- Around line 20-45: The mgmt_auth_header helper only checks CLIENT_MGMT_ISSUER,
so it misses the documented CLIENT_MGMT_ISSUER_URL and
CLIENT_MGMT_REQUIRED_SCOPE names and won’t work for standalone smoke runs.
Update mgmt_auth_header (and the client-mgmt-smoke.sh entrypoint if needed) to
source the local .env first, then resolve the issuer/scope from the documented
env vars with the existing aliases as fallbacks, so the Authorization header is
generated consistently.
- Around line 167-184: ensure_smoke_client should tolerate an already-existing
client when delete_smoke_client cannot clean the DB. Update the logic in
ensure_smoke_client so that after client_mgmt_request returns a non-200
response, it checks for a duplicate_client_id/exists-style failure and then
verifies the existing client via the same clientId and ACTIVE checks before
failing. Keep the current success path intact and use client_mgmt_request,
delete_smoke_client, and the response/status parsing in ensure_smoke_client to
make the helper idempotent without requiring psql access.
---
Outside diff comments:
In `@esignet-service/internal/engine/mosip/mosip_authn.go`:
- Around line 220-226: The IdaKycExchangeRequest construction in mosip_authn.go
only sets ConsentObtained and drops the derived consent claim data from
requested.Names. Update the request payload assembly in the KYC exchange flow to
populate verifiedConsentedClaims and unVerifiedConsentedClaims from
consentedAttributes before sending the request, keeping the existing
transactionID/KycToken handling intact.
In `@esignet-service/README.md`:
- Line 104: The default port documentation is out of sync with the actual
startup behavior in make.sh. Update the PORT entry in the README table to match
the value initialized by make.sh, and ensure any related default-host example
for MOSIP_ESIGNET_HOST stays consistent with that same port. Keep the documented
defaults aligned with the startup script so users following the README land on
the same port as the service.
---
Duplicate comments:
In `@esignet-service/internal/clientmgmt/validate.go`:
- Around line 189-240: ValidatePatch currently calls validateMergedPatchState
without any profile context, so PATCH validation can incorrectly apply the wrong
contract for OIDC versus other profiles. Update ValidatePatch and
validateMergedPatchState to accept and thread the client Profile (same one used
by ValidateCreate/ValidateUpdate) so the merged-state checks can honor
ProfileOIDC rules for ClientNameLangMap, ACR values, and AdditionalConfig. Keep
the field-specific validation in ValidatePatch, then pass the profile through
before the final merged-state validation so PATCH follows the same profile-aware
constraints as create/update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5665cb85-2363-42e0-9468-35134820f354
📒 Files selected for processing (18)
esignet-service/README.mdesignet-service/internal/clientmgmt/db/query.sqlesignet-service/internal/clientmgmt/db/query.sql.goesignet-service/internal/clientmgmt/handler.goesignet-service/internal/clientmgmt/jwk_validate.goesignet-service/internal/clientmgmt/model.goesignet-service/internal/clientmgmt/service.goesignet-service/internal/clientmgmt/service_test.goesignet-service/internal/clientmgmt/validate.goesignet-service/internal/engine/actors.goesignet-service/internal/engine/mosip/model.goesignet-service/internal/engine/mosip/mosip_authn.goesignet-service/postman/README.mdesignet-service/postman/embedder-local.environment.jsonesignet-service/postman/embedder-positive-flow.jsonesignet-service/scripts/client-mgmt-smoke.shesignet-service/scripts/oauth-smoke.shesignet-service/scripts/smoke-client-lib.sh
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
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
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
/client-mgmt/client(create/get/update/patch).8088, new base URL usage, v3 client-mgmt request/response envelopes, and revised smoke steps.