AAE-23566 Fix Audit Event Message Don't Follow Schema #1496
Conversation
revatilimaye
changed the title
|
Hello @revatilimaye please rebase over #1516 once it's merged if you have more changes coming (not needed if you merge right away), thanks! |
|
| @@ -131,5 +131,9 @@ | |||
| <artifactId>spring-boot-configuration-processor</artifactId> | |||
| <optional>true</optional> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.activiti.cloud</groupId> | |||
| <artifactId>activiti-cloud-services-common-security</artifactId> | |||
There was a problem hiding this comment.
We should not use the security dependency inside the events module. This is will put Spring Security and Activiti Security classes on the application classpath, which may have the consequences by including transitive dependencies downstream.
| @@ -51,7 +55,7 @@ public void onEvent(ProcessCreatedEvent event) { | |||
| .ifPresent(details -> | |||
| runtimeService.addUserIdentityLink( | |||
| event.getEntity().getId(), | |||
| principalIdentityProvider.getUserId(principal), | |||
| identityService.findUserByName(event.getEntity().getInitiator()).getId(), | |||
There was a problem hiding this comment.
It is not a good idea to use the identity service to enrich the identity link with initiator user guid, which will make an external rest api call to identity provider inside engine transaction. It will cause a delay for every new process instance which will impact the performance of the system in a negative way.
Besides, not every process instance will have an initiator, because it may be created by system on timer or message events. In this case, the initiator will be null and it will result in NPE.
| @MockBean | ||
| private ClientRegistrationRepository clientRegistrationRepository; | ||
|
|
||
| @MockBean | ||
| private JwtAccessTokenProvider jwtAccessTokenProvider; | ||
|
|
||
| @MockBean | ||
| private JwtDecoder jwtDecoder; | ||
|
|
||
| @MockBean | ||
| private IdentityService identityService; | ||
|
|
There was a problem hiding this comment.
We should avoid using mock beans in integration tests.
|
|
||
| @MockBean | ||
| private IdentityService identityService; |
There was a problem hiding this comment.
Let's not use mock beans in ITs.
| post("/v1/process-instances") | ||
| .contentType(APPLICATION_JSON) | ||
| .content(mapper.writeValueAsString(cmd)) | ||
| .with(csrf()) |
There was a problem hiding this comment.
What is the reason to add CSRF for REST API calls test? The Activiti Runtime Rest API never had the constraint to have CSRF token in cookies..
| <dependency> | ||
| <groupId>org.springframework.security</groupId> | ||
| <artifactId>spring-security-test</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Why this dependency is needed?
| .containsOnly(tuple("hruser", expectedActor.getBytes())); | ||
| .containsOnly(tuple(id, expectedActor.getBytes())); |
There was a problem hiding this comment.
This change affects the semantic meaning of the audit event that switches the user name with user guid. It will cause a breaking change downstream. It is better to use a new identity link record with another type to enrich the process instance context with new information from the provided principal oauth access token and use it to map to audit specific event schema dowstream.
Issue link:https://hyland.atlassian.net/browse/AAE-23566