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
30 changes: 30 additions & 0 deletions app/controllers/users/two_factor_authentication_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module Users
class TwoFactorAuthenticationController < ApplicationController
include TwoFactorAuthenticatable
include ActionView::Helpers::DateHelper

before_action :check_remember_device_preference
before_action :redirect_to_vendor_outage_if_phone_only, only: [:show]
Expand Down Expand Up @@ -164,6 +165,7 @@ def handle_valid_otp_params(method, default = nil)
return handle_too_many_otp_sends if exceeded_otp_send_limit?
otp_rate_limiter.increment
return handle_too_many_otp_sends if exceeded_otp_send_limit?
return handle_too_many_confirmation_sends if exceeded_phone_confirmation_limit?

@telephony_result = send_user_otp(method)
handle_telephony_result(method: method, default: default)
Expand Down Expand Up @@ -196,6 +198,18 @@ def exceeded_otp_send_limit?
return otp_rate_limiter.lock_out_user if otp_rate_limiter.exceeded_otp_send_limit?
end

def phone_confirmation_throttle
@phone_confirmation_throttle ||= Throttle.for(
user: current_user,
throttle_type: :phone_confirmation,
)
end
Comment on lines +202 to +206
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check my understanding:

  • this limits attempts by the same logged-in user for any phone
  • our rack-attack should limit attempts by IP address to submit this form

So we shouldn't need to worry about different users spamming different phones because the other rule covers it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly. The OtpRateLimiter will still ensure a specific phone number is not receiving excessive OTPs, and rack-attack places IP rate limits around the routes where OTPs are sent.


def exceeded_phone_confirmation_limit?
return false unless UserSessionContext.confirmation_context?(context)
phone_confirmation_throttle.throttled_else_increment?
end

def send_user_otp(method)
if PhoneNumberOptOut.find_with_phone(phone_to_deliver_to)
return Telephony::Response.new(
Expand Down Expand Up @@ -274,5 +288,21 @@ def webauthn_params
params[:platform] = current_user.webauthn_configurations.platform_authenticators.present?
params
end

def handle_too_many_confirmation_sends
flash[:error] = t(
'errors.messages.phone_confirmation_throttled',
timeout: distance_of_time_in_words(
Time.zone.now,
[phone_confirmation_throttle.expires_at, Time.zone.now].compact.max,
except: :seconds,
),
)
if user_fully_authenticated?
redirect_to account_url
else
redirect_to login_two_factor_options_url
end
end
end
end
5 changes: 5 additions & 0 deletions app/models/throttle.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class Throttle < ApplicationRecord
verify_gpo_key: 8,
proof_ssn: 9,
proof_address: 10,
phone_confirmation: 11,
}

THROTTLE_CONFIG = {
Expand Down Expand Up @@ -57,6 +58,10 @@ class Throttle < ApplicationRecord
max_attempts: IdentityConfig.store.proof_address_max_attempts,
attempt_window: IdentityConfig.store.proof_address_max_attempt_window_in_minutes,
},
phone_confirmation: {
max_attempts: IdentityConfig.store.phone_confirmation_max_attempts,
attempt_window: IdentityConfig.store.phone_confirmation_max_attempt_window_in_minutes,
},
}.with_indifferent_access.freeze

# Either target or user must be supplied
Expand Down
4 changes: 4 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,8 @@ participate_in_dap: false
partner_api_bucket_prefix: ''
password_max_attempts: 3
personal_key_retired: true
phone_confirmation_max_attempts: 20
phone_confirmation_max_attempt_window_in_minutes: 1_440
phone_setups_per_ip_limit: 25
phone_setups_per_ip_period: 300
phone_setups_per_ip_track_only_mode: false
Expand Down Expand Up @@ -428,6 +430,8 @@ test:
otps_per_ip_period: 10
otps_per_ip_track_only_mode: false
password_pepper: f22d4b2cafac9066fe2f4416f5b7a32c
phone_confirmation_max_attempts: 5
phone_confirmation_max_attempt_window_in_minutes: 10
phone_setups_per_ip_limit: 3
phone_setups_per_ip_period: 10
phone_setups_per_ip_track_only_mode: false
Expand Down
1 change: 1 addition & 0 deletions config/locales/errors/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ en:
otp_failed: Your security code failed to send.
password_incorrect: Incorrect password
personal_key_incorrect: Incorrect personal key
phone_confirmation_throttled: You tried too many times, please try again in %{timeout}.
phone_country_constraint_usa: Must be a U.S. phone number
phone_duplicate: This account is already using the phone number you entered as
an authenticator. Please use a different phone number.
Expand Down
1 change: 1 addition & 0 deletions config/locales/errors/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ es:
otp_failed: Se produjo un error al enviar el código de seguridad.
password_incorrect: La contraseña es incorrecta
personal_key_incorrect: La clave personal es incorrecta
phone_confirmation_throttled: Lo intentaste muchas veces, vuelve a intentarlo en %{timeout}.
phone_country_constraint_usa: Debe ser un número telefónico de los Estados Unidos
phone_duplicate: Esta cuenta ya está utilizando el número de teléfono que
ingresó como autenticador. Por favor, use un número de teléfono
Expand Down
1 change: 1 addition & 0 deletions config/locales/errors/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ fr:
otp_failed: Échec de l’envoi de votre code de sécurité.
password_incorrect: Mot de passe incorrect
personal_key_incorrect: Clé personnelle incorrecte
phone_confirmation_throttled: Vous avez essayé plusieurs fois, essayez à nouveau dans %{timeout}.
phone_country_constraint_usa: Dois être un numéro de téléphone américain
phone_duplicate: Ce compte utilise déjà le numéro de téléphone que vous avez
entré en tant qu’authentificateur. Veuillez utiliser un numéro de
Expand Down
2 changes: 2 additions & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,8 @@ def self.build_store(config_map)
config.add(:password_max_attempts, type: :integer)
config.add(:password_pepper, type: :string)
config.add(:personal_key_retired, type: :boolean)
config.add(:phone_confirmation_max_attempts, type: :integer)
config.add(:phone_confirmation_max_attempt_window_in_minutes, type: :integer)
config.add(:phone_setups_per_ip_limit, type: :integer)
config.add(:phone_setups_per_ip_period, type: :integer)
config.add(:pii_lock_timeout_in_minutes, type: :integer)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
require 'rails_helper'

