Opened 12 years ago
Closed 5 years 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)
Change History (25)
Changed 12 years ago by
Attachment: | credentials.py.patch added |
---|
comment:1 Changed 12 years ago by
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 12 years ago by
Branch: | → no-usernamehashedpassword-3648 |
---|---|
Owner: | Glyph deleted |
Priority: | normal → highest |
Type: | enhancement → defect |
Deprecation ready for review.
comment:3 Changed 12 years ago by
Keywords: | review added |
---|
comment:4 Changed 12 years ago by
Branch: | no-usernamehashedpassword-3648 → /branches/no-usernamehashedpassword-3648 |
---|
comment:5 Changed 12 years ago by
Keywords: | review removed |
---|---|
Owner: | set to washort |
twisted/test/test_newcred.py
- copyright date needs to be updated
test_requestAvatarIdHashed
needs a docstringtest_hashedCredentials
needs a docstringDeprecationTests
won't need to exist if you usecallDeprecated
intest_requestAvatarIdHashed
instead of callingUsernameHashedPassword
directly and then flushing its warnings (alternatively, you can keep callingUsernameHashedPassword
like that and make an assertion about the return value offlushWarnings
).- The changes to
getGoodCredentials
andgetBadCredentials
are a bit too broad. They will flush warnings from any of the subclasses of that mixin, but I think that only theNetworkHashedFilePasswordDBMixin
subclasses should have those warnings suppressed.
comment:6 Changed 11 years ago by
Author: | → washort |
---|---|
Branch: | /branches/no-usernamehashedpassword-3648 → branches/no-usernamehashedpassword-3648 |
Cc: | Thijs Triemstra added |
comment:7 Changed 10 years ago by
Owner: | washort deleted |
---|
comment:9 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | set to Jean-Paul Calderone |
comment:10 Changed 9 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
comment:11 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to mesozoic |
Thanks! Still a few issues:
- 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.
- The version number in the deprecation needs to be updated - it'll be 12.0.0, not 9.0.0.
- Test method docstrings should not start with "Test that"
- Why are those new
getGoodCredentials
andgetBadCredentials
implementations necessary? I deleted them and didn't notice any change. - Some of the unit tests fail
comment:12 Changed 9 years ago by
Author: | washort → washort, mesozoic |
---|---|
Branch: | branches/no-usernamehashedpassword-3648 → branches/no-usernamehashedpassword-3648-2 |
(In [33353]) Branching to 'no-usernamehashedpassword-3648-2'
comment:14 Changed 9 years ago by
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:16 Changed 9 years ago by
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:
- Please update the Twisted version number from 12.0 to 12.1
- I think this branch still needs a news file?
- Merge
Thanks,
comment:17 Changed 6 years ago by
Cc: | mesozoic added |
---|---|
Keywords: | review added |
It appears I never actually closed this.
- Twisted version number has been bumped to 15.2. :)
- Added a news file.
Ready for re-review, I think.
comment:19 Changed 6 years ago by
Branch: | branches/no-usernamehashedpassword-3648-2 → branches/no-usernamehashedpassword-3648-3 |
---|
(In [44838]) Branching to no-usernamehashedpassword-3648-3.
comment:20 Changed 6 years ago by
Keywords: | review added |
---|
comment:21 Changed 6 years ago by
Owner: | mesozoic deleted |
---|
comment:22 Changed 6 years ago by
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 5 years ago by
Resolution: | → wontfix |
---|---|
Status: | new → closed |
OK, I'm going to actually try to close this now. See #8368 for the deprecation ticket.
UsernameHashedPassword.checkPassword.patch