diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index d57fb9ca8f9..8b8bce52aac 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -29,24 +29,18 @@ 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) if sign_in_recaptcha_annotation_enabled? + session.delete(:sign_in_recaptcha_assessment_id) else handle_invalid_verification_for_authentication_context end end def annotate_recaptcha(reason) - if sign_in_recaptcha_annotation_enabled? - RecaptchaAnnotator.annotate(assessment_id: session[:sign_in_recaptcha_assessment_id], reason:) - end + RecaptchaAnnotator.annotate(assessment_id: session[:sign_in_recaptcha_assessment_id], reason:) 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 8b15f55456d..bdd8466f41c 100644 --- a/app/services/recaptcha_annotator.rb +++ b/app/services/recaptcha_annotator.rb @@ -21,10 +21,8 @@ def annotate(assessment_id:, reason: nil, annotation: nil) return if assessment_id.blank? if FeatureManagement.recaptcha_enterprise? - submit_annotation(assessment_id:, reason:, annotation:) - # Future: - # assessment = create_or_update_assessment!(assessment_id:, reason:, annotation:) - # RecaptchaAnnotateJob.perform_later(assessment:) + assessment = create_or_update_assessment!(assessment_id:, reason:, annotation:) + RecaptchaAnnotateJob.perform_later(assessment:) end { assessment_id:, reason:, annotation: } @@ -36,6 +34,7 @@ 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 40b68934712..241550d16af 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -390,7 +390,6 @@ 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 @@ -508,7 +507,6 @@ 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"]' @@ -609,7 +607,6 @@ 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 new file mode 100644 index 00000000000..fccd79540e5 --- /dev/null +++ b/db/primary_migrate/20250228141538_add_timestamps_to_recaptcha_assessments.rb @@ -0,0 +1,7 @@ +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 7838dd11b6a..f1b6d4e19f8 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_24_134110) do +ActiveRecord::Schema[8.0].define(version: 2025_02_28_141538) do # These are extensions that must be enabled in order to support this database enable_extension "citext" enable_extension "pg_catalog.plpgsql" @@ -482,6 +482,8 @@ 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 b1b58cceb31..e523f717896 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -428,7 +428,6 @@ 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 69d7810f705..523c0930514 100644 --- a/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb +++ b/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb @@ -168,53 +168,28 @@ context 'when there is a sign_in_recaptcha_assessment_id in the session' do let(:assessment_id) { 'projects/project-id/assessments/assessment-id' } - 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 PASSED_TWO_FACTOR and clears assessment id from session' do + recaptcha_annotation = { + assessment_id:, + reason: RecaptchaAnnotator::AnnotationReasons::PASSED_TWO_FACTOR, + } - it 'annotates assessment with PASSED_TWO_FACTOR and clears assessment id from session' do - recaptcha_annotation = { - assessment_id:, - reason: RecaptchaAnnotator::AnnotationReasons::PASSED_TWO_FACTOR, - } + controller.session[:sign_in_recaptcha_assessment_id] = assessment_id - controller.session[:sign_in_recaptcha_assessment_id] = assessment_id + expect(RecaptchaAnnotator).to receive(:annotate) + .with(**recaptcha_annotation) + .and_return(recaptcha_annotation) - expect(RecaptchaAnnotator).to receive(:annotate) - .with(**recaptcha_annotation) - .and_return(recaptcha_annotation) + stub_analytics - stub_analytics + expect { result } + .to change { controller.session[:sign_in_recaptcha_assessment_id] } + .from(assessment_id).to(nil) - 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 + expect(@analytics).to have_logged_event( + 'Multi-Factor Authentication', + hash_including(recaptcha_annotation:), + ) end end end @@ -253,52 +228,28 @@ context 'when there is a sign_in_recaptcha_assessment_id in the session' do let(:assessment_id) { 'projects/project-id/assessments/assessment-id' } - 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 + it 'annotates assessment with FAILED_TWO_FACTOR and clears assessment id from session' do + recaptcha_annotation = { + assessment_id:, + reason: RecaptchaAnnotator::AnnotationReasons::FAILED_TWO_FACTOR, + } - expect { result } - .not_to change { controller.session[:sign_in_recaptcha_assessment_id] } - .from(assessment_id) + controller.session[:sign_in_recaptcha_assessment_id] = assessment_id - 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).to receive(:annotate) + .with(**recaptcha_annotation) + .and_return(recaptcha_annotation) - expect(RecaptchaAnnotator).not_to receive(:annotate) + stub_analytics - stub_analytics + expect { result } + .not_to change { controller.session[:sign_in_recaptcha_assessment_id] } + .from(assessment_id) - expect { result } - .not_to change { controller.session[:sign_in_recaptcha_assessment_id] } - end + expect(@analytics).to have_logged_event( + 'Multi-Factor Authentication', + hash_including(recaptcha_annotation:), + ) end end end diff --git a/spec/services/recaptcha_annotator_spec.rb b/spec/services/recaptcha_annotator_spec.rb index 9d8f1bf0c2b..0cfa5a68793 100644 --- a/spec/services/recaptcha_annotator_spec.rb +++ b/spec/services/recaptcha_annotator_spec.rb @@ -52,21 +52,18 @@ .and_return(recaptcha_enterprise_project_id) allow(IdentityConfig.store).to receive(:recaptcha_enterprise_api_key) .and_return(recaptcha_enterprise_api_key) - 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: '{}') + allow(RecaptchaAnnotateJob).to receive(:perform_later) end - it 'submits annotation' do - annotate + 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 - expect(WebMock).to have_requested(:post, annotation_url) + annotate end it 'logs analytics' do @@ -83,11 +80,15 @@ let(:annotation) { nil } subject(:annotate) { RecaptchaAnnotator.annotate(assessment_id:, reason:) } - it 'submits only what is provided' do - annotate + 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 - expect(WebMock).to have_requested(:post, annotation_url) - .with(body: { reasons: [reason] }.to_json) + annotate end it 'returns a hash describing annotation' do @@ -98,34 +99,6 @@ ) 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 @@ -153,9 +126,9 @@ .to_return(headers: { 'Content-Type': 'application/json' }, body: '{}') end - it 'submits annotation' do - result - + it 'submits annotation and destroys the record' do + assessment + expect { result }.to change { RecaptchaAssessment.count }.by(-1) expect(WebMock).to have_requested(:post, annotation_url) end