From a6d95d55d9d738ef31a1d520d23a2ee74a62d906 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 27 Feb 2025 15:01:15 -0500 Subject: [PATCH 01/10] LG-15833: Maintain partner link when session times out at sign-in changelog: Bug Fixes, Sign-In, Maintain partner link when session times out at sign-in --- .../packs/session-expire-session.ts | 24 +++++++++---------- app/views/session_timeout/_warning.html.erb | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/javascript/packs/session-expire-session.ts b/app/javascript/packs/session-expire-session.ts index fd94090aeb3..37d4234b6b3 100644 --- a/app/javascript/packs/session-expire-session.ts +++ b/app/javascript/packs/session-expire-session.ts @@ -8,9 +8,8 @@ const warning = Number(warningEl.dataset.warning!) * 1000; const sessionsURL = warningEl.dataset.sessionsUrl!; const sessionTimeout = Number(warningEl.dataset.sessionTimeoutIn!) * 1000; const modal = document.querySelector('lg-modal.session-timeout-modal')!; -const keepaliveEl = document.getElementById('session-keepalive-btn'); +const keepaliveButton = document.getElementById('session-keepalive-btn')!; const countdownEls: NodeListOf = modal.querySelectorAll('lg-countdown'); -const timeoutRefreshPath = warningEl.dataset.timeoutRefreshPath || ''; let sessionExpiration = new Date(Date.now() + sessionTimeout); @@ -22,19 +21,20 @@ function showModal() { }); } -function keepalive() { +function keepalive(event: MouseEvent) { const isExpired = new Date() > sessionExpiration; if (isExpired) { - document.location.href = timeoutRefreshPath; - } else { - modal.hide(); - sessionExpiration = new Date(Date.now() + sessionTimeout); - - setTimeout(showModal, sessionTimeout - warning); - countdownEls.forEach((countdownEl) => countdownEl.stop()); - extendSession(sessionsURL); + return; } + + event.preventDefault(); + modal.hide(); + sessionExpiration = new Date(Date.now() + sessionTimeout); + + setTimeout(showModal, sessionTimeout - warning); + countdownEls.forEach((countdownEl) => countdownEl.stop()); + extendSession(sessionsURL); } -keepaliveEl?.addEventListener('click', keepalive, false); +keepaliveButton.addEventListener('click', keepalive); setTimeout(showModal, sessionTimeout - warning); diff --git a/app/views/session_timeout/_warning.html.erb b/app/views/session_timeout/_warning.html.erb index 62faad7810b..7ddd4ba1141 100644 --- a/app/views/session_timeout/_warning.html.erb +++ b/app/views/session_timeout/_warning.html.erb @@ -38,7 +38,7 @@

<%= render ButtonComponent.new( - type: :button, + url: url_for(request_id: sp_session[:request_id]), id: 'session-keepalive-btn', big: true, full_width: true, From 2fbfab6ef614ebdd4c9997b8c1da38abfff6d46a Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 28 Feb 2025 09:10:30 -0500 Subject: [PATCH 02/10] Change session timeout config unit to seconds Why: 1. To simplify stubbing tests to simulate very short session timeout (e.g. 1 second) to avoid delays in tests waiting 2. For alignment with related `session_timeout_warning_seconds` config 3. To allow more granular control over session durations --- app/controllers/application_controller.rb | 6 +++--- app/forms/openid_connect_token_form.rb | 2 +- config/application.yml.default | 2 +- config/initializers/ahoy.rb | 2 +- config/initializers/devise.rb | 2 +- config/initializers/session_store.rb | 2 +- lib/identity_config.rb | 2 +- spec/controllers/application_controller_spec.rb | 6 +++--- spec/features/users/sign_in_spec.rb | 5 ++++- 9 files changed, 16 insertions(+), 13 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index faeb5f09a97..638fe5165c3 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -149,7 +149,7 @@ def cache_issuer_in_cookie else { value: current_sp.issuer, - expires: IdentityConfig.store.session_timeout_in_minutes.minutes, + expires: IdentityConfig.store.session_timeout_in_seconds.seconds, } end end @@ -162,12 +162,12 @@ def redirect_with_flash_if_timeout flash[:info] = t( 'notices.session_timedout', app_name: APP_NAME, - minutes: IdentityConfig.store.session_timeout_in_minutes, + minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes, ) elsif current_user.blank? flash[:info] = t( 'notices.session_cleared', - minutes: IdentityConfig.store.session_timeout_in_minutes, + minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes, ) end diff --git a/app/forms/openid_connect_token_form.rb b/app/forms/openid_connect_token_form.rb index 02fc37d7648..fe07f5548e2 100644 --- a/app/forms/openid_connect_token_form.rb +++ b/app/forms/openid_connect_token_form.rb @@ -34,7 +34,7 @@ def initialize(params) ATTRS.each do |key| instance_variable_set(:"@#{key}", params[key]) end - @session_expiration = IdentityConfig.store.session_timeout_in_minutes.minutes.ago + @session_expiration = IdentityConfig.store.session_timeout_in_seconds.seconds.ago @identity = find_identity_with_code end diff --git a/config/application.yml.default b/config/application.yml.default index 40b68934712..23e66c2e61f 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -383,7 +383,7 @@ session_check_delay: 30 session_check_frequency: 30 session_encryption_key: session_encryptor_alert_enabled: false -session_timeout_in_minutes: 15 +session_timeout_in_seconds: 900 session_timeout_warning_seconds: 150 session_total_duration_timeout_in_minutes: 720 short_term_phone_otp_max_attempt_window_in_seconds: 10 diff --git a/config/initializers/ahoy.rb b/config/initializers/ahoy.rb index 8b9db230954..7b7be65b522 100644 --- a/config/initializers/ahoy.rb +++ b/config/initializers/ahoy.rb @@ -15,7 +15,7 @@ Ahoy.api = false # Period of inactivity before a new visit is created -Ahoy.visit_duration = IdentityConfig.store.session_timeout_in_minutes.minutes +Ahoy.visit_duration = IdentityConfig.store.session_timeout_in_seconds.seconds Ahoy.server_side_visits = false Ahoy.geocode = false Ahoy.user_agent_parser = :browser diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 9c625df443d..2a89edfcc1c 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -24,7 +24,7 @@ config.skip_session_storage = [:http_auth] config.strip_whitespace_keys = [] config.stretches = Rails.env.test? ? 1 : 12 - config.timeout_in = IdentityConfig.store.session_timeout_in_minutes.minutes + config.timeout_in = IdentityConfig.store.session_timeout_in_seconds.seconds config.warden do |manager| manager.failure_app = CustomDeviseFailureApp diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb index b2002fb3957..6681028d13e 100644 --- a/config/initializers/session_store.rb +++ b/config/initializers/session_store.rb @@ -16,7 +16,7 @@ write_private_id: true, # Redis expires session after N minutes - ttl: IdentityConfig.store.session_timeout_in_minutes.minutes, + ttl: IdentityConfig.store.session_timeout_in_seconds.seconds, key_prefix: "#{IdentityConfig.store.domain_name}:session:", client_pool: REDIS_POOL, diff --git a/lib/identity_config.rb b/lib/identity_config.rb index b1b58cceb31..81a2a028c04 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -417,7 +417,7 @@ def self.store config.add(:session_check_frequency, type: :integer) config.add(:session_encryption_key, type: :string) config.add(:session_encryptor_alert_enabled, type: :boolean) - config.add(:session_timeout_in_minutes, type: :integer) + config.add(:session_timeout_in_seconds, type: :integer) config.add(:session_timeout_warning_seconds, type: :integer) config.add(:session_total_duration_timeout_in_minutes, type: :integer) config.add(:show_unsupported_passkey_platform_authentication_setup, type: :boolean) diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 9551085d265..0bfd7d790f7 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -37,7 +37,7 @@ def index expect(cookies[:sp_issuer]).to eq(sp.issuer) expect(cookie_expiration).to be_within(3.seconds).of( - IdentityConfig.store.session_timeout_in_minutes.minutes.from_now, + IdentityConfig.store.session_timeout_in_seconds.seconds.from_now, ) end end @@ -414,7 +414,7 @@ def index expect(flash[:info]).to eq t( 'notices.session_timedout', app_name: APP_NAME, - minutes: IdentityConfig.store.session_timeout_in_minutes, + minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes, ) end end @@ -447,7 +447,7 @@ def index expect(flash[:info]).to eq t( 'notices.session_cleared', - minutes: IdentityConfig.store.session_timeout_in_minutes, + minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes, ) end end diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 4228e237c53..bd10674c3c0 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -199,7 +199,10 @@ scenario 'user sees warning before session times out' do minutes_and = [ - t('datetime.dotiw.minutes', count: IdentityConfig.store.session_timeout_in_minutes - 1), + t( + 'datetime.dotiw.minutes', + count: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes - 1, + ), t('datetime.dotiw.two_words_connector'), ].join('') From e8641e5de802bee44e82edac324c8b4f6d3c0059 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 28 Feb 2025 09:10:57 -0500 Subject: [PATCH 03/10] Add test coverage for partner request after session expires --- spec/features/users/sign_in_spec.rb | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index bd10674c3c0..c623b617aa6 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -295,7 +295,7 @@ expect(page).to have_css('.usa-js-modal--active', wait: 10) - click_button t('notices.timeout_warning.partially_signed_in.continue') + click_on t('notices.timeout_warning.partially_signed_in.continue') expect(page).not_to have_css('.usa-js-modal--active') expect(find_field(t('forms.registration.labels.email')).value).not_to be_blank @@ -312,10 +312,29 @@ expect(page).to have_css('.usa-js-modal--active', wait: 10) - click_button t('notices.timeout_warning.partially_signed_in.continue') + click_on t('notices.timeout_warning.partially_signed_in.continue') expect(find_field(t('account.index.email')).value).not_to be_blank end + it 'maintains partner request if the user continues after the session expires', js: true do + allow(IdentityConfig.store).to receive(:session_timeout_in_seconds).and_return(1) + allow(IdentityConfig.store).to receive(:session_check_delay).and_return(0) + + visit_idp_from_sp_with_ial1(:oidc) + + expect(page).to have_css('.usa-js-modal--active', wait: 10) + expect(page).to have_content( + t( + 'notices.timeout_warning.partially_signed_in.message_html', + time_left_in_session_html: t('datetime.dotiw.seconds', count: 0), + ), + wait: 10, + ) + + click_on t('notices.timeout_warning.partially_signed_in.continue') + expect_branded_experience + end + it 'reloads the sign in page when cancel is clicked', js: true do allow(Devise).to receive(:timeout_in) .and_return(IdentityConfig.store.session_timeout_warning_seconds + 1) From 4edfbbc80d9e5c6ffbfe25cf6eb2d816d921818f Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 28 Feb 2025 09:52:33 -0500 Subject: [PATCH 04/10] Stub url_for in view spec --- spec/views/layouts/application.html.erb_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/views/layouts/application.html.erb_spec.rb b/spec/views/layouts/application.html.erb_spec.rb index c7565dccf21..6b2d93c1c7d 100644 --- a/spec/views/layouts/application.html.erb_spec.rb +++ b/spec/views/layouts/application.html.erb_spec.rb @@ -16,6 +16,7 @@ ) allow(view.request).to receive(:original_fullpath).and_return('/foobar') allow(view).to receive(:user_fully_authenticated?).and_return(false) + allow(view).to receive(:url_for).and_return('/') view.title = title_content if title_content end From 57d70573a40229369e234891eabecebc1baa7629 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 28 Feb 2025 10:06:01 -0500 Subject: [PATCH 05/10] Ensure minutes conversion uses integer value --- app/controllers/application_controller.rb | 4 ++-- spec/controllers/application_controller_spec.rb | 4 ++-- spec/features/users/sign_in_spec.rb | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 638fe5165c3..b3d6d9af022 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -162,12 +162,12 @@ def redirect_with_flash_if_timeout flash[:info] = t( 'notices.session_timedout', app_name: APP_NAME, - minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes, + minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes.to_i, ) elsif current_user.blank? flash[:info] = t( 'notices.session_cleared', - minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes, + minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes.to_i, ) end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 0bfd7d790f7..dc190a3cb63 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -414,7 +414,7 @@ def index expect(flash[:info]).to eq t( 'notices.session_timedout', app_name: APP_NAME, - minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes, + minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes.to_i, ) end end @@ -447,7 +447,7 @@ def index expect(flash[:info]).to eq t( 'notices.session_cleared', - minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes, + minutes: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes.to_i, ) end end diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index c623b617aa6..2b42fddad5b 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -201,7 +201,7 @@ minutes_and = [ t( 'datetime.dotiw.minutes', - count: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes - 1, + count: IdentityConfig.store.session_timeout_in_seconds.seconds.in_minutes.to_i - 1, ), t('datetime.dotiw.two_words_connector'), ].join('') From e5ce6a52b1e3d4b98a5de4cf80afc0730b7839b2 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 28 Feb 2025 10:06:29 -0500 Subject: [PATCH 06/10] Prevent link behavior in session timeout pack --- app/javascript/packs/session-timeout-ping.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/javascript/packs/session-timeout-ping.ts b/app/javascript/packs/session-timeout-ping.ts index a86f60254b5..0a9b4b824f9 100644 --- a/app/javascript/packs/session-timeout-ping.ts +++ b/app/javascript/packs/session-timeout-ping.ts @@ -15,7 +15,7 @@ const timeoutURL = warningEl?.dataset.timeoutUrl!; const sessionsURL = warningEl?.dataset.sessionsUrl!; const modal = document.querySelector('lg-modal.session-timeout-modal')!; -const keepaliveEl = document.getElementById('session-keepalive-btn'); +const keepaliveButton = document.getElementById('session-keepalive-btn')!; const countdownEls: NodeListOf = modal.querySelectorAll('lg-countdown'); function success({ isLive, timeout }: SessionStatus) { @@ -46,11 +46,12 @@ function success({ isLive, timeout }: SessionStatus) { const ping = () => requestSessionStatus(sessionsURL).then(success); -function keepalive() { +function keepalive(event: MouseEvent) { + event.preventDefault(); modal.hide(); countdownEls.forEach((countdownEl) => countdownEl.stop()); extendSession(sessionsURL); } -keepaliveEl?.addEventListener('click', keepalive, false); +keepaliveButton.addEventListener('click', keepalive); setTimeout(ping, start); From 334ea045682839be3d410f6d6d1caa23268d8c41 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 28 Feb 2025 10:07:13 -0500 Subject: [PATCH 07/10] Always redirect expired session to sign-in page e.g. they might be partially signed-in at the MFA step, but in that case we'd still want to bring them back to the sign-in page, since the SessionsController is where the request_id is maintained --- app/views/session_timeout/_warning.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/session_timeout/_warning.html.erb b/app/views/session_timeout/_warning.html.erb index 7ddd4ba1141..316afd03c3a 100644 --- a/app/views/session_timeout/_warning.html.erb +++ b/app/views/session_timeout/_warning.html.erb @@ -38,7 +38,7 @@

<%= render ButtonComponent.new( - url: url_for(request_id: sp_session[:request_id]), + url: new_user_session_url(timeout: :session, request_id: sp_session[:request_id]), id: 'session-keepalive-btn', big: true, full_width: true, From 5b4ae35a33938fcb5742dd8c4782bcaf81332893 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 28 Feb 2025 10:08:26 -0500 Subject: [PATCH 08/10] Update button expectation for signed-in session timeout --- spec/features/users/sign_in_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 2b42fddad5b..7722e3e6906 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -221,7 +221,7 @@ scenario 'user can continue browsing with refreshed CSRF token' do token = first('[name=authenticity_token]', visible: false).value - click_button t('notices.timeout_warning.signed_in.continue') + click_on t('notices.timeout_warning.signed_in.continue') expect(page).not_to have_css('.usa-js-modal--active') expect(page).to have_css( "[name=authenticity_token]:not([value='#{token}'])", From ddbabe2b69e3f605d1093b22927fb2cde33c5220 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 28 Feb 2025 10:34:04 -0500 Subject: [PATCH 09/10] Try waiting for content instead of modal --- spec/features/users/sign_in_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 7722e3e6906..62d3429f10d 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -322,7 +322,6 @@ visit_idp_from_sp_with_ial1(:oidc) - expect(page).to have_css('.usa-js-modal--active', wait: 10) expect(page).to have_content( t( 'notices.timeout_warning.partially_signed_in.message_html', From f9e97653d983dca1e2868b491c9b9d8c181e2430 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 28 Feb 2025 10:52:26 -0500 Subject: [PATCH 10/10] Stub Devise timeout This should be assigned through session_timeout_in_seconds, but it may already be cached by the time we've assigned the stub on IdentityConfig --- spec/features/users/sign_in_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 62d3429f10d..d46d8e64c21 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -319,6 +319,7 @@ it 'maintains partner request if the user continues after the session expires', js: true do allow(IdentityConfig.store).to receive(:session_timeout_in_seconds).and_return(1) allow(IdentityConfig.store).to receive(:session_check_delay).and_return(0) + allow(Devise).to receive(:timeout_in).and_return(1) visit_idp_from_sp_with_ial1(:oidc)