From 66c4cd823f91a516103617ea17752548dde05823 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Fri, 21 Aug 2020 21:18:24 +1200 Subject: [PATCH 1/9] Replace Authorize instrument with gem auth This changes our GraphQL code to use the built-in `#authorize` methods to handle permissions. We originally implemented this functionality with a field-extension, but this is no longer necessary. This commit replaces that unnecessary field extension with implementations of `BaseObject#authorize` that use our policy framework. Significant changes included here: - field authorization now works as per the library specification: it authorizes against the current object, not the resolved value. To apply permissions to the resolved value, use the type permissions. - we allow resolvers to do the same (opt-in). - we extend authorization to enums (currently no enums use authorization). Note on enums: We don't actually have any authorization on enums, but we need to detect that efficiently. By supporting `ObjectAuthorization`, we can skip redaction now, and support it later (if we add enum members that require special authorization to see). Removals: - The ManualAuthorization temporary class - The synchronized_object method on BaseResolver - Field.authorize DSL method Changes: The error raised when there is no auth becomes an internal server error (ConfigurationError) since it cannot be caused by the client, and represents a programming mistake. The board issue move mutation has unnecessary logic removed, and the test for this is adjusted to verify the correctness of this change. Co-authored-by: Alex Kalderimis Co-authored-by: Charlie Ablett --- app/controllers/graphql_controller.rb | 1 - app/graphql/gitlab_schema.rb | 1 - app/graphql/mutations/base_mutation.rb | 25 +- .../boards/issues/issue_move_list.rb | 13 +- .../http_integrations_resolver.rb | 2 +- .../alert_management/integrations_resolver.rb | 2 +- app/graphql/resolvers/base_resolver.rb | 18 +- app/graphql/resolvers/board_lists_resolver.rb | 18 +- app/graphql/resolvers/board_resolver.rb | 2 +- .../concerns/manual_authorization.rb | 11 - .../group_merge_requests_resolver.rb | 2 +- .../resolvers/merge_request_resolver.rb | 2 +- .../resolvers/merge_requests_resolver.rb | 2 +- app/graphql/resolvers/milestones_resolver.rb | 2 +- .../resolvers/projects/services_resolver.rb | 12 +- .../resolvers/snippets/blobs_resolver.rb | 3 +- .../user_merge_requests_resolver_base.rb | 2 +- app/graphql/types/base_enum.rb | 12 + app/graphql/types/base_field.rb | 34 +- app/graphql/types/base_interface.rb | 6 + app/graphql/types/base_object.rb | 8 + app/graphql/types/base_union.rb | 3 + config/initializers/graphql.rb | 2 - .../board_groupings/epics_resolver.rb | 2 +- .../resolvers/dast_site_profile_resolver.rb | 2 +- .../dast_site_validation_resolver.rb | 2 +- .../oncall_schedule_resolver.rb | 2 +- .../oncall_shifts_resolver.rb | 2 +- .../resolvers/timebox_report_resolver.rb | 2 +- .../pipeline_security_report_finding_type.rb | 3 +- .../dast_site_profile_resolver_spec.rb | 18 + ee/spec/graphql/types/project_type_spec.rb | 10 +- lib/gitlab/graphql/authorize.rb | 15 - .../authorize/authorize_field_service.rb | 147 ------ .../graphql/authorize/authorize_resource.rb | 39 +- .../authorize/connection_filter_extension.rb | 63 +++ .../graphql/authorize/instrumentation.rb | 21 - .../graphql/authorize/object_authorization.rb | 31 ++ spec/graphql/features/authorization_spec.rb | 97 +++- spec/graphql/gitlab_schema_spec.rb | 4 - .../boards/issues/issue_move_list_spec.rb | 66 ++- .../resolvers/concerns/looks_ahead_spec.rb | 5 + .../resolvers/merge_requests_resolver_spec.rb | 2 +- .../prometheus_integration_type_spec.rb | 24 +- spec/graphql/types/base_object_spec.rb | 434 ++++++++++++++++++ .../authorize/authorize_field_service_spec.rb | 253 ---------- .../authorize/authorize_resource_spec.rb | 77 +--- 47 files changed, 855 insertions(+), 649 deletions(-) delete mode 100644 app/graphql/resolvers/concerns/manual_authorization.rb delete mode 100644 lib/gitlab/graphql/authorize.rb delete mode 100644 lib/gitlab/graphql/authorize/authorize_field_service.rb create mode 100644 lib/gitlab/graphql/authorize/connection_filter_extension.rb delete mode 100644 lib/gitlab/graphql/authorize/instrumentation.rb create mode 100644 lib/gitlab/graphql/authorize/object_authorization.rb create mode 100644 spec/graphql/types/base_object_spec.rb delete mode 100644 spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index f24750243fc576..82005c548f2445 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -35,7 +35,6 @@ class GraphqlController < ApplicationController def execute result = multiplex? ? execute_multiplex : execute_query - render json: result end diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index 7ab5dc36e4a446..eb083950fff7bd 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -12,7 +12,6 @@ class GitlabSchema < GraphQL::Schema use GraphQL::Pagination::Connections use BatchLoader::GraphQL - use Gitlab::Graphql::Authorize use Gitlab::Graphql::Pagination::Connections use Gitlab::Graphql::GenericTracing use Gitlab::Graphql::Timeout, max_seconds: Gitlab.config.gitlab.graphql_timeout diff --git a/app/graphql/mutations/base_mutation.rb b/app/graphql/mutations/base_mutation.rb index ac5ddc5bd4cdf5..1470d54c81d49d 100644 --- a/app/graphql/mutations/base_mutation.rb +++ b/app/graphql/mutations/base_mutation.rb @@ -2,7 +2,7 @@ module Mutations class BaseMutation < GraphQL::Schema::RelayClassicMutation - prepend Gitlab::Graphql::Authorize::AuthorizeResource + include Gitlab::Graphql::Authorize::AuthorizeResource prepend Gitlab::Graphql::CopyFieldDescription prepend ::Gitlab::Graphql::GlobalIDCompatibility @@ -29,10 +29,31 @@ def errors_on_object(record) def ready?(**args) if Gitlab::Database.read_only? - raise Gitlab::Graphql::Errors::ResourceNotAvailable, ERROR_MESSAGE + raise_resource_not_available_error! ERROR_MESSAGE else true end end + + def load_application_object(argument, lookup_as_type, id, context) + ::Gitlab::Graphql::Lazy.new { super }.catch(::GraphQL::UnauthorizedError) do |e| + Gitlab::ErrorTracking.track_exception(e) + # The default behaviour is to abort processing and return nil for the + # entire mutation field, but not set any top-level errors. We prefer to + # at least say that something went wrong. + raise_resource_not_available_error! + end + end + + def self.authorized?(object, context) + # we never provide an object to mutations, but we do need to have a user. + context[:current_user].present? && !context[:current_user].blocked? + end + + def authorized_resource?(object) + ::Gitlab::Graphql::Authorize::ObjectAuthorization + .new(self.class.authorize) + .ok?(object, current_user) + end end end diff --git a/app/graphql/mutations/boards/issues/issue_move_list.rb b/app/graphql/mutations/boards/issues/issue_move_list.rb index ce2126e26c0dd4..f32205643da325 100644 --- a/app/graphql/mutations/boards/issues/issue_move_list.rb +++ b/app/graphql/mutations/boards/issues/issue_move_list.rb @@ -52,13 +52,10 @@ def ready?(**args) super end - def resolve(board:, **args) + def resolve(board:, project_path:, iid:, **args) Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/247861') - raise_resource_not_available_error! unless board - authorize_board!(board) - - issue = authorized_find!(project_path: args[:project_path], iid: args[:iid]) + issue = authorized_find!(project_path: project_path, iid: iid) move_params = { id: issue.id, board_id: board.id }.merge(move_arguments(args)) move_issue(board, issue, move_params) @@ -84,12 +81,6 @@ def move_list_arguments(args) def move_arguments(args) args.slice(:from_list_id, :to_list_id, :move_after_id, :move_before_id) end - - def authorize_board!(board) - return if Ability.allowed?(current_user, :read_issue_board, board.resource_parent) - - raise_resource_not_available_error! - end end end end diff --git a/app/graphql/resolvers/alert_management/http_integrations_resolver.rb b/app/graphql/resolvers/alert_management/http_integrations_resolver.rb index 94a72bca7c71ff..fb6682f8d7ec76 100644 --- a/app/graphql/resolvers/alert_management/http_integrations_resolver.rb +++ b/app/graphql/resolvers/alert_management/http_integrations_resolver.rb @@ -3,7 +3,7 @@ module Resolvers module AlertManagement class HttpIntegrationsResolver < BaseResolver - alias_method :project, :synchronized_object + alias_method :project, :object type Types::AlertManagement::HttpIntegrationType.connection_type, null: true diff --git a/app/graphql/resolvers/alert_management/integrations_resolver.rb b/app/graphql/resolvers/alert_management/integrations_resolver.rb index 4d1fe367277594..e027e0412bdc03 100644 --- a/app/graphql/resolvers/alert_management/integrations_resolver.rb +++ b/app/graphql/resolvers/alert_management/integrations_resolver.rb @@ -3,7 +3,7 @@ module Resolvers module AlertManagement class IntegrationsResolver < BaseResolver - alias_method :project, :synchronized_object + alias_method :project, :object type Types::AlertManagement::IntegrationType.connection_type, null: true diff --git a/app/graphql/resolvers/base_resolver.rb b/app/graphql/resolvers/base_resolver.rb index 67bba079512cb9..7c408382cbe7c1 100644 --- a/app/graphql/resolvers/base_resolver.rb +++ b/app/graphql/resolvers/base_resolver.rb @@ -138,16 +138,6 @@ def object end end - # TODO: remove! This should never be necessary - # Remove as part of https://gitlab.com/gitlab-org/gitlab/-/issues/13984, - # since once we use that authorization approach, the object is guaranteed to - # be synchronized before any field. - def synchronized_object - strong_memoize(:synchronized_object) do - ::Gitlab::Graphql::Lazy.force(object) - end - end - def single? false end @@ -160,5 +150,13 @@ def current_user def select_result(results) results end + + def self.authorization + @authorization ||= ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(try(:required_permissions)) + end + + def self.authorized?(object, context) + authorization.ok?(object, context[:current_user]) + end end end diff --git a/app/graphql/resolvers/board_lists_resolver.rb b/app/graphql/resolvers/board_lists_resolver.rb index e66f7b97b40a8e..0b69900662661c 100644 --- a/app/graphql/resolvers/board_lists_resolver.rb +++ b/app/graphql/resolvers/board_lists_resolver.rb @@ -3,13 +3,12 @@ module Resolvers class BoardListsResolver < BaseResolver include BoardIssueFilterable - prepend ManualAuthorization include Gitlab::Graphql::Authorize::AuthorizeResource + include LooksAhead type Types::BoardListType, null: true - extras [:lookahead] - authorize :read_issue_board_list + authorizes_object! argument :id, Types::GlobalIDType[List], required: false, @@ -21,15 +20,11 @@ class BoardListsResolver < BaseResolver alias_method :board, :object - def resolve(lookahead: nil, id: nil, issue_filters: {}) - authorize!(board) - + def resolve_with_lookahead(id: nil, issue_filters: {}) lists = board_lists(id) context.scoped_set!(:issue_filters, issue_filters(issue_filters)) - if load_preferences?(lookahead) - List.preload_preferences_for_user(lists, current_user) - end + List.preload_preferences_for_user(lists, current_user) if load_preferences? offset_pagination(lists) end @@ -46,9 +41,8 @@ def board_lists(id) service.execute(board, create_default_lists: false) end - def load_preferences?(lookahead) - lookahead&.selection(:edges)&.selection(:node)&.selects?(:collapsed) || - lookahead&.selection(:nodes)&.selects?(:collapsed) + def load_preferences? + node_selection&.selects?(:collapsed) end def extract_list_id(gid) diff --git a/app/graphql/resolvers/board_resolver.rb b/app/graphql/resolvers/board_resolver.rb index 637d690e4cdead..85362ab1422e26 100644 --- a/app/graphql/resolvers/board_resolver.rb +++ b/app/graphql/resolvers/board_resolver.rb @@ -2,7 +2,7 @@ module Resolvers class BoardResolver < BaseResolver.single - alias_method :parent, :synchronized_object + alias_method :parent, :object type Types::BoardType, null: true diff --git a/app/graphql/resolvers/concerns/manual_authorization.rb b/app/graphql/resolvers/concerns/manual_authorization.rb deleted file mode 100644 index 182110b9594bf6..00000000000000 --- a/app/graphql/resolvers/concerns/manual_authorization.rb +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true - -# TODO: remove this entirely when framework authorization is released -# See: https://gitlab.com/gitlab-org/gitlab/-/issues/290216 -module ManualAuthorization - def resolve(**args) - super - rescue ::Gitlab::Graphql::Errors::ResourceNotAvailable - nil - end -end diff --git a/app/graphql/resolvers/group_merge_requests_resolver.rb b/app/graphql/resolvers/group_merge_requests_resolver.rb index 2bad974daf7687..34a4c67bc5644d 100644 --- a/app/graphql/resolvers/group_merge_requests_resolver.rb +++ b/app/graphql/resolvers/group_merge_requests_resolver.rb @@ -4,7 +4,7 @@ module Resolvers class GroupMergeRequestsResolver < MergeRequestsResolver include GroupIssuableResolver - alias_method :group, :synchronized_object + alias_method :group, :object type Types::MergeRequestType.connection_type, null: true diff --git a/app/graphql/resolvers/merge_request_resolver.rb b/app/graphql/resolvers/merge_request_resolver.rb index 8fd33c6626e529..1f7a4b48aae757 100644 --- a/app/graphql/resolvers/merge_request_resolver.rb +++ b/app/graphql/resolvers/merge_request_resolver.rb @@ -4,7 +4,7 @@ module Resolvers class MergeRequestResolver < BaseResolver.single include ResolvesMergeRequests - alias_method :project, :synchronized_object + alias_method :project, :object type ::Types::MergeRequestType, null: true diff --git a/app/graphql/resolvers/merge_requests_resolver.rb b/app/graphql/resolvers/merge_requests_resolver.rb index ecbdaaa3f5524b..5994dc449f6d8c 100644 --- a/app/graphql/resolvers/merge_requests_resolver.rb +++ b/app/graphql/resolvers/merge_requests_resolver.rb @@ -6,7 +6,7 @@ class MergeRequestsResolver < BaseResolver type ::Types::MergeRequestType.connection_type, null: true - alias_method :project, :synchronized_object + alias_method :project, :object def self.accept_assignee argument :assignee_username, GraphQL::STRING_TYPE, diff --git a/app/graphql/resolvers/milestones_resolver.rb b/app/graphql/resolvers/milestones_resolver.rb index 944b61f0c3a17b..c94e3d9e1d88a3 100644 --- a/app/graphql/resolvers/milestones_resolver.rb +++ b/app/graphql/resolvers/milestones_resolver.rb @@ -56,7 +56,7 @@ def timeframe_parameters(args) end def parent - synchronized_object + object end def parent_id_parameters(args) diff --git a/app/graphql/resolvers/projects/services_resolver.rb b/app/graphql/resolvers/projects/services_resolver.rb index f618bf2df775c9..ec31a7dbe6d8b7 100644 --- a/app/graphql/resolvers/projects/services_resolver.rb +++ b/app/graphql/resolvers/projects/services_resolver.rb @@ -3,11 +3,11 @@ module Resolvers module Projects class ServicesResolver < BaseResolver - prepend ManualAuthorization include Gitlab::Graphql::Authorize::AuthorizeResource type Types::Projects::ServiceType.connection_type, null: true authorize :admin_project + authorizes_object! argument :active, GraphQL::BOOLEAN_TYPE, @@ -20,15 +20,7 @@ class ServicesResolver < BaseResolver alias_method :project, :object - def resolve(**args) - authorize!(project) - - services(args[:active], args[:type]) - end - - private - - def services(active, type) + def resolve(active: nil, type: nil) servs = project.services servs = servs.by_active_flag(active) unless active.nil? servs = servs.by_type(type) unless type.blank? diff --git a/app/graphql/resolvers/snippets/blobs_resolver.rb b/app/graphql/resolvers/snippets/blobs_resolver.rb index 569b82149d3d47..4328d38d485924 100644 --- a/app/graphql/resolvers/snippets/blobs_resolver.rb +++ b/app/graphql/resolvers/snippets/blobs_resolver.rb @@ -3,12 +3,12 @@ module Resolvers module Snippets class BlobsResolver < BaseResolver - prepend ManualAuthorization include Gitlab::Graphql::Authorize::AuthorizeResource type Types::Snippets::BlobType.connection_type, null: true authorize :read_snippet calls_gitaly! + authorizes_object! alias_method :snippet, :object @@ -17,7 +17,6 @@ class BlobsResolver < BaseResolver description: 'Paths of the blobs.' def resolve(paths: []) - authorize!(snippet) return [snippet.blob] if snippet.empty_repo? if paths.empty? diff --git a/app/graphql/resolvers/user_merge_requests_resolver_base.rb b/app/graphql/resolvers/user_merge_requests_resolver_base.rb index 47967fe69f954e..0b39d3945f654e 100644 --- a/app/graphql/resolvers/user_merge_requests_resolver_base.rb +++ b/app/graphql/resolvers/user_merge_requests_resolver_base.rb @@ -13,7 +13,7 @@ class UserMergeRequestsResolverBase < MergeRequestsResolver description: 'The global ID of the project the authored merge requests should be in. Incompatible with projectPath.' attr_reader :project - alias_method :user, :synchronized_object + alias_method :user, :object def ready?(project_id: nil, project_path: nil, **args) return early_return unless can_read_profile? diff --git a/app/graphql/types/base_enum.rb b/app/graphql/types/base_enum.rb index 4d470aceca4ab0..527269846ffe01 100644 --- a/app/graphql/types/base_enum.rb +++ b/app/graphql/types/base_enum.rb @@ -36,6 +36,18 @@ def value(*args, **kwargs, &block) def enum @enum_values ||= {}.with_indifferent_access end + + def authorization + @authorization ||= ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(authorize) + end + + def authorize(*abilities) + @abilities = abilities + end + + def authorized?(object, context) + authorization.ok?(object, context[:current_user]) + end end end end diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index 78ab689092336a..771b6745905d51 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -2,7 +2,6 @@ module Types class BaseField < GraphQL::Schema::Field - prepend Gitlab::Graphql::Authorize include GitlabStyleDeprecations argument_class ::Types::BaseArgument @@ -13,6 +12,7 @@ def initialize(**kwargs, &block) @calls_gitaly = !!kwargs.delete(:calls_gitaly) @constant_complexity = kwargs[:complexity].is_a?(Integer) && kwargs[:complexity] > 0 @requires_argument = !!kwargs.delete(:requires_argument) + @authorize = Array.wrap(kwargs.delete(:authorize)) kwargs[:complexity] = field_complexity(kwargs[:resolver_class], kwargs[:complexity]) @feature_flag = kwargs[:feature_flag] kwargs = check_feature_flag(kwargs) @@ -22,8 +22,8 @@ def initialize(**kwargs, &block) # We want to avoid the overhead of this in prod extension ::Gitlab::Graphql::CallsGitaly::FieldExtension if Gitlab.dev_or_test_env? - extension ::Gitlab::Graphql::Present::FieldExtension + extension ::Gitlab::Graphql::Authorize::ConnectionFilterExtension end def may_call_gitaly? @@ -34,6 +34,16 @@ def requires_argument? @requires_argument || arguments.values.any? { |argument| argument.type.non_null? } end + # By default fields authorize against the current object, but that is not how our + # resolvers work - they use declarative permissions to authorize fields + # manually (so we make them opt in). + # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/300922 + # (separate out authorize into permissions on the object, and on the + # resolved values) + def authorized?(object, args, ctx) + field_authorized?(object, ctx) && resolver_authorized?(object, ctx) + end + def base_complexity complexity = DEFAULT_COMPLEXITY complexity += 1 if calls_gitaly? @@ -58,6 +68,26 @@ def visible?(context) attr_reader :feature_flag + def field_authorized?(object, ctx) + authorization.none? || authorization.ok?(object, ctx[:current_user]) + end + + # Historically our resolvers have used declarative permission checks only + # for _what they resolved_, not the _object they resolved these things from_ + # We preserve these semantics here, and only apply resolver authorization + # if the resolver has opted in. + def resolver_authorized?(object, ctx) + if @resolver_class && @resolver_class.try(:authorizes_object?) + @resolver_class.authorized?(object, ctx) + else + true + end + end + + def authorization + @authorization ||= ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(@authorize) + end + def feature_documentation_message(key, description) "#{description} Available only when feature flag `#{key}` is enabled." end diff --git a/app/graphql/types/base_interface.rb b/app/graphql/types/base_interface.rb index 4b1f3193136e0b..c21c95876becbb 100644 --- a/app/graphql/types/base_interface.rb +++ b/app/graphql/types/base_interface.rb @@ -5,5 +5,11 @@ module BaseInterface include GraphQL::Schema::Interface field_class ::Types::BaseField + + definition_methods do + def authorized?(object, context) + resolve_type(object, context).authorized?(object, context) + end + end end end diff --git a/app/graphql/types/base_object.rb b/app/graphql/types/base_object.rb index 9c36c83d4a337d..cd677e50d28b7d 100644 --- a/app/graphql/types/base_object.rb +++ b/app/graphql/types/base_object.rb @@ -19,6 +19,14 @@ def id GitlabSchema.id_from_object(object) end + def self.authorization + @authorization ||= ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(authorize) + end + + def self.authorized?(object, context) + authorization.ok?(object, context[:current_user]) + end + def current_user context[:current_user] end diff --git a/app/graphql/types/base_union.rb b/app/graphql/types/base_union.rb index 30a5668c0bb3e1..aeafbf850205d3 100644 --- a/app/graphql/types/base_union.rb +++ b/app/graphql/types/base_union.rb @@ -2,5 +2,8 @@ module Types class BaseUnion < GraphQL::Schema::Union + def self.authorized?(object, context) + resolve_type(object, context).authorized?(object, context) + end end end diff --git a/config/initializers/graphql.rb b/config/initializers/graphql.rb index f1bc289f1f0acd..52c26e756a506e 100644 --- a/config/initializers/graphql.rb +++ b/config/initializers/graphql.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true GraphQL::ObjectType.accepts_definitions(authorize: GraphQL::Define.assign_metadata_key(:authorize)) -GraphQL::Field.accepts_definitions(authorize: GraphQL::Define.assign_metadata_key(:authorize)) GraphQL::Schema::Object.accepts_definition(:authorize) -GraphQL::Schema::Field.accepts_definition(:authorize) diff --git a/ee/app/graphql/resolvers/board_groupings/epics_resolver.rb b/ee/app/graphql/resolvers/board_groupings/epics_resolver.rb index aa0f51e0490578..fe8a23d2418b37 100644 --- a/ee/app/graphql/resolvers/board_groupings/epics_resolver.rb +++ b/ee/app/graphql/resolvers/board_groupings/epics_resolver.rb @@ -5,7 +5,7 @@ module BoardGroupings class EpicsResolver < BaseResolver include ::BoardIssueFilterable - alias_method :board, :synchronized_object + alias_method :board, :object argument :issue_filters, Types::Boards::BoardIssueInputType, required: false, diff --git a/ee/app/graphql/resolvers/dast_site_profile_resolver.rb b/ee/app/graphql/resolvers/dast_site_profile_resolver.rb index 88865e6873258a..966ebd1dbbef42 100644 --- a/ee/app/graphql/resolvers/dast_site_profile_resolver.rb +++ b/ee/app/graphql/resolvers/dast_site_profile_resolver.rb @@ -2,7 +2,7 @@ module Resolvers class DastSiteProfileResolver < BaseResolver - alias_method :project, :synchronized_object + alias_method :project, :object type Types::DastSiteProfileType.connection_type, null: true diff --git a/ee/app/graphql/resolvers/dast_site_validation_resolver.rb b/ee/app/graphql/resolvers/dast_site_validation_resolver.rb index 8e9d1904359d06..451e6a144de225 100644 --- a/ee/app/graphql/resolvers/dast_site_validation_resolver.rb +++ b/ee/app/graphql/resolvers/dast_site_validation_resolver.rb @@ -2,7 +2,7 @@ module Resolvers class DastSiteValidationResolver < BaseResolver - alias_method :project, :synchronized_object + alias_method :project, :object type Types::DastSiteValidationType.connection_type, null: true diff --git a/ee/app/graphql/resolvers/incident_management/oncall_schedule_resolver.rb b/ee/app/graphql/resolvers/incident_management/oncall_schedule_resolver.rb index a36b090561c191..dfa8cc320800bd 100644 --- a/ee/app/graphql/resolvers/incident_management/oncall_schedule_resolver.rb +++ b/ee/app/graphql/resolvers/incident_management/oncall_schedule_resolver.rb @@ -3,7 +3,7 @@ module Resolvers module IncidentManagement class OncallScheduleResolver < BaseResolver - alias_method :project, :synchronized_object + alias_method :project, :object type Types::IncidentManagement::OncallScheduleType.connection_type, null: true diff --git a/ee/app/graphql/resolvers/incident_management/oncall_shifts_resolver.rb b/ee/app/graphql/resolvers/incident_management/oncall_shifts_resolver.rb index 8b4b1ed708c5b8..1bbd3cf52341de 100644 --- a/ee/app/graphql/resolvers/incident_management/oncall_shifts_resolver.rb +++ b/ee/app/graphql/resolvers/incident_management/oncall_shifts_resolver.rb @@ -3,7 +3,7 @@ module Resolvers module IncidentManagement class OncallShiftsResolver < BaseResolver - alias_method :rotation, :synchronized_object + alias_method :rotation, :object type Types::IncidentManagement::OncallShiftType.connection_type, null: true diff --git a/ee/app/graphql/resolvers/timebox_report_resolver.rb b/ee/app/graphql/resolvers/timebox_report_resolver.rb index 287e1020fc5c8c..85528121ee888b 100644 --- a/ee/app/graphql/resolvers/timebox_report_resolver.rb +++ b/ee/app/graphql/resolvers/timebox_report_resolver.rb @@ -4,7 +4,7 @@ module Resolvers class TimeboxReportResolver < BaseResolver type Types::TimeboxReportType, null: true - alias_method :timebox, :synchronized_object + alias_method :timebox, :object def resolve(*args) response = TimeboxReportService.new(timebox).execute diff --git a/ee/app/graphql/types/pipeline_security_report_finding_type.rb b/ee/app/graphql/types/pipeline_security_report_finding_type.rb index dc167e3d0ef5ec..0d2f2214d2b87e 100644 --- a/ee/app/graphql/types/pipeline_security_report_finding_type.rb +++ b/ee/app/graphql/types/pipeline_security_report_finding_type.rb @@ -32,8 +32,7 @@ class PipelineSecurityReportFindingType < BaseObject description: 'Name of the vulnerability finding.' field :project, ::Types::ProjectType, null: true, - description: 'The project on which the vulnerability finding was found.', - authorize: :read_project + description: 'The project on which the vulnerability finding was found.' field :description, GraphQL::STRING_TYPE, null: true, description: 'Description of the vulnerability finding.' diff --git a/ee/spec/graphql/resolvers/dast_site_profile_resolver_spec.rb b/ee/spec/graphql/resolvers/dast_site_profile_resolver_spec.rb index 9eb23d623f63c1..8bd854fbe96005 100644 --- a/ee/spec/graphql/resolvers/dast_site_profile_resolver_spec.rb +++ b/ee/spec/graphql/resolvers/dast_site_profile_resolver_spec.rb @@ -5,6 +5,10 @@ RSpec.describe Resolvers::DastSiteProfileResolver do include GraphqlHelpers + before do + stub_licensed_features(security_on_demand_scans: true) + end + let_it_be(:project) { create(:project) } let_it_be(:developer) { create(:user, developer_projects: [project] ) } let_it_be(:dast_site_profile1) { create(:dast_site_profile, project: project) } @@ -26,6 +30,20 @@ subject { sync(dast_site_profiles) } it { is_expected.to contain_exactly(dast_site_profile1, dast_site_profile2) } + + context 'when the feature is disabled' do + before do + stub_licensed_features(security_on_demand_scans: false) + end + + it { is_expected.to be_empty } + end + + context 'when the user does not have access' do + let(:current_user) { create(:user) } + + it { is_expected.to be_empty } + end end private diff --git a/ee/spec/graphql/types/project_type_spec.rb b/ee/spec/graphql/types/project_type_spec.rb index 9f59d93c3ecf6a..4dc111dc0a2855 100644 --- a/ee/spec/graphql/types/project_type_spec.rb +++ b/ee/spec/graphql/types/project_type_spec.rb @@ -206,14 +206,18 @@ end describe 'compliance_frameworks' do - it 'queries in batches' do + it 'queries in batches', :request_store, :use_clean_rails_memory_store_caching do projects = create_list(:project, 2, :with_compliance_framework) - projects.each { |p| p.add_maintainer(user) } + projects.each do |p| + p.add_maintainer(user) + # Cache warm up: runs authorization for each user. + resolve_field(:id, p, current_user: user) + end results = batch_sync(max_queries: 1) do projects.flat_map do |p| - resolve_field(:compliance_frameworks, p) + resolve_field(:compliance_frameworks, p, current_user: user) end end frameworks = results.flat_map(&:to_a) diff --git a/lib/gitlab/graphql/authorize.rb b/lib/gitlab/graphql/authorize.rb deleted file mode 100644 index e83b567308b9cd..00000000000000 --- a/lib/gitlab/graphql/authorize.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - # Allow fields to declare permissions their objects must have. The field - # will be set to nil unless all required permissions are present. - module Authorize - extend ActiveSupport::Concern - - def self.use(schema_definition) - schema_definition.instrument(:field, Gitlab::Graphql::Authorize::Instrumentation.new, after_built_ins: true) - end - end - end -end diff --git a/lib/gitlab/graphql/authorize/authorize_field_service.rb b/lib/gitlab/graphql/authorize/authorize_field_service.rb deleted file mode 100644 index e8db619f88ab99..00000000000000 --- a/lib/gitlab/graphql/authorize/authorize_field_service.rb +++ /dev/null @@ -1,147 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module Authorize - class AuthorizeFieldService - def initialize(field) - @field = field - @old_resolve_proc = @field.resolve_proc - end - - def authorizations? - authorizations.present? - end - - def authorized_resolve - proc do |parent_typed_object, args, ctx| - resolved_type = @old_resolve_proc.call(parent_typed_object, args, ctx) - authorizing_object = authorize_against(parent_typed_object, resolved_type) - - filter_allowed(ctx[:current_user], resolved_type, authorizing_object) - end - end - - private - - def authorizations - @authorizations ||= (type_authorizations + field_authorizations).uniq - end - - # Returns any authorize metadata from the return type of @field - def type_authorizations - type = @field.type - - # When the return type of @field is a collection, find the singular type - if @field.connection? - type = node_type_for_relay_connection(type) - elsif type.list? - type = node_type_for_basic_connection(type) - end - - type = type.unwrap if type.kind.non_null? - - Array.wrap(type.metadata[:authorize]) - end - - # Returns any authorize metadata from @field - def field_authorizations - return [] if @field.metadata[:authorize] == true - - Array.wrap(@field.metadata[:authorize]) - end - - def authorize_against(parent_typed_object, resolved_type) - if scalar_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? || @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 - 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 - 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 resolved_type.nil? - # We're not rendering anything, for example when a record was not found - # no need to do anything - elsif authorizing_object - # Authorizing fields representing scalars, or a simple field with an object - ::Gitlab::Graphql::Lazy.with_value(authorizing_object) do |object| - resolved_type if allowed_access?(current_user, object) - end - elsif @field.connection? - ::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 - ::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}" - end - end - - # Ensure that we are dealing with realized objects, not delayed promises - def realized(thing) - ::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 - nil - end - end - - def allowed_access?(current_user, object) - object = realized(object) - - authorizations.all? do |ability| - Ability.allowed?(current_user, ability, object) - end - end - - # Returns the singular type for relay connections. - # This will be the type class of edges.node - def node_type_for_relay_connection(type) - type.unwrap.get_field('edges').type.unwrap.get_field('node').type - end - - # Returns the singular type for basic connections, for example `[Types::ProjectType]` - def node_type_for_basic_connection(type) - type.unwrap - end - - def scalar_type? - node_type_for_basic_connection(@field.type).kind.scalar? - end - end - end - end -end diff --git a/lib/gitlab/graphql/authorize/authorize_resource.rb b/lib/gitlab/graphql/authorize/authorize_resource.rb index 6ee446011d4dda..dbf3c6f8bd5edc 100644 --- a/lib/gitlab/graphql/authorize/authorize_resource.rb +++ b/lib/gitlab/graphql/authorize/authorize_resource.rb @@ -5,6 +5,7 @@ module Graphql module Authorize module AuthorizeResource extend ActiveSupport::Concern + ConfigurationError = Class.new(StandardError) RESOURCE_ACCESS_ERROR = "The resource that you are attempting to access does not exist or you don't have permission to perform this action" @@ -12,7 +13,7 @@ module AuthorizeResource def required_permissions # If the `#authorize` call is used on multiple classes, we add the # permissions specified on a subclass, to the ones that were specified - # on it's superclass. + # on its superclass. @required_permissions ||= if self.respond_to?(:superclass) && superclass.respond_to?(:required_permissions) superclass.required_permissions.dup else @@ -23,6 +24,18 @@ def required_permissions def authorize(*permissions) required_permissions.concat(permissions) end + + def authorizes_object? + defined?(@authorizes_object) ? @authorizes_object : false + end + + def authorizes_object! + @authorizes_object = true + end + + def raise_resource_not_available_error!(msg = RESOURCE_ACCESS_ERROR) + raise ::Gitlab::Graphql::Errors::ResourceNotAvailable, msg + end end def find_object(*args) @@ -37,33 +50,23 @@ def authorized_find!(*args, **kwargs) object end + # authorizes the object using the current class authorization. def authorize!(object) - unless authorized_resource?(object) - raise_resource_not_available_error! - end + raise_resource_not_available_error! unless authorized_resource?(object) end - # this was named `#authorized?`, however it conflicts with the native - # graphql gem version - # TODO consider adopting the gem's built in authorization system - # https://gitlab.com/gitlab-org/gitlab/issues/13984 def authorized_resource?(object) # Sanity check. We don't want to accidentally allow a developer to authorize # without first adding permissions to authorize against - if self.class.required_permissions.empty? - raise Gitlab::Graphql::Errors::ArgumentError, "#{self.class.name} has no authorizations" + if self.class.authorization.none? + raise ConfigurationError, "#{self.class.name} has no authorizations" end - self.class.required_permissions.all? do |ability| - # The actions could be performed across multiple objects. In which - # case the current user is common, and we could benefit from the - # caching in `DeclarativePolicy`. - Ability.allowed?(current_user, ability, object, scope: :user) - end + self.class.authorization.ok?(object, current_user) end - def raise_resource_not_available_error!(msg = RESOURCE_ACCESS_ERROR) - raise Gitlab::Graphql::Errors::ResourceNotAvailable, msg + def raise_resource_not_available_error!(*args) + self.class.raise_resource_not_available_error!(*args) end end end diff --git a/lib/gitlab/graphql/authorize/connection_filter_extension.rb b/lib/gitlab/graphql/authorize/connection_filter_extension.rb new file mode 100644 index 00000000000000..20526e19c2a507 --- /dev/null +++ b/lib/gitlab/graphql/authorize/connection_filter_extension.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Authorize + class ConnectionFilterExtension < GraphQL::Schema::FieldExtension + class Redactor + include ::Gitlab::Graphql::Laziness + + def initialize(type, context) + @type = type + @context = context + end + + def redact(nodes) + remove_unauthorized(nodes) + + nodes + end + + def active? + # some scalar types (such as integers) do not respond to :authorized? + return false unless @type.respond_to?(:authorized?) + + auth = @type.try(:authorization) + + auth.nil? || auth.any? + end + + private + + def remove_unauthorized(nodes) + nodes + .map! { |lazy| force(lazy) } + .keep_if { |forced| @type.authorized?(forced, @context) } + end + end + + def after_resolve(value:, context:, **rest) + if @field.connection? + redact_connection(value, context) + elsif @field.type.list? + redact_list(value.to_a, context) unless value.nil? + end + + value + end + + def redact_connection(conn, context) + redactor = Redactor.new(@field.type.unwrap.node_type, context) + return unless redactor.active? + + conn.redactor = redactor if conn.respond_to?(:redactor=) + end + + def redact_list(list, context) + redactor = Redactor.new(@field.type.unwrap, context) + redactor.redact(list) if redactor.active? + end + end + end + end +end diff --git a/lib/gitlab/graphql/authorize/instrumentation.rb b/lib/gitlab/graphql/authorize/instrumentation.rb deleted file mode 100644 index 15ecc3b04f00ae..00000000000000 --- a/lib/gitlab/graphql/authorize/instrumentation.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module Authorize - class Instrumentation - # Replace the resolver for the field with one that will only return the - # resolved object if the permissions check is successful. - def instrument(_type, field) - service = AuthorizeFieldService.new(field) - - if service.authorizations? - field.redefine { resolve(service.authorized_resolve) } - else - field - end - end - end - end - end -end diff --git a/lib/gitlab/graphql/authorize/object_authorization.rb b/lib/gitlab/graphql/authorize/object_authorization.rb new file mode 100644 index 00000000000000..5f6c266fd61015 --- /dev/null +++ b/lib/gitlab/graphql/authorize/object_authorization.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Authorize + class ObjectAuthorization + attr_reader :abilities + + def initialize(abilities) + @abilities = Array.wrap(abilities).flatten + end + + def none? + abilities.empty? + end + + def any? + abilities.present? + end + + def ok?(object, current_user) + return true if none? + + abilities.all? do |ability| + Ability.allowed?(current_user, ability, object) + end + end + end + end + end +end diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb index 33b11e1ca0928c..31a45cfca0bd6a 100644 --- a/spec/graphql/features/authorization_spec.rb +++ b/spec/graphql/features/authorization_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe 'Gitlab::Graphql::Authorize' do +RSpec.describe 'DeclarativePolicy authorization in GraphQL ' do include GraphqlHelpers include Graphql::ResolverFactories @@ -10,10 +10,14 @@ let(:permission_single) { :foo } let(:permission_collection) { [:foo, :bar] } let(:test_object) { double(name: 'My name') } + let(:authorizing_object) { test_object } + # to override when combining permissions + let(:permission_object_one) { authorizing_object } + let(:permission_object_two) { authorizing_object } + let(:query_string) { '{ item { name } }' } let(:result) do schema = empty_schema - schema.use(Gitlab::Graphql::Authorize) execute_query(query_type, schema: schema) end @@ -33,18 +37,25 @@ shared_examples 'authorization with a collection of permissions' do it 'returns the protected field when user has all permissions' do - permit(*permission_collection) + permit_on(permission_object_one, permission_collection.first) + permit_on(permission_object_two, permission_collection.second) expect(subject).to eq('name' => test_object.name) end it 'returns nil when user only has one of the permissions' do - permit(permission_collection.first) + permit_on(permission_object_one, permission_collection.first) + + expect(subject).to be_nil + end + + it 'returns nil when user only has the other of the permissions' do + permit_on(permission_object_two, permission_collection.second) expect(subject).to be_nil end - it 'returns nil when user only has none of the permissions' do + it 'returns nil when user has neither of the required permissions' do expect(subject).to be_nil end end @@ -56,6 +67,7 @@ describe 'Field authorizations' do let(:type) { type_factory } + let(:authorizing_object) { nil } describe 'with a single permission' do let(:query_type) do @@ -71,9 +83,10 @@ let(:query_type) do permissions = permission_collection query_factory do |qt| - qt.field :item, type, null: true, resolver: new_resolver(test_object) do - authorize permissions - end + qt.field :item, type, + null: true, + resolver: new_resolver(test_object), + authorize: permissions end end @@ -110,9 +123,8 @@ let(:type) do permissions = permission_collection type_factory do |type| - type.field :name, GraphQL::STRING_TYPE, null: true do - authorize permissions - end + type.field :name, GraphQL::STRING_TYPE, null: true, + authorize: permissions end end @@ -163,6 +175,7 @@ end describe 'type and field authorizations together' do + let(:authorizing_object) { anything } let(:permission_1) { permission_collection.first } let(:permission_2) { permission_collection.last } @@ -181,7 +194,61 @@ include_examples 'authorization with a collection of permissions' end - describe 'type authorizations when applied to a relay connection' do + describe 'resolver and field authorizations together' do + let(:permission_1) { permission_collection.first } + let(:permission_2) { permission_collection.last } + let(:type) { type_factory } + + let(:query_type) do + query_factory do |query| + query.field :item, type, null: true, + resolver: resolver, + authorize: permission_2 + end + end + + context 'the resolver authorizes the object' do + let(:permission_object_one) { be_nil } + let(:permission_object_two) { be_nil } + let(:resolver) do + resolver = simple_resolver(test_object) + resolver.include(::Gitlab::Graphql::Authorize::AuthorizeResource) + resolver.authorize permission_1 + resolver.authorizes_object! + resolver + end + + include_examples 'authorization with a collection of permissions' + end + + context 'when the resolver does not authorize the object' do + let(:permission_object_one) { test_object } + let(:permission_object_two) { be_nil } + let(:resolver) do + resolver = new_resolver(test_object, method: :find_object) + resolver.authorize permission_1 + resolver + end + + include_examples 'authorization with a collection of permissions' + end + + context 'when the resolver does not authorize the object, and does not list any permissions' do + let(:permission_object_two) { be_nil } + let(:resolver) do + resolver = new_resolver(test_object, method: :find_object) + resolver + end + + it 'raises a configuration error' do + permit_on(permission_object_two, permission_collection.second) + + expect { execute_query(query_type) }.to raise_error(::Gitlab::Graphql::Authorize::AuthorizeResource::ConfigurationError) + end + end + end + + describe 'when type authorizations when applied to a relay connection' do let(:query_string) { '{ item { edges { node { name } } } }' } let(:second_test_object) { double(name: 'Second thing') } @@ -303,8 +370,12 @@ private def permit(*permissions) + permit_on(authorizing_object, *permissions) + end + + def permit_on(object, *permissions) permissions.each do |permission| - allow(Ability).to receive(:allowed?).with(user, permission, test_object).and_return(true) + allow(Ability).to receive(:allowed?).with(user, permission, object).and_return(true) end end end diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index cb2bb25b098c22..4ddd74b1eb7a07 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -14,10 +14,6 @@ expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::GenericTracing)) end - it 'enables the authorization instrumenter' do - expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::Authorize::Instrumentation)) - end - it 'has the base mutation' do expect(described_class.mutation).to eq(::Types::MutationType) end diff --git a/spec/graphql/mutations/boards/issues/issue_move_list_spec.rb b/spec/graphql/mutations/boards/issues/issue_move_list_spec.rb index 24104a20465b4d..dd9305d21975ff 100644 --- a/spec/graphql/mutations/boards/issues/issue_move_list_spec.rb +++ b/spec/graphql/mutations/boards/issues/issue_move_list_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Mutations::Boards::Issues::IssueMoveList do + include GraphqlHelpers + let_it_be(:group) { create(:group, :public) } let_it_be(:project) { create(:project, group: group) } let_it_be(:board) { create(:board, group: group) } @@ -16,9 +18,8 @@ let_it_be(:existing_issue1) { create(:labeled_issue, project: project, labels: [testing], relative_position: 10) } let_it_be(:existing_issue2) { create(:labeled_issue, project: project, labels: [testing], relative_position: 50) } - let(:current_user) { user } - let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) } - let(:params) { { board: board, project_path: project.full_path, iid: issue1.iid } } + let(:current_ctx) { { current_user: user } } + let(:params) { { board_id: global_id_of(board), project_path: project.full_path, iid: issue1.iid } } let(:move_params) do { from_list_id: list1.id, @@ -33,26 +34,45 @@ group.add_guest(guest) end - subject do - mutation.resolve(**params.merge(move_params)) - end + describe '#resolve' do + subject do + sync(resolve(described_class, args: params.merge(move_params), ctx: current_ctx)) + end + + %i[from_list_id to_list_id].each do |arg_name| + context "when we only pass #{arg_name}" do + let(:move_params) { { arg_name => list1.id } } - describe '#ready?' do - it 'raises an error if required arguments are missing' do - expect { mutation.ready?(**params) } - .to raise_error(Gitlab::Graphql::Errors::ArgumentError, "At least one of the arguments " \ - "fromListId, toListId, afterId or beforeId is required") + it 'raises an error' do + expect { subject }.to raise_error( + Gitlab::Graphql::Errors::ArgumentError, + 'Both fromListId and toListId must be present' + ) + end + end end - it 'raises an error if only one of fromListId and toListId is present' do - expect { mutation.ready?(**params.merge(from_list_id: list1.id)) } - .to raise_error(Gitlab::Graphql::Errors::ArgumentError, - 'Both fromListId and toListId must be present' + context 'when required arguments are missing' do + let(:move_params) { {} } + + it 'raises an error' do + expect { subject }.to raise_error( + Gitlab::Graphql::Errors::ArgumentError, + "At least one of the arguments fromListId, toListId, afterId or beforeId is required" ) + end + end + + context 'when the board ID is wrong' do + before do + params[:board_id] = global_id_of(project) + end + + it 'raises an error' do + expect { subject }.to raise_error(::GraphQL::LoadApplicationObjectFailedError) + end end - end - describe '#resolve' do context 'when user have access to resources' do it 'moves and repositions issue' do subject @@ -63,15 +83,11 @@ end end - context 'when user have no access to resources' do - shared_examples 'raises a resource not available error' do - it { expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) } - end - - context 'when user cannot update issue' do - let(:current_user) { guest } + context 'when user cannot update issue' do + let(:current_ctx) { { current_user: guest } } - it_behaves_like 'raises a resource not available error' + specify do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) end end end diff --git a/spec/graphql/resolvers/concerns/looks_ahead_spec.rb b/spec/graphql/resolvers/concerns/looks_ahead_spec.rb index 27ac1572cab715..72efa2b3e24b73 100644 --- a/spec/graphql/resolvers/concerns/looks_ahead_spec.rb +++ b/spec/graphql/resolvers/concerns/looks_ahead_spec.rb @@ -24,6 +24,11 @@ def resolve_with_lookahead(**args) def preloads { labels: [:labels] } end + + # fake auth, no permissions required to do anything but still needs to be declared + def self.authorize + [] + end end label = Class.new(GraphQL::Schema::Object) do diff --git a/spec/graphql/resolvers/merge_requests_resolver_spec.rb b/spec/graphql/resolvers/merge_requests_resolver_spec.rb index 7dd968d90a8278..5e2a075c5b399a 100644 --- a/spec/graphql/resolvers/merge_requests_resolver_spec.rb +++ b/spec/graphql/resolvers/merge_requests_resolver_spec.rb @@ -41,7 +41,7 @@ # AND "merge_requests"."iid" = 1 ORDER BY "merge_requests"."id" DESC # SELECT "projects".* FROM "projects" WHERE "projects"."id" = 2 # SELECT "project_features".* FROM "project_features" WHERE "project_features"."project_id" = 2 - let(:queries_per_project) { 3 } + let(:queries_per_project) { 4 } context 'no arguments' do it 'returns all merge requests' do diff --git a/spec/graphql/types/alert_management/prometheus_integration_type_spec.rb b/spec/graphql/types/alert_management/prometheus_integration_type_spec.rb index b10c2a2ab2acf1..7e4110af9a41ca 100644 --- a/spec/graphql/types/alert_management/prometheus_integration_type_spec.rb +++ b/spec/graphql/types/alert_management/prometheus_integration_type_spec.rb @@ -48,15 +48,21 @@ end end - context 'without project' do - let_it_be(:integration) { create(:prometheus_service, project: nil, group: create(:group)) } - - it_behaves_like 'has field with value', 'token' do - let(:value) { nil } - end - - it_behaves_like 'has field with value', 'url' do - let(:value) { nil } + context 'group integration' do + let_it_be(:group) { create(:group) } + let_it_be(:integration) { create(:prometheus_service, project: nil, group: group) } + + # Since it is impossible to authorize the parent here, given that the + # project is nil, all fields should be redacted: + + described_class.fields.keys.each do |field_name| + context "field: #{field_name}" do + it 'is redacted' do + expect do + resolve_field(field_name, integration, current_user: user) + end.to raise_error(GraphqlHelpers::UnauthorizedObject) + end + end end end end diff --git a/spec/graphql/types/base_object_spec.rb b/spec/graphql/types/base_object_spec.rb new file mode 100644 index 00000000000000..d144c1f6456351 --- /dev/null +++ b/spec/graphql/types/base_object_spec.rb @@ -0,0 +1,434 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Types::BaseObject do + include GraphqlHelpers + + describe 'scoping items' do + let_it_be(:custom_auth) do + Class.new(::Gitlab::Graphql::Authorize::ObjectAuthorization) do + def any? + true + end + + def ok?(object, _current_user) + return false if object == { id: 100 } + return false if object.try(:deactivated?) + + true + end + end + end + + let_it_be(:test_schema) do + auth = custom_auth.new(nil) + + base_object = Class.new(described_class) do + # Override authorization so we don't need to mock Ability + define_singleton_method :authorization do + auth + end + end + + y_type = Class.new(base_object) do + graphql_name 'Y' + authorize :read_y + field :id, Integer, null: false + + def id + object[:id] + end + end + + number_type = Module.new do + include ::Types::BaseInterface + + graphql_name 'Number' + + field :value, Integer, null: false + end + + odd_type = Class.new(described_class) do + graphql_name 'Odd' + implements number_type + + authorize :read_odd + field :odd_value, Integer, null: false + + def odd_value + object[:value] + end + end + + even_type = Class.new(described_class) do + graphql_name 'Even' + implements number_type + + authorize :read_even + field :even_value, Integer, null: false + + def even_value + object[:value] + end + end + + # an abstract type, delegating authorization to members + odd_or_even = Class.new(::Types::BaseUnion) do + graphql_name 'OddOrEven' + + possible_types odd_type, even_type + + define_singleton_method :resolve_type do |object, ctx| + if object[:value].odd? + odd_type + else + even_type + end + end + end + + number_type.define_singleton_method :resolve_type do |object, ctx| + odd_or_even.resolve_type(object, ctx) + end + + x_type = Class.new(base_object) do + graphql_name 'X' + # Scalar types + field :title, String, null: true + # monomorphic types + field :lazy_list_of_ys, [y_type], null: true + field :list_of_lazy_ys, [y_type], null: true + field :array_ys_conn, y_type.connection_type, null: true + # polymorphic types + field :polymorphic_conn, odd_or_even.connection_type, null: true + field :polymorphic_object, odd_or_even, null: true do + argument :value, Integer, required: true + end + field :interface_conn, number_type.connection_type, null: true + + def lazy_list_of_ys + ::Gitlab::Graphql::Lazy.new { object[:ys] } + end + + def list_of_lazy_ys + object[:ys].map { |y| ::Gitlab::Graphql::Lazy.new { y } } + end + + def array_ys_conn + object[:ys].dup + end + + def polymorphic_conn + object[:values].dup + end + alias_method :interface_conn, :polymorphic_conn + + def polymorphic_object(value) + value + end + end + + user_type = Class.new(base_object) do + graphql_name 'User' + authorize :read_user + field 'name', String, null: true + end + + Class.new(GraphQL::Schema) do + lazy_resolve ::Gitlab::Graphql::Lazy, :force + use ::GraphQL::Pagination::Connections + use ::Gitlab::Graphql::Pagination::Connections + + query(Class.new(::Types::BaseObject) do + graphql_name 'Query' + field :x, x_type, null: true + field :users, user_type.connection_type, null: true + + def x + ::Gitlab::Graphql::Lazy.new { context[:x] } + end + + def users + ::Gitlab::Graphql::Lazy.new { User.id_in(context[:user_ids]).order(id: :asc) } + end + end) + + def unauthorized_object(err) + nil + end + end + end + + def document(path) + GraphQL.parse(<<~GQL) + query { + x { + title + #{query_graphql_path(path, 'id')} + } + } + GQL + end + + let(:data) do + { + x: { + title: 'Hey', + ys: [{ id: 1 }, { id: 100 }, { id: 2 }] + } + } + end + + shared_examples 'array member redaction' do |path| + let(:result) do + query = GraphQL::Query.new(test_schema, document: document(path), context: data) + query.result.to_h + end + + it 'redacts the unauthorized array member' do + expect(graphql_dig_at(result, 'data', 'x', 'title')).to eq('Hey') + expect(graphql_dig_at(result, 'data', 'x', *path)).to contain_exactly( + eq({ 'id' => 1 }), + eq({ 'id' => 2 }) + ) + end + end + + # For example a batchloaded association + context 'a lazy list' do + it_behaves_like 'array member redaction', %w[lazyListOfYs] + end + + # For example using a batchloader to map over a set of IDs + context 'a list of lazy items' do + it_behaves_like 'array member redaction', %w[listOfLazyYs] + end + + context 'an array connection of items' do + it_behaves_like 'array member redaction', %w[arrayYsConn nodes] + end + + context 'an array connection of items, selecting edges' do + it_behaves_like 'array member redaction', %w[arrayYsConn edges node] + end + + it 'paginates arrays correctly' do + n = 7 + + data = { + x: { + ys: (95..105).to_a.map { |id| { id: id } } + } + } + + doc = ->(after) do + GraphQL.parse(<<~GQL) + query { + x { + ys: arrayYsConn(#{attributes_to_graphql(first: n, after: after)}) { + pageInfo { + hasNextPage + hasPreviousPage + endCursor + } + nodes { id } + } + } + } + GQL + end + returned_items = ->(ids) do + ids.to_a.map { |id| eq({ 'id' => id }) } + end + + query = GraphQL::Query.new(test_schema, document: doc[nil], context: data) + result = query.result.to_h + + ys = result.dig('data', 'x', 'ys', 'nodes') + page = result.dig('data', 'x', 'ys', 'pageInfo') + # We expect this page to be smaller, since we paginate before redaction + expect(ys).to match_array(returned_items[(95..101).to_a - [100]]) + expect(page).to include('hasNextPage' => true, 'hasPreviousPage' => false) + + cursor = page['endCursor'] + query_2 = GraphQL::Query.new(test_schema, document: doc[cursor], context: data) + result_2 = query_2.result.to_h + + ys = result_2.dig('data', 'x', 'ys', 'nodes') + page = result_2.dig('data', 'x', 'ys', 'pageInfo') + expect(ys).to match_array(returned_items[102..105]) + expect(page).to include('hasNextPage' => false, 'hasPreviousPage' => true) + end + + it 'filters connections correctly' do + active_users = create_list(:user, 3, state: :active) + inactive = create(:user, state: :deactivated) + + data = { user_ids: [inactive, *active_users].map(&:id) } + + doc = GraphQL.parse(<<~GQL) + query { + users { nodes { name } } + } + GQL + + query = GraphQL::Query.new(test_schema, document: doc, context: data) + result = query.result.to_h + + expect(result.dig('data', 'users', 'nodes')).to match_array(active_users.map do |u| + eq({ 'name' => u.name }) + end) + end + + it 'filters polymorphic connections' do + data = { + current_user: :the_user, + x: { + values: [{ value: 1 }, { value: 2 }, { value: 3 }, { value: 4 }] + } + } + + doc = GraphQL.parse(<<~GQL) + query { + x { + things: polymorphicConn { + nodes { + ... on Odd { oddValue } + ... on Even { evenValue } + } + } + } + } + GQL + + # Each ability check happens twice: once in the collection, and once + # on the type. We expect the ability checks to be cached. + expect(Ability).to receive(:allowed?).twice + .with(:the_user, :read_odd, { value: 1 }).and_return(true) + expect(Ability).to receive(:allowed?).once + .with(:the_user, :read_odd, { value: 3 }).and_return(false) + expect(Ability).to receive(:allowed?).once + .with(:the_user, :read_even, { value: 2 }).and_return(false) + expect(Ability).to receive(:allowed?).twice + .with(:the_user, :read_even, { value: 4 }).and_return(true) + + query = GraphQL::Query.new(test_schema, document: doc, context: data) + result = query.result.to_h + + things = result.dig('data', 'x', 'things', 'nodes') + + expect(things).to contain_exactly( + { 'oddValue' => 1 }, + { 'evenValue' => 4 } + ) + end + + it 'filters interface connections' do + data = { + current_user: :the_user, + x: { + values: [{ value: 1 }, { value: 2 }, { value: 3 }, { value: 4 }] + } + } + + doc = GraphQL.parse(<<~GQL) + query { + x { + things: interfaceConn { + nodes { + value + ... on Odd { oddValue } + ... on Even { evenValue } + } + } + } + } + GQL + + # Each ability check happens twice: once in the collection, and once + # on the type. We expect the ability checks to be cached. + expect(Ability).to receive(:allowed?).twice + .with(:the_user, :read_odd, { value: 1 }).and_return(true) + expect(Ability).to receive(:allowed?).once + .with(:the_user, :read_odd, { value: 3 }).and_return(false) + expect(Ability).to receive(:allowed?).once + .with(:the_user, :read_even, { value: 2 }).and_return(false) + expect(Ability).to receive(:allowed?).twice + .with(:the_user, :read_even, { value: 4 }).and_return(true) + + query = GraphQL::Query.new(test_schema, document: doc, context: data) + result = query.result.to_h + + things = result.dig('data', 'x', 'things', 'nodes') + + expect(things).to contain_exactly( + { 'value' => 1, 'oddValue' => 1 }, + { 'value' => 4, 'evenValue' => 4 } + ) + end + + it 'redacts polymorphic objects' do + data = { + current_user: :the_user, + x: { + values: [{ value: 1 }] + } + } + + doc = GraphQL.parse(<<~GQL) + query { + x { + ok: polymorphicObject(value: 1) { + ... on Odd { oddValue } + ... on Even { evenValue } + } + bad: polymorphicObject(value: 3) { + ... on Odd { oddValue } + ... on Even { evenValue } + } + } + } + GQL + + # Each ability check happens twice: once in the collection, and once + # on the type. We expect the ability checks to be cached. + expect(Ability).to receive(:allowed?).once + .with(:the_user, :read_odd, { value: 1 }).and_return(true) + expect(Ability).to receive(:allowed?).once + .with(:the_user, :read_odd, { value: 3 }).and_return(false) + + query = GraphQL::Query.new(test_schema, document: doc, context: data) + result = query.result.to_h + + expect(result.dig('data', 'x', 'ok')).to eq({ 'oddValue' => 1 }) + expect(result.dig('data', 'x', 'bad')).to be_nil + end + + it 'paginates before scoping' do + # Inactive first so they sort first + n = 3 + inactive = create_list(:user, n - 1, state: :deactivated) + active_users = create_list(:user, 2, state: :active) + + data = { user_ids: [*inactive, *active_users].map(&:id) } + + doc = GraphQL.parse(<<~GQL) + query { + users(first: #{n}) { + pageInfo { hasNextPage } + nodes { name } } + } + GQL + + query = GraphQL::Query.new(test_schema, document: doc, context: data) + result = query.result.to_h + + # We expect the page to be loaded and then filtered - i.e. to have all + # deactivated users removed. + expect(result.dig('data', 'users', 'pageInfo', 'hasNextPage')).to be_truthy + expect(result.dig('data', 'users', 'nodes')) + .to contain_exactly({ 'name' => active_users.first.name }) + 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 deleted file mode 100644 index c88506899cd6ed..00000000000000 --- a/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb +++ /dev/null @@ -1,253 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -# Also see spec/graphql/features/authorization_spec.rb for -# integration tests of AuthorizeFieldService -RSpec.describe Gitlab::Graphql::Authorize::AuthorizeFieldService do - def type(type_authorizations = []) - Class.new(Types::BaseObject) do - graphql_name 'TestType' - - authorize type_authorizations - end - end - - def type_with_field(field_type, field_authorizations = [], resolved_value = 'Resolved value', **options) - Class.new(Types::BaseObject) do - graphql_name 'TestTypeWithField' - options.reverse_merge!(null: true) - field :test_field, field_type, - authorize: field_authorizations, - **options - - define_method :test_field do - resolved_value - end - 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) { 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) } - - let(:type_class) { type_with_field(custom_type, :read_field, presented_object) } - let(:type_instance) { type_class.authorized_new(presented_object, context) } - let(:field) { type_class.fields['testField'].to_graphql } - - subject(:resolved) { ::Gitlab::Graphql::Lazy.force(resolve) } - - 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) } - - let(:upcaser) do - Module.new do - def self.upcase(strs) - strs.map(&:upcase) - end - end - end - - 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 - - it 'does not run authorizations until we force the resolved value' do - expect(Ability).not_to receive(:allowed?) - - expect(resolve).to respond_to(:force) - end - - it 'runs authorizations when we force the resolved value' do - spy_ability_check_for(ability, 'A') - - expect(resolved).to eq('Resolved value') - end - - it 'redacts values that fail the permissions check' do - spy_ability_check_for(ability, 'A', passed: false) - - expect(resolved).to be_nil - end - - 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 '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) - - a = resolve('a') - b = resolve('b') - c = resolve('c') - - expect(a.force).to be_present - expect(b.force).to be_nil - expect(c.force).to be_present - end - end - end - - 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 - - expect(resolved).to eq('Resolved value') - end - - it 'returns nil if the value was not authorized' do - allow(Ability).to receive(:allowed?).and_return false - - expect(resolved).to be_nil - 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_behaves_like 'checking permissions on the presented object' - 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_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 a connection' do - context 'when it resolves to nil' do - let(:type_class) { type_with_field(Types::QueryType.connection_type, :read_field, nil) } - - it 'does not fail when authorizing' do - expect(resolved).to be_nil - end - end - - 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 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) } - - 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 - 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 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 - - 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(:type_class) { type_with_field([custom_type], :read_field, presented_types) } - let(:type_instance) { type_class.authorized_new(presented_types, context) } - - 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' - end - - private - - def spy_ability_check_for(ability, object, passed: true) - expect(Ability) - .to receive(:allowed?) - .with(current_user, ability, object) - .and_return(passed) - end -end diff --git a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb index c5d7665c3b2374..fb48d8200574ce 100644 --- a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb +++ b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb @@ -22,6 +22,14 @@ def find_object def current_user user end + + def context + { current_user: user } + end + + def self.authorization + @authorization ||= ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(required_permissions) + end end end @@ -30,9 +38,16 @@ def current_user subject(:loading_resource) { fake_class.new(user, project) } + before do + # don't allow anything by default + allow(Ability).to receive(:allowed?) do + false + end + end + context 'when the user is allowed to perform the action' do before do - allow(Ability).to receive(:allowed?).with(user, :read_the_thing, project, scope: :user) do + allow(Ability).to receive(:allowed?).with(user, :read_the_thing, project) do true end end @@ -48,24 +63,12 @@ def current_user expect { loading_resource.authorize!(project) }.not_to raise_error end end - - describe '#authorized_resource?' do - it 'is true' do - expect(loading_resource.authorized_resource?(project)).to be(true) - end - end end context 'when the user is not allowed to perform the action' do - before do - allow(Ability).to receive(:allowed?).with(user, :read_the_thing, project, scope: :user) do - false - end - end - describe '#authorized_find!' do it 'raises an error' do - expect { loading_resource.authorize!(project) }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + expect { loading_resource.authorized_find! }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) end end @@ -74,12 +77,6 @@ def current_user expect { loading_resource.authorize!(project) }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) end end - - describe '#authorized_resource?' do - it 'is false' do - expect(loading_resource.authorized_resource?(project)).to be(false) - end - end end context 'when the class does not define #find_object' do @@ -92,46 +89,6 @@ def current_user end end - context 'when the class does not define authorize' do - let(:fake_class) do - Class.new do - include Gitlab::Graphql::Authorize::AuthorizeResource - - attr_reader :user, :found_object - - def initialize(user, found_object) - @user, @found_object = user, found_object - end - - def find_object(*_args) - found_object - end - - def current_user - user - end - - def self.name - 'TestClass' - end - end - end - - let(:error) { /#{fake_class.name} has no authorizations/ } - - describe '#authorized_find!' do - it 'raises a comprehensive error message' do - expect { loading_resource.authorized_find! }.to raise_error(error) - end - end - - describe '#authorized_resource?' do - it 'raises a comprehensive error message' do - expect { loading_resource.authorized_resource?(project) }.to raise_error(error) - end - end - end - describe '#authorize' do it 'adds permissions from subclasses to those of superclasses when used on classes' do base_class = Class.new do -- GitLab From 7eac6d2dec275d009e5c1dfe09e59cae0a90ecaa Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Fri, 26 Feb 2021 20:44:49 +0000 Subject: [PATCH 2/9] Explicitly disable design management This was failing in local tests due to LFS being enabled by default. Disabling this explicitly makes this test more robust. Incidental rubocop fixes. --- .../graphql/mutations/design_management/upload_spec.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/spec/graphql/mutations/design_management/upload_spec.rb b/spec/graphql/mutations/design_management/upload_spec.rb index 326d88cea8095d..e92000194b1a80 100644 --- a/spec/graphql/mutations/design_management/upload_spec.rb +++ b/spec/graphql/mutations/design_management/upload_spec.rb @@ -32,6 +32,10 @@ def run_mutation(files_to_upload = files, project_path = project.full_path, iid end context "when the feature is not available" do + before do + enable_design_management(false) + end + it_behaves_like "resource not available" end @@ -99,20 +103,20 @@ def creates_designs it_behaves_like "resource not available" end - context "a valid design" do + context "with a valid design" do it "returns the updated designs" do expect(resolve[:errors]).to eq [] expect(resolve[:designs].map(&:filename)).to contain_exactly("dk.png") end end - context "context when passing an invalid project" do + context "when passing an invalid project" do let(:project) { build(:project) } it_behaves_like "resource not available" end - context "context when passing an invalid issue" do + context "when passing an invalid issue" do let(:issue) { build(:issue) } it_behaves_like "resource not available" -- GitLab From 75a5faf631c44e1b8af4a8b723e9e4029797688e Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 15 Mar 2021 10:59:03 +0000 Subject: [PATCH 3/9] Add specs for more failure modes This tests that what happens if the boardId is: - not for a board - not for a board the user can read (even though they have all other permissions) --- .../boards/issues/issue_move_list_spec.rb | 37 +++++++++++++++++-- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/spec/requests/api/graphql/mutations/boards/issues/issue_move_list_spec.rb b/spec/requests/api/graphql/mutations/boards/issues/issue_move_list_spec.rb index e24ab0b07f29ae..46ec22e7ef8440 100644 --- a/spec/requests/api/graphql/mutations/boards/issues/issue_move_list_spec.rb +++ b/spec/requests/api/graphql/mutations/boards/issues/issue_move_list_spec.rb @@ -21,7 +21,8 @@ let(:mutation_name) { mutation_class.graphql_name } let(:mutation_result_identifier) { mutation_name.camelize(:lower) } let(:current_user) { user } - let(:params) { { board_id: board.to_global_id.to_s, project_path: project.full_path, iid: issue1.iid.to_s } } + let(:board_id) { global_id_of(board) } + let(:params) { { board_id: board_id, project_path: project.full_path, iid: issue1.iid.to_s } } let(:issue_move_params) do { from_list_id: list1.id, @@ -34,16 +35,44 @@ end shared_examples 'returns an error' do - it 'fails with error' do - message = "The resource that you are attempting to access does not exist or you don't have "\ - "permission to perform this action" + let(:message) do + "The resource that you are attempting to access does not exist or you don't have " \ + "permission to perform this action" + end + it 'fails with error' do post_graphql_mutation(mutation(params), current_user: current_user) expect(graphql_errors).to include(a_hash_including('message' => message)) end end + context 'when the board_id is not a board' do + let(:board_id) { global_id_of(project) } + let(:issue_move_params) do + { move_after_id: existing_issue1.id, move_before_id: existing_issue2.id } + end + + it_behaves_like 'returns an error' do + let(:message) { include('does not represent an instance of') } + end + end + + # This test aims to distinguish between the failures to authorize + # :read_issue_board and :update_issue + context 'when the user cannot read the issue board' do + let(:issue_move_params) do + { move_after_id: existing_issue1.id, move_before_id: existing_issue2.id } + end + + before do + allow(Ability).to receive(:allowed?).with(any_args).and_return(true) + allow(Ability).to receive(:allowed?).with(current_user, :read_issue_board, board).and_return(false) + end + + it_behaves_like 'returns an error' + end + context 'when user has access to resources' do context 'when repositioning an issue' do let(:issue_move_params) { { move_after_id: existing_issue1.id, move_before_id: existing_issue2.id } } -- GitLab From 3544607b10b593d12f652102babfae61a3bf23bf Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 15 Mar 2021 13:13:25 +0000 Subject: [PATCH 4/9] Clarify spec example descriptions --- spec/graphql/features/authorization_spec.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb index 31a45cfca0bd6a..7ab5adadd1f05b 100644 --- a/spec/graphql/features/authorization_spec.rb +++ b/spec/graphql/features/authorization_spec.rb @@ -207,7 +207,7 @@ end end - context 'the resolver authorizes the object' do + context 'when the resolver authorizes the object' do let(:permission_object_one) { be_nil } let(:permission_object_two) { be_nil } let(:resolver) do @@ -221,7 +221,7 @@ include_examples 'authorization with a collection of permissions' end - context 'when the resolver does not authorize the object' do + context 'when the resolver does not authorize the object, but instead calls authorized_find!' do let(:permission_object_one) { test_object } let(:permission_object_two) { be_nil } let(:resolver) do @@ -233,7 +233,7 @@ include_examples 'authorization with a collection of permissions' end - context 'when the resolver does not authorize the object, and does not list any permissions' do + context 'when the resolver calls authorized_find!, but does not list any permissions' do let(:permission_object_two) { be_nil } let(:resolver) do resolver = new_resolver(test_object, method: :find_object) @@ -243,7 +243,8 @@ it 'raises a configuration error' do permit_on(permission_object_two, permission_collection.second) - expect { execute_query(query_type) }.to raise_error(::Gitlab::Graphql::Authorize::AuthorizeResource::ConfigurationError) + expect { execute_query(query_type) } + .to raise_error(::Gitlab::Graphql::Authorize::AuthorizeResource::ConfigurationError) end end end -- GitLab From 93bb2047da978422b5d2fff5665b33f39e02662d Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 15 Mar 2021 13:18:40 +0000 Subject: [PATCH 5/9] Checking none? is redundant --- app/graphql/types/base_field.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index 771b6745905d51..d5443aa045e1d9 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -69,7 +69,7 @@ def visible?(context) attr_reader :feature_flag def field_authorized?(object, ctx) - authorization.none? || authorization.ok?(object, ctx[:current_user]) + authorization.ok?(object, ctx[:current_user]) end # Historically our resolvers have used declarative permission checks only -- GitLab From 801495444f5e6c1fbd56821c2fb3056d1da6c951 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 15 Mar 2021 13:25:04 +0000 Subject: [PATCH 6/9] Ensure Mutations enjoy ConfigurationError sanity check --- app/graphql/mutations/base_mutation.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/graphql/mutations/base_mutation.rb b/app/graphql/mutations/base_mutation.rb index 1470d54c81d49d..ec0f8b54789fb3 100644 --- a/app/graphql/mutations/base_mutation.rb +++ b/app/graphql/mutations/base_mutation.rb @@ -50,10 +50,9 @@ def self.authorized?(object, context) context[:current_user].present? && !context[:current_user].blocked? end - def authorized_resource?(object) - ::Gitlab::Graphql::Authorize::ObjectAuthorization - .new(self.class.authorize) - .ok?(object, current_user) + # See: AuthorizeResource#authorized_resource? + def self.authorization + @authorization ||= ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(authorize) end end end -- GitLab From ce2bf047b7241a04d7f80fc03afc784280fcc618 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 15 Mar 2021 13:27:02 +0000 Subject: [PATCH 7/9] Fix lint errors --- lib/gitlab/graphql/authorize/authorize_resource.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/graphql/authorize/authorize_resource.rb b/lib/gitlab/graphql/authorize/authorize_resource.rb index dbf3c6f8bd5edc..4d575b964e501c 100644 --- a/lib/gitlab/graphql/authorize/authorize_resource.rb +++ b/lib/gitlab/graphql/authorize/authorize_resource.rb @@ -7,14 +7,15 @@ module AuthorizeResource extend ActiveSupport::Concern ConfigurationError = Class.new(StandardError) - RESOURCE_ACCESS_ERROR = "The resource that you are attempting to access does not exist or you don't have permission to perform this action" + RESOURCE_ACCESS_ERROR = "The resource that you are attempting to access does " \ + "not exist or you don't have permission to perform this action" class_methods do def required_permissions # If the `#authorize` call is used on multiple classes, we add the # permissions specified on a subclass, to the ones that were specified # on its superclass. - @required_permissions ||= if self.respond_to?(:superclass) && superclass.respond_to?(:required_permissions) + @required_permissions ||= if respond_to?(:superclass) && superclass.respond_to?(:required_permissions) superclass.required_permissions.dup else [] @@ -58,9 +59,7 @@ def authorize!(object) def authorized_resource?(object) # Sanity check. We don't want to accidentally allow a developer to authorize # without first adding permissions to authorize against - if self.class.authorization.none? - raise ConfigurationError, "#{self.class.name} has no authorizations" - end + raise ConfigurationError, "#{self.class.name} has no authorizations" if self.class.authorization.none? self.class.authorization.ok?(object, current_user) end -- GitLab From 47b6de8dbd10c0ac1d512b34b4d299fcb2b199fd Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 15 Mar 2021 13:33:09 +0000 Subject: [PATCH 8/9] Remove unnecessary authorize definition --- .../graphql/resolvers/concerns/looks_ahead_spec.rb | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/spec/graphql/resolvers/concerns/looks_ahead_spec.rb b/spec/graphql/resolvers/concerns/looks_ahead_spec.rb index 72efa2b3e24b73..4c244da5c621be 100644 --- a/spec/graphql/resolvers/concerns/looks_ahead_spec.rb +++ b/spec/graphql/resolvers/concerns/looks_ahead_spec.rb @@ -24,11 +24,6 @@ def resolve_with_lookahead(**args) def preloads { labels: [:labels] } end - - # fake auth, no permissions required to do anything but still needs to be declared - def self.authorize - [] - end end label = Class.new(GraphQL::Schema::Object) do @@ -43,11 +38,8 @@ def self.authorize user = Class.new(GraphQL::Schema::Object) do graphql_name 'User' field :name, String, null: true - field :issues, issue.connection_type, - null: true - field :issues_with_lookahead, issue.connection_type, - resolver: issues_resolver, - null: true + field :issues, issue.connection_type, null: true + field :issues_with_lookahead, issue.connection_type, resolver: issues_resolver, null: true end Class.new(GraphQL::Schema) do @@ -106,7 +98,7 @@ def run_query(gql_query) expect(res['errors']).to be_blank expect(res.dig('data', 'findUser', 'name')).to eq(the_user.name) - %w(issues issuesWithLookahead).each do |field| + %w[issues issuesWithLookahead].each do |field| expect(all_issue_titles(res, field)).to match_array(issue_titles) expect(all_label_ids(res, field)).to match_array(expected_label_ids) end -- GitLab From 7c5ad6d612b70a35a42630c10cdf9365f52eac89 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 15 Mar 2021 13:38:30 +0000 Subject: [PATCH 9/9] Note that we do not support argument authorization --- app/graphql/types/base_field.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index d5443aa045e1d9..99f328949251b8 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -40,6 +40,9 @@ def requires_argument? # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/300922 # (separate out authorize into permissions on the object, and on the # resolved values) + # We do not support argument authorization in our schema. If/when we do, + # we should call `super` here, to apply argument authorization checks. + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/324647 def authorized?(object, args, ctx) field_authorized?(object, ctx) && resolver_authorized?(object, ctx) end -- GitLab