Merge lp:~mbp/bzr/654684-password into lp:bzr

[Martin Pool]

avoid "KeyError: port" in urllib authentication callback

This is a somewhat shallow fix for bug 654684, but it does add a narrow unit test.

It seems that sometimes urllib2 doesn't give us back a 'port' entry in the authentication callback: I would guess this is when there is no port in the url. This just copes with that and adds a test that we do.

Possibly this indicates a missing test at a different level that could provoke urllib2 to give this behaviour. It might be hard to write that test because we can't listen on port 80 in a test to simulate the situation. (Maybe someone can think of a creative way to hit it.)

[Vincent Ladeuil (vila)]

I'm slightly worried that there could other occurrences of the same problem, but since port=None is handled in the rest of the stack, I see no reason to be pedantic either.

Nice unit tests by the way.

review: Approve

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-10-12 06:18:54 +0000
+++ NEWS	2010-10-12 07:29:40 +0000
@@ -28,6 +28,10 @@
 * Don't force openssh to use protocol=2, since that is now the default.
   (Neil Martinsen-Burrell, #561061)
 
+* Fix ``KeyError: 'port'`` when getting the stored password for an http
+  URL.
+  (Martin Pool, #654684)
+
 * Make ``bzr tag --quiet`` really quiet. (Neil Martinsen-Burrell, #239523)
 
 Improvements
 
=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py	2010-10-11 22:40:23 +0000
+++ bzrlib/tests/test_http.py	2010-10-12 07:29:40 +0000
@@ -1604,11 +1604,11 @@
         ui.ui_factory = tests.TestUIFactory(stdin=stdin_content,
                                             stderr=tests.StringIOWrapper())
         # Create a minimal config file with the right password
-        conf = config.AuthenticationConfig()
-        conf._get_config().update(
-            {'httptest': {'scheme': 'http', 'port': self.server.port,
-                          'user': user, 'password': password}})
-        conf._save()
+        _setup_authentication_config(
+            scheme='http', 
+            port=self.server.port,
+            user=user,
+            password=password)
         # Issue a request to the server to connect
         self.assertEqual('contents of a\n',t.get('a').read())
         # stdin should have  been left untouched
@@ -1616,25 +1616,6 @@
         # Only one 'Authentication Required' error should occur
         self.assertEqual(1, self.server.auth_required_errors)
 
-    def test_user_from_auth_conf(self):
-        if self._testing_pycurl():
-            raise tests.TestNotApplicable(
-                'pycurl does not support authentication.conf')
-        user = 'joe'
-        password = 'foo'
-        self.server.add_user(user, password)
-        # Create a minimal config file with the right password
-        conf = config.AuthenticationConfig()
-        conf._get_config().update(
-            {'httptest': {'scheme': 'http', 'port': self.server.port,
-                          'user': user, 'password': password}})
-        conf._save()
-        t = self.get_user_transport(None, None)
-        # Issue a request to the server to connect
-        self.assertEqual('contents of a\n', t.get('a').read())
-        # Only one 'Authentication Required' error should occur
-        self.assertEqual(1, self.server.auth_required_errors)
-
     def test_changing_nonce(self):
         if self._auth_server not in (http_utils.HTTPDigestAuthServer,
                                      http_utils.ProxyDigestAuthServer):
@@ -1656,6 +1637,56 @@
         # initial 'who are you' and a second 'who are you' with the new nonce)
         self.assertEqual(2, self.server.auth_required_errors)
 
+    def test_user_from_auth_conf(self):
+        if self._testing_pycurl():
+            raise tests.TestNotApplicable(
+                'pycurl does not support authentication.conf')
+        user = 'joe'
+        password = 'foo'
+        self.server.add_user(user, password)
+        _setup_authentication_config(
+            scheme='http', 
+            port=self.server.port,
+            user=user,
+            password=password)
+        t = self.get_user_transport(None, None)
+        # Issue a request to the server to connect
+        self.assertEqual('contents of a\n', t.get('a').read())
+        # Only one 'Authentication Required' error should occur
+        self.assertEqual(1, self.server.auth_required_errors)
+
+
+def _setup_authentication_config(**kwargs):
+    conf = config.AuthenticationConfig()
+    conf._get_config().update({'httptest': kwargs})
+    conf._save()
+
+
+
+class TestUrllib2AuthHandler(tests.TestCaseWithTransport):
+    """Unit tests for glue by which urllib2 asks us for authentication"""
+
+    def test_get_user_password_without_port(self):
+        """We cope if urllib2 doesn't tell us the port.
+
+        See https://bugs.launchpad.net/bzr/+bug/654684
+        """
+        user = 'joe'
+        password = 'foo'
+        _setup_authentication_config(
+            scheme='http', 
+            host='localhost',
+            user=user,
+            password=password)
+        handler = _urllib2_wrappers.HTTPAuthHandler()
+        got_pass = handler.get_user_password(dict(
+            user='joe',
+            protocol='http',
+            host='localhost',
+            path='/',
+            realm='Realm',
+            ))
+        self.assertEquals((user, password), got_pass)
 
 
 class TestProxyAuth(TestAuth):
 
=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
--- bzrlib/transport/http/_urllib2_wrappers.py	2010-10-08 10:50:51 +0000
+++ bzrlib/transport/http/_urllib2_wrappers.py	2010-10-12 07:29:40 +0000
@@ -1202,15 +1202,17 @@
         user = auth.get('user', None)
         password = auth.get('password', None)
         realm = auth['realm']
+        port = auth.get('port', None)
 
         if user is None:
             user = auth_conf.get_user(auth['protocol'], auth['host'],
-                                      port=auth['port'], path=auth['path'],
+                                      port=port, path=auth['path'],
                                       realm=realm, ask=True,
                                       prompt=self.build_username_prompt(auth))
         if user is not None and password is None:
             password = auth_conf.get_password(
-                auth['protocol'], auth['host'], user, port=auth['port'],
+                auth['protocol'], auth['host'], user,
+                port=port,
                 path=auth['path'], realm=realm,
                 prompt=self.build_password_prompt(auth))