describe Users::TwoFactorAuthenticationController do
include ActionView::Helpers::DateHelper

describe 'before_actions' do
it 'includes the appropriate before_actions' do
expect(subject).to have_actions(
Expand Down Expand Up @@ -444,13 +446,13 @@ def index
before do
@user = create(:user)
@unconfirmed_phone = '+1 (202) 555-1213'
end

it 'sends OTP inline when confirming phone' do
sign_in_before_2fa(@user)
subject.user_session[:context] = 'confirmation'
subject.user_session[:unconfirmed_phone] = @unconfirmed_phone
end

it 'sends OTP inline when confirming phone' do
allow(Telephony).to receive(:send_confirmation_otp).and_call_original

get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
Expand All @@ -465,7 +467,62 @@ def index
)
end

it 'rate limits confirmation OTPs on sign up' do
sign_in_before_2fa(@user)
subject.user_session[:context] = 'confirmation'
allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(999)

freeze_time do
(IdentityConfig.store.phone_confirmation_max_attempts + 1).times do
subject.user_session[:unconfirmed_phone] = '+1 (202) 555-1213'
get :send_code,
params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
end

timeout = distance_of_time_in_words(
Throttle.attempt_window_in_minutes(:phone_confirmation).minutes,
)

expect(flash[:error]).to eq(
I18n.t(
'errors.messages.phone_confirmation_throttled',
timeout: timeout,
),
)
expect(response).to redirect_to login_two_factor_options_url
end
end

it 'rate limits confirmation OTPs when adding number to existing account' do
stub_sign_in(@user)
subject.user_session[:context] = 'confirmation'
allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(999)

freeze_time do
(IdentityConfig.store.phone_confirmation_max_attempts + 1).times do
subject.user_session[:unconfirmed_phone] = '+1 (202) 555-1213'
get :send_code,
params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
end

timeout = distance_of_time_in_words(
Throttle.attempt_window_in_minutes(:phone_confirmation).minutes,
)

expect(flash[:error]).to eq(
I18n.t(
'errors.messages.phone_confirmation_throttled',
timeout: timeout,
),
)
expect(response).to redirect_to account_url
end
end

it 'flashes an sms error when the telephony gem responds with an sms error' do
sign_in_before_2fa(@user)
subject.user_session[:context] = 'confirmation'
subject.user_session[:unconfirmed_phone] = @unconfirmed_phone
subject.user_session[:unconfirmed_phone] = '+1 (225) 555-1000'

get :send_code, params: { otp_delivery_selection_form: { otp_delivery_preference: 'sms' } }
Expand Down
27 changes: 27 additions & 0 deletions spec/features/users/sign_up_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
feature 'Sign Up' do
include SamlAuthHelper
include DocAuthHelper
include ActionView::Helpers::DateHelper

context 'confirmation token error message does not persist on success' do
scenario 'with blank token' do
Expand Down Expand Up @@ -107,6 +108,32 @@
expect(page).to have_content(I18n.t('telephony.error.friendly_message.generic'))
end

scenario 'rate limits sign-up phone confirmation attempts' do
allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(999)

sign_up_and_set_password

freeze_time do
(IdentityConfig.store.phone_confirmation_max_attempts + 1).times do
visit phone_setup_path
fill_in 'new_phone_form_phone', with: '2025551313'
click_send_security_code
end

timeout = distance_of_time_in_words(
Throttle.attempt_window_in_minutes(:phone_confirmation).minutes,
)

expect(current_path).to eq(login_two_factor_options_path)
expect(page).to have_content(
I18n.t(
'errors.messages.phone_confirmation_throttled',
timeout: timeout,
),
)
end
end

context 'with js', js: true do
before do
page.driver.browser.execute_cdp(
Expand Down