Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
141 changes: 88 additions & 53 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3639,20 +3639,22 @@ def logout_initiated(
# @param [Boolean] success Whether authentication was successful
# @param [Hash] errors Authentication error reasons, if unsuccessful
# @param [Hash] error_details Details for error that occurred in unsuccessful submission
# @param [String] context
# @param [Boolean] new_device
# @param [String] multi_factor_auth_method
# @param ["authentication","reauthentication","confirmation"] context User session context
# @param [Boolean] new_device Whether the user is authenticating from a new device
# @param [String] multi_factor_auth_method Authentication method used
# @param [DateTime] multi_factor_auth_method_created_at time auth method was created
# @param [Integer] auth_app_configuration_id
# @param [Integer] piv_cac_configuration_id
# @param [Integer] key_id
# @param [Integer] webauthn_configuration_id
# @param [Integer] phone_configuration_id
# @param [Boolean] confirmation_for_add_phone
# @param [String] area_code
# @param [String] country_code
# @param [Integer] auth_app_configuration_id Database ID of authentication app configuration
# @param [Integer] piv_cac_configuration_id Database ID of PIV/CAC configuration
# @param [Integer] key_id PIV/CAC key_id
# @param [Integer] webauthn_configuration_id Database ID of WebAuthn configuration
# @param [Integer] phone_configuration_id Database ID of phone configuration
# @param [Boolean] confirmation_for_add_phone Whether authenticating while adding phone
# @param [String] area_code Area code of phone number
# @param [String] country_code Country code associated with phone number
# @param [String] phone_fingerprint the hmac fingerprint of the phone number formatted as e164
# @param [String] frontend_error Name of error that occurred in frontend during submission
# @param [Boolean] in_account_creation_flow Whether user is going through account creation flow
# @param [Integer] enabled_mfa_methods_count Number of MFAs associated with user
# Multi-Factor Authentication
def multi_factor_auth(
success:,
Expand All @@ -3673,6 +3675,8 @@ def multi_factor_auth(
country_code: nil,
phone_fingerprint: nil,
frontend_error: nil,
in_account_creation_flow: nil,
enabled_mfa_methods_count: nil,
**extra
)
track_event(
Expand All @@ -3696,6 +3700,8 @@ def multi_factor_auth(
country_code: country_code,
phone_fingerprint: phone_fingerprint,
frontend_error:,
in_account_creation_flow:,
enabled_mfa_methods_count:,
**extra,
}.compact,
)
Expand Down Expand Up @@ -3764,24 +3770,39 @@ def multi_factor_auth_enter_backup_code_visit(context:, **extra)
)
end

# @param [String] context
# @param ["authentication","reauthentication","confirmation"] context User session context
# @param [String] multi_factor_auth_method
# @param [Boolean] confirmation_for_add_phone
# @param [Integer] phone_configuration_id
# @param [String] area_code Area code of phone number
# @param [String] country_code Abbreviated 2-letter country code associated with phone number
# @param [String] phone_fingerprint Fingerprint hash of phone number
# @param [Boolean] in_account_creation_flow Whether user is going through account creation flow
# @param [Integer] enabled_mfa_methods_count Number of MFAs associated with user
# Multi-Factor Authentication enter OTP visited
def multi_factor_auth_enter_otp_visit(
context:,
multi_factor_auth_method:,
confirmation_for_add_phone:,
phone_configuration_id:,
area_code:,
country_code:,
phone_fingerprint:,
in_account_creation_flow:,
enabled_mfa_methods_count:,
**extra
)
track_event(
'Multi-Factor Authentication: enter OTP visited',
context: context,
multi_factor_auth_method: multi_factor_auth_method,
confirmation_for_add_phone: confirmation_for_add_phone,
phone_configuration_id: phone_configuration_id,
context:,
multi_factor_auth_method:,
confirmation_for_add_phone:,
phone_configuration_id:,
area_code:,
country_code:,
phone_fingerprint:,
in_account_creation_flow:,
enabled_mfa_methods_count:,
**extra,
)
end
Expand Down Expand Up @@ -4059,17 +4080,23 @@ def oidc_logout_visited(
end

# Tracks when a sucessful openid authorization request is returned
# @param [Boolean] success Whether form validations were succcessful
# @param [Boolean] user_sp_authorized Whether user granted consent during this authorization
# @param [String] client_id
# @param [String] code_digest hash of returned "code" param
def openid_connect_authorization_handoff(
success:,
user_sp_authorized:,
client_id:,
code_digest:,
**extra
)
track_event(
'OpenID Connect: authorization request handoff',
client_id: client_id,
code_digest: code_digest,
success:,
user_sp_authorized:,
client_id:,
code_digest:,
**extra,
)
end
Expand Down Expand Up @@ -4161,29 +4188,32 @@ def openid_connect_token(client_id:, user_id:, code_digest:, expires_in:, ial:,
end

# Tracks when user makes an otp delivery selection
# @param [Boolean] success Whether the form was submitted successfully.
# @param [Hash] errors Errors resulting from form validation
# @param ["authentication","reauthentication","confirmation"] context User session context
# @param [String] otp_delivery_preference (sms or voice)
# @param [Boolean] resend
# @param [String] country_code
# @param [String] area_code
# @param ["authentication","reauthentication","confirmation"] context user session context
# @param [Hash] pii_like_keypaths
# @param [Boolean] resend True if the user re-requested a code
# @param [String] country_code Country code associated with phone number
# @param [String] area_code Area code of phone number
def otp_delivery_selection(
success:,
errors:,
context:,
otp_delivery_preference:,
resend:,
country_code:,
area_code:,
context:,
pii_like_keypaths:,
**extra
)
track_event(
'OTP: Delivery Selection',
otp_delivery_preference: otp_delivery_preference,
resend: resend,
country_code: country_code,
area_code: area_code,
context: context,
pii_like_keypaths: pii_like_keypaths,
success:,
errors:,
context:,
otp_delivery_preference:,
resend:,
country_code:,
area_code:,
**extra,
)
end
Expand Down Expand Up @@ -5182,14 +5212,15 @@ def user_registration_2fa_setup_visit(
end

# User registration has been handed off to agency page
# @param [Boolean] ial2
# @param [Integer] ialmax
# @param [String] service_provider_name
# @param [String] page_occurence
# @param [String] needs_completion_screen_reason
# @param [Boolean] ial2 Whether the user registration was for a verified identity
# @param [Integer] ialmax Whether the user registration was for an IALMax request
# @param [String] service_provider_name The friendly name of the service provider
# @param ['account-page','agency-page'] page_occurence Where the user concluded registration
# @param ['new_sp','new_attributes','reverified_after_consent'] needs_completion_screen_reason The
Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't matter but I notice we are using double quotes for other string values, single quotes here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I think I saw another method where we used single quotes which is why I used them here, so we're clearly a big mess of inconsistency 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're referring to lines like:

# @param ["authentication","reauthentication","confirmation"] context User session context

Those were copied from other methods, hence the internal inconsistency.

I may follow-up to do a bulk normalization, though I feel like it's inevitable we'll regress to having some inconsistency, unless we care to enforce it somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a look at rubocop-yard but unfortunately, it doesn't appear to handle string literals at all. Either way, a task for another time

# reason for the consent screen being shown
# @param [Boolean] in_account_creation_flow Whether user is going through account creation
# @param [Array] sp_request_requested_attributes
# @param [Array] sp_session_requested_attributes
# @param [Array] sp_request_requested_attributes Attributes requested by the service provider
# @param [Array] sp_session_requested_attributes Attributes requested by the service provider
Comment on lines +5222 to +5223
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 describe what the difference is between these two? I'm guessing one is per-request and the other is something else?

Copy link
Contributor Author

@aduth aduth May 31, 2024

Choose a reason for hiding this comment

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

I looked into this a bit, and... I think they're effectively the same? I'll double-check though. And if they are the same, maybe I'll follow-up or create a ticket to remove one or the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing anything in the logs for the _request_ one in the last 2 weeks... very mysterious

| filter ispresent(properties.event_properties.sp_request_requested_attributes) or ispresent(properties.event_properties.sp_request_requested_attributes.0)

Copy link
Contributor Author

@aduth aduth May 31, 2024

Choose a reason for hiding this comment

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

I think the issue is that value reads from service_provider_request, but service_provider_request relies on the request_id URL query parameter, which we wouldn't expect to be present in the completions controller.

I can't imagine that service_provider_request has much utility to be honest, and we should probably be calling sp_session in most places instead. I can see about following-up on that.

def user_registration_agency_handoff_page_visit(
ial2:,
service_provider_name:,
Expand Down Expand Up @@ -5226,18 +5257,21 @@ def user_registration_cancellation(request_came_from:, **extra)
end

# Tracks when user completes registration
# @param [Boolean] ial2
# @param [Boolean] ialmax
# @param [String] service_provider_name
# @param [String] page_occurence
# @param [String] needs_completion_screen_reason
# @param [Array] sp_request_requested_attributes
# @param [Array] sp_session_requested_attributes
# @param [Boolean] ial2 Whether the user registration was for a verified identity
# @param [Boolean] ialmax Whether the user registration was for an IALMax request
# @param [String] service_provider_name The friendly name of the service provider
# @param ['account-page','agency-page'] page_occurence Where the user concluded registration
# @param ['new_sp','new_attributes','reverified_after_consent'] needs_completion_screen_reason The
# reason for the consent screen being shown
# @param [Array] sp_request_requested_attributes Attributes requested by the service provider
# @param [Array] sp_session_requested_attributes Attributes requested by the service provider
Comment on lines +5266 to +5267
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto about differentiation

# @param [Boolean] in_account_creation_flow Whether user is going through account creation flow
# @param [String, nil] disposable_email_domain Disposable email domain used for registration
def user_registration_complete(
ial2:,
service_provider_name:,
page_occurence:,
in_account_creation_flow:,
needs_completion_screen_reason:,
sp_session_requested_attributes:,
sp_request_requested_attributes: nil,
Expand All @@ -5247,14 +5281,15 @@ def user_registration_complete(
)
track_event(
'User registration: complete',
ial2: ial2,
ialmax: ialmax,
service_provider_name: service_provider_name,
page_occurence: page_occurence,
needs_completion_screen_reason: needs_completion_screen_reason,
sp_request_requested_attributes: sp_request_requested_attributes,
sp_session_requested_attributes: sp_session_requested_attributes,
disposable_email_domain: disposable_email_domain,
ial2:,
ialmax:,
service_provider_name:,
page_occurence:,
in_account_creation_flow:,
needs_completion_screen_reason:,
sp_request_requested_attributes:,
sp_session_requested_attributes:,
disposable_email_domain:,
**extra,
)
end
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/sign_up/completions_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.describe SignUp::CompletionsController, allowed_extra_analytics: [:*] do
RSpec.describe SignUp::CompletionsController do
let(:temporary_email) { 'name@temporary.com' }

describe '#show' do
Expand Down
2 changes: 1 addition & 1 deletion spec/features/ialmax/saml_sign_in_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.feature 'SAML IALMAX sign in', allowed_extra_analytics: [:*] do
RSpec.feature 'SAML IALMAX sign in' do
include SamlAuthHelper

context 'with an ial2 SP' do
Expand Down
3 changes: 1 addition & 2 deletions spec/features/remember_device/cookie_expiration_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
require 'rails_helper'

RSpec.describe 'signing in with remember device and closing browser',
allowed_extra_analytics: [:*] do
RSpec.describe 'signing in with remember device and closing browser' do
include SamlAuthHelper

let(:user) { user_with_2fa }
Expand Down
2 changes: 1 addition & 1 deletion spec/features/remember_device/revocation_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.feature 'taking an action that revokes remember device', allowed_extra_analytics: [:*] do
RSpec.feature 'taking an action that revokes remember device' do
include NavigationHelper

before do
Expand Down
3 changes: 1 addition & 2 deletions spec/features/remember_device/session_expiration_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
require 'rails_helper'

RSpec.describe 'signing in with remember device and idling on the sign in page',
allowed_extra_analytics: [:*] do
RSpec.describe 'signing in with remember device and idling on the sign in page' do
include SamlAuthHelper
include OidcAuthHelper

Expand Down
2 changes: 1 addition & 1 deletion spec/features/reports/sp_active_users_report_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.feature 'sp active users report', allowed_extra_analytics: [:*] do
RSpec.feature 'sp active users report' do
include SamlAuthHelper
include OidcAuthHelper
include IdvHelper
Expand Down
2 changes: 1 addition & 1 deletion spec/features/saml/multiple_endpoints_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.describe 'multiple saml endpoints', allowed_extra_analytics: [:*] do
RSpec.describe 'multiple saml endpoints' do
include SamlAuthHelper
include IdvHelper

Expand Down
2 changes: 1 addition & 1 deletion spec/features/saml/saml_logout_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.feature 'SAML logout', allowed_extra_analytics: [:*] do
RSpec.feature 'SAML logout' do
include SamlAuthHelper

let(:user) { create(:user, :fully_registered) }
Expand Down
2 changes: 1 addition & 1 deletion spec/features/saml/saml_relay_state_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.feature 'SAML RelayState', allowed_extra_analytics: [:*] do
RSpec.feature 'SAML RelayState' do
include SamlAuthHelper

context 'when RelayState is passed in authn request' do
Expand Down
2 changes: 1 addition & 1 deletion spec/features/saml/saml_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.feature 'saml api', allowed_extra_analytics: [:*] do
RSpec.feature 'saml api' do
include SamlAuthHelper
include IdvHelper

Expand Down
2 changes: 1 addition & 1 deletion spec/features/sign_in/banned_users_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.feature 'Banning users for an SP', allowed_extra_analytics: [:*] do
RSpec.feature 'Banning users for an SP' do
include SamlAuthHelper
include OidcAuthHelper

Expand Down
2 changes: 1 addition & 1 deletion spec/features/sign_in/remember_device_default_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.describe 'Remember device checkbox', allowed_extra_analytics: [:*] do
RSpec.describe 'Remember device checkbox' do
include SamlAuthHelper

context 'when the user signs in and arrives at the 2FA page' do
Expand Down
2 changes: 1 addition & 1 deletion spec/features/sign_in/sp_return_log_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.feature 'SP return logs', allowed_extra_analytics: [:*] do
RSpec.feature 'SP return logs' do
include SamlAuthHelper

it 'updates user id after user authenticates so we can track any user back to issuer', :email do
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.feature 'sign up with backup code', allowed_extra_analytics: [:*] do
RSpec.feature 'sign up with backup code' do
include DocAuthHelper
include SamlAuthHelper

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.feature 'Second MFA Reminder', allowed_extra_analytics: [:*] do
RSpec.feature 'Second MFA Reminder' do
include OidcAuthHelper

let(:service_provider) { ServiceProvider.find_by(issuer: OidcAuthHelper::OIDC_IAL1_ISSUER) }
Expand Down
3 changes: 1 addition & 2 deletions spec/features/visitors/bad_password_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
require 'rails_helper'

RSpec.feature 'Visitor signs in with bad passwords and gets locked out',
allowed_extra_analytics: [:*] do
RSpec.feature 'Visitor signs in with bad passwords and gets locked out' do
let(:user) { create(:user, :fully_registered) }
let(:bad_password) { 'badpassword' }

Expand Down
3 changes: 1 addition & 2 deletions spec/requests/openid_connect_authorize_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
require 'rails_helper'

RSpec.describe 'user signs in partially and visits openid_connect/authorize',
allowed_extra_analytics: [:*] do
RSpec.describe 'user signs in partially and visits openid_connect/authorize' do
let(:user) { create(:user, :fully_registered, with: { phone: '+1 (202) 555-1213' }) }

it 'prompts the user to 2FA' do
Expand Down