Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add notification email on invalid second authenticator #28822

Merged
merged 1 commit into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/controllers/auth/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,11 @@ def on_authentication_failure(user, security_measure, failure_reason)
ip: request.remote_ip,
user_agent: request.user_agent
)

# Only send a notification email every hour at most
return if redis.set("2fa_failure_notification:#{user.id}", '1', ex: 1.hour, get: true).present?

UserMailer.failed_2fa(user, request.remote_ip, request.user_agent, Time.now.utc).deliver_later!
end

def second_factor_attempts_key(user)
Expand Down
12 changes: 12 additions & 0 deletions app/mailers/user_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,18 @@ def suspicious_sign_in(user, remote_ip, user_agent, timestamp)
end
end

def failed_2fa(user, remote_ip, user_agent, timestamp)
@resource = user
@remote_ip = remote_ip
@user_agent = user_agent
@detection = Browser.new(user_agent)
@timestamp = timestamp.to_time.utc

I18n.with_locale(locale) do
mail subject: default_i18n_subject
end
end

private

def default_devise_subject
Expand Down
24 changes: 24 additions & 0 deletions app/views/user_mailer/failed_2fa.html.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
= content_for :heading do
= render 'application/mailer/heading', heading_title: t('user_mailer.failed_2fa.title'), heading_subtitle: t('user_mailer.failed_2fa.explanation'), heading_image_url: frontend_asset_url('images/mailer-new/heading/login.png')
%table.email-w-full{ cellspacing: 0, cellpadding: 0, border: 0, role: 'presentation' }
%tr
%td.email-body-padding-td
%table.email-inner-card-table{ cellspacing: 0, cellpadding: 0, border: 0, role: 'presentation' }
%tr
%td.email-inner-card-td.email-prose
%p= t 'user_mailer.failed_2fa.details'
%p
%strong #{t('sessions.ip')}:
= @remote_ip
%br/
%strong #{t('sessions.browser')}:
%span{ title: @user_agent }
= t 'sessions.description',
browser: t("sessions.browsers.#{@detection.id}", default: @detection.id.to_s),
platform: t("sessions.platforms.#{@detection.platform.id}", default: @detection.platform.id.to_s)
%br/
%strong #{t('sessions.date')}:
= l(@timestamp.in_time_zone(@resource.time_zone.presence), format: :with_time_zone)
= render 'application/mailer/button', text: t('settings.account_settings'), url: edit_user_registration_url
%p= t 'user_mailer.failed_2fa.further_actions_html',
action: link_to(t('user_mailer.suspicious_sign_in.change_password'), edit_user_registration_url)
15 changes: 15 additions & 0 deletions app/views/user_mailer/failed_2fa.text.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<%= t 'user_mailer.failed_2fa.title' %>

===

<%= t 'user_mailer.failed_2fa.explanation' %>

<%= t 'user_mailer.failed_2fa.details' %>

<%= t('sessions.ip') %>: <%= @remote_ip %>
<%= t('sessions.browser') %>: <%= t('sessions.description', browser: t("sessions.browsers.#{@detection.id}", default: "#{@detection.id}"), platform: t("sessions.platforms.#{@detection.platform.id}", default: "#{@detection.platform.id}")) %>
<%= l(@timestamp.in_time_zone(@resource.time_zone.presence), format: :with_time_zone) %>

<%= t 'user_mailer.failed_2fa.further_actions_html', action: t('user_mailer.suspicious_sign_in.change_password') %>

=> <%= edit_user_registration_url %>
6 changes: 6 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1791,6 +1791,12 @@ en:
extra: It's now ready for download!
subject: Your archive is ready for download
title: Archive takeout
failed_2fa:
details: 'Here are details of the sign-in attempt:'
explanation: Someone has tried to sign in to your account but provided an invalid second authentication factor.
further_actions_html: If this wasn't you, we recommend that you %{action} immediately as it may be compromised.
subject: Second factor authentication failure
title: Failed second factor authentication
suspicious_sign_in:
change_password: change your password
details: 'Here are details of the sign-in:'
Expand Down
18 changes: 16 additions & 2 deletions spec/controllers/auth/sessions_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -265,21 +265,35 @@
context 'when repeatedly using an invalid TOTP code before using a valid code' do
before do
stub_const('Auth::SessionsController::MAX_2FA_ATTEMPTS_PER_HOUR', 2)
end

it 'does not log the user in' do
# Travel to the beginning of an hour to avoid crossing rate-limit buckets
travel_to '2023-12-20T10:00:00Z'
end

it 'does not log the user in' 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

post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s }

expect(controller.current_user).to be_nil
expect(flash[:alert]).to match I18n.t('users.rate_limited')
end

it 'sends a suspicious sign-in mail', :sidekiq_inline 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

post :create, params: { user: { otp_attempt: user.current_otp } }, session: { attempt_user_id: user.id, attempt_user_updated_at: user.updated_at.to_s }

expect(UserMailer.deliveries.size).to eq(1)
expect(UserMailer.deliveries.first.to.first).to eq(user.email)
expect(UserMailer.deliveries.first.subject).to eq(I18n.t('user_mailer.failed_2fa.subject'))
end
end

context 'when using a valid OTP' do
Expand Down
5 changes: 5 additions & 0 deletions spec/mailers/previews/user_mailer_preview.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,9 @@ def appeal_approved
def suspicious_sign_in
UserMailer.suspicious_sign_in(User.first, '127.0.0.1', 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:75.0) Gecko/20100101 Firefox/75.0', Time.now.utc)
end

# Preview this email at http://localhost:3000/rails/mailers/user_mailer/failed_2fa
def failed_2fa
UserMailer.failed_2fa(User.first, '127.0.0.1', 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:75.0) Gecko/20100101 Firefox/75.0', Time.now.utc)
end
end
18 changes: 18 additions & 0 deletions spec/mailers/user_mailer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,24 @@
'user_mailer.suspicious_sign_in.subject'
end

describe '#failed_2fa' do
let(:ip) { '192.168.0.1' }
let(:agent) { 'NCSA_Mosaic/2.0 (Windows 3.1)' }
let(:timestamp) { Time.now.utc }
let(:mail) { described_class.failed_2fa(receiver, ip, agent, timestamp) }

it 'renders failed 2FA notification' do
receiver.update!(locale: nil)

expect(mail)
.to be_present
.and(have_body_text(I18n.t('user_mailer.failed_2fa.explanation')))
end

include_examples 'localized subject',
'user_mailer.failed_2fa.subject'
end

describe '#appeal_approved' do
let(:appeal) { Fabricate(:appeal, account: receiver.account, approved_at: Time.now.utc) }
let(:mail) { described_class.appeal_approved(receiver, appeal) }
Expand Down