Merge branch 'security-bvl-enforce-graphql-type-authorization' into 'master'

Fix type authorizations in GraphQL

See merge request gitlab/gitlabhq!3170

app/graphql/types/ci/detailed_status_type.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 module Types
   module Ci
+    # rubocop: disable Graphql/AuthorizeTypes
+    # This is presented through `PipelineType` that has its own authorization
     class DetailedStatusType < BaseObject
       graphql_name 'DetailedStatus'
 
@@ -13,5 +15,6 @@ module Types
       field :text, GraphQL::STRING_TYPE, null: false
       field :tooltip, GraphQL::STRING_TYPE, null: false, method: :status_tooltip
     end
+    # rubocop: enable Graphql/AuthorizeTypes
   end
 end

app/graphql/types/issue_state_enum.rb

@@ -1,8 +1,11 @@
 # frozen_string_literal: true
 
 module Types
+  # rubocop: disable Graphql/AuthorizeTypes
+  # This is a BaseEnum through IssuableEnum, so it does not need authorization
   class IssueStateEnum < IssuableStateEnum
     graphql_name 'IssueState'
     description 'State of a GitLab issue'
   end
+  # rubocop: enable Graphql/AuthorizeTypes
 end

app/graphql/types/label_type.rb

@@ -4,6 +4,8 @@ module Types
   class LabelType < BaseObject
     graphql_name 'Label'
 
+    authorize :read_label
+
     field :description, GraphQL::STRING_TYPE, null: true
     markdown_field :description_html, null: true
     field :title, GraphQL::STRING_TYPE, null: false

app/graphql/types/merge_request_state_enum.rb

@@ -1,10 +1,13 @@
 # frozen_string_literal: true
 
 module Types
+  # rubocop: disable Graphql/AuthorizeTypes
+  # This is a BaseEnum through IssuableEnum, so it does not need authorization
   class MergeRequestStateEnum < IssuableStateEnum
     graphql_name 'MergeRequestState'
     description 'State of a GitLab merge request'
 
     value 'merged'
   end
+  # rubocop: enable Graphql/AuthorizeTypes
 end

app/graphql/types/metadata_type.rb

@@ -4,6 +4,8 @@ module Types
   class MetadataType < ::Types::BaseObject
     graphql_name 'Metadata'
 
+    authorize :read_instance_metadata
+
     field :version, GraphQL::STRING_TYPE, null: false
     field :revision, GraphQL::STRING_TYPE, null: false
   end

app/graphql/types/namespace_type.rb

@@ -4,6 +4,8 @@ module Types
   class NamespaceType < BaseObject
     graphql_name 'Namespace'
 
+    authorize :read_namespace
+
     field :id, GraphQL::ID_TYPE, null: false
 
     field :name, GraphQL::STRING_TYPE, null: false

app/graphql/types/notes/diff_position_type.rb

@@ -2,6 +2,8 @@
 
 module Types
   module Notes
+    # rubocop: disable Graphql/AuthorizeTypes
+    # This is presented through `NoteType` that has its own authorization
     class DiffPositionType < BaseObject
       graphql_name 'DiffPosition'
 
@@ -42,5 +44,6 @@ module Types
             description: "The total height of the image",
             resolve: -> (position, _args, _ctx) { position.height if position.on_image? }
     end
+    # rubocop: enable Graphql/AuthorizeTypes
   end
 end

app/graphql/types/project_type.rb

@@ -67,14 +67,14 @@ module Types
     field :only_allow_merge_if_all_discussions_are_resolved, GraphQL::BOOLEAN_TYPE, null: true
     field :printing_merge_request_link_enabled, GraphQL::BOOLEAN_TYPE, null: true
 
-    field :namespace, Types::NamespaceType, null: false
+    field :namespace, Types::NamespaceType, null: true
     field :group, Types::GroupType, null: true
 
     field :statistics, Types::ProjectStatisticsType,
           null: true,
           resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchProjectStatisticsLoader.new(obj.id).find }
 
-    field :repository, Types::RepositoryType, null: false
+    field :repository, Types::RepositoryType, null: true
 
     field :merge_requests,
           Types::MergeRequestType.connection_type,

app/graphql/types/query_type.rb

