Uploaded image for project: 'camunda BPM'
  1. camunda BPM
  2. CAM-6431

UserOperationLogManager overwrites userId set on UserOperationLogContext

    Details

    • Type: Bug Report
    • Status: Closed
    • Priority: L3 - Default
    • Resolution: Fixed
    • Affects Version/s: 7.5.1
    • Fix Version/s: 7.6.0, 7.5.4, 7.6.0-alpha3
    • Component/s: engine
    • Labels:
      None

      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());
      }
      

        Activity

        jan.galinski@holisticon.de Jan Galinski created issue -
        Hide
        thorben.lindhauer Thorben Lindhauer added a comment -

        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

        Show
        thorben.lindhauer Thorben Lindhauer added a comment - 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
        thorben.lindhauer Thorben Lindhauer made changes -
        Field Original Value New Value
        Issue Type Bug Report [ 1 ] Task [ 3 ]
        Hide
        jan.galinski@holisticon.de Jan Galinski added a comment -

        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?

        Show
        jan.galinski@holisticon.de Jan Galinski added a comment - 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?
        askar.akhmerov Askar Akhmerov made changes -
        Assignee Askar Akhmerov [ askar.akhmerov ]
        Hide
        askar.akhmerov Askar Akhmerov added a comment -

        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

        Show
        askar.akhmerov Askar Akhmerov added a comment - 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
        askar.akhmerov Askar Akhmerov made changes -
        Assignee Askar Akhmerov [ askar.akhmerov ] Jan Galinski [ jan.galinski@holisticon.de ]
        Hide
        jan.galinski@holisticon.de Jan Galinski added a comment -

        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

        Show
        jan.galinski@holisticon.de Jan Galinski added a comment - 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
        Hide
        askar.akhmerov Askar Akhmerov added a comment -

        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.

        Show
        askar.akhmerov Askar Akhmerov added a comment - 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.
        Hide
        jan.galinski@holisticon.de Jan Galinski added a comment -

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

        Show
        jan.galinski@holisticon.de Jan Galinski added a comment - Cool. I updated the test case, please check https://github.com/camunda/camunda-bpm-platform/pull/236/files
        Hide
        askar.akhmerov Askar Akhmerov added a comment -

        Hi Jan,

        merged, thank you for the contribution.

        Cheers,
        Askar.

        Show
        askar.akhmerov Askar Akhmerov added a comment - Hi Jan, merged, thank you for the contribution. Cheers, Askar.
        askar.akhmerov Askar Akhmerov made changes -
        Assignee Jan Galinski [ jan.galinski@holisticon.de ] Askar Akhmerov [ askar.akhmerov ]
        askar.akhmerov Askar Akhmerov made changes -
        Fix Version/s 7.6.0 [ 14490 ]
        askar.akhmerov Askar Akhmerov made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Original Estimate 0 minutes [ 0 ]
        Remaining Estimate 0 minutes [ 0 ]
        Resolution Fixed [ 1 ]
        Hide
        jan.galinski@holisticon.de Jan Galinski added a comment -

        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.

        Show
        jan.galinski@holisticon.de Jan Galinski added a comment - 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.
        askar.akhmerov Askar Akhmerov made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Assignee Askar Akhmerov [ askar.akhmerov ]
        Hide
        askar.akhmerov Askar Akhmerov added a comment -

        Hi Jan,

        we will decide on Monday.

        Best regards,
        Askar

        Show
        askar.akhmerov Askar Akhmerov added a comment - Hi Jan, we will decide on Monday. Best regards, Askar
        askar.akhmerov Askar Akhmerov made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Assignee Askar Akhmerov [ askar.akhmerov ]
        askar.akhmerov Askar Akhmerov made changes -
        Issue Type Task [ 3 ] Bug Report [ 1 ]
        askar.akhmerov Askar Akhmerov made changes -
        Fix Version/s 7.5.4 [ 14606 ]
        askar.akhmerov Askar Akhmerov made changes -
        Status Reopened [ 4 ] In Progress [ 3 ]
        Hide
        askar.akhmerov Askar Akhmerov added a comment -

        picked to 7.5, will be released with next patch.

        Show
        askar.akhmerov Askar Akhmerov added a comment - picked to 7.5, will be released with next patch.
        askar.akhmerov Askar Akhmerov made changes -
        Status In Progress [ 3 ] Resolved [ 5 ]
        Assignee Askar Akhmerov [ askar.akhmerov ] Philipp Ossler [ philipp.ossler ]
        Resolution Fixed [ 1 ]
        philipp.ossler Philipp Ossler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Assignee Philipp Ossler [ philipp.ossler ]
        gimbel Robert Gimbel made changes -
        Fix Version/s 7.6.0-alpha3 [ 14609 ]
        thorben.lindhauer Thorben Lindhauer made changes -
        Workflow camunda BPM [ 39242 ] Backup_camunda BPM [ 61927 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            jan.galinski@holisticon.de Jan Galinski
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development