diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2019-04-19 10:03:54 +0000 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2019-04-19 10:03:54 +0000 |
commit | 4d2bce770bdf1b12378d3c06f895cc73d46d6f9f (patch) | |
tree | 923cf5f33e3d69c57c718579f14797b4efcc98de | |
parent | fe1407b1fba62b14862613de563857f339e3c31b (diff) | |
parent | eca8e6f09b1800b58904582b527103b5c755e898 (diff) | |
download | gitlab-ce-4d2bce770bdf1b12378d3c06f895cc73d46d6f9f.tar.gz |
Merge branch 'bvl-graphql-only-authorize-rendered-fields' into 'master'
Only check abilities on rendered GraphQL nodes
Closes #58647 and #60355
See merge request gitlab-org/gitlab-ce!27273
-rw-r--r-- | lib/gitlab/graphql/authorize.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/graphql/authorize/authorize_field_service.rb | 86 | ||||
-rw-r--r-- | lib/gitlab/graphql/connections/keyset_connection.rb | 16 | ||||
-rw-r--r-- | lib/gitlab/graphql/errors.rb | 1 | ||||
-rw-r--r-- | spec/graphql/features/authorization_spec.rb | 77 | ||||
-rw-r--r-- | spec/graphql/gitlab_schema_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb | 130 | ||||
-rw-r--r-- | spec/lib/gitlab/graphql/connections/keyset_connection_spec.rb | 5 | ||||
-rw-r--r-- | 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) |