From ac47235696ec53c811fd0c5e5a87186b58be20de Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 28 Mar 2023 09:32:32 -0400 Subject: [PATCH 1/4] Remove redundant "remaining" value from timeout API changelog: Internal, Session Timeout, Remove redundant timeout value from session timeout API --- app/controllers/users/sessions_controller.rb | 8 ++--- app/javascript/packs/session-timeout-ping.ts | 7 +--- .../users/sessions_controller_spec.rb | 36 ------------------- 3 files changed, 3 insertions(+), 48 deletions(-) diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 5d3971a1854..65f59aa7e7c 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -54,14 +54,14 @@ def destroy def active session[:pinged_at] = now Rails.logger.debug(alive?: alive?, expires_at: expires_at) - render json: { live: alive?, timeout: expires_at, remaining: remaining_session_time } + render json: { live: alive?, timeout: expires_at } end def keepalive session[:session_expires_at] = now + Devise.timeout_in if alive? analytics.session_kept_alive if alive? - render json: { live: alive?, timeout: expires_at, remaining: remaining_session_time } + render json: { live: alive?, timeout: expires_at } end def timeout @@ -152,10 +152,6 @@ def expires_at session[:session_expires_at]&.to_datetime || (now - 1) end - def remaining_session_time - expires_at.to_i - Time.zone.now.to_i - end - def browser_is_ie11? BrowserCache.parse(request.user_agent).ie?(11) end diff --git a/app/javascript/packs/session-timeout-ping.ts b/app/javascript/packs/session-timeout-ping.ts index 882e2f2b5aa..290101bf865 100644 --- a/app/javascript/packs/session-timeout-ping.ts +++ b/app/javascript/packs/session-timeout-ping.ts @@ -23,11 +23,6 @@ interface PingResponse { */ live: boolean; - /** - * Time remaining in active session, in seconds. - */ - remaining: number; - /** * ISO8601-formatted date string for session timeout. */ @@ -64,7 +59,7 @@ function handleTimeout(redirectURL: string) { } function success(data: PingResponse) { - let timeRemaining = data.remaining * 1000; + let timeRemaining = new Date(data.timeout).valueOf() - Date.now(); const showWarning = timeRemaining < warning; if (!data.live) { diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 0aa82d80071..6bcf286f36e 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -50,15 +50,6 @@ expect(json['timeout'].to_datetime.to_i).to eq(timeout.to_i) end - - it 'includes the remaining key', freeze_time: true do - controller.session[:session_expires_at] = Time.zone.now + 10 - get :active - - json ||= JSON.parse(response.body) - - expect(json['remaining']).to eq(10) - end end context 'when user is not present' do @@ -78,14 +69,6 @@ expect(json['timeout'].to_datetime.to_i).to eq(Time.zone.now.to_i - 1) end - it 'includes the remaining time', freeze_time: true do - get :active - - json ||= JSON.parse(response.body) - - expect(json['remaining']).to eq(-1) - end - it 'updates the pinged_at session key' do stub_sign_in now = Time.zone.now @@ -721,17 +704,6 @@ ) end - it 'resets the remaining key' do - controller.session[:session_expires_at] = Time.zone.now + 10 - post :keepalive - - json ||= JSON.parse(response.body) - - expect(json['remaining']).to be_within(1).of( - IdentityConfig.store.session_timeout_in_minutes * 60, - ) - end - it 'tracks session refresh visit' do controller.session[:session_expires_at] = Time.zone.now + 10 stub_analytics @@ -758,14 +730,6 @@ expect(json['timeout'].to_datetime.to_i).to be_within(1).of(Time.zone.now.to_i - 1) end - - it 'includes the remaining time' do - post :keepalive - - json ||= JSON.parse(response.body) - - expect(json['remaining']).to eq(-1) - end end it 'does not track analytics event' do From a5344478cdb057386974aabe0d0b3cbb06ceda59 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 28 Mar 2023 14:13:32 -0400 Subject: [PATCH 2/4] Revert session API remaining removal Temporary for first deploy, see https://github.com/18F/identity-idp/pull/8084/files#r1150918192 --- app/controllers/users/sessions_controller.rb | 8 +++-- .../users/sessions_controller_spec.rb | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 65f59aa7e7c..5d3971a1854 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -54,14 +54,14 @@ def destroy def active session[:pinged_at] = now Rails.logger.debug(alive?: alive?, expires_at: expires_at) - render json: { live: alive?, timeout: expires_at } + render json: { live: alive?, timeout: expires_at, remaining: remaining_session_time } end def keepalive session[:session_expires_at] = now + Devise.timeout_in if alive? analytics.session_kept_alive if alive? - render json: { live: alive?, timeout: expires_at } + render json: { live: alive?, timeout: expires_at, remaining: remaining_session_time } end def timeout @@ -152,6 +152,10 @@ def expires_at session[:session_expires_at]&.to_datetime || (now - 1) end + def remaining_session_time + expires_at.to_i - Time.zone.now.to_i + end + def browser_is_ie11? BrowserCache.parse(request.user_agent).ie?(11) end diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 6bcf286f36e..0aa82d80071 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -50,6 +50,15 @@ expect(json['timeout'].to_datetime.to_i).to eq(timeout.to_i) end + + it 'includes the remaining key', freeze_time: true do + controller.session[:session_expires_at] = Time.zone.now + 10 + get :active + + json ||= JSON.parse(response.body) + + expect(json['remaining']).to eq(10) + end end context 'when user is not present' do @@ -69,6 +78,14 @@ expect(json['timeout'].to_datetime.to_i).to eq(Time.zone.now.to_i - 1) end + it 'includes the remaining time', freeze_time: true do + get :active + + json ||= JSON.parse(response.body) + + expect(json['remaining']).to eq(-1) + end + it 'updates the pinged_at session key' do stub_sign_in now = Time.zone.now @@ -704,6 +721,17 @@ ) end + it 'resets the remaining key' do + controller.session[:session_expires_at] = Time.zone.now + 10 + post :keepalive + + json ||= JSON.parse(response.body) + + expect(json['remaining']).to be_within(1).of( + IdentityConfig.store.session_timeout_in_minutes * 60, + ) + end + it 'tracks session refresh visit' do controller.session[:session_expires_at] = Time.zone.now + 10 stub_analytics @@ -730,6 +758,14 @@ expect(json['timeout'].to_datetime.to_i).to be_within(1).of(Time.zone.now.to_i - 1) end + + it 'includes the remaining time' do + post :keepalive + + json ||= JSON.parse(response.body) + + expect(json['remaining']).to eq(-1) + end end it 'does not track analytics event' do From 97265e22685df54c65b5d9ecf1ddc749c850cfa7 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 28 Mar 2023 15:54:14 -0400 Subject: [PATCH 3/4] Optimistically hide modal on session continue 1. Fixes specs 2. Better user experience, don't have to wait for request to finish before modal is closed, thus making app feel more responsive 3. The previous result handling with `success` would cause a second polling interval to begin --- app/javascript/packs/session-timeout-ping.ts | 11 +++++------ spec/features/users/sign_in_spec.rb | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/javascript/packs/session-timeout-ping.ts b/app/javascript/packs/session-timeout-ping.ts index 290101bf865..2f25db66270 100644 --- a/app/javascript/packs/session-timeout-ping.ts +++ b/app/javascript/packs/session-timeout-ping.ts @@ -75,9 +75,6 @@ function success(data: PingResponse) { countdownEl.expiration = new Date(data.timeout); countdownEl.start(); }); - } else { - modal.hide(); - countdownEls.forEach((countdownEl) => countdownEl.stop()); } if (timeRemaining < frequency) { @@ -97,9 +94,11 @@ function ping() { } function keepalive() { - request('/sessions/keepalive', { method: 'POST' }) - .then(success) - .catch((error) => notifyNewRelic(error, 'keepalive')); + modal.hide(); + countdownEls.forEach((countdownEl) => countdownEl.stop()); + request('/sessions/keepalive', { method: 'POST' }).catch((error) => { + notifyNewRelic(error, 'keepalive'); + }); } keepaliveEl?.addEventListener('click', keepalive, false); diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index c3a7b1d0aa5..4b4f539caef 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -275,7 +275,7 @@ scenario 'user can continue browsing with refreshed CSRF token' do expect do click_button t('notices.timeout_warning.signed_in.continue') - expect(page).not_to have_css('.usa-js-modal--active', wait: 5) + expect(page).not_to have_css('.usa-js-modal--active') end.to change { find('[name=authenticity_token]', visible: false).value } expect(current_path).to eq forget_all_browsers_path From 51141920b88193b7973d8eebe83527fc32549745 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 28 Mar 2023 16:16:01 -0400 Subject: [PATCH 4/4] Bring over rest of spec changes from #7966 The CSRF won't be updated until after the request finishes, even if the modal will be disappeared immediately --- spec/features/users/sign_in_spec.rb | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 4b4f539caef..0ce969f8a61 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -273,15 +273,14 @@ end scenario 'user can continue browsing with refreshed CSRF token' do - expect do - click_button t('notices.timeout_warning.signed_in.continue') - expect(page).not_to have_css('.usa-js-modal--active') - end.to change { find('[name=authenticity_token]', visible: false).value } - - expect(current_path).to eq forget_all_browsers_path - - click_button t('forms.buttons.confirm') - expect(current_path).to eq account_path + token = find('[name=authenticity_token]', visible: false).value + click_button 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}'])", + visible: false, + wait: 5, + ) end scenario 'user has option to sign out' do