Skip to content

Commit

Permalink
Add Api:SessionController tests
Browse files Browse the repository at this point in the history
  • Loading branch information
potomak committed Oct 6, 2016
1 parent 3e96597 commit 8a7a1f2
Show file tree
Hide file tree
Showing 7 changed files with 102 additions and 42 deletions.
12 changes: 5 additions & 7 deletions app/controllers/api/base_controller.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
module API
class BaseController < ActionController::Base
skip_before_action :verify_authenticity_token
class Api::BaseController < ActionController::Base

This comment has been minimized.

Copy link
@bugant

bugant Oct 6, 2016

Member

Why are you removing the module declaration?

This comment has been minimized.

Copy link
@dalpo

dalpo Oct 6, 2016

Member

I would be good to use a rubocop rule to explicit the project convention about this.

This comment has been minimized.

Copy link
@potomak

potomak Oct 6, 2016

Author Member

No reason, I was just following Rails convention, but I personally prefer the module declaration, so I think I will reintroduce it.

skip_before_action :verify_authenticity_token

private
private

def unauthorized(reason)
render json: {error: reason}
end
def unauthorized(reason)
render status: :unauthorized, json: { error: reason }
end
end
26 changes: 0 additions & 26 deletions app/controllers/api/session_controller.rb

This file was deleted.

23 changes: 23 additions & 0 deletions app/controllers/api/sessions_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
class Api::SessionsController < Api::BaseController
def create

This comment has been minimized.

Copy link
@bugant

bugant Oct 6, 2016

Member

I would call this github_create since this is only handling the github auth case.

This comment has been minimized.

Copy link
@potomak

potomak Oct 6, 2016

Author Member

I changed the name on purpose, I want it to handle different cases based on a request params provider attribute.

This comment has been minimized.

Copy link
@bugant

bugant Oct 6, 2016

Member

OK, I think that would be OK, we can later add additional specific methods if the code will get too long

begin
client = Octokit::Client.new(access_token: params[:access_token])
github_uid = client.user.try(:[], 'id')
user = User.find_by_auth_provider(provider: 'github', uid: github_uid)

if user.nil?
# TODO: create a new user
raise StandardError('TODO: create a new user')
end

tomatoes_auth = user.authorizations.where(provider: 'tomatoes').first_or_initialize
tomatoes_auth.refresh_token unless tomatoes_auth.token

This comment has been minimized.

Copy link
@bugant

bugant Oct 6, 2016

Member

I do not agree here... I'd always refresh the token: I'm logging in so I'm requesting for a new token.

This comment has been minimized.

Copy link
@bugant

bugant Oct 6, 2016

Member

@potomak I would like to discuss this issue here. I think it is important to assign a new token for every new login.

This comment has been minimized.

Copy link
@potomak

potomak Oct 6, 2016

Author Member

I don't think it would be right. If you always refresh the token you would never allow two different clients from having an open session at the same time. I think the method name is misleading, I'd like to call it generate_token, at that point tomatoes_auth.generate_token unless tomatoes_auth.token would sound better. Do you agree?

tomatoes_auth.save!

render json: { token: tomatoes_auth.token }
rescue => err
Rails.logger.error("Cannot log user in using GitHub: #{err}")
unauthorized('authentication failed')
end
end
end
6 changes: 2 additions & 4 deletions app/models/authorization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,7 @@ def url
"http://#{provider}.com/#{nickname}"
end

def api_authorize
new_token = SecureRandom.hex
self.token = new_token
return new_token
def refresh_token
self.token = SecureRandom.hex
end
end
15 changes: 10 additions & 5 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,17 @@ class User
index('authorizations.provider' => 1)

def self.find_by_omniauth(auth)
find_by_auth_provider(provider: auth['provider'].to_s, uid: auth['uid'].to_s)
end

def self.find_by_auth_provider(provider:, uid:)
any_of(
{ authorizations: {
'$elemMatch' => {
provider: auth['provider'].to_s,
uid: auth['uid'].to_s } } },
provider: auth['provider'], uid: auth['uid']
{
authorizations: {
'$elemMatch' => { provider: provider, uid: uid }

This comment has been minimized.

Copy link
@bugant

bugant Oct 6, 2016

Member

I don't undestand this syntax.. can you please give me some info about it? Also, how is the any_of working?

This comment has been minimized.

Copy link
@potomak

potomak Oct 6, 2016

Author Member

That syntax is a mess, sorry. That should put two conditions in or. The two conditions are the one on the authorizations embedded collection and the one that filters by provider and uid. Note: the second condition is there only to support an old version of the user documents, see #169.

}
},
provider: provider, uid: uid
).first
end

Expand Down
4 changes: 4 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
# The priority is based upon order of creation: first created -> highest priority.
# See how all your routes lay out with "rake routes".

namespace :api do
resource :session, only: [:create]
end

resources :tags, only: [:index, :show]

resources :projects
Expand Down
58 changes: 58 additions & 0 deletions test/functional/api/sessions_controller_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
require 'test_helper'

class Api::SessionsControllerTest < ActionController::TestCase
setup do
@github_user = User.create!(
name: 'name',
email: '[email protected]'
)
@github_user.authorizations.create!(
provider: 'github',
uid: 'github_user_id'
)

@github_user_with_api_auth = User.create!(
name: 'name',
email: '[email protected]'
)
@github_user_with_api_auth.authorizations.create!(
provider: 'github',
uid: 'github_user_with_api_auth_id'
)
@github_user_with_api_auth.authorizations.create!(
provider: 'tomatoes',
token: 'tomatoes_token'
)
end

teardown do
@github_user.destroy
@github_user_with_api_auth.destroy
end

test 'given a github access token it should create a new session' do
github_client = Octokit::Client.new
github_client.expects(:user).returns('id' => 'github_user_id')
Octokit::Client.expects(:new).with(access_token: 'github_access_token').returns(github_client)

assert_difference('@github_user.reload.authorizations.count') do
post :create, provider: 'github', access_token: 'github_access_token'
end
assert_response :success
assert_equal 'application/json', @response.content_type
assert JSON.parse(@response.body).has_key?('token')
end

test 'given a github access token it should return an existing session' do
github_client = Octokit::Client.new
github_client.expects(:user).returns('id' => 'github_user_with_api_auth_id')
Octokit::Client.expects(:new).with(access_token: 'github_access_token').returns(github_client)

assert_no_difference('@github_user_with_api_auth.reload.authorizations.count') do
post :create, provider: 'github', access_token: 'github_access_token'
end
assert_response :success
assert_equal 'application/json', @response.content_type
assert_equal({ token: 'tomatoes_token' }.to_json, @response.body)
end
end

0 comments on commit 8a7a1f2

Please sign in to comment.