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
8 changes: 7 additions & 1 deletion app/controllers/idv/gpo_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,13 @@ def profile_params
end

def form_response(result, success)
FormResponse.new(success: success, errors: result[:errors])
FormResponse.new(
success: success,
errors: result[:errors],
extra: {
pii_like_keypaths: [[:errors, :zipcode]],
},
)
end

def idv_throttle_params
Expand Down
10 changes: 9 additions & 1 deletion app/controllers/idv/phone_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,15 @@ def failure_url(reason)

def async_state_done(async_state)
form_result = step.async_state_done(async_state)
analytics.track_event(Analytics::IDV_PHONE_CONFIRMATION_VENDOR, form_result.to_h)
analytics.track_event(
Analytics::IDV_PHONE_CONFIRMATION_VENDOR,
form_result.to_h.merge(
pii_like_keypaths: [
Copy link
Copy Markdown
Contributor

@aduth aduth Sep 28, 2021

Choose a reason for hiding this comment

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

With how widespread it seems we have innocent logging by attribute name, I wonder if it's worth considering one of the other avenues considered for detecting PII, like common patterns for phone/SSN/etc, or "known" PII values like our mock profile details?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My sense is that approach trades convenience for completeness? Many tests used that PII but not all of them do? Do we think it would be close enough to check for those attribute values?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't really feel too strongly one way or the other. For me, the ticket seemed to be framed as something of a safety net for peace of mind, and not necessarily the primary safeguard against logging PII. I could also wonder if using the mock details could catch logging in a situation where it's not cleanly structured data with attribute names, or with slight variations to the exact attribute name.

The current implementation isn't really problematic to me, though I would wonder if it could be confusing that the pii_like_keypaths is not actually part of the "extra" payload being logged.

We could even do both if we wanted 🤷

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in 77a2aad

Example error:

FakeAnalytics::PiiDetected: track_event example PII first_name (FAKEY) detected in attributes
event: Trackable Event ()
full event: {:some_benign_key=>"FAKEY MCFAKERSON"}"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Another option, is we could update FormResponse and the classes similar to it to have a new attribute (next to extra), which get included in the .to_h? like this:

FormResponse.new(
  success: false,
  errors: { ssn: 'is not formatted right' },
  extra: { num_attempts: 14 },
  pii_like_key_paths: [[:errors, :ssn], [:error_details, :ssn]],
)

[:errors, :phone],
[:context, :stages, :address],
],
),
)
redirect_to_next_step and return if async_state.result[:success]
handle_proofing_failure
end
Expand Down
1 change: 1 addition & 0 deletions app/controllers/users/webauthn_setup_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ def track_delete(success)
Analytics::WEBAUTHN_DELETED,
success: success,
mfa_method_counts: counts_hash,
pii_like_keypaths: [[:mfa_method_counts, :phone]],
)
end

Expand Down
5 changes: 4 additions & 1 deletion app/forms/backup_code_setup_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ def submit
attr_accessor :user

def extra_analytics_attributes
{ mfa_method_counts: MfaContext.new(user).enabled_two_factor_configuration_counts_hash }
{
mfa_method_counts: MfaContext.new(user).enabled_two_factor_configuration_counts_hash,
pii_like_keypaths: [[:mfa_method_counts, :phone]],
}
end
end
6 changes: 5 additions & 1 deletion app/forms/idv/address_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ def initialize(user)
def submit(params)
consume_params(params)

FormResponse.new(success: valid?, errors: errors)
FormResponse.new(
success: valid?,
errors: errors,
extra: { pii_like_keypaths: [[:errors, :zipcode ], [:error_details, :zipcode]] },
)
end

private
Expand Down
1 change: 1 addition & 0 deletions app/forms/idv/api_image_upload_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ def extra_attributes
@extra_attributes ||= {
remaining_attempts: remaining_attempts,
user_id: user_uuid,
pii_like_keypaths: [[:pii]],
}
end

Expand Down
3 changes: 3 additions & 0 deletions app/forms/idv/doc_pii_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ def submit
Idv::DocAuthFormResponse.new(
success: valid?,
errors: errors,
extra: {
pii_like_keypaths: [[:pii]], # see errors.add(:pii)
},
)
end

