Implemented Djblet's rate-limiting feature in ReviewBoard's authentication form.
Review Request #8768 — Created Feb. 20, 2017 and submitted
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. |
 brennie
|
|
|
Col: 80 E501 line too long (85 > 79 characters) |
 reviewbot
|
|
|
Col: 80 E501 line too long (89 > 79 characters) |
reviewbot
|
|
|
Col: 9 E303 too many blank lines (2) |
reviewbot
|
|
|
Missing a docstring for the function here? |
 szhang
|
|
|
Multi-line strings should use parenthesis, according to: https://www.notion.so/reviewboard/Python-Coding-Style-Guide-f9a0e51ee0cf40379fcbbae40bbbec04#ec5a472e3610412ea99d0835a1faf2c0 |
szhang
|
|
|
Correct me if I'm wrong: I don't think we need a blank line here. |
szhang
|
|
|
Can these two if statements be merged into one? A question I would have with this is if, for the … |
szhang
|
|
|
I don't think the call is long enough to warrant this? I think if request started on the same line … |
szhang
|
|
|
A parenthesis instead of a backslash? |
szhang
|
|
|
'settings' imported but unused |
reviewbot
|
|
|
'DEFAULT_KEY' imported but unused |
reviewbot
|
|
|
Col: 17 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 17 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
'settings' imported but unused |
reviewbot
|
|
|
'DEFAULT_KEY' imported but unused |
reviewbot
|
|
|
Col: 17 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 17 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 25 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 25 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
This should be marked for translation with ugettext. |
brennie
|
|
|
Same comment wrt your djblets change. |
brennie
|
|
|
No blank line here, since from the point of view of the Review Board codebase, djblets is a third-party module. |
 chipx86
