[CAM-6431] UserOperationLogManager overwrites userId set on UserOperationLogContext Created: 19/Jul/16  Updated: 11/Aug/16  Resolved: 25/Jul/16

Status: Closed
Project: camunda BPM
Component/s: engine
Affects Version/s: 7.5.1
Fix Version/s: 7.6.0, 7.5.4, 7.6.0-alpha3

Type: Bug Report Priority: L3 - Default
Reporter: Jan Galinski Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: 0 minutes
Time Spent: Not Specified
Original Estimate: 0 minutes


 Description   

I am using CommandContext.getOperationLOgManager() to write custom Operation Log entries.

However, though I set a UserId on the OpLogContext.setUserId, it gets overwritten in

protected void fireUserOperationLog(final UserOperationLogContext context) {
    context.setUserId(getAuthenticatedUserId());

    HistoryEventProcessor.processHistoryEvents(new HistoryEventProcessor.HistoryEventCreator() {
      @Override
      public List<HistoryEvent> createHistoryEvents(HistoryEventProducer producer) {
        return producer.createUserOperationLogEvents(context);
      }
    });
  }

This is not the desired behavior since we do not use IdentityService functions. I guess a simple and valid fix would be to set the userId only when it has not been set before:

if (context.getUserId() == null) {
    context.setUserId(getAuthenticatedUserId());
}


 Comments   
Comment by Thorben Lindhauer [ 19/Jul/16 ]

Hi Jan,

This is not a bug since writing custom user operation log entries is not an API feature. Thus, I'll change the ticket type to Task.

Feel free to provide a pull request to change this behavior. However, we cannot really guarantee that the code change remains in place in future versions, again because API compatibility is the frame in which we can make code changes.

Cheers,
Thorben

Comment by Jan Galinski [ 19/Jul/16 ]

I provided a PR including a test: https://github.com/camunda/camunda-bpm-platform/pull/236

I think it is a cleaner setup, because although it is now public api, when I see

commandContext.getOperationLogManager.logUserOperation(userOperationLogContext)

and I pass a context instance, I assume that this context is logged ... and not modified.

I had some trouble to figure out how this works (because when you are not logged in on the identityService, a userId you set manually is overwritten by null ... and then the operation log entry is not written unless you configure the restriction for logs without userIds ....
This can be easier. And maybe it could become part of the public api someday?

Comment by Askar Akhmerov [ 20/Jul/16 ]

Hi Jan,

could you tell me in a bit more detail what exactly is your usecase? Current implementation is using authenticated user ID to log user operation, you could potentially use identityService to setAuthenticatedUserId user. Is there any specific reason not to do that?

About your code, we are implementing unit tests using JUnit 4 style for the fresh code, could you adjust your unit test to use ProcessEngineRule instead of extending AbstractUserOperationLogTest and annotate test method, you can use org.camunda.bpm.engine.test.history.useroperationlog.LegacyUserOperationLogTest as an example.

Best regards,
Askar

Comment by Jan Galinski [ 20/Jul/16 ]

Hi Askar,

we are using spring-security for login, ... and have a custom user/group management. So we do not use the camunda identityService at all (other than for cockpit admins).
We want to use the operation og for reporting, so we audit all events, custom and core, in the op log table and later select by process or task.
We do need for example a task-creation/task-completed event in the op log. Unfortunately it is not written by camunda, even on history=full. So I implemented a custom history event handler that writes task-lifecycle events to the op log. I set the userId to "system", but got "null", because I am not authenticated at camunda.
Of course I could set the authentication on the identityService to "system" before calling "logUserOperation", but I do not really see, why I should, since the userOperationLogContext already allows me to set the userId directly ... I do not want to call it "bug", but to me it seems a bit spooky to overwrite values I set explicitely with "null" ...

Getting clearer?
Jan

Comment by Askar Akhmerov [ 20/Jul/16 ]

Hi Jan,

thanks for info, getting clearer now. Do I understand correctly that your setup is having embedded engine and you are not using camunda webapp? Or you implemented custom IdentityService and IdentityManager?

In general, I think the change is fine, if you would be so king and adjust unit test, I can merge it.

Comment by Jan Galinski [ 22/Jul/16 ]

Cool. I updated the test case, please check https://github.com/camunda/camunda-bpm-platform/pull/236/files

Comment by Askar Akhmerov [ 22/Jul/16 ]

Hi Jan,

merged, thank you for the contribution.

Cheers,
Askar.

Comment by Jan Galinski [ 22/Jul/16 ]

Will this only come with 7.6? I am using an ugly workaround (custom read only identityService) in a customer project and would love to have this on an ee-patch.

Comment by Askar Akhmerov [ 22/Jul/16 ]

Hi Jan,

we will decide on Monday.

Best regards,
Askar

Comment by Askar Akhmerov [ 25/Jul/16 ]

picked to 7.5, will be released with next patch.

Generated at Mon Aug 26 11:38:58 CEST 2019 using JIRA 6.4.6#64021-sha1:33e5b454af4594f54560ac233c30a6e00459507e.