diff --git a/.gitignore b/.gitignore
index 24391f51483ad062e87fc5482dd6171de0121de4..30cb231e83f1c598336dd4cfa06f74cb3226cecb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -101,3 +101,5 @@ apollo.config.js
/tmp/matching_tests.txt
ee/changelogs/unreleased-ee
/sitespeed-result
+tags.lock
+tags.temp
diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb
index 2f5043f9ffa4f4f164ac91a1de0013e5b17afac9..b1f2ffb85035e7d65936a833d0f4c6657c898537 100644
--- a/app/graphql/gitlab_schema.rb
+++ b/app/graphql/gitlab_schema.rb
@@ -30,6 +30,8 @@ class GitlabSchema < GraphQL::Schema
default_max_page_size 100
+ lazy_resolve ::Gitlab::Graphql::Lazy, :force
+
class << self
def multiplex(queries, **kwargs)
kwargs[:max_complexity] ||= max_query_complexity(kwargs[:context])
diff --git a/bin/feature-flag b/bin/feature-flag
index 0de9b90681fcb2ce068a0080b2a945d26c4b5fc4..613ddc1d8cb663cb8112d7947a11d4f9f1cc5557 100755
--- a/bin/feature-flag
+++ b/bin/feature-flag
@@ -104,7 +104,7 @@ class FeatureFlagOptionParser
end
# Name is a first name
- options.name = argv.first
+ options.name = argv.first.downcase.gsub(/-/, '_')
options
end
diff --git a/config/feature_flags/development/graphql_lazy_authorization.yml b/config/feature_flags/development/graphql_lazy_authorization.yml
new file mode 100644
index 0000000000000000000000000000000000000000..8277c2666ea4992662c2b661dfd7b4fdd33e0af6
--- /dev/null
+++ b/config/feature_flags/development/graphql_lazy_authorization.yml
@@ -0,0 +1,7 @@
+---
+name: graphql_lazy_authorization
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45263
+rollout_issue_url:
+type: development
+group: group::plan
+default_enabled: false
diff --git a/lib/gitlab/graphql/authorize/authorize_field_service.rb b/lib/gitlab/graphql/authorize/authorize_field_service.rb
index cbf3e7b8429f4c0a811c14ad670660ff1db86fc6..e8db619f88ab995796779ec9c51f7e30885042f6 100644
--- a/lib/gitlab/graphql/authorize/authorize_field_service.rb
+++ b/lib/gitlab/graphql/authorize/authorize_field_service.rb
@@ -46,6 +46,8 @@ def type_authorizations
# Returns any authorize metadata from @field
def field_authorizations
+ return [] if @field.metadata[:authorize] == true
+
Array.wrap(@field.metadata[:authorize])
end
@@ -54,7 +56,7 @@ def authorize_against(parent_typed_object, resolved_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 @field.connection? || resolved_type.is_a?(Array)
+ elsif @field.connection? || @field.type.list? || 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
@@ -75,16 +77,25 @@ def filter_allowed(current_user, resolved_type, authorizing_object)
# no need to do anything
elsif authorizing_object
# Authorizing fields representing scalars, or a simple field with an object
- resolved_type if allowed_access?(current_user, authorizing_object)
+ ::Gitlab::Graphql::Lazy.with_value(authorizing_object) do |object|
+ resolved_type if allowed_access?(current_user, object)
+ end
elsif @field.connection?
- # A connection with pagination, modify the visible nodes on the
- # connection type in place
- resolved_type.object.edge_nodes.to_a.keep_if { |node| allowed_access?(current_user, node) }
- resolved_type
- elsif resolved_type.is_a? Array
+ ::Gitlab::Graphql::Lazy.with_value(resolved_type) do |type|
+ # A connection with pagination, modify the visible nodes on the
+ # connection type in place
+ nodes = to_nodes(type)
+ nodes.keep_if { |node| allowed_access?(current_user, node) } if nodes
+ type
+ end
+ elsif @field.type.list? || 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, realized(single_object_type).object)
+ ::Gitlab::Graphql::Lazy.with_value(resolved_type) do |items|
+ items.select do |single_object_type|
+ object_type = realized(single_object_type)
+ object = object_type.try(:object) || object_type
+ allowed_access?(current_user, object)
+ end
end
else
raise "Can't authorize #{@field}"
@@ -93,18 +104,23 @@ def filter_allowed(current_user, resolved_type, authorizing_object)
# Ensure that we are dealing with realized objects, not delayed promises
def realized(thing)
- case thing
- when BatchLoader::GraphQL
- thing.sync
- when GraphQL::Execution::Lazy
- thing.value # part of the private api, but we need to unwrap it here.
+ ::Gitlab::Graphql::Lazy.force(thing)
+ end
+
+ # Try to get the connection
+ # can be at type.object or at type
+ def to_nodes(type)
+ if type.respond_to?(:nodes)
+ type.nodes
+ elsif type.respond_to?(:object)
+ to_nodes(type.object)
else
- thing
+ nil
end
end
def allowed_access?(current_user, object)
- object = object.sync if object.respond_to?(:sync)
+ object = realized(object)
authorizations.all? do |ability|
Ability.allowed?(current_user, ability, object)
diff --git a/lib/gitlab/graphql/lazy.rb b/lib/gitlab/graphql/lazy.rb
index a7f7610a041007640d1e715806996e432099d937..b25e6b33a768b3b3ab4b93c37a89f331733a415b 100644
--- a/lib/gitlab/graphql/lazy.rb
+++ b/lib/gitlab/graphql/lazy.rb
@@ -3,17 +3,45 @@
module Gitlab
module Graphql
class Lazy
+ include Gitlab::Utils::StrongMemoize
+
+ def initialize(&block)
+ @proc = block
+ end
+
+ def force
+ strong_memoize(:force) { self.class.force(@proc.call) }
+ end
+
+ def then(&block)
+ self.class.new { yield force }
+ end
+
# Force evaluation of a (possibly) lazy value
def self.force(value)
case value
+ when ::Gitlab::Graphql::Lazy
+ value.force
when ::BatchLoader::GraphQL
value.sync
+ when ::GraphQL::Execution::Lazy
+ value.value # part of the private api, but we can force this as well
when ::Concurrent::Promise
- value.execute.value
+ value.execute if value.state == :unscheduled
+
+ value.value # value.value(10.seconds)
else
value
end
end
+
+ def self.with_value(unforced, &block)
+ if Feature.enabled?(:graphql_lazy_authorization)
+ self.new { unforced }.then(&block)
+ else
+ block.call(unforced)
+ end
+ end
end
end
end
diff --git a/spec/bin/feature_flag_spec.rb b/spec/bin/feature_flag_spec.rb
index 8e0bed5b769053a6621bb6898c767a2723b9b7ca..710b16069235e918b862069aac192cee63a8b804 100644
--- a/spec/bin/feature_flag_spec.rb
+++ b/spec/bin/feature_flag_spec.rb
@@ -13,7 +13,7 @@
let(:options) { FeatureFlagOptionParser.parse(argv) }
let(:creator) { described_class.new(options) }
let(:existing_flags) do
- { 'existing-feature-flag' => File.join('config', 'feature_flags', 'development', 'existing-feature-flag.yml') }
+ { 'existing_feature_flag' => File.join('config', 'feature_flags', 'development', 'existing_feature_flag.yml') }
end
before do
@@ -32,12 +32,12 @@
it 'properly creates a feature flag' do
expect(File).to receive(:write).with(
- File.join('config', 'feature_flags', 'development', 'feature-flag-name.yml'),
+ File.join('config', 'feature_flags', 'development', 'feature_flag_name.yml'),
anything)
expect do
subject
- end.to output(/name: feature-flag-name/).to_stdout
+ end.to output(/name: feature_flag_name/).to_stdout
end
context 'when running on master' do
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 7576523ce52e2eeb4f255d8ac500e95cfb3b2eaf..fca08ebf48b69e4b5fa0a555a90c41cf1a698e05 100644
--- a/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb
+++ b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb
@@ -27,13 +27,17 @@ def type_with_field(field_type, field_authorizations = [], resolved_value = 'Res
end
end
+ def resolve
+ service.authorized_resolve[type_instance, {}, context]
+ end
+
subject(:service) { described_class.new(field) }
describe '#authorized_resolve' do
let_it_be(:current_user) { build(:user) }
let_it_be(:presented_object) { 'presented object' }
let_it_be(:query_type) { GraphQL::ObjectType.new }
- let_it_be(:schema) { GraphQL::Schema.define(query: query_type, mutation: nil)}
+ let_it_be(:schema) { GitlabSchema }
let_it_be(:query) { GraphQL::Query.new(schema, document: nil, context: {}, variables: {}) }
let_it_be(:context) { GraphQL::Query::Context.new(query: query, values: { current_user: current_user }, object: nil) }
@@ -41,125 +45,211 @@ def type_with_field(field_type, field_authorizations = [], resolved_value = 'Res
let(:type_instance) { type_class.authorized_new(presented_object, context) }
let(:field) { type_class.fields['testField'].to_graphql }
- subject(:resolved) { service.authorized_resolve.call(type_instance, {}, context) }
+ subject(:resolved) { resolve&.force }
- 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
+ context 'reading the field of a lazy value' do
+ let(:ability) { :read_field }
+ let(:presented_object) { lazy_upcase('a') }
+ let(:type_class) { type_with_field(GraphQL::STRING_TYPE, ability) }
- expect(resolved).to eq('Resolved value')
+ let(:upcaser) do
+ Module.new do
+ def self.upcase(strs)
+ strs.map(&:upcase)
+ end
end
+ end
- it 'returns nil if the value was not authorized' do
- allow(Ability).to receive(:allowed?).and_return false
-
- expect(resolved).to be_nil
+ def lazy_upcase(str)
+ ::BatchLoader::GraphQL.for(str).batch do |strs, found|
+ strs.zip(upcaser.upcase(strs)).each { |s, us| found[s, us] }
end
end
- context 'when the field is a built-in scalar type' do
- let(:type_class) { type_with_field(GraphQL::STRING_TYPE, :read_field) }
- let(:expected_permissions) { [:read_field] }
+ it 'does not run authorizations until we force the resolved value' do
+ expect(Ability).not_to receive(:allowed?)
- it_behaves_like 'checking permissions on the presented object'
+ expect(resolve).to respond_to(:force)
end
- context 'when the field is a list of scalar types' do
- let(:type_class) { type_with_field([GraphQL::STRING_TYPE], :read_field) }
- let(:expected_permissions) { [:read_field] }
+ it 'runs authorizations when we force the resolved value' do
+ spy_ability_check_for(ability, 'A')
- it_behaves_like 'checking permissions on the presented object'
+ expect(resolved).to eq('Resolved value')
end
- context 'when the field is sub-classed scalar type' do
- let(:type_class) { type_with_field(Types::TimeType, :read_field) }
- let(:expected_permissions) { [:read_field] }
+ it 'redacts values that fail the permissions check' do
+ spy_ability_check_for(ability, 'A', passed: false)
- it_behaves_like 'checking permissions on the presented object'
+ expect(resolved).to be_nil
end
- context 'when the field is a list of sub-classed scalar types' do
- let(:type_class) { type_with_field([Types::TimeType], :read_field) }
- let(:expected_permissions) { [:read_field] }
+ context 'we batch two calls' do
+ def resolve(value)
+ instance = type_class.authorized_new(lazy_upcase(value), context)
+ service.authorized_resolve[instance, {}, context]
+ end
- it_behaves_like 'checking permissions on the presented object'
- end
- end
+ it 'batches resolution, but authorizes each object separately' do
+ expect(upcaser).to receive(:upcase).once.and_call_original
+ spy_ability_check_for(:read_field, 'A', passed: true)
+ spy_ability_check_for(:read_field, 'B', passed: false)
+ spy_ability_check_for(:read_field, 'C', passed: true)
- context 'when the field is a connection' do
- context 'when it resolves to nil' do
- let(:type_class) { type_with_field(Types::QueryType.connection_type, :read_field, nil) }
+ a = resolve('a')
+ b = resolve('b')
+ c = resolve('c')
- it 'does not fail when authorizing' do
- expect(resolved).to be_nil
+ expect(a.force).to be_present
+ expect(b.force).to be_nil
+ expect(c.force).to be_present
end
end
end
- context 'when the field is a specific type' do
- let(:custom_type) { type(:read_type) }
- let(:object_in_field) { double('presented in field') }
+ shared_examples 'authorizing fields' 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
- let(:type_class) { type_with_field(custom_type, :read_field, object_in_field) }
- let(:type_instance) { type_class.authorized_new(object_in_field, context) }
+ expect(resolved).to eq('Resolved value')
+ end
- subject(:resolved) { service.authorized_resolve.call(type_instance, {}, context) }
+ it 'returns nil if the value was not authorized' do
+ allow(Ability).to receive(:allowed?).and_return 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)
+ expect(resolved).to be_nil
+ end
+ end
- expect(resolved).to eq(object_in_field)
- end
+ context 'when the field is a built-in scalar type' do
+ let(:type_class) { type_with_field(GraphQL::STRING_TYPE, :read_field) }
+ let(:expected_permissions) { [:read_field] }
- 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)
+ it_behaves_like 'checking permissions on the presented object'
+ end
- expect(resolved).to be_nil
+ context 'when the field is a list of scalar types' do
+ let(:type_class) { type_with_field([GraphQL::STRING_TYPE], :read_field) }
+ let(:expected_permissions) { [:read_field] }
+
+ it_behaves_like 'checking permissions on the presented object'
+ end
+
+ context 'when the field is sub-classed scalar type' do
+ let(:type_class) { type_with_field(Types::TimeType, :read_field) }
+ let(:expected_permissions) { [:read_field] }
+
+ it_behaves_like 'checking permissions on the presented object'
+ end
+
+ context 'when the field is a list of sub-classed scalar types' do
+ let(:type_class) { type_with_field([Types::TimeType], :read_field) }
+ let(:expected_permissions) { [:read_field] }
+
+ it_behaves_like 'checking permissions on the presented object'
+ end
end
- context 'when the field is not nullable' do
- let(:type_class) { type_with_field(custom_type, :read_field, object_in_field, null: false) }
+ context 'when the field is a connection' do
+ context 'when it resolves to nil' do
+ let(:type_class) { type_with_field(Types::QueryType.connection_type, :read_field, nil) }
- it 'returns nil when viewing is not allowed' do
- spy_ability_check_for(:read_type, object_in_field, passed: false)
+ it 'does not fail when authorizing' do
+ expect(resolved).to be_nil
+ end
+ end
- expect(resolved).to be_nil
+ context 'when it returns values' do
+ let(:objects) { [1, 2, 3] }
+ let(:field_type) { type([:read_object]).connection_type }
+ let(:type_class) { type_with_field(field_type, [], objects) }
+
+ it 'filters out unauthorized values' do
+ spy_ability_check_for(:read_object, 1, passed: true)
+ spy_ability_check_for(:read_object, 2, passed: false)
+ spy_ability_check_for(:read_object, 3, passed: true)
+
+ expect(resolved.nodes).to eq [1, 3]
+ end
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)] }
+ context 'when the field is a specific type' do
+ let(:custom_type) { type(:read_type) }
+ let(:object_in_field) { double('presented in field') }
+
+ let(:type_class) { type_with_field(custom_type, :read_field, object_in_field) }
+ let(:type_instance) { type_class.authorized_new(object_in_field, context) }
- let(:type_class) { type_with_field([custom_type], :read_field, presented_types) }
- let(:type_instance) { type_class.authorized_new(presented_types, context) }
+ 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)
- it 'checks all permissions' do
- allow(Ability).to receive(:allowed?) { true }
+ expect(resolved).to eq(object_in_field)
+ end
- 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)
+ 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 eq(presented_types)
+ expect(resolved).to be_nil
+ end
+
+ context 'when the field is not nullable' do
+ let(:type_class) { type_with_field(custom_type, :read_field, object_in_field, null: false) }
+
+ 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
- it 'filters out objects that the user cannot see' do
- allow(Ability).to receive(:allowed?) { true }
+ 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)] }
- spy_ability_check_for(:read_type, object_1, passed: false)
+ let(:type_class) { type_with_field([custom_type], :read_field, presented_types) }
+ let(:type_instance) { type_class.authorized_new(presented_types, context) }
- expect(resolved.map(&:object)).to contain_exactly(object_2)
+ 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
+
+ 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)
+
+ expect(resolved).to contain_exactly(have_attributes(object: object_2))
+ end
end
end
end
+
+ it_behaves_like 'authorizing fields'
+
+ context 'the graphql_lazy_authorization feature flag is disabled' do
+ before do
+ stub_feature_flags(graphql_lazy_authorization: false)
+ end
+
+ subject(:resolved) { resolve }
+
+ it_behaves_like 'authorizing fields'
+ end
end
private
diff --git a/spec/lib/gitlab/graphql/lazy_spec.rb b/spec/lib/gitlab/graphql/lazy_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..795978ab0a4140768b978886a86e339b2650e683
--- /dev/null
+++ b/spec/lib/gitlab/graphql/lazy_spec.rb
@@ -0,0 +1,96 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Graphql::Lazy do
+ def load(key)
+ BatchLoader.for(key).batch do |keys, loader|
+ keys.each { |x| loader.call(x, x * x) }
+ end
+ end
+
+ let(:value) { double(x: 1) }
+
+ describe '#force' do
+ subject { described_class.new { value.x } }
+
+ it 'can extract the value' do
+ expect(subject.force).to be 1
+ end
+
+ it 'can derive new lazy values' do
+ expect(subject.then { |x| x + 2 }.force).to be 3
+ end
+
+ it 'only evaluates once' do
+ expect(value).to receive(:x).once
+
+ expect(subject.force).to eq(subject.force)
+ end
+
+ it 'deals with nested laziness' do
+ expect(described_class.new { load(10) }.force).to eq(100)
+ expect(described_class.new { described_class.new { 5 } }.force).to eq 5
+ end
+ end
+
+ describe '.with_value' do
+ let(:inner) { described_class.new { value.x } }
+
+ subject { described_class.with_value(inner) { |x| x.to_s } }
+
+ it 'defers the application of a block to a value' do
+ expect(value).not_to receive(:x)
+
+ expect(subject).to be_an_instance_of(described_class)
+ end
+
+ it 'evaluates to the application of the block to the value' do
+ expect(value).to receive(:x).once
+
+ expect(subject.force).to eq(inner.force.to_s)
+ end
+ end
+
+ describe '.force' do
+ context 'when given a plain value' do
+ subject { described_class.force(1) }
+
+ it 'unwraps the value' do
+ expect(subject).to be 1
+ end
+ end
+
+ context 'when given a wrapped lazy value' do
+ subject { described_class.force(described_class.new { 2 }) }
+
+ it 'unwraps the value' do
+ expect(subject).to be 2
+ end
+ end
+
+ context 'when the value is from a batchloader' do
+ subject { described_class.force(load(3)) }
+
+ it 'syncs the value' do
+ expect(subject).to be 9
+ end
+ end
+
+ context 'when the value is a GraphQL lazy' do
+ subject { described_class.force(GitlabSchema.after_lazy(load(3)) { |x| x + 1 } ) }
+
+ it 'forces the evaluation' do
+ expect(subject).to be 10
+ end
+ end
+
+ context 'when the value is a promise' do
+ subject { described_class.force(::Concurrent::Promise.new { 4 }) }
+
+ it 'executes the promise and waits for the value' do
+ expect(subject).to be 4
+ end
+ end
+ end
+end
diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb
index 01f8bcd242c6e66a02b8611634229a7fde27b32c..1844e12ed7aefc7f42262082bc87e55282115748 100644
--- a/spec/support/helpers/graphql_helpers.rb
+++ b/spec/support/helpers/graphql_helpers.rb
@@ -478,6 +478,8 @@ def execute_query(query_type)
use Gitlab::Graphql::Authorize
use Gitlab::Graphql::Pagination::Connections
+ lazy_resolve ::Gitlab::Graphql::Lazy, :force
+
query(query_type)
end
diff --git a/spec/support/shared_examples/graphql/label_fields.rb b/spec/support/shared_examples/graphql/label_fields.rb
index b1bfb395bc668d5ad1c14ccb3441e9f96663d7b0..caf5dae409a7361565f2169e9e42d3ca9fc1bd30 100644
--- a/spec/support/shared_examples/graphql/label_fields.rb
+++ b/spec/support/shared_examples/graphql/label_fields.rb
@@ -106,13 +106,11 @@ def query_for(*labels)
end
it 'batches queries for labels by title' do
- pending('See: https://gitlab.com/gitlab-org/gitlab/-/issues/217767')
-
multi_selection = query_for(label_b, label_c)
single_selection = query_for(label_d)
expect { run_query(multi_selection) }
- .to issue_same_number_of_queries_as { run_query(single_selection) }
+ .to issue_same_number_of_queries_as { run_query(single_selection) }.ignoring_cached_queries
end
end