Added a digest authentication helper

[feus4177]

What do these changes do?

Added a digest authentication helper.

Are there changes in behavior for the user?

None

Related issue number

Resolves #4939

Checklist

• [x] I think the code is well written
• [x] Unit tests for the changes exist
• [x] Documentation reflects the changes
• [x] If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• [x] Add a new news fragment into the changes folder
  • name it <issue_id>.<type> for example (588.bug)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

I didn't go through the checklist. I just wanted to see if this is something you are interested in incorporating into the aiohttp package. To be able to use this DigestAuth helper like the aiohttp BasicAuth helper would take some restructuring so I decided to keep it as isolated as possible right now. Let me know what you think and I can make any changes as necessary.

[asvetlov]

Sorry, I don't follow how to use the helper. Full test coverage is required also.

[feus4177]

Sorry, I meant to add this snippet as well.

import aiohttp
import asyncio

client = aiohttp.ClientSession()
auth = aiohttp.auth.DigestAuth('usr', 'psswd', client)

async def fetch(url, **kwargs):
    return await auth.request('GET', url, **kwargs)


loop = asyncio.get_event_loop()
response = loop.run_until_complete(fetch('http://httpbin.org/digest-auth/auth/usr/psswd/MD5/never'))

[feus4177]

Obviously, I can add tests, documentation, etc. Before I go through all that though, I just wanted to see if there is any interest in it.

[asvetlov]

Well, idea is good but it should be proved by test suite.

[oleksandr-kuzmenko]

This is a good idea! But I think it would be better to add digest authentication support to standard client, something like:

                                     |
                                     v
async with aiohttp.ClientSession(digest_auth=...) as session:
    async with session.get('https://api.github.com/events') as resp:
        print(resp.status)

[kxepal]

@alxpy Yeap, that's looks simpler for users. But it will overload ClientSession signature too much. Because digest and basic auths are not the only around. Having sort-of decorators / middlewares instead gives you more control: you can implement own auth method like this without touching aiohttp sources. And aiohttp developers wouldn't have to maintain such a jack of all trades object.

[oleksandr-kuzmenko]

@kxepal yes, that makes sense, I agree

[feus4177]

That was the kind of discussion I was looking for. It sounds like you guys would prefer to keep it separated like it currently is. I'll go ahead and add the tests and documentation and then update the pull request. Thanks.

[feus4177]

So I updated the pull request to following the contributing guidelines (test, docs, etc.). Since it has been a while I also went ahead and merged the latest from aio-libs/aiohttp:master into the pull request. It looks like you guys switched over to the async/await syntax so I did that as well. However, half of all the tests are failing now. It looks like it's that way for all the tests though, so I'm assuming it isn't an issue. Let me know if there is anything else you want me to do.

[asvetlov]

