Allow custom authorization mechanisms. #139

markrwilliams · 2016-09-25T22:38:57Z

The auth keyword argument now takes a callable accepts an IAgent and
returns an IAgent. The returned IAgent can then implement whatever
authorization scheme the human mind can imagine.

The auth keyword argument now takes a callable accepts an IAgent and returns an IAgent. The returned IAgent can then implement whatever authorization scheme the human mind can imagine.

markrwilliams · 2016-09-25T22:43:13Z

The goal of this PR is to allow authorization schemes like #131 to live outside of treq. #140 needs to be addressed before this can happen.

codecov-io · 2016-09-25T22:55:05Z

Codecov Report

Merging #139 into master will increase coverage by 0.17%.

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
+ Coverage   94.25%   94.42%   +0.17%     
==========================================
  Files          19       19              
  Lines        1705     1758      +53     
  Branches      145      148       +3     
==========================================
+ Hits         1607     1660      +53     
  Misses         62       62              
  Partials       36       36

Impacted Files	Coverage Δ
treq/client.py	`90.54% <100%> (+0.04%)`	✅
treq/auth.py	`100% <100%> (ø)`	✅
treq/test/test_auth.py	`100% <100%> (ø)`	✅

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 e0b3ad8...794636a. Read the comment docs.

glyph

Thanks for the PR.

This strikes me as somewhat over-general for just "auth" though. The example doesn't actually do any auth (what's the recommendation? modify the headers object? create a new one? at what phase does this run? what about redirects? how long does the "auth" object live - can it store state for challenge response?

I'd suggest - although I'm not adamant - that this should be a new argument, "customize", or somesuch. But, for the use-case of auth, the thing this really needs is a clear example of how you implement a real auth mechanism (ideally one with more than one round trip).

Thanks again!

glyph · 2016-10-25T02:09:11Z

ping

markrwilliams · 2016-10-25T02:38:05Z

@glyph Thanks for the review, and sorry for the silence.

This strikes me as somewhat over-general for just "auth" though.

Agreed.

The example doesn't actually do any auth (what's the recommendation? modify the headers object? create a new one? at what phase does this run? what about redirects? how long does the "auth" object live - can it store state for challenge response?

This PR's real goal is to enable an entirely external digest authentication implementation for treq. Certainly that will have a request-response loop. I purposely omitted such a loop here because I expected to follow up with a real example there.

I'm happy to admit I wasn't imaginative enough to come up with a simpler but useful example :)

I'd suggest - although I'm not adamant - that this should be a new argument, "customize", or somesuch.

The proposed API's breadth made me think the same thing. Also a "customize" argument would make a toy redirect-following function relevant as an example.

If it's OK with you and @dreid I'm happy to change this over to "customize" and write the redirect follower as an example of how to use it.

Thanks again!

Thanks for the review!!

dreid · 2016-11-02T15:59:51Z

These are the relevant bits of requests documentation:
http://requests.readthedocs.io/en/master/user/authentication/
http://requests.readthedocs.io/en/master/user/advanced/#custom-authentication

markrwilliams · 2016-11-03T06:14:23Z

@glyph After talking with @dreid this morning, I decided not to do what I said I would in the previous comment. This should be explicitly for authorization, though its interface is broad, because of where it sits in the Agent composition chain (e.g., it wraps the RedirectAgent, so it will be called after redirects are handled.)

I'll note that the requests documentation that @dreid links too shows that their custom auth example is no better than the one originally in this PR*. However I went ahead and implemented a more meaningful, if still silly, example. I'm not sure if the code's as clear as it could be, so please take a look!

EDIT *Whoops, missed PizzaAuth the first read through the requests links. What a delicious example!

dreid · 2016-11-03T15:46:18Z

@markrwilliams I spoke with @Lukasa on IRC about requests' behavior and he pointed me to https://github.com/kennethreitz/requests/blob/master/requests/sessions.py#L198-L220

