PMS Integration: client-mgmt create endpoint.#1919
Open
SajidMannikeri17 wants to merge 1 commit into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 32 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
WalkthroughThis PR adds a client-management create endpoint with scope-based JWT authorization: a JWKS-backed Verifier, scope middleware, JWK parsing + deterministic hashing, DB persistence (ClientRepo), request validation with an embedded/override JSON Schema, route wiring in server/main, and comprehensive tests. ChangesClient Management with Auth
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 6
🤖 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/.env.example`:
- Around line 43-52: The .env example currently leaves AUTH_JWKS_URI empty which
results in an unprotected default; change to fail-closed by requiring an
explicit opt-out variable (e.g., AUTH_DISABLED=true) to disable JWT verification
and update the comments for AUTH_JWKS_URI and AUTH_ISSUER to include a prominent
security warning (e.g., "⚠️ SECURITY: empty AUTH_JWKS_URI runs WITHOUT
authentication — set AUTH_JWKS_URI/AUTH_ISSUER or set AUTH_DISABLED=true to
explicitly disable auth") and document the required CI/production validation;
also update any startup logic that reads AUTH_JWKS_URI/AUTH_ISSUER to treat an
empty value as a configuration error unless AUTH_DISABLED is explicitly true.
In `@esignet-service/go.mod`:
- Around line 3-42: Update the vulnerable dependencies and Go version in go.mod:
bump the module version for golang.org/x/crypto from v0.33.0 to at least v0.35.0
(prefer v0.45.0) and bump golang.org/x/net from v0.34.0 to at least v0.7.0, then
run go get/boost module tidy to update transitive deps; also consider updating
the go directive from 1.23.0 to the current stable (e.g., 1.26.3) if your
CI/tooling supports it.
In `@esignet-service/internal/auth/verifier.go`:
- Around line 119-131: The extractScopes function's documentation promises an
empty slice for missing/invalid scope but it currently returns nil; update
extractScopes (the function taking tok jwt.Token) so all early-return paths
return an empty []string{} instead of nil (when tok.Get("scope") is missing,
when the claim isn't a string, or when the string is empty) and keep the final
return using strings.Fields(s) for a non-empty string to satisfy the documented
contract.
- Around line 97-102: Remove the manual exp-presence check in the token
validation path (the branch that inspects tok.Expiration().IsZero()) and rely on
jwt.WithValidate(true) in the opts slice to enforce time-claim (exp) validation;
delete the guard and any early-return error tied to missing exp. For
extractScopes, either update its doc comment to state it may return nil when
scope is absent or change the function to return an empty slice (e.g.,
[]string{}) instead of nil when scope is missing/invalid so the contract matches
the comment; adjust callers if they assume non-nil. Ensure references: remove
the tok.Expiration().IsZero() check in verifier.go and update the extractScopes
function/documentation accordingly.
In `@esignet-service/internal/db/client_repo.go`:
- Around line 72-87: ClientRepo.Insert currently dereferences the input
parameter row (the *ClientDetailRow) before checking for nil which can cause a
nil-pointer panic; update the Insert method to early-return a descriptive error
when row == nil (e.g., validate at start of ClientRepo.Insert), and only call
r.pool.Exec with row fields after that nil check so r.pool.Exec(...) is never
invoked with a nil dereference.
In `@esignet-service/internal/handler/client_mgmt.go`:
- Around line 343-351: The current schema fetch reads the entire HTTP body with
io.ReadAll(resp.Body) which can exhaust memory; change it to read with a size
limit by wrapping resp.Body in an io.LimitReader (or io.LimitedReader) and a
defined max bytes constant (e.g., maxSchemaSize = 10<<20) before calling
io.ReadAll (or ioutil.ReadAll) so the read stops at the limit and returns an
error if exceeded; update the code around the HTTP response handling (the block
using http.DefaultClient.Do(req), resp.Body.Close(), resp.StatusCode check, and
io.ReadAll) to use the limited reader and return a clear "schema too large" or
"response exceeds limit" error when the limit is hit.
🪄 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: cc33a062-40b7-4743-ba50-859059f31685
⛔ Files ignored due to path filters (1)
esignet-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
esignet-service/.env.exampleesignet-service/cmd/esignet/main.goesignet-service/go.modesignet-service/internal/auth/verifier.goesignet-service/internal/auth/verifier_test.goesignet-service/internal/cache/redis.goesignet-service/internal/config/config.goesignet-service/internal/config/config_test.goesignet-service/internal/db/client_repo.goesignet-service/internal/db/client_repo_test.goesignet-service/internal/db/postgres_test.goesignet-service/internal/handler/client_mgmt.goesignet-service/internal/handler/client_mgmt_test.goesignet-service/internal/handler/schemas/additional_config.schema.jsonesignet-service/internal/middleware/require_scope.goesignet-service/internal/middleware/require_scope_test.goesignet-service/internal/server/server.goesignet-service/pkg/jwk/jwk.goesignet-service/pkg/jwk/jwk_test.go
SajidMannikeri17
force-pushed
the
feat-client-mgmt-create
branch
from
e3a402c to
ef7945d
Compare
Collaborator
|
IMO we should follow thunderid packaging style. There should be clientmgmt package that handles all things related to client management. It will contain the handler, dbstore, service, additional_config schema etc. @SajidMannikeri17 |
sacrana0
reviewed
May 26, 2026
Collaborator
There was a problem hiding this comment.
This is only whitespace change.
Collaborator
Author
There was a problem hiding this comment.
Added while formatting the code.
sacrana0
reviewed
May 26, 2026
Collaborator
There was a problem hiding this comment.
Check if it is possible to use the validator library to validate additionalConfig field as well instead of jsonschema.
Collaborator
Author
There was a problem hiding this comment.
validator/v10 and jsonschema do different jobs — one checks Go struct fields, the other checks free-form JSON blobs. Using jsonschema for validating the additional Config is a cleaner approach.
sacrana0
reviewed
May 26, 2026
Comment on lines
+65
to
+82
| // ── 6. Client-management dependencies ──────────────────────────────────── | ||
| // additionalConfig schema: embedded by default; override URL when set. | ||
| var additionalCfgSchema *jsonschema.Schema | ||
| if cfg.Client.AdditionalConfigSchemaURL == "" { | ||
| additionalCfgSchema, err = handler.LoadEmbeddedAdditionalConfigSchema() | ||
| if err != nil { | ||
| log.Error("load embedded additionalConfig schema failed", | ||
| slog.String("error", err.Error()), | ||
| ) | ||
| os.Exit(1) | ||
| } | ||
| log.Info("additionalConfig schema loaded (embedded)") | ||
| } else { | ||
| additionalCfgSchema, err = handler.LoadAdditionalConfigSchema(ctx, cfg.Client.AdditionalConfigSchemaURL) | ||
| if err != nil { | ||
| log.Error("load additionalConfig schema failed", | ||
| slog.String("url", cfg.Client.AdditionalConfigSchemaURL), | ||
| slog.String("error", err.Error()), |
Collaborator
There was a problem hiding this comment.
Let's not clutter the main.go with the client-mgmt specific code. clientmgmt package will own the whole related code.
sacrana0
reviewed
May 26, 2026
| module github.com/mosip/esignet | ||
|
|
||
| go 1.23 | ||
| go 1.25.0 |
Collaborator
There was a problem hiding this comment.
Let's move to 1.26 go version.
SajidMannikeri17
force-pushed
the
feat-client-mgmt-create
branch
from
ef7945d to
f962884
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/cmd/esignet/main.go`:
- Around line 101-103: Startup currently continues when AUTH_JWKS_URI is empty
(log.Warn), leaving routes unprotected; change this check so that if
AUTH_JWKS_URI is unset and an explicit opt-out env var (e.g., AUTH_DISABLE=true)
is not present, the process logs an error and exits non-zero; if AUTH_DISABLE is
true, keep the existing warning but make it clear dev-insecure mode is enabled.
Locate the check around the log.Warn call that references AUTH_JWKS_URI in
main.go, read AUTH_DISABLE (or a similarly named flag) from the environment, and
implement the conditional: if jwks == "" && AUTH_DISABLE != "true" then
log.Error(...context...) and os.Exit(1); else if jwks == "" && AUTH_DISABLE ==
"true" keep log.Warn but include the opt-out mention.
In `@esignet-service/internal/handler/client_mgmt.go`:
- Around line 449-453: The log currently prints the raw schemaURL which may
contain secrets; update the code after LoadAdditionalConfigSchema so it redacts
sensitive parts of schemaURL before logging (e.g., remove userinfo and sensitive
query parameters or replace with "<redacted>" using net/url parsing). Replace
the slog.String("url", schemaURL) argument with a redacted string variable
(e.g., redactedURL) and keep the log.Info call otherwise unchanged; reference
LoadAdditionalConfigSchema, schemaURL, and the log.Info/slog.String call to
locate the change.
- Around line 536-541: After decoding env with dec := json.NewDecoder(r.Body)
and dec.Decode(&env), ensure there is no trailing JSON by attempting a second
decode into an empty variable (e.g., var extra json.RawMessage or var empty
struct{}) and verifying it returns io.EOF; if the second decode does not return
io.EOF, call writeEnvelope(w, log, newCreateErrorEnvelope(errInvalidInput)) and
return. Keep dec.DisallowUnknownFields() in place and apply this check
immediately after the initial dec.Decode(&env) success path so requests with
trailing data are rejected.
- Around line 213-225: validateRedirectURL currently accepts malformed HTTP(S)
redirects like "https:/callback" because it only rejects when Host, Opaque and
Path are all empty; update it so that when u.Scheme is "http" or "https" you
require a non-empty u.Host (authority) and reject if u.Host == "" (and continue
rejecting u.Fragment != "" as before). Locate the logic in validateRedirectURL
and add the scheme-specific host check so HTTP(S) redirect URIs without an
authority are rejected.
In `@esignet-service/internal/handler/schemas/additional_config.schema.json`:
- Around line 59-61: The schema property consent_expire_in_mins currently uses
"type": "number" allowing fractional minutes; change it to an integer-based
schema by replacing "type": "number" with "type": "integer" (or keep "number"
and add "multipleOf": 1) and keep the existing "minimum": 10 to ensure only
whole-minute values (>=10) are valid; update the schema entry for
consent_expire_in_mins accordingly.
🪄 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: 1050a451-d67d-4646-a462-e38c444ecb28
⛔ Files ignored due to path filters (1)
esignet-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
esignet-service/.env.exampleesignet-service/cmd/esignet/main.goesignet-service/go.modesignet-service/internal/auth/verifier.goesignet-service/internal/auth/verifier_test.goesignet-service/internal/cache/redis.goesignet-service/internal/config/config.goesignet-service/internal/config/config_test.goesignet-service/internal/db/client_repo.goesignet-service/internal/db/client_repo_test.goesignet-service/internal/db/postgres_test.goesignet-service/internal/handler/client_mgmt.goesignet-service/internal/handler/client_mgmt_test.goesignet-service/internal/handler/schemas/additional_config.schema.jsonesignet-service/internal/middleware/require_scope.goesignet-service/internal/middleware/require_scope_test.goesignet-service/internal/server/server.goesignet-service/pkg/jwk/jwk.goesignet-service/pkg/jwk/jwk_test.go
Signed-off-by: Sajid Mannikeri <sajid.mannikeri@ad.infosys.com>
SajidMannikeri17
force-pushed
the
feat-client-mgmt-create
branch
from
f962884 to
ef8e62f
Compare
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
2 participants
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
Ports the OIDC client-management
POST /v1/esignet/client-mgmt/clientendpoint todevelop-go, gated by scope-based JWT authorization. PMS can now create OIDC clients against the Go service.What's in this PR
Endpoint —
POST /v1/esignet/client-mgmt/clientgo-playground/validator/v10with custom tags for OIDC enums, id format, redirect URLs, and not-blank semanticsadditionalConfigvalidated against a JSON Schema bundled in the binary; override viaCLIENT_ADDITIONAL_CONFIG_SCHEMA_URLif neededpublicKeyparsed as a JWK;public_key_hashcomputed as SHA-256 ofn(RSA) orx||y(EC)client_detailtable — no schema changes, no migrationScope-based JWT auth
internal/auth.Verifier— validates signature, issuer, audience, expiry against a cached JWKSinternal/middleware.RequireScope— extracts theAuthorization: Bearertoken, enforces the scope claimadd_oidc_clientAUTH_JWKS_URI. When unset, the route ships unprotected and the service logs a loud startup warningConfiguration
.env.example. The two operator-critical ones areAUTH_JWKS_URIandAUTH_ISSUER; everything else has sensible defaultsTests
internal/handlerinternal/middlewareinternal/dbinternal/authpkg/jwkinternal/serverinternal/configRefs #1888
Summary by CodeRabbit
New Features
Tests