Skip to content

Remove track_mfa_submit analytics pass-through method#10679

Merged
aduth merged 1 commit intomainfrom
aduth-rm-track-mfa-submit
May 23, 2024
Merged

Remove track_mfa_submit analytics pass-through method#10679
aduth merged 1 commit intomainfrom
aduth-rm-track-mfa-submit

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented May 22, 2024

🛠 Summary of changes

Removes Analytics#track_mfa_submit, updating call-sites to call AnalyticsEvents#multi_factor_auth directly.

Why?

  • It's only purpose was to provide pii_like_keypaths for specific personal key verification properties, which is reasonable to expect to be passed in the one place it's necessary (PersonalKeyVerificationController)
  • The method is the only one of its kind, and it causes confusion to what's actually being logged, since the method name does not correspond directly to an AnalyticsEvents module method or actual event name
  • It is incompatible with have_logged_event, which is the preferred way to ensure that events are logged as expected, contains no PII, and all properties are documented
    • See included revision to document error_details now that it is correctly flagged as undocumented

📜 Testing Plan

Verify that the build passes.

Verify that when signing in with 2FA, the "Multi-Factor Authentication" event is still logged as expected:

  1. Run make watch_events in a separate terminal process from the IdP server
  2. Go to http://localhost:3000 in a private browsing session
  3. Sign in
  4. See "Multi-Factor Authentication" event in logged events, with expected properties

changelog: Internal, Analytics, Remove track_mfa_submit analytics pass-through method
@aduth aduth requested a review from a team May 22, 2024 14:24

describe '#create' do
let(:parsed_phone) { Phonelib.parse(controller.current_user.default_phone_configuration.phone) }
let(:user) { create(:user, :with_phone) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this change is that have_logged_event coming after the controller action works differently than how analytics was previously stubbed with expect before the action, and certain properties rely on the state of the user before the action. Some values like controller.current_user are unavailable after the action is run.

phone_fingerprint: phone_fingerprint,
frontend_error:,
**extra,
}.compact,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this change is that have_logged_event is much stricter about comparing logged properties, and was failing on method-specific properties like auth_app_configuration_id being nil when logged in another method's controller.

I still think we should probably do an automatic .compact on all properties logged, but that's a future problem to address.

@aduth aduth merged commit e3ce517 into main May 23, 2024
@aduth aduth deleted the aduth-rm-track-mfa-submit branch May 23, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants