Conversation
|
🤖 [AI-generated] Hey @Dimfacion! 👋 Thanks a lot for opening PR #6313 — really appreciate the contribution! 🙏 I just had a quick look and I think the description could be enhanced a little to help reviewers get through it faster (it's a sizeable change — +212/−67 across 14 files). Nice job linking the issue with
No rush at all — totally fine to fill these in while the draft is open. Thanks again for contributing to OpenAEV! 🚀 |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (64.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #6313 +/- ##
============================================
+ Coverage 44.21% 44.25% +0.03%
Complexity 7431 7431
============================================
Files 2309 2309
Lines 63899 63909 +10
Branches 8498 8498
============================================
+ Hits 28256 28280 +24
+ Misses 33843 33806 -37
- Partials 1800 1823 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the audit-diff “context” naming from EntityDiffContext to AuditLogContext and introduces a new “audit significance” mechanism to suppress audit logging for endpoint/agent heartbeat-style updates that don’t materially change the endpoint state.
Changes:
- Renamed diff snapshot context usage from
EntityDiffContexttoAuditLogContextacross listeners, aspects, and tests. - Added
AuditSignificanceAware+ entity implementations (Endpoint,Agent) to compute “significant state” snapshots. - Added service-side suppression (
AuditLoggedService) and gated success-path audit logging viaAuditLogContext.isEnabled().
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openaev-model/src/main/java/io/openaev/database/model/Endpoint.java | Implements AuditSignificanceAware and provides significantState() including per-agent significant state. |
| openaev-model/src/main/java/io/openaev/database/model/Agent.java | Implements AuditSignificanceAware and provides significantState() with excluded non-significant fields. |
| openaev-model/src/main/java/io/openaev/database/audit/ModelBaseListener.java | Switches snapshot storage from EntityDiffContext to AuditLogContext and updates related docs. |
| openaev-model/src/main/java/io/openaev/database/audit/AuditSignificanceAware.java | New interface for computing “significant state” used to detect meaningful changes for audit. |
| openaev-model/src/main/java/io/openaev/database/audit/AuditLoggedService.java | New service helper interface to suppress audit logging when before/after significant state is equal. |
| openaev-model/src/main/java/io/openaev/database/audit/AuditLogContext.java | Renames context class and adds per-request/thread “enabled” flag for audit suppression. |
| openaev-model/src/main/java/io/openaev/annotation/AuditDiffTracked.java | Updates Javadoc reference to AuditLogContext. |
| openaev-api/src/test/java/io/openaev/utils/object/ObjectDiffUtilsTest.java | Updates test types from EntityDiffContext to AuditLogContext. |
| openaev-api/src/test/java/io/openaev/aop/audit_log/AuditDiffTrackedTest.java | Updates integration tests to consume snapshots from AuditLogContext. |
| openaev-api/src/test/java/io/openaev/aop/audit_log/AccessControlAuditLogAspectTest.java | Updates snapshot types/captors to AuditLogContext.EntitySnapshot. |
| openaev-api/src/main/java/io/openaev/utils/object/ObjectDiffUtils.java | Updates snapshot input type to AuditLogContext.EntitySnapshot. |
| openaev-api/src/main/java/io/openaev/service/EndpointService.java | Captures before/after endpoint significant state and suppresses audit logging for non-significant updates. |
| openaev-api/src/main/java/io/openaev/aop/audit_log/AuditLogger.java | Updates snapshot parameter type to AuditLogContext.EntitySnapshot. |
| openaev-api/src/main/java/io/openaev/aop/audit_log/AccessControlAuditLogAspect.java | Gates success-path audit logging with AuditLogContext.isEnabled() and updates snapshot capture types. |
Comments suppressed due to low confidence (1)
openaev-model/src/main/java/io/openaev/database/audit/AuditLogContext.java:120
- issue (blocking): The new
ENABLED_TLis cleared inclear(), butclearAfterTransactionCompletion()does not clear it. IfAuditLogContext.setEnabled(false)is used on a non-web thread, the disabled flag can leak to later transactions executed on the same thread and silently suppress audit logging beyond the intended scope.
Dimfacion
force-pushed
the
issue/163
branch
2 times, most recently
from
0ffd3dc to
84dd3db
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
openaev-model/src/main/java/io/openaev/database/audit/AuditLogContext.java:119
- issue (non-blocking):
AuditLogContext.clearAfterTransactionCompletion()clears the snapshot ThreadLocals but does not clearENABLED_TL. IfsetEnabled(false)is ever used in a non-web execution path, audit suppression can leak to subsequent transactions on the same reused thread.
Dimfacion
force-pushed
the
issue/163
branch
2 times, most recently
from
4195fc0 to
d38c0bb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
openaev-api/src/main/java/io/openaev/aop/audit_log/AccessControlAuditLogAspect.java:113
- issue (moderate):
AuditLogContext.setEnabled(false)is never reset totrueafter an@AccessControlinvocation. This can unintentionally suppress audit logging for subsequent@AccessControlactions executed later in the same request/thread.
if (isActive && isActionActive && AuditLogContext.isEnabled()) {
try {
String eventScope = LogUtils.getEventScope(action);
String eventStatus = LogUtils.getEventStatus(EventStatus.SUCCESS);
JsonNode resultNode = getOutputNode(result);
Dimfacion
force-pushed
the
issue/163
branch
2 times, most recently
from
bdf3c6c to
caebae9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
openaev-model/src/main/java/io/openaev/database/audit/AuditLogContext.java:120
- issue (blocking):
clearAfterTransactionCompletion()clears the snapshot ThreadLocals but does not clearENABLED_TL. IfAuditLogContext.setEnabled(false)is ever used outside an HTTP request (ThreadLocal storage), audit suppression can leak into subsequent operations on the same thread.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
openaev-model/src/main/java/io/openaev/database/audit/AuditLogContext.java:123
- issue (blocking): clearAfterTransactionCompletion() does not reset the thread-local audit enabled flag. If AuditLogContext.setEnabled(false) is ever used outside an HTTP request (ThreadLocal fallback), the flag can leak across subsequent tasks on the same thread and suppress unrelated audit logs.
Dimfacion
force-pushed
the
issue/163
branch
2 times, most recently
from
a484a18 to
caf2cda
Compare
| * @param objectMapper the ObjectMapper used to serialize the entity | ||
| * @return a map of significant field names to their current values | ||
| */ | ||
| default Map<String, Object> significantState(ObjectMapper objectMapper) { |
There was a problem hiding this comment.
question; What do you for @manytoone, @manytomany ect properties ?
There was a problem hiding this comment.
objectMapper.convertValue will use the jackson configuration linked to those fields so for instance if the field has the @JsonSerialize(using = MonoIdSerializer.class), we will replace it with an id
| @CreationTimestamp | ||
| private Instant createdAt = now(); | ||
|
|
||
| @AuditStateIgnore |
There was a problem hiding this comment.
question: Wha't the goal to use this annotation without implementing the AuditStateCapturable interface ?
There was a problem hiding this comment.
Endpoint extends Asset so it's needed to ignore this property when using significantState() on an Endpoint
| @@ -1,5 +1,6 @@ | |||
| package io.openaev.annotation; | |||
|
|
|||
| import io.openaev.database.audit.AuditLogContext; | |||
There was a problem hiding this comment.
Wow, import is needed to write a comment. I didn't know that.
There was a problem hiding this comment.
It's not needed but I don't like having the full name in the comment x) Do you feel it would be better to remove it ?
There was a problem hiding this comment.
I mean, it's not needed for openaev to compile but the IDE will yell at you
|
@Dimfacion I have one concern with the current AuditStateCapturable approach: objectMapper.convertValue(entity, Map) serializes the raw JPA entity, Could be good also to set-up some rules for next audit logging objects
|
|
As seen together @RomuDeuxfois, I've added a filter to avoid serializing some fields if they are with the correct annotation. That should make comparisons slimmer. Let me know what you think :) |
| @NotNull | ||
| private Instant createdAt = now(); | ||
|
|
||
| @AuditStateIgnore |
There was a problem hiding this comment.
nitpick: this can be enforced direclty in the AuditStatePropertyFilter
public class AuditStatePropertyFilter extends SimpleBeanPropertyFilter {
private static final Set ALWAYS_IGNORED = Set.of("createdAt", "updatedAt");
private static boolean isAuditStateValid(BeanPropertyWriter writer) {
return !ALWAYS_IGNORED.contains(writer.getName())
&& writer.getAnnotation(AuditStateIgnore.class) == null
}
}
There was a problem hiding this comment.
I'd rather have a declarative approach (let's ban magic) so that a dev wouldn't pull their hair out wondering why it's not logged even though the update date is changing. Wdyt ?
| ThreadLocal.withInitial(() -> false); | ||
|
|
||
| private EntityDiffContext() {} | ||
| private static final ThreadLocal<Boolean> ENABLED_TL = ThreadLocal.withInitial(() -> true); |
There was a problem hiding this comment.
nitpick: I would prefer not to add a new ThreadLocal property, since we have had a lot of issues with ThreadLocal in the past.
As I see it, we have two cases:
- From an API call: everything can be handled through request attributes.
- From a service call: at the service level, we can explicitly call the audit log service and decide whether the diff is significant or not. And if not, not call the audit log service.
What do you think?
There was a problem hiding this comment.
The thread local logic is used as a backup if we're not in an API call so I guess it's true that we could remove it since it shouldn't be happening outside of it
|
|
||
| // Suppress audit logging for heartbeat-only updates (no significant endpoint change) | ||
| Endpoint after = (Endpoint) saved.getAsset(); | ||
| suppressAuditIfUnchanged(before, after.significantState(objectMapper)); |
There was a problem hiding this comment.
nitpick: Explicit call, no magic. I’m starting to prefer this kind of approach.
|
As seen together @RomuDeuxfois, I've added a filter to avoid serializing some fields if they are with the correct annotation. That should make comparisons slimmer. Let me know what you think :) |
| Agent saved = agentService.createOrUpdateAgent(agent); | ||
|
|
||
| // Suppress audit logging for heartbeat-only updates (no significant endpoint change) | ||
| Endpoint after = (Endpoint) saved.getAsset(); | ||
| suppressAuditIfUnchanged(before, after.significantState(objectMapper)); |
There was a problem hiding this comment.
todo: We have an issue here.
This is the log I have
2026-06-29T15:58:19.771+02:00 [AUDIT] {
"id" : "199ffd03-b36a-41c3-bc74-1452d406c81e",
"timestamp" : "2026-06-29T13:58:19.638054Z",
"entity_type" : "Activity",
"created_at" : "2026-06-29T13:58:19.638054Z",
"event_type" : "mutation",
"event_status" : "success",
"event_access" : "extended",
"event_scope" : "create",
"user_id" : "a9d14910-34b6-4679-aa11-262a12ccf2cc",
"tenant_id" : "2cffad3a-0001-4078-b0e2-ef74274022c3",
"user_metadata" : {
"ip" : "0:0:0:0:0:0:0:1",
"user_email" : "9241fc76f00e9b502f4eeba27ff863f56e65c97cbb712f3589bec863118a6a3e",
"user_agent" : "openaev-agent/2.260623.0",
"x_forwarded_for" : null,
"session_id" : "5E488BE6B04B730CF1F3DBBF2C9091CA"
},
"request_metadata" : {
"url" : "http://localhost:8080/api/tenants/2cffad3a-0001-4078-b0e2-ef74274022c3/endpoints/register",
"method" : "POST",
"signature" : {
"method" : "io.openaev.rest.asset.endpoint.EndpointApi.upsertEndpoint",
"parameters" : {
"input" : "@RequestBody"
}
}
},
"context_data" : {
"entity_type" : "Agent",
"input" : {
"seenIp" : "0:0:0:0:0:0:0:1",
"elevated" : true,
"asset_name" : "LAPT-FILI-0067",
"asset_external_reference" : "6acc8c2a54dac178cc73c2eed06d23ab3ea8bb3f2bda6178d802a8ddbe3a1f62",
"endpoint_platform" : "Windows",
"endpoint_arch" : "x86_64",
"endpoint_ips" : [ "fe80::8070:739f:2d82:c37f", "2a02:8428:ae01:6101:fde4:68f8:3c63:5383", "2a02:8428:ae01:6101:38a1:f493:f5eb:dd5b", "fe80::d3c3:2bb9:fdde:34db", "192.168.1.219", "fe80::93d6:8689:7d65:aba0", "fe80::d033:45f2:208f:466f", "fe80::b316:d785:547b:18d", "fe80::dee3:70e6:7b20:e16", "172.18.144.1", "fe80::13d9:ffb0:3d33:95fa", "172.17.192.1" ],
"endpoint_hostname" : "LAPT-FILI-0067",
"endpoint_agent_version" : "2.260623.0",
"endpoint_mac_addresses" : "e56843542f05c938ed043df1b9a00b5753e14db71103b25e7b1ed046a3b64b20",
"agent_is_elevated" : true,
"agent_executed_by_user" : "azuread\romualdlemesle",
"agent_installation_mode" : "session-user",
"agent_installation_directory" : "C:\Users\RomualdLemesle\.openaev\OAEVAgent-Session-Administrator-azureadromualdlemesle",
"agent_service_name" : "OAEVAgent-Session"
},
"output" : {
"listened" : true,
"asset_id" : "551fd5c5-ed22-495e-94c4-002cc0fb0981",
"asset_type" : "Endpoint",
"asset_name" : "LAPT-FILI",
"asset_tags" : [ "3d7efede-19f0-4489-a50f-58e53fa43563" ],
"asset_created_at" : "2026-06-29T13:21:35.935649Z",
"asset_updated_at" : "2026-06-29T13:58:19.626383Z",
"endpoint_ips" : [ "fe80::b316:d785:547b:18d", "192.168.1.219", "fe80::d033:45f2:208f:466f", "fe80::13d9:ffb0:3d33:95fa", "172.17.192.1", "fe80::8070:739f:2d82:c37f", "fe80::93d6:8689:7d65:aba0", "2a02:8428:ae01:6101:38a1:f493:f5eb:dd5b", "172.18.144.1", "fe80::dee3:70e6:7b20:e16", "2a02:8428:ae01:6101:fde4:68f8:3c63:5383", "fe80::d3c3:2bb9:fdde:34db" ],
"endpoint_seen_ip" : "0:0:0:0:0:0:0:1",
"endpoint_hostname" : "lapt-fili-0067",
"endpoint_platform" : "Windows",
"endpoint_arch" : "x86_64",
"endpoint_mac_addresses" : "d2056229107e23ab6f9d93c837b20609529245bde7acf9e5977c7ef4b5530129",
"asset_agents" : [ {
"listened" : true,
"agent_id" : "c9d562ed-3583-42bf-a7a0-320141519911",
"agent_asset" : "551fd5c5-ed22-495e-94c4-002cc0fb0981",
"agent_privilege" : "admin",
"agent_deployment_mode" : "session",
"agent_executed_by_user" : "azuread\romualdlemes",
"agent_executor" : "2f9a0936-c327-4e95-b406-d161d32a2501",
"agent_version" : "2.260623.0",
"agent_external_reference" : "6acc8c2a54dac178cc73c2eed06d23ab3ea8bb3f2bda6178d802a8ddbe3a1f62",
"agent_last_seen" : "2026-06-29T13:56:18.776376Z",
"agent_created_at" : "2026-06-29T13:21:35.948156Z",
"agent_updated_at" : "2026-06-29T13:21:35.948156Z",
"agent_cleared_at" : "2026-06-29T13:21:35.948156Z",
"agent_active" : true
} ]
},
"message" : "creates Agent"
}
}
I have "entity_type" : "Agent", input is an agent but output is an Endpoint. That's weird no ?
There was a problem hiding this comment.
That's the way the API is designed for this endpoint. I agree that this is weird but it's out of scope IMO :
@PostMapping({ENDPOINT_URI + "/register", TENANT_ENDPOINT_URI + "/register"}) @AccessControl(actionPerformed = Action.CREATE, resourceType = ResourceType.AGENT) @Transactional(rollbackFor = Exception.class) public Endpoint upsertEndpoint(@Valid @RequestBody final EndpointRegisterInput input) throws IOException { input.setSeenIp(HttpReqRespUtils.getClientIpAddressIfServletRequestExist()); return this.endpointService.register(input, TenantContext.getCurrentTenant()); }
As you can see, the endpoint is returning an Endpoint but the resourceType is Agent
There was a problem hiding this comment.
Can you check if it's possible to align that ? It's really weird when I read the log.
Also, I add a new agent and my output don't contains it.
There was a problem hiding this comment.
@RomuDeuxfois As seen together, I made a few changes to the way the agent and endpoint are registered
Proposed changes
Testing Instructions
Related issues
Checklist