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

Fix type authorizations in GraphQL

See merge request gitlab/gitlabhq!3170

1 files changed, 48 insertions, 34 deletions

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)