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
6 changes: 5 additions & 1 deletion app/controllers/account_reset/request_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ def show
end

def create
create_account_reset_request
rate_limiter = RateLimiter.new(user: current_user, rate_limit_type: :account_reset_request)
rate_limiter.increment!
unless rate_limiter.limited?
create_account_reset_request
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what happens if account reset request isnt created? right now it looks like we take them to account reset confirm, this could cause confusion for users that may hit this limit naturally especially if its 2 requests per 2 days.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I set the limiter to 2 minutes because the real world thing this is trying to prevent (the controller not POSTing in time with button mashing) I expect should be over by then. 542a916

end
flash[:email] = current_user.email_addresses.take.email

redirect_to account_reset_confirm_request_url
Expand Down
4 changes: 4 additions & 0 deletions app/services/rate_limiter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,10 @@ def self.rate_limit_config

def self.load_rate_limit_config
{
account_reset_request: {
max_attempts: IdentityConfig.store.account_reset_request_max_attempts,
Comment thread
mdiarra3 marked this conversation as resolved.
Outdated
attempt_window: IdentityConfig.store.account_reset_request_attempt_window_in_minutes,
},
idv_doc_auth: {
max_attempts: IdentityConfig.store.doc_auth_max_attempts,
attempt_window: IdentityConfig.store.doc_auth_attempt_window_in_minutes,
Expand Down
15 changes: 9 additions & 6 deletions app/views/account_reset/request/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@
<p><%= t('account_reset.request.delete_email2_html', interval: @account_reset_deletion_period_interval) %></p>

<p><strong><%= t('account_reset.request.continue_deletion') %></strong></p>
<%= button_to(
account_reset_request_path,
form_class: 'margin-top-4 margin-bottom-2',
class: 'usa-button usa-button--big usa-button--wide',
method: :post,
) { t('account_reset.request.yes_continue') } %>

<%= simple_form_for(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mentioned here, but I think a more comprehensive mitigation is needed based on reading the ticket

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have added rate limiting with d4577ff
I left in the UI change so I think unless they have JS turned off they'll never hit the rate limit but it's there as an additional stopgap.

:account_reset,
url: account_reset_request_path,
method: 'POST',
) do |f| %>
<%= f.submit(t('account_reset.request.yes_continue'), data: { disable_with: 'Confirming...' }, class: 'margin-bottom-2') %>
<% end %>

<%= button_to(
root_url,
method: :get,
Expand Down
2 changes: 2 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ aamva_verification_request_timeout: 5.0
aamva_verification_url: https://example.org:12345/verification/url
account_creation_device_profiling: disabled
account_reset_fraud_user_wait_period_days:
account_reset_request_attempt_window_in_minutes: 2
account_reset_request_max_attempts: 2
account_reset_token_valid_for_days: 1
Copy link
Copy Markdown
Contributor

@mitchellhenke mitchellhenke Jun 10, 2025

Choose a reason for hiding this comment

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

I don't have strong opinions on what the values here should be, but it should be monitored for potential adjustments

account_reset_wait_period_days: 1
account_suspended_support_code: EFGHI
Expand Down
2 changes: 2 additions & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ def self.store
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)
config.add(:account_reset_request_attempt_window_in_minutes, type: :integer)
config.add(:account_reset_request_max_attempts, type: :integer)
config.add(:account_suspended_support_code, type: :string)
config.add(:acuant_sdk_initialization_creds)
config.add(:acuant_sdk_initialization_endpoint)
Expand Down
61 changes: 61 additions & 0 deletions spec/controllers/account_reset/request_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -158,5 +158,66 @@

expect(response).to redirect_to authentication_methods_setup_url
end

context 'when the Yes, continue deletion... button is clicked multiple times' do
Comment thread
mdiarra3 marked this conversation as resolved.
Outdated
it 'rate limits submission and prevents multiple sms and emails' do
max_attempts = IdentityConfig.store.account_reset_request_max_attempts
user = create(:user, :fully_registered)
stub_sign_in_before_2fa(user)
stub_analytics

post :create
post :create

expect(@analytics).to have_logged_event(
'Account Reset: request',
success: true,
sms_phone: true,
totp: false,
piv_cac: false,
email_addresses: 1,
request_id: 'fake-message-request-id',
message_id: 'fake-message-id',
)
.exactly(max_attempts - 1)
.times
end
end

context 'when returning to deletion page after previous submission expired' do
it 'allows the user to submit a deletion request' do
user = create(:user, :fully_registered)
stub_sign_in_before_2fa(user)
stub_analytics

post :create

expect(@analytics).to have_logged_event(
'Account Reset: request',
success: true,
sms_phone: true,
totp: false,
piv_cac: false,
email_addresses: 1,
request_id: 'fake-message-request-id',
message_id: 'fake-message-id',
)

travel_to(Time.zone.now + 2.days) do
post :create

expect(@analytics).to have_logged_event(
'Account Reset: request',
success: true,
sms_phone: true,
totp: false,
piv_cac: false,
email_addresses: 1,
request_id: 'fake-message-request-id',
message_id: 'fake-message-id',
)
end
end
end
end
end