diff --git a/app/controllers/concerns/idv/ab_test_analytics_concern.rb b/app/controllers/concerns/idv/ab_test_analytics_concern.rb index 714eeb895da..323bee782d1 100644 --- a/app/controllers/concerns/idv/ab_test_analytics_concern.rb +++ b/app/controllers/concerns/idv/ab_test_analytics_concern.rb @@ -6,7 +6,7 @@ module AbTestAnalyticsConcern include OptInHelper def ab_test_analytics_buckets - buckets = { ab_tests: {} } + buckets = {} if defined?(idv_session) buckets[:skip_hybrid_handoff] = idv_session&.skip_hybrid_handoff @@ -19,23 +19,7 @@ def ab_test_analytics_buckets buckets = buckets.merge(lniv_args) end - if defined?(idv_session) - phone_confirmation_session = idv_session.user_phone_confirmation_session || - PhoneConfirmationSession.new( - code: nil, - phone: nil, - sent_at: nil, - delivery_method: :sms, - user: current_user, - ) - buckets[:ab_tests].merge!( - phone_confirmation_session.ab_test_analytics_args, - ) - end - - buckets.merge!(acuant_sdk_ab_test_analytics_args) - buckets.delete(:ab_tests) if buckets[:ab_tests].blank? - buckets + buckets.merge(acuant_sdk_ab_test_analytics_args) end end end diff --git a/app/controllers/concerns/two_factor_authenticatable.rb b/app/controllers/concerns/two_factor_authenticatable.rb index 31ae84c059c..c56d60cf028 100644 --- a/app/controllers/concerns/two_factor_authenticatable.rb +++ b/app/controllers/concerns/two_factor_authenticatable.rb @@ -7,7 +7,8 @@ module TwoFactorAuthenticatable NEED_AUTHENTICATION = 'need_two_factor_authentication' OTP_LENGTH = 6 DIRECT_OTP_LENGTH = 6 - PROOFING_DIRECT_OTP_LENGTH = 6 + PROOFING_SMS_DIRECT_OTP_LENGTH = 6 + PROOFING_VOICE_DIRECT_OTP_LENGTH = 10 ALLOWED_OTP_DRIFT_SECONDS = 30 DIRECT_OTP_VALID_FOR_MINUTES = IdentityConfig.store.otp_valid_for.freeze DIRECT_OTP_VALID_FOR_SECONDS = DIRECT_OTP_VALID_FOR_MINUTES * 60 diff --git a/app/controllers/idv/otp_verification_controller.rb b/app/controllers/idv/otp_verification_controller.rb index 4253e861938..c777c46920a 100644 --- a/app/controllers/idv/otp_verification_controller.rb +++ b/app/controllers/idv/otp_verification_controller.rb @@ -100,16 +100,11 @@ def phone_confirmation_otp_verification_form end def code_length - if ten_digit_otp? - 10 + if idv_session.user_phone_confirmation_session.delivery_method == :voice + TwoFactorAuthenticatable::PROOFING_VOICE_DIRECT_OTP_LENGTH else - TwoFactorAuthenticatable::PROOFING_DIRECT_OTP_LENGTH + TwoFactorAuthenticatable::PROOFING_SMS_DIRECT_OTP_LENGTH end end - - def ten_digit_otp? - AbTests::IDV_TEN_DIGIT_OTP.bucket(current_user.uuid) == :ten_digit_otp && - idv_session.user_phone_confirmation_session.delivery_method == :voice - end end end diff --git a/app/services/idv/phone_confirmation_session.rb b/app/services/idv/phone_confirmation_session.rb index fa269c829a1..8c58074aaf1 100644 --- a/app/services/idv/phone_confirmation_session.rb +++ b/app/services/idv/phone_confirmation_session.rb @@ -4,13 +4,14 @@ module Idv class PhoneConfirmationSession attr_reader :code, :phone, :sent_at, :delivery_method, :user - def self.generate_code(user:, delivery_method:) - bucket = AbTests::IDV_TEN_DIGIT_OTP.bucket(user&.uuid) - if delivery_method == :voice && bucket == :ten_digit_otp - OtpCodeGenerator.generate_digits(10) - else # original, bucket defaults to :six_alphanumeric_otp + def self.generate_code(delivery_method:) + if delivery_method == :voice + OtpCodeGenerator.generate_digits( + TwoFactorAuthenticatable::PROOFING_VOICE_DIRECT_OTP_LENGTH, + ) + else OtpCodeGenerator.generate_alphanumeric_digits( - TwoFactorAuthenticatable::PROOFING_DIRECT_OTP_LENGTH, + TwoFactorAuthenticatable::PROOFING_SMS_DIRECT_OTP_LENGTH, ) end end @@ -25,7 +26,7 @@ def initialize(code:, phone:, sent_at:, delivery_method:, user:) def self.start(phone:, delivery_method:, user:) new( - code: generate_code(user: user, delivery_method: delivery_method), + code: generate_code(delivery_method: delivery_method), phone: phone, sent_at: Time.zone.now, delivery_method: delivery_method, @@ -33,19 +34,9 @@ def self.start(phone:, delivery_method:, user:) ) end - def ab_test_analytics_args - return {} unless IdentityConfig.store.ab_testing_idv_ten_digit_otp_enabled - - { - AbTests::IDV_TEN_DIGIT_OTP.experiment_name => { - bucket: AbTests::IDV_TEN_DIGIT_OTP.bucket(user.uuid), - }, - } - end - def regenerate_otp self.class.new( - code: self.class.generate_code(user: user, delivery_method: delivery_method), + code: self.class.generate_code(delivery_method: delivery_method), phone: phone, sent_at: Time.zone.now, delivery_method: delivery_method, diff --git a/app/services/idv/send_phone_confirmation_otp.rb b/app/services/idv/send_phone_confirmation_otp.rb index 1770541eaba..191cf73cacb 100644 --- a/app/services/idv/send_phone_confirmation_otp.rb +++ b/app/services/idv/send_phone_confirmation_otp.rb @@ -61,6 +61,13 @@ def otp_rate_limiter end def send_otp + length, format = case delivery_method + when :voice + ['ten', 'digit'] + else + ['six', 'character'] + end + idv_session.user_phone_confirmation_session = user_phone_confirmation_session.regenerate_otp @telephony_response = Telephony.send_confirmation_otp( otp: code, @@ -80,24 +87,6 @@ def send_otp otp_sent_response end - def bucket - @bucket ||= AbTests::IDV_TEN_DIGIT_OTP.bucket( - idv_session.user_phone_confirmation_session.user.uuid, - ) - end - - def format - return 'digit' if delivery_method == :voice && bucket == :ten_digit_otp - - 'character' - end - - def length - return 'ten' if delivery_method == :voice && bucket == :ten_digit_otp - - 'six' - end - def otp_sent_response FormResponse.new( success: telephony_response.success?, extra: extra_analytics_attributes, @@ -105,7 +94,7 @@ def otp_sent_response end def extra_analytics_attributes - attributes = { + { otp_delivery_preference: delivery_method, country_code: parsed_phone.country, area_code: parsed_phone.area_code, @@ -113,15 +102,6 @@ def extra_analytics_attributes rate_limit_exceeded: rate_limit_exceeded?, telephony_response: @telephony_response, } - if IdentityConfig.store.ab_testing_idv_ten_digit_otp_enabled - attributes[:ab_tests] = { - AbTests::IDV_TEN_DIGIT_OTP.experiment_name => { - bucket: bucket, - }, - } - end - - attributes end def parsed_phone diff --git a/config/application.yml.default b/config/application.yml.default index c5bea42e9fa..17748448a87 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -20,8 +20,6 @@ aamva_cert_enabled: true aamva_supported_jurisdictions: '["AL","AR","AZ","CO","CT","DC","DE","FL","GA","HI","IA","ID","IL","IN","KS","KY","MA","MD","ME","MI","MO","MS","MT","NC","ND","NE","NJ","NM","NV","OH","OR","PA","RI","SC","SD","TN","TX","VA","VT","WA","WI","WV","WY"]' aamva_verification_request_timeout: 5.0 aamva_verification_url: https://example.org:12345/verification/url -ab_testing_idv_ten_digit_otp_enabled: false -ab_testing_idv_ten_digit_otp_percent: 0 all_redirect_uris_cache_duration_minutes: 2 allowed_ialmax_providers: '[]' allowed_verified_within_providers: '[]' diff --git a/config/initializers/ab_tests.rb b/config/initializers/ab_tests.rb index feb9607c48c..096647a52c4 100644 --- a/config/initializers/ab_tests.rb +++ b/config/initializers/ab_tests.rb @@ -30,15 +30,4 @@ module AbTests 0, }, ).freeze - - IDV_TEN_DIGIT_OTP = AbTestBucket.new( - experiment_name: 'idv_ten_digit_otp', - default_bucket: :six_alphanumeric_otp, - buckets: { - ten_digit_otp: - IdentityConfig.store.ab_testing_idv_ten_digit_otp_enabled ? - IdentityConfig.store.ab_testing_idv_ten_digit_otp_percent : - 0, - }, - ).freeze end diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 6a9cdfa5281..3e9900653e6 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -35,8 +35,6 @@ def self.store config.add(:aamva_supported_jurisdictions, type: :json) config.add(:aamva_verification_request_timeout, type: :float) config.add(:aamva_verification_url) - config.add(:ab_testing_idv_ten_digit_otp_enabled, type: :boolean) - config.add(:ab_testing_idv_ten_digit_otp_percent, type: :integer) config.add(:account_reset_token_valid_for_days, type: :integer) config.add(:account_reset_wait_period_days, type: :integer) config.add(:account_reset_fraud_user_wait_period_days, type: :integer, allow_nil: true) diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 067fcd2d695..5fc618b23fd 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -595,38 +595,6 @@ def index ) end - it 'sends a 6-digit OTP when the idv_ten_digit_otp A/B test is in progress' do - stub_const( - 'AbTests::IDV_TEN_DIGIT_OTP', - FakeAbTestBucket.new.tap { |ab| ab.assign(@user.uuid => :ten_digit_otp) }, - ) - - sign_in_before_2fa(@user) - subject.user_session[:context] = 'confirmation' - subject.user_session[:unconfirmed_phone] = @unconfirmed_phone - parsed_phone = Phonelib.parse(@unconfirmed_phone) - - allow(Telephony).to receive(:send_confirmation_otp).and_call_original - - get :send_code, params: otp_delivery_form_sms - - expect(Telephony).to have_received(:send_confirmation_otp).with( - otp: subject.current_user.direct_otp, - to: @unconfirmed_phone, - expiration: 10, - channel: :sms, - otp_format: 'digit', - otp_length: '6', - domain: IdentityConfig.store.domain_name, - country_code: 'US', - extra_metadata: { - area_code: parsed_phone.area_code, - phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), - resend: nil, - }, - ) - end - it 'tracks the enrollment attempt event' do sign_in_before_2fa(@user) subject.user_session[:context] = 'confirmation' diff --git a/spec/services/idv/phone_confirmation_session_spec.rb b/spec/services/idv/phone_confirmation_session_spec.rb index 3b883084853..66aa09b2b2c 100644 --- a/spec/services/idv/phone_confirmation_session_spec.rb +++ b/spec/services/idv/phone_confirmation_session_spec.rb @@ -38,63 +38,16 @@ end describe '.generate_code' do - let(:ab_test_enabled) { false } - before do - allow(IdentityConfig.store).to receive(:ab_testing_idv_ten_digit_otp_enabled). - and_return(ab_test_enabled) - end - - context 'A/B test not enabled' do - it 'generates a six-character alphanumeric code' do - code = described_class.generate_code(user: user, delivery_method: :voice) + it 'generates a six-character alphanumeric code for sms' do + code = described_class.generate_code(delivery_method: :sms) - expect(code).to match(six_char_alphanumeric) - end + expect(code).to match(six_char_alphanumeric) end - context '10-digit A/B test enabled' do - let(:ab_test_enabled) { true } - - context '10-digit A/B test puts user in :six_alphanumeric_otp bucket' do - before do - stub_const( - 'AbTests::IDV_TEN_DIGIT_OTP', - FakeAbTestBucket.new.tap { |ab| ab.assign(user.uuid => :six_alphanumeric_otp) }, - ) - end - it 'generates a six-character alphanumeric code for sms' do - code = described_class.generate_code(user: user, delivery_method: :sms) + it 'generates a ten-digit numeric code for voice' do + code = described_class.generate_code(delivery_method: :voice) - expect(code).to match(six_char_alphanumeric) - end - - it 'generates a six-character alphanumeric code for voice' do - code = described_class.generate_code(user: user, delivery_method: :voice) - - expect(code).to match(six_char_alphanumeric) - end - end - - context '10-digit A/B test puts user in :ten_digit_otp bucket' do - before do - stub_const( - 'AbTests::IDV_TEN_DIGIT_OTP', - FakeAbTestBucket.new.tap { |ab| ab.assign(user.uuid => :ten_digit_otp) }, - ) - end - - it 'generates a six-character alphanumeric code for sms' do - code = described_class.generate_code(user: user, delivery_method: :sms) - - expect(code).to match(six_char_alphanumeric) - end - - it 'generates a ten-digit numeric code for voice' do - code = described_class.generate_code(user: user, delivery_method: :voice) - - expect(code).to match(ten_digit_numeric) - end - end + expect(code).to match(ten_digit_numeric) end end diff --git a/spec/services/idv/send_phone_confirmation_otp_spec.rb b/spec/services/idv/send_phone_confirmation_otp_spec.rb index 78d10f72e1e..72bb15575fa 100644 --- a/spec/services/idv/send_phone_confirmation_otp_spec.rb +++ b/spec/services/idv/send_phone_confirmation_otp_spec.rb @@ -94,8 +94,8 @@ to: phone, expiration: 10, channel: :voice, - otp_format: 'character', - otp_length: '6', + otp_format: 'digit', + otp_length: '10', domain: IdentityConfig.store.domain_name, country_code: 'US', extra_metadata: {