diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2019-06-18 18:31:06 +0200 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2019-06-21 12:54:06 +0200 |
commit | b3ce5321895e8fbc7d1c33c57e06df33779be03f (patch) | |
tree | 30736a98df26e8334617e06b0fed38324491d5c0 | |
parent | 3c240b7aea7fee1c4267d0ceb717ba0234e5e788 (diff) | |
download | gitlab-ce-b3ce5321895e8fbc7d1c33c57e06df33779be03f.tar.gz |
Fix failing auhtorizations in GraphQL
0. Add authorize to LabelType and NamespaceType.
1. Make sure that authorizations on non-nullable fields are also
executed.
-rw-r--r-- | app/graphql/types/label_type.rb | 2 | ||||
-rw-r--r-- | app/graphql/types/metadata_type.rb | 2 | ||||
-rw-r--r-- | app/graphql/types/query_type.rb | 5 | ||||
-rw-r--r-- | app/policies/repository_policy.rb | 5 | ||||
-rw-r--r-- | changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/graphql/authorize/authorize_field_service.rb | 2 | ||||
-rw-r--r-- | spec/graphql/types/label_type_spec.rb | 6 | ||||
-rw-r--r-- | spec/graphql/types/metadata_type_spec.rb | 1 | ||||
-rw-r--r-- | spec/graphql/types/query_type_spec.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb | 70 |
10 files changed, 66 insertions, 36 deletions
diff --git a/app/graphql/types/label_type.rb b/app/graphql/types/label_type.rb index ccd466edc1a..41d035c1aab 100644 --- a/app/graphql/types/label_type.rb +++ b/app/graphql/types/label_type.rb @@ -4,6 +4,8 @@ module Types class LabelType < BaseObject graphql_name 'Label' + authorize :read_label + field :description, GraphQL::STRING_TYPE, null: true field :title, GraphQL::STRING_TYPE, null: false field :color, GraphQL::STRING_TYPE, null: false diff --git a/app/graphql/types/metadata_type.rb b/app/graphql/types/metadata_type.rb index 2d8bad0614b..7d7813a7652 100644 --- a/app/graphql/types/metadata_type.rb +++ b/app/graphql/types/metadata_type.rb @@ -4,6 +4,8 @@ module Types class MetadataType < ::Types::BaseObject graphql_name 'Metadata' + authorize :read_instance_metadata + field :version, GraphQL::STRING_TYPE, null: false field :revision, GraphQL::STRING_TYPE, null: false end diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb index 0f655ab9d03..a7627e6926e 100644 --- a/app/graphql/types/query_type.rb +++ b/app/graphql/types/query_type.rb @@ -12,10 +12,7 @@ module Types field :metadata, Types::MetadataType, null: true, resolver: Resolvers::MetadataResolver, - description: 'Metadata about GitLab' do |*args| - - authorize :read_instance_metadata - end + description: 'Metadata about GitLab' field :echo, GraphQL::STRING_TYPE, null: false, function: Functions::Echo.new end diff --git a/app/policies/repository_policy.rb b/app/policies/repository_policy.rb new file mode 100644 index 00000000000..32340749858 --- /dev/null +++ b/app/policies/repository_policy.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class RepositoryPolicy < BasePolicy + delegate { @subject.project } +end diff --git a/changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml b/changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml new file mode 100644 index 00000000000..7dedb9f6230 --- /dev/null +++ b/changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml @@ -0,0 +1,5 @@ +--- +title: Add missing authorizations in GraphQL +merge_request: +author: +type: security diff --git a/lib/gitlab/graphql/authorize/authorize_field_service.rb b/lib/gitlab/graphql/authorize/authorize_field_service.rb index 03d6aabb0e3..de9f193c576 100644 --- a/lib/gitlab/graphql/authorize/authorize_field_service.rb +++ b/lib/gitlab/graphql/authorize/authorize_field_service.rb @@ -39,6 +39,8 @@ module Gitlab type = node_type_for_basic_connection(type) end + type = type.unwrap if type.kind.non_null? + Array.wrap(type.metadata[:authorize]) end diff --git a/spec/graphql/types/label_type_spec.rb b/spec/graphql/types/label_type_spec.rb new file mode 100644 index 00000000000..da2b0b578da --- /dev/null +++ b/spec/graphql/types/label_type_spec.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe GitlabSchema.types['Label'] do + it { is_expected.to require_graphql_authorizations(:read_label) } +end diff --git a/spec/graphql/types/metadata_type_spec.rb b/spec/graphql/types/metadata_type_spec.rb index 55205bf5b6a..5236380e477 100644 --- a/spec/graphql/types/metadata_type_spec.rb +++ b/spec/graphql/types/metadata_type_spec.rb @@ -2,4 +2,5 @@ require 'spec_helper' describe GitlabSchema.types['Metadata'] do it { expect(described_class.graphql_name).to eq('Metadata') } + it { is_expected.to require_graphql_authorizations(:read_instance_metadata) } end diff --git a/spec/graphql/types/query_type_spec.rb b/spec/graphql/types/query_type_spec.rb index 69e3ea8a4a9..89d215fe066 100644 --- a/spec/graphql/types/query_type_spec.rb +++ b/spec/graphql/types/query_type_spec.rb @@ -24,9 +24,5 @@ describe GitlabSchema.types['Query'] do is_expected.to have_graphql_type(Types::MetadataType) is_expected.to have_graphql_resolver(Resolvers::MetadataResolver) end - - it 'authorizes with read_instance_metadata' do - is_expected.to require_graphql_authorizations(:read_instance_metadata) - end 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 95a4eb296fb..91be2b5dcbd 100644 --- a/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb +++ b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb @@ -7,35 +7,39 @@ require 'spec_helper' describe Gitlab::Graphql::Authorize::AuthorizeFieldService do def type(type_authorizations = []) Class.new(Types::BaseObject) do - graphql_name "TestType" + graphql_name 'TestType' authorize type_authorizations end end - def type_with_field(field_type, field_authorizations = [], resolved_value = "Resolved value") + def type_with_field(field_type, field_authorizations = [], resolved_value = 'Resolved value', **options) Class.new(Types::BaseObject) do - graphql_name "TestTypeWithField" - field :test_field, field_type, null: true, authorize: field_authorizations, resolve: -> (_, _, _) { resolved_value} + graphql_name 'TestTypeWithField' + options.reverse_merge!(null: true) + field :test_field, field_type, + authorize: field_authorizations, + resolve: -> (_, _, _) { resolved_value }, + **options end end let(:current_user) { double(:current_user) } 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) } + 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 + 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") + expect(resolved).to eq('Resolved value') end it "returns nil if the value wasn't authorized" do @@ -45,47 +49,57 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do end end - context "when the field is a scalar type" do - let(:field) { type_with_field(GraphQL::STRING_TYPE, :read_field).fields["testField"].to_graphql } + context 'when the field is a built-in scalar type' do + let(:field) { type_with_field(GraphQL::STRING_TYPE, :read_field).fields['testField'].to_graphql } let(:expected_permissions) { [:read_field] } - it_behaves_like "checking permissions on the presented object" + it_behaves_like 'checking permissions on the presented object' 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 } + 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_behaves_like "checking permissions on the presented object" + it_behaves_like 'checking permissions on the presented object' end end - context "when the field is a specific type" do + 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 } + let(:object_in_field) { double('presented in field') } + let(:field) { type_with_field(custom_type, :read_field, object_in_field).fields['testField'].to_graphql } - it "checks both field & type permissions" do + 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 eq(object_in_field) end - it "returns nil if viewing was not allowed" do + 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 be_nil 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") } + context 'when the field is not nullable' do + let(:field) { type_with_field(custom_type, [], object_in_field, null: false).fields['testField'].to_graphql } + + 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 + + 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 } + let(:field) { type_with_field([custom_type], :read_field, presented_types).fields['testField'].to_graphql } - it "checks all permissions" do + it 'checks all permissions' do allow(Ability).to receive(:allowed?) { true } spy_ability_check_for(:read_field, object_1, passed: true) @@ -96,7 +110,7 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do expect(resolved).to eq(presented_types) end - it "filters out objects that the user cannot see" do + 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) |