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

Add telemetry to capture MFA login durations #3376

Merged
merged 3 commits into from Feb 2, 2023

Conversation

bettymakes
Copy link
Contributor

@bettymakes bettymakes commented Jan 26, 2023

πŸ€” What problem are you solving?

Closes https://github.com/Shopify/ruby-dependency-security/issues/146

We hypothesize that WebAuthn improves the MFA user experience by enabling users to authenticate more quickly compared to TOTP. To verify this assumption, we'll capture login durations for Webauthn and TOTP to monitor and compare the difference.

πŸ› οΈ What approach did you choose and why?

1️⃣ Starting the timer for capturing duration
In order to capture the duration of the login flow for both WebAuthn or TOTP, we need a starting point. We store the starting time on the browser's session once the user has successfully logged in with their username and password.

This makes sense as the starting point because the user will then be shown the 2nd factor authentication view (either prompted for TOTP code or WebAuthn).

2️⃣ Calculate and capture duration
Upon successfully authenticating with TOTP or WebAuthn, we'll calculate the duration of time passed (in seconds) and record it.

πŸ‘€ What should reviewers focus on?

I'm open to renaming the helper methods created in the tests (login_to_session and verify_challenge).

@jenshenny & I had also thought about other metrics we may want to track (e.g. tracking failed MFA attempts, potentially its duration to failure?). But we decided it was best to start with the metric we're most interested in first.

πŸ§ͺ Acceptance testing

We can only test this the PR is shipped and we begin capturing data.

🐢 πŸ“ˆ After PR is merged
We'll need to create a dashboard in Datadog in order to review the metrics for login.mfa.webauthn.duration and login.mfa.otp.duration.

@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Merging #3376 (5c0d66c) into master (9acb6dd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3376   +/-   ##
=======================================
  Coverage   98.58%   98.59%           
=======================================
  Files         113      113           
  Lines        3399     3408    +9     
=======================================
+ Hits         3351     3360    +9     
  Misses         48       48           
Impacted Files Coverage Ξ”
app/controllers/sessions_controller.rb 100.00% <100.00%> (ΓΈ)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bettymakes bettymakes force-pushed the record-login-durations branch 2 times, most recently from 8cac74d to c09afbc Compare January 26, 2023 20:38
Copy link
Contributor

@jchestershopify jchestershopify left a comment

Choose a reason for hiding this comment

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

One question but otherwise LGTM.

app/controllers/sessions_controller.rb Outdated Show resolved Hide resolved
@bettymakes bettymakes force-pushed the record-login-durations branch 4 times, most recently from 0de9c58 to 8d7648f Compare January 27, 2023 05:18
@simi
Copy link
Member

simi commented Jan 30, 2023

Last commit is wip commit.... Is there any additional change planned? If not, any chance to squash commits?

@bettymakes
Copy link
Contributor Author

bettymakes commented Feb 1, 2023

Thanks for following up @simi πŸ™Œ . I was gonna review whether I need to add any additional tests before I fix up the commit.

There's also one other metric we realized would be beneficial to capture (number of times WebAuthn is set up). I'll likely be working on this later today. Although, on second thought, it may make more sense to create a separate PR for adding that metric. Either way, I'll be making final amendments later today and will definitely squash the wip commit then 😊 πŸ‘ . Should be good for another round of reviews at that point.

@jenshenny jenshenny force-pushed the record-login-durations branch 2 times, most recently from 8c60488 to ec03967 Compare February 1, 2023 21:39
Copy link
Member

@jenshenny jenshenny left a comment

Choose a reason for hiding this comment

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

LGTM! Just have a few nits regarding the tests but not blocking.

I think we should edit the current login integration test to check if statsd is incremented (mfa login, webauthn login <- this test is moved to webauthn_sign_in_test.rb or something similar if you rebase from current master).

test/functional/sessions_controller_test.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@jchestershopify jchestershopify left a comment

Choose a reason for hiding this comment

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

LGTM.

@bettymakes bettymakes force-pushed the record-login-durations branch 2 times, most recently from be1b1b4 to 6f8ea8e Compare February 2, 2023 17:16
bettymakes and others added 3 commits February 2, 2023 16:45
Add telementry to record how long it takes for a
user to login with webauthn as their 2nd factor.

Also, refactor some tests within #webauthn_create.
Extract helper methods login_to_session and verify_challenge.

Co-authored-by: Jenny Shen <42748004+jenshenny@users.noreply.github.com>
Co-authored-by: bettymakes <makewithbetty@gmail.com>
Add telementry to record how long it takes for a
user to login with otp as their 2nd factor.

Co-authored-by: bettymakes <makewithbetty@gmail.com>
Co-authored-by: Jenny Shen <42748004+jenshenny@users.noreply.github.com>
Since the Time objects are converted to Strings on the client anyway,
we decided it'd be best to represent the data consistently with the client.
So, once we retrieve the session values from the client later, we will always
parse the string value back to a Time object.

Co-authored-by: bettymakes <makewithbetty@gmail.com>
@simi
Copy link
Member

simi commented Feb 2, 2023

πŸ€” Squash and merge or is it ok to merge as is now?

@bettymakes
Copy link
Contributor Author

bettymakes commented Feb 2, 2023

Yup! I think we're good to go. @simi, if you can help me merge, that'd be great πŸ™Œ . But also, I can ping Jenny or Ashley to help with that tomorrow. We plan on checking it out on staging first to make sure it's rigged up correctly 😊.

@simi simi merged commit d9d073c into rubygems:master Feb 2, 2023
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to staging February 2, 2023 23:13 Inactive
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to production February 3, 2023 00:03 Inactive
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.

None yet

4 participants