Tests still are red :(

[codecov-io]

Codecov Report

Merging #2213 into master will increase coverage by 0.41%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2213      +/-   ##
==========================================
+ Coverage   97.16%   97.57%   +0.41%     
==========================================
  Files          40       40              
  Lines        8137     8136       -1     
  Branches     1438     1425      -13     
==========================================
+ Hits         7906     7939      +33     
+ Misses         94       75      -19     
+ Partials      137      122      -15

Impacted Files	Coverage Δ
aiohttp/helpers.py	98.19% <100%> (+0.42%)	:arrow_up:
aiohttp/payload.py	98.71% <0%> (-0.14%)	:arrow_down:
aiohttp/http.py	100% <0%> (ø)	:arrow_up:
aiohttp/test_utils.py	98.56% <0%> (ø)	:arrow_up:
aiohttp/client_reqrep.py	97.22% <0%> (+0.03%)	:arrow_up:
aiohttp/web_response.py	98.25% <0%> (+0.05%)	:arrow_up:
aiohttp/http_websocket.py	98.59% <0%> (+0.11%)	:arrow_up:
aiohttp/http_parser.py	98.04% <0%> (+0.24%)	:arrow_up:
aiohttp/streams.py	99.45% <0%> (+0.4%)	:arrow_up:
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fab06f2...6eac203. Read the comment docs.

[feus4177]

When I merged your master in I made the mistake of leaving the test_dummy_cookie_jar test in when I was fixing the merge conflicts. Removing it appears to have fixed the tests.

[asvetlov]

Sorry, wrote comments two days ago but forgot to press "Submit" button.

[asvetlov]

The code is written good and well tested, but let's discuss the usage design. Now the API is:

session = aiohttp.ClientSession()
auth = aiohttp.DigestAuth('user', 'password', client)
resp = await auth.request('get', '/')

The API is not very handy:

1. async with auth.request('get', url) as resp: is not supported
2. await auth.get(url) is not supported 3 async with client.get(url, auth=aiohttp.DigestAuth(...) is not supported

We could fix 1. and 2. easy but it doesn't make a value. What is really desirable is accepting DigestAuth as auth parameter -- like requests does 1 Later we can add support for OAuth and other authorization protocols. The approach requires inviting an abstract interface for auth classes and supporting it by both BasicAuth and DigestAuth.

@feus4177 what do you think about?

[feus4177]

The difficult part is that for digest auth there is potentially a second request that needs to be launched, and the whole point of the helper is so that you don't have to worry about handling that additional request. If you want to be able to do use the client.get(url, auth=aiohttp.DigestAuth...) syntax, the easiest way to go about it would be to wrap the aiohttp.ClientSession._request method. This way, the digest auth can make multiple calls to the underlying _request method.

This is kind of what I had been asking about earlier though. @alxpy had mentioned doing aiohttp.ClientSession(digest_auth=...) but I thought it sounded like you guys didn't like that. I think for the users client.get(url, auth=aiohttp.DigestAuth...) is the best option but I think it will be a little messier to implement. At the end of the day though, I'm fine with however you guys want to do it.

[dadocsis]

I am wrapping up pr to implement custom auth handlers similar to the requests package. I am also adding a response event hook where the auth handler could be registered to handle a 401. So in this case the usage could be:

auth = aiohttp.DigestAuth('user', 'password', client) session = aiohttp.ClientSession(auth=auth) resp = await auth.request('get', '/')

Does this make sense?

[dadocsis]

See issue 434 https://github.com/aio-libs/aiohttp/issues/434

[feus4177]

In your case, wouldn't it be resp = await session.request('get', '/')?

I referenced the requests library pretty heavily when I was creating this feature and they use event hooks so I think it would fit in well. In my mind we would want event hooks directly before and after the request, and then some mechanism to retry the request. The before hook would help in the case of digest auth because then we could implement the auth-int quality of protection which you need the request body entity to perform.

[dadocsis]

Correct. Right now I am just adding a response hook. Looking at the Digest Auth class I do not see where it needs the "before" hook. The response hook would pass the original request to the handler (I mistyped in my original comment about passing in a response) It makes more sense to pass in the request because from the request you will have access to the response and client session. The "_handle_401" could be registered as the handler and should fit in nicely.

[feus4177]

The before hook would be needed to add support for the auth-int quality of protection. Right now, if they server specifies auth-int for qop, we raise an error. But if we were to add support for it, you would need the before hook.

[RouquinBlanc]

Hi guys,

What do you think the status of this pull request is and do you have a rough idea if it is going to be ready any time soon-ish? It seems quite close from an external eye...

At the moment AFAIK there is no pure asyncio framework supporting digest authentication, you have to either go back to tornado or to give up on asyncio and look at requests...

Thanks!

[asvetlov]

I like the idea but have a feeling that we need to use slightly different and not existing yet API for it.

[DavHau]

At the moment AFAIK there is no pure asyncio framework supporting digest authentication, you have to either go back to tornado or to give up on asyncio and look at requests...

@RouquinBlanc This is not true. There is: Pulsar: https://github.com/quantmind/pulsar (file upload in combination with DigestAuth is broken at the time of writing) or yieldfromrequests: https://github.com/rdbhost/yieldfromrequests (not updated since 4 years, but works for me. Only usable with old generator style async syntax. write yourself a wrapper ;) )

I'm also really looking forward to have DigestAuth available in aiohttp since it is my favorite async http lib.

[sonicblind]

@asvetlov I found a small bug related to Digest Auth Header in helpers.py.

The Digest header can include spaces in realm name, for example: Digest realm="iMC RESTful Web Services", qop="auth", nonce="MTU0....=="

My suggestion as per below:

def parse_key_value_list(header):
    return {
        key: value for key, value in
#       [parse_pair(header_pair) for header_pair in header.split(' ')]
        [parse_pair(header_pair) for header_pair in header.split(', ')]
    }

But this is probably also not bulletproof as the realm could include also comma. So maybe header.split('", ') would work even better.

And if the above make sense, then the "remove trailing comma" section in parse_pair(pair) should also be updated accordingly:

def parse_pair(pair):
    key, value = pair.split('=', 1)

    # If it has a trailing comma, remove it.
#   if value[-1] == ',':
#       value = value[:-1]

    # If it is quoted, then remove them.
    if value[0] == value[-1] == '"':
        value = value[1:-1]
    
    return key, value

[asvetlov]

@sonicblind thanks for the comment. The PR doesn't go to merge but I'd like to keep it open as a reference for eventual new implementation built from scratch.

[bdraco]

Thanks for working on this. @feus4177 @asvetlov Is there a shot that support for digest auth is forthcoming any time soon?

[kiknaio]

Still nothing new. I think it would be amazing if you complete this feature request.

[CLAassistant]

All committers have signed the CLA.

[skewty]

I still don't see aiohttp.helpers.DigestAuth so it would seem this never got merged. Can we get this merged?

[flashnuke]

This would be of tremendous value... any chance of merging?