Implemented Djblet's rate-limiting feature in ReviewBoard's authentication form.
Review Request #8768 — Created Feb. 20, 2017 and submitted
Information | |
---|---|
rkdhatt | |
Review Board | |
master | |
|
|
c44726c... | |
Reviewers | |
reviewboard, students | |
There has been a request to implement a rate-limiting feature in
ReviewBoard's authentication form by tracking the number of failed login
attempts per IP/username in the cache, along with the last login time,
and prevent further logins until some amount of time has passed.
This has been tested manually by attempting to log into reviewboard with
an existing username but incorrect password until the maximum number of
attempts has been reached. In addition, the number of login attempts and
time left before rate limit is over was also tracked during this process
using print statements in djblet's ratelimit.py file (more specifically,
the dictionary returned from the get_usage_count() method in ratelimit.py).
Description | From | Last Updated |
---|---|---|
RR dependencies should live in the depends on field, not the description. |
|
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (89 > 79 characters) |
![]() |
|
Col: 9 E303 too many blank lines (2) |
![]() |
|
Missing a docstring for the function here? |
![]() |
|
Multi-line strings should use parenthesis, according to: https://www.notion.so/reviewboard/Python-Coding-Style-Guide-f9a0e51ee0cf40379fcbbae40bbbec04#ec5a472e3610412ea99d0835a1faf2c0 |
![]() |
|
Correct me if I'm wrong: I don't think we need a blank line here. |
![]() |
|
Can these two if statements be merged into one? A question I would have with this is if, for the … |
![]() |
|
I don't think the call is long enough to warrant this? I think if request started on the same line … |
![]() |
|
A parenthesis instead of a backslash? |
![]() |
|
'settings' imported but unused |
![]() |
|
'DEFAULT_KEY' imported but unused |
![]() |
|
Col: 17 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 17 E128 continuation line under-indented for visual indent |
![]() |
|
'settings' imported but unused |
![]() |
|
'DEFAULT_KEY' imported but unused |
![]() |
|
Col: 17 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 17 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 25 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 25 E128 continuation line under-indented for visual indent |
![]() |
|
This should be marked for translation with ugettext. |
|
|
Same comment wrt your djblets change. |
|
|
No blank line here, since from the point of view of the Review Board codebase, djblets is a third-party module. |
|
|
"informs the user" "have been exceeded." |
|
|
This doesn't validate the clean field, but rather validates the form. These docs also need a "Returns" section, like: """ … |
|
|
"Djblets's" |
|
|
"if the number" "were already exceeded" |
|
|
Alignment is off. In this case, it's best to wrap like: raise forms.ValidationError( _('...')) |
|
|
Blank line between these. |
|
|
Throwing away all validation errors will mean that things like username or password validation will go away, and we'll allow … |
|
|
"for a given user" |
|
|
We're calling this a second time, which we don't want to do. The parent clean() should only be called once. … |
|
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
local variable 'result' is assigned to but never used |
![]() |
|
Col: 80 E501 line too long (81 > 79 characters) |
![]() |
|
local variable 'e' is assigned to but never used |
![]() |
|
Technically, this should be self.cleaned_data = super(...).clean() |
|
|
is None |
|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+30) |

