Merge branch 'security-bvl-enforce-graphql-type-authorization-11-11' into '11-11-stable'

Fix type authorizations in GraphQL

See merge request gitlab/gitlabhq!3173

13 files changed, 79 insertions, 43 deletions

diff --git a/app/graphql/types/label_type.rb b/app/graphql/types/label_type.rb
index ccd466edc1a..41d035c1aab 100644
--- a/app/graphql/types/label_type.rb
+++ b/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
     field :title, GraphQL::STRING_TYPE, null: false
     field :color, GraphQL::STRING_TYPE, null: false
diff --git a/app/graphql/types/metadata_type.rb b/app/graphql/types/metadata_type.rb
index 2d8bad0614b..7d7813a7652 100644
--- a/app/graphql/types/metadata_type.rb
+++ b/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
diff --git a/app/graphql/types/namespace_type.rb b/app/graphql/types/namespace_type.rb
index 36d8ee8c878..501bfcf0fd5 100644
--- a/app/graphql/types/namespace_type.rb
+++ b/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
diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb
index baea6658e05..51ac67217d1 100644
--- a/app/graphql/types/project_type.rb
+++ b/app/graphql/types/project_type.rb
@@ -66,7 +66,7 @@ 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 :merge_requests,
diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb
index 40d7de1a49a..d021658879c 100644
--- a/app/graphql/types/query_type.rb
+++ b/app/graphql/types/query_type.rb
@@ -17,10 +17,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
diff --git a/app/policies/repository_policy.rb b/app/policies/repository_policy.rb
new file mode 100644
index 00000000000..32340749858
--- /dev/null
+++ b/app/policies/repository_policy.rb
@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class RepositoryPolicy < BasePolicy
+  delegate { @subject.project }
+end
diff --git a/changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml b/changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml
new file mode 100644
index 00000000000..7dedb9f6230
--- /dev/null
+++ b/changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml
@@ -0,0 +1,5 @@
+---
+title: Add missing authorizations in GraphQL
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/graphql/authorize/authorize_field_service.rb b/lib/gitlab/graphql/authorize/authorize_field_service.rb
index 619ce100421..3b5dde2fde5 100644
--- a/lib/gitlab/graphql/authorize/authorize_field_service.rb
+++ b/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
 
diff --git a/spec/graphql/types/label_type_spec.rb b/spec/graphql/types/label_type_spec.rb
new file mode 100644
index 00000000000..da2b0b578da
--- /dev/null
+++ b/spec/graphql/types/label_type_spec.rb
@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe GitlabSchema.types['Label'] do
+  it { is_expected.to require_graphql_authorizations(:read_label) }
+end
diff --git a/spec/graphql/types/metadata_type_spec.rb b/spec/graphql/types/metadata_type_spec.rb
index 55205bf5b6a..5236380e477 100644
--- a/spec/graphql/types/metadata_type_spec.rb
+++ b/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
diff --git a/spec/graphql/types/namespace_type.rb b/spec/graphql/types/namespace_type.rb
index 7cd6a79ae5d..3aeea6ed6c7 100644
--- a/spec/graphql/types/namespace_type.rb
+++ b/spec/graphql/types/namespace_type.rb
@@ -4,4 +4,8 @@ require 'spec_helper'
 
 describe GitlabSchema.types['Namespace'] do
   it { expect(described_class.graphql_name).to eq('Namespace') }
+
+  it { expect(described_class).to have_graphql_field(:projects) }
+
+  it { is_expected.to require_graphql_authorizations(:read_namespace) }
 end
diff --git a/spec/graphql/types/query_type_spec.rb b/spec/graphql/types/query_type_spec.rb
index b4626955816..71aa38bf73c 100644
--- a/spec/graphql/types/query_type_spec.rb
+++ b/spec/graphql/types/query_type_spec.rb
@@ -24,9 +24,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
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 aec9c4baf0a..d60d1b7559a 100644
--- a/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb
+++ b/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") }
+      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 '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 }
+        let(:field) { type_with_field([custom_type], :read_field, presented_types).fields['testField'].to_graphql }
 
-        it "checks all permissions" do
+        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)