From 4ea6e2d0fbcf03cd046619e3fb321c7540019379 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20Lled=C3=B3?= Date: Mon, 9 Sep 2024 16:41:23 +0200 Subject: [PATCH 1/5] Audit create/revoke sessions --- app/models/user_session.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/user_session.rb b/app/models/user_session.rb index 02f200b99a..184c7791de 100644 --- a/app/models/user_session.rb +++ b/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) From 4460fce00e7f4159de5329030b47482d524f99cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20Lled=C3=B3?= Date: Mon, 16 Sep 2024 18:04:45 +0200 Subject: [PATCH 2/5] Audit `User` `password_digest` --- app/models/user.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 35601972ad..47c1aae07d 100644 --- a/app/models/user.rb +++ b/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 From 37cd6aab9bf84d04687ec1099443a026adc3d5be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20Lled=C3=B3?= Date: Mon, 9 Sep 2024 16:41:43 +0200 Subject: [PATCH 3/5] Audit log when login fails --- app/controllers/provider/sessions_controller.rb | 6 ++---- .../app/controllers/developer_portal/login_controller.rb | 5 +++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/app/controllers/provider/sessions_controller.rb b/app/controllers/provider/sessions_controller.rb index f2690fc6f6..6c1f7273bf 100644 --- a/app/controllers/provider/sessions_controller.rb +++ b/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 diff --git a/lib/developer_portal/app/controllers/developer_portal/login_controller.rb b/lib/developer_portal/app/controllers/developer_portal/login_controller.rb index 80ff34bd76..10cd4c8205 100644 --- a/lib/developer_portal/app/controllers/developer_portal/login_controller.rb +++ b/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 From b3802b7b12b47f01d5929347af33a1007ad0e27b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20Lled=C3=B3?= Date: Mon, 9 Sep 2024 17:23:29 +0200 Subject: [PATCH 4/5] Audit logs to password change fail --- .../provider/admin/user/personal_details_controller.rb | 1 + .../admin/account/personal_details_controller.rb | 5 ++--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/provider/admin/user/personal_details_controller.rb b/app/controllers/provider/admin/user/personal_details_controller.rb index a7702e6f16..6b3d39f170 100644 --- a/app/controllers/provider/admin/user/personal_details_controller.rb +++ b/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 diff --git a/lib/developer_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb b/lib/developer_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb index 02f6b61422..6bc3537f0e 100644 --- a/lib/developer_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb +++ b/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 From d09a4dd4e27d53c7b6fad237d741cd4795d87cc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20Lled=C3=B3?= Date: Tue, 10 Sep 2024 12:18:46 +0200 Subject: [PATCH 5/5] Add tests for sessions audit --- test/factories/user_session.rb | 7 ++++++ .../personal_details_controller_test.rb | 20 +++++++++++++++++ .../developer_portal/login_controller_test.rb | 13 +++++++++++ .../user/personal_details_controller_test.rb | 16 ++++++++++++++ .../provider/sessions_controller_test.rb | 11 ++++++++-- test/unit/user_session_test.rb | 22 +++++++++++++++++++ 6 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 test/factories/user_session.rb diff --git a/test/factories/user_session.rb b/test/factories/user_session.rb new file mode 100644 index 0000000000..d3f816e828 --- /dev/null +++ b/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 diff --git a/test/functional/developer_portal/admin/account/personal_details_controller_test.rb b/test/functional/developer_portal/admin/account/personal_details_controller_test.rb index 614299b210..8e9e7e1a5c 100644 --- a/test/functional/developer_portal/admin/account/personal_details_controller_test.rb +++ b/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 diff --git a/test/functional/developer_portal/login_controller_test.rb b/test/functional/developer_portal/login_controller_test.rb index 4e3f877d58..43c1a8925d 100644 --- a/test/functional/developer_portal/login_controller_test.rb +++ b/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 diff --git a/test/functional/provider/admin/user/personal_details_controller_test.rb b/test/functional/provider/admin/user/personal_details_controller_test.rb index 2860e63ddb..c65d307bc3 100644 --- a/test/functional/provider/admin/user/personal_details_controller_test.rb +++ b/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 diff --git a/test/integration/provider/sessions_controller_test.rb b/test/integration/provider/sessions_controller_test.rb index 391dddb10d..da8d7f6f29 100644 --- a/test/integration/provider/sessions_controller_test.rb +++ b/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 diff --git a/test/unit/user_session_test.rb b/test/unit/user_session_test.rb index 6f335e6a4f..463c9a7342 100644 --- a/test/unit/user_session_test.rb +++ b/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