From ae110c50213523ba522f953ac238233bd52cd316 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 8 Jul 2022 09:36:34 -0500 Subject: [PATCH 1/6] Add separate proofing OTP length constant --- app/controllers/concerns/two_factor_authenticatable.rb | 1 + app/decorators/user_decorator.rb | 2 +- app/services/db/auth_app_configuration.rb | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable.rb b/app/controllers/concerns/two_factor_authenticatable.rb index 5f1924d0a91..d705b0bc00b 100644 --- a/app/controllers/concerns/two_factor_authenticatable.rb +++ b/app/controllers/concerns/two_factor_authenticatable.rb @@ -5,6 +5,7 @@ module TwoFactorAuthenticatable NEED_AUTHENTICATION = 'need_two_factor_authentication'.freeze OTP_LENGTH = 6 DIRECT_OTP_LENGTH = 6 + PROOFING_DIRECT_OTP_LENGTH = 6 ALLOWED_OTP_DRIFT_SECONDS = 30 DIRECT_OTP_VALID_FOR_MINUTES = IdentityConfig.store.otp_valid_for DIRECT_OTP_VALID_FOR_SECONDS = DIRECT_OTP_VALID_FOR_MINUTES * 60 diff --git a/app/decorators/user_decorator.rb b/app/decorators/user_decorator.rb index bc127eee8ad..01c9eb080a1 100644 --- a/app/decorators/user_decorator.rb +++ b/app/decorators/user_decorator.rb @@ -81,7 +81,7 @@ def qrcode(otp_secret_key) options = { issuer: 'Login.gov', otp_secret_key: otp_secret_key, - digits: TwoFactorAuthenticatable::DIRECT_OTP_LENGTH, + digits: TwoFactorAuthenticatable::OTP_LENGTH, interval: IdentityConfig.store.totp_code_interval, } url = ROTP::TOTP.new(otp_secret_key, options).provisioning_uri(email) diff --git a/app/services/db/auth_app_configuration.rb b/app/services/db/auth_app_configuration.rb index b2d9f5c3170..5b43f2e2ea8 100644 --- a/app/services/db/auth_app_configuration.rb +++ b/app/services/db/auth_app_configuration.rb @@ -30,7 +30,7 @@ def self.authenticate(user, code) def self.confirm(secret, code) totp = ROTP::TOTP.new( secret, - digits: TwoFactorAuthenticatable::DIRECT_OTP_LENGTH, + digits: TwoFactorAuthenticatable::OTP_LENGTH, interval: IdentityConfig.store.totp_code_interval, ) totp.verify( From 89c174d38c5b407604e4b0d45a5a096a5f2ee4ae Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 8 Jul 2022 09:38:25 -0500 Subject: [PATCH 2/6] Add alphanumeric digit generation method and configuration --- app/forms/otp_verification_form.rb | 6 +++- app/models/concerns/user_otp_methods.rb | 9 +++++- .../phone_confirmation/code_generator.rb | 4 ++- config/application.yml.default | 2 ++ lib/identity_config.rb | 1 + lib/otp_code_generator.rb | 4 +++ spec/forms/otp_verification_form_spec.rb | 28 ++++++++++++++++++- spec/lib/otp_code_generator_spec.rb | 18 ++++++++++-- .../confirmaton_session_spec.rb | 4 ++- 9 files changed, 68 insertions(+), 8 deletions(-) diff --git a/app/forms/otp_verification_form.rb b/app/forms/otp_verification_form.rb index 1b40719e5ba..88d37ea3012 100644 --- a/app/forms/otp_verification_form.rb +++ b/app/forms/otp_verification_form.rb @@ -21,7 +21,11 @@ def valid_direct_otp_code? end def pattern_matching_otp_code_format - /\A[a-z0-9]{#{otp_code_length}}\z/i + if IdentityConfig.store.enable_numeric_authentication_otp + /\A[0-9]{#{otp_code_length}}\z/i + else + /\A[a-z0-9]{#{otp_code_length}}\z/i + end end def otp_code_length diff --git a/app/models/concerns/user_otp_methods.rb b/app/models/concerns/user_otp_methods.rb index f8b090c190c..5d888cefcdb 100644 --- a/app/models/concerns/user_otp_methods.rb +++ b/app/models/concerns/user_otp_methods.rb @@ -8,8 +8,15 @@ def max_login_attempts? end def create_direct_otp + otp = + if IdentityConfig.store.enable_numeric_authentication_otp + OtpCodeGenerator.generate_digits(TwoFactorAuthenticatable::DIRECT_OTP_LENGTH) + else + OtpCodeGenerator.generate_alphanumeric_digits(TwoFactorAuthenticatable::DIRECT_OTP_LENGTH) + end + update( - direct_otp: OtpCodeGenerator.generate_digits(TwoFactorAuthenticatable::DIRECT_OTP_LENGTH), + direct_otp: otp, direct_otp_sent_at: Time.zone.now, ) end diff --git a/app/services/phone_confirmation/code_generator.rb b/app/services/phone_confirmation/code_generator.rb index d6acb3a025a..c356c695954 100644 --- a/app/services/phone_confirmation/code_generator.rb +++ b/app/services/phone_confirmation/code_generator.rb @@ -3,7 +3,9 @@ module PhoneConfirmation class CodeGenerator def self.call - OtpCodeGenerator.generate_digits(TwoFactorAuthenticatable::DIRECT_OTP_LENGTH) + OtpCodeGenerator.generate_alphanumeric_digits( + TwoFactorAuthenticatable::PROOFING_DIRECT_OTP_LENGTH, + ) end end end diff --git a/config/application.yml.default b/config/application.yml.default index 82717a47e65..21a73e1928b 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -82,6 +82,7 @@ doc_capture_request_valid_for_minutes: 15 email_from: no-reply@login.gov email_from_display_name: Login.gov enable_load_testing_mode: false +enable_numeric_authentication_otp: true enable_partner_api: false enable_rate_limiting: true enable_test_routes: true @@ -367,6 +368,7 @@ production: doc_auth_vendor_randomize_percent: 0 doc_auth_vendor_randomize_alternate_vendor: '' domain_name: login.gov + enable_numeric_authentication_otp: false enable_test_routes: false enable_usps_verification: false hmac_fingerprinter_key: diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 4e98fef1dc6..fec9a1ece1e 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -154,6 +154,7 @@ def self.build_store(config_map) config.add(:email_from, type: :string) config.add(:email_from_display_name, type: :string) config.add(:enable_load_testing_mode, type: :boolean) + config.add(:enable_numeric_authentication_otp, type: :boolean) config.add(:enable_partner_api, type: :boolean) config.add(:enable_rate_limiting, type: :boolean) config.add(:enable_test_routes, type: :boolean) diff --git a/lib/otp_code_generator.rb b/lib/otp_code_generator.rb index 749a04c2b28..4c801d63eb6 100644 --- a/lib/otp_code_generator.rb +++ b/lib/otp_code_generator.rb @@ -2,6 +2,10 @@ class OtpCodeGenerator def self.generate_digits(digits) + SecureRandom.random_number(10 ** digits).to_s.rjust(digits, '0') + end + + def self.generate_alphanumeric_digits(digits) ProfanityDetector.without_profanity do # 5 bits per character means we must multiply what we want by 5 # :length adds zero padding in case it's a smaller number diff --git a/spec/forms/otp_verification_form_spec.rb b/spec/forms/otp_verification_form_spec.rb index 4ea05acc98c..ae9419fa899 100644 --- a/spec/forms/otp_verification_form_spec.rb +++ b/spec/forms/otp_verification_form_spec.rb @@ -32,8 +32,9 @@ end end - context 'when the format of the code is not exactly 6 characters' do + context 'when alphanumeric is enabled and the code is not exactly 6 characters' do it 'returns FormResponse with success: false' do + allow(IdentityConfig.store).to receive(:enable_numeric_authentication_otp).and_return(false) user = build_stubbed(:user) invalid_codes = [ '123abcd', @@ -56,5 +57,30 @@ end end end + + context 'when numeric is enabled and the code is not exactly 6 digits' do + it 'returns FormResponse with success: false' do + allow(IdentityConfig.store).to receive(:enable_numeric_authentication_otp).and_return(true) + user = build_stubbed(:user) + invalid_codes = [ + 'abcdef', + '12345a', + "aaaaa\n123456\naaaaaaaaa", + ] + + invalid_codes.each do |code| + form = OtpVerificationForm.new(user, code) + allow(user).to receive(:authenticate_direct_otp).with(code).and_return(true) + + result = FormResponse.new( + success: false, + errors: {}, + extra: { multi_factor_auth_method: 'otp_code' }, + ) + + expect(form.submit).to eq(result), "expected #{code.inspect} to not pass" + end + end + end end end diff --git a/spec/lib/otp_code_generator_spec.rb b/spec/lib/otp_code_generator_spec.rb index 3b02012a0b3..02a5129a50b 100644 --- a/spec/lib/otp_code_generator_spec.rb +++ b/spec/lib/otp_code_generator_spec.rb @@ -6,8 +6,20 @@ OtpCodeGenerator.generate_digits(TwoFactorAuthenticatable::DIRECT_OTP_LENGTH) end + it 'pads short strings with zeroes' do + expect(SecureRandom).to receive(:random_number).and_return(0) + + expect(generate_digits).to eq('0' * TwoFactorAuthenticatable::DIRECT_OTP_LENGTH) + end + end + + describe '.generate_alphanumeric_digits' do + subject(:generate_alphanumeric_digits) do + OtpCodeGenerator.generate_alphanumeric_digits(TwoFactorAuthenticatable::DIRECT_OTP_LENGTH) + end + it 'generates crockford-32 encoded words' do - expect(generate_digits). + expect(generate_alphanumeric_digits). to match(/\A[a-z0-9]{#{TwoFactorAuthenticatable::DIRECT_OTP_LENGTH}}\Z/io) end @@ -19,13 +31,13 @@ Base32::Crockford.decode('ABCDE'), ) - expect(generate_digits).to eq('0ABCDE') + expect(generate_alphanumeric_digits).to eq('0ABCDE') end it 'pads short strings with zeroes' do expect(SecureRandom).to receive(:random_number).and_return(0) - expect(generate_digits).to eq('0' * TwoFactorAuthenticatable::DIRECT_OTP_LENGTH) + expect(generate_alphanumeric_digits).to eq('0' * TwoFactorAuthenticatable::DIRECT_OTP_LENGTH) end end end diff --git a/spec/services/phone_confirmation/confirmaton_session_spec.rb b/spec/services/phone_confirmation/confirmaton_session_spec.rb index 82d10a9d42e..ae417c3e428 100644 --- a/spec/services/phone_confirmation/confirmaton_session_spec.rb +++ b/spec/services/phone_confirmation/confirmaton_session_spec.rb @@ -48,7 +48,9 @@ end describe '#matches_code?' do - let(:code) { OtpCodeGenerator.generate_digits(TwoFactorAuthenticatable::DIRECT_OTP_LENGTH) } + let(:code) do + OtpCodeGenerator.generate_alphanumeric_digits(TwoFactorAuthenticatable::DIRECT_OTP_LENGTH) + end subject do described_class.new( code: code, From 3e470ffa90f5ade54ee520926bb04d6b2d217afd Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 8 Jul 2022 09:38:42 -0500 Subject: [PATCH 3/6] allow configuring maxlength for otp input --- app/views/idv/otp_verification/show.html.erb | 1 + .../shared/_one_time_code_input.html.erb | 6 ++++-- .../otp_verification/show.html.erb | 3 ++- .../totp_verification/show.html.erb | 1 + app/views/users/totp_setup/new.html.erb | 1 + .../_one_time_code_input.html.erb_spec.rb | 21 +++++++++++++++++++ 6 files changed, 30 insertions(+), 3 deletions(-) diff --git a/app/views/idv/otp_verification/show.html.erb b/app/views/idv/otp_verification/show.html.erb index 1e5393f2253..498f4e670f7 100644 --- a/app/views/idv/otp_verification/show.html.erb +++ b/app/views/idv/otp_verification/show.html.erb @@ -29,6 +29,7 @@ autofocus: true, numeric: false, class: 'width-full', + maxlength: TwoFactorAuthenticatable::PROOFING_DIRECT_OTP_LENGTH, ) %> <%= submit_tag t('forms.buttons.submit.default'), diff --git a/app/views/shared/_one_time_code_input.html.erb b/app/views/shared/_one_time_code_input.html.erb index 370c4cff65c..bf0d4390de1 100644 --- a/app/views/shared/_one_time_code_input.html.erb +++ b/app/views/shared/_one_time_code_input.html.erb @@ -9,13 +9,15 @@ locals: * transport: WebOTP transport method. Defaults to 'sms'. * value: Field value. Defaults to `''`. * class: CSS classes to add (optional) -* numeric: if the field should only accept digits. Defualts to true +* numeric: if the field should only accept digits. Defaults to true +* maxlength: Sets maxlength for the field. Defaults to TwoFactorAuthenticatable::DIRECT_OTP_LENGTH %> <% form = local_assigns.delete(:form) name = local_assigns.delete(:name) { :code } required = local_assigns.delete(:required) { false } numeric = local_assigns.delete(:numeric) { true } transport = local_assigns.delete(:transport) { 'sms' } + maxlength = local_assigns.delete(:maxlength) { TwoFactorAuthenticatable::DIRECT_OTP_LENGTH } classes = ['field font-family-mono usa-input one-time-code-input'] classes << local_assigns.delete(:class) if local_assigns[:class] %> @@ -27,7 +29,7 @@ locals: input_html: { 'data-transport': transport, pattern: numeric ? '[0-9]*' : '[a-zA-Z0-9]*', - maxlength: TwoFactorAuthenticatable::DIRECT_OTP_LENGTH, + maxlength: maxlength, autocomplete: 'one-time-code', inputmode: numeric ? 'numeric' : 'text', **local_assigns, diff --git a/app/views/two_factor_authentication/otp_verification/show.html.erb b/app/views/two_factor_authentication/otp_verification/show.html.erb index de812472eeb..0a5b9b3918d 100644 --- a/app/views/two_factor_authentication/otp_verification/show.html.erb +++ b/app/views/two_factor_authentication/otp_verification/show.html.erb @@ -19,7 +19,8 @@ value: @presenter.code_value, required: true, autofocus: true, - numeric: false, + numeric: IdentityConfig.store.enable_numeric_authentication_otp ? true : false, + maxlength: TwoFactorAuthenticatable::DIRECT_OTP_LENGTH, ) %> <%= f.input( diff --git a/app/views/two_factor_authentication/totp_verification/show.html.erb b/app/views/two_factor_authentication/totp_verification/show.html.erb index 96a7950a884..545adebe2da 100644 --- a/app/views/two_factor_authentication/totp_verification/show.html.erb +++ b/app/views/two_factor_authentication/totp_verification/show.html.erb @@ -17,6 +17,7 @@ autofocus: true, size: 16, class: 'width-auto', + maxlength: TwoFactorAuthenticatable::OTP_LENGTH, ) %> <%= f.input( :remember_device, diff --git a/app/views/users/totp_setup/new.html.erb b/app/views/users/totp_setup/new.html.erb index 3021c1aabee..c527e40b11a 100644 --- a/app/views/users/totp_setup/new.html.erb +++ b/app/views/users/totp_setup/new.html.erb @@ -46,6 +46,7 @@ aria: { label: t('forms.totp_setup.totp_step_4') }, size: 18, class: 'width-auto', + maxlength: TwoFactorAuthenticatable::OTP_LENGTH, ) %> <% end %> <% end %> diff --git a/spec/views/shared/_one_time_code_input.html.erb_spec.rb b/spec/views/shared/_one_time_code_input.html.erb_spec.rb index 9343307d54d..39e5286e464 100644 --- a/spec/views/shared/_one_time_code_input.html.erb_spec.rb +++ b/spec/views/shared/_one_time_code_input.html.erb_spec.rb @@ -98,4 +98,25 @@ expect(rendered).to have_selector('[data-transport][data-foo="bar"]') end end + + describe 'maxlength' do + context 'no maxlength given' do + it 'renders input maxlength DIRECT_OTP_LENGTH' do + puts rendered + expect(rendered).to have_selector( + "[maxlength=\"#{TwoFactorAuthenticatable::DIRECT_OTP_LENGTH}\"]", + ) + end + end + + context 'maxlength given' do + let(:params) { { maxlength: 10 } } + + it 'renders input given maxlength' do + expect(rendered).to have_selector( + '[maxlength="10"]', + ) + end + end + end end From 47fb5ba79e089b6278eff601118b6b39ea5896e3 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 8 Jul 2022 10:10:30 -0500 Subject: [PATCH 4/6] Update app/views/two_factor_authentication/otp_verification/show.html.erb Co-authored-by: Andrew Duthie --- .../two_factor_authentication/otp_verification/show.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/two_factor_authentication/otp_verification/show.html.erb b/app/views/two_factor_authentication/otp_verification/show.html.erb index 0a5b9b3918d..b5da0b3d202 100644 --- a/app/views/two_factor_authentication/otp_verification/show.html.erb +++ b/app/views/two_factor_authentication/otp_verification/show.html.erb @@ -19,7 +19,7 @@ value: @presenter.code_value, required: true, autofocus: true, - numeric: IdentityConfig.store.enable_numeric_authentication_otp ? true : false, + numeric: IdentityConfig.store.enable_numeric_authentication_otp, maxlength: TwoFactorAuthenticatable::DIRECT_OTP_LENGTH, ) %> From f371e2f5e56c4b8d93ba9370f0c8a25ef5c9076b Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 8 Jul 2022 12:37:03 -0500 Subject: [PATCH 5/6] Update app/decorators/user_decorator.rb Co-authored-by: Zach Margolis --- app/decorators/user_decorator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/decorators/user_decorator.rb b/app/decorators/user_decorator.rb index 01c9eb080a1..d3c4bf550bf 100644 --- a/app/decorators/user_decorator.rb +++ b/app/decorators/user_decorator.rb @@ -79,7 +79,7 @@ def password_reset_profile def qrcode(otp_secret_key) options = { - issuer: 'Login.gov', + issuer: APP_NAME, otp_secret_key: otp_secret_key, digits: TwoFactorAuthenticatable::OTP_LENGTH, interval: IdentityConfig.store.totp_code_interval, From d08c82ea2212b529f8f082be11d8ec508fb2ce52 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 8 Jul 2022 13:40:58 -0500 Subject: [PATCH 6/6] add configuration for enabling numeric OTP input changelog: Upcoming Features, Authentication, Allow configuration of 6 digit numeric authentication OTPs --- .../two_factor_authentication/otp_verification/show.html.erb | 2 +- config/application.yml.default | 2 ++ lib/identity_config.rb | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/views/two_factor_authentication/otp_verification/show.html.erb b/app/views/two_factor_authentication/otp_verification/show.html.erb index b5da0b3d202..9d2435d3a8f 100644 --- a/app/views/two_factor_authentication/otp_verification/show.html.erb +++ b/app/views/two_factor_authentication/otp_verification/show.html.erb @@ -19,7 +19,7 @@ value: @presenter.code_value, required: true, autofocus: true, - numeric: IdentityConfig.store.enable_numeric_authentication_otp, + numeric: IdentityConfig.store.enable_numeric_authentication_otp_input, maxlength: TwoFactorAuthenticatable::DIRECT_OTP_LENGTH, ) %> diff --git a/config/application.yml.default b/config/application.yml.default index 21a73e1928b..4c0a0b42197 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -83,6 +83,7 @@ email_from: no-reply@login.gov email_from_display_name: Login.gov enable_load_testing_mode: false enable_numeric_authentication_otp: true +enable_numeric_authentication_otp_input: true enable_partner_api: false enable_rate_limiting: true enable_test_routes: true @@ -369,6 +370,7 @@ production: doc_auth_vendor_randomize_alternate_vendor: '' domain_name: login.gov enable_numeric_authentication_otp: false + enable_numeric_authentication_otp_input: false enable_test_routes: false enable_usps_verification: false hmac_fingerprinter_key: diff --git a/lib/identity_config.rb b/lib/identity_config.rb index fec9a1ece1e..0e51e14bdc4 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -155,6 +155,7 @@ def self.build_store(config_map) config.add(:email_from_display_name, type: :string) config.add(:enable_load_testing_mode, type: :boolean) config.add(:enable_numeric_authentication_otp, type: :boolean) + config.add(:enable_numeric_authentication_otp_input, type: :boolean) config.add(:enable_partner_api, type: :boolean) config.add(:enable_rate_limiting, type: :boolean) config.add(:enable_test_routes, type: :boolean)