Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
91b6c23
changelog: Internal, Analytics, Adds property to SP redirect initiate…
kevinsmaster5 May 6, 2024
78d633a
update controllers to account for legit nil values at login page visi…
kevinsmaster5 May 6, 2024
40e1d2d
change to duration tracking to allow for nil. fix redundant integer m…
kevinsmaster5 May 7, 2024
fad7f29
remove hard-coded nil value
kevinsmaster5 May 7, 2024
d04598b
rebase merge conflict. fix mis-inserted function in auth count concer…
kevinsmaster5 May 7, 2024
75e24c0
wrap time-based expect blocks in freeze_time. at least one spec was f…
kevinsmaster5 May 7, 2024
2eefd38
reframe freeze block
kevinsmaster5 May 8, 2024
9d73e1a
add travel to expected time elapsed
kevinsmaster5 May 8, 2024
05daf88
add freeze and travel to to test
kevinsmaster5 May 8, 2024
d04e5c6
correct use of freeze_time and add travel to to tests
kevinsmaster5 May 8, 2024
6b77d11
put better control over time in idp controller test
kevinsmaster5 May 8, 2024
82fe600
tracking saml_idp as nil seems more appropriate with the tests there.…
kevinsmaster5 May 8, 2024
215a270
move sign_in_duration method to its own concern
kevinsmaster5 May 8, 2024
9b2cfa8
rename duration method, leverage around -> do for better freeze_time …
kevinsmaster5 May 9, 2024
211eb57
add unit test for SignInDurationConcern
kevinsmaster5 May 10, 2024
8d6da7b
improve concern spec with time freeze and clarify test context
kevinsmaster5 May 10, 2024
bd83f17
make use of time as string consistent
kevinsmaster5 May 10, 2024
38f2264
remove leftover code comments
kevinsmaster5 May 13, 2024
f2d38b2
adjust time freeze
kevinsmaster5 May 13, 2024
0cff096
create more precision by rounding instead of converting to i
kevinsmaster5 May 13, 2024
d7cbaa1
change sign in duration output to a floating number
kevinsmaster5 May 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions app/controllers/concerns/sign_in_duration_concern.rb
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a spec/controllers/concerns/sign_in_duration_concern_spec.rb with test coverage?

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true

module SignInDurationConcern
extend ActiveSupport::Concern

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_f
end
end
3 changes: 2 additions & 1 deletion app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -215,13 +216,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:,
)
track_billing_events
end
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -187,6 +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:,
)
track_billing_events
end
Expand Down
1 change: 1 addition & 0 deletions app/controllers/users/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.to_s
super
end

Expand Down
12 changes: 11 additions & 1 deletion app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5070,14 +5070,24 @@ 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:,
billed_ial:,
sign_in_flow:,
vtr: vtr,
acr_values: acr_values,
sign_in_duration_seconds:,
**extra,
)
end
Expand Down
42 changes: 42 additions & 0 deletions spec/controllers/concerns/sign_in_duration_concern_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
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
let(:sign_in_page_visited_at) {}
around do |example|
freeze_time { example.run }
end

before do
instance.session[:sign_in_page_visited_at] = sign_in_page_visited_at
end

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

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
31 changes: 31 additions & 0 deletions spec/controllers/openid_connect/authorization_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@
before do
stub_sign_in user
session[:sign_in_flow] = sign_in_flow
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

context 'acr with valid params' do
Expand Down Expand Up @@ -107,6 +114,7 @@

context 'with ial1 requested using acr_values' do
it 'tracks IAL1 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 1)
Expand Down Expand Up @@ -145,6 +153,7 @@
'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,
Expand All @@ -161,6 +170,7 @@
end

it 'tracks IAL1 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 1)
Expand Down Expand Up @@ -200,6 +210,7 @@
expect(@analytics).to have_logged_event(
'SP redirect initiated',
ial: 1,
sign_in_duration_seconds: 15,
billed_ial: 1,
sign_in_flow:,
acr_values: '',
Expand Down Expand Up @@ -354,6 +365,7 @@
end

it 'tracks IAL2 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 2)
Expand Down Expand Up @@ -395,6 +407,7 @@
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',
Expand Down Expand Up @@ -728,6 +741,7 @@
end

it 'tracks IAL2 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 2)
Expand Down Expand Up @@ -769,6 +783,7 @@
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',
Expand Down Expand Up @@ -820,6 +835,7 @@
end

it 'tracks IAL1 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 1)
Expand Down Expand Up @@ -860,6 +876,7 @@
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',
Expand Down Expand Up @@ -914,6 +931,7 @@
end

it 'tracks IAL1 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 1)
Expand Down Expand Up @@ -954,6 +972,7 @@
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',
Expand Down Expand Up @@ -1081,6 +1100,7 @@
let(:vtr) { nil }

it 'tracks IAL1 authentication event' do
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])
Expand Down Expand Up @@ -1119,6 +1139,7 @@
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',
Expand All @@ -1136,6 +1157,7 @@
end

it 'tracks IAL1 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 1)
Expand Down Expand Up @@ -1175,6 +1197,7 @@
expect(@analytics).to have_logged_event(
'SP redirect initiated',
ial: 1,
sign_in_duration_seconds: 15,
billed_ial: 1,
sign_in_flow:,
acr_values: '',
Expand Down Expand Up @@ -1330,6 +1353,7 @@
end

it 'tracks IAL2 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 2)
Expand Down Expand Up @@ -1371,6 +1395,7 @@
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',
Expand Down Expand Up @@ -1706,6 +1731,7 @@
end

it 'tracks IAL2 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 2)
Expand Down Expand Up @@ -1747,6 +1773,7 @@
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',
Expand Down Expand Up @@ -1798,6 +1825,7 @@
end

it 'tracks IAL1 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 1)
Expand Down Expand Up @@ -1838,6 +1866,7 @@
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',
Expand Down Expand Up @@ -1892,6 +1921,7 @@
end

it 'tracks IAL1 authentication event' do
travel_to now + 15.seconds
stub_analytics

IdentityLinker.new(user, service_provider).link_identity(ial: 1)
Expand Down Expand Up @@ -1932,6 +1962,7 @@
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',
Expand Down
4 changes: 4 additions & 0 deletions spec/controllers/saml_idp_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +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: nil,
billed_ial: Idp::Constants::IAL2,
sign_in_flow:,
acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF,
Expand Down Expand Up @@ -965,6 +966,7 @@ def name_id_version(format_urn)
expect(@analytics).to receive(:track_event).with(
'SP redirect initiated',
ial: 0,
sign_in_duration_seconds: nil,
billed_ial: 2,
sign_in_flow:,
acr_values: Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF,
Expand Down Expand Up @@ -2434,6 +2436,7 @@ def stub_requested_attributes
expect(@analytics).to receive(:track_event).with(
'SP redirect initiated',
ial: 1,
sign_in_duration_seconds: nil,
billed_ial: 1,
sign_in_flow: :sign_in,
acr_values: [
Expand Down Expand Up @@ -2484,6 +2487,7 @@ def stub_requested_attributes
expect(@analytics).to receive(:track_event).with(
'SP redirect initiated',
ial: 1,
sign_in_duration_seconds: nil,
billed_ial: 1,
sign_in_flow: :sign_in,
acr_values: [
Expand Down
1 change: 1 addition & 0 deletions spec/controllers/users/sessions_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,7 @@
)

get :new
expect(subject.session[:sign_in_page_visited_at]).to_not be(nil)
end

context 'renders partials' do
Expand Down
Loading