@@ -22,10 +22,7 @@ module Types
     field :metadata, Types::MetadataType,
           null: true,
           resolver: Resolvers::MetadataResolver,
-          description: 'Metadata about GitLab' do |*args|
-
-      authorize :read_instance_metadata
-    end
+          description: 'Metadata about GitLab'
 
     field :echo, GraphQL::STRING_TYPE, null: false, function: Functions::Echo.new
   end

app/graphql/types/task_completion_status.rb

@@ -1,6 +1,9 @@
 # frozen_string_literal: true
 
 module Types
+  # rubocop: disable Graphql/AuthorizeTypes
+  # This is used in `IssueType` and `MergeRequestType` both of which have their
+  # own authorization
   class TaskCompletionStatus < BaseObject
     graphql_name 'TaskCompletionStatus'
     description 'Completion status of tasks'
@@ -8,4 +11,5 @@ module Types
     field :count, GraphQL::INT_TYPE, null: false
     field :completed_count, GraphQL::INT_TYPE, null: false
   end
+  # rubocop: enable Graphql/AuthorizeTypes
 end

app/graphql/types/tree/blob_type.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 module Types
   module Tree
+    # rubocop: disable Graphql/AuthorizeTypes
+    # This is presented through `Repository` that has its own authorization
     class BlobType < BaseObject
       implements Types::Tree::EntryType
 
@@ -12,6 +14,7 @@ module Types
       field :lfs_oid, GraphQL::STRING_TYPE, null: true, resolve: -> (blob, args, ctx) do
         Gitlab::Graphql::Loaders::BatchLfsOidLoader.new(blob.repository, blob.id).find
       end
+      # rubocop: enable Graphql/AuthorizeTypes
     end
   end
 end

app/graphql/types/tree/submodule_type.rb

@@ -1,10 +1,13 @@
 # frozen_string_literal: true
 module Types
   module Tree
+    # rubocop: disable Graphql/AuthorizeTypes
+    # This is presented through `Repository` that has its own authorization
     class SubmoduleType < BaseObject
       implements Types::Tree::EntryType
 
       graphql_name 'Submodule'
     end
+    # rubocop: enable Graphql/AuthorizeTypes
   end
 end

app/graphql/types/tree/tree_entry_type.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 module Types
   module Tree
+    # rubocop: disable Graphql/AuthorizeTypes
+    # This is presented through `Repository` that has its own authorization
     class TreeEntryType < BaseObject
       implements Types::Tree::EntryType
 
@@ -11,5 +13,6 @@ module Types
 
       field :web_url, GraphQL::STRING_TYPE, null: true
     end
+    # rubocop: enable Graphql/AuthorizeTypes
   end
 end

app/graphql/types/tree/tree_type.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 module Types
   module Tree
+    # rubocop: disable Graphql/AuthorizeTypes
+    # This is presented through `Repository` that has its own authorization
     class TreeType < BaseObject
       graphql_name 'Tree'
 
@@ -13,6 +15,7 @@ module Types
       field :blobs, Types::Tree::BlobType.connection_type, null: false, resolve: -> (obj, args, ctx) do
         Gitlab::Graphql::Representation::TreeEntry.decorate(obj.blobs, obj.repository)
       end
+      # rubocop: enable Graphql/AuthorizeTypes
     end
   end
 end

app/policies/repository_policy.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class RepositoryPolicy < BasePolicy
+  delegate { @subject.project }
+end

changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml

@@ -0,0 +1,5 @@
+---
+title: Add missing authorizations in GraphQL
+merge_request:
+author:
+type: security

lib/gitlab/graphql/authorize/authorize_field_service.rb

@@ -39,6 +39,8 @@ module Gitlab
             type = node_type_for_basic_connection(type)
           end
 
+          type = type.unwrap if type.kind.non_null?
+
           Array.wrap(type.metadata[:authorize])
         end
 

