Commit 6489bc8e authored by Bob Van Landuyt's avatar Bob Van Landuyt

Fix failing auhtorizations in GraphQL

0. Add authorize to LabelType and NamespaceType.

1. Make sure that authorizations on non-nullable fields are also
executed.
parent e3eeb779
...@@ -4,6 +4,8 @@ module Types ...@@ -4,6 +4,8 @@ module Types
class LabelType < BaseObject class LabelType < BaseObject
graphql_name 'Label' graphql_name 'Label'
authorize :read_label
field :description, GraphQL::STRING_TYPE, null: true field :description, GraphQL::STRING_TYPE, null: true
field :title, GraphQL::STRING_TYPE, null: false field :title, GraphQL::STRING_TYPE, null: false
field :color, GraphQL::STRING_TYPE, null: false field :color, GraphQL::STRING_TYPE, null: false
......
...@@ -4,6 +4,8 @@ module Types ...@@ -4,6 +4,8 @@ module Types
class MetadataType < ::Types::BaseObject class MetadataType < ::Types::BaseObject
graphql_name 'Metadata' graphql_name 'Metadata'
authorize :read_instance_metadata
field :version, GraphQL::STRING_TYPE, null: false field :version, GraphQL::STRING_TYPE, null: false
field :revision, GraphQL::STRING_TYPE, null: false field :revision, GraphQL::STRING_TYPE, null: false
end end
......
...@@ -4,6 +4,8 @@ module Types ...@@ -4,6 +4,8 @@ module Types
class NamespaceType < BaseObject class NamespaceType < BaseObject
graphql_name 'Namespace' graphql_name 'Namespace'
authorize :read_namespace
field :id, GraphQL::ID_TYPE, null: false field :id, GraphQL::ID_TYPE, null: false
field :name, GraphQL::STRING_TYPE, null: false field :name, GraphQL::STRING_TYPE, null: false
......
...@@ -66,7 +66,7 @@ module Types ...@@ -66,7 +66,7 @@ module Types
field :only_allow_merge_if_all_discussions_are_resolved, GraphQL::BOOLEAN_TYPE, null: true field :only_allow_merge_if_all_discussions_are_resolved, GraphQL::BOOLEAN_TYPE, null: true
field :printing_merge_request_link_enabled, GraphQL::BOOLEAN_TYPE, null: true field :printing_merge_request_link_enabled, GraphQL::BOOLEAN_TYPE, null: true
field :namespace, Types::NamespaceType, null: false field :namespace, Types::NamespaceType, null: true
field :group, Types::GroupType, null: true field :group, Types::GroupType, null: true
field :merge_requests, field :merge_requests,
......
...@@ -17,10 +17,7 @@ module Types ...@@ -17,10 +17,7 @@ module Types
field :metadata, Types::MetadataType, field :metadata, Types::MetadataType,
null: true, null: true,
resolver: Resolvers::MetadataResolver, resolver: Resolvers::MetadataResolver,
description: 'Metadata about GitLab' do |*args| description: 'Metadata about GitLab'
authorize :read_instance_metadata
end
field :echo, GraphQL::STRING_TYPE, null: false, function: Functions::Echo.new field :echo, GraphQL::STRING_TYPE, null: false, function: Functions::Echo.new
end end
......
# frozen_string_literal: true
class RepositoryPolicy < BasePolicy
delegate { @subject.project }
end
---
title: Add missing authorizations in GraphQL
merge_request:
author:
type: security
...@@ -39,6 +39,8 @@ module Gitlab ...@@ -39,6 +39,8 @@ module Gitlab
type = node_type_for_basic_connection(type) type = node_type_for_basic_connection(type)
end end
type = type.unwrap if type.kind.non_null?
Array.wrap(type.metadata[:authorize]) Array.wrap(type.metadata[:authorize])
end end
......
# frozen_string_literal: true
require 'spec_helper'
describe GitlabSchema.types['Label'] do
it { is_expected.to require_graphql_authorizations(:read_label) }
end
...@@ -2,4 +2,5 @@ require 'spec_helper' ...@@ -2,4 +2,5 @@ require 'spec_helper'
describe GitlabSchema.types['Metadata'] do describe GitlabSchema.types['Metadata'] do
it { expect(described_class.graphql_name).to eq('Metadata') } it { expect(described_class.graphql_name).to eq('Metadata') }
it { is_expected.to require_graphql_authorizations(:read_instance_metadata) }
end end
...@@ -4,4 +4,8 @@ require 'spec_helper' ...@@ -4,4 +4,8 @@ require 'spec_helper'
describe GitlabSchema.types['Namespace'] do describe GitlabSchema.types['Namespace'] do
it { expect(described_class.graphql_name).to eq('Namespace') } it { expect(described_class.graphql_name).to eq('Namespace') }
it { expect(described_class).to have_graphql_field(:projects) }
it { is_expected.to require_graphql_authorizations(:read_namespace) }
end end
...@@ -24,9 +24,5 @@ describe GitlabSchema.types['Query'] do ...@@ -24,9 +24,5 @@ describe GitlabSchema.types['Query'] do
is_expected.to have_graphql_type(Types::MetadataType) is_expected.to have_graphql_type(Types::MetadataType)
is_expected.to have_graphql_resolver(Resolvers::MetadataResolver) is_expected.to have_graphql_resolver(Resolvers::MetadataResolver)
end end
it 'authorizes with read_instance_metadata' do
is_expected.to require_graphql_authorizations(:read_instance_metadata)
end
end end
end end
...@@ -7,35 +7,39 @@ require 'spec_helper' ...@@ -7,35 +7,39 @@ require 'spec_helper'
describe Gitlab::Graphql::Authorize::AuthorizeFieldService do describe Gitlab::Graphql::Authorize::AuthorizeFieldService do
def type(type_authorizations = []) def type(type_authorizations = [])
Class.new(Types::BaseObject) do Class.new(Types::BaseObject) do
graphql_name "TestType" graphql_name 'TestType'
authorize type_authorizations authorize type_authorizations
end end
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 Class.new(Types::BaseObject) do
graphql_name "TestTypeWithField" graphql_name 'TestTypeWithField'
field :test_field, field_type, null: true, authorize: field_authorizations, resolve: -> (_, _, _) { resolved_value} options.reverse_merge!(null: true)
field :test_field, field_type,
authorize: field_authorizations,
resolve: -> (_, _, _) { resolved_value },
**options
end end
end end
let(:current_user) { double(:current_user) } let(:current_user) { double(:current_user) }
subject(:service) { described_class.new(field) } subject(:service) { described_class.new(field) }
describe "#authorized_resolve" do describe '#authorized_resolve' do
let(:presented_object) { double("presented object") } let(:presented_object) { double('presented object') }
let(:presented_type) { double("parent type", object: presented_object) } let(:presented_type) { double('parent type', object: presented_object) }
subject(:resolved) { service.authorized_resolve.call(presented_type, {}, { current_user: current_user }) } subject(:resolved) { service.authorized_resolve.call(presented_type, {}, { current_user: current_user }) }
context "scalar types" do context 'scalar types' do
shared_examples "checking permissions on the presented object" do shared_examples 'checking permissions on the presented object' do
it "checks the abilities on the object being presented and returns the value" do it 'checks the abilities on the object being presented and returns the value' do
expected_permissions.each do |permission| expected_permissions.each do |permission|
spy_ability_check_for(permission, presented_object, passed: true) spy_ability_check_for(permission, presented_object, passed: true)
end end
expect(resolved).to eq("Resolved value") expect(resolved).to eq('Resolved value')
end end
it "returns nil if the value wasn't authorized" do it "returns nil if the value wasn't authorized" do
...@@ -45,61 +49,71 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do ...@@ -45,61 +49,71 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do
end end
end end
context "when the field is a built-in scalar type" do 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(:field) { type_with_field(GraphQL::STRING_TYPE, :read_field).fields['testField'].to_graphql }
let(:expected_permissions) { [:read_field] } 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 list of scalar types" do 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(:field) { type_with_field([GraphQL::STRING_TYPE], :read_field).fields['testField'].to_graphql }
let(:expected_permissions) { [:read_field] } 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 sub-classed scalar type" do context 'when the field is sub-classed scalar type' do
let(:field) { type_with_field(Types::TimeType, :read_field).fields["testField"].to_graphql } let(:field) { type_with_field(Types::TimeType, :read_field).fields['testField'].to_graphql }
let(:expected_permissions) { [:read_field] } 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 list of sub-classed scalar types" do context 'when the field is a list of sub-classed scalar types' do
let(:field) { type_with_field([Types::TimeType], :read_field).fields["testField"].to_graphql } let(:field) { type_with_field([Types::TimeType], :read_field).fields['testField'].to_graphql }
let(:expected_permissions) { [:read_field] } 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
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(:custom_type) { type(:read_type) }
let(:object_in_field) { double("presented in field") } 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(: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_field, object_in_field, passed: true)
spy_ability_check_for(:read_type, object_in_field, passed: true) spy_ability_check_for(:read_type, object_in_field, passed: true)
expect(resolved).to eq(object_in_field) expect(resolved).to eq(object_in_field)
end 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_field, object_in_field, passed: false)
spy_ability_check_for(:read_type, object_in_field, passed: true) spy_ability_check_for(:read_type, object_in_field, passed: true)
expect(resolved).to be_nil expect(resolved).to be_nil
end end
context "when the field is a list" do context 'when the field is not nullable' do
let(:object_1) { double("presented in field 1") } let(:field) { type_with_field(custom_type, [], object_in_field, null: false).fields['testField'].to_graphql }
let(:object_2) { double("presented in field 2") }
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(: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 } allow(Ability).to receive(:allowed?) { true }
spy_ability_check_for(:read_field, object_1, passed: true) spy_ability_check_for(:read_field, object_1, passed: true)
...@@ -110,7 +124,7 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do ...@@ -110,7 +124,7 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do
expect(resolved).to eq(presented_types) expect(resolved).to eq(presented_types)
end 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 } allow(Ability).to receive(:allowed?) { true }
spy_ability_check_for(:read_type, object_1, passed: false) spy_ability_check_for(:read_type, object_1, passed: false)
......
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