Skip to content

Commit

Permalink
rubykube#13: don't fetch/create user in the database during a18n
Browse files Browse the repository at this point in the history
  • Loading branch information
subaru9 committed Jun 15, 2018
1 parent fe6f316 commit d2472af
Show file tree
Hide file tree
Showing 9 changed files with 34 additions and 154 deletions.
63 changes: 14 additions & 49 deletions app/api/api_v1/auth/jwt_authenticator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,14 @@ def initialize(token)

#
# Decodes and verifies JWT.
# Returns authentic member email or raises an exception.
# Returns hash formed with payload including user id (uid) or raises an exception.
#
# @param [Hash] options
# @return [String, User, NilClass]
def authenticate!(options = {})
unless @token_type == 'Bearer'
raise AuthorizationError, 'Token type is not provided or invalid.'
end
# @return [Hash, NilClass]
def authenticate!
check_token_type
payload, _header = decode_and_verify_token(@token_value)
fetch_user(payload).yield_self do |user|
options[:return] == :user ? user : fetch_uid(payload)
end

{ uid: fetch_uid(payload) }
rescue => e
report_exception(e)
if AuthorizationError === e
Expand All @@ -33,14 +29,20 @@ def authenticate!(options = {})
#
# Exception-safe version of #authenticate!.
#
# @return [String, User, NilClass]
# @return [Hash, NilClass]
def authenticate(*args)
authenticate!(*args)
rescue
nil
end

private
private

def check_token_type
unless @token_type == 'Bearer'
raise AuthorizationError, 'Token type is not provided or invalid.'
end
end

def decode_and_verify_token(token)
JWT.decode(token, Utils.jwt_public_key, true, token_verification_options)
Expand All @@ -49,49 +51,12 @@ def decode_and_verify_token(token)
raise AuthorizationError, "Failed to decode and verify JWT: #{e.inspect}."
end

def fetch_email(payload)
payload[:email].to_s.tap do |email|
raise(AuthorizationError, 'E-Mail is blank.') if email.blank?
raise(AuthorizationError, 'E-Mail is invalid.') unless EmailValidator.valid?(email)
end
end

def fetch_uid(payload)
payload.fetch(:uid).tap do |uid|
raise(AuthorizationError, 'UID is blank.') if uid.blank?
end
end

def fetch_scopes(payload)
Array.wrap(payload[:scopes]).map(&:to_s).map(&:squash).reject(&:blank).tap do |scopes|
raise(AuthorizationError, 'Token scopes are not defined.') if scopes.empty?
end
end

def fetch_user(payload)
if payload[:iss] == 'barong'
from_barong_payload(payload)
else
User.find_by(uid: fetch_uid(payload))
end
end

def from_barong_payload(payload)
User.find_or_initialize_by(uid: fetch_uid(payload)).tap do |member|
member.transaction do
attributes = {
email: fetch_email(payload),
state: payload.fetch(:state).to_s,
level: payload.fetch(:level).to_i
}

# Prevent overheat validations.
member.assign_attributes(attributes)
member.save!(validate: member.new_record?)
end
end
end

