Commit 4d2bce77 authored by Lin Jen-Shin's avatar Lin Jen-Shin

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
parents fe1407b1 eca8e6f0
...@@ -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)
if resolved_obj.respond_to?(:then) filter_allowed(ctx[:current_user], resolved_type, authorizing_object)
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
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 end
def build_checker(current_user, authorizing_obj) def allowed_access?(current_user, object)
lambda do |resolved_obj| object = object.sync if object.respond_to?(:sync)
# 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| authorizations.all? do |ability|
Ability.allowed?(current_user, ability, authorizing_obj || object) Ability.allowed?(current_user, ability, object)
end
end
case resolved_obj
when Array, ActiveRecord::Relation
resolved_obj.select(&check)
else
resolved_obj if check.call(resolved_obj)
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
it 'throws an error' do before do
expect { result }.to raise_error(Gitlab::Graphql::Errors::ConnectionDefinitionError) 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)
result
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,93 +5,105 @@ require 'spec_helper' ...@@ -5,93 +5,105 @@ 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 authorize type_authorizations
let(:checker) do end
service = described_class.new(double(resolve_proc: proc {}))
allow(service).to receive(:authorizations).and_return(abilities)
service.__send__(:build_checker, current_user, nil)
end end
it 'returns a checker which checks for a single object' do def type_with_field(field_type, field_authorizations = [], resolved_value = "Resolved value")
object = double(:object) Class.new(Types::BaseObject) do
graphql_name "TestTypeWithField"
field :test_field, field_type, null: true, authorize: field_authorizations, resolve: -> (_, _, _) { resolved_value}
end
end
abilities.each do |ability| let(:current_user) { double(:current_user) }
spy_ability_check_for(ability, object, passed: true) 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) }
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
expected_permissions.each do |permission|
spy_ability_check_for(permission, presented_object, passed: true)
end end
expect(checker.call(object)).to eq(object) expect(resolved).to eq("Resolved value")
end end
it 'returns a checker which checks for all objects' do it "returns nil if the value wasn't authorized" do
objects = [double(:first), double(:last)] allow(Ability).to receive(:allowed?).and_return false
abilities.each do |ability| expect(resolved).to be_nil
objects.each do |object|
spy_ability_check_for(ability, object, passed: true)
end end
end end
expect(checker.call(objects)).to eq(objects) context "when the field is a scalar type" do
end let(:field) { type_with_field(GraphQL::STRING_TYPE, :read_field).fields["testField"].to_graphql }
let(:expected_permissions) { [:read_field] }
context 'when some objects would not pass the check' do it_behaves_like "checking permissions on the presented object"
it 'returns nil when it is single object' do end
disallowed = double(:object)
spy_ability_check_for(abilities.first, disallowed, passed: false) 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] }
expect(checker.call(disallowed)).to be_nil it_behaves_like "checking permissions on the presented object"
end
end end
it 'returns only objects which passed when there are more than one' do context "when the field is a specific type" do
allowed = double(:allowed) let(:custom_type) { type(:read_type) }
disallowed = double(:disallowed) let(:object_in_field) { double("presented in field") }
let(:field) { type_with_field(custom_type, :read_field, object_in_field).fields["testField"].to_graphql }
spy_ability_check_for(abilities.first, disallowed, passed: false) 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)
abilities.each do |ability| expect(resolved).to eq(object_in_field)
spy_ability_check_for(ability, allowed, passed: true)
end end
expect(checker.call([disallowed, allowed])).to contain_exactly(allowed) it "returns nil if viewing was not allowed" do
end spy_ability_check_for(:read_field, object_in_field, passed: false)
end spy_ability_check_for(:read_type, object_in_field, passed: true)
expect(resolved).to be_nil
end end
context 'when authorizing against another object' do context "when the field is a list" do
let(:authorizing_obj) { 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 }
let(:checker) do it "checks all permissions" do
service = described_class.new(double(resolve_proc: proc {})) allow(Ability).to receive(:allowed?) { true }
allow(service).to receive(:authorizations).and_return(abilities)
service.__send__(:build_checker, current_user, authorizing_obj)
end
it 'returns a checker which checks for a single object' do spy_ability_check_for(:read_field, object_1, passed: true)
object = double(:object) 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)
abilities.each do |ability| expect(resolved).to eq(presented_types)
spy_ability_check_for(ability, authorizing_obj, passed: true)
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 end
expect(checker.call(objects)).to eq(objects)
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