Merge pull request #3885 from jlledom/THREESCALE-10843-user-session-a…

…udited

THREESCALE-10843: Audit user sessions

app/controllers/provider/admin/user/personal_details_controller.rb

@@ -34,6 +34,7 @@ def current_password_verification
 
     unless current_user.authenticated?(user_params[:current_password])
       flash.now[:error] = 'Current password is incorrect.'
+      AuditLogService.call("User tried to change password, but failed due to incorrect current password: #{current_user.id}/#{current_user.username}") if user_params[:password].present?
       render(action: :edit)
     end
   end

app/controllers/provider/sessions_controller.rb

@@ -28,12 +28,12 @@ def create
       create_user_session!(strategy&.authentication_provider_id)
       flash[:notice] = 'Signed in successfully'
 
-      AuditLogService.call("Signed in: #{current_user.id}/#{current_user.username} #{current_user.first_name} #{current_user.last_name}")
-
       redirect_back_or_default provider_admin_path
     else
       new
       flash.now[:error] ||= strategy&.error_message
+      attempted_cred = auth_params.fetch(:username, 'SSO')
+      AuditLogService.call("Login attempt failed from #{request.remote_ip}: #{domain_account.external_admin_domain} - #{attempted_cred}. ERROR: #{strategy&.error_message}")
       render :action => :new
     end
   end
@@ -48,8 +48,6 @@ def destroy
     logout_killing_session!
     destroy_user_session!
 
-    AuditLogService.call("Signed out: #{user.id}/#{user.username} #{user.first_name} #{user.last_name}")
-
     if @provider.partner? && (logout_url = @provider.partner.logout_url)
       redirect_to logout_url + {user_id: user.id, provider_id: @provider.id}.to_query
     else

app/models/user.rb

@@ -32,8 +32,9 @@ class User < ApplicationRecord
     [:notification_preferences, { action: :delete, class_name: 'NotificationPreferences', has_many: false }]
   ].freeze
 
-  audited except: %i[salt posts_count janrain_identifier cas_identifier password_digest
-                     authentication_id open_id last_login_at last_login_ip crypted_password].freeze
+  audited except: %i[salt posts_count janrain_identifier cas_identifier
+                     authentication_id open_id last_login_at last_login_ip crypted_password].freeze,
+          redacted: %i[password_digest].freeze
 
   before_validation :trim_white_space_from_username
   before_destroy :avoid_destruction

app/models/user_session.rb

@@ -18,6 +18,8 @@ class UserSession < ApplicationRecord
 
   delegate :account, to: :user
 
+  audited except: [:accessed_at]
+
   def self.authenticate(key)
     return nil if key.nil?
     self.active.find_by_key(key)

lib/developer_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb

@@ -40,9 +40,7 @@ def resource
   end
 
   def deny_unless_can_update
-    unless can?(:update, current_user)
-      render :plain => 'Action disabled', :status => :forbidden
-    end
+    render :plain => 'Action disabled', :status => :forbidden unless can?(:update, current_user)
   end
 
   def user_params
@@ -56,6 +54,7 @@ def verify_current_password
     return if current_user.authenticated?(user_params[:current_password])
 
     resource.errors.add(:current_password, t('activerecord.errors.models.user.current_password_incorrect'))
+    AuditLogService.call("User tried to change password, but failed due to incorrect current password: #{resource.id}/#{resource.username}") if user_params[:password].present?
     flash.now[:error] = resource.errors.full_messages.to_sentence
   end
 end

lib/developer_portal/app/controllers/developer_portal/login_controller.rb

@@ -31,11 +31,16 @@ def create
       self.current_user = @user
       create_user_session!
       flash[:notice] = @strategy.new_user_created? ? 'Signed up successfully' : 'Signed in successfully'
+
       redirect_back_or_default(@strategy.redirect_to_on_successful_login)
     elsif @strategy.redirects_to_signup?
       @strategy.on_signup(session)
+
       redirect_to @strategy.signup_path(params), notice: 'Successfully authenticated, please complete the signup form'
     else
+      attempted_cred = params.fetch(:username, 'SSO')
+      AuditLogService.call("Login attempt failed: #{request.internal_host} - #{attempted_cred}")
+
       render_login_error(@strategy.error_message)
     end
   end

test/factories/user_session.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :user_session do
+    association :user, factory: :user_with_account
+  end
+end

test/functional/developer_portal/admin/account/personal_details_controller_test.rb

@@ -63,4 +63,24 @@ def setup
     assert_response :success
     assert_equal flash[:error], 'Current password is incorrect'
   end
