Create warning banner CTA to add a second authentication method by jmdembe · Pull Request #6188 · 18F/identity-idp

jmdembe · 2022-04-12T15:24:50Z

This PR adds a CTA for users to add a second authentication banner in order to best secure their account. This is part of the "sad path" when a user does not select more than one method.

Note:

This will not render in production at this time;
The banner should only be displayed if multiple factors are enabled

mitchellhenke · 2022-04-12T16:06:36Z

app/controllers/sign_up/completions_controller.rb

Suggested change

def multiple_factors_enabled?

return if MfaPolicy.new(current_user).multiple_factors_enabled?

end

It looks like the view calls this method directly so this definition isn't needed?

You are right. Fixing this now

zachmargolis · 2022-04-12T16:27:13Z

config/locales/multi_factor_authentication/en.yml

I would consider moving the HTML outside of this translation, maybe even putting the period inside the link copy?

aduth · 2022-04-13T12:20:58Z

app/views/sign_up/completions/show.html.erb

Nit: Can we bring the closing tag onto the same line as the closing parenthesis, for consistency with line 32?

Suggested change

)

%>

) %>

aduth · 2022-04-13T12:27:39Z

app/views/sign_up/completions/show.html.erb

Should this be inverted? In other words, are we expecting to show a banner if the user does not have multiple factors set up?

At first, it felt kind of tricky to name since we are seeking the false value. I will think this through some more--maybe only-one-factor-enabled or something that shows what the variable actually does

aduth · 2022-04-13T12:29:07Z

spec/views/sign_up/completions/show.html.erb_spec.rb

Since the view doesn't deal with the user, but rather an instance variable set by the controller, I might expect the stubbing to occur with the @multiple_factors_enabled variable, not the user (where user stubbing could happen in the controller tests). Also, we're only testing one of the two possible variations of this behavior.

Maybe something like...

Suggested change

context 'mfa banner' do

let(:user) { create(:user, :signed_up, :with_webauthn_platform, :with_phone) }

it 'does not show a banner if multiple factors are enabled' do

render

expect(rendered).not_to have_content(I18n.t('multi_factor_authentication.second_method_warning.text'))

end

end

context 'mfa banner' do

let(:multiple_factors_enabled) { false }

before do

@multiple_factors_enabled = multiple_factors_enabled

end

it 'shows a banner' do

render

expect(rendered).to have_content(t('multi_factor_authentication.second_method_warning.text'))

end

context 'with multiple factors enabled' do

let(:multiple_factors_enabled) { true }

it 'does not show a banner' do

render

expect(rendered).not_to have_content(t('multi_factor_authentication.second_method_warning.text'))

end

end

end

aduth · 2022-04-13T12:29:39Z

app/views/sign_up/completions/show.html.erb

Should this banner only be shown when the select_multiple_mfa_options feature flag is enabled?

zachmargolis · 2022-04-13T20:46:21Z

app/views/sign_up/completions/show.html.erb

bet one could put this on one line if we wanted

Suggested change

<%= t(

'mfa.second_method_warning.text',

) %>

<%= t('mfa.second_method_warning.text') %>

This makes sense. I probably did this while I was frustrated with the linter 😬

aduth · 2022-04-13T20:55:20Z

app/views/sign_up/completions/show.html.erb

Can we use a simple ! negation?

Suggested change

<% if @multiple_factors_enabled == false && IdentityConfig.store.select_multiple_mfa_options == false %>

<% if !@multiple_factors_enabled && !IdentityConfig.store.select_multiple_mfa_options %>

aduth · 2022-04-13T20:55:57Z

app/views/sign_up/completions/show.html.erb

Should we be checking that select_multiple_mfa_options is enabled, not disabled? I'm expecting the notice would only be shown when feature flag is turned on?

Suggested change

<% if @multiple_factors_enabled == false && IdentityConfig.store.select_multiple_mfa_options == false %>

<% if !@multiple_factors_enabled && IdentityConfig.store.select_multiple_mfa_options %>

It should be enabled because we only want it when select_multiple_mfa_options is true. I think I just needed fresh eyes on this 😅

zachmargolis

LGTM

aduth

Small comment about the specs and clarifying question about the link, but otherwise looks good! 👍

spec/views/sign_up/completions/show.html.erb_spec.rb

aduth · 2022-04-15T12:53:37Z

app/views/sign_up/completions/show.html.erb

+    <%= link_to(
+          t('mfa.second_method_warning.link'),
+          login_two_factor_options_url,
+        ) %>


Is this link supposed to be functional right now? When I tested, it looks like it just refreshes the same page.