rubocop/cop/graphql/authorize_types.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+require_relative '../../spec_helpers'
+
+module RuboCop
+  module Cop
+    module Graphql
+      class AuthorizeTypes < RuboCop::Cop::Cop
+        include SpecHelpers
+
+        MSG = 'Add an `authorize :ability` call to the type: '\
+              'https://docs.gitlab.com/ee/development/api_graphql_styleguide.html#type-authorization'
+
+        TYPES_DIR = 'app/graphql/types'
+
+        # We want to exclude our own basetypes and scalars
+        WHITELISTED_TYPES = %w[BaseEnum BaseScalar BasePermissionType MutationType
+                               QueryType GraphQL::Schema BaseUnion].freeze
+
+        def_node_search :authorize?, <<~PATTERN
+          (send nil? :authorize ...)
+        PATTERN
+
+        def on_class(node)
+          return unless in_type?(node)
+          return if whitelisted?(class_constant(node))
+          return if whitelisted?(superclass_constant(node))
+
+          add_offense(node, location: :expression) unless authorize?(node)
+        end
+
+        private
+
+        def in_type?(node)
+          return if in_spec?(node)
+
+          path = node.location.expression.source_buffer.name
+
+          path.include?(TYPES_DIR)
+        end
+
+        def whitelisted?(class_node)
+          return false unless class_node&.const_name
+
+          WHITELISTED_TYPES.any? { |whitelisted| class_node.const_name.include?(whitelisted) }
+        end
+
+        def class_constant(node)
+          node.descendants.first
+        end
+
+        def superclass_constant(class_node)
+          # First one is the class name itself, second is it's superclass
+          _class_constant, *others = class_node.descendants
+
+          others.find { |node| node.const_type? && node&.const_name != 'Types' }
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -43,3 +43,4 @@ require_relative 'cop/code_reuse/serializer'
 require_relative 'cop/code_reuse/active_record'
 require_relative 'cop/group_public_or_visible_to_user'
 require_relative 'cop/inject_enterprise_edition_module'
+require_relative 'cop/graphql/authorize_types'

spec/graphql/types/label_type_spec.rb

@@ -7,4 +7,6 @@ describe GitlabSchema.types['Label'] do
 
     is_expected.to have_graphql_fields(*expected_fields)
   end
+
+  it { is_expected.to require_graphql_authorizations(:read_label) }
 end

spec/graphql/types/metadata_type_spec.rb

@@ -2,4 +2,5 @@ require 'spec_helper'
 
 describe GitlabSchema.types['Metadata'] do
   it { expect(described_class.graphql_name).to eq('Metadata') }
+  it { is_expected.to require_graphql_authorizations(:read_instance_metadata) }
 end

spec/graphql/types/namespace_type_spec.rb

@@ -13,4 +13,6 @@ describe GitlabSchema.types['Namespace'] do
 
     is_expected.to have_graphql_fields(*expected_fields)
   end
+
+  it { is_expected.to require_graphql_authorizations(:read_namespace) }
 end

spec/graphql/types/query_type_spec.rb

@@ -34,9 +34,5 @@ describe GitlabSchema.types['Query'] do
       is_expected.to have_graphql_type(Types::MetadataType)
       is_expected.to have_graphql_resolver(Resolvers::MetadataResolver)
     end
-
-    it 'authorizes with read_instance_metadata' do
-      is_expected.to require_graphql_authorizations(:read_instance_metadata)
-    end
   end
 end

spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb

@@ -7,35 +7,39 @@ require 'spec_helper'
 describe Gitlab::Graphql::Authorize::AuthorizeFieldService do
   def type(type_authorizations = [])
     Class.new(Types::BaseObject) do
-      graphql_name "TestType"
+      graphql_name 'TestType'
 
       authorize type_authorizations
     end
   end
 
-  def type_with_field(field_type, field_authorizations = [], resolved_value = "Resolved value")
+  def type_with_field(field_type, field_authorizations = [], resolved_value = 'Resolved value', **options)
     Class.new(Types::BaseObject) do
-      graphql_name "TestTypeWithField"
-      field :test_field, field_type, null: true, authorize: field_authorizations, resolve: -> (_, _, _) { resolved_value}
+      graphql_name 'TestTypeWithField'
+      options.reverse_merge!(null: true)
+      field :test_field, field_type,
+            authorize: field_authorizations,
+            resolve: -> (_, _, _) { resolved_value },
+            **options
     end
   end
 
   let(:current_user) { double(:current_user) }
   subject(:service) { described_class.new(field) }
 
