From 91b6c234ecd9508aac0f2c02988a73f1833aa930 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Mon, 6 May 2024 14:13:08 -0400 Subject: [PATCH 01/21] changelog: Internal, Analytics, Adds property to SP redirect initiated event --- .../openid_connect/authorization_controller.rb | 3 +++ app/controllers/saml_idp_controller.rb | 3 +++ app/controllers/users/sessions_controller.rb | 1 + app/services/analytics_events.rb | 12 +++++++++++- .../openid_connect/authorization_controller_spec.rb | 1 + spec/controllers/saml_idp_controller_spec.rb | 5 +++++ spec/controllers/users/sessions_controller_spec.rb | 1 + 7 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 074d1dbfb82..0cca38af801 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -222,6 +222,9 @@ def track_events sign_in_flow: session[:sign_in_flow], vtr: sp_session[:vtr], acr_values: sp_session[:acr_values], + sign_in_duration_seconds: (Time.zone.now - Time.zone.parse( + session[:sign_in_page_visited_at], + )).seconds.to_i.round(2), ) track_billing_events end diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index b4dd3343e81..11ac7395639 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -187,6 +187,9 @@ def track_events sign_in_flow: session[:sign_in_flow], vtr: sp_session[:vtr], acr_values: sp_session[:acr_values], + sign_in_duration_seconds: (Time.zone.now - Time.zone.parse( + session[:sign_in_page_visited_at], + )).seconds.to_i.round(2), ) track_billing_events end diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 7b1555de346..f51c9a0c747 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -24,6 +24,7 @@ def new issuer: decorated_sp_session.sp_issuer, ) analytics.sign_in_page_visit(flash: flash[:alert]) + session[:sign_in_page_visited_at] = Time.zone.now super end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 2a4b3ea8482..b874bbe0812 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -5070,7 +5070,16 @@ def sp_inactive_visit # @param [String, nil] sign_in_flow # @param [String, nil] vtr # @param [String, nil] acr_values - def sp_redirect_initiated(ial:, billed_ial:, sign_in_flow:, vtr:, acr_values:, **extra) + # @param [Integer] sign_in_duration_seconds + def sp_redirect_initiated( + ial:, + billed_ial:, + sign_in_flow:, + vtr:, + acr_values:, + sign_in_duration_seconds:, + **extra + ) track_event( 'SP redirect initiated', ial:, @@ -5078,6 +5087,7 @@ def sp_redirect_initiated(ial:, billed_ial:, sign_in_flow:, vtr:, acr_values:, * sign_in_flow:, vtr: vtr, acr_values: acr_values, + sign_in_duration_seconds:, **extra, ) end diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 9f27bd1abe1..07cf927f843 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -52,6 +52,7 @@ before do stub_sign_in user session[:sign_in_flow] = sign_in_flow + session[:sign_in_page_visited_at] = (Time.zone.now - 2.minutes).to_s end context 'acr with valid params' do diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index ac763552800..1acfe422ea7 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -518,6 +518,7 @@ def name_id_version(format_urn) # All the tests here were written prior to the interstitial # authorization confirmation page so let's force the system # to skip past that page + session[:sign_in_page_visited_at] = (Time.zone.now - 2.minutes).to_s allow(controller).to receive(:auth_count).and_return(2) end @@ -817,6 +818,7 @@ def name_id_version(format_urn) expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: Idp::Constants::IAL2, + sign_in_duration_seconds: 120, billed_ial: Idp::Constants::IAL2, sign_in_flow:, acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, @@ -965,6 +967,7 @@ def name_id_version(format_urn) expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: 0, + sign_in_duration_seconds: 120, billed_ial: 2, sign_in_flow:, acr_values: Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF, @@ -2434,6 +2437,7 @@ def stub_requested_attributes expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: 1, + sign_in_duration_seconds: 120, billed_ial: 1, sign_in_flow: :sign_in, acr_values: [ @@ -2484,6 +2488,7 @@ def stub_requested_attributes expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: 1, + sign_in_duration_seconds: 120, billed_ial: 1, sign_in_flow: :sign_in, acr_values: [ diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 81ab41fc292..079aecc3674 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -619,6 +619,7 @@ it 'renders the new template' do get :new expect(response).to render_template(:new) + expect(subject.session[:sign_in_page_visited_at]).to_not be(nil) end it 'tracks page visit, any alert flashes, and the Devise stored location' do From 78d633a7d6eaca330a148716b991d910672b9063 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Mon, 6 May 2024 16:48:02 -0400 Subject: [PATCH 02/21] update controllers to account for legit nil values at login page visit (dual browsers, etc.) revise other tests to include sign_in_duration --- app/controllers/openid_connect/authorization_controller.rb | 2 +- app/controllers/saml_idp_controller.rb | 2 +- spec/features/users/sign_up_spec.rb | 1 + spec/support/features/session_helper.rb | 1 + spec/support/shared_examples/sign_in.rb | 1 + 5 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 0cca38af801..9b6d83c1feb 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -223,7 +223,7 @@ def track_events vtr: sp_session[:vtr], acr_values: sp_session[:acr_values], sign_in_duration_seconds: (Time.zone.now - Time.zone.parse( - session[:sign_in_page_visited_at], + session[:sign_in_page_visited_at] || Time.zone.now.to_s, )).seconds.to_i.round(2), ) track_billing_events diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 11ac7395639..1af0564a7df 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -188,7 +188,7 @@ def track_events vtr: sp_session[:vtr], acr_values: sp_session[:acr_values], sign_in_duration_seconds: (Time.zone.now - Time.zone.parse( - session[:sign_in_page_visited_at], + session[:sign_in_page_visited_at] || Time.zone.now.to_s, )).seconds.to_i.round(2), ) track_billing_events diff --git a/spec/features/users/sign_up_spec.rb b/spec/features/users/sign_up_spec.rb index 674321b43f9..c53687d893f 100644 --- a/spec/features/users/sign_up_spec.rb +++ b/spec/features/users/sign_up_spec.rb @@ -483,6 +483,7 @@ def clipboard_text expect(analytics).to have_logged_event( 'SP redirect initiated', ial: 1, + sign_in_duration_seconds: 1, billed_ial: 1, sign_in_flow: 'create_account', acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, diff --git a/spec/support/features/session_helper.rb b/spec/support/features/session_helper.rb index 4658d9ef97a..64e960bf0d0 100644 --- a/spec/support/features/session_helper.rb +++ b/spec/support/features/session_helper.rb @@ -183,6 +183,7 @@ def sign_in_with_warden(user, auth_method: nil, issuer: nil) Warden.on_next_request do |proxy| session = proxy.env['rack.session'] session['warden.user.user.session'] = {}.with_indifferent_access + session[:sign_in_page_visited_at] = (Time.zone.now - 2.minutes).to_s if auth_method session['warden.user.user.session']['auth_events'] = [{ auth_method:, at: Time.zone.now }] end diff --git a/spec/support/shared_examples/sign_in.rb b/spec/support/shared_examples/sign_in.rb index 16548dcb977..69839b6da96 100644 --- a/spec/support/shared_examples/sign_in.rb +++ b/spec/support/shared_examples/sign_in.rb @@ -47,6 +47,7 @@ expect(analytics).to have_logged_event( 'SP redirect initiated', ial: 1, + sign_in_duration_seconds: 1, billed_ial: 1, sign_in_flow: 'sign_in', acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, From 40e1d2d06737a77a8b51ac6038e491bd0f24a332 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 7 May 2024 09:33:54 -0400 Subject: [PATCH 03/21] change to duration tracking to allow for nil. fix redundant integer management. refactored duration calculation as a separate method in shared module --- app/controllers/concerns/authorization_count_concern.rb | 5 +++++ app/controllers/openid_connect/authorization_controller.rb | 5 +---- app/controllers/saml_idp_controller.rb | 4 +--- spec/controllers/users/sessions_controller_spec.rb | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/controllers/concerns/authorization_count_concern.rb b/app/controllers/concerns/authorization_count_concern.rb index 8b04f6a8124..e1bd9675816 100644 --- a/app/controllers/concerns/authorization_count_concern.rb +++ b/app/controllers/concerns/authorization_count_concern.rb @@ -28,3 +28,8 @@ def delete_auth_count(request_id) session[:sp_auth_count].delete request_id end end + +def sign_in_duration + return unless session[:sign_in_page_visited_at] + (Time.zone.now - Time.zone.parse(session[:sign_in_page_visited_at])).seconds.to_i +end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 9b6d83c1feb..3b70ab755c3 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -215,16 +215,13 @@ def track_events service_provider: @authorize_form.service_provider, user: current_user, ) - analytics.sp_redirect_initiated( ial: event_ial_context.ial, billed_ial: event_ial_context.bill_for_ial_1_or_2, sign_in_flow: session[:sign_in_flow], vtr: sp_session[:vtr], acr_values: sp_session[:acr_values], - sign_in_duration_seconds: (Time.zone.now - Time.zone.parse( - session[:sign_in_page_visited_at] || Time.zone.now.to_s, - )).seconds.to_i.round(2), + sign_in_duration_seconds: sign_in_duration, ) track_billing_events end diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 1af0564a7df..9f07fffc928 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -187,9 +187,7 @@ def track_events sign_in_flow: session[:sign_in_flow], vtr: sp_session[:vtr], acr_values: sp_session[:acr_values], - sign_in_duration_seconds: (Time.zone.now - Time.zone.parse( - session[:sign_in_page_visited_at] || Time.zone.now.to_s, - )).seconds.to_i.round(2), + sign_in_duration_seconds: nil, ) track_billing_events end diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 079aecc3674..49b782f7303 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -619,7 +619,6 @@ it 'renders the new template' do get :new expect(response).to render_template(:new) - expect(subject.session[:sign_in_page_visited_at]).to_not be(nil) end it 'tracks page visit, any alert flashes, and the Devise stored location' do @@ -632,6 +631,7 @@ ) get :new + expect(subject.session[:sign_in_page_visited_at]).to_not be(nil) end context 'renders partials' do From fad7f2938f0398785b46d7fe82440478446f46b6 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 7 May 2024 11:40:50 -0400 Subject: [PATCH 04/21] remove hard-coded nil value --- app/controllers/saml_idp_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 9f07fffc928..cfd4ab9a48d 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -187,7 +187,7 @@ def track_events sign_in_flow: session[:sign_in_flow], vtr: sp_session[:vtr], acr_values: sp_session[:acr_values], - sign_in_duration_seconds: nil, + sign_in_duration_seconds: sign_in_duration, ) track_billing_events end From d04598bfb4c24fe17285f732b6ee7a443dd08200 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 7 May 2024 14:42:02 -0400 Subject: [PATCH 05/21] rebase merge conflict. fix mis-inserted function in auth count concern, leverage helper method --- .../concerns/authorization_count_concern.rb | 8 ++++---- .../authorization_controller_spec.rb | 14 +++++++++++++- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/app/controllers/concerns/authorization_count_concern.rb b/app/controllers/concerns/authorization_count_concern.rb index e1bd9675816..bb886b8f547 100644 --- a/app/controllers/concerns/authorization_count_concern.rb +++ b/app/controllers/concerns/authorization_count_concern.rb @@ -27,9 +27,9 @@ def bump_auth_count def delete_auth_count(request_id) session[:sp_auth_count].delete request_id end -end -def sign_in_duration - return unless session[:sign_in_page_visited_at] - (Time.zone.now - Time.zone.parse(session[:sign_in_page_visited_at])).seconds.to_i + def sign_in_duration + return unless session[:sign_in_page_visited_at] + (Time.zone.now - Time.zone.parse(session[:sign_in_page_visited_at])).seconds.to_i + end end diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 07cf927f843..7b1ca74e73b 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -52,7 +52,7 @@ before do stub_sign_in user session[:sign_in_flow] = sign_in_flow - session[:sign_in_page_visited_at] = (Time.zone.now - 2.minutes).to_s + session[:sign_in_page_visited_at] = 1.minute.ago.to_s end context 'acr with valid params' do @@ -146,6 +146,7 @@ 'SP redirect initiated', ial: 1, billed_ial: 1, + sign_in_duration_seconds: 60, sign_in_flow:, acr_values: 'http://idmanagement.gov/ns/assurance/ial/1', vtr: nil, @@ -201,6 +202,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 1, + sign_in_duration_seconds: 60, billed_ial: 1, sign_in_flow:, acr_values: '', @@ -396,6 +398,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 2, + sign_in_duration_seconds: 60, billed_ial: 2, sign_in_flow:, acr_values: 'http://idmanagement.gov/ns/assurance/ial/2', @@ -770,6 +773,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 0, + sign_in_duration_seconds: 60, billed_ial: 2, sign_in_flow:, acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', @@ -861,6 +865,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 0, + sign_in_duration_seconds: 60, billed_ial: 1, sign_in_flow:, acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', @@ -955,6 +960,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 0, + sign_in_duration_seconds: 60, billed_ial: 1, sign_in_flow:, acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', @@ -1120,6 +1126,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 1, + sign_in_duration_seconds: 60, billed_ial: 1, sign_in_flow:, acr_values: 'http://idmanagement.gov/ns/assurance/ial/1', @@ -1176,6 +1183,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 1, + sign_in_duration_seconds: 60, billed_ial: 1, sign_in_flow:, acr_values: '', @@ -1372,6 +1380,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 2, + sign_in_duration_seconds: 60, billed_ial: 2, sign_in_flow:, acr_values: 'http://idmanagement.gov/ns/assurance/ial/2', @@ -1748,6 +1757,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 0, + sign_in_duration_seconds: 60, billed_ial: 2, sign_in_flow:, acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', @@ -1839,6 +1849,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 0, + sign_in_duration_seconds: 60, billed_ial: 1, sign_in_flow:, acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', @@ -1933,6 +1944,7 @@ expect(@analytics).to have_logged_event( 'SP redirect initiated', ial: 0, + sign_in_duration_seconds: 60, billed_ial: 1, sign_in_flow:, acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', From 75e24c0b8cfa41108fa02e4aabde7596d62c00f9 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 7 May 2024 16:05:37 -0400 Subject: [PATCH 06/21] wrap time-based expect blocks in freeze_time. at least one spec was failing because of time-weirdness --- .../authorization_controller_spec.rb | 240 ++++++++++-------- spec/features/users/sign_up_spec.rb | 20 +- 2 files changed, 143 insertions(+), 117 deletions(-) diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 7b1ca74e73b..029f83a7644 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -142,15 +142,17 @@ user_sp_authorized: true, code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 1, - billed_ial: 1, - sign_in_duration_seconds: 60, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/1', - vtr: nil, - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 1, + billed_ial: 1, + sign_in_duration_seconds: 60, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/1', + vtr: nil, + ) + end end end @@ -199,15 +201,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 1, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: '', - vtr: ['C1'], - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 1, + sign_in_duration_seconds: 60, + billed_ial: 1, + sign_in_flow:, + acr_values: '', + vtr: ['C1'], + ) + end end end @@ -395,15 +399,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 2, - sign_in_duration_seconds: 60, - billed_ial: 2, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/2', - vtr: nil, - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 2, + sign_in_duration_seconds: 60, + billed_ial: 2, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/2', + vtr: nil, + ) + end end context 'SP requests biometric_comparison_required' do @@ -770,15 +776,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 2, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 60, + billed_ial: 2, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) + end end end @@ -862,15 +870,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 60, + billed_ial: 1, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) + end end end @@ -957,15 +967,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 60, + billed_ial: 1, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) + end end end end @@ -1123,15 +1135,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 1, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/1', - vtr: nil, - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 1, + sign_in_duration_seconds: 60, + billed_ial: 1, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/1', + vtr: nil, + ) + end end end @@ -1180,15 +1194,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 1, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: '', - vtr: ['C1'], - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 1, + sign_in_duration_seconds: 60, + billed_ial: 1, + sign_in_flow:, + acr_values: '', + vtr: ['C1'], + ) + end end end @@ -1377,15 +1393,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 2, - sign_in_duration_seconds: 60, - billed_ial: 2, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/2', - vtr: nil, - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 2, + sign_in_duration_seconds: 60, + billed_ial: 2, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/2', + vtr: nil, + ) + end end context 'SP requests biometric_comparison_required' do @@ -1754,15 +1772,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 2, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 60, + billed_ial: 2, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) + end end end @@ -1846,15 +1866,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 60, + billed_ial: 1, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) + end end end @@ -1941,15 +1963,17 @@ code_digest: kind_of(String), ) - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) + freeze_time do + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 60, + billed_ial: 1, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) + end end end end diff --git a/spec/features/users/sign_up_spec.rb b/spec/features/users/sign_up_spec.rb index c53687d893f..6f87240d94b 100644 --- a/spec/features/users/sign_up_spec.rb +++ b/spec/features/users/sign_up_spec.rb @@ -480,15 +480,17 @@ def clipboard_text register_user click_agree_and_continue - expect(analytics).to have_logged_event( - 'SP redirect initiated', - ial: 1, - sign_in_duration_seconds: 1, - billed_ial: 1, - sign_in_flow: 'create_account', - acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, - vtr: nil, - ) + freeze_time do + expect(analytics).to have_logged_event( + 'SP redirect initiated', + ial: 1, + sign_in_duration_seconds: 1, + billed_ial: 1, + sign_in_flow: 'create_account', + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, + vtr: nil, + ) + end end describe 'visiting the homepage by clicking the logo image' do From 2eefd38ce54f90c8ed482de76e05964d0cc98ee5 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 8 May 2024 07:59:56 -0400 Subject: [PATCH 07/21] reframe freeze block --- spec/features/users/sign_up_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/features/users/sign_up_spec.rb b/spec/features/users/sign_up_spec.rb index 6f87240d94b..ed27fcad7c8 100644 --- a/spec/features/users/sign_up_spec.rb +++ b/spec/features/users/sign_up_spec.rb @@ -473,14 +473,14 @@ def clipboard_text end it 'logs expected analytics events for end-to-end sign-up' do - analytics = FakeAnalytics.new - allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(analytics) + freeze_time do + analytics = FakeAnalytics.new + allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(analytics) - visit_idp_from_sp_with_ial1(:oidc) - register_user - click_agree_and_continue + visit_idp_from_sp_with_ial1(:oidc) + register_user + click_agree_and_continue - freeze_time do expect(analytics).to have_logged_event( 'SP redirect initiated', ial: 1, From 9d73e1acb88dd5fa86e41bbf2487d65a0cb0b414 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 8 May 2024 09:20:30 -0400 Subject: [PATCH 08/21] add travel to expected time elapsed --- spec/features/users/sign_up_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/features/users/sign_up_spec.rb b/spec/features/users/sign_up_spec.rb index ed27fcad7c8..e6b18698a01 100644 --- a/spec/features/users/sign_up_spec.rb +++ b/spec/features/users/sign_up_spec.rb @@ -478,13 +478,14 @@ def clipboard_text allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(analytics) visit_idp_from_sp_with_ial1(:oidc) + travel_to Time.zone.now + 15.seconds register_user click_agree_and_continue expect(analytics).to have_logged_event( 'SP redirect initiated', ial: 1, - sign_in_duration_seconds: 1, + sign_in_duration_seconds: 15, billed_ial: 1, sign_in_flow: 'create_account', acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, From 05daf888411b00524b29a97eab1a63b9d01da482 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 8 May 2024 10:00:17 -0400 Subject: [PATCH 09/21] add freeze and travel to to test --- spec/support/shared_examples/sign_in.rb | 43 +++++++++++++------------ 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/spec/support/shared_examples/sign_in.rb b/spec/support/shared_examples/sign_in.rb index 69839b6da96..ed75dcef0cd 100644 --- a/spec/support/shared_examples/sign_in.rb +++ b/spec/support/shared_examples/sign_in.rb @@ -32,27 +32,30 @@ end it 'logs expected analytics events' do - user = create(:user, :fully_registered) - analytics = FakeAnalytics.new(user:) - allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(analytics) - - visit_idp_from_sp_with_ial1(sp) - fill_in_credentials_and_submit(user.email, user.password) - fill_in_code_with_last_phone_otp - click_submit_default - click_submit_default if current_path == complete_saml_path - click_agree_and_continue - click_submit_default if current_path == complete_saml_path + freeze_time do + user = create(:user, :fully_registered) + analytics = FakeAnalytics.new(user:) + allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(analytics) - expect(analytics).to have_logged_event( - 'SP redirect initiated', - ial: 1, - sign_in_duration_seconds: 1, - billed_ial: 1, - sign_in_flow: 'sign_in', - acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, - vtr: nil, - ) + visit_idp_from_sp_with_ial1(sp) + travel_to Time.zone.now + 15.seconds + fill_in_credentials_and_submit(user.email, user.password) + fill_in_code_with_last_phone_otp + click_submit_default + click_submit_default if current_path == complete_saml_path + click_agree_and_continue + click_submit_default if current_path == complete_saml_path + + expect(analytics).to have_logged_event( + 'SP redirect initiated', + ial: 1, + sign_in_duration_seconds: 15, + billed_ial: 1, + sign_in_flow: 'sign_in', + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, + vtr: nil, + ) + end end end From d04e5c610f59b2e6745a0c315e3208eef6d9a305 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 8 May 2024 10:15:54 -0400 Subject: [PATCH 10/21] correct use of freeze_time and add travel to to tests --- .../authorization_controller_spec.rb | 244 ++++++++---------- 1 file changed, 111 insertions(+), 133 deletions(-) diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 029f83a7644..2df11baca46 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -52,7 +52,9 @@ before do stub_sign_in user session[:sign_in_flow] = sign_in_flow - session[:sign_in_page_visited_at] = 1.minute.ago.to_s + session[:sign_in_page_visited_at] = Time.zone.now.to_s + freeze_time + travel_to Time.zone.now + 15.seconds end context 'acr with valid params' do @@ -142,17 +144,15 @@ user_sp_authorized: true, code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 1, - billed_ial: 1, - sign_in_duration_seconds: 60, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/1', - vtr: nil, - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 1, + billed_ial: 1, + sign_in_duration_seconds: 15, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/1', + vtr: nil, + ) end end @@ -201,17 +201,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 1, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: '', - vtr: ['C1'], - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 1, + sign_in_duration_seconds: 15, + billed_ial: 1, + sign_in_flow:, + acr_values: '', + vtr: ['C1'], + ) end end @@ -399,17 +397,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 2, - sign_in_duration_seconds: 60, - billed_ial: 2, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/2', - vtr: nil, - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 2, + sign_in_duration_seconds: 15, + billed_ial: 2, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/2', + vtr: nil, + ) end context 'SP requests biometric_comparison_required' do @@ -776,17 +772,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 2, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 15, + billed_ial: 2, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) end end @@ -870,17 +864,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 15, + billed_ial: 1, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) end end @@ -967,17 +959,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 15, + billed_ial: 1, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) end end end @@ -1135,17 +1125,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 1, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/1', - vtr: nil, - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 1, + sign_in_duration_seconds: 15, + billed_ial: 1, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/1', + vtr: nil, + ) end end @@ -1194,17 +1182,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 1, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: '', - vtr: ['C1'], - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 1, + sign_in_duration_seconds: 15, + billed_ial: 1, + sign_in_flow:, + acr_values: '', + vtr: ['C1'], + ) end end @@ -1393,17 +1379,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 2, - sign_in_duration_seconds: 60, - billed_ial: 2, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/2', - vtr: nil, - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 2, + sign_in_duration_seconds: 15, + billed_ial: 2, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/2', + vtr: nil, + ) end context 'SP requests biometric_comparison_required' do @@ -1772,17 +1756,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 2, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 15, + billed_ial: 2, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) end end @@ -1866,17 +1848,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 15, + billed_ial: 1, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) end end @@ -1963,17 +1943,15 @@ code_digest: kind_of(String), ) - freeze_time do - expect(@analytics).to have_logged_event( - 'SP redirect initiated', - ial: 0, - sign_in_duration_seconds: 60, - billed_ial: 1, - sign_in_flow:, - acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', - vtr: nil, - ) - end + expect(@analytics).to have_logged_event( + 'SP redirect initiated', + ial: 0, + sign_in_duration_seconds: 15, + billed_ial: 1, + sign_in_flow:, + acr_values: 'http://idmanagement.gov/ns/assurance/ial/0', + vtr: nil, + ) end end end From 6b77d11177ddfdae2659d47b2cfa37d6a821da0c Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 8 May 2024 10:26:51 -0400 Subject: [PATCH 11/21] put better control over time in idp controller test --- spec/controllers/saml_idp_controller_spec.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 1acfe422ea7..35d41155bf8 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -518,7 +518,9 @@ def name_id_version(format_urn) # All the tests here were written prior to the interstitial # authorization confirmation page so let's force the system # to skip past that page - session[:sign_in_page_visited_at] = (Time.zone.now - 2.minutes).to_s + session[:sign_in_page_visited_at] = Time.zone.now.to_s + freeze_time + travel_to Time.zone.now + 15.seconds allow(controller).to receive(:auth_count).and_return(2) end @@ -818,7 +820,7 @@ def name_id_version(format_urn) expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: Idp::Constants::IAL2, - sign_in_duration_seconds: 120, + sign_in_duration_seconds: 15, billed_ial: Idp::Constants::IAL2, sign_in_flow:, acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, @@ -967,7 +969,7 @@ def name_id_version(format_urn) expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: 0, - sign_in_duration_seconds: 120, + sign_in_duration_seconds: 15, billed_ial: 2, sign_in_flow:, acr_values: Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF, @@ -2437,7 +2439,7 @@ def stub_requested_attributes expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: 1, - sign_in_duration_seconds: 120, + sign_in_duration_seconds: 15, billed_ial: 1, sign_in_flow: :sign_in, acr_values: [ @@ -2488,7 +2490,7 @@ def stub_requested_attributes expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: 1, - sign_in_duration_seconds: 120, + sign_in_duration_seconds: 15, billed_ial: 1, sign_in_flow: :sign_in, acr_values: [ From 82fe6000cf9e2ce7bbcefaaeb4543c5bd0e18724 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 8 May 2024 14:09:56 -0400 Subject: [PATCH 12/21] tracking saml_idp as nil seems more appropriate with the tests there. removing session setting from session_helper to try changing that back --- spec/controllers/saml_idp_controller_spec.rb | 11 ++++------- spec/support/features/session_helper.rb | 1 - 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 35d41155bf8..ec8a1a73f46 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -518,9 +518,6 @@ def name_id_version(format_urn) # All the tests here were written prior to the interstitial # authorization confirmation page so let's force the system # to skip past that page - session[:sign_in_page_visited_at] = Time.zone.now.to_s - freeze_time - travel_to Time.zone.now + 15.seconds allow(controller).to receive(:auth_count).and_return(2) end @@ -820,7 +817,7 @@ def name_id_version(format_urn) expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: Idp::Constants::IAL2, - sign_in_duration_seconds: 15, + sign_in_duration_seconds: nil, billed_ial: Idp::Constants::IAL2, sign_in_flow:, acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF, @@ -969,7 +966,7 @@ def name_id_version(format_urn) expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: 0, - sign_in_duration_seconds: 15, + sign_in_duration_seconds: nil, billed_ial: 2, sign_in_flow:, acr_values: Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF, @@ -2439,7 +2436,7 @@ def stub_requested_attributes expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: 1, - sign_in_duration_seconds: 15, + sign_in_duration_seconds: nil, billed_ial: 1, sign_in_flow: :sign_in, acr_values: [ @@ -2490,7 +2487,7 @@ def stub_requested_attributes expect(@analytics).to receive(:track_event).with( 'SP redirect initiated', ial: 1, - sign_in_duration_seconds: 15, + sign_in_duration_seconds: nil, billed_ial: 1, sign_in_flow: :sign_in, acr_values: [ diff --git a/spec/support/features/session_helper.rb b/spec/support/features/session_helper.rb index 64e960bf0d0..4658d9ef97a 100644 --- a/spec/support/features/session_helper.rb +++ b/spec/support/features/session_helper.rb @@ -183,7 +183,6 @@ def sign_in_with_warden(user, auth_method: nil, issuer: nil) Warden.on_next_request do |proxy| session = proxy.env['rack.session'] session['warden.user.user.session'] = {}.with_indifferent_access - session[:sign_in_page_visited_at] = (Time.zone.now - 2.minutes).to_s if auth_method session['warden.user.user.session']['auth_events'] = [{ auth_method:, at: Time.zone.now }] end From 215a270d3b667e927e7cf5b68e7350d1af8afed0 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 8 May 2024 15:10:17 -0400 Subject: [PATCH 13/21] move sign_in_duration method to its own concern --- .../concerns/authorization_count_concern.rb | 5 ----- app/controllers/concerns/sign_in_duration_concern.rb | 10 ++++++++++ .../openid_connect/authorization_controller.rb | 1 + app/controllers/saml_idp_controller.rb | 1 + 4 files changed, 12 insertions(+), 5 deletions(-) create mode 100644 app/controllers/concerns/sign_in_duration_concern.rb diff --git a/app/controllers/concerns/authorization_count_concern.rb b/app/controllers/concerns/authorization_count_concern.rb index bb886b8f547..8b04f6a8124 100644 --- a/app/controllers/concerns/authorization_count_concern.rb +++ b/app/controllers/concerns/authorization_count_concern.rb @@ -27,9 +27,4 @@ def bump_auth_count def delete_auth_count(request_id) session[:sp_auth_count].delete request_id end - - def sign_in_duration - return unless session[:sign_in_page_visited_at] - (Time.zone.now - Time.zone.parse(session[:sign_in_page_visited_at])).seconds.to_i - end end diff --git a/app/controllers/concerns/sign_in_duration_concern.rb b/app/controllers/concerns/sign_in_duration_concern.rb new file mode 100644 index 00000000000..862bc26aeb0 --- /dev/null +++ b/app/controllers/concerns/sign_in_duration_concern.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +module SignInDurationConcern + extend ActiveSupport::Concern + + def sign_in_duration + return unless session[:sign_in_page_visited_at] + (Time.zone.now - Time.zone.parse(session[:sign_in_page_visited_at])).seconds.to_i + end +end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 3b70ab755c3..4683246197c 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -10,6 +10,7 @@ class AuthorizationController < ApplicationController include BillableEventTrackable include ForcedReauthenticationConcern include OpenidConnectRedirectConcern + include SignInDurationConcern before_action :build_authorize_form_from_params, only: [:index] before_action :block_biometric_requests_in_production, only: [:index] diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index cfd4ab9a48d..d1adb24ee16 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -17,6 +17,7 @@ class SamlIdpController < ApplicationController include AuthorizationCountConcern include BillableEventTrackable include SecureHeadersConcern + include SignInDurationConcern prepend_before_action :skip_session_load, only: [:metadata, :remotelogout] prepend_before_action :skip_session_expiration, only: [:metadata, :remotelogout] From 9b2cfa8ec01d09e84502c7c64a053a24af6767c1 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Thu, 9 May 2024 15:43:28 -0400 Subject: [PATCH 14/21] rename duration method, leverage around -> do for better freeze_time control --- .../concerns/sign_in_duration_concern.rb | 2 +- .../authorization_controller.rb | 2 +- app/controllers/saml_idp_controller.rb | 2 +- .../authorization_controller_spec.rb | 20 +++++++++++++++++-- 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/app/controllers/concerns/sign_in_duration_concern.rb b/app/controllers/concerns/sign_in_duration_concern.rb index 862bc26aeb0..1208d053408 100644 --- a/app/controllers/concerns/sign_in_duration_concern.rb +++ b/app/controllers/concerns/sign_in_duration_concern.rb @@ -3,7 +3,7 @@ module SignInDurationConcern extend ActiveSupport::Concern - def sign_in_duration + def sign_in_duration_seconds return unless session[:sign_in_page_visited_at] (Time.zone.now - Time.zone.parse(session[:sign_in_page_visited_at])).seconds.to_i end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 4683246197c..3862b699ef5 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -222,7 +222,7 @@ def track_events sign_in_flow: session[:sign_in_flow], vtr: sp_session[:vtr], acr_values: sp_session[:acr_values], - sign_in_duration_seconds: sign_in_duration, + sign_in_duration_seconds:, ) track_billing_events end diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index d1adb24ee16..db8368526eb 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -188,7 +188,7 @@ def track_events sign_in_flow: session[:sign_in_flow], vtr: sp_session[:vtr], acr_values: sp_session[:acr_values], - sign_in_duration_seconds: sign_in_duration, + sign_in_duration_seconds:, ) track_billing_events end diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 2df11baca46..f9b5f75848c 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -53,8 +53,12 @@ stub_sign_in user session[:sign_in_flow] = sign_in_flow session[:sign_in_page_visited_at] = Time.zone.now.to_s - freeze_time - travel_to Time.zone.now + 15.seconds + # freeze_time + # travel_to Time.zone.now + 15.seconds + end + + around do |ex| + freeze_time { ex.run } end context 'acr with valid params' do @@ -110,6 +114,7 @@ context 'with ial1 requested using acr_values' do it 'tracks IAL1 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -165,6 +170,7 @@ end it 'tracks IAL1 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -359,6 +365,7 @@ end it 'tracks IAL2 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 2) @@ -734,6 +741,7 @@ end it 'tracks IAL2 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 2) @@ -827,6 +835,7 @@ end it 'tracks IAL1 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -922,6 +931,7 @@ end it 'tracks IAL1 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -1090,6 +1100,7 @@ let(:vtr) { nil } it 'tracks IAL1 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) user.identities.last.update!(verified_attributes: %w[given_name family_name birthdate]) @@ -1146,6 +1157,7 @@ end it 'tracks IAL1 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -1341,6 +1353,7 @@ end it 'tracks IAL2 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 2) @@ -1718,6 +1731,7 @@ end it 'tracks IAL2 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 2) @@ -1811,6 +1825,7 @@ end it 'tracks IAL1 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -1906,6 +1921,7 @@ end it 'tracks IAL1 authentication event' do + travel_to Time.zone.now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) From 211eb57aa7370782b010c21c72f3f94abafb84c6 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Fri, 10 May 2024 08:25:30 -0400 Subject: [PATCH 15/21] add unit test for SignInDurationConcern --- .../concerns/sign_in_duration_concern_spec.rb | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 spec/controllers/concerns/sign_in_duration_concern_spec.rb diff --git a/spec/controllers/concerns/sign_in_duration_concern_spec.rb b/spec/controllers/concerns/sign_in_duration_concern_spec.rb new file mode 100644 index 00000000000..64058e6a809 --- /dev/null +++ b/spec/controllers/concerns/sign_in_duration_concern_spec.rb @@ -0,0 +1,32 @@ +require 'rails_helper' + +RSpec.describe SignInDurationConcern, type: :controller do + let(:test_class) do + Class.new do + include SignInDurationConcern + + attr_reader :session + + def initialize(session = {}) + @session = session + end + end + end + + let(:instance) { test_class.new } + + describe '#sign_in_duration_seconds' do + before do + instance.session[:sign_in_page_visited_at] = 6.seconds.ago.to_s + end + + it 'returns 6 seconds' do + expect(instance.sign_in_duration_seconds).to eq(6) + end + + it 'returns nil' do + instance.session[:sign_in_page_visited_at] = nil + expect(instance.sign_in_duration_seconds).to eq(nil) + end + end +end From 8d6da7b885be2dc28eca295f303f9f99ddb2c5ba Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Fri, 10 May 2024 10:05:33 -0400 Subject: [PATCH 16/21] improve concern spec with time freeze and clarify test context --- .../concerns/sign_in_duration_concern_spec.rb | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/spec/controllers/concerns/sign_in_duration_concern_spec.rb b/spec/controllers/concerns/sign_in_duration_concern_spec.rb index 64058e6a809..e8183c30b23 100644 --- a/spec/controllers/concerns/sign_in_duration_concern_spec.rb +++ b/spec/controllers/concerns/sign_in_duration_concern_spec.rb @@ -16,17 +16,27 @@ def initialize(session = {}) let(:instance) { test_class.new } describe '#sign_in_duration_seconds' do + let(:sign_in_page_visited_at) {} + around do |example| + freeze_time { example.run } + end + before do - instance.session[:sign_in_page_visited_at] = 6.seconds.ago.to_s + instance.session[:sign_in_page_visited_at] = sign_in_page_visited_at end - it 'returns 6 seconds' do - expect(instance.sign_in_duration_seconds).to eq(6) + context 'when session value is assigned' do + let(:sign_in_page_visited_at) { 6.seconds.ago.to_s } + it 'returns seconds since value' do + expect(instance.sign_in_duration_seconds).to eq(6) + end end - it 'returns nil' do - instance.session[:sign_in_page_visited_at] = nil - expect(instance.sign_in_duration_seconds).to eq(nil) + context 'when session value is not assigned' do + let(:sign_in_page_visited_at) { nil } + it 'returns nil' do + expect(instance.sign_in_duration_seconds).to be_nil + end end end end From bd83f179aa7e2bf7b4635396d782822a24192948 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Fri, 10 May 2024 10:34:41 -0400 Subject: [PATCH 17/21] make use of time as string consistent --- app/controllers/users/sessions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index f51c9a0c747..88dde2be88e 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -24,7 +24,7 @@ def new issuer: decorated_sp_session.sp_issuer, ) analytics.sign_in_page_visit(flash: flash[:alert]) - session[:sign_in_page_visited_at] = Time.zone.now + session[:sign_in_page_visited_at] = Time.zone.now.to_s super end From 38f2264ae99dbbbd481fcaac84c984051a4fd97c Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Mon, 13 May 2024 08:17:28 -0400 Subject: [PATCH 18/21] remove leftover code comments --- .../controllers/openid_connect/authorization_controller_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index f9b5f75848c..d0b477b0da3 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -53,8 +53,6 @@ stub_sign_in user session[:sign_in_flow] = sign_in_flow session[:sign_in_page_visited_at] = Time.zone.now.to_s - # freeze_time - # travel_to Time.zone.now + 15.seconds end around do |ex| From f2d38b2a1be8c5b763298e21fefb80aad2d2097e Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Mon, 13 May 2024 08:33:37 -0400 Subject: [PATCH 19/21] adjust time freeze --- .../authorization_controller_spec.rb | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index d0b477b0da3..bf24475ee3e 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -55,6 +55,8 @@ session[:sign_in_page_visited_at] = Time.zone.now.to_s end + let(:now) { Time.zone.now } + around do |ex| freeze_time { ex.run } end @@ -112,7 +114,7 @@ context 'with ial1 requested using acr_values' do it 'tracks IAL1 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -168,7 +170,7 @@ end it 'tracks IAL1 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -363,7 +365,7 @@ end it 'tracks IAL2 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 2) @@ -739,7 +741,7 @@ end it 'tracks IAL2 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 2) @@ -833,7 +835,7 @@ end it 'tracks IAL1 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -929,7 +931,7 @@ end it 'tracks IAL1 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -1098,7 +1100,7 @@ let(:vtr) { nil } it 'tracks IAL1 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) user.identities.last.update!(verified_attributes: %w[given_name family_name birthdate]) @@ -1155,7 +1157,7 @@ end it 'tracks IAL1 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -1351,7 +1353,7 @@ end it 'tracks IAL2 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 2) @@ -1729,7 +1731,7 @@ end it 'tracks IAL2 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 2) @@ -1823,7 +1825,7 @@ end it 'tracks IAL1 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) @@ -1919,7 +1921,7 @@ end it 'tracks IAL1 authentication event' do - travel_to Time.zone.now + 15.seconds + travel_to now + 15.seconds stub_analytics IdentityLinker.new(user, service_provider).link_identity(ial: 1) From 0cff09624744a66df24ff8e95db881cd6273edf8 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Mon, 13 May 2024 10:09:34 -0400 Subject: [PATCH 20/21] create more precision by rounding instead of converting to i --- app/controllers/concerns/sign_in_duration_concern.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/sign_in_duration_concern.rb b/app/controllers/concerns/sign_in_duration_concern.rb index 1208d053408..36c0dee96c2 100644 --- a/app/controllers/concerns/sign_in_duration_concern.rb +++ b/app/controllers/concerns/sign_in_duration_concern.rb @@ -5,6 +5,6 @@ module SignInDurationConcern def sign_in_duration_seconds return unless session[:sign_in_page_visited_at] - (Time.zone.now - Time.zone.parse(session[:sign_in_page_visited_at])).seconds.to_i + (Time.zone.now - Time.zone.parse(session[:sign_in_page_visited_at])).seconds.round end end From d7cbaa18df64601a98933535318150c9889e7085 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Mon, 13 May 2024 10:19:39 -0400 Subject: [PATCH 21/21] change sign in duration output to a floating number --- app/controllers/concerns/sign_in_duration_concern.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/sign_in_duration_concern.rb b/app/controllers/concerns/sign_in_duration_concern.rb index 36c0dee96c2..d90b7f69bb7 100644 --- a/app/controllers/concerns/sign_in_duration_concern.rb +++ b/app/controllers/concerns/sign_in_duration_concern.rb @@ -5,6 +5,6 @@ module SignInDurationConcern def sign_in_duration_seconds return unless session[:sign_in_page_visited_at] - (Time.zone.now - Time.zone.parse(session[:sign_in_page_visited_at])).seconds.round + (Time.zone.now - Time.zone.parse(session[:sign_in_page_visited_at])).seconds.to_f end end