Proof-of-concept Delay Between OTP Sending (LG-7899)#10360
Proof-of-concept Delay Between OTP Sending (LG-7899)#10360mitchellhenke merged 3 commits intomainfrom
Conversation
mitchellhenke
force-pushed
the
mitchellhenke/short-term-otp-rate-limit
branch
from
b093e24 to
782c892
Compare
changelog: Internal, Rate Limiting, Add short-term rate limit as delay between OTP sends
mitchellhenke
force-pushed
the
mitchellhenke/short-term-otp-rate-limit
branch
from
782c892 to
aff110e
Compare
| phone_recaptcha_mock_validator: false | ||
| piv_cac_verify_token_secret: | ||
| session_encryptor_alert_enabled: true | ||
| short_term_phone_otp_rate_limiter_enabled: false |
There was a problem hiding this comment.
Curious the rationale for this?
There was a problem hiding this comment.
I'd rather default to off for deployed environments and opt-in manually for now. We can/should remove it if we're happy with it though.
| personal_key_retired: true | ||
| phone_carrier_registration_blocklist_array: '[]' | ||
| short_term_phone_otp_max_attempt_window_in_seconds: 10 | ||
| short_term_phone_otp_max_attempts: 2 |
There was a problem hiding this comment.
My initial impression was that this would allow someone to request 2 codes within 10 seconds, but I can only request 1 code within 10 seconds. I think either would be fine, but the configuration value felt a little misleading to me (or at least unexpected), i.e. I would expect this value to be 1 if I can only request 1 code every 10 seconds.
There was a problem hiding this comment.
Yeah, the way the RateLimiter class is kind of weird in that way unfortunately :/
We could switch
identity-idp/app/services/rate_limiter.rb
Line 67 in 4d41f77
> rather than >= and reduce everything by one 😬
|
@mitchellhenke - My understanding is this implementation is going to limit 2 SMS / 10 seconds. I'd like to see the error message that is going to be displayed to the user in this sceanrio. |
The limit is 1 SMS / 10 seconds. Andrew has a comment here, the "max" is not very intuitive. The error will look like: |
| subject.user_session[:context] = 'confirmation' | ||
| allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(999) | ||
|
|
||
| expect(@analytics).to receive(:track_event).with('OTP: Delivery Selection', anything). |
There was a problem hiding this comment.
Can we use have_logged_event after the actions instead of receive(:track_event) so it uses all FakeAnalytics behaviors?
spec/controllers/users/two_factor_authentication_controller_spec.rb
Outdated
Show resolved
Hide resolved
mitchellhenke
force-pushed
the
mitchellhenke/short-term-otp-rate-limit
branch
from
be2fd98 to
1c00b91
Compare
3 participants
🎫 Ticket
Link to the relevant ticket:
LG-7899
🛠 Summary of changes
This is not quite ready yet, but I'm opening this PR to share a proof-of-concept implementation of LG-7899.