Skip to content
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

Use token list of accounts for user authorization #240

Merged
merged 10 commits into from
Feb 10, 2017
2 changes: 1 addition & 1 deletion app/controllers/api/auth/accounts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@ def resources
end

def resources_base
@accounts ||= current_user.approved_accounts
authorization.token_auth_accounts
end
end
2 changes: 1 addition & 1 deletion app/controllers/api/auth/series_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ class Api::Auth::SeriesController < Api::SeriesController
represent_with Api::Auth::SeriesRepresenter

def resources_base
@series ||= current_user.approved_account_series
@series ||= authorization.token_auth_series
end
end
6 changes: 3 additions & 3 deletions app/controllers/api/auth/stories_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,16 @@ def resources_base
@stories ||= if params[:network_id]
super.published
else
current_user.approved_account_stories
authorization.token_auth_stories
end
end

def create_resource
super.tap do |story|
story.creator_id = current_user.id
story.account_id ||= story.series.try(:account_id)
story.account_id ||= current_user.account_id
story.account_id ||= current_user.approved_accounts.first.try(:id)
story.account_id ||= current_user.account_id if authorization.authorized?(current_user.default_account)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [109/100]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding that check - clever to use the authorized? method, I was thinking of doing a detect on the account list - this is much better

story.account_id ||= authorization.token_auth_accounts.first.try(:id)
end
end
end
2 changes: 1 addition & 1 deletion app/controllers/api/authorizations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ class Api::AuthorizationsController < Api::BaseController
private

def resource
current_user
authorization
end
end
4 changes: 4 additions & 0 deletions app/controllers/concerns/api_authenticated.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ def authenticate_user!
user_not_authorized unless current_user
end

def authorization
Authorization.new(prx_auth_token)
end

def cache_show?
false
end
Expand Down
35 changes: 35 additions & 0 deletions app/models/authorization.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# encoding: utf-8

class Authorization
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this whole class.

attr_accessor :token
attr_reader :token_auth_accounts

def initialize(token)
@token = token
end

def id
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this because the AuthorizationRepresenter wants an id to build links ... misguided?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does it want the id for? the self link? I don't think you should need to provide an id for the representer, can we track this down?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is the self link, which is all I can think of, we can override the self_link method on the representer to not need this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug in hal gem - PRX/hal_api-rails#8

default_account.id
end

def default_account
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly, added this because AuthorizationRepresenter wants to return a default_account

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, makes sense to me

User.find(token.user_id).default_account
end

def token_auth_accounts
token_ids = token.authorized_resources.try(:keys)
@token_auth_accounts = Account.where(id: token_ids) if token_ids
end

def token_auth_stories
Story.where(account_id: token_auth_accounts.try(:ids)) unless token_auth_accounts.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need the try when you have the unless nil? already? Fine to leave it this way, I am a belt and suspenders kind of person myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just doing belt + suspenders!

end

def token_auth_series
Series.where(account_id: token_auth_accounts.try(:ids)) unless token_auth_accounts.nil?
end

def authorized?(account)
token_auth_accounts.include?(account)
end
end
22 changes: 0 additions & 22 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,26 +44,4 @@ def networks
def name
"#{first_name} #{last_name}"
end

def approved_accounts
Account.
joins('LEFT OUTER JOIN `memberships` ON `memberships`.`account_id` = `accounts`.`id`').
where(['memberships.user_id = ? and memberships.approved is true', id])
end

def approved_active_accounts
approved_accounts.active
end

def approved_account_stories
Story.
joins('LEFT OUTER JOIN `memberships` ON `memberships`.`account_id` = `pieces`.`account_id`').
where(['memberships.user_id = ? and memberships.approved is true', id])
end

def approved_account_series
Series.
joins('LEFT OUTER JOIN `memberships` ON `memberships`.`account_id` = `series`.`account_id`').
where(['memberships.user_id = ? and memberships.approved is true', id])
end
end
11 changes: 5 additions & 6 deletions app/representers/api/authorization_representer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

class Api::AuthorizationRepresenter < Api::BaseRepresenter
property :id, writeable: false
property :name

link :default_account do
{
Expand All @@ -15,10 +14,10 @@ class Api::AuthorizationRepresenter < Api::BaseRepresenter
{
href: "#{api_authorization_accounts_path}#{index_url_params}",
templated: true,
count: represented.accounts.count
count: represented.token_auth_accounts.count
}
end
embed :approved_active_accounts, as: :accounts, paged: true, per: :all, item_class: Account,
embed :token_auth_accounts, as: :accounts, paged: true, per: :all, item_class: Account,
item_decorator: Api::Auth::AccountMinRepresenter,
url: ->(_r) { api_authorization_accounts_path }

Expand All @@ -27,12 +26,12 @@ class Api::AuthorizationRepresenter < Api::BaseRepresenter
{
href: "#{api_authorization_series_path_template(id: '{id}')}#{show_url_params}",
templated: true,
count: represented.approved_account_series.count
count: represented.token_auth_series.count
},
{
href: "#{api_authorization_series_index_path}#{index_url_params}",
templated: true,
count: represented.approved_account_series.count
count: represented.token_auth_series.count
}
]
end
Expand All @@ -41,7 +40,7 @@ class Api::AuthorizationRepresenter < Api::BaseRepresenter
{
href: "#{api_authorization_stories_path}#{index_url_params}",
templated: true,
count: represented.approved_account_stories.count
count: represented.token_auth_stories.count
}
end

