Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions app/controllers/users/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 if !valid_captcha_result?
return process_failed_captcha unless valid_captcha_result? || log_captcha_failures_only?

rate_limit_password_failure = true
self.resource = warden.authenticate!(auth_options)
Expand Down Expand Up @@ -204,7 +204,10 @@ def handle_valid_authentication
def track_authentication_attempt
user = user_from_params || AnonymousUser.new

success = current_user.present? && !user_locked_out?(user) && valid_captcha_result?
success = current_user.present? &&
!user_locked_out?(user) &&
(valid_captcha_result? || log_captcha_failures_only?)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was curious if we could bake the log-only consideration into valid_captcha_result?, but I see that it's valuable to include the raw result in the logging value below for valid_captcha_result. Nice separation 👍


analytics.email_and_password_auth(
success: success,
user_id: user.uuid,
Expand Down Expand Up @@ -308,6 +311,10 @@ def randomize_check_password?
SecureRandom.random_number(IdentityConfig.store.compromised_password_randomizer_value) >=
IdentityConfig.store.compromised_password_randomizer_threshold
end

def log_captcha_failures_only?
IdentityConfig.store.sign_in_recaptcha_log_failures_only
end
end

def unsafe_redirect_error(_exception)
Expand Down
1 change: 1 addition & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,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_log_failures_only: false
sign_in_recaptcha_percent_tested: 0
sign_in_recaptcha_score_threshold: 0.0
sign_in_user_id_per_ip_attempt_window_exponential_factor: 1.1
Expand Down
1 change: 1 addition & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,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_log_failures_only, type: :boolean)
config.add(:sign_in_recaptcha_percent_tested, type: :integer)
config.add(:sign_in_recaptcha_score_threshold, type: :float)
config.add(:skip_encryption_allowed_list, type: :json)
Expand Down
62 changes: 42 additions & 20 deletions spec/controllers/users/sessions_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -341,33 +341,55 @@
and_return(:sign_in_recaptcha)
end

it 'tracks unsuccessful authentication for failed reCAPTCHA' do
user = create(:user, :fully_registered)
context 'when configured to log failures only' do
before do
allow(IdentityConfig.store).to receive(:sign_in_recaptcha_log_failures_only).
and_return(true)
end

stub_analytics
it 'redirects unsuccessful authentication for failed reCAPTCHA to failed page' do
user = create(:user, :fully_registered)

post :create, params: { user: { email: user.email, password: user.password, score: 0.1 } }
post :create, params: { user: { email: user.email, password: user.password, score: 0.1 } }

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: false,
user_id: user.uuid,
user_locked_out: false,
rate_limited: false,
valid_captcha_result: false,
captcha_validation_performed: true,
bad_password_count: 0,
remember_device: false,
sp_request_url_present: false,
)
expect(response).to redirect_to user_two_factor_authentication_url
end
end

it 'redirects unsuccessful authentication for failed reCAPTCHA to failed page' do
user = create(:user, :fully_registered)
context 'when not configured to log failures only' do
before do
allow(IdentityConfig.store).to receive(:sign_in_recaptcha_log_failures_only).
and_return(false)
end

post :create, params: { user: { email: user.email, password: user.password, score: 0.1 } }
it 'tracks unsuccessful authentication for failed reCAPTCHA' do
user = create(:user, :fully_registered)

expect(response).to redirect_to sign_in_security_check_failed_url
stub_analytics

post :create, params: { user: { email: user.email, password: user.password, score: 0.1 } }

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: false,
user_id: user.uuid,
user_locked_out: false,
rate_limited: false,
valid_captcha_result: false,
captcha_validation_performed: true,
bad_password_count: 0,
remember_device: false,
sp_request_url_present: false,
)
end

it 'redirects unsuccessful authentication for failed reCAPTCHA to failed page' do
user = create(:user, :fully_registered)

post :create, params: { user: { email: user.email, password: user.password, score: 0.1 } }

expect(response).to redirect_to sign_in_security_check_failed_url
end
end
end

Expand Down
46 changes: 12 additions & 34 deletions spec/features/users/sign_in_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -879,12 +879,14 @@
end
end

