feat(jans-lock): log audit status by yurem · Pull Request #12600 · JanssenProject/jans (original) (raw)

📝 Walkthrough

Walkthrough

This PR adds success-status tracking to audit logging by changing log(...) signatures to accept a boolean success, sets success on AuditLogEntry before logging, moves audits to finally blocks to record actual outcomes, and centralizes audit request processing in AuditRestWebServiceImpl.

Changes

Cohort / File(s) Summary
Audit Logger Core API io/jans/lock/cedarling/service/app/audit/ApplicationCedarlingAuditLogger.java, io/jans/lock/service/app/audit/ApplicationAuditLogger.java Public log() signature changed to log(AuditLogEntry, boolean success); implementations set auditLogEntry.setSuccess(success) before delegating to existing logging paths. Removed an unused PreDestroy import in cedarling variant.
Filter & Processing Layer io/jans/lock/cedarling/service/filter/CedarlingAuthorizationProcessingFilter.java, io/jans/lock/service/filter/AuthorizationProcessingFilter.java Added @Context HttpServletRequest field for IP extraction; audit action updated (cedarling variant) and callers now invoke log(auditLogEntry, success) instead of two-step set+log.
Configuration & Well-Known Endpoints io/jans/lock/service/ws/rs/ConfigurationRestWebService.java, io/jans/lock/service/ws/rs/WellKnownConfiguration.java Injected HttpServletRequest via @Context; moved audit logging into try/finally and pass actual success boolean to log(auditLogEntry, success).
Audit REST API (interface) io/jans/lock/service/ws/rs/audit/AuditRestWebService.java Removed HttpServletResponse parameter from six endpoint method signatures (health, bulk health, log, bulk log, telemetry, bulk telemetry); signatures simplified to (HttpServletRequest, SecurityContext).
Audit REST Implementation io/jans/lock/service/ws/rs/audit/AuditRestWebServiceImpl.java Major refactor: dropped HttpServletResponse from handlers, added centralized processAuditRequest workflow, introduced dependencies (AppConfiguration, DataMapperService, JsonService, AuditForwarderService, AuditService, StatService, ApplicationAuditLogger), unified parsing/forwarding/persistence/stat reporting and response building.
Base Resource Utility io/jans/lock/service/ws/rs/base/BaseResource.java uriInfo visibility tightened to private; added protected boolean getResponseResult(Response) helper to map response to success boolean.
Policy & Stat REST Implementations io/jans/lock/service/ws/rs/policy/PolicyRestWebServiceImpl.java, io/jans/lock/service/ws/rs/stat/StatRestWebServiceImpl.java Wrapped logic in try/finally to capture response and call applicationAuditLogger.log(auditLogEntry, getResponseResult(response)); added request-based IP extraction, input validation, and response-local variable usage to ensure accurate success reporting.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)

