diff --git a/app/models/user.rb b/app/models/user.rb index 586b38ae419..02323a85587 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -66,8 +66,8 @@ def confirmed_email_addresses email_addresses.where.not(confirmed_at: nil).order('last_sign_in_at DESC NULLS LAST') end - def need_two_factor_authentication?(_request) - MfaPolicy.new(self).two_factor_enabled? + def fully_registered? + !!registration_log&.registered_at end def confirmed? @@ -320,7 +320,7 @@ def recent_devices map(&:decorate) end - def recent_devices? + def has_devices? !recent_devices.empty? end diff --git a/app/services/user_event_creator.rb b/app/services/user_event_creator.rb index 4408ae71b71..922eb54c60c 100644 --- a/app/services/user_event_creator.rb +++ b/app/services/user_event_creator.rb @@ -76,9 +76,7 @@ def build_disavowal_token # @return [Array(Event, String)] an (event, disavowal_token) tuple def create_event_for_new_device(event_type:, user:, disavowal_token:) - user_has_multiple_devices = user.recent_devices? - - if user_has_multiple_devices && disavowal_token.nil? + if user.fully_registered? && user.has_devices? && disavowal_token.nil? device, event, disavowal_token = Device.transaction do device = create_device_for_user(user) event, disavowal_token = create_user_event_with_disavowal( diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index bdfd10bee85..41c9682b0ca 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -39,6 +39,6 @@ unless bypass_by_cookie auth.session(options[:scope])[TwoFactorAuthenticatable::NEED_AUTHENTICATION] = - user.need_two_factor_authentication?(auth.request) + MfaPolicy.new(user).two_factor_enabled? end end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 2bf8cc5ae41..e63249a5eb9 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -177,6 +177,10 @@ trait :signed_up do with_phone + + after :create do |user| + user.create_registration_log(registered_at: Time.zone.now) + end end trait :unconfirmed do diff --git a/spec/features/new_device_tracking_spec.rb b/spec/features/new_device_tracking_spec.rb index b3bbe1f3550..2469cd02c29 100644 --- a/spec/features/new_device_tracking_spec.rb +++ b/spec/features/new_device_tracking_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' describe 'New device tracking' do + include SamlAuthHelper + let(:user) { create(:user, :signed_up) } context 'user has existing devices' do @@ -37,28 +39,20 @@ end end - context 'user does not have a phone configured' do - let(:user) { create(:user) } - - before do - create(:device, user: user) - end - - it 'does not send an SMS' do - sign_in_user(user) + context 'user signs up and confirms email in a different browser' do + let(:user) { build(:user) } - expect(user.reload.devices.length).to eq 2 + it 'does not send an email' do + perform_in_browser(:one) do + visit_idp_from_sp_with_ial1(:oidc) + sign_up_user_from_sp_without_confirming_email(user.email) + end - device = user.devices.order(created_at: :desc).first - expect_delivered_email_count(1) - expect_delivered_email( - to: [user.email_addresses.first.email], - subject: t('user_mailer.new_device_sign_in.subject', app_name: APP_NAME), - body: [device.last_used_at.in_time_zone('Eastern Time (US & Canada)'). - strftime('%B %-d, %Y %H:%M Eastern Time'), - 'From 127.0.0.1 (IP address potentially located in United States)'], - ) - expect(Telephony::Test::Message.messages.count).to eq 0 + perform_in_browser(:two) do + expect do + confirm_email_in_a_different_browser(user.email) + end.not_to change { ActionMailer::Base.deliveries.count } + end end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index cfa549b117a..91a92761ff6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -113,27 +113,26 @@ end end - context '#need_two_factor_authentication?' do - let(:request) { ActionController::TestRequest.new } - - it 'is true when two_factor_enabled' do - user = build_stubbed(:user) + describe '#fully_registered?' do + let(:user) { create(:user) } + subject(:fully_registered?) { user.fully_registered? } - mock_mfa = MfaPolicy.new(user) - allow(mock_mfa).to receive(:two_factor_enabled?).and_return true - allow(MfaPolicy).to receive(:new).with(user).and_return mock_mfa + context 'with unconfirmed user' do + let(:user) { create(:user, :unconfirmed) } - expect(user.need_two_factor_authentication?(nil)).to be_truthy + it { expect(fully_registered?).to eq(false) } end - it 'is false when not two_factor_enabled' do - user = build_stubbed(:user) + context 'with confirmed user' do + let(:user) { create(:user) } + + it { expect(fully_registered?).to eq(false) } + end - mock_mfa = MfaPolicy.new(user) - allow(mock_mfa).to receive(:two_factor_enabled?).and_return false - allow(MfaPolicy).to receive(:new).with(user).and_return(mock_mfa) + context 'with mfa-enabled user' do + let(:user) { create(:user, :signed_up) } - expect(user.need_two_factor_authentication?(nil)).to be_falsey + it { expect(fully_registered?).to eq(true) } end end @@ -1133,6 +1132,23 @@ def it_should_not_send_survey end end + describe '#has_devices?' do + let(:user) { create(:user) } + subject(:has_devices?) { user.has_devices? } + + context 'with no devices' do + it { expect(has_devices?).to eq(false) } + end + + context 'with a device' do + before do + create(:device, user:) + end + + it { expect(has_devices?).to eq(true) } + end + end + describe '#password_reset_profile' do let(:user) { create(:user) } diff --git a/spec/services/user_event_creator_spec.rb b/spec/services/user_event_creator_spec.rb index 954f5213e70..e43ade9a4db 100644 --- a/spec/services/user_event_creator_spec.rb +++ b/spec/services/user_event_creator_spec.rb @@ -18,7 +18,7 @@ cookie_jar: cookie_jar, ) end - let(:user) { create(:user) } + let(:user) { create(:user, :signed_up) } let(:device) { create(:device, user: user, cookie_uuid: existing_device_cookie) } let(:event_type) { 'account_created' } diff --git a/spec/support/features/session_helper.rb b/spec/support/features/session_helper.rb index b026522634b..b8e6e79bb5b 100644 --- a/spec/support/features/session_helper.rb +++ b/spec/support/features/session_helper.rb @@ -196,7 +196,6 @@ def sign_in_before_2fa(user = create(:user)) def sign_in_with_warden(user, auth_method: nil) login_as(user, scope: :user, run_callbacks: false) - allow(user).to receive(:need_two_factor_authentication?).and_return(false) Warden.on_next_request do |proxy| session = proxy.env['rack.session']