We should probably match this behavior.

The easiest implementation of this is probably an Agent that takes two agents, and uses the first if the host matches the first host it sees a request for, the second agent is used when the host doesn't match.

base_agent = Agent(…)
auth_agent = AuthAgent(base_agent)
pinning_agent = PinToFirstHost(auth_agent, base_agent)
redirect_agent = RedirectAgent(pinning_agent)

(this is just an example of the composition hierarchy if it is being done manually, probably each auth method should be in charge of determining the pinning behavior, and PinToFirstHost is just a convenient container for the default behavior implemented by the HTTPBasicAuth and friends.)

requests does not currently support the ability to specify credentials per-host (unless using netrc), but since treq's "credential" objects are callables that return Agents they have full access to the request so implementing this is at least possible as a third party.

This prevents treq from leaking authorization headers to unrelated hosts via redirects.

glyph

The general idea behind the implementation seems pretty solid here, but I'd feel a lot better if you could address my nitpicking before we merge it ;). I particularly would like to see narrative docstrings for the tests and the removal of the word "silly" from the docs.

glyph · 2017-01-03T20:50:20Z

docs/examples/custom_auth.py

+import treq
+
+
+class SillyAuthResource(Resource):


type(Resource) is ClassType so this should be Resource, object.

glyph · 2017-01-03T20:50:32Z

docs/examples/custom_auth.py

+    """
+    isLeaf = True
+
+    def __init__(self, header, secret):


Perhaps it's time to start using attrs? :)

glyph · 2017-01-03T20:51:32Z

docs/examples/custom_auth.py

+
+class SillyAuthResource(Resource):
+    """
+    A resource that uses a silly, header-based authentication


Thinking from the perspective of a non-native English speaker for a moment - the "silliness" here is not obvious, so perhaps it would be better to use a word like "custom"?

hm, what about "trivial"?

glyph · 2017-01-03T20:52:48Z

treq/auth.py

@@ -26,6 +27,31 @@ def request(self, method, uri, headers=None, bodyProducer=None):
            method, uri, headers=headers, bodyProducer=bodyProducer)


+class _PinToFirstHostAgent(object):
+    """
+    An {twisted.web.iweb.IAgent} implementing object that takes two


"implementing object" is really a content-free phrase here; those words could just be deleted with no loss of meaning.

glyph · 2017-01-03T20:54:26Z

treq/test/test_auth.py

@@ -42,6 +48,61 @@ def test_overrides_per_request_headers(self):
        )


+class PinToFirstHostAgentTests(TestCase):
+    def setUp(self):
+        self.first_agent = mock.Mock(Agent)


I really dislike mock. Sometimes it's necessary for expedience here, but given that we already have treq.testing, couldn't we use that instead?

I may like mock less than any other person on the planet. I chose it here to stay consistent with the existing test case, since a person wandering in would wonder why one used mock and the others treq.testing. I'd be happy to do a follow up PR that switched things over to treq.testing.

glyph · 2017-01-03T20:54:42Z

treq/test/test_auth.py

@@ -42,6 +48,61 @@ def test_overrides_per_request_headers(self):
        )


+class PinToFirstHostAgentTests(TestCase):


These tests should really have docstrings.

glyph · 2017-01-03T20:56:02Z

treq/auth.py

+    """
+
+    def __init__(self, first_agent, second_agent):
+        self._first_agent = first_agent


I'm inclined to suggest Twisted coding-standard names rather than pep8, which seems to be the standard in this repo https://github.com/twisted/treq/blob/master/src/treq/testing.py#L45-L46 and is officially the standard in Klein (despite a few lapses there)

Lots of treq.client, for example, prefers underscore_names. I'd rather confront naming consistency in a subsequent PR and stick with pep8 names in this one, since the API I'm extending prefers underscores

It's trivial!

glyph · 2017-07-26T07:52:35Z

