From ccb4edbca1aa7e94a76a5a8d361af02fd093e1b9 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 18 Feb 2019 14:19:49 +1300 Subject: [PATCH] Improve GraphQL Authorization DSL Previously GraphQL field authorization happened like this: class ProjectType field :my_field, MyFieldType do authorize :permission end end This change allowed us to authorize like this instead: class ProjectType field :my_field, MyFieldType, authorize: :permission end A new initializer registers the `authorize` metadata keyword on GraphQL Schema Objects and Fields, and we can collect this data within the context of Instrumentation like this: field.metadata[:authorize] The previous functionality of authorize is still being used for mutations, as the #authorize method here is called at during the code that executes during the mutation, rather than when a field resolves. https://gitlab.com/gitlab-org/gitlab-ce/issues/57828 --- app/graphql/types/issue_type.rb | 10 +- app/graphql/types/merge_request_type.rb | 4 +- app/graphql/types/project_type.rb | 10 +- app/graphql/types/query_type.rb | 5 +- config/initializers/graphql.rb | 4 + doc/development/api_graphql_styleguide.md | 24 ++-- lib/gitlab/graphql/authorize.rb | 15 --- .../graphql/authorize/authorize_resource.rb | 17 ++- .../graphql/authorize/instrumentation.rb | 10 +- spec/graphql/features/authorization_spec.rb | 106 ++++++++++++++++++ .../authorize/authorize_resource_spec.rb | 18 +++ spec/lib/gitlab/graphql/authorize_spec.rb | 20 ---- spec/support/matchers/graphql_matchers.rb | 4 +- 13 files changed, 175 insertions(+), 72 deletions(-) create mode 100644 config/initializers/graphql.rb create mode 100644 spec/graphql/features/authorization_spec.rb delete mode 100644 spec/lib/gitlab/graphql/authorize_spec.rb diff --git a/app/graphql/types/issue_type.rb b/app/graphql/types/issue_type.rb index 87f6b1f8278..5ad3ea52930 100644 --- a/app/graphql/types/issue_type.rb +++ b/app/graphql/types/issue_type.rb @@ -15,18 +15,16 @@ module Types field :author, Types::UserType, null: false, - resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, obj.author_id).find } do - authorize :read_user - end + resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, obj.author_id).find }, + authorize: :read_user 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 } do - authorize :read_milestone - end + resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Milestone, obj.milestone_id).find }, + authorize: :read_milestone 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 7827b6e3717..1ed27a14e33 100644 --- a/app/graphql/types/merge_request_type.rb +++ b/app/graphql/types/merge_request_type.rb @@ -48,9 +48,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 do - authorize :read_pipeline - end + field :head_pipeline, Types::Ci::PipelineType, null: true, method: :actual_head_pipeline, authorize: :read_pipeline field :pipelines, Types::Ci::PipelineType.connection_type, resolver: Resolvers::MergeRequestPipelinesResolver end diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index d25c8c8bd90..3ef0cc5020c 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -69,16 +69,14 @@ module Types field :merge_requests, Types::MergeRequestType.connection_type, null: true, - resolver: Resolvers::MergeRequestsResolver do - authorize :read_merge_request - end + resolver: Resolvers::MergeRequestsResolver, + authorize: :read_merge_request field :merge_request, Types::MergeRequestType, null: true, - resolver: Resolvers::MergeRequestsResolver.single do - authorize :read_merge_request - end + resolver: Resolvers::MergeRequestsResolver.single, + authorize: :read_merge_request field :issues, Types::IssueType.connection_type, diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb index 7c41716b82a..954bcc0a5a3 100644 --- a/app/graphql/types/query_type.rb +++ b/app/graphql/types/query_type.rb @@ -7,9 +7,8 @@ module Types field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver, - description: "Find a project" do - authorize :read_project - end + description: "Find a project", + authorize: :read_project field :echo, GraphQL::STRING_TYPE, null: false, function: Functions::Echo.new end diff --git a/config/initializers/graphql.rb b/config/initializers/graphql.rb new file mode 100644 index 00000000000..1ed93019329 --- /dev/null +++ b/config/initializers/graphql.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +GraphQL::Field.accepts_definitions(authorize: GraphQL::Define.assign_metadata_key(:authorize)) +Types::BaseField.accepts_definition(:authorize) diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md index 95722c027ba..501092ff2aa 100644 --- a/doc/development/api_graphql_styleguide.md +++ b/doc/development/api_graphql_styleguide.md @@ -12,24 +12,34 @@ add a `HTTP_PRIVATE_TOKEN` header. ### Authorization Fields can be authorized using the same abilities used in the Rails -app. This can be done using the `authorize` helper: +app. This can be done by supplying the `authorize` option: ```ruby module Types class QueryType < BaseObject graphql_name 'Query' - field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver do - authorize :read_project - end + field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver, authorize: :read_project end +end +``` + +Fields can be authorized against multiple abilities, in which case all +ability checks must pass. This requires explicitly passing a block to `field`: + +```ruby +field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver do + authorize [:read_project, :another_ability] +end ``` The object found by the resolve call is used for authorization. -This works for authorizing a single record, for authorizing -collections, we should only load what the currently authenticated user -is allowed to view. Preferably we use our existing finders for that. +TIP: **Tip:** +When authorizing collections, try to load only what the currently +authenticated user is allowed to view with our existing finders first. +This minimizes database queries and unnecessary authorization checks of +the loaded records. ## Types diff --git a/lib/gitlab/graphql/authorize.rb b/lib/gitlab/graphql/authorize.rb index 5e48bf9043d..f62813db82c 100644 --- a/lib/gitlab/graphql/authorize.rb +++ b/lib/gitlab/graphql/authorize.rb @@ -10,21 +10,6 @@ module Gitlab def self.use(schema_definition) schema_definition.instrument(:field, Instrumentation.new) end - - def required_permissions - # If the `#authorize` call is used on multiple classes, we add the - # permissions specified on a subclass, to the ones that were specified - # on it's superclass. - @required_permissions ||= if self.respond_to?(:superclass) && superclass.respond_to?(:required_permissions) - superclass.required_permissions.dup - else - [] - end - end - - def authorize(*permissions) - required_permissions.concat(permissions) - end end end end diff --git a/lib/gitlab/graphql/authorize/authorize_resource.rb b/lib/gitlab/graphql/authorize/authorize_resource.rb index a56c4f6368d..b367a97105c 100644 --- a/lib/gitlab/graphql/authorize/authorize_resource.rb +++ b/lib/gitlab/graphql/authorize/authorize_resource.rb @@ -6,8 +6,21 @@ module Gitlab module AuthorizeResource extend ActiveSupport::Concern - included do - extend Gitlab::Graphql::Authorize + class_methods do + def required_permissions + # If the `#authorize` call is used on multiple classes, we add the + # permissions specified on a subclass, to the ones that were specified + # on it's superclass. + @required_permissions ||= if self.respond_to?(:superclass) && superclass.respond_to?(:required_permissions) + superclass.required_permissions.dup + else + [] + end + end + + def authorize(*permissions) + required_permissions.concat(permissions) + end end def find_object(*args) diff --git a/lib/gitlab/graphql/authorize/instrumentation.rb b/lib/gitlab/graphql/authorize/instrumentation.rb index 2a3d790d67b..593da8471dd 100644 --- a/lib/gitlab/graphql/authorize/instrumentation.rb +++ b/lib/gitlab/graphql/authorize/instrumentation.rb @@ -6,19 +6,15 @@ module Gitlab class Instrumentation # Replace the resolver for the field with one that will only return the # resolved object if the permissions check is successful. - # - # Collections are not supported. Apply permissions checks for those at the - # database level instead, to avoid loading superfluous data from the DB def instrument(_type, field) - field_definition = field.metadata[:type_class] - return field unless field_definition.respond_to?(:required_permissions) - return field if field_definition.required_permissions.empty? + required_permissions = Array.wrap(field.metadata[:authorize]) + return field if required_permissions.empty? 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], field_definition.required_permissions) + checker = build_checker(ctx[:current_user], required_permissions) if resolved_obj.respond_to?(:then) resolved_obj.then(&checker) diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb new file mode 100644 index 00000000000..a229d29afa6 --- /dev/null +++ b/spec/graphql/features/authorization_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Gitlab::Graphql::Authorization' do + set(:user) { create(:user) } + + 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(:execute) do + schema.execute( + query_string, + context: { current_user: user }, + variables: {} + ) + end + + let(:result) { execute['data'] } + + 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 } }' } + + subject { result['singlePermission'] } + + it 'should return the protected field when user has permission' do + permit(:foo) + + expect(subject['name']).to eq(test_object.name) + end + + it 'should return nil when user is not authorized' do + expect(subject).to be_nil + 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) + end + + it 'should return nil when user only has one of the permissions' do + permit(:foo) + + expect(subject).to be_nil + end + + it 'should return nil when user only has none of the permissions' do + expect(subject).to be_nil + end + end + + private + + def permit(*permissions) + permissions.each do |permission| + allow(Ability).to receive(:allowed?).with(user, permission, test_object).and_return(true) + end + end + + def object_type_class + Class.new(Types::BaseObject) do + graphql_name 'TestObject' + + field :name, GraphQL::STRING_TYPE, null: true + end + end + + def query_type_class(type, object) + 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 + end + end + + def schema_class(query) + Class.new(GraphQL::Schema) do + use Gitlab::Graphql::Authorize + + query(query) + end + end +end diff --git a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb index 95bf7685ade..13cf52fd795 100644 --- a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb +++ b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb @@ -100,4 +100,22 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do expect { fake_class.new.find_object }.to raise_error(/Implement #find_object in #{fake_class.name}/) end end + + describe '#authorize' do + it 'adds permissions from subclasses to those of superclasses when used on classes' do + base_class = Class.new do + include Gitlab::Graphql::Authorize::AuthorizeResource + + authorize :base_authorization + end + + sub_class = Class.new(base_class) do + authorize :sub_authorization + end + + expect(base_class.required_permissions).to contain_exactly(:base_authorization) + expect(sub_class.required_permissions) + .to contain_exactly(:base_authorization, :sub_authorization) + end + end end diff --git a/spec/lib/gitlab/graphql/authorize_spec.rb b/spec/lib/gitlab/graphql/authorize_spec.rb deleted file mode 100644 index 9c17a3b0e4b..00000000000 --- a/spec/lib/gitlab/graphql/authorize_spec.rb +++ /dev/null @@ -1,20 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Graphql::Authorize do - describe '#authorize' do - it 'adds permissions from subclasses to those of superclasses when used on classes' do - base_class = Class.new do - extend Gitlab::Graphql::Authorize - - authorize :base_authorization - end - sub_class = Class.new(base_class) do - authorize :sub_authorization - end - - expect(base_class.required_permissions).to contain_exactly(:base_authorization) - expect(sub_class.required_permissions) - .to contain_exactly(:base_authorization, :sub_authorization) - end - end -end diff --git a/spec/support/matchers/graphql_matchers.rb b/spec/support/matchers/graphql_matchers.rb index 7be84838e00..7894484f590 100644 --- a/spec/support/matchers/graphql_matchers.rb +++ b/spec/support/matchers/graphql_matchers.rb @@ -1,8 +1,6 @@ RSpec::Matchers.define :require_graphql_authorizations do |*expected| match do |field| - field_definition = field.metadata[:type_class] - expect(field_definition).to respond_to(:required_permissions) - expect(field_definition.required_permissions).to contain_exactly(*expected) + expect(field.metadata[:authorize]).to eq(*expected) end end