context 'reCAPTCHA check fails' do
context 'when the reCAPTCHA check fails' do
let(:user) { create(:user, :fully_registered) }

before do
allow(FeatureManagement).to receive(:sign_in_recaptcha_enabled?).and_return(true)
allow(IdentityConfig.store).to receive(:recaptcha_mock_validator).and_return(true)
allow(IdentityConfig.store).to receive(:sign_in_recaptcha_log_failures_only).
and_return(sign_in_recaptcha_log_failures_only)
allow(IdentityConfig.store).to receive(:sign_in_recaptcha_score_threshold).and_return(0.2)
allow(IdentityConfig.store).to receive(:sign_in_recaptcha_percent_tested).and_return(100)
reload_ab_tests
Expand All @@ -894,40 +896,16 @@
reload_ab_tests
end

it 'redirects user to security check failed page' do
visit new_user_session_path

asserted_expected_user = false
fake_analytics = FakeAnalytics.new
allow_any_instance_of(ApplicationController).to receive(:analytics).
and_wrap_original do |original|
original_analytics = original.call
if original_analytics.request.params[:controller] == 'users/sessions' &&
original_analytics.request.params[:action] == 'create'
expect(original_analytics.user).to eq(user)
asserted_expected_user = true
end

fake_analytics
end
context 'when configured to log failures only' do
let(:sign_in_recaptcha_log_failures_only) { true }
it_behaves_like 'logs reCAPTCHA event and redirects appropriately',
successful_sign_in: true
end

fill_in :user_recaptcha_mock_score, with: '0.1'
fill_in_credentials_and_submit(user.email, user.password)
expect(asserted_expected_user).to eq(true)
expect(fake_analytics).to have_logged_event(
'reCAPTCHA verify result received',
recaptcha_result: {
assessment_id: kind_of(String),
success: true,
score: 0.1,
errors: [],
reasons: [],
},
evaluated_as_valid: false,
score_threshold: 0.2,
form_class: 'RecaptchaMockForm',
)
expect(current_path).to eq sign_in_security_check_failed_path
context 'when not configured to log failures only' do
let(:sign_in_recaptcha_log_failures_only) { false }
it_behaves_like 'logs reCAPTCHA event and redirects appropriately',
successful_sign_in: false
end
end

Expand Down
50 changes: 50 additions & 0 deletions spec/support/shared_examples/sign_in.rb
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,56 @@ def user_with_broken_personal_key(scenario)
end
end

RSpec.shared_examples 'logs reCAPTCHA event and redirects appropriately' do |successful_sign_in:|
it 'logs reCAPTCHA event and redirects to the correct location' do
visit new_user_session_path

asserted_expected_user = false
fake_analytics = FakeAnalytics.new
allow_any_instance_of(ApplicationController).to receive(:analytics).
and_wrap_original do |original|
original_analytics = original.call
if original_analytics.request.params[:controller] == 'users/sessions' &&
original_analytics.request.params[:action] == 'create'
expect(original_analytics.user).to eq(user)
asserted_expected_user = true
end

fake_analytics
end

fill_in :user_recaptcha_mock_score, with: '0.1'
fill_in_credentials_and_submit(user.email, user.password)
expect(asserted_expected_user).to eq(true)
expect(fake_analytics).to have_logged_event(
'reCAPTCHA verify result received',
recaptcha_result: {
assessment_id: kind_of(String),
success: true,
score: 0.1,
errors: [],
reasons: [],
},
evaluated_as_valid: false,
score_threshold: 0.2,
form_class: 'RecaptchaMockForm',
)
expect(fake_analytics).to have_logged_event(
'Email and Password Authentication',
hash_including(
success: successful_sign_in,
valid_captcha_result: false,
captcha_validation_performed: true,
),
)
if successful_sign_in
expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms')
else
expect(current_path).to eq sign_in_security_check_failed_path
end
end
end

def ial1_sign_in_with_personal_key_goes_to_sp(sp)
user = create_ial1_account_go_back_to_sp_and_sign_out(sp)
old_personal_key = PersonalKeyGenerator.new(user).generate!
Expand Down