Make GraphQL Types authorizable54417-authorize-types

Added a new initializer to allow the `authorize` keyword to be used
when defining a field, or a Type.

Modified Gitlab::GraphQL::Authorize and associated classes to check the
authorize data from the authorize metadata, and support checks on Types,
as well as Scalar fields.

Moved existing checks for Types from their field definitions and into
the Type definition.

Issue:
https://gitlab.com/gitlab-org/gitlab-ce/issues/54417

21 files changed, 179 insertions, 108 deletions

diff --git a/app/graphql/types/ci/pipeline_type.rb b/app/graphql/types/ci/pipeline_type.rb
index 2bbffad4563..ec2b62b8121 100644
--- a/app/graphql/types/ci/pipeline_type.rb
+++ b/app/graphql/types/ci/pipeline_type.rb
@@ -5,6 +5,8 @@ module Types
     class PipelineType < BaseObject
       expose_permissions Types::PermissionTypes::Ci::Pipeline
 
+      authorize :read_pipeline
+
       graphql_name 'Pipeline'
 
       field :id, GraphQL::ID_TYPE, null: false
diff --git a/app/graphql/types/issue_type.rb b/app/graphql/types/issue_type.rb
index a8f2f7914a8..c6338c03dc7 100644
--- a/app/graphql/types/issue_type.rb
+++ b/app/graphql/types/issue_type.rb
@@ -15,18 +15,14 @@ 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 }
 
     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 }
 
     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 47b915b451e..7c399eff533 100644
--- a/app/graphql/types/merge_request_type.rb
+++ b/app/graphql/types/merge_request_type.rb
@@ -4,10 +4,12 @@ module Types
   class MergeRequestType < BaseObject
     expose_permissions Types::PermissionTypes::MergeRequest
 
-    present_using MergeRequestPresenter
-
     graphql_name 'MergeRequest'
 
+    authorize :read_merge_request
+
+    present_using MergeRequestPresenter
+
     field :id, GraphQL::ID_TYPE, null: false
     field :iid, GraphQL::ID_TYPE, null: false
     field :title, GraphQL::STRING_TYPE, null: false
@@ -49,9 +51,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
     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 050706f97be..13a0ea87caf 100644
--- a/app/graphql/types/project_type.rb
+++ b/app/graphql/types/project_type.rb
@@ -6,6 +6,8 @@ module Types
 
     graphql_name 'Project'
 
+    authorize :read_project
+
     field :id, GraphQL::ID_TYPE, null: false
 
     field :full_path, GraphQL::ID_TYPE, null: false
@@ -69,9 +71,7 @@ module Types
     field :merge_request,
           Types::MergeRequestType,
           null: true,
-          resolver: Resolvers::MergeRequestResolver do
-      authorize :read_merge_request
-    end
+          resolver: Resolvers::MergeRequestResolver
 
     field :issues,
           Types::IssueType.connection_type,
diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb
index 7c41716b82a..e342ff22705 100644
--- a/app/graphql/types/query_type.rb
+++ b/app/graphql/types/query_type.rb
@@ -7,9 +7,7 @@ module Types
     field :project, Types::ProjectType,
           null: true,
           resolver: Resolvers::ProjectResolver,
-          description: "Find a project" do
-      authorize :read_project
-    end
+          description: "Find a project"
 
     field :echo, GraphQL::STRING_TYPE, null: false, function: Functions::Echo.new
   end
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/config/initializers/graphql.rb b/config/initializers/graphql.rb
new file mode 100644
index 00000000000..28f2be1dcbe
--- /dev/null
+++ b/config/initializers/graphql.rb
@@ -0,0 +1,5 @@
+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.rb b/lib/gitlab/graphql/authorize.rb
index 5e48bf9043d..3c110429fc2 100644
--- a/lib/gitlab/graphql/authorize.rb
+++ b/lib/gitlab/graphql/authorize.rb
@@ -5,26 +5,9 @@ module Gitlab
     # Allow fields to declare permissions their objects must have. The field
     # will be set to nil unless all required permissions are present.
     module Authorize
