From ccb8bcb98f4d0d4830ac2c5cf3a354180251aa36 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 5 Nov 2024 14:25:32 -0500 Subject: [PATCH 1/3] Include FormResponse error details for reCAPTCHA at sign-in changelog: Internal, Analytics, Include FormResponse error details for reCAPTCHA at sign-in --- app/controllers/users/sessions_controller.rb | 14 +++++++------- app/services/analytics_events.rb | 7 +++++-- spec/controllers/users/sessions_controller_spec.rb | 1 + 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 95c4149e587..b77e7351c39 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -39,7 +39,7 @@ def create return process_rate_limited if session_bad_password_count_max_exceeded? return process_locked_out_user if current_user && user_locked_out?(current_user) return process_rate_limited if rate_limited? - return process_failed_captcha unless valid_captcha_result? || log_captcha_failures_only? + return process_failed_captcha unless recaptcha_response.success? || log_captcha_failures_only? rate_limit_password_failure = true self.resource = warden.authenticate!(auth_options) @@ -100,11 +100,10 @@ def locked_out_time_remaining distance_of_time_in_words(Time.zone.now, time_lockout_expires, true) end - def valid_captcha_result? - return @valid_captcha_result if defined?(@valid_captcha_result) - @valid_captcha_result = recaptcha_form.submit( + def recaptcha_response + @recaptcha_response ||= recaptcha_form.submit( recaptcha_token: params.require(:user)[:recaptcha_token], - ).success? + ) end def recaptcha_form @@ -206,15 +205,16 @@ def track_authentication_attempt success = current_user.present? && !user_locked_out?(user) && - (valid_captcha_result? || log_captcha_failures_only?) + (recaptcha_response.success? || log_captcha_failures_only?) analytics.email_and_password_auth( + **recaptcha_response.to_h, success: success, user_id: user.uuid, user_locked_out: user_locked_out?(user), rate_limited: rate_limited?, captcha_validation_performed: captcha_validation_performed?, - valid_captcha_result: valid_captcha_result?, + valid_captcha_result: recaptcha_response.success?, bad_password_count: session[:bad_password_count].to_i, sp_request_url_present: sp_session[:request_url].present?, remember_device: remember_device_cookie.present?, diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 21041899d5a..1baee84fce9 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -436,8 +436,9 @@ def edit_password_visit(required_password_change: false, **extra) ) end - # @param [Boolean] success - # @param [String] user_id + # @param [Boolean] success Whether form validation was successful + # @param [Hash] error_details Details for errors that occurred in unsuccessful submission + # @param [String] user_id Database ID for user associated with attempted email address # @param [Boolean] user_locked_out if the user is currently locked out of their second factor # @param [Boolean] rate_limited Whether the user has exceeded user IP rate limiting # @param [Boolean] valid_captcha_result Whether user passed the reCAPTCHA check or was exempt @@ -459,11 +460,13 @@ def email_and_password_auth( sp_request_url_present:, remember_device:, new_device:, + error_details: nil, **extra ) track_event( 'Email and Password Authentication', success:, + error_details:, user_id:, user_locked_out:, rate_limited:, diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 6a535885546..c0a3f57cf76 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -372,6 +372,7 @@ expect(@analytics).to have_logged_event( 'Email and Password Authentication', success: false, + error_details: { recaptcha_token: { blank: true } }, user_id: user.uuid, user_locked_out: false, rate_limited: false, From 5843e895cc02dd6c01110448595a8335e67fad97 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 5 Nov 2024 14:40:44 -0500 Subject: [PATCH 2/3] Fix user_id described as UUID, not database ID --- app/services/analytics_events.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 1baee84fce9..2a6968c3848 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -438,7 +438,7 @@ def edit_password_visit(required_password_change: false, **extra) # @param [Boolean] success Whether form validation was successful # @param [Hash] error_details Details for errors that occurred in unsuccessful submission - # @param [String] user_id Database ID for user associated with attempted email address + # @param [String] user_id UUID for user associated with attempted email address # @param [Boolean] user_locked_out if the user is currently locked out of their second factor # @param [Boolean] rate_limited Whether the user has exceeded user IP rate limiting # @param [Boolean] valid_captcha_result Whether user passed the reCAPTCHA check or was exempt From 2d11927c2b0e6b7880caa7bc2aefed107a08e0a8 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 5 Nov 2024 14:43:08 -0500 Subject: [PATCH 3/3] Fix grammAr for new_device description --- app/services/analytics_events.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 2a6968c3848..5d6c0b352cd 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -447,7 +447,7 @@ def edit_password_visit(required_password_change: false, **extra) # @param [Boolean] sp_request_url_present if was an SP request URL in the session # @param [Boolean] remember_device if the remember device cookie was present # @param [Boolean, nil] new_device Whether the user is authenticating from a new device. Nil if - # there is the attempt was unsuccessful, since it cannot be known whether it's a new device. + # the attempt was unsuccessful, since it cannot be known whether it's a new device. # Tracks authentication attempts at the email/password screen def email_and_password_auth( success:,