Strip username spaces #106
Conversation
This is the default behaviour in django.contrib.auth.forms.AuthenticationForm https://github.com/django/django/blob/1.11.6/django/contrib/auth/forms.py#L156
| username = request.POST['username'] | ||
| # we are using AuthenticationForm in which username is CharField with strip=True that automatically strips | ||
| # whitespace characters, we need to copy that behaviour | ||
| username = request.POST['username'].strip() |
There was a problem hiding this comment.
I think this method was probably copy/pasted long ago from an example Django login doc.. but, should we just be creating the form here and initializing it with the form POST variables, calling validate and then using cleaned_data? IE, doing it the django way instead of introspecting the POST variables directly? That feels more correct to me.
There was a problem hiding this comment.
I think we added the failed login count logic in here too
There was a problem hiding this comment.
Another plus to using the form here is that then it is up to the form to figure out to strip or not, so it can be up to users/callers to choose what they care about here.
There was a problem hiding this comment.
there is one small issue with using AuthenticationForm(request=request) ... it will try to authenticate user as part of the clean process: https://github.com/django/django/blob/1.11.6/django/contrib/auth/forms.py#L187
if the user can not be authenticated, form is not valid, and that prevents 'failed login count logic' for valid users ... in the end we still need to get the username and strip it, because invalid forms do not have cleaned_data property, so we must use raw input
There was a problem hiding this comment.
Oh good catch, well shoot, ya then I guess what we're doing is the best we can do.
This is a bug fix for: rapidpro/rapidpro#390
We should think about refactoring tests and create isolated tests, because these 'all-in-one' are hard to debug and context changes throughout the single test method