Expand Down
21 changes: 10 additions & 11 deletions test/controllers/api/auth/accounts_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,17 @@
describe Api::Auth::AccountsController do

let (:user) { create(:user_with_accounts, group_accounts: 2) }
let (:token) { OpenStruct.new.tap { |t| t.user_id = user.id } }
let (:individual_account) { user.individual_account }
let (:member_account) { user.accounts.member.first }
let (:unapproved_account) { user.accounts.member.last }
let (:random_account) { create(:account) }
before { unapproved_account.memberships.first.update!(approved: false) }
let (:member_account) { create(:account) }
let (:unapproved_account) { create(:account) }
let (:token) { StubToken.new(nil, nil, user.id) }

before do
token.authorized_resources = {
member_account.id => 'member',
individual_account.id => 'admin'
}
end

describe 'with a valid token' do

Expand All @@ -31,11 +36,6 @@
assert_response :not_found
end

it 'does not show non-member accounts' do
get(:show, api_version: 'v1', id: random_account.id)
assert_response :not_found
end

it 'indexes only member accounts' do
get(:index, api_version: 'v1')
assert_response :success
Expand All @@ -46,7 +46,6 @@
ids.must_include individual_account.id
ids.must_include member_account.id
ids.wont_include unapproved_account.id
ids.wont_include random_account.id
end

end
Expand Down
9 changes: 1 addition & 8 deletions test/controllers/api/authorizations_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,7 @@
describe Api::AuthorizationsController do

let (:user) { create(:user) }
let (:token) { OpenStruct.new.tap { |t| t.user_id = user.id } }

it 'shows the user with a valid token' do
@controller.stub(:prx_auth_token, token) do
get(:show, api_version: 'v1')
assert_response :success
end
end
let (:token) { StubToken.new(user.individual_account.id, 'admin', user.id) }

it 'returns unauthorized with invalid token' do
@controller.stub(:prx_auth_token, OpenStruct.new) do
Expand Down
10 changes: 10 additions & 0 deletions test/controllers/concerns/api_authenticated_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ def current_user
def user_not_authorized
raise 'user_not_authorized'
end

def prx_auth_token; end
end

let(:controller) { ApiAuthenticatedTestController.new }
Expand All @@ -33,4 +35,12 @@ def user_not_authorized
err.message.must_match /user_not_authorized/
end

it 'builds an authorization from token' do
controller.stub(:current_user, true) do
controller.stub(:prx_auth_token, StubToken.new(123, 'admin', nil)) do
controller.authorization.wont_be_nil
end
end
end

end
26 changes: 26 additions & 0 deletions test/models/authorization_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require 'test_helper'

describe Authorization do
let(:account) { create(:account) }
let(:token) { StubToken.new(account.id, ['member'], 456) }
let(:authorization) { Authorization.new(token) }
let(:unauth_account) { create(:account) }

it 'has a token' do
authorization.token.wont_be_nil
end

it 'has a list of accounts authorized on token' do
authorization.token_auth_accounts.must_include account
end

it 'has lists of stories and series belonging to accounts authorized on token' do
authorization.token_auth_stories.must_equal account.stories
authorization.token_auth_series.must_equal account.series
end

it 'checks against token to see if accounts are authorized' do
authorization.authorized?(account).must_equal true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you could also write this as:
authorization.must_be :authorized?, account

just a point of information - this test is great

authorization.authorized?(unauth_account).must_equal false
end
end
6 changes: 0 additions & 6 deletions test/models/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,4 @@
it 'has a list of networks' do
user.networks.must_include network
end

it 'has a list of stories for approved accounts' do
start_count = user.individual_account.stories.count
create(:account, user: user, stories_count: 2)
user.approved_account_stories.count.must_equal start_count + 2
end
end
4 changes: 3 additions & 1 deletion test/representers/api/authorization_representer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@

let(:user) { create(:user) }
let(:account) { user.default_account }
let(:representer) { Api::AuthorizationRepresenter.new(user) }
let(:token) { StubToken.new(account.id, 'admin', user.id) }
let(:authorization) { Authorization.new(token) }
let(:representer) { Api::AuthorizationRepresenter.new(authorization) }
let(:json) { JSON.parse(representer.to_json) }

def get_link(rel, key = 'href')
Expand Down
6 changes: 5 additions & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,11 @@ def extract_filename(uri)

StubToken = Struct.new(:resource, :scopes, :user_id)
class StubToken
attr_accessor :authorized_resources
@@fake_user_id = 0

def initialize(res, scopes, explicit_user_id = nil)
@authorized_resources = { res => scopes }
if explicit_user_id
super(res.to_s, scopes, explicit_user_id)
else
Expand All @@ -74,7 +76,9 @@ def initialize(res, scopes, explicit_user_id = nil)
end

def authorized?(r, s = nil)
resource == r.to_s && (s.nil? || scopes.include?(s.to_s))
res = authorized_resources.keys
roles = authorized_resources.values.flatten
res.include?(r) && (s.nil? || roles.include?(s.to_s))
end
end

Expand Down