Implemented Djblet's rate-limiting feature in ReviewBoard's authentication form.

Review Request #8768 - Created Feb. 20, 2017 and submitted

Raman Dhatt
Review Board
master
8698
c44726c...
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).

  • 0
  • 0
  • 38
  • 0
  • 38
Description From Last Updated
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. 
      
Raman Dhatt
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. 
      
Raman Dhatt
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. reviewboard/accounts/forms/auth.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  3. reviewboard/accounts/forms/auth.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  4. reviewboard/accounts/forms/auth.py (Diff revision 3)
     
     
    Col: 9
     E303 too many blank lines (2)
    
  5. 
      
Raman Dhatt
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. 
      
Simon Zhang
  1. 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. :)

  2. reviewboard/accounts/forms/auth.py (Diff revision 4)
     
     

    Missing a docstring for the function here?

  3. 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

  4. reviewboard/accounts/forms/auth.py (Diff revision 4)
     
     

    Correct me if I'm wrong: I don't think we need a blank line here.

  5. 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 if foo evaluates to false?

    Otherwise, I think these two statements should be merged into one.

  6. 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 as ratelimited() 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

  7. reviewboard/accounts/forms/auth.py (Diff revision 4)
     
     
     

    A parenthesis instead of a backslash?

  8. 
      
Raman Dhatt
Review Bot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. 
      
Raman Dhatt
Raman Dhatt
Review Bot
  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
    
    
  2. reviewboard/accounts/forms/auth.py (Diff revision 6)
     
     
     'settings' imported but unused
    
  3. reviewboard/accounts/forms/auth.py (Diff revision 6)
     
     
     'DEFAULT_KEY' imported but unused
    
  4. reviewboard/accounts/forms/auth.py (Diff revision 6)
     
     
    Col: 17
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/accounts/forms/auth.py (Diff revision 6)
     
     
    Col: 17
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/accounts/forms/auth.py (Diff revision 7)
     
     
     'settings' imported but unused
    
  7. reviewboard/accounts/forms/auth.py (Diff revision 7)
     
     
     'DEFAULT_KEY' imported but unused
    
  8. reviewboard/accounts/forms/auth.py (Diff revision 7)
     
     
    Col: 17
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/accounts/forms/auth.py (Diff revision 7)
     
     
    Col: 17
     E128 continuation line under-indented for visual indent
    
  10. 
      
Raman Dhatt
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. reviewboard/accounts/forms/auth.py (Diff revision 8)
     
     
    Col: 25
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/accounts/forms/auth.py (Diff revision 8)
     
     
    Col: 25
     E128 continuation line under-indented for visual indent
    
  4. 
      
Raman Dhatt
Review Bot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. 
      
Barret Rennie
  1. 
      
  2. reviewboard/accounts/forms/auth.py (Diff revision 9)
     
     

    This should be marked for translation with ugettext.

  3. reviewboard/accounts/forms/auth.py (Diff revision 9)
     
     
     
     
     
     

    Same comment wrt your djblets change.

  4. 
      
Raman Dhatt
Review Bot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. 
      
Raman Dhatt
Review Bot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. 
      
Christian Hammond
  1. 
      
  2. 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.

  3. reviewboard/accounts/forms/auth.py (Diff revision 11)
     
     

    "informs the user"

    "have been exceeded."

  4. 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.
    """
    
  5. reviewboard/accounts/forms/auth.py (Diff revision 11)
     
     

    "Djblets's"

  6. reviewboard/accounts/forms/auth.py (Diff revision 11)
     
     

    "if the number"

    "were already exceeded"

  7. reviewboard/accounts/forms/auth.py (Diff revision 11)
     
     
     

    Alignment is off. In this case, it's best to wrap like:

    raise forms.ValidationError(
        _('...'))
    
  8. reviewboard/accounts/forms/auth.py (Diff revision 11)
     
     
     

    Blank line between these.

  9. 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.

    1. I belive I did this because of the issue I am having based on another review you gave (see review at line 508).

      This is also why I must have called the super clean() class the second time.

    2. Fixed it thanks to Barret's help! See why at review you made at line 508.

  10. reviewboard/accounts/forms/auth.py (Diff revision 11)
     
     

    "for a given user"

  11. 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.

    1. THe first time I try to call the super clean() method is so that I can try to authenticate the given user. If the result shows that the authentication is failed ('None' value), then I can increment the number of attempts.

      The reason why I tried to call the super clean method the second time is that I get the following error:

      Exception Type: AttributeError at /account/login/
      Exception Value: 'AnonymousUser' object has no attribute 'backend'

      This was a hacky solution at the time, any suggestions on how to fix this would be great, totally forgot about this. :(

      I believe David gave me this suggestion on working with the super clean() method here.

    2. Fixed it thanks to Barret!

      We had to remove the ValidationError exception and use a 'try/finally' block instead. Also had to call the is_valid() method from the superclass to determine if the clean() method was able to authenticate the user with the given credentials. wooho! :)

  12. 
      
Raman Dhatt
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. reviewboard/accounts/forms/auth.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
Raman Dhatt
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. 
      
Raman Dhatt
Raman Dhatt
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. reviewboard/accounts/forms/auth.py (Diff revision 14)
     
     
     local variable 'result' is assigned to but never used
    
  3. reviewboard/accounts/forms/auth.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. 
      
Raman Dhatt
Review Bot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. 
      
Raman Dhatt
  1. Ship It!
  2. 
      
Raman Dhatt
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. reviewboard/accounts/forms/auth.py (Diff revision 16)
     
     
     local variable 'e' is assigned to but never used
    
  3. 
      
Raman Dhatt
Review Bot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/forms/auth.py
    
    
  2. 
      
Raman Dhatt
  1. Ship It!
  2. 
      
Barret Rennie
  1. 
      
  2. reviewboard/accounts/forms/auth.py (Diff revision 17)
     
     

    Technically, this should be self.cleaned_data = super(...).clean()

  3. reviewboard/accounts/forms/auth.py (Diff revision 17)
     
     

    is None

  4. 
      
Raman Dhatt
Raman Dhatt
Raman Dhatt
Raman Dhatt
Barret Rennie
  1. 
      
  2. RR dependencies should live in the depends on field, not the description.

  3. 
      
Raman Dhatt
David Trowbridge
  1. Diff revisions 19 and 20 look like the wrong change, but 18 looks good. Landing with a few small cleanups.

  2. 
      
Raman Dhatt
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (4930d37)
Loading...