-  describe "#authorized_resolve" do
-    let(:presented_object) { double("presented object") }
-    let(:presented_type) { double("parent type", object: presented_object) }
+  describe '#authorized_resolve' do
+    let(:presented_object) { double('presented object') }
+    let(:presented_type) { double('parent type', object: presented_object) }
     subject(:resolved) { service.authorized_resolve.call(presented_type, {}, { current_user: current_user }) }
 
-    context "scalar types" do
-      shared_examples "checking permissions on the presented object" do
-        it "checks the abilities on the object being presented and returns the value" do
+    context 'scalar types' do
+      shared_examples 'checking permissions on the presented object' do
+        it 'checks the abilities on the object being presented and returns the value' do
           expected_permissions.each do |permission|
             spy_ability_check_for(permission, presented_object, passed: true)
           end
 
-          expect(resolved).to eq("Resolved value")
+          expect(resolved).to eq('Resolved value')
         end
 
         it "returns nil if the value wasn't authorized" do
@@ -45,61 +49,71 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do
         end
       end
 
-      context "when the field is a built-in scalar type" do
-        let(:field) { type_with_field(GraphQL::STRING_TYPE, :read_field).fields["testField"].to_graphql }
+      context 'when the field is a built-in scalar type' do
+        let(:field) { type_with_field(GraphQL::STRING_TYPE, :read_field).fields['testField'].to_graphql }
         let(:expected_permissions) { [:read_field] }
 
-        it_behaves_like "checking permissions on the presented object"
+        it_behaves_like 'checking permissions on the presented object'
       end
 
-      context "when the field is a list of scalar types" do
-        let(:field) { type_with_field([GraphQL::STRING_TYPE], :read_field).fields["testField"].to_graphql }
+      context 'when the field is a list of scalar types' do
+        let(:field) { type_with_field([GraphQL::STRING_TYPE], :read_field).fields['testField'].to_graphql }
         let(:expected_permissions) { [:read_field] }
 
-        it_behaves_like "checking permissions on the presented object"
+        it_behaves_like 'checking permissions on the presented object'
       end
 
-      context "when the field is sub-classed scalar type" do
-        let(:field) { type_with_field(Types::TimeType, :read_field).fields["testField"].to_graphql }
+      context 'when the field is sub-classed scalar type' do
+        let(:field) { type_with_field(Types::TimeType, :read_field).fields['testField'].to_graphql }
         let(:expected_permissions) { [:read_field] }
 
-        it_behaves_like "checking permissions on the presented object"
+        it_behaves_like 'checking permissions on the presented object'
       end
 
-      context "when the field is a list of sub-classed scalar types" do
-        let(:field) { type_with_field([Types::TimeType], :read_field).fields["testField"].to_graphql }
+      context 'when the field is a list of sub-classed scalar types' do
+        let(:field) { type_with_field([Types::TimeType], :read_field).fields['testField'].to_graphql }
         let(:expected_permissions) { [:read_field] }
 
-        it_behaves_like "checking permissions on the presented object"
+        it_behaves_like 'checking permissions on the presented object'
       end
     end
 
-    context "when the field is a specific type" do
+    context 'when the field is a specific type' do
       let(:custom_type) { type(:read_type) }
-      let(:object_in_field) { double("presented in field") }
-      let(:field) { type_with_field(custom_type, :read_field, object_in_field).fields["testField"].to_graphql }
+      let(:object_in_field) { double('presented in field') }
+      let(:field) { type_with_field(custom_type, :read_field, object_in_field).fields['testField'].to_graphql }
 
-      it "checks both field & type permissions" do
+      it 'checks both field & type permissions' do
         spy_ability_check_for(:read_field, object_in_field, passed: true)
         spy_ability_check_for(:read_type, object_in_field, passed: true)
 
         expect(resolved).to eq(object_in_field)
       end
 
-      it "returns nil if viewing was not allowed" do
+      it 'returns nil if viewing was not allowed' do
         spy_ability_check_for(:read_field, object_in_field, passed: false)
         spy_ability_check_for(:read_type, object_in_field, passed: true)
 
         expect(resolved).to be_nil
       end
 
