Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
HTTP Digest authentication support #131
HTTP Digest authentication support #131
Changes from 8 commits
31197b8
fffaaca
0c5d729
1d3b18c
42d695d
e749665
a627f0b
aa8e102
2c1817e
91a5c7b
19f6ad8
1848e93
5651fc3
2e2e6d8
9e6e298
ffdbfa7
4bd52ff
a4dd9f2
05ecf65
4ae05dc
b9f4452
1283fea
03b784b
3515008
e0d7208
457c56a
f8c5e7f
dcc772d
f1e3222
00af1b7
8ca2274
7dba100
8150f5d
ec3e5c9
259a059
c5a6e86
f5a30ea
b87b767
d01bd08
73a68d3
b1ade85
d9a48a6
455605d
12c0739
4a8e463
99a0532
5269ff1
4cbd556
5359a2a
484b9ba
bcb946d
57769c9
b5f5d95
f833561
1c35956
9198250
e1cf128
580eb8a
92fd51c
ae9d582
ffa168e
7ab4927
fc145a4
c27e977
badf9fb
a688242
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably just use https://attrs.readthedocs.io/en/stable/
(see also: https://glyph.twistedmatrix.com/2016/08/attrs.html)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also shouldn't be public. Is this really a class variable? It looks like it's only accessed via the instance so it should probably be initialized in in
__init__
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I put this inside of init it appeared to no longer cache the authentication across requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
treq tries to avoid having global mutable state like this cache.
The reason for this is that with a global cache like this it is very easy to implement it in such a way that different execution contexts can clobber the cached data.
For example, let's say we have a service that posts payloads to user configured URLs with digest authentication.
If two different users of this service configure it to post to same URI with different credentials they 'https://foo.bar/baz' they will end up sharing the cache entry for
('POST', 'https://foo.bar/baz')
and one of them would never be able to authenticate.Rather than add the credentials to cache key though it'd be better if the caller of the treq request methods had to opt into the cache.
The only other place where there is some expectation of the top-level treq request functions would reuse state is for cookie storage where the user has to explicitly pass a CookieJar instance (instead of a dict) to have it reused. Perhaps this cache should be on the credentials object instead then?
Such that
it would also make sense to augment
treq.client.HTTPClient
to take auth as a parameter to init such that:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would appear a shared cache would reduce the amount of requests needed for how I'm using this from 6 to 4. I'm already formatting it with a shared auth like your "and share a cache." example in my automation script.
I'm using this with automation code that hits 3 separate URL's comprising 2 GET's and 1 POST that all can share the same credentials and cache.
I'm a little unsure of how I should go about refactoring this though, got any hints on where I should start?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like requests also put the shared state on the HTTPDigestAuth object (https://github.com/kennethreitz/requests/blob/master/requests/auth.py#L79).
Probably start with a couple of test cases that exercise the desired sharing behavior, and then start moving the digest_auth_cache and to the HTTPDigestAuth instance. Probably reasonable to put the header construction code there too. And just have the
_RequestDigestAuthenticationAgent
ask the credentials for the header value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This self (and the agent argument in
_build_digest_authentication_header
are redundant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None is the default default value of dict.get.