It is supposed to be functional. Turns out I used login_two_factor_options_url (which doesn't exist) and that caused the 302 that happened in between the click -> redirect -> same page.

Hm, after testing again with the latest version of the branch, it still doesn't seem to be working for me.

On a related note, it might also be good to have some feature specs for this flow to have some end-to-end assurance that the user experience is working as expected.

I had not pushed the change up yet. That's my mistake.

I agree with having feature specs. I hadn't thought of starting that because most of the tickets for the feature were about building out the UI as the routing was being completed.

I also just learned that those tests are being written in the "good path" flow, that is already being started.

Another note, feature specs for the "good path" are being written in this PR. Since this is for the sad path, I am garnering that we can add on to the "sad path" after the "happy path" since the only way to get to this banner is to click "Skip for now" and that is coming in after the interstitial page routing is complete.

zachmargolis · 2022-04-20T18:45:43Z

app/views/sign_up/completions/show.html.erb

erblint will get cranky at this

Suggested change

two_factor_options_url(:mfa_selected => @multiple_factors_enabled),

two_factor_options_url(mfa_selected: @multiple_factors_enabled),

Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
In file: app/views/sign_up/completions/show.html.erb:31

zachmargolis · 2022-04-20T18:46:28Z

spec/features/multi_factor_authentication/mfa_cta_spec.rb

surprising indentation here (it's tabs... :/)

Suggested change

include DocAuthHelper

include SamlAuthHelper

context 'multiple factor authentication feature is disabled' do

include DocAuthHelper

include SamlAuthHelper

context 'multiple factor authentication feature is disabled' do

aduth · 2022-04-21T14:54:39Z

spec/features/multi_factor_authentication/mfa_cta_spec.rb

+    it 'displays a banner' do
+      visit_idp_from_sp_with_ial1(:oidc)
+      user = sign_up_and_set_password
+      select_2fa_option('backup_code')
+      click_continue
+
+      expect(page).to have_current_path(sign_up_completed_path)
+      expect(MfaPolicy.new(user).multiple_factors_enabled?).to eq false
+      expect(page).to have_content(t('mfa.second_method_warning.text'))
+    end
+
+    it 'does not display a banner' do


Could we clarify the difference between these two test case descriptions?

Either with context....

context 'with one MFA selected' do it 'displays a banner' do # ... context 'with multiple MFA selected' do it 'does not display a banner' do

Or in the name itself:

it 'displays a banner after configuring a single MFA' do # ... it 'does not display a banner after configuring multiple MFA' do

aduth · 2022-04-21T15:12:28Z

app/views/sign_up/completions/show.html.erb

+  <%= render(AlertComponent.new(type: :warning, class: 'margin-bottom-4')) do %>
+    <%= link_to(
+          t('mfa.second_method_warning.link'),
+          two_factor_options_url(mfa_selected: @multiple_factors_enabled),


Passing @multiple_factors_enabled here gives me the impression the parameter is handled as either true or false, when in reality the value doesn't seem to matter, since the check for params[:mfa_selected].present? would pass regardless if it was "true" or "false".

I wonder if instead, we should name it something which indicates that they're being sent for the purpose of setting up another MFA, e.g.

Suggested change

two_factor_options_url(mfa_selected: @multiple_factors_enabled),

two_factor_options_url(multiple_mfa_setup: ''),

Or, maybe we just disable the logic of confirm_user_needs_2fa_setup altogether, if we're now expecting that a user might be setting up multiple MFAs? (Speaking of: should the logic there be considering the feature flag?)

From what I understand, .present? requires a boolean value. Now, if we want to test if the multiple_mfa_setup simply exists, params.has_key?() appears to be what we are looking for. I can swap that out because we are not doing anything with the true or false value.

As for check if the logic is there and if the feature flag is enabled, I am not sure if that is needed. On that line, I was thinking that line returns that if the param exists. That param is only added with the link on the banner, and that banner only shows if the flag is on and multiple factors are not enabled. So it is like that check is being done beforehand.

With confirm_user_needs_2fa_setup, I am not sure if that should be disabled because we still need to determine if user_needs_sp_auth_method_setup? is true. I can see the case for confirm_user_needs_2fa_setup being refactored to better describe what it will check and if that logic should reflect whether or not a service provider will require MFA setup.

aduth

Small comment about typo, but otherwise LGTM 👍

aduth · 2022-04-21T16:43:09Z

app/controllers/users/two_factor_authentication_setup_controller.rb

Typo:

Suggested change

return if params.has_key?(:multipe_mfa_setup)

return if params.has_key?(:multiple_mfa_setup)

aduth · 2022-04-21T16:43:18Z

app/views/sign_up/completions/show.html.erb

Suggested change

two_factor_options_url(multipe_mfa_setup: ''),

two_factor_options_url(multiple_mfa_setup: ''),

…s to add a second MFA changelog: Improvements, Multi-factor authentication feature, Add call to action banner to add second MFA options Co-Authored-By: Andrew Duthie <andrew.duthie@gsa.gov>

mitchellhenke reviewed Apr 12, 2022

View reviewed changes

zachmargolis reviewed Apr 12, 2022

View reviewed changes

jmdembe force-pushed the LG-6102-warning-banner branch from 5884fd6 to bebda5a Compare April 12, 2022 20:57

jmdembe marked this pull request as ready for review April 12, 2022 21:33

aduth reviewed Apr 13, 2022

View reviewed changes

jmdembe force-pushed the LG-6102-warning-banner branch 6 times, most recently from 57b053d to de92cae Compare April 13, 2022 20:33

zachmargolis reviewed Apr 13, 2022

View reviewed changes

jmdembe force-pushed the LG-6102-warning-banner branch from de92cae to 9e28d5f Compare April 13, 2022 20:49

aduth reviewed Apr 13, 2022

View reviewed changes

jmdembe force-pushed the LG-6102-warning-banner branch 2 times, most recently from ed57522 to 26a9ae8 Compare April 14, 2022 16:46

zachmargolis approved these changes Apr 14, 2022

View reviewed changes

jmdembe force-pushed the LG-6102-warning-banner branch 4 times, most recently from 03e1df8 to 2e24033 Compare April 14, 2022 17:49

aduth approved these changes Apr 15, 2022

View reviewed changes

jmdembe force-pushed the LG-6102-warning-banner branch 6 times, most recently from bfda7f3 to d837622 Compare April 20, 2022 13:26

zachmargolis reviewed Apr 20, 2022

View reviewed changes

jmdembe force-pushed the LG-6102-warning-banner branch from b697c06 to d19b205 Compare April 20, 2022 21:22

jmdembe requested review from aduth, mdiarra3, mitchellhenke and zachmargolis April 21, 2022 14:09

jmdembe force-pushed the LG-6102-warning-banner branch from 2a4ba34 to db1129d Compare April 21, 2022 14:25

aduth reviewed Apr 21, 2022

View reviewed changes

aduth approved these changes Apr 21, 2022

View reviewed changes

jmdembe force-pushed the LG-6102-warning-banner branch 4 times, most recently from 3f26711 to 73a1937 Compare April 21, 2022 19:59

LG-6102: Add CTA banner on sign in completions page to encourage user…

061e04d

…s to add a second MFA changelog: Improvements, Multi-factor authentication feature, Add call to action banner to add second MFA options Co-Authored-By: Andrew Duthie <andrew.duthie@gsa.gov>

jmdembe force-pushed the LG-6102-warning-banner branch from 73a1937 to 061e04d Compare April 21, 2022 20:43

jmdembe merged commit 2fd36fc into main Apr 22, 2022

jmdembe deleted the LG-6102-warning-banner branch April 22, 2022 13:52

jmdembe mentioned this pull request Apr 26, 2022

Deploy RC 188 to Prod #6254

Merged

-    def multiple_factors_enabled?
-      return if MfaPolicy.new(current_user).multiple_factors_enabled?
-    end

-  context 'mfa banner' do
-    let(:user) { create(:user, :signed_up, :with_webauthn_platform, :with_phone) }
-    it 'does not show a banner if multiple factors are enabled' do
-      render
-      expect(rendered).not_to have_content(I18n.t('multi_factor_authentication.second_method_warning.text'))
-    end
-  end
+  context 'mfa banner' do
+    let(:multiple_factors_enabled) { false }
+    before do
+      @multiple_factors_enabled = multiple_factors_enabled
+    end
+    it 'shows a banner' do
+      render
+      expect(rendered).to have_content(t('multi_factor_authentication.second_method_warning.text'))
+    end
+    context 'with multiple factors enabled' do
+      let(:multiple_factors_enabled) { true }
+      it 'does not show a banner' do
+        render
+        expect(rendered).not_to have_content(t('multi_factor_authentication.second_method_warning.text'))
+      end
+    end
+  end

-    <%= t(
-          'mfa.second_method_warning.text',
-        ) %>
+    <%= t('mfa.second_method_warning.text') %>

	<% if @multiple_factors_enabled == false && IdentityConfig.store.select_multiple_mfa_options == false %>
	<% if !@multiple_factors_enabled && !IdentityConfig.store.select_multiple_mfa_options %>

	<% if @multiple_factors_enabled == false && IdentityConfig.store.select_multiple_mfa_options == false %>
	<% if !@multiple_factors_enabled && IdentityConfig.store.select_multiple_mfa_options %>

	two_factor_options_url(:mfa_selected => @multiple_factors_enabled),
	two_factor_options_url(mfa_selected: @multiple_factors_enabled),

-	include DocAuthHelper
-	include SamlAuthHelper
-	context 'multiple factor authentication feature is disabled' do
+  include DocAuthHelper
+  include SamlAuthHelper
+  context 'multiple factor authentication feature is disabled' do

	two_factor_options_url(mfa_selected: @multiple_factors_enabled),
	two_factor_options_url(multiple_mfa_setup: ''),

	return if params.has_key?(:multipe_mfa_setup)
	return if params.has_key?(:multiple_mfa_setup)

	two_factor_options_url(multipe_mfa_setup: ''),
	two_factor_options_url(multiple_mfa_setup: ''),

Conversation

jmdembe commented Apr 12, 2022

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!

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!

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!

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!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

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!

jmdembe Apr 19, 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!

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!

jmdembe Apr 21, 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!

aduth 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmdembe Apr 19, 2022 •

edited

Loading

jmdembe Apr 21, 2022 •

edited

Loading