Opened 13 years ago

Closed 13 years ago

Last modified 3 years ago

#1475 enhancement closed fixed (fixed)

Basic and Digest HTTP-Auth Implementation.

Reported by: David Reid Owned by:
Priority: normal Milestone:
Component: web2 Keywords:
Cc: Glyph, David Reid, Wilfredo Sánchez Vega, oubiwann, ed@… Branch:
Author:

Description


Attachments (1)

authentication.xhtml.diff (3.2 KB) - added by edsuom 13 years ago.
Patch with some readability and stylistic edits

Download all attachments as: .zip

Change History (25)

comment:1 Changed 13 years ago by David Reid

Twisted currently has a server side HTTP-AUTH implementation in 
twisted.protocols.sip, this should be cleaned up and moved to a more approprate 
location, possibly twisted.python.digest or twisted.cred.digest

comment:2 Changed 13 years ago by David Reid

Branched to dreid/http-auth-1475

comment:3 Changed 13 years ago by Glyph

Since it's auth, cred seems more appropriate to me.

comment:4 Changed 13 years ago by David Reid

Currently it's checked in as twisted/web2/httpauth.py there was some question as
to it's general usefulness and applicability to other protocols (such as SMTP
and IMAP DIGEST-MD5) and SIP.  I personally don't see any reason why there
should be any problems, except that SIP seems a little more liberal with certain
parts of the response, and SMTP and IMAP require that challenges and responses
be base64 encoded.  All of which seem to be able to be provided by subclasses of
the provided api.

The one big problem is the ICredentialFactory (both in name and what it does)
unfortunately it is a necessary object for doing http as it serves the purpose
of remembering challenges across requests.

comment:5 Changed 13 years ago by David Reid

Component: coreweb2
Owner: changed from David Reid to jknight
Priority: lownormal
Summary: Consolidate and clean up server side HTTP-AUTH implementationsBasic and Digest HTTP-Auth Implementation.

Merged forward and reorganized in http-auth-1475-2, there is still some question as to wether or not the appropriately reusable parts should be pulled out of web2, so i'm shelving that issue for a later time. Reassigning for review.

comment:6 Changed 13 years ago by David Reid

Keywords: review added

comment:7 Changed 13 years ago by jknight

How do you set up a server which supports more than one kind of authentication (e.g. accepts either digest or basic?)

comment:8 Changed 13 years ago by David Reid

Currently you don't. Atleast not with the same http auth wrapper. I wanted it to be as simple as possible and didn't deam multiple credential factories a worthwhile effort (at that time.) Though now that I look at the implementation it should be reasonably simple.

comment:9 Changed 13 years ago by Wilfredo Sánchez Vega

Cc: Wilfredo Sánchez Vega added

comment:10 Changed 13 years ago by oubiwann

Cc: oubiwann added

comment:11 Changed 13 years ago by radix

Keywords: review removed
Owner: changed from jknight to David Reid
  • I notice there is some commented-out Auth code in http.py. now that your code obsoletes the ideas behind it, it should be deleted.
  • ICredentialFactory.getChallenge is documented as requiring a 'str' return value, but both implementations return (scheme, {...}).
  • ICredentialFactory.decode should probably be documented as able to raise error.LoginFailed.
  • I don't think it's worth pretending DigestCalcResponse and DigestCalcHA1 are classes (I assume that's what the intenion behind their capitalization was).
  • Shouldn't DigestCredentialFactory.outstanding have some occasional cleanup? Probably no need for a reactory-timer, just a check in getChallenge or decode to see if it's been so long since an item was added.
  • I am slightly unhappy about checking for iter on the credentialFactories object and wrapping it in a list if it doesn't have one. But that is not a big deal.

In wrapper.py:

        self.credentialFactories = dict([(factory.scheme, factory) \
                                         for factory in credentialFactories])

  • Man, you don't need no backslash! That is so C. Does C even require that? I don't know.
  • What would make this branch so totally hot and awesome is if it added an example to the documentation.

comment:12 Changed 13 years ago by edsuom

Cc: ed@… added

I'd really like to see this finished and approved.

comment:13 Changed 13 years ago by oubiwann

Owner: changed from David Reid to oubiwann

I'm going to work on the docs, and then I'll pass it back to dreid.

comment:14 Changed 13 years ago by Wilfredo Sánchez Vega

Owner: changed from oubiwann to David Reid

comment:15 Changed 13 years ago by Wilfredo Sánchez Vega

Owner: changed from David Reid to oubiwann

oops

comment:16 Changed 13 years ago by David Reid

Owner: changed from oubiwann to David Reid

comment:17 Changed 13 years ago by David Reid

Keywords: review added

comment:18 Changed 13 years ago by PenguinOfDoom

Typo in authentication.xhtml -- s/propogate/propagate/

comment:19 Changed 13 years ago by PenguinOfDoom

Unauthorized.Resource.init docstring describes parameter wwwAuthenticate, but init takes "factories" as the only argument.

comment:20 Changed 13 years ago by PenguinOfDoom

ICredentialFactory.getChallenge should probably take the request as the argument. The request includes the peer address as well as any other information a hypothetical authentication method might need.

Changed 13 years ago by edsuom

Attachment: authentication.xhtml.diff added

Patch with some readability and stylistic edits

comment:21 Changed 13 years ago by David Reid

Owner: changed from David Reid to PenguinOfDoom

comment:22 Changed 13 years ago by David Reid

Resolution: fixed
Status: newclosed

(In [16848]) Merge http-auth-1475-3

Author: dreid Review: PenguinOfDoom Fixes #1475

RFC 2617 implementation of Basic and Digest authentication, with unittests and documentation.

comment:23 Changed 8 years ago by <automation>

Owner: PenguinOfDoom deleted

comment:24 Changed 3 years ago by hawkowl

Cc: glyph, dreid, wsanchez,oubiwann, ed@eepatents.comglyph, dreid, wsanchez, oubiwann, ed@eepatents.com
Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.