+
+  test 'changing password is audited' do
+    user = @buyer.admins.first
+    login_as user
+
+    assert_difference(Audited.audit_class.method(:count)) do
+      User.with_synchronous_auditing do
+        put :update, params: { user: {current_password: 'supersecret', password: 'new_password', password_confirmation: 'new_password'} }
+      end
+    end
+
+    expected = [Audited::Auditor::AuditedInstanceMethods::REDACTED] * 2
+    assert_equal expected,user.audits.last.audited_changes['password_digest']
+  end
+
+  test 'failed password change creates an audit log' do
+    login_as @buyer.admins.first
+    AuditLogService.expects(:call).with { |msg| msg.start_with? "User tried to change password" }
+    put :update, params: { user: {current_password: 'wrong_password', password: 'new_password', password_confirmation: 'new_password'} }
+  end
 end

test/functional/developer_portal/login_controller_test.rb

@@ -122,6 +122,19 @@ class DeveloperPortal::LoginControllerTest < DeveloperPortal::ActionController::
     assert_equal error, flash[:error]
   end
 
+  test 'login fail generates an audit log' do
+    buyer_account = FactoryBot.create :buyer_account
+    buyer_settings = buyer_account.settings
+    buyer_settings.authentication_strategy = 'internal'
+    buyer_settings.save!
+    user = buyer_account.admins.first
+
+    AuditLogService.expects(:call).with { |msg| msg.start_with? "Login attempt failed" }
+
+    host! buyer_account.provider_account.external_domain
+    post :create, params: { username: user.username, password: 'wrong_pass' }
+  end
+
   def user
     @user ||= create_user_and_account
   end

test/functional/provider/admin/user/personal_details_controller_test.rb

@@ -27,4 +27,20 @@ def setup
     assert_template 'edit'
   end
 
+  test 'changing password is audited' do
+    assert_difference(Audited.audit_class.method(:count)) do
+      User.with_synchronous_auditing do
+        put :update, params: { user: {current_password: 'supersecret', password: 'new_password', password_confirmation: 'new_password'} }
+      end
+    end
+
+    expected = [Audited::Auditor::AuditedInstanceMethods::REDACTED] * 2
+    assert_equal expected, @provider.first_admin.audits.last.audited_changes['password_digest']
+  end
+
+  test 'failed password change creates an audit log' do
+    AuditLogService.expects(:call).with { |msg| msg.start_with? "User tried to change password" }
+    put :update, params: { user: {current_password: 'wrong_password', password: 'new_password', password_confirmation: 'new_password'} }
+  end
+
 end

test/integration/provider/sessions_controller_test.rb

@@ -32,15 +32,22 @@ class Provider::SessionsControllerTest < ActionDispatch::IntegrationTest
     partner = FactoryBot.create(:partner, logout_url: "http://example.net/?")
     account = FactoryBot.create(:provider_account, partner: partner)
 
-    AuditLogService.expects(:call).with { |msg| msg.start_with? "Signed in: #{account.admins.first.id}/" }
     login! account
 
-    AuditLogService.expects(:call).with { |msg| msg.start_with? "Signed out: #{account.admins.first.id}/" }
     delete provider_sessions_path
 
     assert_redirected_to "http://example.net/?provider_id=#{account.id}&user_id=#{account.admin_user.id}"
   end
 
+  test 'failed login generates an audit log' do
+    partner = FactoryBot.create(:partner, logout_url: "http://example.net/?")
+    provider = FactoryBot.create(:provider_account, partner: partner)
+    AuditLogService.expects(:call).with { |msg| msg.start_with? 'Login attempt failed' }
+
+    host! provider.external_admin_domain
+    provider_login_with provider.admins.first.username, 'wrong_pass'
+  end
+
   test "does not redirect users to SSO even with enforce_sso and single provider" do
     @provider.settings.update_column(:enforce_sso, true)
     get new_provider_sessions_path

test/unit/user_session_test.rb

@@ -83,4 +83,26 @@ def request
     assert_equal 3.weeks, UserSession.send(:ttl_value, 3.weeks.seconds.to_s)
     assert_raise(ApplicationConfigurationError) { UserSession.send(:ttl_value, "123p") }
   end
+
+  test 'new session is audited' do
+    user = FactoryBot.create :user_with_account
+    session = FactoryBot.build :user_session, user: user
+
+    assert_difference(Audited.audit_class.method(:count)) do
+      UserSession.with_synchronous_auditing do
+        session.save!
+      end
+    end
+  end
+
+  test 'revoke session is audited' do
+    user = FactoryBot.create :user_with_account
+    session = FactoryBot.create :user_session, user: user
+
+    assert_difference(Audited.audit_class.method(:count)) do
+      UserSession.with_synchronous_auditing do
+        session.revoke!
+      end
+    end
+  end
 end