diff --git a/app/controllers/concerns/new_device_concern.rb b/app/controllers/concerns/new_device_concern.rb new file mode 100644 index 00000000000..b96b1f64d05 --- /dev/null +++ b/app/controllers/concerns/new_device_concern.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module NewDeviceConcern + def set_new_device_session + user_session[:new_device] = !current_user.authenticated_device?(cookie_uuid: cookies[:device]) + end + + def new_device? + user_session[:new_device] != false + end +end diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index e717304c48e..c4eea88da74 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -5,6 +5,7 @@ module TwoFactorAuthenticatableMethods include RememberDeviceConcern include SecureHeadersConcern include MfaSetupConcern + include NewDeviceConcern def auth_methods_session @auth_methods_session ||= AuthMethodsSession.new(user_session:) @@ -14,8 +15,7 @@ def handle_valid_verification_for_authentication_context(auth_method:) mark_user_session_authenticated(auth_method:, authentication_type: :valid_2fa) disavowal_event, disavowal_token = create_user_event_with_disavowal(:sign_in_after_2fa) - if IdentityConfig.store.feature_new_device_alert_aggregation_enabled && - user_session[:new_device] != false + if IdentityConfig.store.feature_new_device_alert_aggregation_enabled && new_device? if current_user.sign_in_new_device_at.blank? current_user.update(sign_in_new_device_at: disavowal_event.created_at) end diff --git a/app/controllers/users/piv_cac_login_controller.rb b/app/controllers/users/piv_cac_login_controller.rb index 1bcb2b34b47..19a2181ef2a 100644 --- a/app/controllers/users/piv_cac_login_controller.rb +++ b/app/controllers/users/piv_cac_login_controller.rb @@ -5,6 +5,7 @@ class PivCacLoginController < ApplicationController include PivCacConcern include VerifySpAttributesConcern include TwoFactorAuthenticatableMethods + include NewDeviceConcern def new if params.key?(:token) @@ -74,7 +75,7 @@ def process_valid_submission presented: true, ) - user_session[:new_device] = current_user.new_device?(cookie_uuid: cookies[:device]) + set_new_device_session handle_valid_verification_for_authentication_context( auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC, ) diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 88dde2be88e..b6015b46228 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -8,6 +8,7 @@ class SessionsController < Devise::SessionsController include Ial2ProfileConcern include Api::CsrfTokenConcern include ForcedReauthenticationConcern + include NewDeviceConcern rescue_from ActionController::InvalidAuthenticityToken, with: :redirect_to_signin @@ -112,8 +113,9 @@ def process_locked_out_user def handle_valid_authentication sign_in(resource_name, resource) cache_profiles(auth_params[:password]) - user_session[:new_device] = current_user.new_device?(cookie_uuid: cookies[:device]) - create_user_event(:sign_in_before_2fa) + set_new_device_session + event, = create_user_event(:sign_in_before_2fa) + UserAlerts::AlertUserAboutNewDevice.schedule_alert(event:) if new_device? EmailAddress.update_last_sign_in_at_on_user_id_and_email( user_id: current_user.id, email: auth_params[:email], diff --git a/app/models/user.rb b/app/models/user.rb index 8411ef8a682..2274e8dd6d9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -432,6 +432,11 @@ def new_device?(cookie_uuid:) !cookie_uuid || !devices.exists?(cookie_uuid:) end + def authenticated_device?(cookie_uuid:) + return false if cookie_uuid.blank? + devices.joins(:events).exists?(cookie_uuid:, events: { event_type: :sign_in_after_2fa }) + end + # Returns the number of times the user has signed in, corresponding to the `sign_in_before_2fa` # event. # diff --git a/app/services/user_alerts/alert_user_about_new_device.rb b/app/services/user_alerts/alert_user_about_new_device.rb index 30037eebc39..28d70f4172f 100644 --- a/app/services/user_alerts/alert_user_about_new_device.rb +++ b/app/services/user_alerts/alert_user_about_new_device.rb @@ -3,26 +3,28 @@ module UserAlerts class AlertUserAboutNewDevice def self.call(event:, device:, disavowal_token:) - if IdentityConfig.store.feature_new_device_alert_aggregation_enabled - event.user.sign_in_new_device_at ||= event.created_at - event.user.save - else - device_decorator = DeviceDecorator.new(device) - login_location = device_decorator.last_sign_in_location_and_ip - device_name = device_decorator.nice_name + return if IdentityConfig.store.feature_new_device_alert_aggregation_enabled + device_decorator = DeviceDecorator.new(device) + login_location = device_decorator.last_sign_in_location_and_ip + device_name = device_decorator.nice_name - event.user.confirmed_email_addresses.each do |email_address| - UserMailer.with(user: event.user, email_address: email_address).new_device_sign_in( - date: device.last_used_at.in_time_zone('Eastern Time (US & Canada)'). - strftime('%B %-d, %Y %H:%M Eastern Time'), - location: login_location, - device_name: device_name, - disavowal_token: disavowal_token, - ).deliver_now_or_later - end + event.user.confirmed_email_addresses.each do |email_address| + UserMailer.with(user: event.user, email_address: email_address).new_device_sign_in( + date: device.last_used_at.in_time_zone('Eastern Time (US & Canada)'). + strftime('%B %-d, %Y %H:%M Eastern Time'), + location: login_location, + device_name: device_name, + disavowal_token: disavowal_token, + ).deliver_now_or_later end end + def self.schedule_alert(event:) + return if !IdentityConfig.store.feature_new_device_alert_aggregation_enabled || + event.user.sign_in_new_device_at.present? + event.user.update(sign_in_new_device_at: event.created_at) + end + def self.send_alert(user:, disavowal_event:, disavowal_token:) return false unless user.sign_in_new_device_at diff --git a/spec/controllers/concerns/new_device_concern_spec.rb b/spec/controllers/concerns/new_device_concern_spec.rb new file mode 100644 index 00000000000..1cb413cdf45 --- /dev/null +++ b/spec/controllers/concerns/new_device_concern_spec.rb @@ -0,0 +1,63 @@ +require 'rails_helper' + +RSpec.describe NewDeviceConcern, type: :controller do + let(:test_class) do + Class.new do + include NewDeviceConcern + + attr_reader :current_user, :user_session, :cookies + + def initialize(current_user:, user_session:, cookies:) + @current_user = current_user + @user_session = user_session + @cookies = cookies + end + end + end + + let(:cookies) { {} } + let(:current_user) { create(:user) } + let(:user_session) { {} } + let(:instance) { test_class.new(current_user:, user_session:, cookies:) } + + describe '#set_new_device_session' do + context 'with new device' do + it 'sets user session value to true' do + instance.set_new_device_session + + expect(user_session[:new_device]).to eq(true) + end + end + + context 'with authenticated device' do + let(:current_user) { create(:user, :with_authenticated_device) } + let(:cookies) { { device: current_user.devices.last.cookie_uuid } } + + it 'sets user session value to false' do + instance.set_new_device_session + + expect(user_session[:new_device]).to eq(false) + end + end + end + + describe '#new_device?' do + subject(:new_device?) { instance.new_device? } + + context 'session value is unassigned' do + it { expect(new_device?).to eq(true) } + end + + context 'session value is true' do + let(:user_session) { { new_device: true } } + + it { expect(new_device?).to eq(true) } + end + + context 'session value is false' do + let(:user_session) { { new_device: false } } + + it { expect(new_device?).to eq(false) } + end + end +end diff --git a/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb b/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb index 82f6d0b25f6..7ba560c4fc2 100644 --- a/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb +++ b/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb @@ -64,7 +64,7 @@ context 'with an existing device' do before do - controller.user_session[:new_device] = false + allow(controller).to receive(:new_device?).and_return(false) end it 'does not send an alert' do @@ -76,7 +76,7 @@ context 'with a new device' do before do - controller.user_session[:new_device] = true + allow(controller).to receive(:new_device?).and_return(true) end it 'sends the new device alert using 2fa event date' do @@ -119,7 +119,7 @@ context 'with an existing device' do before do - controller.user_session[:new_device] = false + allow(controller).to receive(:new_device?).and_return(false) end it 'does not send an alert' do @@ -131,7 +131,7 @@ context 'with a new device' do before do - controller.user_session[:new_device] = true + allow(controller).to receive(:new_device?).and_return(true) end it 'sends the new device alert' do diff --git a/spec/controllers/users/piv_cac_login_controller_spec.rb b/spec/controllers/users/piv_cac_login_controller_spec.rb index c4c80039b27..859cbdc7c45 100644 --- a/spec/controllers/users/piv_cac_login_controller_spec.rb +++ b/spec/controllers/users/piv_cac_login_controller_spec.rb @@ -61,17 +61,20 @@ }.with_indifferent_access end + subject(:response) { get :new, params: { token: } } + before do - subject.piv_session[:piv_cac_nonce] = nonce - subject.session[:sp] = sp_session + controller.piv_session[:piv_cac_nonce] = nonce + controller.session[:sp] = sp_session allow(PivCacService).to receive(:decode_token).with(token) { data } stub_analytics(user:) - get :new, params: { token: token } end context 'without a valid user' do - before do + it 'calls decode_token twice' do + response + # valid_token? is being called twice, once to determine if it's a valid submission # and once to set the session variable in process_invalid_submission # good opportunity for a refactor @@ -79,6 +82,8 @@ end it 'tracks the login attempt' do + response + expect(@analytics).to have_logged_event( :piv_cac_login, errors: { @@ -90,7 +95,9 @@ end it 'sets the session variable' do - expect(subject.session[:needs_to_setup_piv_cac_after_sign_in]).to be true + response + + expect(controller.session[:needs_to_setup_piv_cac_after_sign_in]).to be true end it 'redirects to the error url' do @@ -110,12 +117,15 @@ }.with_indifferent_access end - before do + it 'calls decode_token' do + response + expect(PivCacService).to have_received(:decode_token).with(token) { data } - sign_in user end it 'tracks the login attempt' do + response + expect(@analytics).to have_logged_event( :piv_cac_login, errors: {}, @@ -125,10 +135,10 @@ end it 'sets the session correctly' do - expect(controller.user_session[:new_device]).to eq(true) + response + expect(controller.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]). to eq false - expect(controller.auth_methods_session.auth_events).to match( [ { @@ -139,7 +149,15 @@ ) end + it 'sets new device session value' do + expect(controller).to receive(:set_new_device_session) + + response + end + it 'tracks the user_marked_authed event' do + response + expect(@analytics).to have_logged_event( 'User marked authenticated', authentication_type: :valid_2fa, @@ -147,6 +165,8 @@ end it 'saves the piv_cac session information' do + response + session_info = { subject: data[:subject], issuer: data[:issuer], @@ -155,16 +175,6 @@ expect(controller.user_session[:decrypted_x509]).to eq session_info.to_json end - context 'from existing device' do - before do - allow(user).to receive(:new_device?).and_return(false) - end - - it 'sets the session correctly' do - expect(controller.user_session[:new_device]).to eq(true) - end - end - context 'when the user has not accepted the most recent terms of use' do let(:user) do build(:user, accepted_terms_at: IdentityConfig.store.rules_of_use_updated_at - 1.year) @@ -177,6 +187,8 @@ describe 'it handles the otp_context' do it 'tracks the user_marked_authed event' do + response + expect(@analytics).to have_logged_event( 'User marked authenticated', authentication_type: :valid_2fa, diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 49b782f7303..561746547d1 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -41,52 +41,83 @@ describe 'POST /' do include AccountResetHelper - it 'tracks the successful authentication for existing user' do - user = create(:user, :fully_registered) + context 'successful authentication' do + let(:user) { create(:user, :fully_registered) } - stub_analytics - stub_attempts_tracker - analytics_hash = { - success: true, - user_id: user.uuid, - user_locked_out: false, - bad_password_count: 0, - sp_request_url_present: false, - remember_device: false, - } + subject(:response) do + post :create, params: { user: { email: user.email, password: user.password } } + end - expect(@analytics).to receive(:track_event). - with('Email and Password Authentication', analytics_hash) + it 'tracks the successful authentication for existing user' do + stub_analytics + stub_attempts_tracker - expect(@irs_attempts_api_tracker).to receive(:login_email_and_password_auth). - with(email: user.email, success: true) + response - post :create, params: { user: { email: user.email, password: user.password } } - expect(subject.session[:sign_in_flow]).to eq(:sign_in) - end + expect(@analytics).to have_logged_event( + 'Email and Password Authentication', + success: true, + user_id: user.uuid, + user_locked_out: false, + bad_password_count: 0, + sp_request_url_present: false, + remember_device: false, + ) + end - it 'saves and refreshes cookie for device for successful authentication' do - user = create(:user, :fully_registered) + it 'assigns sign_in_flow session value' do + response + + expect(controller.session[:sign_in_flow]).to eq(:sign_in) + end - first_expires = nil + it 'sets new device session value' do + expect(controller).to receive(:set_new_device_session) - freeze_time do - post :create, params: { user: { email: user.email, password: user.password } } + response + end - device_cookie = response.headers['set-cookie'].find { |c| c.start_with?('device=') } - first_expires = CGI::Cookie.parse(device_cookie)['expires'].first - expect(Time.zone.parse(first_expires)).to be >= 20.years.from_now + it 'schedules new device alert' do + expect(UserAlerts::AlertUserAboutNewDevice).to receive(:schedule_alert) do |event:| + expect(event).to eq(user.events.where(event_type: :sign_in_before_2fa).last) + end + + response end - sign_out(user) - expect(cookies[:device]).to be_present + it 'saves and refreshes cookie for device for successful authentication' do + first_expires = nil - travel_to 10.minutes.from_now do - post :create, params: { user: { email: user.email, password: user.password } } + freeze_time do + device_cookie = response.headers['set-cookie'].find { |c| c.start_with?('device=') } + first_expires = Time.zone.parse(CGI::Cookie.parse(device_cookie)['expires'].first) + expect(first_expires).to be >= 20.years.from_now + end + + sign_out(user) + expect(cookies[:device]).to be_present + + travel_to 10.minutes.from_now do + response = post :create, params: { user: { email: user.email, password: user.password } } + + device_cookie = response.headers['set-cookie'].find { |c| c.start_with?('device=') } + second_expires = Time.zone.parse(CGI::Cookie.parse(device_cookie)['expires'].first) + expect(second_expires).to be >= first_expires + 10.minutes + end + end - device_cookie = response.headers['set-cookie'].find { |c| c.start_with?('device=') } - second_expires = CGI::Cookie.parse(device_cookie)['expires'].first - expect(Time.zone.parse(second_expires)).to be >= Time.zone.parse(first_expires) + 10.minutes + context 'with authenticated device' do + let(:user) { create(:user, :with_authenticated_device) } + + before do + request.cookies[:device] = user.devices.last.cookie_uuid + end + + it 'does not schedule new device alert' do + expect(UserAlerts::AlertUserAboutNewDevice).not_to receive(:schedule_alert) + + response + end end end diff --git a/spec/factories/devices.rb b/spec/factories/devices.rb index dded2c5574a..0ba25b7e7c1 100644 --- a/spec/factories/devices.rb +++ b/spec/factories/devices.rb @@ -8,5 +8,14 @@ last_used_at { Time.zone.now } last_ip { '127.0.0.1' } user + + trait :authenticated do + events do + [ + association(:event, event_type: :sign_in_before_2fa), + association(:event, event_type: :sign_in_after_2fa), + ] + end + end end end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index f3dd63cf6d6..e9a3dbb05fc 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -177,6 +177,13 @@ end end + trait :with_authenticated_device do + fully_registered + after(:create) do |user| + user.devices << create(:device, :authenticated, user:) + end + end + trait :unconfirmed do confirmed_at { nil } password { nil } diff --git a/spec/features/new_device_tracking_spec.rb b/spec/features/new_device_tracking_spec.rb index 7f300f26f1f..49ebc7bd324 100644 --- a/spec/features/new_device_tracking_spec.rb +++ b/spec/features/new_device_tracking_spec.rb @@ -6,11 +6,11 @@ let(:user) { create(:user, :fully_registered) } context 'user has existing devices and aggregated new device alerts is disabled' do + let(:user) { create(:user, :with_authenticated_device) } before do allow(IdentityConfig.store).to receive( :feature_new_device_alert_aggregation_enabled, ).and_return(false) - create(:device, user: user) end it 'sends a user notification on signin' do @@ -46,11 +46,11 @@ end context 'user has existing devices and aggregated new device alerts is enabled' do + let(:user) { create(:user, :with_authenticated_device) } before do allow(IdentityConfig.store).to receive( :feature_new_device_alert_aggregation_enabled, ).and_return(true) - create(:device, user: user) end it 'sends a user notification on signin' do @@ -64,6 +64,63 @@ ) end + it 'sends all notifications for an expired sign-in session' do + allow(IdentityConfig.store).to receive(:new_device_alert_delay_in_minutes).and_return(5) + allow(IdentityConfig.store).to receive(:session_timeout_warning_seconds).and_return(15) + + sign_in_user(user) + + # Notified after expired delay for successful email password, but incomplete MFA + travel_to 6.minutes.from_now do + CreateNewDeviceAlert.new.perform(Time.zone.now) + open_last_email + email_page = Capybara::Node::Simple.new(current_email.default_part_body) + expect(email_page).to have_css( + '.usa-table td.font-family-mono', + count: 1, + text: t('user_mailer.new_device_sign_in_attempts.events.sign_in_before_2fa'), + ) + end + + reset_email + + travel_to 16.minutes.from_now do + visit root_url + expect(current_path).to eq(new_user_session_path) + sign_in_user(user) + end + + # Notified after session expired, user returned for another successful email password, no MFA + travel_to 22.minutes.from_now do + CreateNewDeviceAlert.new.perform(Time.zone.now) + open_last_email + email_page = Capybara::Node::Simple.new(current_email.default_part_body) + expect(email_page).to have_css( + '.usa-table td.font-family-mono', + count: 1, + text: t('user_mailer.new_device_sign_in_attempts.events.sign_in_before_2fa'), + ) + end + + reset_email + + # Notified after session expired, user returned for successful email password and MFA + travel_to 38.minutes.from_now do + visit root_url + expect(current_path).to eq(new_user_session_path) + sign_in_live_with_2fa(user) + open_last_email + email_page = Capybara::Node::Simple.new(current_email.default_part_body) + expect(email_page).to have_css('.usa-table td.font-family-mono', count: 2) + expect(email_page).to have_content( + t('user_mailer.new_device_sign_in_attempts.events.sign_in_before_2fa'), + ) + expect(email_page).to have_content( + t('user_mailer.new_device_sign_in_attempts.events.sign_in_after_2fa'), + ) + end + end + context 'from existing device' do before do Capybara.current_session.driver.browser.current_session.cookie_jar[:device] = diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 72f86878ae8..67798ff36d4 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1589,6 +1589,42 @@ def it_should_not_send_survey end end + describe '#authenticated_device?' do + let(:user) { create(:user, :fully_registered) } + let(:device) { create(:device, user:) } + let(:cookie_uuid) { device.cookie_uuid } + subject(:result) { user.authenticated_device?(cookie_uuid:) } + + context 'with blank cookie uuid' do + let(:cookie_uuid) { nil } + + it { expect(result).to eq(false) } + end + + context 'with cookie uuid not matching user device' do + let(:cookie_uuid) { 'invalid' } + + it { expect(result).to eq(false) } + end + + context 'with existing device without sign_in_after_2fa event' do + before do + create(:event, device:, event_type: :sign_in_before_2fa) + end + + it { expect(result).to eq(false) } + end + + context 'with existing device with sign_in_after_2fa event' do + before do + create(:event, device:, event_type: :sign_in_before_2fa) + create(:event, device:, event_type: :sign_in_after_2fa) + end + + it { expect(result).to eq(true) } + end + end + describe '#password_reset_profile' do let(:user) { create(:user) } diff --git a/spec/services/user_alerts/alert_user_about_new_device_spec.rb b/spec/services/user_alerts/alert_user_about_new_device_spec.rb index 321619d2e9a..c69aa11899a 100644 --- a/spec/services/user_alerts/alert_user_about_new_device_spec.rb +++ b/spec/services/user_alerts/alert_user_about_new_device_spec.rb @@ -14,11 +14,10 @@ ).and_return(true) end - it 'sets the user sign_in_new_device_at value to time of the given event' do + it 'does not send any emails' do described_class.call(event:, device:, disavowal_token:) - expect(user.sign_in_new_device_at.change(usec: 0)).to be_present. - and eq(event.created_at.change(usec: 0)) + expect_delivered_email_count(0) end end @@ -51,6 +50,34 @@ end end + describe '.schedule_alert' do + subject(:result) { described_class.schedule_alert(event:) } + + context 'aggregated new device alerts enabled' do + before do + allow(IdentityConfig.store).to receive(:feature_new_device_alert_aggregation_enabled). + and_return(true) + end + + it 'sets the user sign_in_new_device_at value to time of the given event' do + expect { result }.to change { user.reload.sign_in_new_device_at&.change(usec: 0) }. + from(nil). + to(event.created_at.change(usec: 0)) + end + end + + context 'aggregated new device alerts disabled' do + before do + allow(IdentityConfig.store).to receive(:feature_new_device_alert_aggregation_enabled). + and_return(false) + end + + it 'does not set sign_in_new_device_at value' do + expect { result }.not_to change { user.reload.sign_in_new_device_at&.change(usec: 0) } + end + end + end + describe '.send_alert' do let(:sign_in_new_device_at) { 3.minutes.ago } let(:user) { create(:user, :fully_registered, sign_in_new_device_at:) }