diff options
author | Brett Walker <bwalker@gitlab.com> | 2019-03-21 02:39:18 +0300 |
---|---|---|
committer | Brett Walker <bwalker@gitlab.com> | 2019-04-04 17:03:21 +0300 |
commit | 6643b92b8807e2d59f36d676303b89ea01824f22 (patch) | |
tree | d03308ddb6bd51362325cd3384deaa4cfa08a9ef | |
parent | 702f18261a2ac0b45e2b002055950816ad34e92c (diff) |
Use parent object when authorizing scalar types
3 files changed, 138 insertions, 41 deletions
diff --git a/lib/gitlab/graphql/authorize/authorize_field_service.rb b/lib/gitlab/graphql/authorize/authorize_field_service.rb index f3ca82ec697..8deff79fc84 100644 --- a/lib/gitlab/graphql/authorize/authorize_field_service.rb +++ b/lib/gitlab/graphql/authorize/authorize_field_service.rb @@ -14,9 +14,10 @@ module Gitlab end def authorized_resolve - proc do |obj, args, ctx| - resolved_obj = @old_resolve_proc.call(obj, args, ctx) - checker = build_checker(ctx[:current_user]) + proc do |parent_typed_object, args, ctx| + resolved_obj = @old_resolve_proc.call(parent_typed_object, args, ctx) + authorizing_obj = authorize_against(parent_typed_object) + checker = build_checker(ctx[:current_user], authorizing_obj) if resolved_obj.respond_to?(:then) resolved_obj.then(&checker) @@ -51,22 +52,28 @@ module Gitlab Array.wrap(@field.metadata[:authorize]) end - def build_checker(current_user) - lambda do |value| + # If it's a built-in/scalar type, authorize using its parent object. + # nil means authorize using the resolved object + def authorize_against(parent_typed_object) + parent_typed_object.object if built_in_type? && parent_typed_object.respond_to?(:object) + end + + def build_checker(current_user, authorizing_obj) + lambda do |resolved_obj| # Load the elements if they were not loaded by BatchLoader yet - value = value.sync if value.respond_to?(:sync) + resolved_obj = resolved_obj.sync if resolved_obj.respond_to?(:sync) check = lambda do |object| authorizations.all? do |ability| - Ability.allowed?(current_user, ability, object) + Ability.allowed?(current_user, ability, authorizing_obj || object) end end - case value + case resolved_obj when Array, ActiveRecord::Relation - value.select(&check) + resolved_obj.select(&check) else - value if check.call(value) + resolved_obj if check.call(resolved_obj) end end end @@ -88,6 +95,10 @@ module Gitlab def node_type_for_basic_connection(type) type.unwrap end + + def built_in_type? + GraphQL::Schema::BUILT_IN_TYPES.has_value?(node_type_for_basic_connection(@field.type)) + end end end end diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb index f863c4444b8..00e31568a9e 100644 --- a/spec/graphql/features/authorization_spec.rb +++ b/spec/graphql/features/authorization_spec.rb @@ -75,6 +75,59 @@ describe 'Gitlab::Graphql::Authorization' do end end + describe 'Field authorizations when field is a built in type' do + let(:query_type) do + query_factory do |query| + query.field :object, type, null: true, resolve: ->(obj, args, ctx) { test_object } + end + end + + describe 'with a single permission' do + let(:type) do + type_factory do |type| + type.field :name, GraphQL::STRING_TYPE, null: true, authorize: permission_single + end + end + + it 'returns the protected field when user has permission' do + permit(permission_single) + + expect(subject).to eq('name' => test_object.name) + end + + it 'returns nil when user is not authorized' do + expect(subject).to eq('name' => nil) + end + end + + describe 'with a collection of permissions' do + let(:type) do + permissions = permission_collection + type_factory do |type| + type.field :name, GraphQL::STRING_TYPE, null: true do + authorize permissions + end + end + end + + it 'returns the protected field when user has all permissions' do + permit(*permission_collection) + + expect(subject).to eq('name' => test_object.name) + end + + it 'returns nil when user only has one of the permissions' do + permit(permission_collection.first) + + expect(subject).to eq('name' => nil) + end + + it 'returns nil when user only has none of the permissions' do + expect(subject).to eq('name' => nil) + end + end + end + describe 'Type authorizations' do let(:query_type) do query_factory do |query| diff --git a/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb index ce320a2bdb0..6114aca0616 100644 --- a/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb +++ b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb @@ -9,55 +9,88 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do let(:current_user) { double(:current_user) } let(:abilities) { [double(:first_ability), double(:last_ability)] } - let(:checker) do - service = described_class.new(double(resolve_proc: proc {})) - allow(service).to receive(:authorizations).and_return(abilities) - service.__send__(:build_checker, current_user) - end + context 'when authorizing against the object' do + let(:checker) do + service = described_class.new(double(resolve_proc: proc {})) + allow(service).to receive(:authorizations).and_return(abilities) + service.__send__(:build_checker, current_user, nil) + end - it 'returns a checker which checks for a single object' do - object = double(:object) + it 'returns a checker which checks for a single object' do + object = double(:object) - abilities.each do |ability| - spy_ability_check_for(ability, object, passed: true) - end + abilities.each do |ability| + spy_ability_check_for(ability, object, passed: true) + end - expect(checker.call(object)).to eq(object) - end + expect(checker.call(object)).to eq(object) + end - it 'returns a checker which checks for all objects' do - objects = [double(:first), double(:last)] + it 'returns a checker which checks for all objects' do + objects = [double(:first), double(:last)] - abilities.each do |ability| - objects.each do |object| - spy_ability_check_for(ability, object, passed: true) + abilities.each do |ability| + objects.each do |object| + spy_ability_check_for(ability, object, passed: true) + end end + + expect(checker.call(objects)).to eq(objects) end - expect(checker.call(objects)).to eq(objects) - end + context 'when some objects would not pass the check' do + it 'returns nil when it is single object' do + disallowed = double(:object) + + spy_ability_check_for(abilities.first, disallowed, passed: false) - context 'when some objects would not pass the check' do - it 'returns nil when it is single object' do - disallowed = double(:object) + expect(checker.call(disallowed)).to be_nil + end + + it 'returns only objects which passed when there are more than one' do + allowed = double(:allowed) + disallowed = double(:disallowed) - spy_ability_check_for(abilities.first, disallowed, passed: false) + spy_ability_check_for(abilities.first, disallowed, passed: false) - expect(checker.call(disallowed)).to be_nil + abilities.each do |ability| + spy_ability_check_for(ability, allowed, passed: true) + end + + expect(checker.call([disallowed, allowed])).to contain_exactly(allowed) + end end + end + + context 'when authorizing against another object' do + let(:authorizing_obj) { double(:object) } - it 'returns only objects which passed when there are more than one' do - allowed = double(:allowed) - disallowed = double(:disallowed) + let(:checker) do + service = described_class.new(double(resolve_proc: proc {})) + allow(service).to receive(:authorizations).and_return(abilities) + service.__send__(:build_checker, current_user, authorizing_obj) + end + + it 'returns a checker which checks for a single object' do + object = double(:object) + + abilities.each do |ability| + spy_ability_check_for(ability, authorizing_obj, passed: true) + end + + expect(checker.call(object)).to eq(object) + end - spy_ability_check_for(abilities.first, disallowed, passed: false) + it 'returns a checker which checks for all objects' do + objects = [double(:first), double(:last)] abilities.each do |ability| - spy_ability_check_for(ability, allowed, passed: true) + objects.each do |object| + spy_ability_check_for(ability, authorizing_obj, passed: true) + end end - expect(checker.call([disallowed, allowed])) - .to contain_exactly(allowed) + expect(checker.call(objects)).to eq(objects) end end end |