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

Mailer: Email to remind maintainers of gems with 180M+ downloads to enable MFA #3166

Merged
merged 9 commits into from
Aug 9, 2022

Conversation

bettymakes
Copy link
Contributor

@bettymakes bettymakes commented Aug 4, 2022

🤷‍♀️ What problem are you solving?

Contributes to https://github.com/Shopify/ruby-conventions/issues/134
Closes #3164

We need to send out a reminder email on August 8, 2022 to remind maintainers of gems with 180M+ downloads to enable MFA. This mailer is crafted with a targeted approach. It's only being sent to those impacted maintainers who have yet to enable MFA (disabled), or have weak MFA enabled (ui_only).

📋 How will you solve this?

🔹 Create a mailer

  • Adds a mailer action mfa_required_soon_announcement and a mailer view in both HTML and plain text formats
  • Note that there are minor copy variations dependent on a user's MFA status:
    • If the user does not have MFA enabled at all (disabled)
    • If the user has MFA enabled (ui_only)

🔹 Add rake task for delivering the email

  • This will be the task that the RubyGems Team will run on August 8 to send out the email reminder
  • I've followed similar logging that they've done in a previous mailer
  • Task to be run: rake mfa_policy:reminder_enable_mfa

🔹 Set up mailer preview

  • So we can more easily tophat and preview the email in both HTML and plain text, I've set up a mailer preview

🎩 Tophat instructions

Below are the instructions for trying this out yourself:

📧 To preview the email:

  • Once you have the app running (rails s) ...
  • You can navigate to (http://localhost:3000/rails/mailers) to review the list of all mailers available for preview
    • To view this mailer, click on the link titled: mfa_required_soon_announcement
  • The mailer for this PR is located at http://localhost:3000/rails/mailers/mailer/mfa_required_soon_announcement

👤 To change the user:

  • The user is configured on the mailer preview based on MailerPreview#mfa_required_soon_announcement.
    • To change the user to verify various MFA states, you can either replace User.last.id with a different user (e.g. User.first)
    • Alternatively, you can adjust the MFA status on your last user

👀 Email versions

🚫 Email for users with MFA disabled

Email for users with MFA disabled - HTML format Email for users with MFA disabled - Plain text format

🤞 Email for users with MFA enabled (ui_only) -- weak MFA

Email for users with MFA enabled set to ui_only - HTML format Email for users with MFA enabled set to ui_only - Plain text format

@bettymakes bettymakes force-pushed the mfa-mailer-reminder-one-week branch 4 times, most recently from 1c81217 to 3bfe9c2 Compare August 4, 2022 06:29
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.

Apart from test bug, LGTM.

app/views/mailer/mfa_required_soon_announcement.text.erb Outdated Show resolved Hide resolved
users.each do |user|
Mailer.delay.mfa_required_soon_announcement(user.id) if mx_exists?(user.email)
i += 1
print format("\r%.2f%% (%d/%d) complete", i.to_f / total_users * 100.0, i, total_users)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to count and report failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. It wouldn't hurt if we did. I think the value of counting and reporting failures is if we expect someone will do something with that information. I'd defer this to whoever will be running the task, if they'd want to action on that info.

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.

Didn't tophat but the email copies looks good to me overall!

app/views/mailer/mfa_required_soon_announcement.text.erb Outdated Show resolved Hide resolved
lib/tasks/mfa_policy.rake Outdated Show resolved Hide resolved
@bettymakes bettymakes force-pushed the mfa-mailer-reminder-one-week branch 2 times, most recently from 5b5f402 to 9d5901f Compare August 5, 2022 03:13
@bettymakes bettymakes marked this pull request as ready for review August 5, 2022 03:35
@bettymakes bettymakes force-pushed the mfa-mailer-reminder-one-week branch 3 times, most recently from 6d86cc9 to dd0d59f Compare August 5, 2022 22:02
@bettymakes bettymakes force-pushed the mfa-mailer-reminder-one-week branch 2 times, most recently from e972c08 to 19160d4 Compare August 7, 2022 05:30
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! Couple of last suggestions. Tophatted the mailer previews and they look good

Screen Shot 2022-08-08 at 12 43 18 PM Screen Shot 2022-08-08 at 12 38 27 PM

app/views/mailer/mfa_required_soon_announcement.html.erb Outdated Show resolved Hide resolved
app/mailers/mailer.rb Outdated Show resolved Hide resolved
@bettymakes bettymakes force-pushed the mfa-mailer-reminder-one-week branch 2 times, most recently from 05cb8b1 to 6d263bb Compare August 8, 2022 21:43
@bettymakes
Copy link
Contributor Author

@sonalkr132 The mailer is good to go for tomorrow 👍.

Subject line and heading should be relevant to the user. For users who
have MFA enabled, but simply need to stregnthen the mfa level, the copy
of 'Enable MFA' is an inaccurate call to action. Therefore, the copy has
been revised to better reflect an accurate CTA.
@bettymakes bettymakes force-pushed the mfa-mailer-reminder-one-week branch from 6d263bb to 63072d5 Compare August 9, 2022 03:24
@sonalkr132 sonalkr132 merged commit 3c6a688 into rubygems:master Aug 9, 2022
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to staging August 9, 2022 17:31 Inactive
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to production August 9, 2022 17:37 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.

Create a mailer to remind maintainers to enable MFA one week before MFA policy enforcement
5 participants