Incorrect User in Audit

[sauloarruda]

Hello,

We are using the audit to generate around 20 thousand records daily. In some cases we find that the user who performed the action is incorrect. Analyzing the code we saw that the Thread.current[:audited_store] method is called to get the current user. We use AWS with loadbalancer with 2 to 4 servers at peak hours.

We need help to understand why this is happening.

[tiagocassio]

@sauloarruda did you solve this problem? I'm getting the same problem here, im some cases the user that makes the change is not the correct one.

[sauloarruda]

Hi @tiagocassio We solved using RequestStore. This is the monkey patch. I hope it helps you too.

  class << self
    def store
      current_store_value = RequestStore.store[:audited_store]

      if current_store_value.nil?
        RequestStore.store[:audited_store] = {}
      else
        current_store_value
      end
    end
  end

  # add sudo_user_id
  class Sweeper
    STORED_DATA = {
      current_remote_address: :remote_ip,
      current_request_uuid: :request_uuid,
      current_user: :current_user,
      sudo_user_id: :sudo_user_id
    }

    def sudo_user_id
      controller.try(:sudo_user_id)
    end
  end

  class Audit
    def set_audit_user
      self.user ||= ::Audited.store[:audited_user] # from .as_user
      self.user ||= ::Audited.store[:current_user].try!(:call) # from Sweeper
      self.sudo_user_id ||= ::Audited.store[:sudo_user_id]
      nil # prevent stopping callback chains
    end
  end
end

[sauloarruda]

the sudo user part is a new implementation, ignore it please.

[tiagocassio]

Hi @sauloarruda! I've already proposed a PR implementing RequestStore. See https://github.com/collectiveidea/audited/pull/669

[the-spectator]

@sauloarruda @tiagocassio I have created https://github.com/collectiveidea/audited/pull/673 as an alternative attempt to solve the issue using ActiveSupport::CurrentAttributes. If possible can you test your use case against my branch?

gem 'audited', git: 'https://github.com/the-spectator/audited', branch: 'migrate_to_current_attributes'

[jlledom]

After updating to 5.3.3, which uses RequestStore, I'm facing some issues. The reason is RequestStore clears the store object after each request [Docs].

So before the update, the workflow was like this:

1. The rails server starts.
2. A model, e.g. User, calls the audited method, that adds the users_auditing_enabled key to the store.
3. A request is received, the key users_auditing_enabled is fetched from the store, in order to check the audit is enabled for that class.
4. The request is served, the store is not modified and the next request will be able to fetch users_auditing_enabled from the store.

And this is the new workflow when using RequestStore:

1. The rails server starts.
2. A model, e.g. User, calls the audited method, that adds the users_auditing_enabled key to the store.
3. A request is received, the key users_auditing_enabled is fetched from the store, in order to check the audit is enabled for that class.
4. The request is served, the store is cleared and all future requests will fail to fetch users_auditing_enabled from the store.
5. Failing to fetch the key defaults to true, so disabling auditing is not possible.

I tried checking out the branch for https://github.com/collectiveidea/audited/pull/673 that uses ActiveSupport::CurrentAttributes and the result is the same, because in this case the store is cleared after and before every request [Docs]

When the store is cleared, it's remains cleared for the remaining life of the scope, which can be a Thread or a Process depending on the server being used, configuration, etc. I tested this locally using Unicorn with two workers. In this case the server uses only one thread and spawns to processes, one for each worker. Only the first request attended by each worker gets the store correctly. The problem is this will be different for different servers and configs.

In order to fix this issue, I think it's OK to remove keys like :audited_user or :current_user after every request, but "#{table_name}_auditing_enabled" keys should be kept between requests.