Adds support for JWT OAuth flow #68
Conversation
|
Hello, @skostojohn! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
|
Hi @chulkilee - please let me know if you are waiting for me on any items for this PR. I am new to this, and not sure if I need to anything else to submit it for review. |
| key = %{"pem" => Keyword.fetch!(payload, :jwt_key)} | ||
| signer = Joken.Signer.create("RS256", key) |
There was a problem hiding this comment.
This assume that jwt_key is in the PEM format. Hm.. what about taking signer?
There was a problem hiding this comment.
I don't quite understand your suggestion about "taking signer" - can you please clarify? Are you saying that instead of the user passing in the private key to get_token via jwt_key in the payload, they should instead create a Joken.Signer struct like signer themselves (presumeably via Joken.Signer.create) , and then pass that instead?
If so, are we then tied to Joken? If you want to internally use a different approach to JWT in the future, would that impact our interface, which would be expecting a Joken.Signer struct? Also, are we then complicating the user experience by asking them to work with Joken to create the struct, when before the use of Joken was transparent to them?
In some ways here I am adopting the behavior of the restforce ruby gem for Salesforce , which I am familiar with. It also takes the private key as a parameter to support JWT authentication.
I am happy to change what I have done here, if you can just help me understand a bit more clearly what you are suggesting. Thanks!
There was a problem hiding this comment.
First of all, if I were to use this feature, I'm going to keep signer struct instead of PEM format to avoid extra decoding work on each request.
We may introduce an adapter for Joken - like it does with Tesla for http. That Joken adopter will tak any params Joken.Signer.create/2 would take.
There was a problem hiding this comment.
What do you mean "extra decoding work on each request"? In my experience, you call authenticate (e.g. via get_token) once to get an access token from Salesforce, which you would then use on all subsequent calls to the API for authentication. So you only really use the pem string once, to get the access_token. The access_token you get after JWT auth is the same as the access_token you get from any other oauth flow..
Sorry that I am having trouble following your concerns here.
lib/ex_force/oauth.ex
Outdated
| def get_token(url, payload) when is_binary(url) do | ||
| url | ||
| |> build_client | ||
| |> then( | ||
| &if Keyword.fetch!(payload, :grant_type) == "jwt" do | ||
| get_token_jwt(&1, url, payload) | ||
| else | ||
| get_token(&1, payload) | ||
| end | ||
| ) | ||
| end |
There was a problem hiding this comment.
get_token(url, payload) is a thin wrapper for get_token(client, payload). Instead of adding this logic in it, let's do it inside get_token.
def get_token(client, opts) do
grant_type = Keyword.fetch!(payload, :grant_type)
client
|> request(build_payload(opts))
|> verify(grant_type, opts)
end
def verify_signature(response, "jwt", _), do: response
def verify_signature(response, _, opts) do
# ...
end
There was a problem hiding this comment.
OK - Let me take a crack at this. I think I understand your direction here.
There was a problem hiding this comment.
OK - I refactored this to try and follow your direction for the get_token method. I am a bit concerned now that verify_signature is doing two jobs - verifying the signature and formatting the output into an %OAuthResponse. In the case of the jwt path, it is actually only formatting and not verifying anything at all. I wonder if we should separate the formatting into another function?
There was a problem hiding this comment.
Any thoughts on the changes I have made here? Hopefully you will find them to be aligned with your suggestions.
RE Issue #66
Changes to support JWT auth flow in ex_force.
Includes:
get_token_jwtto make the auth call and handle resultscreate_jwt_payloadto create the JWT assertion payloadget_tokento call the correct function given the grant typeassert_form_body_jwtThe new function
get_token_jwtcould potentially be avoided, but there are enough differences from the other grant types that it seemed cleaner to create a new function rather than try to add this to the existing function.I wasn't sure if I needed to make any documentation updates - I can do that if you can point me in the right direction.
As I mentioned earlier, I am a beginner with Elixir - I welcome any feedback and am happy to rework this to better suit your preferences or to make it more Elixir-idiomatic.
Thank you!
Scott