diff --git a/app/controllers/auth/omniauth_callbacks_controller.rb b/app/controllers/auth/omniauth_callbacks_controller.rb index 9d496220a3d..16de03fd72d 100644 --- a/app/controllers/auth/omniauth_callbacks_controller.rb +++ b/app/controllers/auth/omniauth_callbacks_controller.rb @@ -38,8 +38,7 @@ class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController private def record_login_activity - LoginActivity.create( - user: @user, + @user.login_activities.create( success: true, authentication_method: :omniauth, provider: @provider, diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index 2808066aaf9..c52bda67b0a 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -151,12 +151,11 @@ class Auth::SessionsController < Devise::SessionsController sign_in(user) flash.delete(:notice) - LoginActivity.create( - user: user, - success: true, - authentication_method: security_measure, - ip: request.remote_ip, - user_agent: request.user_agent + user.login_activities.create( + request_details.merge( + authentication_method: security_measure, + success: true + ) ) UserMailer.suspicious_sign_in(user, request.remote_ip, request.user_agent, Time.now.utc).deliver_later! if @login_is_suspicious @@ -167,13 +166,12 @@ class Auth::SessionsController < Devise::SessionsController end def on_authentication_failure(user, security_measure, failure_reason) - LoginActivity.create( - user: user, - success: false, - authentication_method: security_measure, - failure_reason: failure_reason, - ip: request.remote_ip, - user_agent: request.user_agent + user.login_activities.create( + request_details.merge( + authentication_method: security_measure, + failure_reason: failure_reason, + success: false + ) ) # Only send a notification email every hour at most @@ -182,6 +180,13 @@ class Auth::SessionsController < Devise::SessionsController UserMailer.failed_2fa(user, request.remote_ip, request.user_agent, Time.now.utc).deliver_later! end + def request_details + { + ip: request.remote_ip, + user_agent: request.user_agent, + } + end + def second_factor_attempts_key(user) "2fa_auth_attempts:#{user.id}:#{Time.now.utc.hour}" end diff --git a/app/controllers/settings/login_activities_controller.rb b/app/controllers/settings/login_activities_controller.rb index 50e2d70cb9a..ae32dbf5570 100644 --- a/app/controllers/settings/login_activities_controller.rb +++ b/app/controllers/settings/login_activities_controller.rb @@ -5,6 +5,6 @@ class Settings::LoginActivitiesController < Settings::BaseController skip_before_action :require_functional! def index - @login_activities = LoginActivity.where(user: current_user).order(id: :desc).page(params[:page]) + @login_activities = current_user.login_activities.order(id: :desc).page(params[:page]) end end diff --git a/app/models/user.rb b/app/models/user.rb index 966ffbe9c37..762522f2822 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -90,6 +90,7 @@ class User < ApplicationRecord has_many :applications, class_name: 'Doorkeeper::Application', as: :owner, dependent: nil has_many :backups, inverse_of: :user, dependent: nil has_many :invites, inverse_of: :user, dependent: nil + has_many :login_activities, inverse_of: :user, dependent: :destroy has_many :markers, inverse_of: :user, dependent: :destroy has_many :webauthn_credentials, dependent: :destroy has_many :ips, class_name: 'UserIp', inverse_of: :user, dependent: nil diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index c852b16a69c..949af2a4259 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -100,11 +100,14 @@ RSpec.describe Auth::SessionsController do let(:user) { Fabricate(:user, email: 'foo@bar.com', password: 'abcdefgh') } context 'when using a valid password' do - before do + subject do post :create, params: { user: { email: user.email, password: user.password } } end it 'redirects to home and logs the user in' do + expect { subject } + .to change(user.login_activities.where(success: true), :count).by(1) + expect(response).to redirect_to(root_path) expect(controller.current_user).to eq user @@ -265,10 +268,9 @@ RSpec.describe Auth::SessionsController do it 'does not log the user in, sets a flash message, and sends a suspicious sign in email', :inline_jobs do emails = capture_emails do - Auth::SessionsController::MAX_2FA_ATTEMPTS_PER_HOUR.times do - post :create, params: { user: { otp_attempt: '1234' } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } - expect(controller.current_user).to be_nil - end + expect { process_maximum_two_factor_attempts } + .to change(user.login_activities.where(success: false), :count).by(1) + post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } end @@ -286,6 +288,13 @@ RSpec.describe Auth::SessionsController do subject: eq(I18n.t('user_mailer.failed_2fa.subject')) ) end + + def process_maximum_two_factor_attempts + Auth::SessionsController::MAX_2FA_ATTEMPTS_PER_HOUR.times do + post :create, params: { user: { otp_attempt: '1234' } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s } + expect(controller.current_user).to be_nil + end + end end context 'when using a valid OTP' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b732b2d84d0..c71b7a600ba 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -24,6 +24,7 @@ RSpec.describe User do describe 'Associations' do it { is_expected.to belong_to(:account).required } + it { is_expected.to have_many(:login_activities) } end describe 'Validations' do