[MERGE] Better http basic auth handling

[John Arbash Meinel]

(Again, I tried to post this through BB, and it gave me a 500 Internal
Error).


So are you saying this doesn't fix any of the bugs? Just a step in that
direction? (It would be nice to reference things in NEWS, but if it
doesn't actually close them, I guess we don't mention them).

+class BasicAuthRequestHandler(TestingHTTPRequestHandler):
+    """Requires a basic authentification to process requests."""
+

^- authentication

v- Shouldn't this:

1) Fail with 401 even if tcs.auth != 'basic'? Otherwise you can't say
that you "Require Basic Auth". If it is just for handling basic auth
when the server is configured for it, then the doc string should be updated.

2) Check that "auth_header.startswith('Basic ')" before it removes it.
Otherwise it would seems that you could have a server request basic
auth, but the clients sends digest auth, etc.

+    def do_GET(self):
+        tcs = self.server.test_case_server
+        if tcs.auth == 'basic':
+            auth_header = self.headers.get('Authorization')
+            authorized = False
+            if auth_header:
+                coded_auth = auth_header[len('Basic '):]
+                user, password = coded_auth.decode('base64').split(':')
+                authorized = tcs.authorized(user, password)
+            if not authorized:
+                self.send_response(401)
+                self.send_header('www-authenticate',
+                                 'Basic realm="Thou should not pass"')
+                self.end_headers()
+                return
+
+        TestingHTTPRequestHandler.do_GET(self)
+

-- Extra newline here

+class AuthHTTPServer(HttpServer):
+    """AuthHTTPServer extends HttpServer with a dictionary of passwords"""
+
+    """AuthHTTPServer extends HttpServer with a dictionary of passwords"""
+
+    def __init__(self, request_handler, auth):
+        HttpServer.__init__(self, request_handler)
+        # No authentification is done by default
+        self.auth = auth
+        self.password_of = {}
+
+    def add_user(self, user, password):
+        self.password_of[user] = password
+
+    def authorized(self, user, password):
+        return self.password_of[user] == password

^- This will do bad things if 'user' doesn't exist. It should be:

expected_password = self.password_of.get(user, None)
return expected_password is not None and password == expected_password


+++ bzrlib/tests/test_http.py
@@ -24,6 +24,7 @@
 import select
 import socket
 import threading
+from cStringIO import StringIO

^- imports are sorted, and 'cString' comes before 'select'.

...

@@ -66,6 +71,7 @@
     )
 from bzrlib.transport.http._urllib import HttpTransport_urllib

+import bzrlib.ui

 class FakeManager(object):

^- This should be tight against "from bzrlib.transport" with 2 blank
lines before class FakeManager:

 from bzrlib.transport.http._urllib import HttpTransport_urllib
+import bzrlib.ui


 class FakeManager(object):

^- And I prefer "from bzrlib import ui" and a later "ui.ui_factory...."
since it prevents bugs where code does "bzrlib.module.foo" but
"bzrlib.module" hasn't been imported directly. Sometimes it will work,
but the more lazy imports we do, the more often it will fail.

Why were these tests needed (in test_ui.py):

             self.assertEqual(u'Hello \u1234 some\u1234: ',
                              ui.stdout.getvalue().decode('utf8'))
+            # stdin should be empty
+            self.assertEqual('', ui.stdin.readline())
         finally:
             pb.finished()


...
+            # urllib2 will be confused if it find
+            # authentification infos in the urls. So we handle
+            # them separatly. Note that we don't need to do that
+            # when cloning (as above) since the cloned base is
+            # already clean.

# urllib2 will be confused if it finds
# authentication info in the url. So we
# handle them separately.
# Note: We don't need to when cloning, because
#       it was already done.


+            self._auth = None # We have to wait the 401 to know

I think it would be clearer as "self._authentication_method" or maybe
just "self._auth_method". But when I first read it, I thought it was the
authentication token, not 'digest' versus 'basic'.

And the comment could be

# This will be set once the server tells us with a 401

...

+    def http_error_401(self, req, fp, code, msg, headers):
+        """Trap the 401 to gather the auth type."""
+        response = urllib2.HTTPBasicAuthHandler.http_error_401(self,
req, fp,
+                                                               code, msg,
+                                                               headers)
+        if response is not None:
+            req.auth = 'basic'
+
+        return response

^- It seems like you are trapping all 401 errors and setting the
required auth to basic auth. Shouldn't you first check that the actual
401 is basic and not digest? (If it is digest, you should error rather
than thinking you should supply basic auth).


This is looking pretty good. But I'd like to review it again before it
is merged. So +0 for now.

John
=:->