diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2019-04-19 13:03:54 +0300 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2019-04-19 13:03:54 +0300 |
commit | 4d2bce770bdf1b12378d3c06f895cc73d46d6f9f (patch) | |
tree | 923cf5f33e3d69c57c718579f14797b4efcc98de /spec | |
parent | fe1407b1fba62b14862613de563857f339e3c31b (diff) | |
parent | eca8e6f09b1800b58904582b527103b5c755e898 (diff) |
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
Diffstat (limited to 'spec')
-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 |
5 files changed, 165 insertions, 77 deletions
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) |