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

app/controllers/concerns/two_factor_authenticatable_methods.rb

@@ -29,18 +29,24 @@ def handle_verification_for_authentication_context(result:, auth_method:, extra_
     if result.success?
       handle_valid_verification_for_authentication_context(auth_method:)
       user_session.delete(:mfa_attempts)
-      session.delete(:sign_in_recaptcha_assessment_id)
+      session.delete(:sign_in_recaptcha_assessment_id) if sign_in_recaptcha_annotation_enabled?
     else
       handle_invalid_verification_for_authentication_context
     end
   end
 
   def annotate_recaptcha(reason)
-    RecaptchaAnnotator.annotate(assessment_id: session[:sign_in_recaptcha_assessment_id], reason:)
+    if sign_in_recaptcha_annotation_enabled?
+      RecaptchaAnnotator.annotate(assessment_id: session[:sign_in_recaptcha_assessment_id], reason:)
+    end
   end
 
   private
 
+  def sign_in_recaptcha_annotation_enabled?
+    IdentityConfig.store.sign_in_recaptcha_annotation_enabled
+  end
+
   def handle_valid_verification_for_authentication_context(auth_method:)
     mark_user_session_authenticated(auth_method:, authentication_type: :valid_2fa)
     disavowal_event, disavowal_token = create_user_event_with_disavowal(:sign_in_after_2fa)

app/services/recaptcha_annotator.rb

@@ -21,8 +21,10 @@ def annotate(assessment_id:, reason: nil, annotation: nil)
       return if assessment_id.blank?
 
       if FeatureManagement.recaptcha_enterprise?
-        assessment = create_or_update_assessment!(assessment_id:, reason:, annotation:)
-        RecaptchaAnnotateJob.perform_later(assessment:)
+        submit_annotation(assessment_id:, reason:, annotation:)
+        # Future:
+        # assessment = create_or_update_assessment!(assessment_id:, reason:, annotation:)
+        # RecaptchaAnnotateJob.perform_later(assessment:)
       end
 
       { assessment_id:, reason:, annotation: }
@@ -34,7 +36,6 @@ def submit_assessment(assessment)
         annotation: assessment.annotation_before_type_cast,
         reason: assessment.annotation_reason_before_type_cast,
       )
-      assessment.destroy
     end
 
     private

config/application.yml.default

@@ -390,6 +390,7 @@ short_term_phone_otp_max_attempt_window_in_seconds: 10
 short_term_phone_otp_max_attempts: 2
 show_unsupported_passkey_platform_authentication_setup: false
 show_user_attribute_deprecation_warnings: false
+sign_in_recaptcha_annotation_enabled: false
 sign_in_recaptcha_log_failures_only: false
 sign_in_recaptcha_percent_tested: 0
 sign_in_recaptcha_score_threshold: 0.0
@@ -507,6 +508,7 @@ development:
   secret_key_base: development_secret_key_base
   session_encryption_key: 27bad3c25711099429c1afdfd1890910f3b59f5a4faec1c85e945cb8b02b02f261ba501d99cfbb4fab394e0102de6fecf8ffe260f322f610db3e96b2a775c120
   show_unsupported_passkey_platform_authentication_setup: true
+  sign_in_recaptcha_annotation_enabled: true
   sign_in_recaptcha_percent_tested: 100
   sign_in_recaptcha_score_threshold: 0.3
   skip_encryption_allowed_list: '["urn:gov:gsa:SAML:2.0.profiles:sp:sso:localhost"]'
@@ -607,6 +609,7 @@ test:
   secret_key_base: test_secret_key_base
   session_encryption_key: 27bad3c25711099429c1afdfd1890910f3b59f5a4faec1c85e945cb8b02b02f261ba501d99cfbb4fab394e0102de6fecf8ffe260f322f610db3e96b2a775c120
   short_term_phone_otp_max_attempts: 100
+  sign_in_recaptcha_annotation_enabled: true
   skip_encryption_allowed_list: '[]'
   socure_docv_document_request_endpoint: 'https://sandbox.socure.test/documnt-request'
   socure_docv_webhook_secret_key: 'secret-key'

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.0].define(version: 2025_02_28_141538) do
+ActiveRecord::Schema[8.0].define(version: 2025_02_24_134110) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
   enable_extension "pg_catalog.plpgsql"
@@ -482,8 +482,6 @@
   create_table "recaptcha_assessments", id: :string, force: :cascade do |t|
     t.string "annotation", comment: "sensitive=false"
     t.string "annotation_reason", comment: "sensitive=false"
-    t.datetime "created_at", null: false, comment: "sensitive=false"
-    t.datetime "updated_at", null: false, comment: "sensitive=false"
   end
 
   create_table "registration_logs", force: :cascade do |t|

