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

Rename user OTP methods to reference OTP instead of MFA [2/4] #3807

Merged
merged 3 commits into from May 19, 2023

Conversation

jenshenny
Copy link
Member

Follow up to #3806

What problem are you solving?

This PR is based off of a larger PR #3805

As part of #3800, there are methods in UserOtpMethods that use mfa in the name though they only relate to TOTP devices

What approach did you choose and why?

Renamed these methods to reference otp instead of mfa

What should reviewers focus on?

  • Are there better names for these methods?

@jenshenny jenshenny changed the title Rename to user OTP methods to reference OTP instead of MFA Rename user OTP methods to reference OTP instead of MFA May 18, 2023
@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Merging #3807 (976036e) into otp-mfa-concern (0d88d30) will increase coverage by 98.81%.
The diff coverage is 100.00%.

@@                 Coverage Diff                  @@
##           otp-mfa-concern    #3807       +/-   ##
====================================================
+ Coverage                 0   98.81%   +98.81%     
====================================================
  Files                    0      214      +214     
  Lines                    0     5243     +5243     
====================================================
+ Hits                     0     5181     +5181     
- Misses                   0       62       +62     
Impacted Files Coverage Δ
app/avo/actions/reset_user_2fa.rb 100.00% <100.00%> (ø)
app/controllers/multifactor_auths_controller.rb 100.00% <100.00%> (ø)
app/models/concerns/user_otp_methods.rb 100.00% <100.00%> (ø)
app/models/user.rb 100.00% <100.00%> (ø)

... and 210 files with indirect coverage changes

@jenshenny jenshenny changed the title Rename user OTP methods to reference OTP instead of MFA Rename user OTP methods to reference OTP instead of MFA [2/4] May 18, 2023
Copy link
Contributor

@ericherscovich ericherscovich left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@bettymakes bettymakes left a comment

Choose a reason for hiding this comment

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

Looks good 💯 .
The only other name I can think of is to be slightly more verbose in naming with totp. The benefit of this allows us to be extra clear that we're making a differentiating between time-based one time passwords vs the otp attribute on webauthn_verification records.

If we do make this change to be more explicit in naming to totp as opposed to otp, @ericherscovich should also revise his PR to be consistent. The new column in Eric's PR should be called totp_seed and the subsequent application code should align with this terminology too.

That said, this is simply a suggestion. I'm not strongly opinionated on this. Leaving it as food for thought with y'all.

@jenshenny jenshenny merged commit 976036e into rubygems:otp-mfa-concern May 19, 2023
7 checks passed
@jenshenny
Copy link
Member Author

jenshenny commented May 19, 2023

ack I don't remember merging this PR in 🤔

After discussing with Betty and Eric, we're gonna do the renaming to totp as it is more clear that it is referencing time based authentication. I'll make the edits in #3806 since this was merged somehow.

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

3 participants