diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 8b8bce52aac..d57fb9ca8f9 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/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) diff --git a/app/services/recaptcha_annotator.rb b/app/services/recaptcha_annotator.rb index bdd8466f41c..8b15f55456d 100644 --- a/app/services/recaptcha_annotator.rb +++ b/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 diff --git a/config/application.yml.default b/config/application.yml.default index f60b1d27667..23e66c2e61f 100644 --- a/config/application.yml.default +++ b/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' diff --git a/db/primary_migrate/20250228141538_add_timestamps_to_recaptcha_assessments.rb b/db/primary_migrate/20250228141538_add_timestamps_to_recaptcha_assessments.rb deleted file mode 100644 index fccd79540e5..00000000000 --- a/db/primary_migrate/20250228141538_add_timestamps_to_recaptcha_assessments.rb +++ /dev/null @@ -1,7 +0,0 @@ -class AddTimestampsToRecaptchaAssessments < ActiveRecord::Migration[8.0] - def 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' - end -end diff --git a/db/schema.rb b/db/schema.rb index f1b6d4e19f8..7838dd11b6a 100644 --- a/db/schema.rb +++ b/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| diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 49e296b8973..81a2a028c04 100644 --- a/lib/identity_config.rb +++ b/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) diff --git a/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb b/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb index 523c0930514..69d7810f705 100644 --- a/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb +++ b/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 diff --git a/spec/services/recaptcha_annotator_spec.rb b/spec/services/recaptcha_annotator_spec.rb index 0cfa5a68793..9d8f1bf0c2b 100644 --- a/spec/services/recaptcha_annotator_spec.rb +++ b/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