Repository navigation
Added client caching and logger enhancements - #2180
Merged
Merged
Conversation
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
WalkthroughClient management now uses runtime-store caching with configurable TTL and invalidation. The service also adds structured JSON logging, correlation/access middleware, dynamic OAuth scope claims, endpoint scope enforcement, and diagnostic logs across startup and providers. ChangesRuntime, observability, and authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CorrelationID
participant ScopeMiddleware
participant Mux
participant AccessLog
Client->>CorrelationID: Send request
CorrelationID->>ScopeMiddleware: Attach trace ID
ScopeMiddleware->>Mux: Validate token and mapped scope
Mux-->>AccessLog: Return response
AccessLog-->>Client: Return response and access record
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
esignet-service/internal/security/scope_middleware.go (1)
55-62: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftReplace hardcoded scope validation with dynamic configuration.
Hardcoding the
"test"scope in the security middleware bypasses proper authorization checks. This will either improperly block valid users or grant unauthorized access to sensitive endpoints, which directly violates MOSIP compliance and access control standards.Please implement the TODO and resolve the required scope dynamically from
config.SecurityConfig.ScopeMapping.🤖 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/security/scope_middleware.go` around lines 55 - 62, Replace the hardcoded "test" scope check in the scope middleware with validation derived from config.SecurityConfig.ScopeMapping. Resolve the required scope for the current request or endpoint, pass that value to claimHasScope, and use it in the warning and forbidden error messages while preserving the existing rejection behavior.
🤖 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/service.go`:
- Around line 309-318: The client lookup flow around GetClient and cacheRow must
cache sql.ErrNoRows results using the cache’s established negative-entry
mechanism, with a short TTL or not-found marker, before returning
ErrClientNotFound. Preserve existing positive-result caching and error handling
for other database failures.
- Around line 363-370: Update Service.invalidateCache to return the cache
deletion error instead of logging and swallowing it, while preserving the
no-cache early return. In esignet-service/internal/clientmgmt/service.go lines
193-194, make UpdateClient check and propagate this error; likewise check and
propagate it in PatchClient at lines 299-301 so both operations fail when
invalidation fails.
In `@esignet-service/internal/engine/actor_provider.go`:
- Around line 60-62: Rename the local pkceRequired boolean to isPKCERequired in
both affected sections of actor_provider.go (lines 60-62 and 107-109), and
update all references to that local value while preserving pkceRequired as the
map-key constant.
- Around line 240-249: Update getAllowedScopes to sort the keys collected from
standardScopeClaims before returning them, ensuring deterministic base-scope
ordering. Replace the direct []string assertion for
additionalConfig[allowedAuthorizationScopes] with handling that accepts
JSON-decoded []any values and converts valid string elements into []string,
while preserving support for []string and avoiding dropped authorization scopes.
In `@esignet-service/internal/httpmiddleware/accesslog.go`:
- Around line 21-41: Update AccessLog to sanitize the request URI before logging
it, replacing the raw r.RequestURI value passed to applog.String with
sanitizeURI(r.RequestURI). Implement or reuse sanitizeURI so sensitive query
parameters including state, nonce, code, login_hint, and id_token_hint are
redacted while preserving the rest of the URI.
- Around line 43-58: Update statusRecorder to forward the underlying
http.ResponseWriter’s http.Flusher and http.Hijacker capabilities, delegating
Flush and Hijack calls to the wrapped writer so streaming and protocol-upgrade
handlers retain their optional interfaces.
In `@esignet-service/internal/log/log.go`:
- Around line 127-160: Update all six Logger methods—Debug, Info, Warn, Error,
Fatal, and Access—to call convertFields on the caller-provided fields before
appending the level field. Append the converted level field only to the newly
created slice, preserving each method’s existing logging behavior and Fatal
process exit.
---
Outside diff comments:
In `@esignet-service/internal/security/scope_middleware.go`:
- Around line 55-62: Replace the hardcoded "test" scope check in the scope
middleware with validation derived from config.SecurityConfig.ScopeMapping.
Resolve the required scope for the current request or endpoint, pass that value
to claimHasScope, and use it in the warning and forbidden error messages while
preserving the existing rejection behavior.
🪄 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: 86ce4c18-003e-4448-87d5-7313df2acaf6
📒 Files selected for processing (35)
esignet-service/cmd/esignet/main.goesignet-service/data/deployment.yamlesignet-service/internal/clientmgmt/handler.goesignet-service/internal/clientmgmt/handler_test.goesignet-service/internal/clientmgmt/service.goesignet-service/internal/clientmgmt/service_test.goesignet-service/internal/config/app.goesignet-service/internal/consentmgmt/model.goesignet-service/internal/consentmgmt/model_test.goesignet-service/internal/consentmgmt/service.goesignet-service/internal/consentmgmt/service_test.goesignet-service/internal/consentmgmt/util.goesignet-service/internal/consentmgmt/util_test.goesignet-service/internal/engine/actor_provider.goesignet-service/internal/engine/actor_provider_test.goesignet-service/internal/engine/authz_provider.goesignet-service/internal/engine/consent_provider.goesignet-service/internal/engine/consent_provider_test.goesignet-service/internal/engine/design_provider.goesignet-service/internal/engine/executors.goesignet-service/internal/engine/flow_provider.goesignet-service/internal/engine/i18n_provider.goesignet-service/internal/engine/mock/authenticator_test.goesignet-service/internal/engine/mock/init.goesignet-service/internal/engine/mosip/init.goesignet-service/internal/engine/sunbird/init.goesignet-service/internal/httpmiddleware/accesslog.goesignet-service/internal/httpmiddleware/accesslog_test.goesignet-service/internal/httpmiddleware/correlationid.goesignet-service/internal/httpmiddleware/correlationid_test.goesignet-service/internal/log/log.goesignet-service/internal/log/log_test.goesignet-service/internal/security/jwks.goesignet-service/internal/security/requesttime.goesignet-service/internal/security/scope_middleware.go
sacrana0
reviewed
Jul 20, 2026
zesu22
reviewed
Jul 20, 2026
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/httpmiddleware/accesslog.go`:
- Around line 62-79: Add an Unwrap method to statusRecorder that returns its
wrapped http.ResponseWriter, allowing http.ResponseController to inspect the
underlying writer’s actual capabilities. Keep the existing Flush and Hijack
delegation unchanged.
🪄 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: 820fd0fc-7f8d-4556-8c97-4f956b1afe32
📒 Files selected for processing (14)
esignet-service/data/deployment.yamlesignet-service/internal/clientmgmt/service.goesignet-service/internal/clientmgmt/service_test.goesignet-service/internal/consentmgmt/service.goesignet-service/internal/consentmgmt/util.goesignet-service/internal/engine/actor_provider.goesignet-service/internal/engine/actor_provider_test.goesignet-service/internal/httpmiddleware/accesslog.goesignet-service/internal/httpmiddleware/accesslog_test.goesignet-service/internal/httpmiddleware/correlationid.goesignet-service/internal/httpmiddleware/correlationid_test.goesignet-service/internal/log/log.goesignet-service/internal/security/scope_middleware.goesignet-service/internal/security/scope_middleware_test.go
This was referenced Aug 7, 2026
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
3 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 by CodeRabbit