[MERGE][bug #300347] Don't require "user@" in HTTP(S) URLs that need auth (was Re: [MERGE] [bug #256612])

[Ben Jansen]

On Fri, 19 Dec 2008 14:01:09 -0500 (EST), John Arbash Meinel
<john at arbash-meinel.com> wrote:
> John Arbash Meinel has voted tweak.

I've taken the liberty of updating Vincent's patch to my original patch
with your suggestions.

> 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'

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.

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.

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".

[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.)

I added a description of the "auth" parameter to the comment for
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(...)".

Well, this statement is actually incorrect. The function auth.get_user()
does not exist. In fact, before this patch, that line was never executed
because of a conditional higher up in the call stack. I guess that at one
point auth.get_user() was valid, then the conditional was added and the
code went stale.

So, the logic is: if auth['user'] is None, attempt to get a user name out
of authentication.conf.

- Ben
-------------- next part --------------
A non-text attachment was scrubbed...
Name: httpauth.patch
Type: text/x-patch
Size: 8587 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20081224/013ded0e/attachment.bin