[MERGE][bug #300347] Don't require "user@" in HTTP(S) URLs that need auth
Vincent Ladeuil
v.ladeuil+lp at free.fr
Thu Dec 25 11:51:01 GMT 2008
>>>>> "Ben" == Ben Jansen <aogail at w007.org> writes:
Ben> On Fri, 19 Dec 2008 14:01:09 -0500 (EST), John Arbash Meinel
Ben> <john at arbash-meinel.com> wrote:
>> John Arbash Meinel has voted tweak.
Ben> I've taken the liberty of updating Vincent's patch to my
Ben> original patch with your suggestions.
Great and well done, I'll merge it asap.
>> Status is now: Conditionally approved
>> Comment:
>> + def test_user_from_auth_conf(self):
>> + if self._testing_pycurl():
>> + raise tests.TestNotApplicable(
>> + 'pycurl does not support authentication.conf')
>> + user =' joe'
>>
>> ^- This looks like a typo, and you really want:
>> user = 'joe'
>>
>> Though perhaps you wanted to test user names that start with a space?
>> user = ' joe'
Ben> Fixed.
>> + 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()
>> + # Issue a request to the server to connect
>> + self.assertEqual('contents of a\n',t.get('a').read())
>>
>> ^- a space here after ',' would be good.
Ben> Fixed.
>> Doing "get_user_transport(user=None) would be a bit clearer *to me* that
>> you
>> are ensuring the URL didn't include a known user. Especially since the
>> name of
>> the function is "..._user_..." I went around and checked that it
>> actually was
>> doing the right thing. It may not be true for others, though.
Ben> I left this as-is. If anyone has a strong opinion they can object. ;)
>> I find this rather confusing:
>> def get_user_password(self, auth):
>> """Ask user for a password if none is already available."""
>> auth_conf = config.AuthenticationConfig()
>> user = auth['user']
>> password = auth['password']
>> realm = auth['realm']
>>
>> What is "auth" versus "auth_conf".
Ben> [snip]
>> Perhaps just documenting what the "auth" parameter is, and then maybe
>> adding a
>> comment about how "auth_conf" differs from "auth". (Then again, maybe
>> I'm only
>> confused because I don't know the code well enough.)
Ben> I added a description of the "auth" parameter to the comment for
Ben> get_user_password().
>> Or perhaps the confusion is because whatever "auth" is, it somehow has
>> the
>> property that:
>>
>> auth['user'] can be None, but "auth.get_user(...)" can return something
>> useful,
>> but apparently not as useful as "auth_conf.get_user(...)".
Ben> Well, this statement is actually incorrect. The function
Ben> auth.get_user() does not exist. In fact, before this patch,
Ben> that line was never executed because of a conditional higher up
Ben> in the call stack. I guess that at one point auth.get_user()
Ben> was valid, then the conditional was added and the code went
Ben> stale.
Ben is definitely right here, this was dead code, untested code, broken
code.
Ben> So, the logic is: if auth['user'] is None, attempt to get a user name out
Ben> of authentication.conf.
Exactly.
Yet, if authentication.conf can't provide a user either, don't try to
authenticate.
A future version will prompt for a user, but we're not there yet.
Ben> - Ben
Ben> # Bazaar merge directive format 2 (Bazaar 0.90)
Ben> # revision_id: aogail at w007.org-20081223231216-2wt2wokz59af10cf
Ben> # target_branch: lp:bzr
Ben> # testament_sha1: e6984974a7256c6154d9fa8ea28130b65d07c28d
Ben> # timestamp: 2008-12-23 15:14:58 -0800
Ben> # base_revision_id: pqm at pqm.ubuntu.com-20081217102138-pz7pfli9o3k50zq7
Ben> #
Ben> # Begin patch
Ben> === modified file 'NEWS'
Ben> --- NEWS 2008-12-17 10:21:38 +0000
Ben> +++ NEWS 2008-12-19 10:37:42 +0000
Ben> @@ -46,6 +46,9 @@
Ben> that's in a stackable shared repository to a location with default
Ben> stack-on location. (Andrew Bennetts, #291046)
Ben> + * Don't require embedding user in HTTP(S) URLs do use authentication.conf.
Ben> + (Ben Jansen, Vincent Ladeuil, #300347)
Ben> +
Ben> * Fix compilation error in ``_dirstate_helpers_c`` on SunOS/Solaris.
Ben> (Jari Aalto)
Ben> === modified file 'bzrlib/tests/test_http.py'
Ben> --- bzrlib/tests/test_http.py 2008-12-04 16:02:51 +0000
Ben> +++ bzrlib/tests/test_http.py 2008-12-23 23:12:16 +0000
Ben> @@ -1557,6 +1557,25 @@
Ben> # Only one 'Authentication Required' error should occur
Ben> self.assertEqual(1, self.server.auth_required_errors)
Ben> + def test_user_from_auth_conf(self):
Ben> + if self._testing_pycurl():
Ben> + raise tests.TestNotApplicable(
Ben> + 'pycurl does not support authentication.conf')
Ben> + user = 'joe'
Ben> + password = 'foo'
Ben> + self.server.add_user(user, password)
Ben> + # Create a minimal config file with the right password
Ben> + conf = config.AuthenticationConfig()
Ben> + conf._get_config().update(
Ben> + {'httptest': {'scheme': 'http', 'port': self.server.port,
Ben> + 'user': user, 'password': password}})
Ben> + conf._save()
Ben> + t = self.get_user_transport()
Ben> + # Issue a request to the server to connect
Ben> + self.assertEqual('contents of a\n', t.get('a').read())
Ben> + # Only one 'Authentication Required' error should occur
Ben> + self.assertEqual(1, self.server.auth_required_errors)
Ben> +
Ben> def test_changing_nonce(self):
Ben> if self._auth_scheme != 'digest':
Ben> raise tests.TestNotApplicable('HTTP auth digest only test')
Ben> === modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
Ben> --- bzrlib/transport/http/_urllib2_wrappers.py 2008-11-11 00:57:17 +0000
Ben> +++ bzrlib/transport/http/_urllib2_wrappers.py 2008-12-23 23:12:16 +0000
Ben> @@ -965,10 +965,6 @@
Ben> raise KeyError('%s not found' % self.auth_required_header)
Ben> auth = self.get_auth(request)
Ben> - if auth.get('user', None) is None:
Ben> - # Without a known user, we can't authenticate
Ben> - return None
Ben> -
Ben> auth['modified'] = False
Ben> if self.auth_match(server_header, auth):
Ben> # auth_match may have modified auth (by adding the
Ben> @@ -978,6 +974,10 @@
Ben> # We already tried that, give up
Ben> return None
Ben> + if auth.get('user', None) is None:
Ben> + # Without a known user, we can't authenticate
Ben> + return None
Ben> +
Ben> # Housekeeping
Ben> request.connection.cleanup_pipe()
Ben> response = self.parent.open(request)
Ben> @@ -1039,21 +1039,20 @@
Ben> self._retry_count = None
Ben> def get_user_password(self, auth):
Ben> - """Ask user for a password if none is already available."""
Ben> + """Ask user for a password if none is already available.
Ben> +
Ben> + :param auth: authentication headers from the HTTP response
Ben> + """
Nit-picking 'auth' comes from the request (user, pass) and can be
updated with parts from the response (auth scheme, realm).
It represents the data used in the authentication dialog between the
server and the client.
By contrast, auth_conf *is* authentication.conf.
Ben> auth_conf = config.AuthenticationConfig()
Ben> user = auth['user']
Ben> password = auth['password']
Ben> realm = auth['realm']
Ben> if user is None:
Ben> - user = auth.get_user(auth['protocol'], auth['host'],
Ben> - port=auth['port'], path=auth['path'],
Ben> - realm=realm)
Ben> - if user is None:
Ben> - # Default to local user
Ben> - user = getpass.getuser()
Ben> -
And yes we get rid of defaulting to getpass.getuser() without concern
for backward compatibility because.... as Ben demonstrated, this was
dead code and was never respected. Since that wasn't making a lot of
sense anyway, I'm happy to get rid of it.
Ben> - if password is None:
Ben> + user = auth_conf.get_user(auth['protocol'], auth['host'],
Ben> + port=auth['port'], path=auth['path'],
Ben> + realm=realm)
Ben> + if user is not None and password is None:
Ben> password = auth_conf.get_password(
Ben> auth['protocol'], auth['host'], user, port=auth['port'],
Ben> path=auth['path'], realm=realm,
Vincent
More information about the bazaar
mailing list