Better BasicAuth tuple

[kxepal]

No description provided.

[fafhrd91]

i think you should add .encode() method to BasicAuth

[fafhrd91]

also let move it to helpers.py

[kxepal]

OK for move to helpers.

As about .encode() method I'll repeat myself:

mmm...it will be only useful if we'll raise an exception instead of warning when non BasicAuth instance arrives like it happens for ProxyConnector.

So should I replace warning with TypeError/assertion or let the warning be and just wrap auth with BasicAuth before .encode() call? Otherwise this method would make no much sense.

[fafhrd91]

if auth is not BaseAuth, create new BasicAuth with *auth params
and then do:
self.headers['AUTHORIZATION'] = BasucAuth.encode()

everything else should go to .encode() method.

[kxepal]

@fafhrd91 done. I was a bit upset by auth.login is not None or auth.password is not None condition since better to have this inside encode and raise ValueError out there, but this cause proxy auth override test failure since it cannot override it by using None credentials /:

Don't have any good idea how to handle this better for now.

[fafhrd91]

1. BasicAuth.password should be optional, then update_host could be simplified.
2. move auth.login checks and ValueError into .encode(), i think encode() should return complete header value and not just encoded login,password

[kxepal]

Good point about update_host() simplification.

About checks there is a problem: if we move all the checks inside .encode() then we make test_proxy_override_auth fail since it uses (None, None) to unset Authorization header. So we have to check for auth.login from outside and check for both login and password inside to raise proper ValueError.

[fafhrd91]

then we should fix test. test is just test.
i think None is not valid as login at all, we probably should add None to BasicAuth.init
or do not check it at all.

In any case BasicAuth(None, None) doesnt make any sense.

[kxepal]

AFAIS it uses as the way to override credentials specified in url. Some kind sentinel value to tell ClientRequest that it should ignore credentials which it extracted from url before.

[fafhrd91]

@asvetlov @popravich is BasicAuth(None, None) is valid use case for proxy connector?
lets create sentinel object for that if it is real use case, something like DROP_BASIC_AUTH=object()

[popravich]

No, BasicAuth(None, None) is invalid use case. The test_proxy_override_auth test there just to be sure that it does what it does (ie drops auth).
When BasicAuth was implemented there was bad check for login/password pair and thus BasicAuth(None, None) was a valid auth. At that moment I had no time to fix that issue in full scope so the earlier test appeared.

I think BasicAuth with None's should be invalid and maybe raise error.
As for sentinel object -- I and @asvetlov, we don't have use cases now to forcibly drop auth.

[lock]

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.