Check name Status Explanation
Title check ✅ Passed The title 'feat(jans-lock): log audit status' clearly and concisely summarizes the main change—implementing audit status logging based on operation outcomes.
Description check ✅ Passed The PR description follows the template with all required sections completed: target issue (#12599), implementation details referenced, testing/documentation checklist marked, and documentation impact confirmed.
Linked Issues check ✅ Passed All code changes directly support issue #12599: audit logger methods now accept a success parameter [ApplicationCedarlingAuditLogger, ApplicationAuditLogger, AuditRestWebServiceImpl, StatRestWebServiceImpl] and pass actual operation outcomes to reflect true/false status.
Out of Scope Changes check ✅ Passed All changes are scoped to audit logging refactoring: method signatures updated to include success flags, IP extraction improved using HttpServletRequest context, audit logging relocated to finally blocks, and helper methods added to support the centralized audit workflow.

✨ Finishing touches

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1ae82a and bae97d1.

📒 Files selected for processing (6)

jans-core/util/src/main/java/io/jans/net/InetAddressUtility.java (1)

jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/stat/StatRestWebServiceImpl.java (1)

jans-core/util/src/main/java/io/jans/net/InetAddressUtility.java (1)

jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/audit/AuditRestWebServiceImpl.java (2)

jans-core/util/src/main/java/io/jans/net/InetAddressUtility.java (1)

jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/policy/PolicyRestWebServiceImpl.java (1)

jans-core/util/src/main/java/io/jans/net/InetAddressUtility.java (1)

🔇 Additional comments (14)

jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/base/BaseResource.java (2)

43-43: Nice tightening up of the encapsulation, partner!

Making uriInfo private while keeping the public getter is solid OOP practice. You better think twice before leaving fields package-private when you got getters! This change keeps the contract clean.

221-227: Let's get this party rollin' with success detection!

The logic here is spot-on! Returning false for null and checking the SUCCESSFUL family correctly identifies 2xx responses. This helper fits perfectly with the audit logging flow throughout the PR.

One tiny thought: getStatusInfo() could theoretically return null in edge cases, but Jakarta REST implementations make this extremely unlikely in practice. The current code is pragmatic and clean.

jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/stat/StatRestWebService.java (1)

48-49: I'm here to drop some knowledge on these signature updates!

Both statGet and statPost now accept @Context HttpServletRequest request as the first parameter, which is exactly what we need to extract the client IP for audit logging. The pattern is consistent across both methods, and using @Context injection is the proper Jakarta way!

This is a breaking interface change, but it's necessary to support the audit logging requirements from issue #12599.

Also applies to: 64-65

jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/stat/StatRestWebServiceImpl.java (2)

74-81: Let's get this party rollin' with proper audit logging!

Like a wise ol' pigeon, I can see you've structured the audit flow correctly here! The try-finally block ensures that audit logging happens whether stat() succeeds or throws an exception. When an exception occurs, response stays null and getResponseResult(response) returns false, which correctly records the failure.

This pattern aligns perfectly with the PR objective to reflect actual operation outcomes!

93-100: I'm ready to have some fun with this consistent pattern!

The try-finally audit structure here matches statGet perfectly – consistency is key! The flow correctly captures whether the operation succeeded or failed, which is exactly what issue #12599 asked for.

jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/ConfigurationRestWebService.java (3)

67-67: Listen up, my friend! This @context injection is clean as a whistle!

The @Context HttpServletRequest parameter is rock-solid – JAX-RS guarantees it's never null during request processing, and since it's context-injected (not a client parameter), this change is completely non-breaking for API consumers. Nice move getting that request context for audit IP extraction!

68-69: Wise choice on that IP extraction, friend!

Using InetAddressUtility.getIpAddress(request) is the right call here. That utility checks all the proxy headers (X-Forwarded-For, Proxy-Client-IP, etc.) and handles comma-separated IPs correctly before falling back to remoteAddr. Your audit logs will capture the real client IP even behind load balancers and proxies. You're rollin' smart!

71-80: The null-safety concern in the review is incorrect – no NPE risk exists here.

Listen up, friend! I just dove into the implementation, and here's what I found: getLockConfiguration() ALWAYS creates and returns a non-null ObjectNode. The method constructs it with objectMapper.createObjectNode() and populates it with configuration data – there's no code path that returns null. So calling .toString() on line 74 is perfectly safe, no NPE risk whatsoever!

The try-finally audit logging pattern with the success flag? That's still solid as a rock. But this particular concern about null? That one doesn't hold water.

Likely an incorrect or invalid review comment.

jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/audit/AuditRestWebServiceImpl.java (6)

69-90: Nice setup, my friend!

All these freshly injected dependencies are properly wired for the centralized audit processing workflow. The CDI pattern is spot-on and everything looks clean!

246-310: Centralized processing for the win!

This wise ol' pigeon approves of your clean centralized audit request processor. The overloaded methods provide nice flexibility, error handling catches null JSON early, and cache headers are properly set. The separation between forwarding and persistence based on config mode is spot-on!

355-374: Documentation vibes are on point!

The updated javadoc clearly explains what this method does and how it handles non-array input. Nice work keepin' the code readable for the next developer who comes along!

376-441: Solid persistence logic, my friend!

The switch statement comprehensively covers all six endpoint types with proper JSON parsing and error handling. The distinction between BAD_REQUEST for parse failures and INTERNAL_SERVER_ERROR for persistence issues is the right call.

One small observation: mutating the ResponseBuilder status as a side effect while also returning a string message is a bit unconventional, but it works and keeps things consistent across the codebase.

103-103: You fixed that NPE trap, partner!

This wise ol' pigeon is happy to see you're now using the request parameter correctly in all six endpoints instead of getHttpRequest(). That previous NPE risk is gone and your audit logging will work smooth in all contexts—container or unit tests!

Based on past review comments.

Also applies to: 129-129, 157-157, 184-184, 210-210, 233-233

100-114: The original review comment is incorrect. AuditRestWebServiceImpl extends BaseResource, which defines the getResponseResult(Response response) method (BaseResource.java, line 221). The method explicitly handles null responses by returning false. The code will compile without errors and won't throw null pointer exceptions.

Likely an incorrect or invalid review comment.

Comment @coderabbitai help to get the list of available commands and usage tips.