Api log contains only username + refactor #2793

app/controllers/application_controller.rb

@@ -56,15 +56,7 @@ class ApplicationController < ActionController::Base
   end
 
   def user_for_paper_trail
-    if defined?(current_user) && current_user.present?
-      # Most of the time it's not loaded in correct time because PaperTrail before filter kicks in
-      # before current_user is defined. PaperTrail is triggered also at current_user
-      api_user_log_str(current_user)
-    elsif current_user.present?
-      "#{current_user.id}-#{current_user.username}"
-    else
-      'public'
-    end
+    user_log_str(current_user)
   end
 
   def depp_current_user
@@ -74,11 +66,8 @@ class ApplicationController < ActionController::Base
     )
   end
 
-  def api_user_log_str(user)
-    if user.present?
-      "#{user.id}-api-#{user.username}"
-    else
-      'api-public'
-    end
+  def user_log_str(user)
+    return 'public' if user.nil?
+    "#{user.id}-#{user.class}: #{user.username}"
   end
 end

app/controllers/epp_controller.rb

@@ -120,7 +120,7 @@ class EppController < ApplicationController
     @current_user ||= ApiUser.find_by_id(epp_session[:api_user_id])
     # by default PaperTrail uses before filter and at that
     # time current_user is not yet present
-    ::PaperTrail.whodunnit = api_user_log_str(@current_user)
+    ::PaperTrail.whodunnit = user_log_str(@current_user)
     ::PaperSession.session = epp_session.session_id if epp_session.session_id.present?
     @current_user
   end
@@ -350,6 +350,7 @@ class EppController < ApplicationController
   # rubocop: enable Style/PredicateName
 
   # rubocop: disable Metrics/CyclomaticComplexity
+  # rubocop: disable Metrics/PerceivedComplexity
   def write_to_epp_log
     # return nil if EPP_LOG_ENABLED
     request_command = params[:command] || params[:action] # error receives :command, other methods receive :action
@@ -366,12 +367,13 @@ class EppController < ApplicationController
       request_successful: epp_errors.empty?,
       request_object: params[:epp_object_type],
       response: @response,
-      api_user_name: api_user_log_str(@api_user || current_user),
+      api_user_name: @api_user.try(:username) || current_user.try(:username) || 'api-public',
       api_user_registrar: @api_user.try(:registrar).try(:to_s) || current_user.try(:registrar).try(:to_s),
       ip: request.ip
     })
   end
   # rubocop: enable Metrics/CyclomaticComplexity
+  # rubocop: enable Metrics/PerceivedComplexity
 
   def iptables_counter_update
     return if ENV['iptables_counter_enabled'].blank? && ENV['iptables_counter_enabled'] != 'true'

app/models/concerns/versions.rb

@@ -20,17 +20,15 @@ module Versions
       true
     end
 
-    # needs refactoring
-    # TODO: optimization work
-    # belongs_to :api_creator, class_name: 'ApiUser', foreign_key: :creator_str
-    # belongs_to :creator, class_name: 'User', foreign_key: :creator_str
     def creator
       return nil if creator_str.blank?
 
-      if creator_str =~ /^\d-api-/
-        creator = ApiUser.find_by(id: creator_str)
-      else
+      if creator_str =~ /^\d+-AdminUser:/
         creator = AdminUser.find_by(id: creator_str)
+      elsif creator_str =~ /^\d+-ApiUser:/
+        creator = ApiUser.find_by(id: creator_str)
+      elsif creator_str =~ /^\d+-api-/ # depricated
+        creator = ApiUser.find_by(id: creator_str)
       end
 
       creator.present? ? creator : creator_str
@@ -39,10 +37,12 @@ module Versions
     def updator
       return nil if updator_str.blank?
 
-      if updator_str =~ /^\d-api-/
-        updator = ApiUser.find_by(id: updator_str)
-      else
+      if updator_str =~ /^\d+-AdminUser:/
         updator = AdminUser.find_by(id: updator_str)
+      elsif updator_str =~ /^\d+-ApiUser:/
+        updator = ApiUser.find_by(id: updator_str)
+      elsif updator_str =~ /^\d+-api-/ # depricated
+        updator = ApiUser.find_by(id: updator_str)
       end
 
       updator.present? ? updator : updator_str