Expand Down
1 change: 1 addition & 0 deletions app/forms/idv/phone_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def extra_analytics_attributes
{
country_code: parsed_phone.country,
area_code: parsed_phone.area_code,
pii_like_keypaths: [[:errors, :phone], [:error_details, :phone]], # see errors.add(:phone)
}
end

Expand Down
6 changes: 5 additions & 1 deletion app/forms/idv/ssn_format_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ def initialize(user)
def submit(params)
consume_params(params)

FormResponse.new(success: valid?, errors: errors)
FormResponse.new(
success: valid?,
errors: errors,
extra: { pii_like_keypaths: [[:errors, :ssn ], [:error_details, :ssn]] },
)
end

private
Expand Down
1 change: 1 addition & 0 deletions app/forms/new_phone_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ def extra_analytics_attributes
carrier: @phone_info&.carrier,
country_code: parsed_phone.country,
area_code: parsed_phone.area_code,
pii_like_keypaths: [[:errors, :phone], [:error_details, :phone]],
}.tap do |extra|
extra[:redacted_phone] = @redacted_phone if @redacted_phone
end
Expand Down
1 change: 1 addition & 0 deletions app/forms/otp_delivery_selection_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def extra_analytics_attributes
country_code: parsed_phone.country,
area_code: parsed_phone.area_code,
context: context,
pii_like_keypaths: [[:errors, :phone], [:error_details, :phone]],
}
end

Expand Down
1 change: 1 addition & 0 deletions app/forms/two_factor_authentication/phone_deletion_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ def extra_analytics_attributes
configuration_id: configuration&.id,
configuration_owner: configuration&.user&.uuid,
mfa_method_counts: MfaContext.new(user.reload).enabled_two_factor_configuration_counts_hash,
pii_like_keypaths: [[:mfa_method_counts, :phone]],
}
end

Expand Down
8 changes: 7 additions & 1 deletion app/forms/verify_account_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,13 @@ def submit
else
reset_sensitive_fields
end
FormResponse.new(success: result, errors: errors)
FormResponse.new(
success: result,
errors: errors,
extra: {
pii_like_keypaths: [[:errors, :otp], [:error_details, :otp]],
},
)
end

protected
Expand Down
1 change: 1 addition & 0 deletions app/forms/webauthn_setup_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ def extra_analytics_attributes
{
mfa_method_counts: MfaContext.new(user).enabled_two_factor_configuration_counts_hash,
multi_factor_auth_method: 'webauthn',
pii_like_keypaths: [[:mfa_method_counts, :phone]],
}
end
end
1 change: 1 addition & 0 deletions app/services/account_reset/delete_account.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ def extra_analytics_attributes
email: user.email_addresses.take&.email,
account_age_in_days: account_age,
mfa_method_counts: mfa_method_counts,
pii_like_keypaths: [[:mfa_method_counts, :phone]],
}
end
end
Expand Down
1 change: 1 addition & 0 deletions app/services/analytics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ def initialize(user:, request:, sp:, session:, ahoy: nil)
end

