lowercasify username on login request

[ayleph]

When a user registers, the chosen username is lowercased before being stored to the database. When a user attempts to log in, the supplied username is passed through as it was typed, which may contain uppercase letters. A user who registered his or her name using uppercase letters may not realize that the username was lowercased internally and may continue trying to log in with uppercase letters.

After a failed login attempt with uppercase letters, the login form helpfully displays the username lowercased. However, it still may not be clear to the end user that he or she needs to log in with that lowercased username.

[ayleph]

I branched off current GMG master and pushed a fix.

​https://gitorious.org/mediagoblin/aylephs-mediagoblin/commit/dc52d4493f6da24f2aba6d7616f6b6596c6181c8

Summary: modify mediagoblin/auth/views.py to send the post-validated (ie, lowercasified) username to check_login_simple, rather than the pre-validated username.

[Matt Molyneaux]

This could do with a a unit test to prevent regressions.

Also, the commit you've linked to doesn't seem to do anything other than refactor a few lines - am I missing something? (the parent commit is from the main GMG master branch)

[ayleph]

Yes, the commit does something :) I added a lengthy comment to the commit. I don't know how to write tests, and the docs don't really give much help there. I'll try to re-explain here.

In the original code, username = login_form.username.data is called to store the username before login_form.validate() is run. This is bad because login_form.validate() can change the value of login_form.username.data.

If a user types in capital letters when he registers his account, the capital letters are lowercased before being stored in the database. If the user tries to log in with a username that contains capital letters, the username is lowercased when login_form.validate() is run. So a username stored before login_form.validate() is run may still contain capital letters and will fail to match the username in the database.

Steps to reproduce:

1. Register an account with username 'TestUser'
2. Try to log in with 'TestUser' and be denied
3. See [mediagoblin.auth.tools] User u'TestUser' not found in log file, while login form now displays username as 'testuser' because the username was modified during validation
4. Try to log in with 'testuser' and be granted access
5. See [mediagoblin.auth.tools] Logging u'testuser' in in log file

The solution is to wait until after login_form.validate() to store the username. I ran into this exact problem on my instance, and this fixed it.

You could simply move line that stores username after login_form.validate() like this:

    if request.method == 'POST':

        if login_form.validate():
		username = login_form.username.data
		user = check_login_simple(username, login_form.password.data)

But username isn't used elsewhere in this function, so why bother storing it to its own variable? So I just mimicked the way password was handled and wrote it like this:

    if request.method == 'POST':

        if login_form.validate():
		user = check_login_simple(
                    login_form.username.data,
                    login_form.password.data)

[Matt Molyneaux]

OK, didn't realise login_form.validate() changed object data as well as returning a value (I'll grumble about that elsewhere :P )

As for unit tests, take a look at the tests already in Mediagoblin - your test only needs to POST some data and read the response. Something like ​this should give you a few hints.

Feel free to ping me in IRC if you need a bit more help with unit tests!

[ayleph]

Alright, I don't know if this is what you had in mind, but I modified test_auth.py to add three tests to check whether a username gets properly lowercased when the user attempts to log in with an uppercased username.

​https://gitorious.org/mediagoblin/aylephs-mediagoblin/commit/7a47f82ccc32fdd89b0416b9f3012935dde90bd3

New tests:

• Verify that username is no longer uppercased
• Verify that username is properly lowercased
• Verify that username which was converted from uppercase to lowercase returns a proper login

If you run this test on the codebase without the change from the commit linked in this ticket, the new tests will fail. If you modify the code with the changes in the linked commit, the new tests will pass.

@@ -229,6 +229,21 @@ def test_register_views(test_app):
     assert urlparse.urlsplit(response.location)[2] == '/'
     assert 'mediagoblin/root.html' in template.TEMPLATE_TEST_CONTEXT
 
+    ## Verify that username is lowercased on login attempt
+    template.clear_test_template_context()
+    response = test_app.post(
+        '/auth/login/', {
+            'username': u'ANGRYGIRL',
+            'password': 'iamveryveryangry'})
+
+    # Username should no longer be uppercased; it should be lowercased
+    assert not form.username.data == u'ANGRYGIRL'
+    assert form.username.data == u'angrygirl'
+
+    # User should be redirected
+    response.follow()
+    assert urlparse.urlsplit(response.location)[2] == '/'
+    assert 'mediagoblin/root.html' in template.TEMPLATE_TEST_CONTEXT
 
 def test_authentication_views(test_app):
     """

The code change and the unit test updates are included in the branch linked below.

​https://gitorious.org/mediagoblin/aylephs-mediagoblin/commits/7a47f82ccc32fdd89b0416b9f3012935dde90bd3

[Matt Molyneaux]

Looks good to me! Passed unit tests locally. Didn't test rebasing/merging back to master.

[Matt Molyneaux]

Ah, I spoke too soon!

form is not the form from the last response.

You can verify this by changing the username to "ANGRYGIRL1", form.username.data will still be angrygirl rather than angrygirl1

In order to avoid contamination with other parts of that unit test, I would recommend you split this bit of code into it's own test.

[Matt Molyneaux]

In fact, it should probably be added to test_authentication_views (which is just below) - you can see examples there for getting the fresh form instance from each request too.

[ayleph]

Okay, I think I got the unit test fixed. Let me know if there are still issues.

​https://gitorious.org/mediagoblin/aylephs-mediagoblin/commit/591ac0ad4a17d0f1b3821d85aa359d2d91bdfd57

[Matt Molyneaux]

OK, unit tests are fine now.

[Christopher Allan Webber]

Merged and pushed! Thank you!