lib/identity_config.rb

@@ -428,6 +428,7 @@ def self.store
     config.add(:sign_in_user_id_per_ip_attempt_window_in_minutes, type: :integer)
     config.add(:sign_in_user_id_per_ip_attempt_window_max_minutes, type: :integer)
     config.add(:sign_in_user_id_per_ip_max_attempts, type: :integer)
+    config.add(:sign_in_recaptcha_annotation_enabled, type: :boolean)
     config.add(:sign_in_recaptcha_log_failures_only, type: :boolean)
     config.add(:sign_in_recaptcha_percent_tested, type: :integer)
     config.add(:sign_in_recaptcha_score_threshold, type: :float)

spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb

@@ -168,28 +168,53 @@
       context 'when there is a sign_in_recaptcha_assessment_id in the session' do
         let(:assessment_id) { 'projects/project-id/assessments/assessment-id' }
 
-        it 'annotates assessment with PASSED_TWO_FACTOR and clears assessment id from session' do
-          recaptcha_annotation = {
-            assessment_id:,
-            reason: RecaptchaAnnotator::AnnotationReasons::PASSED_TWO_FACTOR,
-          }
+        context 'when sign_in_recaptcha_annotation_enabled is true' do
+          before do
+            allow(IdentityConfig.store).to receive(:sign_in_recaptcha_annotation_enabled)
+              .and_return(true)
+          end
 
-          controller.session[:sign_in_recaptcha_assessment_id] = assessment_id
+          it 'annotates assessment with PASSED_TWO_FACTOR and clears assessment id from session' do
+            recaptcha_annotation = {
+              assessment_id:,
+              reason: RecaptchaAnnotator::AnnotationReasons::PASSED_TWO_FACTOR,
+            }
 
-          expect(RecaptchaAnnotator).to receive(:annotate)
-            .with(**recaptcha_annotation)
-            .and_return(recaptcha_annotation)
+            controller.session[:sign_in_recaptcha_assessment_id] = assessment_id
 
-          stub_analytics
+            expect(RecaptchaAnnotator).to receive(:annotate)
+              .with(**recaptcha_annotation)
+              .and_return(recaptcha_annotation)
 
-          expect { result }
-            .to change { controller.session[:sign_in_recaptcha_assessment_id] }
-            .from(assessment_id).to(nil)
+            stub_analytics
 
-          expect(@analytics).to have_logged_event(
-            'Multi-Factor Authentication',
-            hash_including(recaptcha_annotation:),
-          )
+            expect { result }
+              .to change { controller.session[:sign_in_recaptcha_assessment_id] }
+              .from(assessment_id).to(nil)
+
+            expect(@analytics).to have_logged_event(
+              'Multi-Factor Authentication',
+              hash_including(recaptcha_annotation:),
+            )
+          end
+        end
+
+        context 'when sign_in_recaptcha_annotation_enabled is false' do
+          before do
+            allow(IdentityConfig.store).to receive(:sign_in_recaptcha_annotation_enabled)
+              .and_return(false)
+          end
+
+          it 'does not annotate the assessment' do
+            controller.session[:sign_in_recaptcha_assessment_id] = assessment_id
+
+            expect(RecaptchaAnnotator).not_to receive(:annotate)
+
+            stub_analytics
+
+            expect { result }
+              .not_to change { controller.session[:sign_in_recaptcha_assessment_id] }
+          end
         end
       end
     end
@@ -228,28 +253,52 @@
       context 'when there is a sign_in_recaptcha_assessment_id in the session' do
         let(:assessment_id) { 'projects/project-id/assessments/assessment-id' }
 
-        it 'annotates assessment with FAILED_TWO_FACTOR and clears assessment id from session' do
-          recaptcha_annotation = {
-            assessment_id:,
-            reason: RecaptchaAnnotator::AnnotationReasons::FAILED_TWO_FACTOR,
-          }
+        context 'when sign_in_recaptcha_annotation_enabled is true' do
+          before do
+            allow(IdentityConfig.store).to receive(:sign_in_recaptcha_annotation_enabled)
+              .and_return(true)
+          end
+
+          it 'annotates assessment with FAILED_TWO_FACTOR and clears assessment id from session' do
+            recaptcha_annotation = {
+              assessment_id:,
+              reason: RecaptchaAnnotator::AnnotationReasons::FAILED_TWO_FACTOR,
+            }
+
+            controller.session[:sign_in_recaptcha_assessment_id] = assessment_id
+
+            expect(RecaptchaAnnotator).to receive(:annotate)
+              .with(**recaptcha_annotation)
+              .and_return(recaptcha_annotation)
+
+            stub_analytics
 
