From eca8e6f09b1800b58904582b527103b5c755e898 Mon Sep 17 00:00:00 2001
From: Bob Van Landuyt <bob@vanlanduyt.co>
Date: Thu, 11 Apr 2019 11:07:06 +0200
Subject: Only check abilities on rendered GraphQL nodes

With this we only check abilities on the rendered edges of a GraphQL
connection instead of all the nodes in it.
---
 lib/gitlab/graphql/authorize.rb                    |   2 +-
 .../graphql/authorize/authorize_field_service.rb   |  86 ++++++++------
 .../graphql/connections/keyset_connection.rb       |  16 ++-
 lib/gitlab/graphql/errors.rb                       |   1 -
 spec/graphql/features/authorization_spec.rb        |  77 +++++++++---
 spec/graphql/gitlab_schema_spec.rb                 |   2 +-
 .../authorize/authorize_field_service_spec.rb      | 130 +++++++++++----------
 .../graphql/connections/keyset_connection_spec.rb  |   5 +
 spec/requests/api/graphql/project/issues_spec.rb   |  28 ++++-
 9 files changed, 226 insertions(+), 121 deletions(-)

diff --git a/lib/gitlab/graphql/authorize.rb b/lib/gitlab/graphql/authorize.rb
index f62813db82c..f8d0208e275 100644
--- a/lib/gitlab/graphql/authorize.rb
+++ b/lib/gitlab/graphql/authorize.rb
@@ -8,7 +8,7 @@ module Gitlab
       extend ActiveSupport::Concern
 
       def self.use(schema_definition)
-        schema_definition.instrument(:field, Instrumentation.new)
+        schema_definition.instrument(:field, Instrumentation.new, after_built_ins: true)
       end
     end
   end
diff --git a/lib/gitlab/graphql/authorize/authorize_field_service.rb b/lib/gitlab/graphql/authorize/authorize_field_service.rb
index 8deff79fc84..03d6aabb0e3 100644
--- a/lib/gitlab/graphql/authorize/authorize_field_service.rb
+++ b/lib/gitlab/graphql/authorize/authorize_field_service.rb
@@ -15,15 +15,10 @@ module Gitlab
 
         def authorized_resolve
           proc do |parent_typed_object, args, ctx|
-            resolved_obj = @old_resolve_proc.call(parent_typed_object, args, ctx)
-            authorizing_obj = authorize_against(parent_typed_object)
-            checker = build_checker(ctx[:current_user], authorizing_obj)
-
-            if resolved_obj.respond_to?(:then)
-              resolved_obj.then(&checker)
-            else
-              checker.call(resolved_obj)
-            end
+            resolved_type = @old_resolve_proc.call(parent_typed_object, args, ctx)
+            authorizing_object = authorize_against(parent_typed_object, resolved_type)
+
+            filter_allowed(ctx[:current_user], resolved_type, authorizing_object)
           end
         end
 
@@ -38,7 +33,7 @@ module Gitlab
           type = @field.type
 
           # When the return type of @field is a collection, find the singular type
-          if type.get_field('edges')
+          if @field.connection?
             type = node_type_for_relay_connection(type)
           elsif type.list?
             type = node_type_for_basic_connection(type)
@@ -52,43 +47,60 @@ module Gitlab
           Array.wrap(@field.metadata[:authorize])
         end
 
-        # If it's a built-in/scalar type, authorize using its parent object.
-        # nil means authorize using the resolved object
-        def authorize_against(parent_typed_object)
-          parent_typed_object.object if built_in_type? && parent_typed_object.respond_to?(:object)
+        def authorize_against(parent_typed_object, resolved_type)
+          if built_in_type?
+            # The field is a built-in/scalar type, or a list of scalars
+            # authorize using the parent's object
+            parent_typed_object.object
+          elsif resolved_type.respond_to?(:object)
+            # The field is a type representing a single object, we'll authorize
+            # against the object directly
+            resolved_type.object
+          elsif @field.connection? || resolved_type.is_a?(Array)
+            # The field is a connection or a list of non-built-in types, we'll
+            # authorize each element when rendering
+            nil
+          else
+            # Resolved type is a single object that might not be loaded yet by
+            # the batchloader, we'll authorize that
+            resolved_type
+          end
         end
 
