LG-12712 Use phone fingerprint as rate limit key for phone confirmation

[kevinsmaster5]

🎫 Ticket

Link to the relevant ticket:
LG-12712

🛠 Summary of changes

Adds a method to PhoneSetupController that gets called before create and checks the submitted phone as an encoded fingerprint against a RateLimit. Specify limit to 20 submissions in a 10 minute window and not on a per-user basis but rather on a per phone basis.

📜 Testing Plan

(in your local application.yml set phone_submissions_per_fingerprint_limit: 5 for easier testing)

• Log in at http://localhost:3000
• Attempt to add a phone and when OTP page shows click 'Use another phone number' and repeat 5x with the same number
• Observe you are sent to your account page with a flash warning ' you tried too many times ... ' and a 10 minute cooldown
• Log out, log in as another user
• Attempt to add the same phone number as above and receive the same warning and lockout to account page

[mitchellhenke]

I think this could be potentially problematic in the case where I maliciously submit the same number a bunch of times, and then you come along within the next N timeframe and submit it for the first time (that you're aware of), and the rate limiter locks you out.

[mitchellhenke]

I think there's overlap between this and the OtpRateLimiter here, which uses the phone number hash as the discriminator in the same way. Is it worth looking at updating that instead of creating a somewhat duplicative rate limiter?

[kevinsmaster5]

I was looking at OtpRateLimiter initially but since I only so far recreate the phone_fingerprint method. I thought maybe I'd have to do something with the otp_findtime and otp_maxretry_times functions since they are specific to how it is originally applied, but maybe they wouldn't have had any impact on using it here.
It just seemed to be less impact calling Pii::Fingerprinter.fingerprint(... than revising OtpRateLimiter.

[mitchellhenke]

I guess my suggestion is more along the lines of whether we should update the configuration values used in OtpRateLimiter rather than add a very similar additional rate limit.

Right now, otp_findtime and otp_maxretry_times are used for both confirmed and unconfirmed phone numbers in OtpRateLimiter, but I think it's worth considering splitting that to have separate configurations for confirmed/unconfirmed phone numbers over the approach here.

[aduth]

I feel like the reuse of OtpRateLimiter is nice insofar as being able to reuse some of the logic around creating a rate-limit key with the phone fingerprint, but that's really all the value I see from that class. I don't think we should inherit the same behaviors of locking out the user or logging them out. I'd almost rather we just extracted a PhoneFingerprinter service class and avoid OtpRateLimiter altogether.

[aduth]

I really don't think we should redirect the user to the account page, since that may throw them off course if they had intended to return to the service provider.

This kind of validation should probably be part of NewPhoneForm, to keep the controller lean and so that it can take advantage of the existing unsuccessful submission handling in the create method above.

That being said, create doesn't handle error messages, or at least it didn't as of the current branch point of this pull request. This was changed in #10454 to support error messages for reCAPTCHA. I could maybe imagine the code for first_error_message being updated to support multiple keys so that we could surface validation errors for either reCAPTCHA or rate-limiting.

flash.now[:error] = result.first_error_message(:rate_limited, :recaptcha_token)

At the very least, I'd think we should redirect back to the phone setup form so the error message is shown on the same page, not redirect the user away..

[kevinsmaster5]

That makes sense to let the limiter do the work. I have changed that and moved the validation to the form. I'm not 100% sure how I executed the error message since it doesn't use flash but it looks like it's working.

[aduth]

Can we add a feature spec which validates that the alert banner is visible to the user after they're rate-limited?

(I believe this will help surface a bug in the current implementation)

[aduth]

LGTM 👍

[aduth]

Is this necessary?

[kevinsmaster5]

Not at all, apparently. It would be useful for hammering at the phone submissions limiter and not wanting to hit that other limit it seems. Isn't a problem here. Removed.

[aduth]

We could make the test run a little faster if we reduced this a bit so it's not looping as many times in the loop that follows.

Suggested change

	).and_return(10)
	).and_return(2)

[aduth]

If we're not using this anywhere outside the scope of this method, I don't think we need to assign it as an instance variable.

Suggested change

	@submission_rate_limiter ||= RateLimiter.new(
	submission_rate_limiter ||= RateLimiter.new(

[kevinsmaster5]

That makes sense. 👍

[aduth]

Temporarily submitting a blocking review, I'm looking at whether there's some redundancy here with existing rate-limiting.

[aduth]

Unfortunately, I think this behavior may already be implemented, stemming back to @mitchellhenke's comment in #10459 (comment) . It's a little non-obvious, especially since it happens through the TwoFactorAuthenticationController controller, which is typically only responsible for authentication, not MFA setup. But in the case of phone setup, the code confirmation also goes through that controller.

There's an existing feature spec here which appears to verify that this behavior is expected to be enforced:

identity-idp/spec/support/shared_examples/phone/rate_limitting.rb

Lines 26 to 44 in dc7ea61

	it 'limits the number of times a code can be sent to a phone across accounts' do
	visit_otp_confirmation(delivery_method)
	max_otp_sends.times do
	click_on t('links.two_factor_authentication.send_another_code')
	end
	Capybara.reset_session!
	visit root_path
	sign_up_and_set_password
	select_2fa_option(:phone)
	fill_in :new_phone_form_phone, with: phone
	click_send_one_time_code
	expect(page).to have_content(t('two_factor_authentication.max_otp_requests_reached'))
	expect_user_to_be_rate_limitted
	expect_rate_limitting_to_expire
	end

The actual numbers of the rate limit may be different from what we had come up with for this work, but I think we'd be okay with slight differences from what we'd planned.

Can you take a look and confirm if the behavior we were seeking to implement here is already handled via OtpRateLimiter?

[kevinsmaster5]

Can you take a look and confirm if the behavior we were seeking to implement here is already handled via OtpRateLimiter?

Affirmative. It does in fact apply rate limiting for phone fingerprint regardless of the account used.
The difference being as you noted OtpRateLimiter checks during 2FA rather than at phone setup as in this PR. Also a distinction between that and what I've been working on is the user doesn't get signed out and their 2FA locked where in the OtpRateLimiter it does. Which ironically could lead to a scenario cautioned against in this comment and in our slack discussion.