spec/epp/contact_spec.rb

@@ -85,7 +85,7 @@ describe 'EPP Contact', epp: true do
         log.request_command.should == 'create'
         log.request_object.should == 'contact'
         log.request_successful.should == true
-        log.api_user_name.should == '1-api-registrar1'
+        log.api_user_name.should == 'registrar1'
         log.api_user_registrar.should == 'registrar1'
       end
 

spec/epp/domain_spec.rb

@@ -67,7 +67,7 @@ describe 'EPP Domain', epp: true do
     log.request_command.should == 'create'
     log.request_object.should == 'domain'
     log.request_successful.should == false
-    log.api_user_name.should == '1-api-registrar1'
+    log.api_user_name.should == 'registrar1'
     log.api_user_registrar.should == 'registrar1'
     log.request.should_not be_blank
     log.response.should_not be_blank
@@ -1061,7 +1061,7 @@ describe 'EPP Domain', epp: true do
         log.request_command.should == 'transfer'
         log.request_object.should == 'domain'
         log.request_successful.should == true
-        log.api_user_name.should == '2-api-registrar2'
+        log.api_user_name.should == 'registrar2'
         log.api_user_registrar.should == 'registrar2'
         log.request.should_not be_blank
         log.response.should_not be_blank

spec/epp/poll_spec.rb

@@ -32,7 +32,7 @@ describe 'EPP Poll', epp: true do
     log.request_command.should == 'poll'
     log.request_object.should == 'poll'
     log.request_successful.should == true
-    log.api_user_name.should == '1-api-registrar1'
+    log.api_user_name.should == 'registrar1'
     log.api_user_registrar.should == @registrar1.name
     log.request.should_not be_blank
     log.response.should_not be_blank

spec/epp/session_spec.rb

@@ -47,7 +47,7 @@ describe 'EPP Session', epp: true do
       log = ApiLog::EppLog.last
       log.request_command.should == 'login'
       log.request_successful.should == false
-      log.api_user_name.should == '2-api-inactive-user'
+      log.api_user_name.should == 'inactive-user'
     end
 
     it 'prohibits further actions unless logged in' do
@@ -88,7 +88,7 @@ describe 'EPP Session', epp: true do
         log = ApiLog::EppLog.last
         log.request_command.should == 'login'
         log.request_successful.should == true
-        log.api_user_name.should == '1-api-gitlab'
+        log.api_user_name.should == 'gitlab'
       end
 
       it 'does not log in twice' do
@@ -104,7 +104,7 @@ describe 'EPP Session', epp: true do
         log = ApiLog::EppLog.last
         log.request_command.should == 'login'
         log.request_successful.should == false
-        log.api_user_name.should == '1-api-gitlab'
+        log.api_user_name.should == 'gitlab'
       end
 
       it 'logs out epp user' do

spec/models/domain_spec.rb

@@ -387,10 +387,10 @@ describe Domain do
           @api_user = Fabricate(:api_user)
           @user.id.should == 1
           @api_user.id.should == 2
-          ::PaperTrail.whodunnit = '2-api-testuser'
+          ::PaperTrail.whodunnit = '2-ApiUser: testuser'
 
           @domain = Fabricate(:domain)
-          @domain.creator_str.should == '2-api-testuser'
+          @domain.creator_str.should == '2-ApiUser: testuser'
 
           @domain.creator.should == @api_user
           @domain.creator.should_not == @user
@@ -399,14 +399,14 @@ describe Domain do
 
       it 'should return api_creator when created by api user' do
         with_versioning do
-          @user = Fabricate(:admin_user)
-          @api_user = Fabricate(:api_user)
-          @user.id.should == 3
-          @api_user.id.should == 4
-          ::PaperTrail.whodunnit = '3-testuser'
+          @user = Fabricate(:admin_user, id: 1000)
+          @api_user = Fabricate(:api_user, id: 2000)
+          @user.id.should == 1000
+          @api_user.id.should == 2000
+          ::PaperTrail.whodunnit = '1000-AdminUser: testuser'
 
           @domain = Fabricate(:domain)
-          @domain.creator_str.should == '3-testuser'
+          @domain.creator_str.should == '1000-AdminUser: testuser'
 
           @domain.creator.should == @user
           @domain.creator.should_not == @api_user