diff options
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/concerns/uploads_actions.rb | 6 | ||||
-rw-r--r-- | app/finders/releases_finder.rb | 23 | ||||
-rw-r--r-- | app/graphql/resolvers/releases_resolver.rb | 25 | ||||
-rw-r--r-- | app/graphql/types/ci/catalog/resource_type.rb | 5 | ||||
-rw-r--r-- | app/models/namespace.rb | 2 | ||||
-rw-r--r-- | app/models/organization.rb | 29 | ||||
-rw-r--r-- | app/models/organizations/organization.rb | 31 | ||||
-rw-r--r-- | app/uploaders/object_storage.rb | 2 | ||||
-rw-r--r-- | app/workers/all_queues.yml | 9 | ||||
-rw-r--r-- | app/workers/merge_requests/mergeability_check_batch_worker.rb | 34 |
10 files changed, 120 insertions, 46 deletions
diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index db756ae336f..0d64a685065 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -10,6 +10,10 @@ module UploadsActions included do prepend_before_action :set_request_format_from_path_extension rescue_from FileUploader::InvalidSecret, with: :render_404 + + rescue_from ::Gitlab::Utils::PathTraversalAttackError do + head :bad_request + end end def create @@ -33,6 +37,8 @@ module UploadsActions # - or redirect to its URL # def show + Gitlab::Utils.check_path_traversal!(params[:filename]) + return render_404 unless uploader&.exists? ttl, directives = *cache_settings diff --git a/app/finders/releases_finder.rb b/app/finders/releases_finder.rb index 78240e0a050..16d3f46e1af 100644 --- a/app/finders/releases_finder.rb +++ b/app/finders/releases_finder.rb @@ -6,7 +6,7 @@ class ReleasesFinder attr_reader :parent, :current_user, :params def initialize(parent, current_user = nil, params = {}) - @parent = parent + @parent = Array.wrap(parent) @current_user = current_user @params = params @@ -15,7 +15,7 @@ class ReleasesFinder end def execute(preload: true) - return Release.none if projects.empty? + return Release.none if authorized_projects.empty? releases = get_releases releases = by_tag(releases) @@ -26,16 +26,17 @@ class ReleasesFinder private def get_releases - Release.where(project_id: projects).where.not(tag: nil) # rubocop: disable CodeReuse/ActiveRecord + Release.where(project_id: authorized_projects).where.not(tag: nil) # rubocop: disable CodeReuse/ActiveRecord end - def projects - strong_memoize(:projects) do - if parent.is_a?(Project) - Ability.allowed?(current_user, :read_release, parent) ? [parent] : [] - end - end + def authorized_projects + # Preload policy for all projects to avoid N+1 queries + projects = Project.id_in(parent.map(&:id)).include_project_feature + Preloaders::ProjectPolicyPreloader.new(projects, current_user).execute + + projects.select { |project| authorized?(project) } end + strong_memoize_attr :authorized_projects # rubocop: disable CodeReuse/ActiveRecord def by_tag(releases) @@ -48,4 +49,8 @@ class ReleasesFinder def order_releases(releases) releases.sort_by_attribute("#{params[:order_by]}_#{params[:sort]}") end + + def authorized?(project) + Ability.allowed?(current_user, :read_release, project) + end end diff --git a/app/graphql/resolvers/releases_resolver.rb b/app/graphql/resolvers/releases_resolver.rb index 06f4ca2065c..8c9235c2b5f 100644 --- a/app/graphql/resolvers/releases_resolver.rb +++ b/app/graphql/resolvers/releases_resolver.rb @@ -8,8 +8,6 @@ module Resolvers required: false, default_value: :released_at_desc, description: 'Sort releases by given criteria.' - alias_method :project, :object - # This resolver has a custom singular resolver def self.single Resolvers::ReleaseResolver @@ -23,11 +21,24 @@ module Resolvers }.freeze def resolve(sort:) - ReleasesFinder.new( - project, - current_user, - SORT_TO_PARAMS_MAP[sort] - ).execute + BatchLoader::GraphQL.for(project).batch do |projects, loader| + releases = ReleasesFinder.new( + projects, + current_user, + SORT_TO_PARAMS_MAP[sort] + ).execute + + # group_by will not cause N+1 queries here because ReleasesFinder preloads projects + releases.group_by(&:project).each do |project, versions| + loader.call(project, versions) + end + end + end + + private + + def project + object.respond_to?(:project) ? object.project : object end end end diff --git a/app/graphql/types/ci/catalog/resource_type.rb b/app/graphql/types/ci/catalog/resource_type.rb index b5947826fa1..e4566aac9aa 100644 --- a/app/graphql/types/ci/catalog/resource_type.rb +++ b/app/graphql/types/ci/catalog/resource_type.rb @@ -20,6 +20,11 @@ module Types field :icon, GraphQL::Types::String, null: true, description: 'Icon for the catalog resource.', method: :avatar_path, alpha: { milestone: '15.11' } + + field :versions, Types::ReleaseType.connection_type, null: true, + description: 'Versions of the catalog resource.', + resolver: Resolvers::ReleasesResolver, + alpha: { milestone: '16.1' } end # rubocop: enable Graphql/AuthorizeTypes end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 52a6cbdafad..a36ceb77d8f 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -57,7 +57,7 @@ class Namespace < ApplicationRecord # This should _not_ be `inverse_of: :namespace`, because that would also set # `user.namespace` when this user creates a group with themselves as `owner`. belongs_to :owner, class_name: 'User' - belongs_to :organization + belongs_to :organization, class_name: 'Organizations::Organization' belongs_to :parent, class_name: "Namespace" has_many :children, -> { where(type: Group.sti_name) }, class_name: "Namespace", foreign_key: :parent_id diff --git a/app/models/organization.rb b/app/models/organization.rb deleted file mode 100644 index 0d03d95b5ff..00000000000 --- a/app/models/organization.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -class Organization < ApplicationRecord - DEFAULT_ORGANIZATION_ID = 1 - - scope :without_default, -> { where.not(id: DEFAULT_ORGANIZATION_ID) } - - before_destroy :check_if_default_organization - - has_many :namespaces - has_many :groups - - validates :name, - presence: true, - length: { maximum: 255 }, - uniqueness: { case_sensitive: false } - - def default? - id == DEFAULT_ORGANIZATION_ID - end - - private - - def check_if_default_organization - return unless default? - - raise ActiveRecord::RecordNotDestroyed, _('Cannot delete the default organization') - end -end diff --git a/app/models/organizations/organization.rb b/app/models/organizations/organization.rb new file mode 100644 index 00000000000..425869b1bb0 --- /dev/null +++ b/app/models/organizations/organization.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Organizations + class Organization < ApplicationRecord + DEFAULT_ORGANIZATION_ID = 1 + + scope :without_default, -> { where.not(id: DEFAULT_ORGANIZATION_ID) } + + before_destroy :check_if_default_organization + + has_many :namespaces + has_many :groups + + validates :name, + presence: true, + length: { maximum: 255 }, + uniqueness: { case_sensitive: false } + + def default? + id == DEFAULT_ORGANIZATION_ID + end + + private + + def check_if_default_organization + return unless default? + + raise ActiveRecord::RecordNotDestroyed, _('Cannot delete the default organization') + end + end +end diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index e1e103e20f8..70f7625ffbd 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -426,6 +426,8 @@ module ObjectStorage end def retrieve_from_store!(identifier) + Gitlab::Utils.check_path_traversal!(identifier) + # We need to force assign the value of @filename so that we will still # get the original_filename in cases wherein the file points to a random generated # path format. This happens for direct uploaded files to final location. diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 16ab9b625bf..06e07a59311 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -3027,6 +3027,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: merge_requests_mergeability_check_batch + :worker_name: MergeRequests::MergeabilityCheckBatchWorker + :feature_category: :code_review_workflow + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: merge_requests_resolve_todos :worker_name: MergeRequests::ResolveTodosWorker :feature_category: :code_review_workflow diff --git a/app/workers/merge_requests/mergeability_check_batch_worker.rb b/app/workers/merge_requests/mergeability_check_batch_worker.rb new file mode 100644 index 00000000000..cbe34ac3790 --- /dev/null +++ b/app/workers/merge_requests/mergeability_check_batch_worker.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module MergeRequests + class MergeabilityCheckBatchWorker + include ApplicationWorker + + data_consistency :sticky + + sidekiq_options retry: 3 + + feature_category :code_review_workflow + idempotent! + + def logger + @logger ||= Sidekiq.logger + end + + def perform(merge_request_ids) + merge_requests = MergeRequest.id_in(merge_request_ids) + + merge_requests.each do |merge_request| + result = merge_request.check_mergeability + + next unless result&.error? + + logger.error( + worker: self.class.name, + message: "Failed to check mergeability of merge request: #{result.message}", + merge_request_id: merge_request.id + ) + end + end + end +end |