Skip to content

add some rate limiting to the password reset functionality - #2711

Merged
chrisndodge merged 2 commits into
masterfrom
cdodge/throttle-forgot-password
Feb 27, 2014
Merged

add some rate limiting to the password reset functionality#2711
chrisndodge merged 2 commits into
masterfrom
cdodge/throttle-forgot-password

Conversation

@chrisndodge

Copy link
Copy Markdown
Contributor

This uses the same 3rd party rate limiting library as is used in the login paths.

@dianakhuang can you take a look?

@ovnicraft

Copy link
Copy Markdown
Contributor

@chrisndodge 3rd party not needed to be added to requirements ?

@chrisndodge

Copy link
Copy Markdown
Contributor Author

This is already in use in the stack, I'm just re-using it.

Comment thread common/djangoapps/student/views.py Outdated

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.

Instead of instantiating a Mixin class, and duplicating code from ratelimitbackend/backends.py, can we create a reusable class that lets us add rate limiting in a more modular way? Looks like a .request_allowed(request) method and a .bad_request(request) method would do the trick. It's too bad the ratelimitbackend wasn't written this way to start with.

@chrisndodge

Copy link
Copy Markdown
Contributor Author

@nedbat, better?

Comment thread common/djangoapps/student/views.py Outdated

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.

Love the refactor, thanks! Did you mean for this to say "is rated limit exceeded", or "is rate limit exceeded"?

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.

doh!

@nedbat

nedbat commented Feb 27, 2014

Copy link
Copy Markdown
Contributor

@chrisndodge looks good. Fix the function name if you meant it the other way, then merge it. 👍

chrisndodge pushed a commit that referenced this pull request Feb 27, 2014
add some rate limiting to the password reset functionality
@chrisndodge
chrisndodge merged commit 7884e49 into master Feb 27, 2014
@jzoldak
jzoldak deleted the cdodge/throttle-forgot-password branch May 5, 2014 14:54
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Jun 21, 2019
…-2654-7

openedx#2654 Add module category to manage status. add test code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants