The Wayback Machine - https://web.archive.org/web/20200906101613/https://github.com/jwt/ruby-jwt/pull/172/
Skip to content

/ ruby-jwt

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 commented Oct 17, 2016 •
edited by excpt

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
@CodeMonkeySteve
Pass payload to keyfinder.
Loading status checks…
c2304eb
@excpt excpt added this to the Version 1.6.0 milestone Oct 18, 2016
@CodeMonkeySteve
Raise exception if keyfinder didn't return a key.
Loading status checks…
775c031
@CodeMonkeySteve
Copy link
Contributor Author

CodeMonkeySteve commented Oct 19, 2016

Added check that keyfinder returns a key.
@excpt
excpt reviewed Oct 20, 2016
View changes
Copy link
Member

excpt left a comment

I added a view comments to your changes. If you find the time you may fix them. Otherwise the PR looks good to me.
lib/jwt.rb Outdated
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)

This comment has been minimized.

Sign in to view
@excpt

excpt Oct 20, 2016

Member

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)

This comment has been minimized.

Sign in to view
@CodeMonkeySteve

CodeMonkeySteve Oct 20, 2016

Author Contributor

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.
lib/jwt.rb Outdated
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

This comment has been minimized.

Sign in to view
@excpt

excpt Oct 20, 2016

Member

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

This comment has been minimized.

Sign in to view
@CodeMonkeySteve

CodeMonkeySteve Oct 20, 2016

Author Contributor

Apparently no one likes my style of putting two spaces before postfix conditionals, but I wouldn't call it a code smell. ;)
@CodeMonkeySteve
Style changes from code review.
Loading status checks…
2f837b2
@CodeMonkeySteve
Copy link
Contributor Author

CodeMonkeySteve commented Oct 20, 2016

Made style changes.
@excpt excpt merged commit 729f8db into jwt:master Oct 20, 2016
1 of 2 checks passed
1 of 2 checks passed
codeclimate Code Climate found 4 new issues and 1 fixed issue.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@excpt
Copy link
Member

excpt commented Oct 20, 2016

Thanks. :)
@CodeMonkeySteve CodeMonkeySteve mentioned this pull request Sep 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@excpt excpt

Assignees
No one assigned
Labels
enhancement feature review required
Projects
None yet
Milestone
Version 2.0.0
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@CodeMonkeySteve @excpt
You can’t perform that action at this time.