-        def build_checker(current_user, authorizing_obj)
-          lambda do |resolved_obj|
-            # Load the elements if they were not loaded by BatchLoader yet
-            resolved_obj = resolved_obj.sync if resolved_obj.respond_to?(:sync)
-
-            check = lambda do |object|
-              authorizations.all? do |ability|
-                Ability.allowed?(current_user, ability, authorizing_obj || object)
-              end
+        def filter_allowed(current_user, resolved_type, authorizing_object)
+          if authorizing_object
+            # Authorizing fields representing scalars, or a simple field with an object
+            resolved_type if allowed_access?(current_user, authorizing_object)
+          elsif @field.connection?
+            # A connection with pagination, modify the visible nodes in on the
+            # connection type in place
+            resolved_type.edge_nodes.to_a.keep_if { |node| allowed_access?(current_user, node) }
+            resolved_type
+          elsif resolved_type.is_a? Array
+            # A simple list of rendered types  each object being an object to authorize
+            resolved_type.select do |single_object_type|
+              allowed_access?(current_user, single_object_type.object)
             end
+          elsif resolved_type.nil?
+            # We're not rendering anything, for example when a record was not found
+            # no need to do anything
+          else
+            raise "Can't authorize #{@field}"
+          end
+        end
 
-            case resolved_obj
-            when Array, ActiveRecord::Relation
-              resolved_obj.select(&check)
-            else
-              resolved_obj if check.call(resolved_obj)
-            end
+        def allowed_access?(current_user, object)
+          object = object.sync if object.respond_to?(:sync)
+
+          authorizations.all? do |ability|
+            Ability.allowed?(current_user, ability, object)
           end
         end
 
         # Returns the singular type for relay connections.
         # This will be the type class of edges.node
         def node_type_for_relay_connection(type)
-          type = type.get_field('edges').type.unwrap.get_field('node')&.type
-
-          if type.nil?
-            raise Gitlab::Graphql::Errors::ConnectionDefinitionError,
-              'Connection Type must conform to the Relay Cursor Connections Specification'
-          end
-
-          type
+          type.unwrap.get_field('edges').type.unwrap.get_field('node').type
         end
 
         # Returns the singular type for basic connections, for example `[Types::ProjectType]`
diff --git a/lib/gitlab/graphql/connections/keyset_connection.rb b/lib/gitlab/graphql/connections/keyset_connection.rb
index 851054c0393..715963a44c1 100644
--- a/lib/gitlab/graphql/connections/keyset_connection.rb
+++ b/lib/gitlab/graphql/connections/keyset_connection.rb
@@ -22,8 +22,17 @@ module Gitlab
         end
         # rubocop: enable CodeReuse/ActiveRecord
 
-        # rubocop: disable CodeReuse/ActiveRecord
         def paged_nodes
+          # These are the nodes that will be loaded into memory for rendering
+          # So we're ok loading them into memory here as that's bound to happen
+          # anyway. Having them ready means we can modify the result while
+          # rendering the fields.
+          @paged_nodes ||= load_paged_nodes.to_a
+        end
+
+        private
+
+        def load_paged_nodes
           if first && last
             raise Gitlab::Graphql::Errors::ArgumentError.new("Can only provide either `first` or `last`, not both")
           end
@@ -31,12 +40,9 @@ module Gitlab
           if last
             sliced_nodes.last(limit_value)
           else
-            sliced_nodes.limit(limit_value)
+            sliced_nodes.limit(limit_value) # rubocop: disable CodeReuse/ActiveRecord
           end
         end
-        # rubocop: enable CodeReuse/ActiveRecord
-
-        private
 
         def before_slice
           if sort_direction == :asc
diff --git a/lib/gitlab/graphql/errors.rb b/lib/gitlab/graphql/errors.rb
index bcbba72e017..fe74549e322 100644
--- a/lib/gitlab/graphql/errors.rb
+++ b/lib/gitlab/graphql/errors.rb
@@ -6,7 +6,6 @@ module Gitlab
       BaseError = Class.new(GraphQL::ExecutionError)
       ArgumentError = Class.new(BaseError)
       ResourceNotAvailable = Class.new(BaseError)
-      ConnectionDefinitionError = Class.new(BaseError)
     end
   end
 end
diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb
index 00e31568a9e..f5eb628a982 100644
--- a/spec/graphql/features/authorization_spec.rb
+++ b/spec/graphql/features/authorization_spec.rb
@@ -177,6 +177,7 @@ describe 'Gitlab::Graphql::Authorization' do
 
   describe 'type authorizations when applied to a relay connection' do
     let(:query_string) { '{ object() { edges { node { name } } } }' }
+    let(:second_test_object) { double(name: 'Second thing') }
 
     let(:type) do
       type_factory do |type|
@@ -186,22 +187,41 @@ describe 'Gitlab::Graphql::Authorization' do
 
     let(:query_type) do
       query_factory do |query|
-        query.field :object, type.connection_type, null: true, resolve: ->(obj, args, ctx) { [test_object] }
+        query.field :object, type.connection_type, null: true, resolve: ->(obj, args, ctx) { [test_object, second_test_object] }
       end
     end
 
     subject { result.dig('object', 'edges') }
 
-    it 'returns the protected field when user has permission' do
+    it 'returns only the elements visible to the user' do
       permit(permission_single)
 
-      expect(subject).not_to be_empty
+      expect(subject.size).to eq 1
       expect(subject.first['node']).to eq('name' => test_object.name)
     end
 
     it 'returns nil when user is not authorized' do
       expect(subject).to be_empty
     end
+
+    describe 'limiting connections with multiple objects' do
+      let(:query_type) do
+        query_factory do |query|
+          query.field :object, type.connection_type, null: true, resolve: ->(obj, args, ctx) do
+            [test_object, second_test_object]
+          end
+        end
+      end
+
+      let(:query_string) { '{ object(first: 1) { edges { node { name } } } }' }
+
+      it 'only checks permissions for the first object' do
+        expect(Ability).to receive(:allowed?).with(user, permission_single, test_object) { true }
+        expect(Ability).not_to receive(:allowed?).with(user, permission_single, second_test_object)
+
+        expect(subject.size).to eq(1)
+      end
+    end
   end
 
   describe 'type authorizations when applied to a basic connection' do
@@ -222,28 +242,53 @@ describe 'Gitlab::Graphql::Authorization' do
     include_examples 'authorization with a single permission'
   end
 
-  describe 'when connections do not follow the correct specification' do
-    let(:query_string) { '{ object() { edges { node { name }} } }' }
+  describe 'Authorizations on active record relations' do
+    let!(:visible_project) { create(:project, :private) }
+    let!(:other_project) { create(:project, :private) }
+    let!(:visible_issues) { create_list(:issue, 2, project: visible_project) }
+    let!(:other_issues) { create_list(:issue, 2, project: other_project) }
+    let!(:user) { visible_project.owner }
 
-    let(:type) do
-      bad_node = type_factory do |type|
-        type.graphql_name 'BadNode'
-        type.field :bad_node, GraphQL::STRING_TYPE, null: true
+    let(:issue_type) do
+      type_factory do |type|
+        type.graphql_name 'FakeIssueType'
+        type.authorize :read_issue
+        type.field :id, GraphQL::ID_TYPE, null: false
       end
-
+    end
+    let(:project_type) do |type|
       type_factory do |type|
-        type.field :edges, [bad_node], null: true
+        type.graphql_name 'FakeProjectType'
+        type.field :test_issues, issue_type.connection_type, null: false, resolve: -> (_, _, _) { Issue.where(project: [visible_project, other_project]) }
       end
     end
-
     let(:query_type) do
       query_factory do |query|
-        query.field :object, type, null: true
+        query.field :test_project, project_type, null: false, resolve: -> (_, _, _) { visible_project }
       end
     end
+    let(:query_string) do
+      <<~QRY
+        { testProject { testIssues(first: 3) { edges { node { id } } } } }
+      QRY
+    end
+
+    before do
+      allow(Ability).to receive(:allowed?).and_call_original
+    end
+
+    it 'renders the issues the user has access to' do
+      issue_edges = result['testProject']['testIssues']['edges']
+      issue_ids = issue_edges.map { |issue_edge| issue_edge['node']&.fetch('id') }
+
+      expect(issue_edges.size).to eq(visible_issues.size)
+      expect(issue_ids).to eq(visible_issues.map { |i| i.id.to_s })
+    end
+
+    it 'does not check access on fields that will not be rendered' do
+      expect(Ability).not_to receive(:allowed?).with(user, :read_issue, other_issues.last)
 
