Use a pluggable authorizer to calculated the `Authorization` header field value. #65

pezra · 2018-10-01T14:39:48Z

No description provided.

alakra

Really love the direction this is headed in!

lib/exhal/client.ex

adherr · 2018-10-01T17:51:58Z

lib/exhal/client.ex

-  {:ok, Document.t() | NonHalResponse.t(), ResponseHeader.t()}
-  | {:error, Document.t() | NonHalResponse.t(), ResponseHeader.t() }
-  | {:error, Error.t()}
+  {:ok, Document.t | NonHalResponse.t, ResponseHeader.t}


are you having a disagreement with mix format @pezra ?

Interestingly the core code doesn't use parens in its typespecs.

adherr · 2018-10-01T20:17:00Z

I'm excited about the switch to mox!

adherr · 2018-10-01T20:35:41Z

I'd like to cache my authorization header value in my Authorizer impl. How do you feel about adding a reset callback to the protocol that is called when a request fails due to authorization problems? I can put a case statement in my client code for this, but it would be cleaner if I didn't have to wrap my ExHal requests with this logic.

pezra · 2018-10-01T22:12:40Z

I am torn about mox. It seems to work but i am pretty sure it increased the chance of bugs lurking in the code. My use of it couples the tests to HTTP library and interface of the Client module. In a very real sense those tests now verify that the functions are implemented a particular way, rather than that they achieve the desired goal.

pezra · 2018-10-01T22:28:50Z

@adherr, how would you like that "reset" callback to work?

Maybe, an new function

unauthorized_response_received(url, [challenge]) :: :retry | :continue

Upon receive a 401 Unauthorized response ExHal would call unauthorized_response_received/2 for the current authorizer. If it returned :retry then the request would be retried form the top. If it returned :continue the current error flow would be executed.

Questions:

Could we add this as a separate PR?
How digested should the challenge be? Values of the WWW-Authenticate header field are a standardized format so we could parse it.

pezra · 2018-10-01T22:50:19Z

@adherr, @alakra, another question: should authorization/2 return a header fields map, rather than just the credentials? This might allow it to support esoteric auth schemes that use non-standard and/or support for the Signature header, etc

but not integrated into request flow

This one's for you, Andrew.

adherr · 2018-10-04T15:24:58Z

lib/exhal/authorizer.ex

+  needed this function should return and empty map.
+
+  """
+  @spec authorization(authorizer, url()) :: %{optional(header_field_name()) => String.t()}


I'm confused how a Map key can be optional. Also, should the map value type be credentials?

adherr · 2018-10-04T15:27:03Z

lib/exhal/authorizer.ex

@@ -0,0 +1,35 @@
+defprotocol ExHal.Authorizer do
+  @typedoc """
+  The value of the `Authorization` header field.


Looks like this has been generalized, and the doc should be updated

adherr · 2018-10-04T17:35:10Z

lib/exhal/client.ex

-  def new(headers) do
-    new(headers, follow_redirect: true)
+  # deprecated call patterns
+  def new(headers) when is_list(headers) do


do these need the @deprecated annotation?

adherr · 2018-10-04T17:58:45Z

general curious question: Why a protocol instead of a behaviour?

I think the choice to return a header map from the Authorizer was a good one.

I like the proposed unauthorized_response_received function, especially the ability to retry. Parsing the WWW-Authenticate header is fine, but our services don't issue them, so ExHal shouldn't count on them being sent with a 401 or 403. I doubt (I may be wrong here) we would do anything useful with the challenge argument, we'd just attempt to reauthenticate and retry any time the function was called.

A separate PR for that work is 👍

pezra · 2018-10-07T23:53:39Z

@adherr asked:

Why a protocol instead of a behaviour

Excellent question. I'm not entirely sure protocol is the best choice. The two seem basically isomorphic for most use cases. Three use cases i specifically considered are the built-in SimpleAuthorizer, a config based basic auth authorizer, and a password grant oauth2 authorizer.

Protocols make SimpleAuthorizer very simple to implement. No state holding processes or attending supervisors are required. We just shove the state in a struct and have the protocol implementation access that data.

For the config based basic auth authorizer use case a behavior might be marginally simpler. In this scenario the authorizer module would just Application.get_env with the appropriate application and item names. Having to define a struct is pure, albeit minimal, overhead.

A password grant oauth2 authorizer is going to need a state holding process, and attending supervisors. It would convert a username and password into an access token at runtime and then remember that token for the remainder of the session. A behavior might be slightly more natural, but it doesn't seem to be a big win either way.

As you can see, two out of those three use cases would seem to benefit from using a behavior, rather than a protocol. There is, apparently, a pretty significant performance penalty to protocols vs behaviors, so that is a potential downside. However, SimpleAuthorizer would be substantially more complex as a behavior.

What you do you think? Should i convert authorizers into a behavior?

alakra approved these changes Oct 1, 2018

View reviewed changes

adherr reviewed Oct 1, 2018

View reviewed changes

lib/exhal/client.ex Show resolved Hide resolved

adherr reviewed Oct 1, 2018

View reviewed changes

pezra added 5 commits October 3, 2018 18:26

authorizer protocol and basic implementations

72341ea

but not integrated into request flow

use authorizer to calculate Authorization header field value.

1f1a3bf

update docs and bump version

85a53dc

uglify the code

9c260d1

This one's for you, Andrew.

improve Authorizer type docs

4fa54c1

pezra force-pushed the authorizer branch from ab7d92f to 271f94c Compare October 4, 2018 00:28

better authorization interface

264a660

pezra force-pushed the authorizer branch from 271f94c to 264a660 Compare October 4, 2018 02:49

adherr reviewed Oct 4, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a pluggable authorizer to calculated the `Authorization` header field value. #65

Use a pluggable authorizer to calculated the `Authorization` header field value. #65

pezra commented Oct 1, 2018

alakra left a comment

adherr Oct 1, 2018

pezra Oct 1, 2018

adherr commented Oct 1, 2018

adherr commented Oct 1, 2018

pezra commented Oct 1, 2018

pezra commented Oct 1, 2018 •

edited

Loading

pezra commented Oct 1, 2018 •

edited

Loading

adherr Oct 4, 2018 •

edited

Loading

adherr Oct 4, 2018

adherr Oct 4, 2018

adherr commented Oct 4, 2018

pezra commented Oct 7, 2018

Use a pluggable authorizer to calculated the Authorization header field value. #65

Are you sure you want to change the base?

Use a pluggable authorizer to calculated the Authorization header field value. #65

Conversation

pezra commented Oct 1, 2018

alakra left a comment

Choose a reason for hiding this comment

adherr Oct 1, 2018

Choose a reason for hiding this comment

pezra Oct 1, 2018

Choose a reason for hiding this comment

adherr commented Oct 1, 2018

adherr commented Oct 1, 2018

pezra commented Oct 1, 2018

pezra commented Oct 1, 2018 • edited Loading

pezra commented Oct 1, 2018 • edited Loading

adherr Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

adherr Oct 4, 2018

Choose a reason for hiding this comment

adherr Oct 4, 2018

Choose a reason for hiding this comment

adherr commented Oct 4, 2018

pezra commented Oct 7, 2018

Use a pluggable authorizer to calculated the `Authorization` header field value. #65

Use a pluggable authorizer to calculated the `Authorization` header field value. #65

pezra commented Oct 1, 2018 •

edited

Loading

pezra commented Oct 1, 2018 •

edited

Loading

adherr Oct 4, 2018 •

edited

Loading