def track_event(event, attributes = {})
attributes.delete(:pii_like_keypaths)
update_session_events_and_paths_visited_for_analytics(event) if attributes[:success] != false
analytics_hash = {
event_properties: attributes.except(:user_id),
Expand Down
3 changes: 3 additions & 0 deletions app/services/idv/steps/document_capture_step.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ def post_images_and_handle_result
doc_auth_form_result = DocAuth::Response.new(
success: false,
errors: doc_pii_form_result.errors,
extra: {
pii_like_keypaths: [[:pii]],
},
)
doc_auth_form_result = doc_auth_form_result.merge(response)
return handle_document_verification_failure(doc_auth_form_result)
Expand Down
4 changes: 2 additions & 2 deletions app/services/idv/steps/verify_base_step.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,13 @@ def skip_legacy_steps
idv_session['resolution_successful'] = 'phone'
end

def idv_result_to_form_response(idv_result:, state: nil)
def idv_result_to_form_response(idv_result:, state: nil, extra: {})
state_id = idv_result.dig(:context, :stages, :state_id)
state_id[:state] = state if state && state_id
FormResponse.new(
success: idv_success(idv_result),
errors: idv_errors(idv_result),
extra: { proofing_results: idv_extra(idv_result) },
extra: extra.merge(proofing_results: idv_extra(idv_result)),
)
end

Expand Down
2 changes: 2 additions & 0 deletions app/services/idv/steps/verify_wait_step_show.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ def async_state_done(current_async_state)
form_response = idv_result_to_form_response(
idv_result: current_async_state.result,
state: flow_session[:pii_from_doc][:state],
extra: { pii_like_keypaths: [[:errors, :ssn]] },
)

if form_response.success?
response = check_ssn(flow_session[:pii_from_doc]) if form_response.success?
form_response = form_response.merge(response)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
success: true,
errors: {},
mfa_method_counts: { backup_codes: 10, webauthn: 2, phone: 2 },
pii_like_keypaths: [[:mfa_method_counts, :phone]],
account_age_in_days: 0,
}
expect(@analytics).
Expand All @@ -39,6 +40,7 @@
errors: { token: [t('errors.account_reset.granted_token_invalid')] },
error_details: { token: [t('errors.account_reset.granted_token_invalid')] },
mfa_method_counts: {},
pii_like_keypaths: [[:mfa_method_counts, :phone]],
account_age_in_days: 0,
}
expect(@analytics).
Expand All @@ -59,6 +61,7 @@
errors: { token: [t('errors.account_reset.granted_token_missing')] },
error_details: { token: [:blank] },
mfa_method_counts: {},
pii_like_keypaths: [[:mfa_method_counts, :phone]],
account_age_in_days: 0,
}
expect(@analytics).to receive(:track_event).
Expand All @@ -83,6 +86,7 @@
errors: { token: [t('errors.account_reset.granted_token_expired')] },
error_details: { token: [t('errors.account_reset.granted_token_expired')] },
mfa_method_counts: {},
pii_like_keypaths: [[:mfa_method_counts, :phone]],
account_age_in_days: 2,
}
expect(@analytics).to receive(:track_event).
Expand Down
19 changes: 17 additions & 2 deletions spec/controllers/idv/doc_auth_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,14 @@
mock_next_step(:back_image)
allow_any_instance_of(Flow::BaseFlow).to \
receive(:flow_session).and_return(pii_from_doc: {})
result = { success: true, errors: {}, step: 'ssn', flow_path: 'standard', step_count: 1 }
result = {
success: true,
errors: {},
step: 'ssn',
flow_path: 'standard',
step_count: 1,
pii_like_keypaths: [[:errors, :ssn], [:error_details, :ssn]],
}

put :update, params: {step: 'ssn', doc_auth: { step: 'ssn', ssn: '111-11-1111' } }

Expand All @@ -140,7 +147,13 @@
mock_next_step(:back_image)
allow_any_instance_of(Flow::BaseFlow).to \
receive(:flow_session).and_return(pii_from_doc: {})
result = { success: true, errors: {}, step: 'ssn', step_count: 1 }
result = {
success: true,
errors: {},
step: 'ssn',
step_count: 1,
pii_like_keypaths: [[:errors, :ssn], [:error_details, :ssn]],
}

put :update, params: {step: 'ssn', doc_auth: { step: 'ssn', ssn: '666-66-6666' } }
put :update, params: {step: 'ssn', doc_auth: { step: 'ssn', ssn: '111-11-1111' } }
Expand Down Expand Up @@ -391,6 +404,7 @@
step: 'verify_document_status',
flow_path: 'standard',
step_count: 1,
pii_like_keypaths: [[:pii]],
}
)
expect(@analytics).to have_received(:track_event).with(
Expand All @@ -402,6 +416,7 @@
step: 'verify_document_status',
flow_path: 'standard',
step_count: 1,
pii_like_keypaths: [[:pii]],
}
)
end
Expand Down
Loading