-      extend ActiveSupport::Concern
-
       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_field_service.rb b/lib/gitlab/graphql/authorize/authorize_field_service.rb
new file mode 100644
index 00000000000..0a9bc9ef05a
--- /dev/null
+++ b/lib/gitlab/graphql/authorize/authorize_field_service.rb
@@ -0,0 +1,97 @@
+# 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 authorization_checks?
+          authorization_checks.present?
+        end
+
+        def authorized_resolve
+          proc do |obj, args, ctx|
+            resolved_object = @old_resolve_proc.call(obj, args, ctx)
+
+            authorized = authorization_checks.all? do |authorization_check|
+              # Authorization checks can be a Symbol (i.e.: :read_project)
+              # or a Proc.
+              #
+              # If the check is a Symbol, turn this into an Ability check.
+              if authorization_check.is_a?(Symbol)
+                ability_subject = subject_for_ability(resolved_object, obj)
+                Ability.allowed?(ctx[:current_user], authorization_check, ability_subject)
+              elsif authorization_check.is_a?(Proc)
+                authorization_check.call(obj, args, ctx).present?
+              else
+                raise NotImplementedError, "Cannot handle authorization for #{authorization_check.inspect}"
+              end
+            end
+
+            if authorized
+              resolved_object
+            end
+          end
+        end
+
+        private
+
+        def authorization_checks
+          @authorization_checks ||= type_authorization_checks + field_authorization_checks
+        end
+
+        # Returns any authorize metadata from the return type of @field
+        def type_authorization_checks
+          type = @field.metadata[:type_class]&.type
+          if type.respond_to?(:authorize) && type.authorize
+            type.authorize.flatten
+          else
+            []
+          end
+        end
+
+        # Returns any authorize metadata from the field
+        def field_authorization_checks
+          Array.wrap(@field.metadata[:authorize])
+        end
+
+        # Returns the subject that will be used in the Ability check:
+        # Ability.allow?(user, :permission, subject)
+        #
+        # For most occasions, whether or not the field's type is something
+        # subclassed from Types::BaseObject is what should determine what
+        # the subject is.
+        #
+        # For example, the subject here should be the MergeRequest:
+        #
+        # class ProjectType
+        #   field :mergeRequest, Types::MergeRequest, authorize: :read_merge_request
+        # end
+        #
+        # But for this Scalar-type field, the subject should be the Project:
+        #
+        # class ProjectType
+        #   field :id, GraphQL::ID_TYPE, authorize: :read_project
+        # end
+        #
+        # If this is ever not correct, a Proc can be used with authorize to
+        # do a custom authorization
+        def subject_for_ability(resolved_object, obj)
+          # Load the element if it wasn't loaded by BatchLoader yet
+          resolved_object = resolved_object.sync if resolved_object.respond_to?(:sync)
+          return_type = @field.metadata[:type_class].type
+
+          if return_type.is_a?(Class) && return_type < Types::BaseObject
+            resolved_object
+          else
+            obj.object
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/graphql/authorize/authorize_resource.rb b/lib/gitlab/graphql/authorize/authorize_resource.rb
index a56c4f6368d..72932580ffd 100644
--- a/lib/gitlab/graphql/authorize/authorize_resource.rb
+++ b/lib/gitlab/graphql/authorize/authorize_resource.rb
@@ -42,6 +42,23 @@ module Gitlab
             Ability.allowed?(current_user, ability, object, scope: :user)
           end
         end
+
+        module ClassMethods
+          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
   end
diff --git a/lib/gitlab/graphql/authorize/instrumentation.rb b/lib/gitlab/graphql/authorize/instrumentation.rb
index d638d2b43ee..5f825292399 100644
--- a/lib/gitlab/graphql/authorize/instrumentation.rb
+++ b/lib/gitlab/graphql/authorize/instrumentation.rb
@@ -10,35 +10,12 @@ module Gitlab
         # 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?
