[backend] feat(audit-log): login/logout/SSO auth event hooks + tests (#5483)#5807
Open
a19836 wants to merge 11 commits into
Open
[backend] feat(audit-log): login/logout/SSO auth event hooks + tests (#5483)#5807a19836 wants to merge 11 commits into
a19836 wants to merge 11 commits into
Conversation
a19836
added
filigran team
6 tasks
a19836
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure authentication events (local login success/failure, SSO login success/failure, logout) are captured by the audit logging system via targeted hooks in auth-related components (since these flows bypass the AOP aspect).
Changes:
- Added explicit audit auth-event logging calls in
UserApi.login(), SSO success/failure handlers, and the Spring Security logout handler. - Refactored/extended log infrastructure: generic vs audit log enable flags, generic transport dispatcher naming, and added a log correlation UUID support path.
- Introduced object normalization/clean-up utilities and a new cache manager intended to speed up audit child-resource detection.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| openaev-model/src/main/java/io/openaev/config/EngineConfig.java | Adds default audit-log lifecycle config fields (retention/rollover). |
| openaev-api/src/main/resources/application.properties | Renames generic log enablement properties; updates audit log service enabled flag. |
| openaev-api/src/main/java/io/openaev/utils/object/ObjectSanitizationUtils.java | Removes prior sanitization utility (logic moved/merged into normalization). |
| openaev-api/src/main/java/io/openaev/utils/object/ObjectRedactionUtils.java | Adds SHA-256 hashing helper for redacting values (e.g., user email). |
| openaev-api/src/main/java/io/openaev/utils/object/ObjectNormalizationUtils.java | Adds normalization + stripping of insignificant values for stable audit diffs/logs. |
| openaev-api/src/main/java/io/openaev/utils/object/ObjectDiffUtils.java | Switches skip/empty-node helpers to ObjectNormalizationUtils and adds TODO. |
| openaev-api/src/main/java/io/openaev/utils/log/transport/GenericLogTransportUtils.java | Renames generic transport interface from the previous LogTransportUtils. |
| openaev-api/src/main/java/io/openaev/utils/log/transport/GenericFileLogTransportUtils.java | Updates to new generic log properties + interface name. |
| openaev-api/src/main/java/io/openaev/utils/log/transport/GenericConsoleLogTransportUtils.java | Updates to new generic log properties + interface name. |
| openaev-api/src/main/java/io/openaev/utils/log/transport/AuditConsoleLogTransportUtils.java | Minor comment change in audit console transport. |
| openaev-api/src/main/java/io/openaev/utils/log/dispatcher/GenericLogTransportDispatcherUtils.java | Renames dispatcher to use GenericLogTransportUtils list. |
| openaev-api/src/main/java/io/openaev/utils/HttpReqRespUtils.java | Adds HTTP method extraction fallback to thread context; request getter formatting changed. |
| openaev-api/src/main/java/io/openaev/service/LogService.java | Renames generic log flags/type, adds log UUID support, switches to normalization utility, and updates dispatchers. |
| openaev-api/src/main/java/io/openaev/security/SsoRefererAuthenticationSuccessHandler.java | Logs SSO login success audit event (provider from OAuth2 registration id). |
| openaev-api/src/main/java/io/openaev/security/SsoRefererAuthenticationFailureHandler.java | Logs SSO login failure audit event; constructor updated to accept audit logger. |
| openaev-api/src/main/java/io/openaev/rest/user/UserApi.java | Logs local login success/failure audit events. |
| openaev-api/src/main/java/io/openaev/rest/log/LogApi.java | Updates generic log API endpoint to new AuditLogType.GENERIC + log UUID param. |
| openaev-api/src/main/java/io/openaev/config/ThreadPoolTaskLoggerConfig.java | Captures HTTP method into thread context for async logging correlation. |
| openaev-api/src/main/java/io/openaev/config/security/OpenSamlConfig.java | Wires the new SSO success handler constructor with the audit logger. |
| openaev-api/src/main/java/io/openaev/config/CachingConfig.java | Registers a new Caffeine cache for audit child-resource detection. |
| openaev-api/src/main/java/io/openaev/config/cache/AuditChildResourceCacheManager.java | New cache manager intended to cache audit child-resource detection results. |
| openaev-api/src/main/java/io/openaev/config/AppSecurityConfig.java | Adds logout audit hook; wires SSO handlers with audit logger. |
| openaev-api/src/main/java/io/openaev/aop/audit_log/AuditResourceDetector.java | Delegates child resource detection to cache manager; adds extensive WARN logging/TODOs. |
| openaev-api/src/main/java/io/openaev/aop/audit_log/AuditRequestValidator.java | Updates audit enabled property key to openaev.audit-logs.service.enabled. |
| openaev-api/src/main/java/io/openaev/aop/audit_log/AccessControlAuditLogger.java | Propagates logUUID through audit logging calls; uses request-method extraction helper. |
| openaev-api/src/main/java/io/openaev/aop/audit_log/AccessControlAuditLogAspect.java | Generates correlation UUID per audited request and passes it through. |
Comments suppressed due to low confidence (6)
openaev-api/src/main/java/io/openaev/config/cache/AuditChildResourceCacheManager.java:59
- issue (blocking): The
@Cacheablekey only includes methodSignature/resourceType/parentResourceId, but the resolved value is a specific child resource (childId + snapshot). If multiple child IDs exist under the same parent, this cache can return the wrong child snapshot for later requests. Include the childId in the cache key (or avoid caching ChildResourceInfo/snapshots across requests) to prevent incorrect audit attribution.
@Cacheable(
value = "auditChildResource",
key = "#methodSignature + '|' + #resourceType + '|' + #parentResourceId",
unless = "#result == null")
public ChildResourceInfo resolveChildResource(
openaev-api/src/main/java/io/openaev/config/cache/AuditChildResourceCacheManager.java:75
- issue (blocking): Debug scaffolding is still present (TODO + multiple log.warn + System.out.println). This will spam logs in production and can leak identifiers. Please remove System.out.println, downgrade to debug where needed, and either fix the cache logic or remove the non-working TODO before merging.
//TODO AUDIT: cache not working bc parentResourceId == paramValue
log.warn("pathVariableValues {}", pathVariableValues);
log.warn("parentResourceId {}", parentResourceId);
for (String paramValue : pathVariableValues) {
if (paramValue == null || paramValue.equals(parentResourceId)) {
continue;
}
System.out.println("methodSignature " + methodSignature + "; paramValue " + paramValue + "; parentResourceId " + parentResourceId);
log.warn("methodSignature {}", methodSignature);
openaev-api/src/main/java/io/openaev/aop/audit_log/AuditResourceDetector.java:136
- issue (blocking): detectChildResource() contains WARN-level trace logs and a TODO indicating the implementation is not working ("childInfo is always null"). This should be fixed or the incomplete code removed before merging; otherwise audit child-resource detection will silently fail and logs will be noisy.
public ChildResourceInfo detectChildResource(ProceedingJoinPoint joinPoint, ResourceType resourceType, String parentResourceId) {
try {
MethodSignature sig = (MethodSignature) joinPoint.getSignature();
String methodSignature = sig.toLongString();
String[] pathVariableValues = extractPathVariableValues(sig, joinPoint.getArgs());
log.warn("start detectChildResource");
ChildResourceInfo childInfo = auditChildResourceCacheManager.resolveChildResource(methodSignature, resourceType, parentResourceId, pathVariableValues);
log.warn("detectChildResource {}", childInfo);
//TODO AUDIT: childInfo is always null and cache not working bc parentResourceId == paramValue
openaev-api/src/main/java/io/openaev/utils/object/ObjectNormalizationUtils.java:103
- issue (blocking): The JavaDoc for stripInsignificantValues() is indented as if it’s inside another block (starts mid-column). This will likely fail Spotless/google-java-format and reduces readability. Reformat the block so the JavaDoc aligns with the method declaration.
/**
* Recursively strips insignificant values (nulls, empty strings, empty arrays, false booleans)
* from a {@link JsonNode} tree. Also removes fields listed in {@link #DIFF_SKIP_FIELDS}. This
* keeps create-event audit entries concise — only meaningful, non-default values are logged.
*
* <p>For arrays, each element is cleaned recursively (but elements are never removed, to preserve
* positional semantics). For objects, fields whose cleaned value is insignificant are dropped.
*
* @return a new, cleaned copy of the tree — the original is never mutated
*/
public JsonNode stripInsignificantValues(JsonNode node) {
openaev-api/src/main/java/io/openaev/utils/HttpReqRespUtils.java:160
- issue (blocking): The getCurrentRequest() method is mis-indented (extra leading spaces) which is inconsistent with the rest of the file and likely to fail Spotless. Please run formatting on this section.
public static HttpServletRequest getCurrentRequest() {
try {
ServletRequestAttributes attrs = (ServletRequestAttributes) RequestContextHolder.getRequestAttributes();
return attrs != null ? attrs.getRequest() : null;
} catch (Exception e) {
return null;
openaev-api/src/main/java/io/openaev/rest/user/UserApi.java:81
- issue: UserApi.login() now emits an audit auth event but still writes a UserEventService DB event (login success/failure). If the intent is to migrate to audit logging (per #5483), keeping both will duplicate events and complicate deprecation. Consider removing the UserEventService calls here (or clearly documenting why both are needed).
if (userService.isUserPasswordValid(user, input.getPassword())) {
userService.createUserSession(user);
userEventService.createLoginSuccessEvent(user);
accessControlAuditLogger.logAuthEvent("login", "success", "local", null, null);
return user;
}
}
userEventService.createLoginFailedEvent("local login", BadCredentialsException.class.getSimpleName());
accessControlAuditLogger.logAuthEvent("login", "error", "local", BadCredentialsException.class.getSimpleName(), null);
Comment on lines
+239
to
+241
| openaev.generic-logs.service.enabled=true | ||
| openaev.generic-logs.console.enabled=true | ||
| openaev.generic-logs.file.enabled=true |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (3)
openaev-api/src/main/java/io/openaev/config/AppSecurityConfig.java:130
- issue (blocking): the logout handler logs a "logout"/"success" auth event even when
authenticationis null (e.g., anonymous user calling/logout, expired session, etc.). This can produce misleading audit events; consider skipping whenauthentication == nullor loggingevent_status="error"(and/orprovider) based on the actual authentication state.
.addLogoutHandler(
(request, response, authentication) -> {
try {
accessControlAuditLogger.logAuthEvent(
"logout", "success", null, null, null);
openaev-api/src/main/java/io/openaev/rest/user/UserApi.java:82
- issue (blocking):
UserEventServiceis still used to create login failure events. If the intention is to replace legacy user_events with audit auth events (per issue #5483), this call should be removed/migrated to avoid duplicate records and to unblock removing the legacy tables.
userEventService.createLoginFailedEvent(
"local login", BadCredentialsException.class.getSimpleName());
accessControlAuditLogger.logAuthEvent(
"login", "error", "local", BadCredentialsException.class.getSimpleName(), null);
openaev-api/src/main/java/io/openaev/rest/user/UserApi.java:82
- todo (blocking): new behavior (audit auth event emission) isn’t covered by existing
UserApiTestlogin assertions. Add a test that mocksAccessControlAuditLogger(orLogService) and verifieslogAuthEvent("login", "success", "local", ...)andlogAuthEvent("login", "error", "local", ...)are called for the corresponding login outcomes (see testing.instructions.md / CodingGuidelineID:1000004).
accessControlAuditLogger.logAuthEvent("login", "success", "local", null, null);
return user;
}
}
userEventService.createLoginFailedEvent(
"local login", BadCredentialsException.class.getSimpleName());
accessControlAuditLogger.logAuthEvent(
"login", "error", "local", BadCredentialsException.class.getSimpleName(), null);
Comment on lines
+35
to
+39
| if (authentication instanceof OAuth2AuthenticationToken oauth2Token) { | ||
| provider = oauth2Token.getAuthorizedClientRegistrationId(); | ||
| } | ||
|
|
||
| accessControlAuditLogger.logAuthEvent("login", "success", provider, null, null); |
Comment on lines
34
to
+38
| userEventService.createLoginFailedEvent( | ||
| request.getRequestURI(), exception.getClass().getSimpleName()); | ||
|
|
||
| accessControlAuditLogger.logAuthEvent( | ||
| "login", "error", request.getRequestURI(), exception.getClass().getSimpleName(), null); |
Comment on lines
34
to
36
| userEventService.createLoginFailedEvent( | ||
| request.getRequestURI(), exception.getClass().getSimpleName()); | ||
|
|
Comment on lines
+73
to
+75
| userService.createUserSession(user); | ||
| userEventService.createLoginSuccessEvent(user); | ||
| accessControlAuditLogger.logAuthEvent("login", "success", "local", null, null); |
Comment on lines
+32
to
+39
| // Audit: log SSO login success | ||
| try { | ||
| String provider = "sso"; | ||
| if (authentication instanceof OAuth2AuthenticationToken oauth2Token) { | ||
| provider = oauth2Token.getAuthorizedClientRegistrationId(); | ||
| } | ||
|
|
||
| accessControlAuditLogger.logAuthEvent("login", "success", provider, null, null); |
Comment on lines
34
to
+39
| userEventService.createLoginFailedEvent( | ||
| request.getRequestURI(), exception.getClass().getSimpleName()); | ||
|
|
||
| accessControlAuditLogger.logAuthEvent( | ||
| "login", "error", request.getRequestURI(), exception.getClass().getSimpleName(), null); | ||
|
|
Comment on lines
120
to
+134
| .logout( | ||
| logout -> | ||
| logout | ||
| // Audit Log: audit handler fires first, then Spring Security's built-in | ||
| // SecurityContextLogoutHandler | ||
| // invalidates the session and clears cookies | ||
| .addLogoutHandler( | ||
| (request, response, authentication) -> { | ||
| try { | ||
| accessControlAuditLogger.logAuthEvent( | ||
| "logout", "success", null, null, null); | ||
| } catch (Exception e) { | ||
| // Never block the logout flow | ||
| } | ||
| }) |
a19836
changed the title
… didn't work the before (#5483)
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
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.
Authentication events (login, logout, SSO) do not flow through the AOP aspect. They are captured via targeted hooks in existing auth components. Ensure all auth events are captured correctly.
Proposed changes
Testing Instructions
Related issues