def token_verification_options
{ verify_expiration: true,
verify_not_before: true,
Expand Down
4 changes: 2 additions & 2 deletions app/api/api_v1/auth/middleware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ class Middleware < Grape::Middleware::Base
def before
return unless auth_by_jwt?

env['api.v1.authenticated_uid'] = \
JWTAuthenticator.new(headers['Authorization']).authenticate!(return: :uid)
env['api.v1.authenticated_data'] = \
JWTAuthenticator.new(headers['Authorization']).authenticate!
end

private
Expand Down
2 changes: 1 addition & 1 deletion app/api/api_v1/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class Base < Grape::API
doc_version: '0.0.1',
info: {
title: 'User API V1',
description: 'User API is client-to-server API'
description: 'User API is client-to-server API v.1'
},
hide_format: true,
hide_documentation_path: true
Expand Down
2 changes: 1 addition & 1 deletion app/api/api_v1/cors/middleware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def call(env)
end
end

private
private

def request
@request ||= Grape::Request.new(env)
Expand Down
10 changes: 5 additions & 5 deletions app/api/api_v1/helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ module Helpers
extend Memoist

def authenticate!
current_user || raise(AuthorizationError)
current_uid || raise(AuthorizationError)
end

def current_user
key = 'api.v1.authenticated_uid' # JWT authentication provides user uid.
def current_uid
key = 'api.v1.authenticated_data' # JWT authentication provides user uid.
return unless env.key?(key)

User.find_by(uid: env[key])
env[key][:uid]
end
memoize :current_user
memoize :current_uid
end
end
2 changes: 1 addition & 1 deletion app/api/api_v1/withdraw.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class Withdraw < Grape::API

post '/withdraws' do
Peatio::ManagementAPIv1Client.new.create_withdraw \
uid: env['api.v1.authenticated_uid'],
uid: current_uid,
currency: params[:currency],
amount: params[:amount],
otp_code: params[:otp],
Expand Down
87 changes: 5 additions & 82 deletions spec/api/v1/auth/jwt_authenticator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,14 @@
headers: { 'Authorization' => token }
end

let :user do
create(:user, :level_3)
end

let :payload do
{ x: 'x', y: 'y', z: 'z', uid: user.uid }
{ x: 'x', y: 'y', z: 'z', uid: 'O90Y88JDPQ7167' }
end

subject { APIv1::Auth::JWTAuthenticator.new(request.headers['Authorization']) }

it 'should work in standard conditions' do
expect(subject.authenticate!).to eq user.uid
expect(subject.authenticate!).to eq payload.slice(:uid)
end

it 'should raise exception when uid is not provided' do
Expand Down Expand Up @@ -60,23 +56,11 @@
end
end

describe 'authentication options' do
it 'should return uid if return: :uid specified' do
expect(subject.authenticate!(return: :uid)).to eq payload[:uid]
end

it 'should return user if return: :user specified' do
create(:user)
payload[:uid] = user.uid
expect(subject.authenticate!(return: :user)).to eq user
end
end

context 'valid issuer' do
before { ENV['JWT_ISSUER'] = 'qux' }
before { payload[:iss] = 'qux' }
after { ENV.delete('JWT_ISSUER') }
it('should validate issuer') { expect(subject.authenticate!).to eq user.uid }
it('should validate issuer') { expect(subject.authenticate!).to eq payload.slice(:uid) }
end

context 'invalid issuer' do
Expand All @@ -92,7 +76,7 @@
before { ENV['JWT_AUDIENCE'] = 'foo,bar' }
before { payload[:aud] = ['bar'] }
after { ENV.delete('JWT_AUDIENCE') }
it('should validate audience') { expect(subject.authenticate!).to eq user.uid }
it('should validate audience') { expect(subject.authenticate!).to eq payload.slice(:uid) }
end

context 'invalid audience' do
Expand Down Expand Up @@ -120,67 +104,6 @@

context 'issued at before future' do
before { payload[:iat] = 3.seconds.ago.to_i }
it('should allow JWT') { expect(subject.authenticate!).to eq user.uid }
end

describe 'on the fly registration' do
context 'token not issued by Barong' do
before { payload[:iss] = 'someone' }
it 'should not register user unless token is issued by Barong' do
expect { subject.authenticate! }.not_to change(User, :count)
end
end

context 'token issued by Barong' do
before { payload[:iss] = 'barong' }

it 'should require level to be present in payload' do
payload.merge!(state: 'pending', uid: Faker::Internet.password(14, 14), email: Faker::Internet.email)
expect { subject.authenticate! }.to raise_error(APIv1::AuthorizationError) { |e| expect(e.reason).to match /key not found: :level/ }
end

it 'should require state to be present in payload' do
payload.merge!(level: 1, uid: Faker::Internet.password(14, 14), email: Faker::Internet.email)
expect { subject.authenticate! }.to raise_error(APIv1::AuthorizationError) { |e| expect(e.reason).to match /key not found: :state/ }
end

it 'should require UID to be present in payload' do
payload.merge!(level: 1, state: 'disabled', email: Faker::Internet.email).delete(:uid)
expect { subject.authenticate! }.to raise_error(APIv1::AuthorizationError) { |e| expect(e.reason).to match /key not found: :uid/ }
end

it 'should require UID to be not blank' do
payload.merge!(level: 1, state: 'disabled', email: Faker::Internet.email, uid: ' ')
expect { subject.authenticate! }.to raise_error(APIv1::AuthorizationError) { |e| expect(e.reason).to match /UID is blank/ }
end

it 'should raise exception when email is invalid' do
payload[:email] = '@gmail.com'
expect { subject.authenticate! }.to raise_error(APIv1::AuthorizationError) { |e| expect(e.reason).to match /invalid/ }
end

it 'should register user' do
payload.merge!(email: 'guyfrombarong@email.com', uid: 'BARONG1234', state: 'active', level: 2)
expect { subject.authenticate! }.to change(User, :count).by(1)
record = User.last
expect(record.uid).to eq payload[:uid]
expect(record.level).to eq 2
end

it 'should update user if exists' do
user = create(:user, :level_1)
payload.merge!(email: Faker::Internet.email, uid: user.uid, state: 'blocked', level: 3)
expect { subject.authenticate! }.not_to change(User, :count)
user.reload
expect(user.uid).to eq payload[:uid]
expect(user.level).to eq 3
end

it 'should register new user and return instance' do
payload.merge!(email: 'guyfrombarong@email.com', uid: 'BARONG1234', state: '', level: 100)
expect(subject.authenticate!(return: :user)).to eq User.last
expect(User.last.level).to eq 100
end
end
it('should allow JWT') { expect(subject.authenticate!).to eq payload.slice(:uid) }
end
end
16 changes: 4 additions & 12 deletions spec/api/v1/auth/middleware_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,14 @@ class TestApp < Grape::API

get '/' do
authenticate!
current_user.uid
current_uid
end
end

let(:app) { TestApp.new }

context 'when using JWT authentication' do
let(:user) { create(:user, :level_3) }
let(:payload) { { x: 'x', y: 'y', z: 'z', uid: user.uid } }
let(:payload) { { x: 'x', y: 'y', z: 'z', uid: 'O90Y88JDPQ7167' } }
let(:token) { jwt_build(payload) }

it 'should deny access when token is not given' do
Expand All @@ -32,17 +31,10 @@ class TestApp < Grape::API
expect(response.body).to eq '{"error":{"code":2001,"message":"Authorization failed"}}'
end

it 'should deny access when member doesn\'t exist' do
payload[:uid] = 'BARONG1234'
api_get '/', token: token
expect(response.code).to eq '401'
expect(response.body).to eq '{"error":{"code":2001,"message":"Authorization failed"}}'
end

it 'should allow access when valid token is given' do
api_get '/', token: token
expect(response).to be_success
expect(response.body).to eq user.uid
expect(response).to be_successful
expect(response.body).to eq payload[:uid]
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/api/v1/cors_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def check_cors(response)
it 'sends CORS headers when requesting using OPTIONS' do
reset! unless integration_session
integration_session.send :process, 'OPTIONS', '/api/v1/withdraws'
expect(response).to be_success
expect(response).to be_successful
check_cors(response)
end

Expand Down

0 comments on commit d2472af

Please sign in to comment.