Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f69cc71
LG-14216: Implement configurable percent tested reCAPTCHA at sign-in
aduth Aug 26, 2024
e13d5bc
Add A/B testing concern to controller where used
aduth Aug 27, 2024
4ab1e13
Annotate reCAPTCHA result received event
aduth Aug 27, 2024
6a03c7b
Exclude from A/B test where captcha validation not performed
aduth Aug 27, 2024
ed53156
Determine exempt before captcha submission
aduth Aug 27, 2024
dbd5367
Fix error unknown keywords
aduth Aug 27, 2024
b97767e
Add specs for AbTest should_log respond_to include
aduth Aug 27, 2024
4e4ffbe
Add specs for ab_test_bucket user keyword argument
aduth Aug 27, 2024
f95f6df
Update SignInRecaptchaForm specs
aduth Aug 27, 2024
1d6f8cb
Avoid return statement in block
aduth Aug 27, 2024
ae7b7c5
Document captcha_validation_performed
aduth Aug 27, 2024
cbff747
Force reCAPTCHA performed in feature spec
aduth Aug 27, 2024
7b57e3c
Update sessions controller asserted logging expectations
aduth Aug 27, 2024
8bf82c4
Handle nil user, user_session
aduth Aug 27, 2024
cee6579
Memoize user_from_params
aduth Aug 27, 2024
3da3eb1
Use Array#to_set to avoid extra indentation
aduth Aug 27, 2024
f210c96
Reload A/B tests after stubbing bucket configuration
aduth Aug 27, 2024
d8a16e8
Reload A/B tests for enabled reCAPTCHA in sessions controlller
aduth Aug 27, 2024
708408c
Stub controller ab_test_bucket
aduth Aug 27, 2024
5b1e99c
Add specs for AbTests:: RECAPTCHA_SIGN_IN
aduth Aug 29, 2024
c74d4be
Default to always test in development
aduth Aug 30, 2024
4799980
Replace user action events with application-facing
aduth Aug 30, 2024
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
4 changes: 2 additions & 2 deletions app/controllers/concerns/ab_testing_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ module AbTestingConcern
# @param [Symbol] test Name of the test, which should correspond to an A/B test defined in
# # config/initializer/ab_tests.rb.
# @return [Symbol,nil] Bucket to use for the given test, or nil if the test is not active.
def ab_test_bucket(test_name)
def ab_test_bucket(test_name, user: current_user)
test = AbTests.all[test_name]
raise "Unknown A/B test: #{test_name}" unless test

test.bucket(
request:,
service_provider: current_sp&.issuer,
session:,
user: current_user,
user:,
user_session:,
)
end
Expand Down
33 changes: 26 additions & 7 deletions app/controllers/users/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class SessionsController < Devise::SessionsController
include Api::CsrfTokenConcern
include ForcedReauthenticationConcern
include NewDeviceConcern
include AbTestingConcern

rescue_from ActionController::InvalidAuthenticityToken, with: :redirect_to_signin

Expand Down Expand Up @@ -41,7 +42,7 @@ def create
handle_valid_authentication
ensure
handle_invalid_authentication if rate_limit_password_failure && !current_user
track_authentication_attempt(auth_params[:email])
track_authentication_attempt
end

def destroy
Expand Down Expand Up @@ -97,13 +98,24 @@ def locked_out_time_remaining

def valid_captcha_result?
return @valid_captcha_result if defined?(@valid_captcha_result)
@valid_captcha_result = SignInRecaptchaForm.new(**recaptcha_form_args).submit(
email: auth_params[:email],
@valid_captcha_result = recaptcha_form.submit(
recaptcha_token: params.require(:user)[:recaptcha_token],
device_cookie: cookies[:device],
).success?
end

def recaptcha_form
@recaptcha_form ||= SignInRecaptchaForm.new(
email: auth_params[:email],
device_cookie: cookies[:device],
ab_test_bucket: ab_test_bucket(:RECAPTCHA_SIGN_IN, user: user_from_params),
**recaptcha_form_args,
)
end

def captcha_validation_performed?
!recaptcha_form.exempt?
end

def process_failed_captcha
sign_out(:user)
warden.lock!
Expand Down Expand Up @@ -140,6 +152,11 @@ def check_user_needs_redirect
end
end

def user_from_params
return @user_from_params if defined?(@user_from_params)
@user_from_params = User.find_with_email(auth_params[:email])
end

def auth_params
params.require(:user).permit(:email, :password)
end
Expand Down Expand Up @@ -169,21 +186,23 @@ def handle_valid_authentication
user_id: current_user.id,
email: auth_params[:email],
)
user_session[:captcha_validation_performed_at_sign_in] = captcha_validation_performed?
user_session[:platform_authenticator_available] =
params[:platform_authenticator_available] == 'true'
check_password_compromised
redirect_to next_url_after_valid_authentication
end