|
|
|
"informs the user" "have been exceeded." |
chipx86
|
|
|
This doesn't validate the clean field, but rather validates the form. These docs also need a "Returns" section, like: """ … |
chipx86
|
|
|
"Djblets's" |
chipx86
|
|
|
"if the number" "were already exceeded" |
chipx86
|
|
|
Alignment is off. In this case, it's best to wrap like: raise forms.ValidationError( _('...')) |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Throwing away all validation errors will mean that things like username or password validation will go away, and we'll allow … |
chipx86
|
|
|
"for a given user" |
chipx86
|
|
|
We're calling this a second time, which we don't want to do. The parent clean() should only be called once. … |
chipx86
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
local variable 'result' is assigned to but never used |
reviewbot
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
local variable 'e' is assigned to but never used |
reviewbot
|
|
|
Technically, this should be self.cleaned_data = super(...).clean() |
brennie
|
|
|
is None |
brennie
|
-
Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py
RK
- Summary:
-
Implementing rate-limiting feature in ReviewBoard's authentication form. In progress (Need to fix bug where authentication is not working -- AnonymousUser has noattribute 'backend' whenever trying to log in; cannot get information from self.cache_user whether authenticated or not.)Implemented Djblet's rate-limiting feature in ReviewBoard's authentication form.
- Description:
-
~ Implementing rate-limiting feature in ReviewBoard's authentication form. In progress (Need to fix bug where authentication is not working -- AnonymousUser has noattribute 'backend' whenever trying to log in; cannot get information from self.cache_user whether authenticated or not.)
~ 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 also relates to review request 8698, where the ratelimiting has been implemented in djblets.
- Testing Done:
-
~ None at the moment.
~ 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).
- Commit:
-
a8b9d1e3724bdc396807aaac2a387f6d7baa666ca7114821093569b34a4f47ebea1dabc6f00b4215
- Diff:
-
Revision 3 (+40 -1)
RK
- Commit:
-
a7114821093569b34a4f47ebea1dabc6f00b4215ef699c62050d8e1f47d45bfbeea5fbbbb3522f0a
- 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. :)
-
-
Multi-line strings should use parenthesis, according to: https://www.notion.so/reviewboard/Python-Coding-Style-Guide-f9a0e51ee0cf40379fcbbae40bbbec04#ec5a472e3610412ea99d0835a1faf2c0
-
-
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 iffooevaluates tofalse?Otherwise, I think these two statements should be merged into one.
-
I don't think the call is long enough to warrant this?
I think if
requeststarted 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
-
RK
- Commit:
-
ef699c62050d8e1f47d45bfbeea5fbbbb3522f0ab8307fa0a51fa9d8f821c042e8f67dc84529a3f9
- 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
RK
- Commit:
-
b8307fa0a51fa9d8f821c042e8f67dc84529a3f9c18f500aa80d49fe0c621a100cebd81037151109
- Diff:
-
Revision 6 (+37 -1)
RK
- Commit:
-
c18f500aa80d49fe0c621a100cebd81037151109e74c8beae413a285891def3f3a4dfca295f7ddf0
- 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 -
-
-
-
-
-
-
-
RK
- Commit:
-
e74c8beae413a285891def3f3a4dfca295f7ddf08b16b3de2d62526dfc68e1a6cce2db95716153f6
- Diff:
-
Revision 8 (+42 -1)
RK
- Commit:
-
8b16b3de2d62526dfc68e1a6cce2db95716153f62ba6d9d8ac944bda87a193c96fc1306c22ad4263
- 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
RK
- Commit:
-
2ba6d9d8ac944bda87a193c96fc1306c22ad42632ad58472cbcb44a6f5b262922599e6939c989d61
- 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
RK
- Commit:
-
2ad58472cbcb44a6f5b262922599e6939c989d6155f0fec4a078242837e500fc5f4cbee750bdd209
- 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
-
-
No blank line here, since from the point of view of the Review Board codebase,
djbletsis a third-party module. -
-
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. """ -
-
-
-
-
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.
-
-
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.
RK
- Commit:
-
55f0fec4a078242837e500fc5f4cbee750bdd20964b9b21f8f2139dc1363aad16fa488b74ea4da5e
- Diff:
-
Revision 12 (+42 -1)
RK
- Commit:
-
64b9b21f8f2139dc1363aad16fa488b74ea4da5e1a36f0bf2bd16a954cb4f6480213d25a7d0a0cf5
- 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
RK
- Description:
-
~ 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 also relates to review request 8698, where the ratelimiting has been implemented in djblets.
~ 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 is dependent on review request 8698, where the ratelimiting has been implemented in djblets.
RK
- Commit:
-
1a36f0bf2bd16a954cb4f6480213d25a7d0a0cf505072b018d18d2a6d871e49f2197a127422e8817
- Diff:
-
Revision 14 (+39 -1)
RK
- Commit:
-
05072b018d18d2a6d871e49f2197a127422e88177fecbe3a6d6e9d2fa6a29cbd7fae502a87854fe3
- 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
RK
- Commit:
-
7fecbe3a6d6e9d2fa6a29cbd7fae502a87854fe3fab85f9683f573ff1d0c9f2bd22118a5c9a7dddb
- Diff:
-
Revision 16 (+37 -1)
RK
- Commit:
-
fab85f9683f573ff1d0c9f2bd22118a5c9a7dddb1f42f5e3895b4d84ff52bd36f6db5f419514e296
- 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
RK
- Description:
-
~ 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 is dependent on review request 8698, where the ratelimiting has been implemented in djblets.
~ 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 is dependent on review request 8698, where the ratelimiting has + been implemented in djblets. - Testing Done:
-
~ 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).
~ 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). - Commit:
-
1f42f5e3895b4d84ff52bd36f6db5f419514e296c44726cf2e31b7d5218f9be3edd69c5e1a8b7c42
- Diff:
-
Revision 18 (+37 -1)
Checks run (3 succeeded)
JSHint
passed.
PEP8 Style Checker
passed.
Pyflakes
passed.
RK
- Diff:
-
Revision 19 (+26 -1)
Checks run (3 succeeded)
JSHint
passed.
PEP8 Style Checker
passed.
Pyflakes
passed.
RK
Checks run (2 succeeded, 1 failed with error)
JSHint
passed.
PEP8 Style Checker
internal error.
Pyflakes
passed.
RK
- Description:
-
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 is dependent on review request 8698, where the ratelimiting has ~ This is dependent on review requests 8698 and 8839 (which is based off of 8698), where the ratelimiting has been implemented in djblets.
RK
- Description:
-
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. ~ and prevent further logins until some amount of time has passed. - This is dependent on review requests 8698 and 8839 (which is based off of 8698), where the ratelimiting has - been implemented in djblets.
Tool: Pyflakes Processed Files: reviewboard/accounts/forms/auth.py Tool: PEP8 Style Checker Processed Files: reviewboard/accounts/forms/auth.py