Use a pluggable authorizer to calculated the Authorization header field value.#65
Use a pluggable authorizer to calculated the Authorization header field value.#65
Authorization header field value.#65Conversation
alakra
left a comment
alakra
left a comment
There was a problem hiding this comment.
Really love the direction this is headed in!
lib/exhal/client.ex
Outdated
| {:ok, Document.t() | NonHalResponse.t(), ResponseHeader.t()} | ||
| | {:error, Document.t() | NonHalResponse.t(), ResponseHeader.t() } | ||
| | {:error, Error.t()} | ||
| {:ok, Document.t | NonHalResponse.t, ResponseHeader.t} |
There was a problem hiding this comment.
are you having a disagreement with mix format @pezra ?
There was a problem hiding this comment.
Interestingly the core code doesn't use parens in its typespecs.
|
I'm excited about the switch to |
|
I'd like to cache my authorization header value in my Authorizer impl. How do you feel about adding a |
|
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. |
|
@adherr, how would you like that "reset" callback to work? Maybe, an new function Upon receive a 401 Unauthorized response ExHal would call Questions:
|
but not integrated into request flow
This one's for you, Andrew.
| needed this function should return and empty map. | ||
|
|
||
| """ | ||
| @spec authorization(authorizer, url()) :: %{optional(header_field_name()) => String.t()} |
There was a problem hiding this comment.
I'm confused how a Map key can be optional. Also, should the map value type be credentials?
| @@ -0,0 +1,35 @@ | |||
| defprotocol ExHal.Authorizer do | |||
| @typedoc """ | |||
| The value of the `Authorization` header field. | |||
There was a problem hiding this comment.
Looks like this has been generalized, and the doc should be updated
| def new(headers) do | ||
| new(headers, follow_redirect: true) | ||
| # deprecated call patterns | ||
| def new(headers) when is_list(headers) do |
There was a problem hiding this comment.
do these need the @deprecated annotation?
|
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 A separate PR for that work is 👍 |
|
@adherr asked:
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 Protocols make For the config based basic auth authorizer use case a behavior might be marginally simpler. In this scenario the authorizer module would just 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, What you do you think? Should i convert authorizers into a behavior? |
3 participants
No description provided.