-    it 'throws an error' do
-      expect { result }.to raise_error(Gitlab::Graphql::Errors::ConnectionDefinitionError)
+      result
     end
   end
 
@@ -276,6 +321,8 @@ describe 'Gitlab::Graphql::Authorization' do
   def execute_query(query_type)
     schema = Class.new(GraphQL::Schema) do
       use Gitlab::Graphql::Authorize
+      use Gitlab::Graphql::Connections
+
       query(query_type)
     end
 
diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb
index 74e93b2c4df..05f10fb40f0 100644
--- a/spec/graphql/gitlab_schema_spec.rb
+++ b/spec/graphql/gitlab_schema_spec.rb
@@ -74,6 +74,6 @@ describe GitlabSchema do
   end
 
   def field_instrumenters
-    described_class.instrumenters[:field]
+    described_class.instrumenters[:field] + described_class.instrumenters[:field_after_built_ins]
   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 6114aca0616..95a4eb296fb 100644
--- a/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb
+++ b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb
@@ -5,92 +5,104 @@ require 'spec_helper'
 # Also see spec/graphql/features/authorization_spec.rb for
 # integration tests of AuthorizeFieldService
 describe Gitlab::Graphql::Authorize::AuthorizeFieldService do
-  describe '#build_checker' do
-    let(:current_user) { double(:current_user) }
-    let(:abilities) { [double(:first_ability), double(:last_ability)] }
-
-    context 'when authorizing against the object' do
-      let(:checker) do
-        service = described_class.new(double(resolve_proc: proc {}))
-        allow(service).to receive(:authorizations).and_return(abilities)
-        service.__send__(:build_checker, current_user, nil)
-      end
+  def type(type_authorizations = [])
+    Class.new(Types::BaseObject) do
+      graphql_name "TestType"
 
-      it 'returns a checker which checks for a single object' do
-        object = double(:object)
+      authorize type_authorizations
+    end
+  end
 
-        abilities.each do |ability|
-          spy_ability_check_for(ability, object, passed: true)
-        end
+  def type_with_field(field_type, field_authorizations = [], resolved_value = "Resolved value")
+    Class.new(Types::BaseObject) do
+      graphql_name "TestTypeWithField"
+      field :test_field, field_type, null: true, authorize: field_authorizations, resolve: -> (_, _, _) { resolved_value}
+    end
+  end
 
-        expect(checker.call(object)).to eq(object)
-      end
+  let(:current_user) { double(:current_user) }
+  subject(:service) { described_class.new(field) }
 
-      it 'returns a checker which checks for all objects' do
-        objects = [double(:first), double(:last)]
+  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 }) }
 
-        abilities.each do |ability|
-          objects.each do |object|
-            spy_ability_check_for(ability, object, passed: true)
+    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")
         end
 
-        expect(checker.call(objects)).to eq(objects)
+        it "returns nil if the value wasn't authorized" do
+          allow(Ability).to receive(:allowed?).and_return false
+
+          expect(resolved).to be_nil
+        end
       end
 
-      context 'when some objects would not pass the check' do
-        it 'returns nil when it is single object' do
-          disallowed = double(:object)
+      context "when the field is a scalar type" do
+        let(:field) { type_with_field(GraphQL::STRING_TYPE, :read_field).fields["testField"].to_graphql }
+        let(:expected_permissions) { [:read_field] }
 
-          spy_ability_check_for(abilities.first, disallowed, passed: false)
+        it_behaves_like "checking permissions on the presented object"
+      end
 
-          expect(checker.call(disallowed)).to be_nil
-        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 }
+        let(:expected_permissions) { [:read_field] }
 
-        it 'returns only objects which passed when there are more than one' do
-          allowed = double(:allowed)
-          disallowed = double(:disallowed)
+        it_behaves_like "checking permissions on the presented object"
+      end
+    end
 
-          spy_ability_check_for(abilities.first, disallowed, passed: false)
+    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 }
 
-          abilities.each do |ability|
-            spy_ability_check_for(ability, allowed, passed: true)
-          end
+      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(checker.call([disallowed, allowed])).to contain_exactly(allowed)
-        end
+        expect(resolved).to eq(object_in_field)
       end
-    end
 
