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
current_user.approved_accounts
end
end
9 changes: 8 additions & 1 deletion app/controllers/concerns/api_authenticated.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,20 @@ module ApiAuthenticated
extend ActiveSupport::Concern

included do
before_filter :authenticate_user!
before_filter :authenticate_user!, :get_authorized_resources
end

def authenticate_user!
user_not_authorized unless current_user
end

def get_authorized_resources
token_accounts = prx_auth_token.authorized_resources.try(:keys)
if token_accounts
current_user.approved_accounts ||= Account.where(id: token_accounts)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want the user model, which is retrieved from the DB and is in the model layer, also aware of the token and its contents which come from the controller layer. Seems like we are having controller / request logic and information leak into the model which should not be aware of or rely on that layer.

The other part of this is that it is could be misleading, because the user may have a different set of accounts they can access than is allowed by and listed in the token.

For example, you could generate a token with only a subset of the actual accounts the user has access to, or using client credentials, get a different account entirely in aur than is reflected in the user's account membership.

So the more I think about it, it seems like with the move to get authorized accounts from the token, not the user, they become 2 different things - there is the user that logged in, and the list of accounts authorized in the token, and they are not necessarily related except for having both been identified in the token.

Copy link
Member

Choose a reason for hiding this comment

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

Curious what @cavis might say about this as well - with the separation of accounts and users, where the token provides the list of accounts not the user, I think it makes sense to move these methods using the list of accounts out of the user model, perhaps into methods on the ApiAuthenticated concern, or just in the relevant controller?

end
end

def cache_show?
false
end
Expand Down
18 changes: 5 additions & 13 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ class User < BaseModel

after_commit :create_individual_account, on: [:create]

attr_accessor :approved_accounts

def individual_account
accounts.where('type = \'IndividualAccount\'').first
end
Expand Down Expand Up @@ -45,25 +47,15 @@ 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
approved_accounts.try(: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])
!approved_accounts.nil? ? Story.where(account_id: approved_accounts.try(:ids)) : []
Copy link
Member

@kookster kookster Jan 31, 2017

Choose a reason for hiding this comment

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

could Story.where(account_id: approved_accounts.try(:ids)) be in a controller method instead of in the user model? doesn't seem to be using anything in the user model, except the convenience that it is the resource loaded by authorized controllers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could certainly be in there! I'm not quite sure where to place the method definitions for approved_account_series and approved_account_stories because they are called in the representers, which would lead me to want to put them in the model layer, but also are called in the controller...

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that is annoying - looks like they do get used for counts in the authorization_representer, but only that one representer at least.

Copy link
Member

@kookster kookster Feb 1, 2017

Choose a reason for hiding this comment

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

We can talk about it when I get in - one way to handle this may be to create an authorization resource based on the token, which could encompass both access to the user, and the list of accounts. That's sorta what feeder does, if you have a look at that, where the auth controller has this for the resource:

  def resource
    Authorization.new(prx_auth_token)
  end

And then the authorization model is basically a wrapper around the token, accessing the related user(well, user_id, feeder would have to make a remote call to getthe user), and could also return the approved_accounts and such:

class Authorization
  include HalApi::RepresentedModel

  attr_accessor :token

  def initialize(token)
    @token = token
  end

  def user_id
    token.user_id
  end
...
end

Copy link
Member

Choose a reason for hiding this comment

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

That would separate the idea of what info comes from the token and it's list of accounts vs. the user and the list of all member accounts, might be better?

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])
!approved_accounts.nil? ? Series.where(account_id: approved_accounts.try(:ids)) : []
end
end
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'
}

Choose a reason for hiding this comment

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

} at 17, 4 is not aligned with StubToken.new(nil, nil, user.id).tap { |t| at 12, 17 or let (:token) { StubToken.new(nil, nil, user.id).tap { |t| at 12, 2.

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
2 changes: 1 addition & 1 deletion test/controllers/api/authorizations_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
describe Api::AuthorizationsController do

let (:user) { create(:user) }
let (:token) { OpenStruct.new.tap { |t| t.user_id = user.id } }
let (:token) { StubToken.new(user.individual_account.id, 'admin', user.id) }

it 'shows the user with a valid token' do
@controller.stub(:prx_auth_token, token) do
Expand Down
13 changes: 9 additions & 4 deletions test/models/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
describe User do
let (:user) { create(:user) }
let (:network) { create(:network, account: user.individual_account) }
let (:story) { create(:story, account: user.individual_account) }
let (:series) { create(:series, account: user.individual_account) }

before do
user.approved_accounts = Account.where(id: [story.account_id, series.account_id])
end

it 'has a table defined' do
User.table_name.must_equal 'users'
Expand Down Expand Up @@ -39,9 +45,8 @@
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
it 'has a list of stories and series for accounts approved on token' do
user.approved_account_stories.must_include story
Copy link
Member

Choose a reason for hiding this comment

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

Are you leaving the User methods like approved_account_stories, even though they aren't being used anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

approved_account_stories and approved_account_series are both used in the Authorization Representer. Do you think that representer should be looking at the stories + series associated with the accounts on the token instead?

user.approved_account_series.must_include series
end
end
4 changes: 4 additions & 0 deletions test/representers/api/authorization_representer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
let(:representer) { Api::AuthorizationRepresenter.new(user) }
let(:json) { JSON.parse(representer.to_json) }

before do
user.approved_accounts = Account.where(id: account.id)
end

def get_link(rel, key = 'href')
json['_links'][rel] ? json['_links'][rel][key] : nil
end
Expand Down
7 changes: 6 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,10 @@ 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