LG-13220: Fix aggregated new device sign-in for expired session

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

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

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,
       )

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],

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.
   #

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
 

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

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

spec/controllers/users/piv_cac_login_controller_spec.rb

@@ -61,24 +61,29 @@
           }.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
             expect(PivCacService).to have_received(:decode_token).with(token) { data }.twice
           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,14 +149,24 @@
             )
           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,
             )
           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,