Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

django 1.3 compatibility with dynamic hashers from 1.4 #16

Merged
merged 1 commit into from Sep 20, 2012

Conversation

Projects
None yet
3 participants
Contributor

diox commented Jul 24, 2012

If a project uses django_sha2 + django 1.4 + dynamic hashers generation as suggested in the README, and then goes back to 1.3, checking the password of any user whose password was "upgraded" to the new-style hashers fails.

If I understand the code correctly, this is because the hashers used for 1.4 support have different prefixes, one for each entry in HMAC_KEYS, which wasn't the case before. In check_password() monkeypatch, there is a check to see if django_sha2 should use its own bcrypt_auth.check_password() code but it only checks if the hasher is "bcrypt" or "hh", so it fails if any dynamic hasher was used.

This pull request makes the check_password() monkeypatch also check if the hasher is in the dynamic hashers list. I haven't included tests yet because I'm not sure this is the right solution at the moment and I'd prefer having feedback on the patch before working on some tests.

rik commented Sep 6, 2012

Hey @fwenzel can you take a look at this?

@fwenzel fwenzel commented on an outdated diff Sep 7, 2012

django_sha2/auth.py
@@ -63,7 +65,7 @@ def check_password(self, raw_password):
Supports automatic upgrading to stronger hashes.
"""
hashed_with = self.password.split('$', 1)[0]
- if hashed_with in ['bcrypt', 'hh']:
+ if hashed_with in ['bcrypt', 'hh'] or hashed_with in get_dynamic_hasher_names(settings.HMAC_KEYS):
@fwenzel

fwenzel Sep 7, 2012

Owner

Line-break after the "or".

Owner

fwenzel commented Sep 7, 2012

Oh wow, how did I miss this pull request? Thanks for the PR @diox, good work! (And thanks @rik for pointing me back here).

I think the code is good! @diox will you write some tests and add them to the PR?

Again, sorry for the silly delay :-/

Contributor

diox commented Sep 7, 2012

Sure, working on that now.

Contributor

diox commented Sep 19, 2012

@fwenzel : Corrected the pep8 issues and added a test.

While doing the tests, I stumbled on a couple weird issues :

  • HMAC_KEYS was not sorted so wasn't used correctly in 1.4 sometimes, depending on how the dict was set up and how python was returning it. I modified the settings to demonstrate the issues, and fixed it as well.
  • Also, Sha2Tests were failing in django 1.3 (see issue #15) and I did a quick fix while I was at it. Sorry if that pollutes the PR, this was annoying me when running the tests.

I think everything is all right now, but a second pair of eyes would be nice.

@fwenzel fwenzel commented on an outdated diff Sep 19, 2012

test/django13/tests/test_bcrypt.py
@@ -42,6 +42,37 @@ def test_nokey(self):
assert not authenticate(username='jude', password=u'abcéäêëôøà')
assert not authenticate(username='jude', password=u'çççbbbààà')
+ def test_password_from_django14(self):
+ """Test that a password generated by django_sha2 with django 1.4 is
+ recognized and changed to a 1.3 version"""
+ # We can't easily call 1.4's hashers so we hardcode the passwords as
+ # returned with the specific salts and hmac_key in 1.4.
+ prefix = 'bcrypt2011_01_01$2a$12$'
+ suffix = '$2011-01-01'
+ raw_hashes = {
+ 'john': '02CfJWdVwLK80jlRe/Xx1u8sTHAR0JUmKV9YB4BS.Os4LK6nsoLie',
+ 'jane': '.ipDt6gRL3CPkVH7FEyR6.8YXeQFXAMyiX3mXpDh4YDBonrdofrcG',
+ 'jude': '6Ol.vgIFxMQw0LBhCLtv7OkV.oyJjen2GVMoiNcLnbsljSfYUkQqe',
+ }
+
+ u = User.objects.get(username="john")
+ django14_style_password = prefix + raw_hashes['john'] + suffix
@fwenzel

fwenzel Sep 19, 2012

Owner

Chaining of "+" to concatenate text is frowned upon in Python. You want ''.join((my, tuple)).

@fwenzel

fwenzel Sep 19, 2012

Owner

or '%s%s%s' % (my, other, tuple) of course ;)

Owner

fwenzel commented Sep 19, 2012

Good job, this looks good! Will you squash your commits into a single commit? (git rebase -i HEAD~7 or so and then squash them together).

Contributor

diox commented Sep 20, 2012

Done ! The string concatenation is gone, all tests are passing and I rebased everything in one commit.

Owner

fwenzel commented Sep 20, 2012

You, sir, are a scholar and a gentleman. Thank you!

@fwenzel fwenzel added a commit that referenced this pull request Sep 20, 2012

@fwenzel fwenzel Merge pull request #16 from diox/master
django 1.3 compatibility with dynamic hashers from 1.4
f4519bf

@fwenzel fwenzel merged commit f4519bf into fwenzel:master Sep 20, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment