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
4 changes: 1 addition & 3 deletions app/forms/new_phone_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,8 @@ def extra_analytics_attributes

def validate_not_voip
return if phone.blank? || !IdentityConfig.store.phone_service_check
return unless IdentityConfig.store.voip_block

if phone_info.type == :voip &&
!FeatureManagement.voip_allowed_phones.include?(parsed_phone.e164)
if phone_info.type == :voip
errors.add(:phone, I18n.t('errors.messages.voip_phone'), type: :voip_phone)
elsif phone_info.error
errors.add(:phone, I18n.t('errors.messages.voip_check_error'), type: :voip_check_error)
Expand Down
4 changes: 1 addition & 3 deletions app/views/users/phone_setup/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@

<p>
<%= t('two_factor_authentication.phone_fee_disclosure') %>
<% if IdentityConfig.store.voip_block %>
<%= t('two_factor_authentication.two_factor_choice_options.phone_info_no_voip') %>
<% end %>
<%= t('two_factor_authentication.two_factor_choice_options.phone_info_no_voip') %>
</p>

<%= simple_form_for(
Expand Down
2 changes: 0 additions & 2 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -361,8 +361,6 @@ get_usps_proofing_results_job_reprocess_delay_minutes: 5
get_usps_proofing_results_job_request_delay_milliseconds: 1000
voice_otp_pause_time: '0.5s'
voice_otp_speech_rate: 'slow'
voip_block: true
voip_allowed_phones: '[]'
weekly_auth_funnel_report_config: '[]'

development:
Expand Down
9 changes: 0 additions & 9 deletions lib/feature_management.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,15 +119,6 @@ def self.recaptcha_enterprise?
IdentityConfig.store.recaptcha_enterprise_project_id.present?
end

# Manual allowlist for VOIPs, should only include known VOIPs that we use for smoke tests
# @return [Set<String>] set of phone numbers normalized to e164
def self.voip_allowed_phones
@voip_allowed_phones ||= begin
allowed_phones = IdentityConfig.store.voip_allowed_phones
allowed_phones.map { |p| Phonelib.parse(p).e164 }.to_set
end
end

# Whether we collect device profiling information as part of the proofing process.
def self.proofing_device_profiling_collecting_enabled?
case IdentityConfig.store.proofing_device_profiling
Expand Down
2 changes: 0 additions & 2 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -498,8 +498,6 @@ def self.build_store(config_map)
config.add(:version_headers_enabled, type: :boolean)
config.add(:voice_otp_pause_time)
config.add(:voice_otp_speech_rate)
config.add(:voip_allowed_phones, type: :json)
config.add(:voip_block, type: :boolean)
config.add(:weekly_auth_funnel_report_config, type: :json)

@key_types = config.key_types
Expand Down
1 change: 0 additions & 1 deletion spec/controllers/users/phone_setup_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
let(:mfa_selections) { ['voice'] }
before do
allow(IdentityConfig.store).to receive(:phone_service_check).and_return(true)
allow(IdentityConfig.store).to receive(:voip_block).and_return(true)
end

describe 'GET index' do
Expand Down
1 change: 0 additions & 1 deletion spec/features/phone/add_phone_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,6 @@
let(:telephony_gem_voip_number) { '+12255551000' }

scenario 'adding a VOIP phone' do
allow(IdentityConfig.store).to receive(:voip_block).and_return(true)
allow(IdentityConfig.store).to receive(:phone_service_check).and_return(true)

user = create(:user, :fully_registered)
Expand Down
81 changes: 26 additions & 55 deletions spec/forms/new_phone_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -234,83 +234,54 @@

context 'voip numbers' do
let(:telephony_gem_voip_number) { '+12255552000' }
let(:voip_block) { false }
let(:phone_service_check) { true }

before do
allow(IdentityConfig.store).to receive(:voip_block).and_return(voip_block)
allow(IdentityConfig.store).to receive(:phone_service_check).and_return(phone_service_check)
end

subject(:result) do
form.submit(params.merge(phone: telephony_gem_voip_number))
end

context 'when voip numbers are blocked' do
let(:voip_block) { true }

it 'is invalid' do
expect(result.success?).to eq(false)
expect(result.errors[:phone]).to eq([I18n.t('errors.messages.voip_phone')])
end

it 'logs the type and carrier' do
expect(result.extra).to include(
phone_type: :voip,
carrier: 'Test VOIP Carrier',
)
end
it 'voip numbers are invalid' do
expect(result.success?).to eq(false)
expect(result.errors[:phone]).to eq([I18n.t('errors.messages.voip_phone')])
end

context 'when the number is on the allowlist' do
before do
expect(FeatureManagement).to receive(:voip_allowed_phones).
and_return([telephony_gem_voip_number])
end
it 'logs the type and carrier' do
expect(result.extra).to include(
phone_type: :voip,
carrier: 'Test VOIP Carrier',
)
end

it 'is valid' do
expect(result.success?).to eq(true)
expect(result.errors).to be_blank
end
context 'when AWS rate limits info type checks' do
before do
expect(Telephony).to receive(:phone_info).
and_raise(Aws::Pinpoint::Errors::TooManyRequestsException.new(nil, 'error message'))
end

context 'when AWS rate limits info type checks' do
before do
expect(Telephony).to receive(:phone_info).
and_raise(Aws::Pinpoint::Errors::TooManyRequestsException.new(nil, 'error message'))
end

it 'logs a warning and fails open' do
expect(result.extra[:warn]).to include('AWS pinpoint phone info rate limit')
it 'logs a warning and fails open' do
expect(result.extra[:warn]).to include('AWS pinpoint phone info rate limit')

expect(result.success?).to eq(true)
expect(result.errors).to be_blank
end
expect(result.success?).to eq(true)
expect(result.errors).to be_blank
end
end

context 'when voip checks are disabled' do
let(:phone_service_check) { false }

it 'does not check the phone type' do
expect(Telephony).to_not receive(:phone_info)
context 'when voip checks are disabled' do
let(:phone_service_check) { false }

result
end
it 'does not check the phone type' do
expect(Telephony).to_not receive(:phone_info)

it 'allows voip numbers since it cannot check the type' do
expect(result.success?).to eq(true)
expect(result.errors).to be_blank
end
result
end
end

context 'when voip numbers are allowed' do
let(:voip_block) { false }

it 'does a voip check but does not enforce it' do
expect(Telephony).to receive(:phone_info).and_call_original

it 'allows voip numbers since it cannot check the type' do
expect(result.success?).to eq(true)
expect(result.to_h).to include(phone_type: :voip)
expect(result.errors).to be_blank
end
end

Expand Down
14 changes: 0 additions & 14 deletions spec/lib/feature_management_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -350,20 +350,6 @@
end
end

describe '.voip_allowed_phones' do
before do
# clear memoization
FeatureManagement.instance_variable_set(:@voip_allowed_phones, nil)
end

it 'normalizes phone numbers and put them in a set' do
voip_allowed_phones = ['18885551234', '+18888675309']

expect(IdentityConfig.store).to receive(:voip_allowed_phones).and_return(voip_allowed_phones)
expect(FeatureManagement.voip_allowed_phones).to eq(Set['+18885551234', '+18888675309'])
end
end

describe '#proofing_device_profiling_collecting_enabled?' do
it 'returns false for disabled' do
expect(IdentityConfig.store).to receive(:proofing_device_profiling).and_return(:disabled)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,6 @@
it 'does not include a masked number' do
expect(presenter_without_mfa.info).to_not include('***')
end

context 'when VOIP numbers are blocked' do
before do
allow(IdentityConfig.store).to receive(:voip_block).and_return(true)
end
end
end
end

Expand Down
26 changes: 4 additions & 22 deletions spec/views/phone_setup/index.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,28 +28,10 @@
end

context 'voip numbers' do
before do
allow(IdentityConfig.store).to receive(:voip_block).and_return(voip_block)
end

context 'when voip numbers are allowed' do
let(:voip_block) { false }

it 'does not mention voip' do
expect(render).to_not have_content(
t('two_factor_authentication.two_factor_choice_options.phone_info_no_voip'),
)
end
end

context 'when voip numbers are blocked' do
let(:voip_block) { true }

it 'tells users to not use VOIP numbers' do
expect(render).to have_content(
t('two_factor_authentication.two_factor_choice_options.phone_info_no_voip'),
)
end
it 'tells users to not use VOIP numbers' do
expect(render).to have_content(
t('two_factor_authentication.two_factor_choice_options.phone_info_no_voip'),
)
end
end

Expand Down