diff --git a/app/graphql/types/ci/pipeline_type.rb b/app/graphql/types/ci/pipeline_type.rb index 18696293b97..de7d6570a3e 100644 --- a/app/graphql/types/ci/pipeline_type.rb +++ b/app/graphql/types/ci/pipeline_type.rb @@ -3,10 +3,12 @@ module Types module Ci class PipelineType < BaseObject - expose_permissions Types::PermissionTypes::Ci::Pipeline - graphql_name 'Pipeline' + authorize :read_pipeline + + expose_permissions Types::PermissionTypes::Ci::Pipeline + field :id, GraphQL::ID_TYPE, null: false field :iid, GraphQL::ID_TYPE, null: false diff --git a/app/graphql/types/issue_type.rb b/app/graphql/types/issue_type.rb index 5ad3ea52930..adb137dfee3 100644 --- a/app/graphql/types/issue_type.rb +++ b/app/graphql/types/issue_type.rb @@ -2,10 +2,12 @@ module Types class IssueType < BaseObject - expose_permissions Types::PermissionTypes::Issue - graphql_name 'Issue' + authorize :read_issue + + expose_permissions Types::PermissionTypes::Issue + present_using IssuePresenter field :iid, GraphQL::ID_TYPE, null: false @@ -15,16 +17,14 @@ module Types field :author, Types::UserType, null: false, - resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, obj.author_id).find }, - authorize: :read_user + resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, obj.author_id).find } field :assignees, Types::UserType.connection_type, null: true field :labels, Types::LabelType.connection_type, null: true field :milestone, Types::MilestoneType, null: true, - resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Milestone, obj.milestone_id).find }, - authorize: :read_milestone + resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Milestone, obj.milestone_id).find } field :due_date, Types::TimeType, null: true field :confidential, GraphQL::BOOLEAN_TYPE, null: false diff --git a/app/graphql/types/merge_request_type.rb b/app/graphql/types/merge_request_type.rb index 1ed27a14e33..120ffe0dfde 100644 --- a/app/graphql/types/merge_request_type.rb +++ b/app/graphql/types/merge_request_type.rb @@ -2,12 +2,14 @@ module Types class MergeRequestType < BaseObject + graphql_name 'MergeRequest' + + authorize :read_merge_request + expose_permissions Types::PermissionTypes::MergeRequest present_using MergeRequestPresenter - graphql_name 'MergeRequest' - field :id, GraphQL::ID_TYPE, null: false field :iid, GraphQL::ID_TYPE, null: false field :title, GraphQL::STRING_TYPE, null: false @@ -48,7 +50,7 @@ module Types field :downvotes, GraphQL::INT_TYPE, null: false field :subscribed, GraphQL::BOOLEAN_TYPE, method: :subscribed?, null: false - field :head_pipeline, Types::Ci::PipelineType, null: true, method: :actual_head_pipeline, authorize: :read_pipeline + field :head_pipeline, Types::Ci::PipelineType, null: true, method: :actual_head_pipeline field :pipelines, Types::Ci::PipelineType.connection_type, resolver: Resolvers::MergeRequestPipelinesResolver end diff --git a/app/graphql/types/milestone_type.rb b/app/graphql/types/milestone_type.rb index af31b572c9a..2772fbec86f 100644 --- a/app/graphql/types/milestone_type.rb +++ b/app/graphql/types/milestone_type.rb @@ -4,6 +4,8 @@ module Types class MilestoneType < BaseObject graphql_name 'Milestone' + authorize :read_milestone + field :description, GraphQL::STRING_TYPE, null: true field :title, GraphQL::STRING_TYPE, null: false field :state, GraphQL::STRING_TYPE, null: false diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index b96c2f3afb2..fbb4eddd13c 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -2,10 +2,12 @@ module Types class ProjectType < BaseObject - expose_permissions Types::PermissionTypes::Project - graphql_name 'Project' + authorize :read_project + + expose_permissions Types::PermissionTypes::Project + field :id, GraphQL::ID_TYPE, null: false field :full_path, GraphQL::ID_TYPE, null: false @@ -67,14 +69,12 @@ module Types field :merge_requests, Types::MergeRequestType.connection_type, null: true, - resolver: Resolvers::MergeRequestsResolver, - authorize: :read_merge_request + resolver: Resolvers::MergeRequestsResolver field :merge_request, Types::MergeRequestType, null: true, - resolver: Resolvers::MergeRequestsResolver.single, - authorize: :read_merge_request + resolver: Resolvers::MergeRequestsResolver.single field :issues, Types::IssueType.connection_type, @@ -88,7 +88,7 @@ module Types field :pipelines, Types::Ci::PipelineType.connection_type, - null: false, + null: true, resolver: Resolvers::ProjectPipelinesResolver end end diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb index 472fe5d6ec2..0f655ab9d03 100644 --- a/app/graphql/types/query_type.rb +++ b/app/graphql/types/query_type.rb @@ -7,8 +7,7 @@ module Types field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver, - description: "Find a project", - authorize: :read_project + description: "Find a project" field :metadata, Types::MetadataType, null: true, diff --git a/app/graphql/types/user_type.rb b/app/graphql/types/user_type.rb index a13e65207df..6b53554314b 100644 --- a/app/graphql/types/user_type.rb +++ b/app/graphql/types/user_type.rb @@ -4,6 +4,8 @@ module Types class UserType < BaseObject graphql_name 'User' + authorize :read_user + present_using UserPresenter field :name, GraphQL::STRING_TYPE, null: false diff --git a/changelogs/unreleased/54417-graphql-type-authorization.yml b/changelogs/unreleased/54417-graphql-type-authorization.yml new file mode 100644 index 00000000000..528b58a858a --- /dev/null +++ b/changelogs/unreleased/54417-graphql-type-authorization.yml @@ -0,0 +1,5 @@ +--- +title: GraphQL Types can be made to always authorize access to resources of that Type +merge_request: 25724 +author: +type: added diff --git a/config/initializers/graphql.rb b/config/initializers/graphql.rb index e653556231d..f1bc289f1f0 100644 --- a/config/initializers/graphql.rb +++ b/config/initializers/graphql.rb @@ -1,4 +1,7 @@ # frozen_string_literal: true +GraphQL::ObjectType.accepts_definitions(authorize: GraphQL::Define.assign_metadata_key(:authorize)) GraphQL::Field.accepts_definitions(authorize: GraphQL::Define.assign_metadata_key(:authorize)) + +GraphQL::Schema::Object.accepts_definition(:authorize) GraphQL::Schema::Field.accepts_definition(:authorize) diff --git a/lib/gitlab/graphql/authorize/authorize_field_service.rb b/lib/gitlab/graphql/authorize/authorize_field_service.rb new file mode 100644 index 00000000000..f3ca82ec697 --- /dev/null +++ b/lib/gitlab/graphql/authorize/authorize_field_service.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Authorize + class AuthorizeFieldService + def initialize(field) + @field = field + @old_resolve_proc = @field.resolve_proc + end + + def authorizations? + authorizations.present? + end + + def authorized_resolve + proc do |obj, args, ctx| + resolved_obj = @old_resolve_proc.call(obj, args, ctx) + checker = build_checker(ctx[:current_user]) + + if resolved_obj.respond_to?(:then) + resolved_obj.then(&checker) + else + checker.call(resolved_obj) + end + end + end + + private + + def authorizations + @authorizations ||= (type_authorizations + field_authorizations).uniq + end + + # Returns any authorize metadata from the return type of @field + def type_authorizations + type = @field.type + + # When the return type of @field is a collection, find the singular type + if type.get_field('edges') + type = node_type_for_relay_connection(type) + elsif type.list? + type = node_type_for_basic_connection(type) + end + + Array.wrap(type.metadata[:authorize]) + end + + # Returns any authorize metadata from @field + def field_authorizations + Array.wrap(@field.metadata[:authorize]) + end + + def build_checker(current_user) + lambda do |value| + # Load the elements if they were not loaded by BatchLoader yet + value = value.sync if value.respond_to?(:sync) + + check = lambda do |object| + authorizations.all? do |ability| + Ability.allowed?(current_user, ability, object) + end + end + + case value + when Array, ActiveRecord::Relation + value.select(&check) + else + value if check.call(value) + end + end + end + + # Returns the singular type for relay connections. + # This will be the type class of edges.node + def node_type_for_relay_connection(type) + type = type.get_field('edges').type.unwrap.get_field('node')&.type + + if type.nil? + raise Gitlab::Graphql::Errors::ConnectionDefinitionError, + 'Connection Type must conform to the Relay Cursor Connections Specification' + end + + type + end + + # Returns the singular type for basic connections, for example `[Types::ProjectType]` + def node_type_for_basic_connection(type) + type.unwrap + end + end + end + end +end diff --git a/lib/gitlab/graphql/authorize/instrumentation.rb b/lib/gitlab/graphql/authorize/instrumentation.rb index 593da8471dd..15ecc3b04f0 100644 --- a/lib/gitlab/graphql/authorize/instrumentation.rb +++ b/lib/gitlab/graphql/authorize/instrumentation.rb @@ -7,46 +7,12 @@ module Gitlab # Replace the resolver for the field with one that will only return the # resolved object if the permissions check is successful. def instrument(_type, field) - required_permissions = Array.wrap(field.metadata[:authorize]) - return field if required_permissions.empty? + service = AuthorizeFieldService.new(field) - old_resolver = field.resolve_proc - - new_resolver = -> (obj, args, ctx) do - resolved_obj = old_resolver.call(obj, args, ctx) - checker = build_checker(ctx[:current_user], required_permissions) - - if resolved_obj.respond_to?(:then) - resolved_obj.then(&checker) - else - checker.call(resolved_obj) - end - end - - field.redefine do - resolve(new_resolver) - end - end - - private - - def build_checker(current_user, abilities) - lambda do |value| - # Load the elements if they weren't loaded by BatchLoader yet - value = value.sync if value.respond_to?(:sync) - - check = lambda do |object| - abilities.all? do |ability| - Ability.allowed?(current_user, ability, object) - end - end - - case value - when Array - value.select(&check) - else - value if check.call(value) - end + if service.authorizations? + field.redefine { resolve(service.authorized_resolve) } + else + field end end end diff --git a/lib/gitlab/graphql/errors.rb b/lib/gitlab/graphql/errors.rb index fe74549e322..bcbba72e017 100644 --- a/lib/gitlab/graphql/errors.rb +++ b/lib/gitlab/graphql/errors.rb @@ -6,6 +6,7 @@ module Gitlab BaseError = Class.new(GraphQL::ExecutionError) ArgumentError = Class.new(BaseError) ResourceNotAvailable = Class.new(BaseError) + ConnectionDefinitionError = Class.new(BaseError) end end end diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb index a229d29afa6..f863c4444b8 100644 --- a/spec/graphql/features/authorization_spec.rb +++ b/spec/graphql/features/authorization_spec.rb @@ -5,61 +5,192 @@ require 'spec_helper' describe 'Gitlab::Graphql::Authorization' do set(:user) { create(:user) } + let(:permission_single) { :foo } + let(:permission_collection) { [:foo, :bar] } let(:test_object) { double(name: 'My name') } - let(:object_type) { object_type_class } - let(:query_type) { query_type_class(object_type, test_object) } - let(:schema) { schema_class(query_type) } + let(:query_string) { '{ object() { name } }' } + let(:result) { execute_query(query_type)['data'] } - let(:execute) do - schema.execute( - query_string, - context: { current_user: user }, - variables: {} - ) + subject { result['object'] } + + shared_examples 'authorization with a single permission' do + 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 be_nil + end end - let(:result) { execute['data'] } + shared_examples 'authorization with a collection of permissions' do + 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 be_nil + end + + it 'returns nil when user only has none of the permissions' do + expect(subject).to be_nil + end + end before do # By default, disallow all permissions. allow(Ability).to receive(:allowed?).and_return(false) end - describe 'authorizing with a single permission' do - let(:query_string) { '{ singlePermission() { name } }' } + describe 'Field authorizations' do + let(:type) { type_factory } - subject { result['singlePermission'] } + describe 'with a single permission' do + let(:query_type) do + query_factory do |query| + query.field :object, type, null: true, resolve: ->(obj, args, ctx) { test_object }, authorize: permission_single + end + end - it 'should return the protected field when user has permission' do - permit(:foo) - - expect(subject['name']).to eq(test_object.name) + include_examples 'authorization with a single permission' end - it 'should return nil when user is not authorized' do - expect(subject).to be_nil + describe 'with a collection of permissions' do + let(:query_type) do + permissions = permission_collection + query_factory do |qt| + qt.field :object, type, null: true, resolve: ->(obj, args, ctx) { test_object } do + authorize permissions + end + end + end + + include_examples 'authorization with a collection of permissions' end end - describe 'authorizing with an Array of permissions' do - let(:query_string) { '{ permissionCollection() { name } }' } - - subject { result['permissionCollection'] } - - it 'should return the protected field when user has all permissions' do - permit(:foo, :bar) - - expect(subject['name']).to eq(test_object.name) + describe 'Type authorizations' do + let(:query_type) do + query_factory do |query| + query.field :object, type, null: true, resolve: ->(obj, args, ctx) { test_object } + end end - it 'should return nil when user only has one of the permissions' do - permit(:foo) + describe 'with a single permission' do + let(:type) do + type_factory do |type| + type.authorize permission_single + end + end - expect(subject).to be_nil + include_examples 'authorization with a single permission' end - it 'should return nil when user only has none of the permissions' do - expect(subject).to be_nil + describe 'with a collection of permissions' do + let(:type) do + type_factory do |type| + type.authorize permission_collection + end + end + + include_examples 'authorization with a collection of permissions' + end + end + + describe 'type and field authorizations together' do + let(:permission_1) { permission_collection.first } + let(:permission_2) { permission_collection.last } + + let(:type) do + type_factory do |type| + type.authorize permission_1 + end + end + + let(:query_type) do + query_factory do |query| + query.field :object, type, null: true, resolve: ->(obj, args, ctx) { test_object }, authorize: permission_2 + end + end + + include_examples 'authorization with a collection of permissions' + end + + describe 'type authorizations when applied to a relay connection' do + let(:query_string) { '{ object() { edges { node { name } } } }' } + + let(:type) do + type_factory do |type| + type.authorize permission_single + end + end + + let(:query_type) do + query_factory do |query| + query.field :object, type.connection_type, null: true, resolve: ->(obj, args, ctx) { [test_object] } + end + end + + subject { result.dig('object', 'edges') } + + it 'returns the protected field when user has permission' do + permit(permission_single) + + expect(subject).not_to be_empty + expect(subject.first['node']).to eq('name' => test_object.name) + end + + it 'returns nil when user is not authorized' do + expect(subject).to be_empty + end + end + + describe 'type authorizations when applied to a basic connection' do + let(:type) do + type_factory do |type| + type.authorize permission_single + end + end + + let(:query_type) do + query_factory do |query| + query.field :object, [type], null: true, resolve: ->(obj, args, ctx) { [test_object] } + end + end + + subject { result['object'].first } + + include_examples 'authorization with a single permission' + end + + describe 'when connections do not follow the correct specification' do + let(:query_string) { '{ object() { edges { node { name }} } }' } + + let(:type) do + bad_node = type_factory do |type| + type.graphql_name 'BadNode' + type.field :bad_node, GraphQL::STRING_TYPE, null: true + end + + type_factory do |type| + type.field :edges, [bad_node], null: true + end + end + + let(:query_type) do + query_factory do |query| + query.field :object, type, null: true + end + end + + it 'throws an error' do + expect { result }.to raise_error(Gitlab::Graphql::Errors::ConnectionDefinitionError) end end @@ -71,36 +202,34 @@ describe 'Gitlab::Graphql::Authorization' do end end - def object_type_class + def type_factory Class.new(Types::BaseObject) do - graphql_name 'TestObject' + graphql_name 'TestType' field :name, GraphQL::STRING_TYPE, null: true + + yield(self) if block_given? end end - def query_type_class(type, object) + def query_factory Class.new(Types::BaseObject) do graphql_name 'TestQuery' - field :single_permission, type, - null: true, - authorize: :foo, - resolve: ->(obj, args, ctx) { object } - - field :permission_collection, type, - null: true, - resolve: ->(obj, args, ctx) { object } do - authorize [:foo, :bar] - end + yield(self) if block_given? end end - def schema_class(query) - Class.new(GraphQL::Schema) do + def execute_query(query_type) + schema = Class.new(GraphQL::Schema) do use Gitlab::Graphql::Authorize - - query(query) + query(query_type) end + + schema.execute( + query_string, + context: { current_user: user }, + variables: {} + ) end end diff --git a/spec/graphql/types/issue_type_spec.rb b/spec/graphql/types/issue_type_spec.rb index 63a07647a60..dc37b15001f 100644 --- a/spec/graphql/types/issue_type_spec.rb +++ b/spec/graphql/types/issue_type_spec.rb @@ -4,4 +4,6 @@ describe GitlabSchema.types['Issue'] do it { expect(described_class).to expose_permissions_using(Types::PermissionTypes::Issue) } it { expect(described_class.graphql_name).to eq('Issue') } + + it { expect(described_class).to require_graphql_authorizations(:read_issue) } end diff --git a/spec/graphql/types/merge_request_type_spec.rb b/spec/graphql/types/merge_request_type_spec.rb index c369953e3ea..89c12879074 100644 --- a/spec/graphql/types/merge_request_type_spec.rb +++ b/spec/graphql/types/merge_request_type_spec.rb @@ -3,14 +3,9 @@ require 'spec_helper' describe GitlabSchema.types['MergeRequest'] do it { expect(described_class).to expose_permissions_using(Types::PermissionTypes::MergeRequest) } - describe 'head pipeline' do - it 'has a head pipeline field' do - expect(described_class).to have_graphql_field(:head_pipeline) - end + it { expect(described_class).to require_graphql_authorizations(:read_merge_request) } - it 'authorizes the field' do - expect(described_class.fields['headPipeline']) - .to require_graphql_authorizations(:read_pipeline) - end + describe 'nested head pipeline' do + it { expect(described_class).to have_graphql_field(:head_pipeline) } end end diff --git a/spec/graphql/types/milestone_type_spec.rb b/spec/graphql/types/milestone_type_spec.rb new file mode 100644 index 00000000000..f7ee79eae9f --- /dev/null +++ b/spec/graphql/types/milestone_type_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe GitlabSchema.types['Milestone'] do + it { expect(described_class.graphql_name).to eq('Milestone') } + + it { expect(described_class).to require_graphql_authorizations(:read_milestone) } +end diff --git a/spec/graphql/types/project_type_spec.rb b/spec/graphql/types/project_type_spec.rb index e8f1c84f8d6..e0ad09bdf22 100644 --- a/spec/graphql/types/project_type_spec.rb +++ b/spec/graphql/types/project_type_spec.rb @@ -5,19 +5,11 @@ describe GitlabSchema.types['Project'] do it { expect(described_class.graphql_name).to eq('Project') } + it { expect(described_class).to require_graphql_authorizations(:read_project) } + describe 'nested merge request' do it { expect(described_class).to have_graphql_field(:merge_requests) } it { expect(described_class).to have_graphql_field(:merge_request) } - - it 'authorizes the merge request' do - expect(described_class.fields['mergeRequest']) - .to require_graphql_authorizations(:read_merge_request) - end - - it 'authorizes the merge requests' do - expect(described_class.fields['mergeRequests']) - .to require_graphql_authorizations(:read_merge_request) - end end describe 'nested issues' do diff --git a/spec/graphql/types/query_type_spec.rb b/spec/graphql/types/query_type_spec.rb index 07c61ea7647..69e3ea8a4a9 100644 --- a/spec/graphql/types/query_type_spec.rb +++ b/spec/graphql/types/query_type_spec.rb @@ -15,10 +15,6 @@ describe GitlabSchema.types['Query'] do is_expected.to have_graphql_type(Types::ProjectType) is_expected.to have_graphql_resolver(Resolvers::ProjectResolver) end - - it 'authorizes with read_project' do - is_expected.to require_graphql_authorizations(:read_project) - end end describe 'metadata field' do diff --git a/spec/graphql/types/user_type_spec.rb b/spec/graphql/types/user_type_spec.rb new file mode 100644 index 00000000000..8134cc13eb4 --- /dev/null +++ b/spec/graphql/types/user_type_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe GitlabSchema.types['User'] do + it { expect(described_class.graphql_name).to eq('User') } + + it { expect(described_class).to require_graphql_authorizations(:read_user) } +end diff --git a/spec/lib/gitlab/graphql/authorize/instrumentation_spec.rb b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb similarity index 73% rename from spec/lib/gitlab/graphql/authorize/instrumentation_spec.rb rename to spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb index cf3a8bcc8b4..ce320a2bdb0 100644 --- a/spec/lib/gitlab/graphql/authorize/instrumentation_spec.rb +++ b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb @@ -2,13 +2,17 @@ require 'spec_helper' -describe Gitlab::Graphql::Authorize::Instrumentation do +# Also see spec/graphql/features/authorization_spec.rb for +# integration tests of AuthorizeFieldService +describe Gitlab::Graphql::Authorize::AuthorizeFieldService do describe '#build_checker' do let(:current_user) { double(:current_user) } let(:abilities) { [double(:first_ability), double(:last_ability)] } let(:checker) do - described_class.new.__send__(:build_checker, current_user, abilities) + service = described_class.new(double(resolve_proc: proc {})) + allow(service).to receive(:authorizations).and_return(abilities) + service.__send__(:build_checker, current_user) end it 'returns a checker which checks for a single object' do @@ -56,12 +60,14 @@ describe Gitlab::Graphql::Authorize::Instrumentation do .to contain_exactly(allowed) end end + end - def spy_ability_check_for(ability, object, passed: true) - expect(Ability) - .to receive(:allowed?) - .with(current_user, ability, object) - .and_return(passed) - end + private + + def spy_ability_check_for(ability, object, passed: true) + expect(Ability) + .to receive(:allowed?) + .with(current_user, ability, object) + .and_return(passed) end end