Skip to content
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

Pass payload to keyfinder #172

Merged
merged 3 commits into from
Oct 20, 2016
Merged

Pass payload to keyfinder #172

merged 3 commits into from
Oct 20, 2016

Conversation

CodeMonkeySteve
Copy link
Contributor

@CodeMonkeySteve CodeMonkeySteve commented Oct 17, 2016

I have an app that I would like to authenticate to using tokens signed with HMAC (created by the app) or ECDSA (created by other companies), depending on the value of the issuer (iss) claim. I'm using the keyfinder block to implement that logic, but it needs to have the payload passed to it in order to find the appropriate public key for the issuer. To that end, I made a small change to pass both the header and payload to keyfinder, if it has arity of 2. Now my calling code looks like:

payload = JWT.decode(token, nil, true) do |header, payload|
  alg = header['alg']
  if alg.starts_with?('HS')
    # HMAC token
    Rails.application.secrets[:secret_key_base]

  elsif alg.starts_with?('ES') && (pub_key = Company.where(id: payload['iss']).pluck(:public_key).first)
    # ECDSA token
    OpenSSL::PKey::EC.new(pub_key)

  else
    nil
  end
end.first

@excpt excpt added this to the Version 1.6.0 milestone Oct 18, 2016
@CodeMonkeySteve
Copy link
Contributor Author

Added check that keyfinder returns a key.

Copy link
Member

@excpt excpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a view comments to your changes. If you find the time you may fix them. Otherwise the PR looks good to me.

key = yield(header) if keyfinder
def signature_algorithm_and_key(header, payload, key, &keyfinder)
if keyfinder
key = (keyfinder.arity == 2) ? keyfinder.call(header, payload) : keyfinder.call(header)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change your code to use yield instead of keyfinder.call (yield(header, playload) and yield(header)) and use a full if else condition statement instead of your ternary conditions.

key = if keyfinder.arity == 2
          yield(header, payload)
        else
          yield(header)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the arity check, I prefer call to yield, as it makes it more obvious that keyfinder is the block being called. But I'll defer to your style.

def signature_algorithm_and_key(header, payload, key, &keyfinder)
if keyfinder
key = (keyfinder.arity == 2) ? keyfinder.call(header, payload) : keyfinder.call(header)
raise JWT::DecodeError, 'No verification key available' unless key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code smell: There is an additional space in front of the unless key word.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently no one likes my style of putting two spaces before postfix conditionals, but I wouldn't call it a code smell. ;)

@CodeMonkeySteve
Copy link
Contributor Author

Made style changes.

@excpt excpt merged commit 729f8db into jwt:master Oct 20, 2016
@excpt
Copy link
Member

excpt commented Oct 20, 2016

Thanks. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants