Add rate limiting for check_username_availability #35688
Conversation
dimagimon
added
the
Risk: High
corehq/apps/registration/utils.py
Outdated
| @@ -313,3 +321,89 @@ def send_mobile_experience_reminder(recipient, full_name, additional_email_conte | |||
| logging.warning( | |||
| "Can't send email, but the message was:\n%s" % message_plaintext) | |||
| raise | |||
|
|
|||
|
|
|||
| @run_only_when(not settings.ENTERPRISE_MODE and not settings.UNIT_TESTING) | |||
There was a problem hiding this comment.
Should we enforce this for enterprise environments as well?
corehq/apps/registration/utils.py
Outdated
| _status_bad_request = 'bad_request' | ||
| _status_accepted = 'accepted' | ||
|
|
||
| def get_session_and_ip(): |
There was a problem hiding this comment.
move these nested methods out (for readability) and privatize them (if it makes sense)
corehq/apps/registration/utils.py
Outdated
| if request and request.session: | ||
| return request.session.session_key, get_ip(request) | ||
| else: | ||
| return None |
corehq/apps/registration/utils.py
Outdated
| def get_session_and_ip(): | ||
| request = get_request() | ||
| if request and request.session: | ||
| return request.session.session_key, get_ip(request) | ||
| else: | ||
| return None | ||
|
|
||
| def _check_for_exceeded_rate_limits(session, ip): |
There was a problem hiding this comment.
Can we move these outside to not be nested functions?
corehq/apps/registration/utils.py
Outdated
| if request and request.session: | ||
| return request.session.session_key, get_ip(request) | ||
| else: | ||
| return None |
There was a problem hiding this comment.
needs to return None, None
corehq/apps/registration/utils.py
Outdated
|
|
||
| session_id, ip_address = get_session_and_ip() | ||
|
|
||
| if ip_address and session_id: |
There was a problem hiding this comment.
Given that these are bad-faith actors, is it possible to continually create new session ids? Is this a concern?
There was a problem hiding this comment.
Not as far as I've found. If given a bad session ID, the request is rejected on the backend and the sessionid cookie gets removed. From django docs:
A MAC (Message Authentication Code) is used to protect the data against changes by the client, so that the session data will be invalidated when being tampered with.
corehq/apps/registration/utils.py
Outdated
| _status_session_rate_limited = 'session_rate_limited' | ||
| _status_ip_rate_limited = 'ip_rate_limited' | ||
| _status_bad_request = 'bad_request' | ||
| _status_accepted = 'accepted' |
There was a problem hiding this comment.
I think our standard for constants is all-caps. Can also move these to a const module.
There was a problem hiding this comment.
Didn't move to a const module, but did move the rate limiter to its own file, as we seem to do in other apps (ota, motech, receiverwrapper)
7f9d328
This pattern is used in other apps with rate limiters
- run rate limiter in Enterprise environments - return None, None fallback from _get_session_and_ip - return isValid: False if check_username_availability is rate limited
Product Description
Adds rate limiting when checking username availability prior to signup. Invisible under normal circumstances.
Technical Summary
SAAS-16420
Based on rate_limit_two_factor_setup, this adds a rate limiter for the
check_username_availabilityendpoint that is based on both IP (more lax, since public IPs can sometimes be shared) and session ID (more strict).Specific rates here were chosen because they seemed "about right" for someone possibly typing and retyping an email address several times during signup, with the fastest per-session rate matching the frontend rate limit on the signup form (a 400ms delay). Open to other thoughts here as well.
Safety Assurance
Safety story
Tested locally to see that IP and session ID get stored and checked against as expected for rate limiting, and that rate limits do seem to apply correctly when requests are quick enough.
Automated test coverage
No automated tests here.
QA Plan
Not currently planning QA.
Rollback instructions
Labels & Review