launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/apparently-we-support-basic-auth into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/apparently-we-support-basic-auth/+merge/55845

Summary
=======

Launchpad sort of supports basic auth, but sending badly encoded or unencoded authentication headers results in a 500 error on the client side and an OOPS on our side about base 64 encoding. If you send lousy authentication headers the correct response is simply stating that your still unauthorized, which is an exception conveniently ignored in our OOPS reporting.

Preimp
======

Spoke with Robert Collins about lp's basic auth support.

Implementation
==============

In authenticate(), we catch the base64 encoding error; since this is a result of bad data, we raise Unauthorized. Unauthorized is on the list of exceptions our oops reports ignore, as long as it's coming from an unauthenticated user.

Tests
=====

bin/test -m canonical.launchpad.webapp

No new tests were added, b/c to test this particular error case requires an actual fully fledged independently running lp instance (.dev or otherwise). None of the layers properly provide that, and out TestRequest machine can't simulate what's actually going on. It's a small change though, so the risk is minimal.

If my assertion is wrong and you can provide a way to test this, I'm happy to implement it. More than happy, actually.

QA
==

Try to make a request with urllib2 and an uncoded email:password Basic auth string. You should get an Unauthorized resonse instead of Server Error.

Lint
====

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/webapp/authentication.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/apparently-we-support-basic-auth/+merge/55845
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/apparently-we-support-basic-auth into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/authentication.py'
--- lib/canonical/launchpad/webapp/authentication.py	2011-02-04 14:41:18 +0000
+++ lib/canonical/launchpad/webapp/authentication.py	2011-03-31 22:08:39 +0000
@@ -30,6 +30,7 @@
 from zope.event import notify
 from zope.interface import implements
 from zope.preference.interfaces import IPreferenceGroup
+from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 from zope.session.interfaces import ISession
 
@@ -121,10 +122,15 @@
         # To avoid confusion (hopefully), basic auth trumps cookie auth
         # totally, and all the time.  If there is any basic auth at all,
         # then cookie auth won't even be considered.
-
         # XXX daniels 2004-12-14: allow authentication scheme to be put into
         #     a view; for now, use basic auth by specifying ILoginPassword.
-        credentials = ILoginPassword(request, None)
+        try:
+            credentials = ILoginPassword(request, None)
+        except binascii.Error:
+            # We have probably been sent Basic auth credentials that aren't
+            # encoded properly. That's a client error, so we don't really
+            # care, and we're done.
+            raise Unauthorized("Bad Basic authentication.")
         if credentials is not None and credentials.getLogin() is not None:
             return self._authenticateUsingBasicAuth(credentials, request)
         else: