LG-229 Add reCAPTCHA to account creation and password reset screens

[stevegsa]

Why: To prevent bots from creating accounts or abusing the mail system.

How: By incorporating the recaptcha gem (invisible option) and logging three conditions upon submit: captcha valid, captcha present (the user can click on the page to make the captcha disappear and the token won't be sent), and captcha enabled (our system level toggle). We have also added an A/B test service which allows us to slow roll the feature while gathering statistics about impact. Effectively you can set the percentage of time you want the feature enabled through the recaptcha_enabled_percent feature flag. This service can easily be added to any feature flag for A/B testing. The state of the test (enabled/disabled) is stored in session and there is an option to have it reset at the beginning of each test.

Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:

• For DB changes, check for missing indexes, check to see if the changes
  affect other apps (such as the dashboard), make sure the DB columns in the
  various environments are properly populated, coordinate with devops, plan
  migrations in separate steps.

• For route changes, make sure GET requests don't change state or result in
  destructive behavior. GET requests should only result in information being
  read, not written.

• For encryption changes, make sure it is compatible with data that was
  encrypted with the old code.

• For secrets changes, make sure to update the S3 secrets bucket with the
  new configs in all environments.

• Do not disable Rubocop or Reek offenses unless you are absolutely sure
  they are false positives. If you're not sure how to fix the offense, please
  ask a teammate.

• When reading data, write tests for nil values, empty strings,
  and invalid formats.

• When calling redirect_to in a controller, use _url, not _path.

• When adding user data to the session, use the user_session helper
  instead of the session helper so the data does not persist beyond the user's
  session.

• When adding a new controller that requires the user to be fully
  authenticated, make sure to add before_action :confirm_two_factor_authenticated.

[stevegsa]

Screenshots:

If google thinks you are a bot:

[davemcorwin]

In general LG, some organizational comments.

[davemcorwin]

Is it possible to separate checking whether the feature is enabled and enabling the feature? Changing the state as a side effect of seeing if its enabled it seems unexpected to me (unless I'm measuring its quantum state...) Also, this looks like the only case where FeatureManagement is looking to any logic besides env variables to see if a feature is enabled and that may be confusing.

It seems like there are 2 levels here

1. whether the recaptcha feature is enabled for the environment (controlled by FeatureManagement)
2. whether the session requires the recaptcha (controlled by AbTest`)

What do you think about asking AbTest if its enabled and AbTest asking FeatureManagement instead of the other way around? Then maybe enabling it explicitly as a second step?

[stevegsa]

The enabled? method for the Ab test is really just deciding whether to run A or B so it is somewhat misleading. I'm not in favor of AB Test calling FeatureManagement because I wanted to leave it loosely coupled (though I did think about it...). The AB test service can be used standalone without any dependency on our FeatureManagement config.

[davemcorwin]

I'm wondering why this logic belongs in the ApplicationController instead of as a concern or super class for the form objects. It seems to me that its really just another part of the form that needs to be validated, and this would simplify the implementation and the tests a bit since you won't have to alter the controllers and can test most of the functionality at the form object level instead of at the controller level.

[stevegsa]

The form objects isn't a good option because verify_recaptcha is placed in ActionController::Base by the gem. The reason why I chose ApplicationController over a concern is because I thought eventually Recaptcha would be part of most pages.

[monfresh]

I would think we would only need it for unauthenticated pages. Also, what does recaptcha provide that rate limiting with Rack Attack doesn't?

[stevegsa]

Spinning through IPs in a block, spinning through anonymous IPs, etc.

[jmhooper]

👍 to just putting it where we need it for now. It isn't hard move it into the application controller later if we need to.

[stevegsa]

Yes. Adding it to a separate concern right now.

[davemcorwin]

That sounds reasonable, perhaps then we can have something responsible for Recaptcha that depends on both FeatureManagement and AbTest to determine whether to add it or not, that way both FeatureManagement and AbTest are independent of each other.

…

On Mon, May 7, 2018 at 9:21 AM, Steve Urciuoli ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/services/ab_test.rb <#2136 (comment)>: > @@ -0,0 +1,13 @@ +class AbTest + def initialize(key, percent_on) + @key = key + @percent_on = percent_on.to_s.to_i + end + + def enabled?(session, reset) The enabled? method for the Ab test is really just deciding whether to run A or B so it is somewhat misleading. I'm not in favor of AB Test calling FeatureManagement because I wanted to leave it loosely coupled (though I did think about it...). The AB test service can be used standalone without any dependency on our FeatureManagement config. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2136 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGWo43vSxmeFwBccnJSJaVpnATD-d5Jjks5twHR5gaJpZM4Tv5d4> .

-- David Corwin 18F <https://18f.gsa.gov/> | Consulting Engineer [email protected]

[davemcorwin]

Looks like we can add `verify_recaptcha` by adding `include Recaptcha::Verify` wherever we need it if we want to put it elsewhere...

…

On Mon, May 7, 2018 at 9:11 AM, Steve Urciuoli ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/controllers/application_controller.rb <#2136 (comment)>: > @@ -194,4 +194,16 @@ def analytics_exception_info(exception) exception_class: exception.class.name, } end + + def validate_recaptcha Spinning through IPs in a block, spinning through anonymous IPs, etc. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2136 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGWo433pL5PJmeebZ_3zh9NhzsCLjvM8ks5twHIvgaJpZM4Tv5d4> .

-- David Corwin 18F <https://18f.gsa.gov/> | Consulting Engineer [email protected]

[stevegsa]

@davemcorwin I see what you are saying wrt the featuremanagement and abtest. It would allow me to mock out a single behavior and not run through so many combinations. I did think about it but I'd still be stuck with providing some type of feature spec so I tried to mock out everything at the lowest possible level to sufficiently test all the code paths.

[davemcorwin]

Also, it looks like if recaptcha fails, it will fail the form validation, but we're not capturing the errors (except in analytics). Do we want to include any of this feedback to users? Will this be confusing when debugging/testing the forms?

[stevegsa]

The flash/errors are hidden because the recaptcha will popup again and obscure them. Essentially their error is seeing the recaptcha again.

[stevegsa]

wrt the featuremanagement and abtest: After looking at my code again the featuremanagement method was that single behavior fronting the mocks. Originally there was no dependence on the FeatureManagement since I was simply reading in an environment variable in Figaro.

[stevegsa]

We can't add include Recaptcha::Verify wherever we need it because it's pulling from request and params directly

[davemcorwin]

ahh, the magic of Rails!

…

On Mon, May 7, 2018 at 1:45 PM, Steve Urciuoli ***@***.***> wrote: We can't add include Recaptcha::Verify wherever we need it because it's pulling from request and params directly — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2136 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGWo46h1Q3cjYcMKedINSX-gcSpvEvJIks5twLJkgaJpZM4Tv5d4> .

-- David Corwin 18F <https://18f.gsa.gov/> | Consulting Engineer [email protected]