-      context "when the field is a list" do
-        let(:object_1) { double("presented in field 1") }
-        let(:object_2) { double("presented in field 2") }
-        let(:presented_types) { [double(object: object_1), double(object: object_2)] }
-        let(:field) { type_with_field([custom_type], :read_field, presented_types).fields["testField"].to_graphql }
+      context 'when the field is not nullable' do
+        let(:field) { type_with_field(custom_type, [], object_in_field, null: false).fields['testField'].to_graphql }
 
-        it "checks all permissions" do
+        it 'returns nil when viewing is not allowed' do
+          spy_ability_check_for(:read_type, object_in_field, passed: false)
+
+          expect(resolved).to be_nil
+        end
+      end
+
+      context 'when the field is a list' do
+        let(:object_1) { double('presented in field 1') }
+        let(:object_2) { double('presented in field 2') }
+        let(:presented_types) { [double(object: object_1), double(object: object_2)] }
+        let(:field) { type_with_field([custom_type], :read_field, presented_types).fields['testField'].to_graphql }
+
+        it 'checks all permissions' do
           allow(Ability).to receive(:allowed?) { true }
 
           spy_ability_check_for(:read_field, object_1, passed: true)
@@ -110,7 +124,7 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do
           expect(resolved).to eq(presented_types)
         end
 
-        it "filters out objects that the user cannot see" do
+        it 'filters out objects that the user cannot see' do
           allow(Ability).to receive(:allowed?) { true }
 
           spy_ability_check_for(:read_type, object_1, passed: false)

spec/requests/api/graphql/namespace/projects_spec.rb

@@ -58,9 +58,7 @@ describe 'getting projects', :nested_groups do
       it 'finds only public projects' do
         post_graphql(query, current_user: nil)
 
-        expect(graphql_data['namespace']['projects']['edges'].size).to eq(1)
-        project = graphql_data['namespace']['projects']['edges'][0]['node']
-        expect(project['id']).to eq(public_project.to_global_id.to_s)
+        expect(graphql_data['namespace']).to be_nil
       end
     end
   end

spec/requests/api/graphql/project/repository_spec.rb

@@ -34,4 +34,28 @@ describe 'getting a repository in a project' do
       expect(graphql_data['project']).to be(nil)
     end
   end
+
+  context 'when the repository is only accessible to members' do
+    let(:project) do
+      create(:project, :public, :repository, repository_access_level: ProjectFeature::PRIVATE)
+    end
+
+    it 'returns a repository for the owner' do
+      post_graphql(query, current_user: current_user)
+
+      expect(graphql_data['project']['repository']).not_to be_nil
+    end
+
+    it 'returns nil for the repository for other users' do
+      post_graphql(query, current_user: create(:user))
+
+      expect(graphql_data['project']['repository']).to be_nil
+    end
+
+    it 'returns nil for the repository for other users' do
+      post_graphql(query, current_user: nil)
+
+      expect(graphql_data['project']['repository']).to be_nil
+    end
+  end
 end

spec/rubocop/cop/graphql/authorize_types_spec.rb

@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../../rubocop/cop/graphql/authorize_types'
+
+describe RuboCop::Cop::Graphql::AuthorizeTypes do
+  include RuboCop::RSpec::ExpectOffense
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'when in a type folder' do
+    before do
+      allow(cop).to receive(:in_type?).and_return(true)
+    end
+
+    it 'adds an offense when there is no authorize call' do
+      inspect_source(<<~TYPE)
+        module Types
+          class AType < BaseObject
+            field :a_thing
+            field :another_thing
+          end
+        end
+      TYPE
+
+      expect(cop.offenses.size).to eq 1
+    end
+
+    it 'does not add an offense for classes that have an authorize call' do
+      expect_no_offenses(<<~TYPE.strip)
+        module Types
+          class AType < BaseObject
+            graphql_name 'ATypeName'
+
+            authorize :an_ability, :second_ability
+
+            field :a_thing
+          end
+        end
+      TYPE
+    end
+
+    it 'does not add an offense for classes that only have an authorize call' do
+      expect_no_offenses(<<~TYPE.strip)
+        module Types
+          class AType < SuperClassWithFields
+            authorize :an_ability
+          end
+        end
+      TYPE
+    end
+
+    it 'does not add an offense for base types' do
+      expect_no_offenses(<<~TYPE)
+        module Types
+          class AType < BaseEnum
+            field :a_thing
+          end
+        end
+      TYPE
+    end
+  end
+end