-    context 'when authorizing against another object' do
-      let(:authorizing_obj) { double(:object) }
+      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)
 
-      let(:checker) do
-        service = described_class.new(double(resolve_proc: proc {}))
-        allow(service).to receive(:authorizations).and_return(abilities)
-        service.__send__(:build_checker, current_user, authorizing_obj)
+        expect(resolved).to be_nil
       end
 
-      it 'returns a checker which checks for a single object' do
-        object = double(:object)
+      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 }
 
-        abilities.each do |ability|
-          spy_ability_check_for(ability, authorizing_obj, passed: true)
+        it "checks all permissions" do
+          allow(Ability).to receive(:allowed?) { true }
+
+          spy_ability_check_for(:read_field, object_1, passed: true)
+          spy_ability_check_for(:read_type, object_1, passed: true)
+          spy_ability_check_for(:read_field, object_2, passed: true)
+          spy_ability_check_for(:read_type, object_2, passed: true)
+
+          expect(resolved).to eq(presented_types)
         end
 
-        expect(checker.call(object)).to eq(object)
-      end
+        it "filters out objects that the user cannot see" do
+          allow(Ability).to receive(:allowed?) { true }
 
-      it 'returns a checker which checks for all objects' do
-        objects = [double(:first), double(:last)]
+          spy_ability_check_for(:read_type, object_1, passed: false)
 
-        abilities.each do |ability|
-          objects.each do |object|
-            spy_ability_check_for(ability, authorizing_obj, passed: true)
-          end
+          expect(resolved.map(&:object)).to contain_exactly(object_2)
         end
-
-        expect(checker.call(objects)).to eq(objects)
       end
     end
   end
diff --git a/spec/lib/gitlab/graphql/connections/keyset_connection_spec.rb b/spec/lib/gitlab/graphql/connections/keyset_connection_spec.rb
index 9bcc1e78a78..fefa2881b18 100644
--- a/spec/lib/gitlab/graphql/connections/keyset_connection_spec.rb
+++ b/spec/lib/gitlab/graphql/connections/keyset_connection_spec.rb
@@ -85,6 +85,11 @@ describe Gitlab::Graphql::Connections::KeysetConnection do
       expect(subject.paged_nodes.size).to eq(3)
     end
 
+    it 'is a loaded memoized array' do
+      expect(subject.paged_nodes).to be_an(Array)
+      expect(subject.paged_nodes.object_id).to eq(subject.paged_nodes.object_id)
+    end
+
     context 'when `first` is passed' do
       let(:arguments) { { first: 2 } }
 
diff --git a/spec/requests/api/graphql/project/issues_spec.rb b/spec/requests/api/graphql/project/issues_spec.rb
index c2934430821..4f9f916f22e 100644
--- a/spec/requests/api/graphql/project/issues_spec.rb
+++ b/spec/requests/api/graphql/project/issues_spec.rb
@@ -7,8 +7,8 @@ describe 'getting an issue list for a project' do
   let(:current_user) { create(:user) }
   let(:issues_data) { graphql_data['project']['issues']['edges'] }
   let!(:issues) do
-    create(:issue, project: project, discussion_locked: true)
-    create(:issue, project: project)
+    [create(:issue, project: project, discussion_locked: true),
+     create(:issue, project: project)]
   end
   let(:fields) do
     <<~QUERY
@@ -47,6 +47,30 @@ describe 'getting an issue list for a project' do
     expect(issues_data[1]['node']['discussionLocked']).to eq true
   end
 
+  context 'when limiting the number of results' do
+    let(:query) do
+      graphql_query_for(
+        'project',
+        { 'fullPath' => project.full_path },
+        "issues(first: 1) { #{fields} }"
+      )
+    end
+
+    it_behaves_like 'a working graphql query' do
+      before do
+        post_graphql(query, current_user: current_user)
+      end
+    end
+
+    it "is expected to check permissions on the first issue only" do
+      allow(Ability).to receive(:allowed?).and_call_original
+      # Newest first, we only want to see the newest checked
+      expect(Ability).not_to receive(:allowed?).with(current_user, :read_issue, issues.first)
+
+      post_graphql(query, current_user: current_user)
+    end
+  end
+
   context 'when the user does not have access to the issue' do
     it 'returns nil' do
       project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
-- 
cgit v1.2.1