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

[cocteau]

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

[cocteau]

UsernameHashedPassword.checkPassword.patch

[cocteau]

credentials.py.patch

[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?

[washort]

Deprecation ready for review.

[Jean-Paul Calderone]

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.

[mesozoic]

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

[Jean-Paul Calderone]

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

[mesozoic]

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

[mesozoic]

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

[mesozoic]

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.

[Glyph]

​Build results, in case anyone feels like reviewing.

[David Sturgis]

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,

[mesozoic]

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.

[mesozoic]

Merge conflicts. Not review-ready yet.

[mesozoic]

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

[Adi Roiban]

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!

[mesozoic]

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