Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement BasicAuth decode classmethod. #744

Merged
merged 12 commits into from
May 23, 2016

Conversation

luhn
Copy link
Contributor

@luhn luhn commented Jan 19, 2016

If you can encode, you should be able to decode.

Loosely based off of https://github.com/rdegges/python-basicauth, which is in the public domain.

raise ValueError('Unknown authorization method %s' % split[0])
to_decode = split[1]
elif len(split) == 1:
to_decode = split[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this case exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because https://github.com/rdegges/python-basicauth implements it. Happy to remove it if you want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be better strict to the specification where auth scheme is mandatory. Unlikely Authorization: Zm9vOmJhcg== will be a valid basic auth header.

@luhn luhn force-pushed the basic-auth-decode branch from 337346e to 27a5feb Compare January 19, 2016 16:30
@luhn luhn force-pushed the basic-auth-decode branch from 27a5feb to ef0e9f7 Compare January 19, 2016 16:30
@asvetlov
Copy link
Member

I want have a real use case anyway.
What the purpose of added method?

@luhn
Copy link
Contributor Author

luhn commented Jan 19, 2016

So I can implement Basic Auth on my server.

auth = BasicAuth.decode(request.headers['Authorization'])
if not (auth.login == 'admin' and auth.password == 'password123'):
    raise HTTPForbidden(reason='Go away, hacker!')

@asvetlov
Copy link
Member

@luhn sounds good.
Would you implement Request.authorization property also? It may be similar to .if_modified_since https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/web_reqrep.py#L229

@luhn
Copy link
Contributor Author

luhn commented Jan 19, 2016

Done.

@asvetlov
Copy link
Member

Almost perfect from my side but BasicAuth.decode should be documented in docs/client_reference.rst and Request.authorization in docs/web_reference.rst.
Sorry, I don't want to use autodoc feature -- docstrings should be readable and relative short.
All autodoc-styled docstrings will be replaced eventually.

@kxepal are you ok with PR's functionality?

header = self.headers.get(_AUTHORIZATION)
if header is not None:
try:
return BasicAuth.decode(header)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the trick:

>>> header = 'basic aGVsbG860LzQuNGAIQ=='
>>> auth = BasicAuth.decode(header)
>>> auth.password
'миÑ\x80!
'>>> bytearray([ord(i) for i in auth.password]).decode('utf-8')
'мир!'

So, unfortunately, this method will work correctly until you stay in latin1 name space. If your users will be allowed to have unicode wide password, they'll have to reimplement it. Somehow you'll have to inject encoding here. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remain encoding-neutral and have BasicAuth store login and password as bytes rather than strings.

Or we could assume UTF-8, since it is fairly universal these days. Also, the latest update on HTTP Basic Auth defines a charset field, but UTF-8 is the only allowed value (seems silly), so UTF-8 doesn't seem that bad of an assumption.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luhn I would agree with that with happy, but you don't know how many services that works with the local national encoding by default (this includes latin1, cp1251, etc.) /:

The charset parameter you reference works only with the flow:

  1. Make request
  2. Receive HTTP 401 with WWW-Authenticate and that charset param
  3. Resend request with Authorization header where credentials respects previously mentioned charset

This means some interaction with the user and works perfectly with the browsers, but not with the libraries ):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood.

We could make authorization a method instead of a property and have an encoding argument?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suddenly, it seems so.

Or may be don't try to parse anything here and just return Authorization header as is. The more I'm thinking about the more I realize that to handle this header no matter for what auth scheme you need somehow interact with application config (for the encoding, secrets, hash algos etc.). The request eventually known nothing about these bits and it shouldn't. This will delegate parse task to user land where it's possible to do that correctly and without much hacks.

@asvetlov what do you think?

@kxepal
Copy link
Member

kxepal commented Jan 19, 2016

@asvetlov Basically, yes, but there are few edge cases. The Request.authorization is good idea and I wonder if it's possible to make it plug-able to allow users easily manage various auth scheme handlers for oauth, api tokens and else reusing Request API instead on implementing own helpers. Feature is definitely not for this PR, but I think in basics it can help to deal with annoying encoding bit.

@asvetlov
Copy link
Member

Ok. Let's implement .encode() only without .authorization.
It's really not obvious now how to process different auth schemes.
@luhn would you update the Pull Request?

@luhn
Copy link
Contributor Author

luhn commented May 3, 2016

Hi guys, sorry for the delay, forgot about this PR. Is this what you wanted?

@fafhrd91
Copy link
Member

@asvetlov @kxepal status?

header.

:param auth_header: The value of the ``Authorization`` header.
:type auth_header: str
Copy link
Member

@kxepal kxepal May 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use :param str auth_header: ... today, but aiohttp doesn't uses docstring in code, but separate files under doc/ directory.

@kxepal
Copy link
Member

kxepal commented May 23, 2016

LGFM except docs bit.

@luhn
Copy link
Contributor Author

luhn commented May 23, 2016

How's that?

@coveralls
Copy link

coveralls commented May 23, 2016

Coverage Status

Coverage decreased (-0.02%) to 98.196% when pulling 1f14c40 on luhn:basic-auth-decode into 024f6e1 on KeepSafe:master.

@kxepal
Copy link
Member

kxepal commented May 23, 2016

@luhn
Copy link
Contributor Author

luhn commented May 23, 2016

I don't understand what it is you wish me to do.

@luhn
Copy link
Contributor Author

luhn commented May 23, 2016

To clarify: I've already updated the docs in client_reference.rst and I'm unsure what further needs to be done.

@kxepal
Copy link
Member

kxepal commented May 23, 2016

@luhn mmm...it seems I was look on some old commit then. Sorry for confusion, everything looks good for me. +1

@fafhrd91 fafhrd91 merged commit bc165f1 into aio-libs:master May 23, 2016
@fafhrd91
Copy link
Member

Thanks!

@lock
Copy link

lock bot commented Oct 29, 2019

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.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants