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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the mailer content to specify the MFA implementation used (TOTP) #3903

Merged
merged 2 commits into from Jul 6, 2023

Conversation

george-ma
Copy link
Contributor

@george-ma george-ma commented Jul 5, 2023

What problem are you solving?

Closes https://github.com/Shopify/ruby-dependency-security/issues/331
Refactored the email content to specify the MFA implementation used (TOTP)

What should reviewers focus on?

Not the biggest fan of the lengthiness of some of the content:

ie. the email subject

TOTP multi-factor authentication disabled on RubyGems.org

and the content body

(TOTP (Time-Based One-Time Password) multi-factor authentication)

but also just didn't like the way TOTP MFA looked (acronym overload), but I would be okay to change some/both of these if there any suggestions 馃槃

Testing 馃帺

Borrowing the testing instructions from a previous PR within the same section of code (thanks @garyhtou):

  1. Setup OTP MFA at http://localhost:3000/settings/edit
  2. Go to http://localhost:3000/letter_opener and verify MFA Enabled mailer was sent
  3. Disable OTP MFA at http://localhost:3000/settings/edit
  4. Go to http://localhost:3000/letter_opener and verify MFA Disabled mailer was sent

Can also view the updated mailers individually through http://localhost:3000/rails/mailers

  • Select "totp_disabled" / "totp_enabled"

@george-ma george-ma changed the title Rename Refactor the mailer content to specify the MFA implementation used (TOTP) Jul 5, 2023
@george-ma george-ma force-pushed the gm/rename-mfa-to-totp-on-mailer branch from 1fef7e9 to c7f45f9 Compare July 5, 2023 16:57
@simi
Copy link
Member

simi commented Jul 5, 2023

Thanks for the PR. Change looks good to me, but since I'm not native English speaker I'll leave this open for additional reviewer.

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #3903 (4eec994) into master (570a439) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3903   +/-   ##
=======================================
  Coverage   98.79%   98.79%           
=======================================
  Files         216      216           
  Lines        5391     5391           
=======================================
  Hits         5326     5326           
  Misses         65       65           
Impacted Files Coverage 螖
app/mailers/mailer.rb 100.00% <100.00%> (酶)
app/models/concerns/user_totp_methods.rb 100.00% <100.00%> (酶)

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! Thanks for helping to refactor this George.

I'm torn on the email heading title as well. Not a fan of the double acronyms. TOTP is too long to spell out, I think we can still work with spelling out MFA. Here are some alternatives I thought of. What do you think about these options?

  • TOTP Multi-factor Authentication Enabled
  • Multi-factor Authentication Enabled: TOTP
  • MFA Enabled: One-Time Password
    • I know I dropped the "Time-Based" part in this title. I figured it'd be an acceptable in-between since the email body notes that it's TOTP.

Mailer with heading: TOTP Multi-factor Authentication Enabled

Mailer with heading: Multi-factor Authentication Enabled: TOTP

Mailer with heading: MFA Enabled: One-Time Password

app/views/mailer/totp_enabled.html.erb Outdated Show resolved Hide resolved
app/views/mailer/totp_disabled.html.erb Outdated Show resolved Hide resolved
@george-ma
Copy link
Contributor Author

george-ma commented Jul 6, 2023

Email subject title changed to: Authentication app enabled/disabled on RubyGems.org

After some more discussions with Betty/Jenny, the rest of the email looks like this:

let me know if this looks good! cc: @bettymakes

@bettymakes
Copy link
Contributor

Changes look good! 馃殌

@george-ma george-ma force-pushed the gm/rename-mfa-to-totp-on-mailer branch from 5fa5de4 to 220ec1a Compare July 6, 2023 19:36
Update the mailer tests for the mfa to totp refactor

Rename other locales with the mfa to totp refactor

Update the wording to include 'authenticator app'
@george-ma george-ma force-pushed the gm/rename-mfa-to-totp-on-mailer branch from 220ec1a to e024593 Compare July 6, 2023 19:43
@jenshenny jenshenny merged commit f3140a9 into rubygems:master Jul 6, 2023
13 checks passed
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to staging July 6, 2023 20:27 Inactive
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to production July 12, 2023 20: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

5 participants