JWT authentication ( after the commit b06aa7a )#167
JWT authentication ( after the commit b06aa7a )#167ebihara99999 wants to merge 23 commits intoSorcery:masterfrom
Conversation
See the discussion here: Sorcery#70 (comment)
There is a suggestion that a message in the case not authenticated could be changeable in the discussion: Sorcery#70 (comment) But I decide to make use of a message sent from `JWT::DecodeError` because it's difficult to make the message flexible because of the design. Changes: - `#jwt_require_auth` only rescues `JWT::DecodeError` because JWT::VerificationError is a subclass. - `#jwt_decode` raises `JWT::DecodeError` or the error of the subclasses, and does not rescue in it. - `#jwt_not_authenticated` receives an argument `JWT::DecodeError` are from here: https://github.com/jwt/ruby-jwt/blob/master/lib/jwt/error.rb
`JWT::ExpiredSignature: Signature has expired` is expected when a token is expired. Review comment is: Sorcery#70 (comment)
the review comment is: Sorcery#70 (comment)
|
TODO
|
ebihara99999
changed the title
joshbuker
left a comment
joshbuker
left a comment
There was a problem hiding this comment.
Looks like there's still some conflicts with the latest changes on master, can you merge master into this branch and resolve any file conflicts?
|
@athix Thanks, I've fixed them just now |
| s.add_dependency 'bcrypt', '~> 3.1' | ||
| s.add_dependency 'oauth', '~> 0.4', '>= 0.4.4' | ||
| s.add_dependency 'oauth2', '~> 1.0', '>= 0.8.0' | ||
| s.add_dependency 'bcrypt', '~> 3.1' |
There was a problem hiding this comment.
@ebihara99999 Duplicate bcrypt dependency, likely from a merge. I would double check anything that changed on your merge commits.
There was a problem hiding this comment.
@athix I'm sorry to add the wrong dependency. I've fixed it just now
|
If anyone in the community would like to help with this, I think JWT support would be a great addition to Sorcery. |
| # A flag that specifies whether to perform a database query to set the current_user | ||
| # Default: true | ||
| # | ||
| # config.jwt_set_user = |
There was a problem hiding this comment.
Do we need this flag? Is there ever a case when current_user should not be an instance of a user model?
|
|
||
| # To be used as a before_action. | ||
| def jwt_require_auth | ||
| binding.pry |
There was a problem hiding this comment.
I guess this is here by accident? :)
|
|
||
| module InstanceMethods | ||
| # This method return generated token if user can be authenticated | ||
| def jwt_auth(*credentials) |
There was a problem hiding this comment.
I think slightly more appropriate name for the method would be jwt_authenticate. Simply to stay consistent with User.authenticate.
There was a problem hiding this comment.
Method should also allow users to get login failure reason, similar to what login method does.
| binding.pry | ||
| @current_user = Config.jwt_set_user ? User.find(jwt_user_id) : jwt_user_data | ||
| rescue JWT::DecodeError => e | ||
| jwt_not_authenticated(message: e.message) && return |
There was a problem hiding this comment.
Is trailing return call needed here?
There was a problem hiding this comment.
jwt_not_authenticated is currently only called when JWT is not decoded properly. We should also handle cases when JWT is properly decoded, but the user record is missing.
| # Take token from header, by key defined in config | ||
| # With memoization | ||
| def jwt_from_header | ||
| @jwt_header_token ||= request.headers[Config.jwt_headers_key] |
There was a problem hiding this comment.
I think that more sensible default here would be to expect header value in one of the following formats:Bearer <jwt-token> or Token <jwt-token>. Same as what ActionController's authentication does – https://api.rubyonrails.org/classes/ActionController/HttpAuthentication/Token.html
Further more, does it makes sense to allow users to define how should token be extracted from request?
| now = Time.current | ||
| default_payload = { | ||
| sub: user.id, | ||
| exp: (now + 3.days).to_i, |
There was a problem hiding this comment.
Token duration should be configurable.
joshbuker
added
the
to be implemented in v1
|
Closing this out to reduce noise, see #239 and Sorcery/sorcery-rework#9 for the latest on JWT in Sorcery. |
4 participants
This is from #70.
There are some reviews with no response and the PR is stale for a long time (though the PR is great!!).
Thank @wilddima for implementing them firstly!!