New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LG-12712 Use phone fingerprint as rate limit key for phone confirmation #10459
Conversation
) | ||
submission_rate_limiter.increment! | ||
|
||
lock_out_user if submission_rate_limiter.maxed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
submission_rate_limiter ||= RateLimiter.new( | ||
target: @phone, | ||
rate_limit_type: :phone_submissions, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
except: :seconds, | ||
), | ||
) | ||
redirect_to account_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
b4ab5a3
to
07a7ceb
Compare
…limit key for phone confirmation
…urfaced, better lockout
…ect to phone setup and remove 2f lockout
22acfd5
to
53b187c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
end | ||
it 'displays an error banner' do | ||
sign_in_before_2fa(@user) | ||
allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(999) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(999) | ||
allow(IdentityConfig.store).to receive( | ||
:phone_submissions_per_fingerprint_max_attempts_window_in_minutes, | ||
).and_return(10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
).and_return(10) | |
).and_return(2) |
app/forms/new_phone_form.rb
Outdated
|
||
def validate_phone_submission_limit | ||
fingerprint = Pii::Fingerprinter.fingerprint(Phonelib.parse(phone).e164.to_s) | ||
@submission_rate_limiter ||= RateLimiter.new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@submission_rate_limiter ||= RateLimiter.new( | |
submission_rate_limiter ||= RateLimiter.new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily submitting a blocking review, I'm looking at whether there's some redundancy here with existing rate-limiting.
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 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
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 |
Affirmative. It does in fact apply rate limiting for phone fingerprint regardless of the account used. |
🎫 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)