def track_authentication_attempt(email)
user = User.find_with_email(email) || AnonymousUser.new
def track_authentication_attempt
user = user_from_params || AnonymousUser.new

success = current_user.present? && !user_locked_out?(user) && valid_captcha_result?
analytics.email_and_password_auth(
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?,
bad_password_count: session[:bad_password_count].to_i,
sp_request_url_present: sp_session[:request_url].present?,
Expand All @@ -202,7 +221,7 @@ def rate_limited?

def rate_limiter
return @rate_limiter if defined?(@rate_limiter)
user = User.find_with_email(auth_params[:email])
user = user_from_params
return @rate_limiter = nil unless user
@rate_limiter = RateLimiter.new(
rate_limit_type: :sign_in_user_id_per_ip,
Expand Down
25 changes: 19 additions & 6 deletions app/forms/sign_in_recaptcha_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,37 @@ class SignInRecaptchaForm

RECAPTCHA_ACTION = 'sign_in'

attr_reader :form_class, :form_args, :email, :recaptcha_token, :device_cookie
attr_reader :form_class, :form_args, :email, :recaptcha_token, :device_cookie, :ab_test_bucket

validate :validate_recaptcha_result

def initialize(form_class: RecaptchaForm, **form_args)
def initialize(
email:,
device_cookie:,
ab_test_bucket:,
form_class: RecaptchaForm,
**form_args
)
@email = email
@device_cookie = device_cookie
@ab_test_bucket = ab_test_bucket
@form_class = form_class
@form_args = form_args
end

def submit(email:, recaptcha_token:, device_cookie:)
@email = email
def submit(recaptcha_token:)
@recaptcha_token = recaptcha_token
@device_cookie = device_cookie

success = valid?
FormResponse.new(success:, errors:, serialize_error_details_only: true)
end

def exempt?
IdentityConfig.store.sign_in_recaptcha_score_threshold.zero? ||
ab_test_bucket != :sign_in_recaptcha ||
device.present?
end

private

def validate_recaptcha_result
Expand All @@ -35,7 +48,7 @@ def device
end

def score_threshold
if IdentityConfig.store.sign_in_recaptcha_score_threshold.zero? || device.present?
if exempt?
0.0
else
IdentityConfig.store.sign_in_recaptcha_score_threshold
Expand Down
5 changes: 4 additions & 1 deletion app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,8 @@ def edit_password_visit(required_password_change: false, **extra)
# @param [String] user_id
# @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
# @param [Boolean] valid_captcha_result Whether user passed the reCAPTCHA check or was exempt
# @param [Boolean] captcha_validation_performed Whether a reCAPTCHA check was performed
# @param [String] bad_password_count represents number of prior login failures
# @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
Expand All @@ -443,6 +444,7 @@ def email_and_password_auth(
user_locked_out:,
rate_limited:,
valid_captcha_result:,
captcha_validation_performed:,
bad_password_count:,
sp_request_url_present:,
remember_device:,
Expand All @@ -456,6 +458,7 @@ def email_and_password_auth(
user_locked_out:,
rate_limited:,
valid_captcha_result:,
captcha_validation_performed:,
bad_password_count:,
sp_request_url_present:,
remember_device:,
Expand Down
2 changes: 2 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,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_percent_tested: 0
sign_in_recaptcha_score_threshold: 0.0
sign_in_user_id_per_ip_attempt_window_exponential_factor: 1.1
sign_in_user_id_per_ip_attempt_window_in_minutes: 720
Expand Down Expand Up @@ -428,6 +429,7 @@ development:
secret_key_base: development_secret_key_base
session_encryption_key: 27bad3c25711099429c1afdfd1890910f3b59f5a4faec1c85e945cb8b02b02f261ba501d99cfbb4fab394e0102de6fecf8ffe260f322f610db3e96b2a775c120
show_unsupported_passkey_platform_authentication_setup: 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"]'
socure_webhook_secret_key: 'secret-key'
Expand Down
17 changes: 17 additions & 0 deletions config/initializers/ab_tests.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,21 @@ def self.all
) do |service_provider:, session:, user:, user_session:, **|
document_capture_session_uuid_discriminator(service_provider:, session:, user:, user_session:)
end.freeze

RECAPTCHA_SIGN_IN = AbTest.new(
experiment_name: 'reCAPTCHA at Sign-In',
should_log: [
'Email and Password Authentication',
'IdV: doc auth verify proofing results',
'reCAPTCHA verify result received',
:idv_enter_password_submitted,
].to_set,
buckets: { sign_in_recaptcha: IdentityConfig.store.sign_in_recaptcha_percent_tested },
) do |user:, user_session:, **|
if user_session&.[](:captcha_validation_performed_at_sign_in) == false
nil
else
user&.uuid
end
end.freeze
end
2 changes: 2 additions & 0 deletions lib/ab_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ def bucket(request:, service_provider:, session:, user:, user_session:)
def include_in_analytics_event?(event_name)
if should_log.is_a?(Regexp)
should_log.match?(event_name)
elsif should_log.respond_to?(:include?)
should_log.include?(event_name)
elsif !should_log.nil?
raise 'Unexpected value used for should_log'
else
Expand Down
1 change: 1 addition & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,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_percent_tested, type: :integer)
config.add(:sign_in_recaptcha_score_threshold, type: :float)
config.add(:skip_encryption_allowed_list, type: :json)
config.add(:socure_webhook_enabled, type: :boolean)
Expand Down
64 changes: 60 additions & 4 deletions spec/config/initializers/ab_tests_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
require 'rails_helper'

RSpec.describe AbTests do
include AbTestsHelper

describe '#all' do
it 'returns all registered A/B tests' do
expect(AbTests.all.values).to all(be_kind_of(AbTest))
Expand Down Expand Up @@ -157,10 +159,64 @@
it_behaves_like 'an A/B test that uses document_capture_session_uuid as a discriminator'
end

def reload_ab_tests
AbTests.all.each do |(name, _)|
AbTests.send(:remove_const, name)
describe 'RECAPTCHA_SIGN_IN' do
let(:user) { nil }
let(:user_session) { {} }

subject(:bucket) do
AbTests::RECAPTCHA_SIGN_IN.bucket(
request: nil,
service_provider: nil,
session: nil,
user:,
user_session:,
)
end

context 'when A/B test is disabled' do
before do
allow(IdentityConfig.store).to receive(:sign_in_recaptcha_percent_tested).and_return(0)
reload_ab_tests
end

context 'when it would otherwise assign a bucket' do
let(:user) { build(:user) }

it 'does not return a bucket' do
expect(bucket).to be_nil
end
end
end

context 'when A/B test is enabled' do
before do
allow(IdentityConfig.store).to receive(:sign_in_recaptcha_percent_tested).and_return(100)
reload_ab_tests
end

context 'with no associated user' do
let(:user) { nil }

it 'does not return a bucket' do
expect(bucket).to be_nil
end
end

context 'with an associated user' do
let(:user) { build(:user) }

it 'returns a bucket' do
expect(bucket).not_to be_nil
end

context 'with user session indicating recaptcha was not performed at sign-in' do
let(:user_session) { { captcha_validation_performed_at_sign_in: false } }

it 'does not return a bucket' do
expect(bucket).to be_nil
end
end
end
end
load('config/initializers/ab_tests.rb')
end
end
16 changes: 16 additions & 0 deletions spec/controllers/concerns/ab_testing_concern_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,5 +68,21 @@
end.to raise_error RuntimeError, 'Unknown A/B test: NOT_A_REAL_TEST'
end
end

context 'with user keyword argument' do
let(:other_user) { build(:user) }

it 'returns bucket determined using given user' do
expect(ab_test).to receive(:bucket).with(
user: other_user,
request:,
service_provider: service_provider.issuer,
session:,
user_session:,
).and_call_original

subject.ab_test_bucket(:TEST_TEST, user: other_user)
end
end
end
end
Loading