-
Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py
Summary: |
|
||||||
---|---|---|---|---|---|---|---|
Description: |
|
||||||
Testing Done: |
|
||||||
Commit: |
|
||||||
Diff: |
Revision 3 (+40 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py
-
reviewboard/accounts/forms/auth.py (Diff revision 3) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/accounts/forms/auth.py (Diff revision 3) Col: 80 E501 line too long (89 > 79 characters)
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+41 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py

-
I am a little confused about how this all works, but perhaps a docstring for
clean()
might help with that?Otherwise, these are just mostly comments on style. :)
-
-
reviewboard/accounts/forms/auth.py (Diff revision 4) Multi-line strings should use parenthesis, according to: https://www.notion.so/reviewboard/Python-Coding-Style-Guide-f9a0e51ee0cf40379fcbbae40bbbec04#ec5a472e3610412ea99d0835a1faf2c0
-
reviewboard/accounts/forms/auth.py (Diff revision 4) Correct me if I'm wrong: I don't think we need a blank line here.
-
reviewboard/accounts/forms/auth.py (Diff revision 4) Can these two if statements be merged into one?
A question I would have with this is if, for the example code:
if foo and bar():
Will
bar()
be executed iffoo
evaluates tofalse
?Otherwise, I think these two statements should be merged into one.
-
reviewboard/accounts/forms/auth.py (Diff revision 4) I don't think the call is long enough to warrant this?
I think if
request
started on the same line asratelimited()
and all other arguments were aligned with that, that would be better.So, example 2 instead of example 3 from what's listed here: https://www.notion.so/reviewboard/Python-Coding-Style-Guide-f9a0e51ee0cf40379fcbbae40bbbec04#c8f86d436ebc4a5198ff42612b3fef92
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+41 -1) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+37 -1) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+43 -1) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py
-
-
-
reviewboard/accounts/forms/auth.py (Diff revision 6) Col: 17 E128 continuation line under-indented for visual indent
-
reviewboard/accounts/forms/auth.py (Diff revision 6) Col: 17 E128 continuation line under-indented for visual indent
-
-
-
reviewboard/accounts/forms/auth.py (Diff revision 7) Col: 17 E128 continuation line under-indented for visual indent
-
reviewboard/accounts/forms/auth.py (Diff revision 7) Col: 17 E128 continuation line under-indented for visual indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+42 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py
-
reviewboard/accounts/forms/auth.py (Diff revision 8) Col: 25 E128 continuation line under-indented for visual indent
-
reviewboard/accounts/forms/auth.py (Diff revision 8) Col: 25 E128 continuation line under-indented for visual indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+42 -1) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py
-
-
reviewboard/accounts/forms/auth.py (Diff revision 9) This should be marked for translation with
ugettext
. -
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+42 -1) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+38 -1) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py
-
-
reviewboard/accounts/forms/auth.py (Diff revision 11) No blank line here, since from the point of view of the Review Board codebase,
djblets
is a third-party module. -
-
reviewboard/accounts/forms/auth.py (Diff revision 11) This doesn't validate the clean field, but rather validates the form.
These docs also need a "Returns" section, like:
""" ... Returns: dict: The cleaned data for all fields in the form. """
-
-
-
reviewboard/accounts/forms/auth.py (Diff revision 11) Alignment is off. In this case, it's best to wrap like:
raise forms.ValidationError( _('...'))
-
-
reviewboard/accounts/forms/auth.py (Diff revision 11) Throwing away all validation errors will mean that things like username or password validation will go away, and we'll allow for things like duplicate usernames.
-
-
reviewboard/accounts/forms/auth.py (Diff revision 11) We're calling this a second time, which we don't want to do. The parent
clean()
should only be called once. It's very possible for there to be side effects otherwise.I'm actually a bit confused about what this method is trying to do. Shouldn't this all solely live in Djblets? I woudn't expect this form to have to do much of anything, just inherit from a mixin in Djblets that implements all this.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+42 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py
-
reviewboard/accounts/forms/auth.py (Diff revision 12) Col: 80 E501 line too long (80 > 79 characters)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+42 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py
Description: |
|
||||||
---|---|---|---|---|---|---|---|
Depends On: |
|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+39 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py
-
reviewboard/accounts/forms/auth.py (Diff revision 14) local variable 'result' is assigned to but never used
-
reviewboard/accounts/forms/auth.py (Diff revision 14) Col: 80 E501 line too long (81 > 79 characters)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+37 -1) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+37 -1) |

-
Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py
-
reviewboard/accounts/forms/auth.py (Diff revision 16) local variable 'e' is assigned to but never used
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+37 -1) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py
-
-
reviewboard/accounts/forms/auth.py (Diff revision 17) Technically, this should be
self.cleaned_data = super(...).clean()
-
Description: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||
Diff: |
Revision 18 (+37 -1) |
Checks run (3 succeeded)
Diff: |
Revision 19 (+26 -1) |
---|
Checks run (3 succeeded)
Diff: |
Revision 20 (+42 -1) |
---|
Checks run (2 succeeded, 1 failed with error)
Description: |
|
---|
Description: |
|
---|