LG-8860: Add reCAPTCHA fallback error screen

[aduth]

🎫 Ticket

LG-8860

🛠 Summary of changes

Adds fallback error screen handling for reCAPTCHA failure scenario.

📜 Testing Plan

1. Generate reCAPTCHA credentials for use in your local environment: https://www.google.com/recaptcha/admin/create (Note you will need two sets of credentials, one for v2 and another for v3)
2. In config/application.yml:
   1. Assign credentials as recaptcha_site_key_v2, recaptcha_site_key_v3, recaptcha_secret_key_v2, and recaptcha_secret_key_v3
   2. Set phone_setup_recaptcha_score_threshold to 1.0
3. Sign in
4. Add a phone
5. Submit a number (e.g. 5135550100 for U.S., 3065550100 for international)
6. Observe error screen with reCAPTCHA checkbox if entering international number on Step 5, otherwise observe OTP confirmation screen
7. If presented with error screen with reCAPTCHA checkbox, confirm that clicking checkbox and successfully validating reCAPTCHA proceeds to OTP confirmation screen

👀 Screenshots

[aduth]

I think we could consider being more restrictive in this logic, specifically in checking that the reason for failure was due to a score falling outside the acceptable range (i.e. disallow failures due to blank or otherwise invalid tokens). One of the challenges with this is that if the user refreshes the page and resubmits the form contents, we'll likely get a timeout-or-duplicate error from the reCAPTCHA service. From the user's perspective, I think they should still have the chance to retry at this point. It has me wondering if should cache the token verification result in Redis for a short time, so we don't have to call out to the external service, and so that we would be able to validate it based on the initial service response.

That being said, it's probably best to save that work for a follow-on ticket.

[zachmargolis]

is this fallback page always version 2? is there a way to set this fro a controller or local variable from the form? just to minimize the number of places we hardcode things?

[aduth]

Yes it is. Although interestingly, Google enforces that the reCAPTCHA keys matches the configured version for the frontend display of the checkbox, but it appears that any reCAPTCHA key can be used for the verification endpoint.

There might be some option to configure this in the controller. One interesting thing is that all of these values will carry over from the previous submission, including recaptcha_version: 3 from the initial challenge. We'd want to make sure that we set that at the right point to make sure that we'd not be populating the wrong version in the form input.

[aduth]

I think that alternative ends up looking like adding a line before this:

https://github.com/18F/identity-idp/blob/38e3643026574229cda5b230543e927954c3a9c4/app/controllers/users/phones_controller.rb#L23

...as:

@new_phone_form.recaptcha_version = 2

...but it feels a little awkward / unpredictable to modify the form like that ahead of rendering the view? I guess contrasted with how it's implemented here, I'm a bit more comfortable with forcefully overriding the value in the view.

[zachmargolis]

just thinking out loud, if this new template is so different from existing stuff, should we add a new controller for it? so we redirect to users/phone_setup/spam_production#index or something?

counterpoint, would we be able to easily link back to the phone setup from that new controller if we did?

[aduth]

Yeah, I noodled on a few different approaches here. One key thing that is hard to do with other approaches is maintaining form values between the initial submission and the subsequent checkbox submission. As implemented here without a redirect, those form values carry over automatically. With a redirect, we'd have to find some other way to save those values (e.g. storing them in session).

[mdiarra3]

is there a reason we have to hardcode it like this and not let recaptcha version be passed in?

[aduth]

The main idea was to limit the set of potential values we'd allow for constructing the RecaptchaValidator class, since we wouldn't want to allow invalid versions, e.g. 4, etc (related spec). Perhaps that should be handled a bit more strictly within the validator class itself, though.

[aduth]

I think I'd like to keep this in the controller, but I also added some extra checks to the validator class in e4f0e1ac6 to add extra assurances that it's initialized correctly.

[aduth]

I refactored this a bit in a7018b44a to extract a few methods for readability, based on team review discussion yesterday that the implied nil score handling from a reCAPTCHA v2 result was not very obvious.