Commit eca8e6f0 authored by Bob Van Landuyt's avatar Bob Van Landuyt

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.
parent 0a99e022
...@@ -8,7 +8,7 @@ module Gitlab ...@@ -8,7 +8,7 @@ module Gitlab
extend ActiveSupport::Concern extend ActiveSupport::Concern
def self.use(schema_definition) def self.use(schema_definition)
schema_definition.instrument(:field, Instrumentation.new) schema_definition.instrument(:field, Instrumentation.new, after_built_ins: true)
end end
end end
end end
......
...@@ -15,15 +15,10 @@ module Gitlab ...@@ -15,15 +15,10 @@ module Gitlab
def authorized_resolve def authorized_resolve
proc do |parent_typed_object, args, ctx| proc do |parent_typed_object, args, ctx|
resolved_obj = @old_resolve_proc.call(parent_typed_object, args, ctx) resolved_type = @old_resolve_proc.call(parent_typed_object, args, ctx)
authorizing_obj = authorize_against(parent_typed_object) authorizing_object = authorize_against(parent_typed_object, resolved_type)
checker = build_checker(ctx[:current_user], authorizing_obj)
filter_allowed(ctx[:current_user], resolved_type, authorizing_object)
if resolved_obj.respond_to?(:then)
resolved_obj.then(&checker)
else
checker.call(resolved_obj)
end
end end
end end
...@@ -38,7 +33,7 @@ module Gitlab ...@@ -38,7 +33,7 @@ module Gitlab
type = @field.type type = @field.type
# When the return type of @field is a collection, find the singular 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) type = node_type_for_relay_connection(type)
elsif type.list? elsif type.list?
type = node_type_for_basic_connection(type) type = node_type_for_basic_connection(type)
...@@ -52,43 +47,60 @@ module Gitlab ...@@ -52,43 +47,60 @@ module Gitlab
Array.wrap(@field.metadata[:authorize]) Array.wrap(@field.metadata[:authorize])
end end
# If it's a built-in/scalar type, authorize using its parent object. def authorize_against(parent_typed_object, resolved_type)
# nil means authorize using the resolved object if built_in_type?
def authorize_against(parent_typed_object) # The field is a built-in/scalar type, or a list of scalars
parent_typed_object.object if built_in_type? && parent_typed_object.respond_to?(:object) # 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 end
def build_checker(current_user, authorizing_obj) def filter_allowed(current_user, resolved_type, authorizing_object)
lambda do |resolved_obj| if authorizing_object
# Load the elements if they were not loaded by BatchLoader yet # Authorizing fields representing scalars, or a simple field with an object
resolved_obj = resolved_obj.sync if resolved_obj.respond_to?(:sync) resolved_type if allowed_access?(current_user, authorizing_object)
elsif @field.connection?
check = lambda do |object| # A connection with pagination, modify the visible nodes in on the
authorizations.all? do |ability| # connection type in place
Ability.allowed?(current_user, ability, authorizing_obj || object) resolved_type.edge_nodes.to_a.keep_if { |node| allowed_access?(current_user, node) }
end 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 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 def allowed_access?(current_user, object)
when Array, ActiveRecord::Relation object = object.sync if object.respond_to?(:sync)
resolved_obj.select(&check)
else authorizations.all? do |ability|
resolved_obj if check.call(resolved_obj) Ability.allowed?(current_user, ability, object)
end
end end
end end
# Returns the singular type for relay connections. # Returns the singular type for relay connections.
# This will be the type class of edges.node # This will be the type class of edges.node
def node_type_for_relay_connection(type) def node_type_for_relay_connection(type)
type = type.get_field('edges').type.unwrap.get_field('node')&.type type.unwrap.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
end end
# Returns the singular type for basic connections, for example `[Types::ProjectType]` # Returns the singular type for basic connections, for example `[Types::ProjectType]`
......
...@@ -22,8 +22,17 @@ module Gitlab ...@@ -22,8 +22,17 @@ module Gitlab
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def paged_nodes 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 if first && last
raise Gitlab::Graphql::Errors::ArgumentError.new("Can only provide either `first` or `last`, not both") raise Gitlab::Graphql::Errors::ArgumentError.new("Can only provide either `first` or `last`, not both")
end end
...@@ -31,12 +40,9 @@ module Gitlab ...@@ -31,12 +40,9 @@ module Gitlab
if last if last
sliced_nodes.last(limit_value) sliced_nodes.last(limit_value)
else else
sliced_nodes.limit(limit_value) sliced_nodes.limit(limit_value) # rubocop: disable CodeReuse/ActiveRecord
end end
end end
# rubocop: enable CodeReuse/ActiveRecord
private
def before_slice def before_slice
if sort_direction == :asc if sort_direction == :asc
......
...@@ -6,7 +6,6 @@ module Gitlab ...@@ -6,7 +6,6 @@ module Gitlab
BaseError = Class.new(GraphQL::ExecutionError) BaseError = Class.new(GraphQL::ExecutionError)
ArgumentError = Class.new(BaseError) ArgumentError = Class.new(BaseError)
ResourceNotAvailable = Class.new(BaseError) ResourceNotAvailable = Class.new(BaseError)
ConnectionDefinitionError = Class.new(BaseError)
end end
end end
end end
...@@ -177,6 +177,7 @@ describe 'Gitlab::Graphql::Authorization' do ...@@ -177,6 +177,7 @@ describe 'Gitlab::Graphql::Authorization' do
describe 'type authorizations when applied to a relay connection' do describe 'type authorizations when applied to a relay connection' do
let(:query_string) { '{ object() { edges { node { name } } } }' } let(:query_string) { '{ object() { edges { node { name } } } }' }
let(:second_test_object) { double(name: 'Second thing') }
let(:type) do let(:type) do
type_factory do |type| type_factory do |type|
...@@ -186,22 +187,41 @@ describe 'Gitlab::Graphql::Authorization' do ...@@ -186,22 +187,41 @@ describe 'Gitlab::Graphql::Authorization' do
let(:query_type) do let(:query_type) do
query_factory do |query| 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
end end
subject { result.dig('object', 'edges') } 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) 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) expect(subject.first['node']).to eq('name' => test_object.name)
end end
it 'returns nil when user is not authorized' do it 'returns nil when user is not authorized' do
expect(subject).to be_empty expect(subject).to be_empty
end 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 end
describe 'type authorizations when applied to a basic connection' do describe 'type authorizations when applied to a basic connection' do
...@@ -222,28 +242,53 @@ describe 'Gitlab::Graphql::Authorization' do ...@@ -222,28 +242,53 @@ describe 'Gitlab::Graphql::Authorization' do
include_examples 'authorization with a single permission' include_examples 'authorization with a single permission'
end end
describe 'when connections do not follow the correct specification' do describe 'Authorizations on active record relations' do
let(:query_string) { '{ object() { edges { node { name }} } }' } 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 let(:issue_type) do
bad_node = type_factory do |type| type_factory do |type|
type.graphql_name 'BadNode' type.graphql_name 'FakeIssueType'
type.field :bad_node, GraphQL::STRING_TYPE, null: true type.authorize :read_issue
type.field :id, GraphQL::ID_TYPE, null: false
end end
end
let(:project_type) do |type|
type_factory 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
end end
let(:query_type) do let(:query_type) do
query_factory do |query| query_factory do |query|
query.field :object, type, null: true query.field :test_project, project_type, null: false, resolve: -> (_, _, _) { visible_project }
end end
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 result
expect { result }.to raise_error(Gitlab::Graphql::Errors::ConnectionDefinitionError)
end end
end end
...@@ -276,6 +321,8 @@ describe 'Gitlab::Graphql::Authorization' do ...@@ -276,6 +321,8 @@ describe 'Gitlab::Graphql::Authorization' do
def execute_query(query_type) def execute_query(query_type)
schema = Class.new(GraphQL::Schema) do schema = Class.new(GraphQL::Schema) do
use Gitlab::Graphql::Authorize use Gitlab::Graphql::Authorize
use Gitlab::Graphql::Connections
query(query_type) query(query_type)
end end
......
...@@ -74,6 +74,6 @@ describe GitlabSchema do ...@@ -74,6 +74,6 @@ describe GitlabSchema do
end end
def field_instrumenters def field_instrumenters
described_class.instrumenters[:field] described_class.instrumenters[:field] + described_class.instrumenters[:field_after_built_ins]
end end
end end
...@@ -5,92 +5,104 @@ require 'spec_helper' ...@@ -5,92 +5,104 @@ require 'spec_helper'
# Also see spec/graphql/features/authorization_spec.rb for # Also see spec/graphql/features/authorization_spec.rb for
# integration tests of AuthorizeFieldService # integration tests of AuthorizeFieldService
describe Gitlab::Graphql::Authorize::AuthorizeFieldService do describe Gitlab::Graphql::Authorize::AuthorizeFieldService do
describe '#build_checker' do def type(type_authorizations = [])
let(:current_user) { double(:current_user) } Class.new(Types::BaseObject) do
let(:abilities) { [double(:first_ability), double(:last_ability)] } graphql_name "TestType"
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
it 'returns a checker which checks for a single object' do authorize type_authorizations
object = double(:object) end
end
abilities.each do |ability| def type_with_field(field_type, field_authorizations = [], resolved_value = "Resolved value")
spy_ability_check_for(ability, object, passed: true) Class.new(Types::BaseObject) do
end 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) let(:current_user) { double(:current_user) }
end subject(:service) { described_class.new(field) }
it 'returns a checker which checks for all objects' do describe "#authorized_resolve" do
objects = [double(:first), double(:last)] 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| context "scalar types" do
objects.each do |object| shared_examples "checking permissions on the presented object" do
spy_ability_check_for(ability, object, passed: true) 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 end
expect(resolved).to eq("Resolved value")
end 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 end
context 'when some objects would not pass the check' do context "when the field is a scalar type" do
it 'returns nil when it is single object' do let(:field) { type_with_field(GraphQL::STRING_TYPE, :read_field).fields["testField"].to_graphql }
disallowed = double(:object) 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 context "when the field is a list of scalar types" do
end 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 it_behaves_like "checking permissions on the presented object"
allowed = double(:allowed) end
disallowed = double(:disallowed) 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| it "checks both field & type permissions" do
spy_ability_check_for(ability, allowed, passed: true) spy_ability_check_for(:read_field, object_in_field, passed: true)
end spy_ability_check_for(:read_type, object_in_field, passed: true)
expect(checker.call([disallowed, allowed])).to contain_exactly(allowed) expect(resolved).to eq(object_in_field)
end
end end
end
context 'when authorizing against another object' do it "returns nil if viewing was not allowed" do
let(:authorizing_obj) { double(:object) } 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 expect(resolved).to be_nil
service = described_class.new(double(resolve_proc: proc {}))
allow(service).to receive(:authorizations).and_return(abilities)
service.__send__(:build_checker, current_user, authorizing_obj)
end end
it 'returns a checker which checks for a single object' do context "when the field is a list" do
object = double(:object) 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| it "checks all permissions" do
spy_ability_check_for(ability, authorizing_obj, passed: true) 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 end
expect(checker.call(object)).to eq(object) it "filters out objects that the user cannot see" do
end allow(Ability).to receive(:allowed?) { true }
it 'returns a checker which checks for all objects' do spy_ability_check_for(:read_type, object_1, passed: false)
objects = [double(:first), double(:last)]
abilities.each do |ability| expect(resolved.map(&:object)).to contain_exactly(object_2)
objects.each do |object|
spy_ability_check_for(ability, authorizing_obj, passed: true)
end
end end
expect(checker.call(objects)).to eq(objects)
end end
end end
end end
......
...@@ -85,6 +85,11 @@ describe Gitlab::Graphql::Connections::KeysetConnection do ...@@ -85,6 +85,11 @@ describe Gitlab::Graphql::Connections::KeysetConnection do
expect(subject.paged_nodes.size).to eq(3) expect(subject.paged_nodes.size).to eq(3)
end 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 context 'when `first` is passed' do
let(:arguments) { { first: 2 } } let(:arguments) { { first: 2 } }
......
...@@ -7,8 +7,8 @@ describe 'getting an issue list for a project' do ...@@ -7,8 +7,8 @@ describe 'getting an issue list for a project' do
let(:current_user) { create(:user) } let(:current_user) { create(:user) }
let(:issues_data) { graphql_data['project']['issues']['edges'] } let(:issues_data) { graphql_data['project']['issues']['edges'] }
let!(:issues) do let!(:issues) do
create(:issue, project: project, discussion_locked: true) [create(:issue, project: project, discussion_locked: true),
create(:issue, project: project) create(:issue, project: project)]
end end
let(:fields) do let(:fields) do
<<~QUERY <<~QUERY
...@@ -47,6 +47,30 @@ describe 'getting an issue list for a project' do ...@@ -47,6 +47,30 @@ describe 'getting an issue list for a project' do
expect(issues_data[1]['node']['discussionLocked']).to eq true expect(issues_data[1]['node']['discussionLocked']).to eq true
end 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 context 'when the user does not have access to the issue' do
it 'returns nil' do it 'returns nil' do
project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE) project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment