LG-15655: Submit reCAPTCHA annotations through worker job (Part 2 of 2)

[aduth]

🎫 Ticket

LG-15655

🛠 Summary of changes

Updates RecaptchaAnnotator to asynchronously schedule a worker job to perform the annotation later.

It also removes the feature flag controlling whether to submit annotations for reCAPTCHA assessments performed at sign-in, effectively enabling the behavior everywhere. This is because the asynchronous annotations behavior was the blocker for enabling the behavior, and therefore is satisfied by the changes to RecaptchaAnnotator.

This builds upon:

• LG-15655: Offload reCAPTCHA annotations to worker job (Part 1 of 2) #11883, which implemented the worker job, to be deployed separate from the changes that use it
• Annotate sign-in reCAPTCHA from 2-factor attempts #11801, which introduced the feature-flagged reCAPTCHA annotations at sign-in

📜 Testing Plan

Repeat Testing Plan from #11883, noting that you will not have to replace commented code, since those are the changes being implemented in this pull request.

[mdiarra3]

Looks good. Tested this locally and worked as expected. Had a recaptcha assessment stored in db too.

[aduth]

Had a recaptcha assessment stored in db too.

Actually, this reminds me I'd originally wondered if it would make sense to delete the record after it's submitted successfully, as a sort of auto-cleanup for the table, since we don't need it any more 🤔

[aduth]

Actually, this reminds me I'd originally wondered if it would make sense to delete the record after it's submitted successfully, as a sort of auto-cleanup for the table, since we don't need it any more 🤔

I pushed changes for this in ebc7d1e.

[aduth]

Noting that these changes would only be effective for half of the worker instances in the 50/50 state, therefore some stale assessments could remain. That might be acceptable, since this is just a clean-up of unnecessary data, but the other option would be to split out a separate pull request with just these changes to deploy before the main pull request.

A third option could be to move the deletion into the RecaptchaAnnotator class. I wasn't originally a fan of that because it feels like the annotator's job should be to just submit the annotation, but maybe it fits the scope of "annotating" to include the clean-up of the assessment record, and keeps the job implementation lean. Pragmatically, it would also avoid having to deal with these issues or a third pull request.

[aduth]

Actually, the third option might still be best, but it doesn't actually work around the 50/50 state, since the new application instances would schedule the job from RecaptchaAnnotator, but the actual submission in RecaptchaAnnotator would still happen in a (possibly-old) worker instance.

[aduth]

Based on a conversation in Slack, I added timestamps to the table in be44d95, which should more easily allow us to delete any stale data in the future.

[aduth]

Normally I would have expected to be able to add the comment as part of the add_timestamps call:

Suggested change

	add_timestamps :recaptcha_assessments
	change_column_comment :recaptcha_assessments, :created_at, from: nil, to: 'sensitive=false'
	change_column_comment :recaptcha_assessments, :updated_at, from: nil, to: 'sensitive=false'
	add_timestamps :recaptcha_assessments, comment: 'sensitive=false'

But this was causing an error during migration, and it looks like the SQL query was including a stringified Ruby proc. Maybe a bug with add_timestamps? I had even tried it with a change_table { |t| t.add_timestamps ... } and got the same result.

ALTER TABLE "recaptcha_assessments" ADD "created_at" timestamp(6) NOT NULL, #<Proc:0x00000001476d1120 /gems/activerecord-8.0.1/lib/active_record/connection_adapters/postgresql/schema_statements.rb:1048>, ADD "updated_at" timestamp(6) NOT NULL, #<Proc:0x00000001476d0d38 /gems/activerecord-8.0.1/lib/active_record/connection_adapters/postgresql/schema_statements.rb:1048>