From a61176a2654c7142fda550ea92db4693f532f9c9 Mon Sep 17 00:00:00 2001 From: Merric de Launey Date: Mon, 11 Nov 2019 09:55:28 -0800 Subject: [PATCH] Allow for include parameter on listing and showing roles * GET /v3/roles?include=user & GET /v3/roles/:guid?include=user endpoints * Moved a controller function that makes the call to UAA to get usernames and origins into the user model so it could be shared by the decorators * Updated docs [#164176870] Co-authored-by: Merric de Launey Co-authored-by: Mona Mohebbi --- app/controllers/v3/roles_controller.rb | 16 ++- app/controllers/v3/users_controller.rb | 13 +- app/decorators/include_role_user_decorator.rb | 21 +++ app/messages/role_show_message.rb | 14 ++ app/messages/roles_list_message.rb | 6 +- app/models/runtime/user.rb | 5 + app/presenters/v3/role_presenter.rb | 3 +- .../experimental_resources/roles/_get.md.erb | 6 + .../experimental_resources/roles/_list.md.erb | 2 + spec/request/roles_spec.rb | 134 ++++++++++++++++-- .../include_role_user_decorator_spec.rb | 62 ++++++++ spec/unit/messages/role_show_message_spec.rb | 18 +++ spec/unit/messages/roles_list_message_spec.rb | 16 +++ 13 files changed, 291 insertions(+), 25 deletions(-) create mode 100644 app/decorators/include_role_user_decorator.rb create mode 100644 app/messages/role_show_message.rb create mode 100644 spec/unit/decorators/include_role_user_decorator_spec.rb create mode 100644 spec/unit/messages/role_show_message_spec.rb diff --git a/app/controllers/v3/roles_controller.rb b/app/controllers/v3/roles_controller.rb index 836279d233f..cfd7bd62085 100644 --- a/app/controllers/v3/roles_controller.rb +++ b/app/controllers/v3/roles_controller.rb @@ -1,10 +1,12 @@ require 'messages/role_create_message' require 'messages/roles_list_message' +require 'messages/role_show_message' require 'fetchers/role_list_fetcher' require 'actions/role_create' require 'actions/role_guid_populate' require 'actions/role_delete' require 'presenters/v3/role_presenter' +require 'decorators/include_role_user_decorator' class RolesController < ApplicationController def create @@ -29,19 +31,29 @@ def index RoleGuidPopulate.populate roles = RoleListFetcher.fetch(message, readable_roles) + decorators = [] + decorators << IncludeRoleUserDecorator if IncludeRoleUserDecorator.match?(message.include) + render status: :ok, json: Presenters::V3::PaginatedListPresenter.new( presenter: Presenters::V3::RolePresenter, paginated_result: SequelPaginator.new.get_page(roles, message.try(:pagination_options)), path: '/v3/roles', - message: message + message: message, + decorators: decorators, ) end def show + message = RoleShowMessage.from_params(query_params) + unprocessable!(message.errors.full_messages) unless message.valid? + + decorators = [] + decorators << IncludeRoleUserDecorator if IncludeRoleUserDecorator.match?(message.include) + role = readable_roles.first(guid: hashed_params[:guid]) resource_not_found!(:role) unless role - render status: :ok, json: Presenters::V3::RolePresenter.new(role) + render status: :ok, json: Presenters::V3::RolePresenter.new(role, decorators: decorators) end def destroy diff --git a/app/controllers/v3/users_controller.rb b/app/controllers/v3/users_controller.rb index 2d8da8b6991..33412fa7652 100644 --- a/app/controllers/v3/users_controller.rb +++ b/app/controllers/v3/users_controller.rb @@ -20,7 +20,7 @@ def index paginated_result: SequelPaginator.new.get_page(users, message.try(:pagination_options)), path: '/v3/users', message: message, - extra_presenter_args: { uaa_users: uaa_users_info(user_guids) }, + extra_presenter_args: { uaa_users: User.uaa_users_info(user_guids) }, ) end @@ -31,7 +31,7 @@ def create unprocessable!(message.errors.full_messages) unless message.valid? user = UserCreate.new.create(message: message) - render status: :created, json: Presenters::V3::UserPresenter.new(user, uaa_users: uaa_users_info([user.guid])) + render status: :created, json: Presenters::V3::UserPresenter.new(user, uaa_users: User.uaa_users_info([user.guid])) rescue UserCreate::Error => e unprocessable!(e) end @@ -40,7 +40,7 @@ def show user = fetch_user_if_readable(hashed_params[:guid]) user_not_found! unless user - render status: :ok, json: Presenters::V3::UserPresenter.new(user, uaa_users: uaa_users_info([user.guid])) + render status: :ok, json: Presenters::V3::UserPresenter.new(user, uaa_users: User.uaa_users_info([user.guid])) end def destroy @@ -68,7 +68,7 @@ def update user = UserUpdate.new.update(user: user, message: message) - render status: :ok, json: Presenters::V3::UserPresenter.new(user, uaa_users: uaa_users_info([hashed_params[:guid]])) + render status: :ok, json: Presenters::V3::UserPresenter.new(user, uaa_users: User.uaa_users_info([hashed_params[:guid]])) end private @@ -83,11 +83,6 @@ def fetch_user_if_readable(desired_guid) readable_users.first(guid: desired_guid) end - def uaa_users_info(user_guids) - uaa_client = CloudController::DependencyLocator.instance.uaa_client - uaa_client.users_for_ids(user_guids) - end - def user_not_found! resource_not_found!(:user) end diff --git a/app/decorators/include_role_user_decorator.rb b/app/decorators/include_role_user_decorator.rb new file mode 100644 index 00000000000..8501fc664ae --- /dev/null +++ b/app/decorators/include_role_user_decorator.rb @@ -0,0 +1,21 @@ +# require 'models/runtime/user' + +module VCAP::CloudController + class IncludeRoleUserDecorator + class << self + def match?(include_params) + include_params&.include?('user') + end + + def decorate(hash, roles) + hash[:included] ||= {} + user_guids = roles.map(&:user_guid).uniq + users = User.where(guid: user_guids).order(:created_at) + uaa_users = User.uaa_users_info(user_guids) + + hash[:included][:users] = users.map { |user| Presenters::V3::UserPresenter.new(user, uaa_users: uaa_users).to_hash } + hash + end + end + end +end diff --git a/app/messages/role_show_message.rb b/app/messages/role_show_message.rb new file mode 100644 index 00000000000..fbea104a306 --- /dev/null +++ b/app/messages/role_show_message.rb @@ -0,0 +1,14 @@ +require 'messages/base_message' + +module VCAP::CloudController + class RoleShowMessage < BaseMessage + register_allowed_keys [:include] + + validates_with NoAdditionalParamsValidator + validates_with IncludeParamValidator, valid_values: ['user'] + + def self.from_params(params) + super(params, %w(include)) + end + end +end diff --git a/app/messages/roles_list_message.rb b/app/messages/roles_list_message.rb index a72b3f055aa..c48f57f78cf 100644 --- a/app/messages/roles_list_message.rb +++ b/app/messages/roles_list_message.rb @@ -7,10 +7,12 @@ class RolesListMessage < ListMessage :organization_guids, :space_guids, :user_guids, - :types + :types, + :include ] validates_with NoAdditionalParamsValidator + validates_with IncludeParamValidator, valid_values: ['user'] validates :guids, allow_nil: true, array: true validates :organization_guids, allow_nil: true, array: true @@ -21,7 +23,7 @@ class RolesListMessage < ListMessage def self.from_params(params) params['order_by'] ||= 'created_at' - super(params, %w(guids organization_guids space_guids user_guids types)) + super(params, %w(guids organization_guids space_guids user_guids types include)) end end end diff --git a/app/models/runtime/user.rb b/app/models/runtime/user.rb index aee47d24d17..84e26249839 100644 --- a/app/models/runtime/user.rb +++ b/app/models/runtime/user.rb @@ -201,6 +201,11 @@ def visible_users_in_my_orgs distinct end + def self.uaa_users_info(user_guids) + uaa_client = CloudController::DependencyLocator.instance.uaa_client + uaa_client.users_for_ids(user_guids) + end + def self.readable_users_for_current_user(can_read_globally, current_user) if can_read_globally User.dataset diff --git a/app/presenters/v3/role_presenter.rb b/app/presenters/v3/role_presenter.rb index c435b3e4a3e..9c9836e0b81 100644 --- a/app/presenters/v3/role_presenter.rb +++ b/app/presenters/v3/role_presenter.rb @@ -3,7 +3,7 @@ module VCAP::CloudController::Presenters::V3 class RolePresenter < BasePresenter def to_hash - { + hash = { guid: role.guid, created_at: role.created_at, updated_at: role.updated_at, @@ -11,6 +11,7 @@ def to_hash relationships: build_relationships, links: build_links } + @decorators.reduce(hash) { |memo, d| d.decorate(memo, [role]) } end private diff --git a/docs/v3/source/includes/experimental_resources/roles/_get.md.erb b/docs/v3/source/includes/experimental_resources/roles/_get.md.erb index ee62d6b445f..0fa3c83a751 100644 --- a/docs/v3/source/includes/experimental_resources/roles/_get.md.erb +++ b/docs/v3/source/includes/experimental_resources/roles/_get.md.erb @@ -27,6 +27,12 @@ This endpoint gets an individual role resource. `GET /v3/roles/:guid` +#### Query parameters + +Name | Type | Description +---- | ---- | ------------ +**include** | _string_ | **Experimental** - Optionally include a list of unique related resources in the response.
The only valid value is `user` + #### Permitted roles Role | Notes diff --git a/docs/v3/source/includes/experimental_resources/roles/_list.md.erb b/docs/v3/source/includes/experimental_resources/roles/_list.md.erb index 354a0916278..51c037e7d62 100644 --- a/docs/v3/source/includes/experimental_resources/roles/_list.md.erb +++ b/docs/v3/source/includes/experimental_resources/roles/_list.md.erb @@ -39,6 +39,8 @@ Name | Type | Description **page** | _integer_ | Page to display. Valid values are integers >= 1. **per_page** | _integer_ | Number of results per page.
Valid values are 1 through 5000. **order_by** | _string_ | Value to sort by. Defaults to ascending. Prepend with `-` to sort descending.
Valid values are `created_at`, `updated_at`. +**include** | _string_ | **Experimental** - Optionally include a list of unique related resources in the response.
The only valid value is `user` + #### Permitted roles | diff --git a/spec/request/roles_spec.rb b/spec/request/roles_spec.rb index 293a9aeec37..9cf6252fff1 100644 --- a/spec/request/roles_spec.rb +++ b/spec/request/roles_spec.rb @@ -10,6 +10,11 @@ let(:user_guid) { user.guid } let(:space_guid) { space.guid } let(:user_unaffiliated) { VCAP::CloudController::User.make(guid: 'user_no_role') } + let(:uaa_client) { double(:uaa_client) } + + before do + allow(CloudController::DependencyLocator.instance).to receive(:uaa_client).and_return(uaa_client) + end describe 'POST /v3/roles' do let(:api_call) { lambda { |user_headers| post '/v3/roles', params.to_json, user_headers } } @@ -127,11 +132,9 @@ let(:uaa_client) { double(:uaa_client) } before do - allow(CloudController::DependencyLocator.instance).to receive(:uaa_client).and_return(uaa_client) allow(uaa_client).to receive(:users_for_ids).with([user_with_role.guid]).and_return( { user_with_role.guid => { 'username' => 'mona', 'origin' => 'uaa' } } ) - org.add_user(user_with_role) post '/v3/roles', params.to_json, admin_header end @@ -253,7 +256,6 @@ let(:uaa_client) { double(:uaa_client) } before do - allow(CloudController::DependencyLocator.instance).to receive(:uaa_client).and_return(uaa_client) allow(uaa_client).to receive(:users_for_ids).with([user_with_role.guid]).and_return( { user_with_role.guid => { 'username' => 'mona', 'origin' => 'uaa' } } ) @@ -337,7 +339,6 @@ end before do - allow(CloudController::DependencyLocator.instance).to receive(:uaa_client).and_return(uaa_client) allow(uaa_client).to receive(:origins_for_username).with('uuu').and_return(['uaa']) allow(uaa_client).to receive(:id_for_username).with('uuu', origin: 'uaa').and_return(user_with_role.guid) @@ -349,7 +350,6 @@ context 'when there are multiple users with the same username' do before do - allow(CloudController::DependencyLocator.instance).to receive(:uaa_client).and_return(uaa_client) allow(uaa_client).to receive(:origins_for_username).with('uuu').and_return(%w(uaa ldap okta)) allow(uaa_client).to receive(:id_for_username).with('uuu', origin: 'uaa').and_return(user_with_role.guid) end @@ -366,7 +366,6 @@ context 'when there is no user with the given username' do before do - allow(CloudController::DependencyLocator.instance).to receive(:uaa_client).and_return(uaa_client) allow(uaa_client).to receive(:origins_for_username).with('uuu').and_return([]) allow(uaa_client).to receive(:id_for_username).with('uuu', origin: nil).and_return(nil) end @@ -382,7 +381,7 @@ end end - context 'creating role by user GUID as org manager for unaffiliated user' do + context 'creating role by user GUID for unaffiliated user' do let(:params) do { type: 'organization_auditor', @@ -502,7 +501,6 @@ let(:uaa_client) { double(:uaa_client) } before do - allow(CloudController::DependencyLocator.instance).to receive(:uaa_client).and_return(uaa_client) allow(uaa_client).to receive(:id_for_username).with('uuu', origin: 'okta').and_return(user_with_role.guid) org.add_user(user_with_role) @@ -548,7 +546,6 @@ context 'when there is no user with the given username and origin' do before do - allow(CloudController::DependencyLocator.instance).to receive(:uaa_client).and_return(uaa_client) allow(uaa_client).to receive(:origins_for_username).with('uuu').and_return(['something-else']) allow(uaa_client).to receive(:id_for_username).with('uuu', origin: 'okta').and_return(nil) end @@ -567,7 +564,6 @@ # granted any space roles context 'as org manager for unaffiliated user' do before do - allow(CloudController::DependencyLocator.instance).to receive(:uaa_client).and_return(uaa_client) allow(uaa_client).to receive(:origins_for_username).with('bob_unaffiliated').and_return(['uaa']) allow(uaa_client).to receive(:id_for_username).with('bob_unaffiliated', origin: 'uaa').and_return(user_unaffiliated.guid) end @@ -652,7 +648,7 @@ describe 'GET /v3/roles' do let(:api_call) { lambda { |user_headers| get '/v3/roles', nil, user_headers } } - let(:other_user) { VCAP::CloudController::User.make } + let(:other_user) { VCAP::CloudController::User.make(guid: 'other-user-guid') } let!(:space_auditor) do VCAP::CloudController::SpaceAuditor.make( @@ -871,6 +867,87 @@ def make_space_role_for_current_user(type) it_behaves_like 'permissions for list endpoint', ALL_PERMISSIONS end + + context 'listing roles with include' do + let(:other_user_response) do + { + 'guid' => other_user.guid, + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'username' => 'other_user_name', + 'presentation_name' => 'other_user_name', # username is nil, so presenter defaults to guid + 'origin' => 'uaa', + 'metadata' => { + 'labels' => {}, + 'annotations' => {}, + }, + 'links' => { + 'self' => { 'href' => %r(#{Regexp.escape(link_prefix)}\/v3\/users\/#{other_user.guid}) }, + } + } + end + + before do + allow(uaa_client).to receive(:users_for_ids).with([other_user.guid]).and_return( + { other_user.guid => { 'username' => 'other_user_name', 'origin' => 'uaa' } } + ) + end + + it 'includes the requested users' do + get('/v3/roles?include=user', nil, admin_header) + expect(last_response.status).to eq(200) + + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response['included']['users'][0]).to be_a_response_like(other_user_response) + end + + context 'when there are multiple users with multiple roles' do + let(:another_user) { VCAP::CloudController::User.make(guid: 'another-user-guid') } + let(:another_user_response) do + { + 'guid' => another_user.guid, + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'username' => 'another_user_name', + 'presentation_name' => 'another_user_name', # username is nil, so presenter defaults to guid + 'origin' => 'uaa', + 'metadata' => { + 'labels' => {}, + 'annotations' => {}, + }, + 'links' => { + 'self' => { 'href' => %r(#{Regexp.escape(link_prefix)}\/v3\/users\/#{another_user.guid}) }, + } + } + end + let!(:organization_billing_manager) do + VCAP::CloudController::OrganizationBillingManager.make( + guid: 'organization_billing_manager-guid', + organization: org, + user: another_user, + created_at: Time.now - 3.minutes, + ) + end + + before do + allow(uaa_client).to receive(:users_for_ids).with([other_user.guid, another_user.guid]).and_return( + { + another_user.guid => { 'username' => 'another_user_name', 'origin' => 'uaa' }, + other_user.guid => { 'username' => 'other_user_name', 'origin' => 'uaa' } + } + ) + end + + it 'returns all of the relevant users' do + get('/v3/roles?include=user', nil, admin_header) + expect(last_response.status).to eq(200) + + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response['included']['users'][0]).to be_a_response_like(other_user_response) + expect(parsed_response['included']['users'][1]).to be_a_response_like(another_user_response) + end + end + end end describe 'GET /v3/roles/:guid' do @@ -973,6 +1050,41 @@ def make_space_role_for_current_user(type) expect(last_response).to have_error_message('Authentication error') end end + + context 'listing roles with include' do + let(:role) { VCAP::CloudController::SpaceAuditor.make(user: user_with_role, space: space) } + let(:user_with_role_response) do + { + 'guid' => user_with_role.guid, + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'username' => 'user_name', + 'presentation_name' => 'user_name', # username is nil, so presenter defaults to guid + 'origin' => 'uaa', + 'metadata' => { + 'labels' => {}, + 'annotations' => {}, + }, + 'links' => { + 'self' => { 'href' => %r(#{Regexp.escape(link_prefix)}\/v3\/users\/#{user_with_role.guid}) }, + } + } + end + + before do + allow(uaa_client).to receive(:users_for_ids).with([user_with_role.guid]).and_return( + { user_with_role.guid => { 'username' => 'user_name', 'origin' => 'uaa' } } + ) + end + + it 'includes the requested users' do + get("/v3/roles/#{role.guid}?include=user", nil, admin_header) + expect(last_response.status).to eq(200) + + parsed_response = MultiJson.load(last_response.body) + expect(parsed_response['included']['users'][0]).to be_a_response_like(user_with_role_response) + end + end end describe 'DELETE /v3/roles/:guid' do diff --git a/spec/unit/decorators/include_role_user_decorator_spec.rb b/spec/unit/decorators/include_role_user_decorator_spec.rb new file mode 100644 index 00000000000..b9c7fd4316a --- /dev/null +++ b/spec/unit/decorators/include_role_user_decorator_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' +require 'decorators/include_role_user_decorator' + +module VCAP::CloudController + RSpec.describe IncludeRoleUserDecorator do + subject(:decorator) { IncludeRoleUserDecorator } + let(:user1) { User.make(guid: 'user-1-guid') } + let(:user2) { User.make(guid: 'user-2-guid') } + let(:roles) do + [ + SpaceDeveloper.make(user: user1), + OrganizationManager.make(user: user2), + ] + end + + describe '#decorate' do + let(:uaa_client) { double(:uaa_client) } + let(:user_uaa_info) do + { + user1.guid => { 'username' => 'user-1-name', 'origin' => 'uaa' }, + user2.guid => { 'username' => 'user-2-name', 'origin' => 'uaa' } + } + end + + before do + allow(CloudController::DependencyLocator.instance).to receive(:uaa_client).and_return(uaa_client) + allow(uaa_client).to receive(:users_for_ids).with([user1.guid, user2.guid]).and_return(user_uaa_info) + end + + it 'decorates the given hash with associated users' do + undecorated_hash = { foo: 'bar' } + hash = subject.decorate(undecorated_hash, roles) + expect(hash[:foo]).to eq('bar') + expect(hash[:included][:users]).to match_array([ + Presenters::V3::UserPresenter.new(user1, uaa_users: user_uaa_info).to_hash, + Presenters::V3::UserPresenter.new(user2, uaa_users: user_uaa_info).to_hash + ]) + end + + it 'does not overwrite other included fields' do + undecorated_hash = { foo: 'bar', included: { monkeys: ['zach', 'greg'] } } + hash = subject.decorate(undecorated_hash, roles) + expect(hash[:foo]).to eq('bar') + expect(hash[:included][:users]).to match_array([ + Presenters::V3::UserPresenter.new(user1, uaa_users: user_uaa_info).to_hash, + Presenters::V3::UserPresenter.new(user2, uaa_users: user_uaa_info).to_hash + ]) + expect(hash[:included][:monkeys]).to match_array(['zach', 'greg']) + end + end + + describe '#match?' do + it 'matches include arrays containing "user"' do + expect(decorator.match?(['potato', 'user', 'turnip'])).to be_truthy + end + + it 'does not match other include arrays' do + expect(decorator.match?(['potato', 'turnip'])).to be_falsey + end + end + end +end diff --git a/spec/unit/messages/role_show_message_spec.rb b/spec/unit/messages/role_show_message_spec.rb new file mode 100644 index 00000000000..ec302a266a6 --- /dev/null +++ b/spec/unit/messages/role_show_message_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +module VCAP::CloudController + RSpec.describe RoleShowMessage do + it 'does not accept fields not in the set' do + message = RoleShowMessage.from_params({ 'foobar' => 'pants' }) + expect(message).not_to be_valid + expect(message.errors[:base]).to include("Unknown query parameter(s): 'foobar'") + end + + it 'does not accept include that is not user' do + message = RoleShowMessage.from_params({ 'include' => 'user' }) + expect(message).to be_valid + message = RoleShowMessage.from_params({ 'include' => 'greg\'s buildpack' }) + expect(message).not_to be_valid + end + end +end diff --git a/spec/unit/messages/roles_list_message_spec.rb b/spec/unit/messages/roles_list_message_spec.rb index 4e99cdecca4..6e92b68c53f 100644 --- a/spec/unit/messages/roles_list_message_spec.rb +++ b/spec/unit/messages/roles_list_message_spec.rb @@ -14,6 +14,7 @@ module VCAP::CloudController 'user_guids' => 'my-user-guid', 'space_guids' => 'my-space-guid', 'organization_guids' => 'my-organization-guid', + 'include' => 'user', } end @@ -28,6 +29,7 @@ module VCAP::CloudController expect(message.user_guids).to eq(['my-user-guid']) expect(message.space_guids).to eq(['my-space-guid']) expect(message.organization_guids).to eq(['my-organization-guid']) + expect(message.include).to eq(['user']) end it 'converts requested keys to symbols' do @@ -40,6 +42,7 @@ module VCAP::CloudController expect(message.requested?(:user_guids)).to be true expect(message.requested?(:space_guids)).to be true expect(message.requested?(:organization_guids)).to be true + expect(message.requested?(:include)).to be true end it 'defaults the order_by parameter if not provided' do @@ -59,6 +62,7 @@ module VCAP::CloudController 'user_guids' => 'my-user-guid', 'space_guids' => 'my-space-guid', 'organization_guids' => 'my-organization-guid', + 'include' => 'user', }) expect(message).to be_valid end @@ -135,6 +139,18 @@ module VCAP::CloudController expect(message.errors[:types]).to include('must be an array') end + it 'accepts an include param' do + message = RolesListMessage.from_params({ include: ['user'] }) + expect(message).to be_valid + expect(message.include).to eq(['user']) + end + + it 'does not accept an include param that is invalid' do + message = RolesListMessage.from_params({ include: ['garbage'] }) + expect(message).to be_invalid + expect(message.errors[:base]).to contain_exactly("Invalid included resource: 'garbage'") + end + it 'reject an invalid order_by field' do message = RolesListMessage.from_params({ 'order_by' => 'fail!',