Opened 6 years ago

Closed 4 years ago

#3056 defect closed fixed (fixed)

twisted.spread.pb.IUsernameMD5Password's docstring should say it accepts an MD5 digest of a password, not plaintext

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: pb Keywords:
Cc: thijs, jesstess Branch: branches/pb-md5-docstring-3056
(diff, github, buildbot, log)
Author: jesstess Launchpad Bug:

Description

This interface seems to be entirely redundant with IUsernameHashedPassword.

Attachments (1)

IUsernameMD5Password_Deprecated.patch (1.9 KB) - added by jesstess 5 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by thijs

  • Cc thijs added

comment:2 Changed 5 years ago by jesstess

  • Cc jesstess added
  • Owner changed from warner to jesstess

Changed 5 years ago by jesstess

comment:3 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

Some questions on this ticket:

  • I'm not sure how to eke a DeprecationWarning out of an interface to make a test case. Implementing the interface doesn't cause one. Is there a standard way to do this?
  • Should I be doing something to class _PortalAuthChallenger as well since it implements IUsernameMD5Password?

comment:4 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner set to jesstess
  1. twisted.python.deprecate.deprecatedModuleAttributes will let you deprecate the attribute (this results in a deprecation warning when the attribute is accessed, either via '.' or an import statement). I'm not sure if there's a way to hook into implements() or the other zope.interface declaration APIs; that'd be cool, but probably isn't totally necessary. Emitting a deprecation from __init__ won't do anything, I think, as interfaces aren't intended to be instantiated (and perhaps cannot be instantiated (I hope)).
  2. Yea. It'd be good to eliminate any uses of this interface within Twisted, as long as doing so doesn't introduce backwards compatibility issues. In the case of _PortalAuthChallenger, this is a private class, so we can do pretty much anything we want to it without worrying about compatibility issues.

comment:5 Changed 4 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/pb-md5-deprecate-3056

(In [28129]) Branching to 'pb-md5-deprecate-3056'

comment:6 Changed 4 years ago by jesstess

(In [28132]) Deprecate IUsernameMD5Password using deprecatedModuleAttribute and add
a unit test for the deprecation.

refs #3056

comment:7 Changed 4 years ago by jesstess

(In [28133]) Remove references to IUsernameMD5Password in _PortalAuthChallenger.

refs #3056

comment:8 Changed 4 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

I left checkPassword and checkMD5Password as distinct functions in _PortalAuthChallenger to be conservative about compatibility, but I can roll checkMD5Password into checkPassword if people prefer that.

comment:9 Changed 4 years ago by thijs

  • Keywords review removed
  • Owner set to jesstess

Can you open a ticket for the removal of this interface?

comment:10 Changed 4 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

#4267 was opened for the removal.

comment:11 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to jesstess
  1. There's some weird quoting in test_pb.py, in the docstring for the new test, test_IUsernameMD5Password.
  2. You can just do pb.IUsernameMD5Password in that test, rather than using getattr, can't you? It should do the same thing, I think, but it's simpler.
  3. I suppose that _PortalAuthChallenger.checkMD5Password should be deprecated as well. Someone might get their hands on an instance of _PortalAuthChallenger via cred (eg, by implementing a checker for it) and call the method. They should get warned in that case. As long as we're doing that, I guess there's also no reason to remove the IUsernameMD5Password implements declaration from _PortalAuthChallenger right now either. We can delete it when we delete the interface definition (and delete checkMD5Password then as well). I think the only annoying thing about this is that to avoid a deprecation warning when calling checkPassword the implementation will have to shift around a bit.

comment:12 Changed 4 years ago by exarkun

Errgh. In responding to a recent post on tpml about this code, I realized that IUsernameHashedPassword and IUsernameMD5Password actually have a difference. IUsernameMD5Password.checkMD5Password accepts an MD5 digest of a password, while IUsernameHashedPassword.checkPassword accepts a plaintext password. Thus, if you don't want to store plaintext passwords on your server, IUsernameMD5Password is a better choice.

I think was misled by the documentation for IUsernameMD5Password which incorrectly claims that it accepts a plaintext password just like IUsernameHashedPassword.

comment:13 Changed 4 years ago by jesstess

  • Summary changed from Deprecate and then delete twisted.spread.pb.IUsernameMD5Password to twisted.spread.pb.IUsernameMD5Password's docstring should say it accepts an MD5 digest of a password, not plaintext
  • Type changed from task to defect

comment:14 Changed 4 years ago by jesstess

  • Branch changed from branches/pb-md5-deprecate-3056 to branches/pb-md5-docstring-3056

(In [28225]) Branching to 'pb-md5-docstring-3056'

comment:15 Changed 4 years ago by jesstess

(In [28226]) checkMD5Password should say it works on a digest, not plaintext. Also,
the @return markup on a number of interfaces extending ICredentials
is hopefully now less confusing.

refs #3056

comment:16 Changed 4 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

comment:17 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to jesstess

Looks good. Please merge. Thanks!

comment:18 Changed 4 years ago by jesstess

  • Resolution set to fixed
  • Status changed from new to closed

(In [28247]) Merge pb-md5-docstring-3056

Author: jesstess
Reviewer: exarkun
Fixes: #3056

IUsernameMD5Password.checkMD5Password now correctly say it works on a
digest, not plaintext. Also, the @return description on a number of
interfaces extending ICredentials has been clarified.

comment:19 Changed 3 years ago by <automation>

  • Owner jesstess deleted
Note: See TracTickets for help on using tickets.