-          controller.session[:sign_in_recaptcha_assessment_id] = assessment_id
+            expect { result }
+              .not_to change { controller.session[:sign_in_recaptcha_assessment_id] }
+              .from(assessment_id)
 
-          expect(RecaptchaAnnotator).to receive(:annotate)
-            .with(**recaptcha_annotation)
-            .and_return(recaptcha_annotation)
+            expect(@analytics).to have_logged_event(
+              'Multi-Factor Authentication',
+              hash_including(recaptcha_annotation:),
+            )
+          end
+        end
+        context 'when sign_in_recaptcha_annotation_enabled is false' do
+          before do
+            allow(IdentityConfig.store).to receive(:sign_in_recaptcha_annotation_enabled)
+              .and_return(false)
+          end
+
+          it 'does not annotate the assessment' do
+            controller.session[:sign_in_recaptcha_assessment_id] = assessment_id
 
-          stub_analytics
+            expect(RecaptchaAnnotator).not_to receive(:annotate)
 
-          expect { result }
-            .not_to change { controller.session[:sign_in_recaptcha_assessment_id] }
-            .from(assessment_id)
+            stub_analytics
 
-          expect(@analytics).to have_logged_event(
-            'Multi-Factor Authentication',
-            hash_including(recaptcha_annotation:),
-          )
+            expect { result }
+              .not_to change { controller.session[:sign_in_recaptcha_assessment_id] }
+          end
         end
       end
     end

spec/services/recaptcha_annotator_spec.rb

@@ -52,18 +52,21 @@
           .and_return(recaptcha_enterprise_project_id)
         allow(IdentityConfig.store).to receive(:recaptcha_enterprise_api_key)
           .and_return(recaptcha_enterprise_api_key)
-        allow(RecaptchaAnnotateJob).to receive(:perform_later)
+        stub_request(:post, annotation_url)
+          .with do |req|
+            parsed_body = JSON.parse(req.body)
+            next if reason && parsed_body['reasons'] != [reason.to_s]
+            next if !reason && parsed_body.key?('reasons')
+            next if annotation && parsed_body['annotation'] != annotation.to_s
+            true
+          end
+          .to_return(headers: { 'Content-Type': 'application/json' }, body: '{}')
       end
 
-      it 'schedules the annotation to be submitted' do
-        expect(RecaptchaAnnotateJob).to receive(:perform_later) do |assessment:|
-          expect(assessment).to be_kind_of(RecaptchaAssessment)
-          expect(assessment.annotation_before_type_cast).to eq(annotation)
-          expect(assessment.annotation_reason_before_type_cast).to eq(reason)
-          expect(assessment).to be_persisted
-        end
-
+      it 'submits annotation' do
         annotate
+
+        expect(WebMock).to have_requested(:post, annotation_url)
       end
 
       it 'logs analytics' do
@@ -80,15 +83,11 @@
         let(:annotation) { nil }
         subject(:annotate) { RecaptchaAnnotator.annotate(assessment_id:, reason:) }
 
-        it 'schedules the annotation to be with only what is provided' do
-          expect(RecaptchaAnnotateJob).to receive(:perform_later) do |assessment:|
-            expect(assessment).to be_kind_of(RecaptchaAssessment)
-            expect(assessment.annotation_before_type_cast).to be_nil
-            expect(assessment.annotation_reason_before_type_cast).to eq(reason)
-            expect(assessment).to be_persisted
-          end
-
+        it 'submits only what is provided' do
           annotate
+
+          expect(WebMock).to have_requested(:post, annotation_url)
+            .with(body: { reasons: [reason] }.to_json)
         end
 
         it 'returns a hash describing annotation' do
@@ -99,6 +98,34 @@
           )
         end
       end
+
+      context 'with nil assessment id' do
+        let(:assessment_id) { nil }
+
+        it 'does not submit annotation' do
+          annotate
+
+          expect(WebMock).not_to have_requested(:post, annotation_url)
+        end
+
+        it { expect(annotate).to be_nil }
+      end
+
+      context 'with connection error' do
+        before do
+          stub_request(:post, annotation_url).to_timeout
+        end
+
+        it 'fails gracefully' do
+          annotate
+        end
+
+        it 'notices the error to NewRelic' do
+          expect(NewRelic::Agent).to receive(:notice_error).with(Faraday::Error)
+
+          annotate
+        end
+      end
     end
   end
 
@@ -126,9 +153,9 @@
         .to_return(headers: { 'Content-Type': 'application/json' }, body: '{}')
     end
 
-    it 'submits annotation and destroys the record' do
-      assessment
-      expect { result }.to change { RecaptchaAssessment.count }.by(-1)
+    it 'submits annotation' do
+      result
+
       expect(WebMock).to have_requested(:post, annotation_url)
     end