Lg 6167 second mfa for sms

[SammySteiner]

Why

Phones are considered a restricted mfa method and are not considered as secure as other mfa methods. Therefore, we want to require all new users to setup their accounts with an additional mfa method if they select phone as their only mfa method. Ultimately, we'd like to force all users to have an authentication method that is not phone.

How

Creating an mfa checkbox component with validation and js to remove error when they select an additional mfa method.

Issues

Should this component be brought into the design system?

I still need to create a test to confirm the error disappears when another option is selected or phone is unselected.

[aduth]

The custom element isn't really designed to be used independent of the ViewComponent implementation. An effect of this is there's no guaranteed it'd actually be defined in the first place. It might be currently working incidentally based on how Webpack is splitting JavaScript bundles, but this could break in the future.

Based on how this is implemented, I wonder if we could take advantage of the fact that ValidatedField is basically a wrapper for a SimpleForm input field, and instead implement this as a custom input, or a wrapper configuration like we do with other checkboxes and radios.

This would also help avoid reimplementing error alert messages, which ValidatedField is already meant to be handling to create accessible linking between the field and the error message (not present here).

[SammySteiner]

Thanks for the feedback, I think I understood your comment, but let me know if I'm missing something.

I switched it to a div with a class for the scss to hit it.

I initially tried to use ValidatedField for this but ran into a couple issues.

1. The phone input is only invalid if no other input is selected, I wasn't able to figure out how to have validated field check other inputs to determine validation.
2. ValidatedField requires a :name to wire everything up properly. That name must be an attribute on the model, in this case the two_factor_options_form. However the form has a selection attribute that is an array, and name is only compatible with strings. I spent about a day trying to figure out how to convert from two_factor_options_form[:selection][] (or two_factor_options_form[:selection][:phone], etc.) to a usable :name.
3. Refactoring two_factor_options_form to have the appropriate attributes so that I can use validated field adds a bunch of extra work for a requirement with a lot of urgency behind it and doesn't clear up the first issue for me.

[aduth]

This is pretty specific to the one form, and not really a reusable component. Also, ValidatedField is already implementing quite a bit around field validation that seems to be repeated here.

Could we target a class or element name instead? Also, if we apply validity to the input using input.setCustomValidity, the default behavior of ValidatedField might be able to automatically handle parts of showing / hiding error messages (see PhoneInputComponent for example), as well as do some client-side validation, so we don't have to do a full server form submission to validate the input.

const checkboxes = document.querySelectorAll('.some-checkbox-class');
const phoneCheckbox = checkboxes.find((checkbox) => checkbox.name === 'phone');
checkboxes.forEach((checkbox) => {
  checkbox.addEventListener('input', () => {
    const checked = checkboxes
      .filter((checkbox) => checkbox.checked)
      .map((checkbox) => checkbox.name);
    const hasPhone = checked.includes('phone');
    const hasOther = hasPhone && checked.length > 1;
    phoneCheckbox.setCustomValidity(hasPhone && !hasOther ? 'some error' : '');
  });
});

[SammySteiner]

I couldn't reuse validated field for the reasons I mentioned in my other comment above.

I'm targeting a pretty specific label name to get a list of the labels I want to target already. Not sure what using the querySelectorAll would add.

One note is that the intended behavior is for the error to clear when the user selects another input (or unselects phone input), but not come back if they unselect the other input (or reselect just the phone input). It would only show again if they tried to resubmit with just phone selected.

[aduth]

I think the key thing for me here is that a ViewComponent implies some amount of reusability, or at least leverages the separation of the class from the view for managing a complex view model. I don't really see either of those here, so I'd almost wonder if a partial makes more sense, located somewhere alongside other MFA form views.

Calling document.getElementsByName('two_factor_options_form[selection][]') will only work for this one specific form, so the idea with querySelectorAll was to at least make it generic enough if that form name were to change, or if the component/partial were to be used anywhere else.

[SammySteiner]

Sounds good. I turned it into a partial since I think this component and validation requirement is super specific to this form and I move the js into a javascript/pack.

[zachmargolis]

is this temporary debugging code?

[SammySteiner]

yes, I was having trouble getting the js to run in the feature test and I was confirming that was the issue.

[SammySteiner]

This has been removed.

[aduth]

This component doesn't exist anymore, should be adapted to a partial spec.

[SammySteiner]

thanks for catching that.

[aduth]

Since the script applies to partial content, probably makes sense for the javascript_packs_tag_once call to occur inside the partial.

[SammySteiner]

Agreed. I tried to put it there initially, but it wasn't working. Turns out I had to restart the application for that to work.

[aduth]

The name and way this method is called wouldn't imply it depends on a feature flag. I suppose if it's temporary, it's not a big deal, but alternative we could name the method accordingly, or update the if: condition to apply multiple conditions.

def enforce_multiple_mfa_with_phone?

validates :selection,
          length: { minimum: 2, message: 'phone' },
          if: [:multiple_mfa_options_enabled?, :phone_selected?]

[SammySteiner]

agreed, that's cleaner.

[aduth]

I'm sorta confused by what selections this class is expecting to receive. In practice, we only offer the user a single "phone" option when configuring MFA, so when would it be "voice" or "sms"?

[SammySteiner]

I think that's for editing the users's selection later. After they change their preference, we still need to enforce the requirement for alternatives to phone, sms, and voice.

[aduth]

From what I can tell, how we're using this alert is as an error message, so seems we can just use the default styling, rather than creating new variant of alert.

https://design.login.gov/components/form-fields/#form-validation-errors

Suggested change

	<%= render AlertComponent.new(type: :error, id: 'phone_error', class: 'margin-bottom-1 margin-top-neg-1 checkbox__alert', message: flash[:phone_error]) %>
	<div id="phone_error" class="usa-error-message margin-bottom-1 margin-top-neg-1" role="alert">
	<%= flash[:phone_error] %>
	</div>

[SammySteiner]

agreed. I removed the unnecessary sass as well

[aduth]

Closing tag is main thing to be fixed, but otherwise LGTM 👍

[aduth]

Since we're TypeScript-first now, can this be a .ts file? Should hopefully just be a matter of renaming.

[aduth]

If this is feature-flagged, do we need to make sure we only include the bolded message if the feature flag is enabled?

[SammySteiner]

Ugh, that is a really good point. Checking with content.