Opened 9 years ago

Closed 18 months ago

#3648 defect closed wontfix (wontfix)

twisted.cred.credentials.UsernameHashedPassword doesn't hash password strings when checkPassword

Reported by: cocteau Owned by: mesozoic
Priority: highest Milestone:
Component: core Keywords:
Cc: Thijs Triemstra, mesozoic Branch: branches/no-usernamehashedpassword-3648-3
branch-diff, diff-cov, branch-cov, buildbot
Author: washort, mesozoic

Description

checkPassword method does not hash an input password. Because of this, checkPassword method return wrong result.

Attachments (2)

credentials.py.patch (888 bytes) - added by cocteau 9 years ago.
UsernameHashedPassword.checkPassword.patch
無題.txt (582 bytes) - added by cocteau 9 years ago.
credentials.py.patch

Download all attachments as: .zip

Change History (25)

Changed 9 years ago by cocteau

Attachment: credentials.py.patch added

UsernameHashedPassword.checkPassword.patch

Changed 9 years ago by cocteau

Attachment: 無題.txt added

credentials.py.patch

comment:1 Changed 9 years ago by Glyph

Huh. This bug report appears to be correct. The attached patch doesn't fix the problem though, because it's not really clear what hash algorithm UsernameHashedPassword is supposed to use.

I think maybe the class should just be deprecated and then deleted?

comment:2 Changed 9 years ago by washort

Branch: no-usernamehashedpassword-3648
Owner: Glyph deleted
Priority: normalhighest
Type: enhancementdefect

Deprecation ready for review.

comment:3 Changed 9 years ago by washort

Keywords: review added

comment:4 Changed 9 years ago by Jean-Paul Calderone

Branch: no-usernamehashedpassword-3648/branches/no-usernamehashedpassword-3648

comment:5 Changed 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to washort
  1. twisted/test/test_newcred.py
    1. copyright date needs to be updated
    2. test_requestAvatarIdHashed needs a docstring
    3. test_hashedCredentials needs a docstring
    4. DeprecationTests won't need to exist if you use callDeprecated in test_requestAvatarIdHashed instead of calling UsernameHashedPassword directly and then flushing its warnings (alternatively, you can keep calling UsernameHashedPassword like that and make an assertion about the return value of flushWarnings).
    5. The changes to getGoodCredentials and getBadCredentials are a bit too broad. They will flush warnings from any of the subclasses of that mixin, but I think that only the NetworkHashedFilePasswordDBMixin subclasses should have those warnings suppressed.

comment:6 Changed 8 years ago by Thijs Triemstra

Author: washort
Branch: /branches/no-usernamehashedpassword-3648branches/no-usernamehashedpassword-3648
Cc: Thijs Triemstra added

comment:7 Changed 7 years ago by <automation>

Owner: washort deleted

comment:8 Changed 6 years ago by mesozoic

(In [33311]) addressing feedback in comment:5 (refs #3648)

comment:9 Changed 6 years ago by mesozoic

Keywords: review added
Owner: set to Jean-Paul Calderone

comment:10 Changed 6 years ago by mesozoic

Owner: Jean-Paul Calderone deleted

comment:11 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to mesozoic

Thanks! Still a few issues:

  1. There's some conflicts in this branch in the unit tests. The branch needs to be merged forward and these resolved before the final review can happen.
  2. The version number in the deprecation needs to be updated - it'll be 12.0.0, not 9.0.0.
  3. Test method docstrings should not start with "Test that"
  4. Why are those new getGoodCredentials and getBadCredentials implementations necessary? I deleted them and didn't notice any change.
  5. Some of the unit tests fail

comment:12 Changed 6 years ago by mesozoic

Author: washortwashort, mesozoic
Branch: branches/no-usernamehashedpassword-3648branches/no-usernamehashedpassword-3648-2

(In [33353]) Branching to 'no-usernamehashedpassword-3648-2'

comment:13 Changed 6 years ago by mesozoic

(In [33355]) Branching to 'no-usernamehashedpassword-3648-2'

comment:14 Changed 6 years ago by mesozoic

Keywords: review added
Owner: mesozoic deleted

I can't reproduce a failure on any of the unit tests, either on my Mac or on Ubuntu. Can you share the test failure you're seeing (unless they were from unbranching)?

The reimplementations of getGoodCredentials/getBadCredentials were a response to bullet #5 in comment:5, where you suggested that only the NetworkHashedFilePasswordDBMixin should suppress the deprecation warnings. If the deprecation warnings are acceptable, then I think this is ready to get merged.

comment:15 Changed 6 years ago by Glyph

Build results, in case anyone feels like reviewing.

comment:16 Changed 6 years ago by David Sturgis

Keywords: review removed
Owner: set to mesozoic

The unit test failures are only in the Windows build, and do not seem to relate to any of the issues (or changes) in this branch, so I think there are just a few more steps to go:

  1. Please update the Twisted version number from 12.0 to 12.1
  2. I think this branch still needs a news file?
  3. Merge

Thanks,

comment:17 Changed 3 years ago by mesozoic

Cc: mesozoic added
Keywords: review added

It appears I never actually closed this.

  1. Twisted version number has been bumped to 15.2. :)
  2. Added a news file.

Ready for re-review, I think.

comment:18 Changed 3 years ago by mesozoic

Keywords: review removed

Merge conflicts. Not review-ready yet.

comment:19 Changed 3 years ago by mesozoic

Branch: branches/no-usernamehashedpassword-3648-2branches/no-usernamehashedpassword-3648-3

(In [44838]) Branching to no-usernamehashedpassword-3648-3.

comment:20 Changed 3 years ago by mesozoic

Keywords: review added

comment:21 Changed 3 years ago by mesozoic

Owner: mesozoic deleted

comment:22 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to mesozoic

Thanks for your changes!

15.2.0 was already released ... the branch should be updated to 15.3.0

From what I can see, the latest proposed changes does not fix the bug but just deprecate the code.

I think that you should either update the ticket description/title or create a new ticket dedicated to deprecation... and close this ticket as wont fix.


The deprecation code needs a dedicated new test. Check current test suite for test named 'test_deprecation*`

Maybe the news file should be updated to

twisted.cred.credentials.UsernameHashedPassword is now deprecated.

Thanks!

comment:23 Changed 18 months ago by mesozoic

Resolution: wontfix
Status: newclosed

OK, I'm going to actually try to close this now. See #8368 for the deprecation ticket.

Note: See TracTickets for help on using tickets.