LG-6885: Remind users about backup codes & regenerate by jc-gsa · Pull Request #6723 · 18F/identity-idp

jc-gsa · 2022-08-10T19:35:19Z

No description provided.

app/controllers/concerns/verify_profile_concern.rb

mitchellhenke · 2022-08-10T19:40:17Z

app/decorators/user_decorator.rb

+      event_type: ['sign_in_before_2fa',
+                   'sign_in_after_2fa'],


If we're including sign_in_after_2fa, do we need to include sign_in_before_2fa?

Yeah, I questioned that myself. As the method is named second_last_signed_in_at I figured I'd keep it generic as possible and count both events. Open to other suggestions.

I find the name second_last_signed_in_at somewhat awkward, but found it necessary for a currently signed in user. Maybe the wording can be changed. Otherwise a method like last_signed_in_at returns the current login date for the current_user.

These columns were previously dropped from the User table when Devise's trackable module was removed, I believe: https://gsa-tts.slack.com/archives/C0NGESUN5/p1649098117066389

I guess that had the distinction:
current_sign_in_at vs. last_sign_in_at

Yep, they were unused (and duplicative of events), so I think it's fine to use events. I don't mind the naming, I think it's clear and explicit 🙂

When someone signs in, they will create both a sign_in_before_2fa and sign_in_after_2fa event, but I'm not sure if that means the 2nd most recent event will likely always be within the last few minutes. If it does, that's probably a decent enough reason to drop sign_in_before_2fa from the query?

aduth · 2022-08-11T12:34:51Z

app/views/users/backup_code_setup/reminder.html.erb

+  <%= @presenter.body_info %>
+</p>
+
+<div class="col-12">


I've seen this col-12 make an appearance quite often in recent months. col-12 is a remnant from Basscss which was removed quite a while ago. It seems redundant anyways, since it just assigns width: 100%, which would be the default for a block-level <div> element.

Can we just remove the wrapper?

Suggested change

<div class="col-12">

mitchellhenke · 2022-08-15T14:26:07Z

app/decorators/user_decorator.rb

+  def second_last_signed_in_at
+    user.events.where(
+      event_type: 'sign_in_after_2fa'
+    ).order(id: :desc).pluck(:created_at).second


Suggested change

).order(id: :desc).pluck(:created_at).second

).order(created_at: :desc).pluck(:created_at).second

I think created_at is preferable for the sort, and we already have an index on (user_id, created_at) as a bonus.

During testing, I think I had some specific reason for preferring id. This has been changed.

mitchellhenke · 2022-08-15T14:26:58Z

app/presenters/backup_code_reminder_presenter.rb

@@ -0,0 +1,21 @@
+class BackupCodeReminderPresenter


What do you think of dropping the presenter and putting the I18n calls directly in the template? In the case where we only have content in the presenter, I think it would be good to skip a layer of abstraction.

changelog: Internal, Authentication, Add backup code reminder MFA (LG-6885)

jmdembe

👍🏾

mitchellhenke

Couple comments, otherwise looks good!

mitchellhenke · 2022-08-17T15:17:58Z

app/controllers/concerns/verify_profile_concern.rb

+    mfa_user = MfaContext.new(current_user)
+    mfa_user.backup_code_configurations.present?


Suggested change

mfa_user = MfaContext.new(current_user)

mfa_user.backup_code_configurations.present?

MfaContext.new(current_user).backup_code_configurations.present?

mitchellhenke · 2022-08-17T15:18:56Z

app/decorators/user_decorator.rb

+  def last_signed_in_at
+    user.devices.order(last_used_at: :desc).first&.last_used_at
+  end
+


Suggested change

def last_signed_in_at

user.devices.order(last_used_at: :desc).first&.last_used_at

end

It looks like this isn't used?

Yeah, this isn't used. It can be removed. I originally included it because it seemed odd to have a method for second_last_signed_in_at but no method for last_signed_in_at.

zachmargolis

test approve after requested changes

jc-gsa added 10 commits August 4, 2022 15:39

Add route

469c6b4

Add controller action

df4c84c

Add reminder page

06e6161

Add presenter

1242d5e

Add English locale

2aa1238

Add spec for presenter

f792005

Add user last signed-in at helper

0394cdd

Add intercept for user needs backup codes reminder

1d7ba0d

Change check for user's last sign in date

2ac1aba

Add translations

fa24482

jc-gsa added the status - work in progress label Aug 10, 2022

mitchellhenke reviewed Aug 10, 2022

View reviewed changes

aduth reviewed Aug 11, 2022

View reviewed changes

jc-gsa added 2 commits August 11, 2022 16:45

Add refactor

636c5e4

Add spec

9db2962

mitchellhenke reviewed Aug 15, 2022

View reviewed changes

jc-gsa removed the status - work in progress label Aug 16, 2022

jc-gsa force-pushed the lg-6885-backup-code-reminder-regenerate branch 2 times, most recently from 86716a9 to 9ff9629 Compare August 16, 2022 15:35

Add refactor

ab336c3

jc-gsa force-pushed the lg-6885-backup-code-reminder-regenerate branch from 9ff9629 to ab336c3 Compare August 16, 2022 15:43

jc-gsa added 3 commits August 16, 2022 09:07

Add linting

838ac63

Add and change feature flag

e169bb3

changelog: Internal, Authentication, Add backup code reminder MFA (LG-6885)

Add linting

264e661

jc-gsa requested a review from a team August 16, 2022 21:38

jmdembe approved these changes Aug 17, 2022

View reviewed changes

mitchellhenke suggested changes Aug 17, 2022

View reviewed changes

Add review

77eb3ae

zachmargolis approved these changes Aug 17, 2022

View reviewed changes

sheldon-b approved these changes Aug 17, 2022

View reviewed changes

mitchellhenke approved these changes Aug 17, 2022

View reviewed changes

jc-gsa merged commit 9070c33 into main Aug 17, 2022

jc-gsa deleted the lg-6885-backup-code-reminder-regenerate branch August 17, 2022 21:49

jc-gsa restored the lg-6885-backup-code-reminder-regenerate branch August 29, 2022 23:12

jc-gsa deleted the lg-6885-backup-code-reminder-regenerate branch December 26, 2024 17:53

	).order(id: :desc).pluck(:created_at).second
	).order(created_at: :desc).pluck(:created_at).second

		mfa_user = MfaContext.new(current_user)
		mfa_user.backup_code_configurations.present?

	mfa_user = MfaContext.new(current_user)
	mfa_user.backup_code_configurations.present?
	MfaContext.new(current_user).backup_code_configurations.present?

	def last_signed_in_at
	user.devices.order(last_used_at: :desc).first&.last_used_at
	end

Conversation

jc-gsa commented Aug 10, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jc-gsa Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchellhenke Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdembe left a comment

Choose a reason for hiding this comment

Uh oh!

mitchellhenke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jc-gsa Aug 10, 2022 •

edited

Loading

aduth Aug 11, 2022 •

edited

Loading

mitchellhenke Aug 15, 2022 •

edited

Loading