Can you resolve the conflicts which have crept in?

ktdreyer · 2018-03-25T00:11:40Z

I'm definitely interested in this. Today I wrote https://github.com/ktdreyer/treq-kerberos to make treq work with HTTP Negotiate authentication (similar to requests-kerberos). I think treq-kerberos could be another authentication provider.

glyph · 2018-04-07T08:37:43Z

@ktdreyer That would be awesome.

glyph · 2018-05-24T07:12:30Z

@markrwilliams bump

glyph · 2020-12-01T07:46:20Z

@ktdreyer @markrwilliams any chance of updating this?

glyph · 2022-01-11T07:43:58Z

I'm going to close this since it is accumulating conflicts and hasn't been updated in a year. Feel free to resurrect this contribution and reopen.

Allow custom authorization mechanisms.

52742fe

The auth keyword argument now takes a callable accepts an IAgent and returns an IAgent. The returned IAgent can then implement whatever authorization scheme the human mind can imagine.

markrwilliams mentioned this pull request Sep 25, 2016

Authorization wrapper should be added before any redirect wrappers #140

Open

glyph requested changes Oct 19, 2016

View reviewed changes

Add a more meaningful example of a custom auth callable

04a4cb0

Emulate requests' host pinning for authorization.

363bf52

This prevents treq from leaking authorization headers to unrelated hosts via redirects.

glyph reviewed Jan 3, 2017

View reviewed changes

markrwilliams added 2 commits February 1, 2017 18:28

Correct type for Resource subclass in example.

3531934

The example isn't silly, which is a value judgement and unclear.

794636a

It's trivial!

glyph closed this Jan 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom authorization mechanisms. #139

Allow custom authorization mechanisms. #139

markrwilliams commented Sep 25, 2016

markrwilliams commented Sep 25, 2016

codecov-io commented Sep 25, 2016 •

edited

Loading

glyph left a comment

glyph commented Oct 25, 2016

markrwilliams commented Oct 25, 2016

dreid commented Nov 2, 2016

markrwilliams commented Nov 3, 2016 •

edited

Loading

dreid commented Nov 3, 2016 •

edited

Loading

glyph left a comment

glyph Jan 3, 2017

glyph Jan 3, 2017

glyph Jan 3, 2017

markrwilliams Feb 2, 2017

glyph Jan 3, 2017

glyph Jan 3, 2017

markrwilliams Feb 2, 2017

glyph Jan 3, 2017

glyph Jan 3, 2017

markrwilliams Feb 2, 2017

glyph commented Jul 26, 2017

ktdreyer commented Mar 25, 2018

glyph commented Apr 7, 2018

glyph commented May 24, 2018

glyph commented Dec 1, 2020

glyph commented Jan 11, 2022

		@@ -42,6 +48,61 @@ def test_overrides_per_request_headers(self):
		)


		class PinToFirstHostAgentTests(TestCase):

Allow custom authorization mechanisms. #139

Allow custom authorization mechanisms. #139

Conversation

markrwilliams commented Sep 25, 2016

markrwilliams commented Sep 25, 2016

codecov-io commented Sep 25, 2016 • edited Loading

Codecov Report

glyph left a comment

Choose a reason for hiding this comment

glyph commented Oct 25, 2016

markrwilliams commented Oct 25, 2016

dreid commented Nov 2, 2016

markrwilliams commented Nov 3, 2016 • edited Loading

dreid commented Nov 3, 2016 • edited Loading

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyph commented Jul 26, 2017

ktdreyer commented Mar 25, 2018

glyph commented Apr 7, 2018

glyph commented May 24, 2018

glyph commented Dec 1, 2020

glyph commented Jan 11, 2022

codecov-io commented Sep 25, 2016 •

edited

Loading

markrwilliams commented Nov 3, 2016 •

edited

Loading

dreid commented Nov 3, 2016 •

edited

Loading