Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove track_mfa_submit analytics pass-through method #10679

Merged
merged 1 commit into from
May 23, 2024

Conversation

aduth
Copy link
Member

@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
@@ -122,41 +122,43 @@
end

describe '#create' do
let(:parsed_phone) { Phonelib.parse(controller.current_user.default_phone_configuration.phone) }
let(:user) { create(:user, :with_phone) }
Copy link
Member 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
Member 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
2 checks passed
@aduth aduth deleted the aduth-rm-track-mfa-submit branch May 23, 2024 19:37
mitchellhenke pushed a commit that referenced this pull request May 28, 2024
changelog: Internal, Analytics, Remove track_mfa_submit analytics pass-through method
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