-
Notifications
You must be signed in to change notification settings - Fork 229
JWT authentication #70
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
Changes from all commits
9e33d10
5b3fe7c
270ea1c
ef86e9c
9bb9443
a70144a
edd5483
7f12bd4
3366df1
31fcc44
9f84e58
e6a3d4e
87de4bf
b06aa7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
module Sorcery | ||
module Controller | ||
module Submodules | ||
module JwtAuth | ||
def self.included(base) | ||
base.send(:include, InstanceMethods) | ||
end | ||
|
||
module InstanceMethods | ||
# This method return generated token if user can be authenticated | ||
def jwt_auth(*credentials) | ||
user = user_class.authenticate(*credentials) | ||
if user | ||
user_params = Config.jwt_user_params.each_with_object({}) do |val, acc| | ||
acc[val] = user.public_send(val) | ||
end | ||
{ Config.jwt_user_data_key => user_params, | ||
Config.jwt_auth_token_key => jwt_encode(user_params) } | ||
end | ||
end | ||
|
||
# To be used as a before_action. | ||
def jwt_require_auth | ||
jwt_not_authenticated && return unless jwt_user_id | ||
|
||
@current_user = Config.jwt_set_user ? User.find(jwt_user_id) : jwt_user_data | ||
rescue JWT::VerificationError, JWT::DecodeError | ||
jwt_not_authenticated && return | ||
end | ||
|
||
# This method creating JWT token by payload | ||
def jwt_encode(payload) | ||
JWT.encode(payload, Config.jwt_secret_key) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should specify an algorithm to use, else the created JWT is unsigned! See here: https://github.com/jwt/ruby-jwt#algorithms-and-usage I think it should be a config option, with The same algorithm parameter has to be passed to |
||
end | ||
|
||
# This method decoding JWT token | ||
# Return nil if token incorrect | ||
def jwt_decode(token) | ||
HashWithIndifferentAccess.new( | ||
JWT.decode(token, Config.jwt_secret_key)[0] | ||
) | ||
rescue JWT::DecodeError | ||
nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be removed, considering that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jovanmaric |
||
end | ||
|
||
# Take token from header, by key defined in config | ||
# With memoization | ||
def jwt_from_header | ||
@jwt_header_token ||= request.headers[Config.jwt_headers_key] | ||
end | ||
|
||
# Return user data which decoded from token | ||
# With memoization | ||
def jwt_user_data(token = jwt_from_header) | ||
@jwt_user_data ||= jwt_decode(token) | ||
end | ||
|
||
# Return user id from user data if id present. | ||
# Else return nil | ||
def jwt_user_id | ||
jwt_user_data.try(:[], :id) | ||
end | ||
|
||
# This method called if user not authenticated | ||
def jwt_not_authenticated | ||
respond_to do |format| | ||
format.html { not_authenticated } | ||
format.json { render json: { status: 401 }, status: 401 } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO the message should not be the status code because it's verbose. format.json {
render json: {
"error": {
"message": "Need to login first.",
}
},
status: 401
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also suggest you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about making this changeable, instead of hardcoding the return message There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's difficult to make an error message changeable because of the design. The cases are different when this method is called. I use ruby-jwt error messages and I want it to be overridden if someone wants to change the message. See this commit for details: ebihara99999@9e02135 |
||
end | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to set the new line. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ Gem::Specification.new do |s| | |
s.add_dependency 'oauth', '~> 0.4', '>= 0.4.4' | ||
s.add_dependency 'oauth2', '~> 1.0', '>= 0.8.0' | ||
s.add_dependency 'bcrypt', '~> 3.1' | ||
s.add_dependency 'jwt', '~> 1.5' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The latest is 2.1.0 |
||
|
||
s.add_development_dependency 'yard', '~> 0.6.0' | ||
s.add_development_dependency 'timecop' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
require 'spec_helper' | ||
|
||
describe SorceryController, type: :controller do | ||
let!(:user) { double('user', id: 42) } | ||
before(:each) do | ||
request.env['HTTP_ACCEPT'] = "application/json" if ::Rails.version < '5.0.0' | ||
end | ||
|
||
describe 'with jwt auth features' do | ||
let(:user_email) { '[email protected]' } | ||
let(:user_password) { 'testpass' } | ||
let(:auth_token) { 'eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpZCI6NDJ9.rrjj-sXvbIjT8y4MLGb88Cv7XvfpJXj-HEgaBimT_-0' } | ||
let(:response_data) do | ||
{ | ||
user_data: { id: user.id }, | ||
auth_token: auth_token | ||
} | ||
end | ||
|
||
before(:all) do | ||
sorcery_reload!([:jwt_auth]) | ||
end | ||
|
||
describe '#jwt_auth' do | ||
context 'when success' do | ||
before do | ||
allow(User).to receive(:authenticate).with(user_email, user_password).and_return(user) | ||
|
||
post :test_jwt_auth, params: { email: user_email, password: user_password } | ||
end | ||
|
||
it 'assigns user to @token variable' do | ||
expect(assigns[:token]).to eq response_data | ||
end | ||
end | ||
|
||
context 'when fails' do | ||
before do | ||
allow(User).to receive(:authenticate).with(user_email, user_password).and_return(nil) | ||
|
||
post :test_jwt_auth, params: { email: user_email, password: user_password } | ||
end | ||
|
||
it 'assigns user to @token variable' do | ||
expect(assigns[:token]).to eq nil | ||
end | ||
end | ||
end | ||
|
||
describe '#jwt_require_auth' do | ||
context 'when success' do | ||
before do | ||
allow(User).to receive(:find).with(user.id).and_return(user) | ||
allow(user).to receive(:set_last_activity_at) | ||
end | ||
|
||
it 'does return 200' do | ||
request.headers.merge! Authorization: auth_token | ||
|
||
get :some_action_jwt, format: :json | ||
|
||
expect(response.status).to eq(200) | ||
end | ||
end | ||
|
||
context 'when fails' do | ||
let(:user_email) { '[email protected]' } | ||
let(:user_password) { 'testpass' } | ||
|
||
context 'without auth header' do | ||
it 'does return 401' do | ||
get :some_action_jwt, format: :json | ||
|
||
expect(response.status).to eq(401) | ||
end | ||
end | ||
|
||
context 'with incorrect auth header' do | ||
let(:incorrect_header) { '123.123.123' } | ||
|
||
it 'does return 401' do | ||
request.headers.merge! Authorization: incorrect_header | ||
|
||
get :some_action_jwt, format: :json | ||
|
||
expect(response.status).to eq(401) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to set the new line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's an idea to replace user_id with a jti (SecureRandom.uuid) and implement a whitelisting strategy to support login from multiple devices and independent logout. This implies that on login, a record is created in the whitelists table and that on logout this record gets deleted.
Also i'm missing expiration from this implementation. The generated jwt keys are valid indefinitely. The duration a key is persisted could be set from the config for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jovanmaric I don't know why
user_id
is here though the token includes user_id as the subject claim ( of course, it's needed to decode to acquire it ) .I'd like to remove and only returns a token like
eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJzdWIiOjQyLCJleHAiOjE1NDc3MTkyMDAsImlhdCI6MTU0NzQ2MDAwMH0.QM5mTkYiDwI-10cEOq4b_bfrwe99BRuef6pnIB-jqIk
.What do you think about it?
And I would appreciate it if @wilddima could answer why
Config.jwt_user_data_key => user_params
is needed.