+          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], field_definition.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)
-          proc do |obj|
-            # Load the elements if they weren't loaded by BatchLoader yet
-            obj = obj.sync if obj.respond_to?(:sync)
-            obj if abilities.all? { |ability| Ability.allowed?(current_user, ability, obj) }
+          if service.authorization_checks?
+            field.redefine { resolve(service.authorized_resolve) }
+          else
+            field
           end
         end
       end
diff --git a/spec/graphql/types/merge_request_type_spec.rb b/spec/graphql/types/merge_request_type_spec.rb
index c369953e3ea..75db99a7d87 100644
--- a/spec/graphql/types/merge_request_type_spec.rb
+++ b/spec/graphql/types/merge_request_type_spec.rb
@@ -3,14 +3,7 @@ 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 { is_expected.to require_graphql_authorizations(:read_merge_request) }
 
-    it 'authorizes the field' do
-      expect(described_class.fields['headPipeline'])
-        .to require_graphql_authorizations(:read_pipeline)
-    end
-  end
+  it { is_expected.to have_graphql_field(:head_pipeline) }
 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..1523a08c8ee
--- /dev/null
+++ b/spec/graphql/types/milestone_type_spec.rb
@@ -0,0 +1,5 @@
+require 'spec_helper'
+
+describe GitlabSchema.types['Milestone'] do
+  it { is_expected.to require_graphql_authorizations(:read_milestone) }
+end
diff --git a/spec/graphql/types/pipeline_type_spec.rb b/spec/graphql/types/pipeline_type_spec.rb
new file mode 100644
index 00000000000..562b2d2cbd9
--- /dev/null
+++ b/spec/graphql/types/pipeline_type_spec.rb
@@ -0,0 +1,5 @@
+require 'spec_helper'
+
+describe GitlabSchema.types['Pipeline'] do
+  it { is_expected.to require_graphql_authorizations(:read_pipeline) }
+end
diff --git a/spec/graphql/types/project_type_spec.rb b/spec/graphql/types/project_type_spec.rb
index 01d71abfac9..c995688af19 100644
--- a/spec/graphql/types/project_type_spec.rb
+++ b/spec/graphql/types/project_type_spec.rb
@@ -5,18 +5,11 @@ describe GitlabSchema.types['Project'] do
 
   it { expect(described_class.graphql_name).to eq('Project') }
 
-  describe 'nested merge request' do
-    it { expect(described_class).to have_graphql_field(:merge_request) }
+  it { is_expected.to require_graphql_authorizations(:read_project) }
 
-    it 'authorizes the merge request' do
-      expect(described_class.fields['mergeRequest'])
-        .to require_graphql_authorizations(:read_merge_request)
-    end
-  end
+  it { is_expected.to have_graphql_field(:merge_request) }
 
-  describe 'nested issues' do
-    it { expect(described_class).to have_graphql_field(:issues) }
-  end
+  it { is_expected.to have_graphql_field(:issues) }
 
   it { is_expected.to have_graphql_field(:pipelines) }
 end
diff --git a/spec/graphql/types/query_type_spec.rb b/spec/graphql/types/query_type_spec.rb
index e1df6f9811d..1640c170c6e 100644
--- a/spec/graphql/types/query_type_spec.rb
+++ b/spec/graphql/types/query_type_spec.rb
@@ -15,9 +15,5 @@ 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
 end
diff --git a/spec/graphql/types/user_type_spec.rb b/spec/graphql/types/user_type_spec.rb
new file mode 100644
index 00000000000..5f81170b863
--- /dev/null
+++ b/spec/graphql/types/user_type_spec.rb
@@ -0,0 +1,5 @@
+require 'spec_helper'
+
+describe GitlabSchema.types['User'] do
+  it { is_expected.to require_graphql_authorizations(:read_user) }
+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..7a7c3b7dc33 100644
--- a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb
+++ b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb
@@ -100,4 +100,21 @@ 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