diff options
| author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-02-03 11:35:56 +0000 |
|---|---|---|
| committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-02-03 11:35:56 +0000 |
| commit | 33bbb6aa7b6369fea0037f3d8a9243824e48f64f (patch) | |
| tree | 18ae1428e70ddcfe1115f355ebdad6ad6f0a6e56 | |
| parent | 41fd6d4d38aaef723e501ff3ab38ae63e31d4efb (diff) | |
| download | gitlab-ce-33bbb6aa7b6369fea0037f3d8a9243824e48f64f.tar.gz | |
Add latest changes from gitlab-org/security/gitlab@14-7-stable-ee
75 files changed, 950 insertions, 272 deletions
diff --git a/app/assets/javascripts/create_item_dropdown.js b/app/assets/javascripts/create_item_dropdown.js index 1472adf458b..b39720c6094 100644 --- a/app/assets/javascripts/create_item_dropdown.js +++ b/app/assets/javascripts/create_item_dropdown.js @@ -1,4 +1,3 @@ -import { escape } from 'lodash'; import initDeprecatedJQueryDropdown from '~/deprecated_jquery_dropdown'; export default class CreateItemDropdown { @@ -37,14 +36,14 @@ export default class CreateItemDropdown { }, selectable: true, toggleLabel(selected) { - return selected && 'id' in selected ? escape(selected.title) : this.defaultToggleLabel; + return selected && 'id' in selected ? selected.title : this.defaultToggleLabel; }, fieldName: this.fieldName, text(item) { - return escape(item.text); + return item.text; }, id(item) { - return escape(item.id); + return item.id; }, onFilter: this.toggleCreateNewButton.bind(this), clicked: (options) => { diff --git a/app/controllers/jira_connect/users_controller.rb b/app/controllers/jira_connect/users_controller.rb index 569dc42fed3..a37c68de299 100644 --- a/app/controllers/jira_connect/users_controller.rb +++ b/app/controllers/jira_connect/users_controller.rb @@ -5,7 +5,17 @@ class JiraConnect::UsersController < ApplicationController layout 'signup_onboarding' + before_action :verify_return_to_url, only: [:show] + def show @jira_app_link = params.delete(:return_to) end + + private + + def verify_return_to_url + return unless params[:return_to].present? + + params.delete(:return_to) unless Integrations::Jira.valid_jira_cloud_url?(params[:return_to]) + end end diff --git a/app/graphql/mutations/packages/destroy.rb b/app/graphql/mutations/packages/destroy.rb index 979a54da6bd..81fa53fc116 100644 --- a/app/graphql/mutations/packages/destroy.rb +++ b/app/graphql/mutations/packages/destroy.rb @@ -15,7 +15,7 @@ module Mutations def resolve(id:) package = authorized_find!(id: id) - result = ::Packages::DestroyPackageService.new(container: package, current_user: current_user).execute + result = ::Packages::MarkPackageForDestructionService.new(container: package, current_user: current_user).execute errors = result.error? ? Array.wrap(result[:message]) : [] diff --git a/app/graphql/mutations/packages/destroy_file.rb b/app/graphql/mutations/packages/destroy_file.rb index 35a486666d5..4aa33b3504c 100644 --- a/app/graphql/mutations/packages/destroy_file.rb +++ b/app/graphql/mutations/packages/destroy_file.rb @@ -15,7 +15,7 @@ module Mutations def resolve(id:) package_file = authorized_find!(id: id) - if package_file.destroy + if package_file.update(status: :pending_destruction) return { errors: [] } end diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index f4067552f55..d49244f6b65 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -438,6 +438,12 @@ module Types ::Security::CiConfiguration::SastParserService.new(object).configuration end + def service_desk_address + return unless Ability.allowed?(current_user, :admin_issue, project) + + object.service_desk_address + end + def tag_list object.topic_list end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index b69c0199c70..3c9f7c4dd7f 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -10,6 +10,7 @@ class ApplicationSetting < ApplicationRecord ignore_columns %i[elasticsearch_shards elasticsearch_replicas], remove_with: '14.4', remove_after: '2021-09-22' ignore_columns %i[static_objects_external_storage_auth_token], remove_with: '14.9', remove_after: '2022-03-22' + ignore_column %i[max_package_files_for_package_destruction], remove_with: '14.9', remove_after: '2022-03-22' INSTANCE_REVIEW_MIN_USERS = 50 GRAFANA_URL_ERROR_MESSAGE = 'Please check your Grafana URL setting in ' \ diff --git a/app/models/concerns/integrations/enable_ssl_verification.rb b/app/models/concerns/integrations/enable_ssl_verification.rb new file mode 100644 index 00000000000..11dc8a76a2b --- /dev/null +++ b/app/models/concerns/integrations/enable_ssl_verification.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Integrations + module EnableSslVerification + extend ActiveSupport::Concern + + prepended do + boolean_accessor :enable_ssl_verification + end + + def initialize_properties + super + + self.enable_ssl_verification = true if new_record? && enable_ssl_verification.nil? + end + + def fields + super.tap do |fields| + url_index = fields.index { |field| field[:name].ends_with?('_url') } + insert_index = url_index ? url_index + 1 : -1 + + fields.insert(insert_index, { + type: 'checkbox', + name: 'enable_ssl_verification', + title: s_('Integrations|SSL verification'), + checkbox_label: s_('Integrations|Enable SSL verification'), + help: s_('Integrations|Clear if using a self-signed certificate.') + }) + end + end + end +end diff --git a/app/models/concerns/integrations/has_web_hook.rb b/app/models/concerns/integrations/has_web_hook.rb index dabe7152b18..bc28c32695c 100644 --- a/app/models/concerns/integrations/has_web_hook.rb +++ b/app/models/concerns/integrations/has_web_hook.rb @@ -15,7 +15,11 @@ module Integrations # Return whether the webhook should use SSL verification. def hook_ssl_verification - true + if respond_to?(:enable_ssl_verification) + enable_ssl_verification + else + true + end end # Create or update the webhook, raising an exception if it cannot be saved. diff --git a/app/models/integrations/bamboo.rb b/app/models/integrations/bamboo.rb index 0774b84b69f..57767c63cf4 100644 --- a/app/models/integrations/bamboo.rb +++ b/app/models/integrations/bamboo.rb @@ -4,6 +4,7 @@ module Integrations class Bamboo < BaseCi include ActionView::Helpers::UrlHelper include ReactivelyCached + prepend EnableSslVerification prop_accessor :bamboo_url, :build_key, :username, :password @@ -162,7 +163,7 @@ module Integrations end def build_get_params(query_params) - params = { verify: false, query: query_params } + params = { verify: enable_ssl_verification, query: query_params } return params if username.blank? && password.blank? query_params[:os_authType] = 'basic' diff --git a/app/models/integrations/buildkite.rb b/app/models/integrations/buildkite.rb index 9fad3a42647..90593d78a5d 100644 --- a/app/models/integrations/buildkite.rb +++ b/app/models/integrations/buildkite.rb @@ -137,7 +137,7 @@ module Integrations end def request_options - { verify: false, extra_log_info: { project_id: project_id } } + { extra_log_info: { project_id: project_id } } end end end diff --git a/app/models/integrations/drone_ci.rb b/app/models/integrations/drone_ci.rb index 856d14c022d..3c18e5d8732 100644 --- a/app/models/integrations/drone_ci.rb +++ b/app/models/integrations/drone_ci.rb @@ -5,10 +5,12 @@ module Integrations include HasWebHook include PushDataValidations include ReactivelyCached + prepend EnableSslVerification extend Gitlab::Utils::Override + DRONE_SAAS_HOSTNAME = 'cloud.drone.io' + prop_accessor :drone_url, :token - boolean_accessor :enable_ssl_verification validates :drone_url, presence: true, public_url: true, if: :activated? validates :token, presence: true, if: :activated? @@ -95,8 +97,7 @@ module Integrations def fields [ { type: 'text', name: 'token', help: s_('ProjectService|Token for the Drone project.'), required: true }, - { type: 'text', name: 'drone_url', title: s_('ProjectService|Drone server URL'), placeholder: 'http://drone.example.com', required: true }, - { type: 'checkbox', name: 'enable_ssl_verification', title: "Enable SSL verification" } + { type: 'text', name: 'drone_url', title: s_('ProjectService|Drone server URL'), placeholder: 'http://drone.example.com', required: true } ] end @@ -105,15 +106,24 @@ module Integrations [drone_url, "/hook", "?owner=#{project.namespace.full_path}", "&name=#{project.path}", "&access_token=#{token}"].join end - override :hook_ssl_verification - def hook_ssl_verification - !!enable_ssl_verification - end - override :update_web_hook! def update_web_hook! # If using a service template, project may not be available super if project end + + def enable_ssl_verification + original_value = Gitlab::Utils.to_boolean(properties['enable_ssl_verification']) + original_value.nil? ? (new_record? || url_is_saas?) : original_value + end + + private + + def url_is_saas? + parsed_url = Addressable::URI.parse(drone_url) + parsed_url&.scheme == 'https' && parsed_url.hostname == DRONE_SAAS_HOSTNAME + rescue Addressable::URI::InvalidURIError + false + end end end diff --git a/app/models/integrations/jenkins.rb b/app/models/integrations/jenkins.rb index e5c1d5ad0d7..5ea92170c26 100644 --- a/app/models/integrations/jenkins.rb +++ b/app/models/integrations/jenkins.rb @@ -4,6 +4,7 @@ module Integrations class Jenkins < BaseCi include HasWebHook include ActionView::Helpers::UrlHelper + prepend EnableSslVerification extend Gitlab::Utils::Override prop_accessor :jenkins_url, :project_name, :username, :password diff --git a/app/models/integrations/jira.rb b/app/models/integrations/jira.rb index 816f5cbe177..966ad07afad 100644 --- a/app/models/integrations/jira.rb +++ b/app/models/integrations/jira.rb @@ -56,6 +56,12 @@ module Integrations @reference_pattern ||= /(?<issue>\b#{Gitlab::Regex.jira_issue_key_regex})/ end + def self.valid_jira_cloud_url?(url) + return false unless url.present? + + !!URI(url).hostname&.end_with?(JIRA_CLOUD_HOST) + end + def initialize_properties {} end @@ -565,7 +571,7 @@ module Integrations end def jira_cloud? - server_info['deploymentType'] == 'Cloud' || URI(client_url).hostname.end_with?(JIRA_CLOUD_HOST) + server_info['deploymentType'] == 'Cloud' || self.class.valid_jira_cloud_url?(client_url) end def set_deployment_type_from_url @@ -578,7 +584,7 @@ module Integrations # we can only assume it's either Cloud or Server # based on the URL being *.atlassian.net - if URI(client_url).hostname.end_with?(JIRA_CLOUD_HOST) + if self.class.valid_jira_cloud_url?(client_url) data_fields.deployment_cloud! else data_fields.deployment_server! diff --git a/app/models/integrations/mock_ci.rb b/app/models/integrations/mock_ci.rb index 7359be83d4f..568fb609a44 100644 --- a/app/models/integrations/mock_ci.rb +++ b/app/models/integrations/mock_ci.rb @@ -3,6 +3,8 @@ # For an example companion mocking service, see https://gitlab.com/gitlab-org/gitlab-mock-ci-service module Integrations class MockCi < BaseCi + prepend EnableSslVerification + ALLOWED_STATES = %w[failed canceled running pending success success-with-warnings skipped not_found].freeze prop_accessor :mock_service_url @@ -55,7 +57,7 @@ module Integrations # # => 'running' # def commit_status(sha, ref) - response = Gitlab::HTTP.get(commit_status_path(sha), verify: false, use_read_total_timeout: true) + response = Gitlab::HTTP.get(commit_status_path(sha), verify: enable_ssl_verification, use_read_total_timeout: true) read_commit_status(response) rescue Errno::ECONNREFUSED :error @@ -68,19 +70,16 @@ module Integrations end def read_commit_status(response) - return :error unless response.code == 200 || response.code == 404 - - status = if response.code == 404 - 'pending' - else - response['status'] - end + return :pending if response.code == 404 + return :error unless response.code == 200 - if status.present? && ALLOWED_STATES.include?(status) - status - else - :error + begin + status = Gitlab::Json.parse(response.body).try(:fetch, 'status', nil) + return status if ALLOWED_STATES.include?(status) + rescue JSON::ParserError end + + :error end def testable? diff --git a/app/models/integrations/teamcity.rb b/app/models/integrations/teamcity.rb index 008b591c304..f0f83f118d7 100644 --- a/app/models/integrations/teamcity.rb +++ b/app/models/integrations/teamcity.rb @@ -4,6 +4,9 @@ module Integrations class Teamcity < BaseCi include PushDataValidations include ReactivelyCached + prepend EnableSslVerification + + TEAMCITY_SAAS_HOSTNAME = /\A[^\.]+\.teamcity\.com\z/i.freeze prop_accessor :teamcity_url, :build_type, :username, :password @@ -104,8 +107,20 @@ module Integrations end end + def enable_ssl_verification + original_value = Gitlab::Utils.to_boolean(properties['enable_ssl_verification']) + original_value.nil? ? (new_record? || url_is_saas?) : original_value + end + private + def url_is_saas? + parsed_url = Addressable::URI.parse(teamcity_url) + parsed_url&.scheme == 'https' && parsed_url.hostname.match?(TEAMCITY_SAAS_HOSTNAME) + rescue Addressable::URI::InvalidURIError + false + end + def execute_push(data) branch = Gitlab::Git.ref_name(data[:ref]) post_to_build_queue(data, branch) if push_valid?(data) @@ -155,7 +170,7 @@ module Integrations end def get_path(path) - Gitlab::HTTP.try_get(build_url(path), verify: false, basic_auth: basic_auth, extra_log_info: { project_id: project_id }, use_read_total_timeout: true) + Gitlab::HTTP.try_get(build_url(path), verify: enable_ssl_verification, basic_auth: basic_auth, extra_log_info: { project_id: project_id }, use_read_total_timeout: true) end def post_to_build_queue(data, branch) @@ -165,6 +180,7 @@ module Integrations "<buildType id=#{build_type.encode(xml: :attr)}/>"\ '</build>', headers: { 'Content-type' => 'application/xml' }, + verify: enable_ssl_verification, basic_auth: basic_auth, use_read_total_timeout: true ) diff --git a/app/models/packages/package.rb b/app/models/packages/package.rb index 52dd0aba43b..1b2c3e25283 100644 --- a/app/models/packages/package.rb +++ b/app/models/packages/package.rb @@ -25,7 +25,7 @@ class Packages::Package < ApplicationRecord terraform_module: 12 } - enum status: { default: 0, hidden: 1, processing: 2, error: 3 } + enum status: { default: 0, hidden: 1, processing: 2, error: 3, pending_destruction: 4 } belongs_to :project belongs_to :creator, class_name: 'User' @@ -304,6 +304,12 @@ class Packages::Package < ApplicationRecord build_infos.find_or_create_by!(pipeline: build.pipeline) end + def mark_package_files_for_destruction + return unless pending_destruction? + + ::Packages::MarkPackageFilesForDestructionWorker.perform_async(id) + end + private def composer_tag_version? diff --git a/app/models/packages/package_file.rb b/app/models/packages/package_file.rb index 072ff4a3a5f..190081c4e8e 100644 --- a/app/models/packages/package_file.rb +++ b/app/models/packages/package_file.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true class Packages::PackageFile < ApplicationRecord + include EachBatch include UpdateProjectStatistics include FileStoreMounter include Packages::Installable diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 7b13109dbc4..a3c9db90b5d 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -10,7 +10,7 @@ class SystemNoteMetadata < ApplicationRecord # in the same project (i.e. with the same permissions) TYPES_WITH_CROSS_REFERENCES = %w[ commit cross_reference - close duplicate + closed duplicate moved merge label milestone relate unrelate diff --git a/app/services/concerns/protected_ref_name_sanitizer.rb b/app/services/concerns/protected_ref_name_sanitizer.rb deleted file mode 100644 index 3966c410fec..00000000000 --- a/app/services/concerns/protected_ref_name_sanitizer.rb +++ /dev/null @@ -1,12 +0,0 @@ -# frozen_string_literal: true - -module ProtectedRefNameSanitizer - def sanitize_name(name) - name = CGI.unescapeHTML(name) - name = Sanitize.fragment(name) - - # Sanitize.fragment escapes HTML chars, so unescape again to allow names - # like `feature->master` - CGI.unescapeHTML(name) - end -end diff --git a/app/services/packages/mark_package_files_for_destruction_service.rb b/app/services/packages/mark_package_files_for_destruction_service.rb new file mode 100644 index 00000000000..3672b44b409 --- /dev/null +++ b/app/services/packages/mark_package_files_for_destruction_service.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Packages + # WARNING: ensure that permissions are verified before using this service. + class MarkPackageFilesForDestructionService + BATCH_SIZE = 500 + + def initialize(package_files) + @package_files = package_files + end + + def execute + @package_files.each_batch(of: BATCH_SIZE) do |batched_package_files| + batched_package_files.update_all(status: :pending_destruction) + end + + service_response_success('Package files are now pending destruction') + end + + private + + def service_response_success(message) + ServiceResponse.success(message: message) + end + end +end diff --git a/app/services/packages/destroy_package_service.rb b/app/services/packages/mark_package_for_destruction_service.rb index 697f1fa3ac8..3417febe79a 100644 --- a/app/services/packages/destroy_package_service.rb +++ b/app/services/packages/mark_package_for_destruction_service.rb @@ -1,19 +1,20 @@ # frozen_string_literal: true module Packages - class DestroyPackageService < BaseContainerService + class MarkPackageForDestructionService < BaseContainerService alias_method :package, :container def execute return service_response_error("You don't have access to this package", 403) unless user_can_delete_package? - package.destroy! + package.pending_destruction! + package.mark_package_files_for_destruction package.sync_maven_metadata(current_user) - service_response_success('Package was successfully deleted') + service_response_success('Package was successfully marked as pending destruction') rescue StandardError - service_response_error('Failed to remove the package', 400) + service_response_error('Failed to mark the package as pending destruction', 400) end private diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb index 1ab3ccfcaae..f48e02ab4b5 100644 --- a/app/services/protected_branches/base_service.rb +++ b/app/services/protected_branches/base_service.rb @@ -2,8 +2,6 @@ module ProtectedBranches class BaseService < ::BaseService - include ProtectedRefNameSanitizer - # current_user - The user that performs the action # params - A hash of parameters def initialize(project, current_user = nil, params = {}) @@ -15,14 +13,5 @@ module ProtectedBranches def after_execute(*) # overridden in EE::ProtectedBranches module end - - private - - def filtered_params - return unless params - - params[:name] = sanitize_name(params[:name]) if params[:name].present? - params - end end end diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb index ea494dd4426..dada449989a 100644 --- a/app/services/protected_branches/create_service.rb +++ b/app/services/protected_branches/create_service.rb @@ -21,7 +21,7 @@ module ProtectedBranches end def protected_branch - @protected_branch ||= project.protected_branches.new(filtered_params) + @protected_branch ||= project.protected_branches.new(params) end end end diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb index 40e9a286af9..1e70f2d9793 100644 --- a/app/services/protected_branches/update_service.rb +++ b/app/services/protected_branches/update_service.rb @@ -8,7 +8,7 @@ module ProtectedBranches old_merge_access_levels = protected_branch.merge_access_levels.map(&:clone) old_push_access_levels = protected_branch.push_access_levels.map(&:clone) - if protected_branch.update(filtered_params) + if protected_branch.update(params) after_execute(protected_branch: protected_branch, old_merge_access_levels: old_merge_access_levels, old_push_access_levels: old_push_access_levels) end diff --git a/app/services/protected_tags/base_service.rb b/app/services/protected_tags/base_service.rb deleted file mode 100644 index e0181815f0f..00000000000 --- a/app/services/protected_tags/base_service.rb +++ /dev/null @@ -1,16 +0,0 @@ -# frozen_string_literal: true - -module ProtectedTags - class BaseService < ::BaseService - include ProtectedRefNameSanitizer - - private - - def filtered_params - return unless params - - params[:name] = sanitize_name(params[:name]) if params[:name].present? - params - end - end -end diff --git a/app/services/protected_tags/create_service.rb b/app/services/protected_tags/create_service.rb index 7d2b583a295..65303f21a4a 100644 --- a/app/services/protected_tags/create_service.rb +++ b/app/services/protected_tags/create_service.rb @@ -1,13 +1,13 @@ # frozen_string_literal: true module ProtectedTags - class CreateService < ProtectedTags::BaseService + class CreateService < ::BaseService attr_reader :protected_tag def execute raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project) - project.protected_tags.create(filtered_params) + project.protected_tags.create(params) end end end diff --git a/app/services/protected_tags/update_service.rb b/app/services/protected_tags/update_service.rb index e337ec39898..283aa8882c5 100644 --- a/app/services/protected_tags/update_service.rb +++ b/app/services/protected_tags/update_service.rb @@ -1,11 +1,11 @@ # frozen_string_literal: true module ProtectedTags - class UpdateService < ProtectedTags::BaseService + class UpdateService < ::BaseService def execute(protected_tag) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project) - protected_tag.update(filtered_params) + protected_tag.update(params) protected_tag end end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 14fa6599073..239b66bdeb0 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -1384,6 +1384,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: package_cleanup:packages_mark_package_files_for_destruction + :worker_name: Packages::MarkPackageFilesForDestructionWorker + :feature_category: :package_registry + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: package_repositories:packages_debian_generate_distribution :worker_name: Packages::Debian::GenerateDistributionWorker :feature_category: :package_registry diff --git a/app/workers/concerns/packages/cleanup_artifact_worker.rb b/app/workers/concerns/packages/cleanup_artifact_worker.rb index db6c7330ea3..d4ad023b4a8 100644 --- a/app/workers/concerns/packages/cleanup_artifact_worker.rb +++ b/app/workers/concerns/packages/cleanup_artifact_worker.rb @@ -9,11 +9,15 @@ module Packages def perform_work return unless artifact - log_metadata(artifact) + artifact.transaction do + log_metadata(artifact) - artifact.destroy! - rescue StandardError - artifact&.error! + artifact.destroy! + rescue StandardError + artifact&.error! + end + + after_destroy end def remaining_work_count @@ -34,6 +38,10 @@ module Packages raise NotImplementedError end + def after_destroy + # no op + end + def artifact strong_memoize(:artifact) do model.transaction do diff --git a/app/workers/packages/cleanup_package_file_worker.rb b/app/workers/packages/cleanup_package_file_worker.rb index cb2b2a12c5e..f188017ee7a 100644 --- a/app/workers/packages/cleanup_package_file_worker.rb +++ b/app/workers/packages/cleanup_package_file_worker.rb @@ -19,6 +19,14 @@ module Packages private + def after_destroy + pkg = artifact.package + + pkg.transaction do + pkg.destroy if model.for_package_ids(pkg.id).empty? + end + end + def model Packages::PackageFile end diff --git a/app/workers/packages/mark_package_files_for_destruction_worker.rb b/app/workers/packages/mark_package_files_for_destruction_worker.rb new file mode 100644 index 00000000000..d8e52202841 --- /dev/null +++ b/app/workers/packages/mark_package_files_for_destruction_worker.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Packages + class MarkPackageFilesForDestructionWorker + include ApplicationWorker + + data_consistency :sticky + queue_namespace :package_cleanup + feature_category :package_registry + urgency :low + deduplicate :until_executing, including_scheduled: true + idempotent! + + sidekiq_options retry: 3 + + def perform(package_id) + package = ::Packages::Package.find_by_id(package_id) + + return unless package + + ::Packages::MarkPackageFilesForDestructionService.new(package.package_files) + .execute + end + end +end diff --git a/db/migrate/20220113135449_add_package_files_limit_to_application_settings.rb b/db/migrate/20220113135449_add_package_files_limit_to_application_settings.rb new file mode 100644 index 00000000000..6d3deacdda3 --- /dev/null +++ b/db/migrate/20220113135449_add_package_files_limit_to_application_settings.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddPackageFilesLimitToApplicationSettings < Gitlab::Database::Migration[1.0] + def change + add_column :application_settings, :max_package_files_for_package_destruction, :smallint, default: 100, null: false + end +end diff --git a/db/migrate/20220113135924_add_application_settings_package_files_limit_constraints.rb b/db/migrate/20220113135924_add_application_settings_package_files_limit_constraints.rb new file mode 100644 index 00000000000..65fbccbd1ae --- /dev/null +++ b/db/migrate/20220113135924_add_application_settings_package_files_limit_constraints.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddApplicationSettingsPackageFilesLimitConstraints < Gitlab::Database::Migration[1.0] + CONSTRAINT_NAME = 'app_settings_max_package_files_for_package_destruction_positive' + + disable_ddl_transaction! + + def up + add_check_constraint :application_settings, 'max_package_files_for_package_destruction > 0', CONSTRAINT_NAME + end + + def down + remove_check_constraint :application_settings, CONSTRAINT_NAME + end +end diff --git a/db/schema_migrations/20220113135449 b/db/schema_migrations/20220113135449 new file mode 100644 index 00000000000..57e6ede94b5 --- /dev/null +++ b/db/schema_migrations/20220113135449 @@ -0,0 +1 @@ +46796379175ce9343907234d3ae14a417442c7c5ebbfcf6987db1d759eca2c3a
\ No newline at end of file diff --git a/db/schema_migrations/20220113135924 b/db/schema_migrations/20220113135924 new file mode 100644 index 00000000000..41328a43c37 --- /dev/null +++ b/db/schema_migrations/20220113135924 @@ -0,0 +1 @@ +2a230758c13111c9e3738794008c31a3608dda2f0d071fbde0ad3cd782d29162
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index fff600c392a..47e111bf48f 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -10489,12 +10489,14 @@ CREATE TABLE application_settings ( container_registry_import_max_step_duration integer DEFAULT 300 NOT NULL, container_registry_import_target_plan text DEFAULT 'free'::text NOT NULL, container_registry_import_created_before timestamp with time zone DEFAULT '2022-01-23 00:00:00+00'::timestamp with time zone NOT NULL, + max_package_files_for_package_destruction smallint DEFAULT 100 NOT NULL, runner_token_expiration_interval integer, group_runner_token_expiration_interval integer, project_runner_token_expiration_interval integer, CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)), CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)), CONSTRAINT app_settings_ext_pipeline_validation_service_url_text_limit CHECK ((char_length(external_pipeline_validation_service_url) <= 255)), + CONSTRAINT app_settings_max_package_files_for_package_destruction_positive CHECK ((max_package_files_for_package_destruction > 0)), CONSTRAINT app_settings_p_cleanup_package_file_worker_capacity_positive CHECK ((packages_cleanup_package_file_worker_capacity >= 0)), CONSTRAINT app_settings_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)), CONSTRAINT app_settings_yaml_max_depth_positive CHECK ((max_yaml_depth > 0)), diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 3148fd1a6df..d7bf89c67ce 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -17301,6 +17301,7 @@ Values for sorting package. | <a id="packagestatusdefault"></a>`DEFAULT` | Packages with a default status. | | <a id="packagestatuserror"></a>`ERROR` | Packages with a error status. | | <a id="packagestatushidden"></a>`HIDDEN` | Packages with a hidden status. | +| <a id="packagestatuspending_destruction"></a>`PENDING_DESTRUCTION` | Packages with a pending_destruction status. | | <a id="packagestatusprocessing"></a>`PROCESSING` | Packages with a processing status. | ### `PackageTypeEnum` diff --git a/doc/api/integrations.md b/doc/api/integrations.md index dd5a8c568fb..4719fe69c7b 100644 --- a/doc/api/integrations.md +++ b/doc/api/integrations.md @@ -165,6 +165,7 @@ Parameters: | Parameter | Type | Required | Description | | --------- | ---- | -------- | ----------- | | `bamboo_url` | string | true | Bamboo root URL. For example, `https://bamboo.example.com`. | +| `enable_ssl_verification` | boolean | false | Enable SSL verification. Defaults to true (enabled). | | `build_key` | string | true | Bamboo build plan key like KEY | | `username` | string | true | A user with API access, if applicable | | `password` | string | true | Password of the user | @@ -521,7 +522,7 @@ Parameters: | --------- | ---- | -------- | ----------- | | `token` | string | true | Drone CI project specific token | | `drone_url` | string | true | `http://drone.example.com` | -| `enable_ssl_verification` | boolean | false | Enable SSL verification | +| `enable_ssl_verification` | boolean | false | Enable SSL verification. Defaults to true (enabled). | | `push_events` | boolean | false | Enable notifications for push events | | `merge_requests_events` | boolean | false | Enable notifications for merge request events | | `tag_push_events` | boolean | false | Enable notifications for tag push events | @@ -1396,6 +1397,7 @@ Parameters: | Parameter | Type | Required | Description | | --------- | ---- | -------- | ----------- | | `teamcity_url` | string | true | TeamCity root URL. For example, `https://teamcity.example.com` | +| `enable_ssl_verification` | boolean | false | Enable SSL verification. Defaults to true (enabled). | | `build_type` | string | true | Build configuration ID | | `username` | string | true | A user with permissions to trigger a manual build | | `password` | string | true | The password of the user | @@ -1434,6 +1436,7 @@ Parameters: | Parameter | Type | Required | Description | | --------- | ---- | -------- | ----------- | | `jenkins_url` | string | true | Jenkins URL like `http://jenkins.example.com`. | +| `enable_ssl_verification` | boolean | false | Enable SSL verification. Defaults to true (enabled). | | `project_name` | string | true | The URL-friendly project name. Example: `my_project_name`. | | `username` | string | false | Username for authentication with the Jenkins server, if authentication is required by the server. | | `password` | string | false | Password for authentication with the Jenkins server, if authentication is required by the server. | @@ -1513,6 +1516,7 @@ Parameters: | Parameter | Type | Required | Description | | --------- | ---- | -------- | ----------- | | `mock_service_url` | string | true | `http://localhost:4004` | +| `enable_ssl_verification` | boolean | false | Enable SSL verification. Defaults to true (enabled). | ### Disable MockCI integration diff --git a/lib/api/helpers/integrations_helpers.rb b/lib/api/helpers/integrations_helpers.rb index 3af0dd4c532..72b16a23dd6 100644 --- a/lib/api/helpers/integrations_helpers.rb +++ b/lib/api/helpers/integrations_helpers.rb @@ -197,6 +197,12 @@ module API desc: 'Bamboo root URL like https://bamboo.example.com' }, { + required: false, + name: :enable_ssl_verification, + type: Boolean, + desc: 'Enable SSL verification' + }, + { required: true, name: :build_key, type: String, @@ -368,7 +374,7 @@ module API required: false, name: :enable_ssl_verification, type: Boolean, - desc: 'Enable SSL verification for communication' + desc: 'Enable SSL verification' } ], 'emails-on-push' => [ @@ -468,6 +474,12 @@ module API desc: 'Jenkins root URL like https://jenkins.example.com' }, { + required: false, + name: :enable_ssl_verification, + type: Boolean, + desc: 'Enable SSL verification' + }, + { required: true, name: :project_name, type: String, @@ -749,6 +761,12 @@ module API desc: 'TeamCity root URL like https://teamcity.example.com' }, { + required: false, + name: :enable_ssl_verification, + type: Boolean, + desc: 'Enable SSL verification' + }, + { required: true, name: :build_type, type: String, diff --git a/lib/api/package_files.rb b/lib/api/package_files.rb index 5e421da2c55..3bf47fe1e8b 100644 --- a/lib/api/package_files.rb +++ b/lib/api/package_files.rb @@ -65,7 +65,9 @@ module API not_found! unless package_file - destroy_conditionally!(package_file) + destroy_conditionally!(package_file) do |package_file| + package_file.pending_destruction! + end end end end diff --git a/lib/api/project_packages.rb b/lib/api/project_packages.rb index 54c0a0628a7..c997afea865 100644 --- a/lib/api/project_packages.rb +++ b/lib/api/project_packages.rb @@ -71,7 +71,7 @@ module API .new(user_project, params[:package_id]).execute destroy_conditionally!(package) do |package| - ::Packages::DestroyPackageService.new(container: package, current_user: current_user).execute + ::Packages::MarkPackageForDestructionService.new(container: package, current_user: current_user).execute end end end diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index f092e03046a..48228ede684 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -148,9 +148,17 @@ module Gitlab unless allow_local_network validate_local_network(address_info) validate_link_local(address_info) + validate_shared_address(address_info) end end + def validate_shared_address(addrs_info) + netmask = IPAddr.new('100.64.0.0/10') + return unless addrs_info.any? { |addr| netmask.include?(addr.ip_address) } + + raise BlockedUrlError, "Requests to the shared address space are not allowed" + end + def get_port(uri) uri.port || uri.default_port end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8f5be8ace27..150f6214e95 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -19142,6 +19142,9 @@ msgstr "" msgid "Integrations|Browser limitations" msgstr "" +msgid "Integrations|Clear if using a self-signed certificate." +msgstr "" + msgid "Integrations|Comment detail:" msgstr "" @@ -19172,6 +19175,9 @@ msgstr "" msgid "Integrations|Enable GitLab.com slash commands in a Slack workspace." msgstr "" +msgid "Integrations|Enable SSL verification" +msgstr "" + msgid "Integrations|Enable comments" msgstr "" @@ -19250,6 +19256,9 @@ msgstr "" msgid "Integrations|Return to GitLab for Jira" msgstr "" +msgid "Integrations|SSL verification" +msgstr "" + msgid "Integrations|Save settings?" msgstr "" diff --git a/spec/factories/packages/packages.rb b/spec/factories/packages/packages.rb index 153518f4cd3..90c68074a3b 100644 --- a/spec/factories/packages/packages.rb +++ b/spec/factories/packages/packages.rb @@ -20,6 +20,10 @@ FactoryBot.define do status { :error } end + trait :pending_destruction do + status { :pending_destruction } + end + factory :maven_package do maven_metadatum diff --git a/spec/features/issues/notes_on_issues_spec.rb b/spec/features/issues/notes_on_issues_spec.rb index be85d73d777..4e98062e8b2 100644 --- a/spec/features/issues/notes_on_issues_spec.rb +++ b/spec/features/issues/notes_on_issues_spec.rb @@ -91,4 +91,62 @@ RSpec.describe 'Create notes on issues', :js do expect(page).to have_selector '.gfm-project_member.current-user', text: user.username end + + shared_examples "when reference belongs to a private project" do + let(:project) { create(:project, :private, :repository) } + let(:issue) { create(:issue, project: project) } + + before do + sign_in(user) + end + + context 'when the user does not have permission to see the reference' do + before do + project.add_guest(user) + end + + it 'does not show the user the reference' do + visit project_issue_path(project, issue) + + expect(page).not_to have_content('closed via') + end + end + + context 'when the user has permission to see the reference' do + before do + project.add_developer(user) + end + + it 'shows the user the reference' do + visit project_issue_path(project, issue) + + page.within('div#notes li.note .system-note-message') do + expect(page).to have_content('closed via') + expect(page.find('a')).to have_content(reference_content) + end + end + end + end + + context 'when the issue is closed via a merge request' do + it_behaves_like "when reference belongs to a private project" do + let(:reference) { create(:merge_request, source_project: project) } + let(:reference_content) { reference.to_reference } + + before do + create(:resource_state_event, issue: issue, state: :closed, created_at: '2020-02-05', source_merge_request: reference) + end + end + end + + context 'when the issue is closed via a commit' do + it_behaves_like "when reference belongs to a private project" do + let(:reference) { create(:commit, project: project) } + let(:reference_content) { reference.short_sha } + + before do + create(:resource_state_event, issue: issue, state: :closed, created_at: '2020-02-05', source_commit: reference.id) + end + end + end end diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb index 4278efc5a8f..389a51a10e0 100644 --- a/spec/features/protected_branches_spec.rb +++ b/spec/features/protected_branches_spec.rb @@ -38,6 +38,17 @@ RSpec.describe 'Protected Branches', :js do sign_in(user) end + it 'allows to create a protected branch with name containing HTML tags' do + visit project_protected_branches_path(project) + set_defaults + set_protected_branch_name('foo<b>bar<\b>') + click_on "Protect" + + within(".protected-branches-list") { expect(page).to have_content('foo<b>bar<\b>') } + expect(ProtectedBranch.count).to eq(1) + expect(ProtectedBranch.last.name).to eq('foo<b>bar<\b>') + end + describe 'Delete protected branch' do before do create(:protected_branch, project: project, name: 'fix') diff --git a/spec/frontend/create_item_dropdown_spec.js b/spec/frontend/create_item_dropdown_spec.js index 56c09cd731e..143ccb9b930 100644 --- a/spec/frontend/create_item_dropdown_spec.js +++ b/spec/frontend/create_item_dropdown_spec.js @@ -17,6 +17,11 @@ const DROPDOWN_ITEM_DATA = [ id: 'three', text: 'three', }, + { + title: '<b>four</b>title', + id: '<b>four</b>id', + text: '<b>four</b>text', + }, ]; describe('CreateItemDropdown', () => { @@ -63,6 +68,10 @@ describe('CreateItemDropdown', () => { const $itemEls = $wrapperEl.find('.js-dropdown-content a'); expect($itemEls.length).toEqual(DROPDOWN_ITEM_DATA.length); + + DROPDOWN_ITEM_DATA.forEach((dataItem, i) => { + expect($($itemEls[i]).text()).toEqual(dataItem.text); + }); }); }); @@ -177,7 +186,7 @@ describe('CreateItemDropdown', () => { const $itemEls = $wrapperEl.find('.js-dropdown-content a'); expect($itemEls.length).toEqual(1 + DROPDOWN_ITEM_DATA.length); - expect($($itemEls[3]).text()).toEqual('new-item-text'); + expect($($itemEls[DROPDOWN_ITEM_DATA.length]).text()).toEqual('new-item-text'); expect($wrapperEl.find('.dropdown-toggle-text').text()).toEqual('new-item-title'); }); }); diff --git a/spec/graphql/types/packages/package_status_enum_spec.rb b/spec/graphql/types/packages/package_status_enum_spec.rb index 71d05da35ea..2c79c92498b 100644 --- a/spec/graphql/types/packages/package_status_enum_spec.rb +++ b/spec/graphql/types/packages/package_status_enum_spec.rb @@ -4,6 +4,6 @@ require 'spec_helper' RSpec.describe GitlabSchema.types['PackageStatus'] do it 'exposes all package statuses' do - expect(described_class.values.keys).to contain_exactly(*%w[DEFAULT HIDDEN PROCESSING ERROR]) + expect(described_class.values.keys).to contain_exactly(*%w[DEFAULT HIDDEN PROCESSING ERROR PENDING_DESTRUCTION]) end end diff --git a/spec/graphql/types/project_type_spec.rb b/spec/graphql/types/project_type_spec.rb index cd216232569..961e12288d4 100644 --- a/spec/graphql/types/project_type_spec.rb +++ b/spec/graphql/types/project_type_spec.rb @@ -595,4 +595,45 @@ RSpec.describe GitlabSchema.types['Project'] do expect(cluster_agent.agent_tokens.size).to be(count) end end + + describe 'service_desk_address' do + let(:user) { create(:user) } + let(:query) do + %( + query { + project(fullPath: "#{project.full_path}") { + id + serviceDeskAddress + } + } + ) + end + + subject { GitlabSchema.execute(query, context: { current_user: user }).as_json } + + before do + allow(::Gitlab::ServiceDeskEmail).to receive(:enabled?) { true } + allow(::Gitlab::ServiceDeskEmail).to receive(:address_for_key) { 'address-suffix@example.com' } + end + + context 'when a user can admin issues' do + let(:project) { create(:project, :public, :service_desk_enabled) } + + before do + project.add_reporter(user) + end + + it 'is present' do + expect(subject.dig('data', 'project', 'serviceDeskAddress')).to be_present + end + end + + context 'when a user can not admin issues' do + let(:project) { create(:project, :public, :service_desk_disabled) } + + it 'is empty' do + expect(subject.dig('data', 'project', 'serviceDeskAddress')).to be_blank + end + end + end end diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 0713475d59b..5b77290ce2e 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -279,6 +279,8 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do end context 'when allow_local_network is' do + let(:shared_address_space_ips) { ['100.64.0.0', '100.64.127.127', '100.64.255.255'] } + let(:local_ips) do [ '192.168.1.2', @@ -292,7 +294,8 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do '[::ffff:ac10:20]', '[feef::1]', '[fee2::]', - '[fc00:bf8b:e62c:abcd:abcd:aaaa:aaaa:aaaa]' + '[fc00:bf8b:e62c:abcd:abcd:aaaa:aaaa:aaaa]', + *shared_address_space_ips ] end @@ -385,18 +388,7 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do '127.0.0.1', '127.0.0.2', '192.168.1.1', - '192.168.1.2', - '0:0:0:0:0:ffff:192.168.1.2', - '::ffff:c0a8:102', - '10.0.0.2', - '0:0:0:0:0:ffff:10.0.0.2', - '::ffff:a00:2', - '172.16.0.2', - '0:0:0:0:0:ffff:172.16.0.2', - '::ffff:ac10:20', - 'feef::1', - 'fee2::', - 'fc00:bf8b:e62c:abcd:abcd:aaaa:aaaa:aaaa', + *local_ips, '0:0:0:0:0:ffff:169.254.169.254', '::ffff:a9fe:a9fe', '::ffff:169.254.168.100', diff --git a/spec/models/concerns/integrations/enable_ssl_verification_spec.rb b/spec/models/concerns/integrations/enable_ssl_verification_spec.rb new file mode 100644 index 00000000000..802e950c0c2 --- /dev/null +++ b/spec/models/concerns/integrations/enable_ssl_verification_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Integrations::EnableSslVerification do + let(:described_class) do + Class.new(Integration) do + prepend Integrations::EnableSslVerification + + def fields + [ + { name: 'main_url' }, + { name: 'other_url' }, + { name: 'username' } + ] + end + end + end + + let(:integration) { described_class.new } + + include_context Integrations::EnableSslVerification +end diff --git a/spec/models/integrations/bamboo_spec.rb b/spec/models/integrations/bamboo_spec.rb index 60ff6685c3d..b5684d153f2 100644 --- a/spec/models/integrations/bamboo_spec.rb +++ b/spec/models/integrations/bamboo_spec.rb @@ -23,6 +23,8 @@ RSpec.describe Integrations::Bamboo, :use_clean_rails_memory_store_caching do ) end + include_context Integrations::EnableSslVerification + describe 'Validations' do context 'when active' do before do diff --git a/spec/models/integrations/drone_ci_spec.rb b/spec/models/integrations/drone_ci_spec.rb index 062e23d628e..dd64dcfc52c 100644 --- a/spec/models/integrations/drone_ci_spec.rb +++ b/spec/models/integrations/drone_ci_spec.rb @@ -5,6 +5,8 @@ require 'spec_helper' RSpec.describe Integrations::DroneCi, :use_clean_rails_memory_store_caching do include ReactiveCachingHelpers + subject(:integration) { described_class.new } + describe 'validations' do context 'active' do before do @@ -59,6 +61,52 @@ RSpec.describe Integrations::DroneCi, :use_clean_rails_memory_store_caching do end end + include_context Integrations::EnableSslVerification do + describe '#enable_ssl_verification' do + before do + allow(integration).to receive(:new_record?).and_return(false) + end + + it 'returns true for a known hostname' do + integration.drone_url = 'https://cloud.drone.io' + + expect(integration.enable_ssl_verification).to be(true) + end + + it 'returns true for new records' do + allow(integration).to receive(:new_record?).and_return(true) + integration.drone_url = 'http://example.com' + + expect(integration.enable_ssl_verification).to be(true) + end + + it 'returns false for an unknown hostname' do + integration.drone_url = 'https://example.com' + + expect(integration.enable_ssl_verification).to be(false) + end + + it 'returns false for a HTTP URL' do + integration.drone_url = 'http://cloud.drone.io' + + expect(integration.enable_ssl_verification).to be(false) + end + + it 'returns false for an invalid URL' do + integration.drone_url = 'https://example.com:foo' + + expect(integration.enable_ssl_verification).to be(false) + end + + it 'returns the persisted value if present' do + integration.drone_url = 'https://cloud.drone.io' + integration.enable_ssl_verification = false + + expect(integration.enable_ssl_verification).to be(false) + end + end + end + it_behaves_like Integrations::HasWebHook do include_context :drone_ci_integration diff --git a/spec/models/integrations/jenkins_spec.rb b/spec/models/integrations/jenkins_spec.rb index 9286d026290..3d6393f2793 100644 --- a/spec/models/integrations/jenkins_spec.rb +++ b/spec/models/integrations/jenkins_spec.rb @@ -24,6 +24,10 @@ RSpec.describe Integrations::Jenkins do let(:jenkins_authorization) { "Basic " + ::Base64.strict_encode64(jenkins_username + ':' + jenkins_password) } + include_context Integrations::EnableSslVerification do + let(:integration) { described_class.new(jenkins_params) } + end + it_behaves_like Integrations::HasWebHook do let(:integration) { described_class.new(jenkins_params) } let(:hook_url) { "http://#{ERB::Util.url_encode jenkins_username}:#{ERB::Util.url_encode jenkins_password}@jenkins.example.com/project/my_project" } diff --git a/spec/models/integrations/jira_spec.rb b/spec/models/integrations/jira_spec.rb index e80fa6e3b70..6ce84c28044 100644 --- a/spec/models/integrations/jira_spec.rb +++ b/spec/models/integrations/jira_spec.rb @@ -130,6 +130,23 @@ RSpec.describe Integrations::Jira do end end + describe '.valid_jira_cloud_url?' do + using RSpec::Parameterized::TableSyntax + + where(:url, :result) do + 'https://abc.atlassian.net' | true + 'abc.atlassian.net' | false # This is how it behaves currently, but we may need to consider adding scheme if missing + 'https://somethingelse.com' | false + nil | false + end + + with_them do + specify do + expect(described_class.valid_jira_cloud_url?(url)).to eq(result) + end + end + end + describe '#create' do let(:params) do { diff --git a/spec/models/integrations/mock_ci_spec.rb b/spec/models/integrations/mock_ci_spec.rb new file mode 100644 index 00000000000..d29c63b3a97 --- /dev/null +++ b/spec/models/integrations/mock_ci_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Integrations::MockCi do + let_it_be(:project) { build(:project) } + + subject(:integration) { described_class.new(project: project, mock_service_url: generate(:url)) } + + include_context Integrations::EnableSslVerification + + describe '#commit_status' do + let(:sha) { generate(:sha) } + + def stub_request(*args) + WebMock.stub_request(:get, integration.commit_status_path(sha)).to_return(*args) + end + + def commit_status + integration.commit_status(sha, 'master') + end + + it 'returns allowed states' do + described_class::ALLOWED_STATES.each do |state| + stub_request(status: 200, body: { status: state }.to_json) + + expect(commit_status).to eq(state) + end + end + + it 'returns :pending for 404 responses' do + stub_request(status: 404) + + expect(commit_status).to eq(:pending) + end + + it 'returns :error for responses other than 200 or 404' do + stub_request(status: 500) + + expect(commit_status).to eq(:error) + end + + it 'returns :error for unknown states' do + stub_request(status: 200, body: { status: 'unknown' }.to_json) + + expect(commit_status).to eq(:error) + end + + it 'returns :error for invalid JSON' do + stub_request(status: 200, body: '') + + expect(commit_status).to eq(:error) + end + + it 'returns :error for non-hash JSON responses' do + stub_request(status: 200, body: 23.to_json) + + expect(commit_status).to eq(:error) + end + + it 'returns :error for JSON responses without a status' do + stub_request(status: 200, body: { foo: :bar }.to_json) + + expect(commit_status).to eq(:error) + end + + it 'returns :error when connection is refused' do + stub_request(status: 500).to_raise(Errno::ECONNREFUSED) + + expect(commit_status).to eq(:error) + end + end +end diff --git a/spec/models/integrations/teamcity_spec.rb b/spec/models/integrations/teamcity_spec.rb index 0713141ea08..e1f4e577503 100644 --- a/spec/models/integrations/teamcity_spec.rb +++ b/spec/models/integrations/teamcity_spec.rb @@ -6,8 +6,8 @@ RSpec.describe Integrations::Teamcity, :use_clean_rails_memory_store_caching do include ReactiveCachingHelpers include StubRequests - let(:teamcity_url) { 'http://gitlab.com/teamcity' } - let(:teamcity_full_url) { 'http://gitlab.com/teamcity/httpAuth/app/rest/builds/branch:unspecified:any,revision:123' } + let(:teamcity_url) { 'https://gitlab.teamcity.com' } + let(:teamcity_full_url) { 'https://gitlab.teamcity.com/httpAuth/app/rest/builds/branch:unspecified:any,revision:123' } let(:project) { create(:project) } subject(:integration) do @@ -22,6 +22,52 @@ RSpec.describe Integrations::Teamcity, :use_clean_rails_memory_store_caching do ) end + include_context Integrations::EnableSslVerification do + describe '#enable_ssl_verification' do + before do + allow(integration).to receive(:new_record?).and_return(false) + end + + it 'returns true for a known hostname' do + integration.teamcity_url = 'https://example.teamcity.com' + + expect(integration.enable_ssl_verification).to be(true) + end + + it 'returns true for new records' do + allow(integration).to receive(:new_record?).and_return(true) + integration.teamcity_url = 'http://example.com' + + expect(integration.enable_ssl_verification).to be(true) + end + + it 'returns false for an unknown hostname' do + integration.teamcity_url = 'https://sub.example.teamcity.com' + + expect(integration.enable_ssl_verification).to be(false) + end + + it 'returns false for a HTTP URL' do + integration.teamcity_url = 'http://example.teamcity.com' + + expect(integration.enable_ssl_verification).to be(false) + end + + it 'returns false for an invalid URL' do + integration.teamcity_url = 'https://example.com:foo' + + expect(integration.enable_ssl_verification).to be(false) + end + + it 'returns the persisted value if present' do + integration.teamcity_url = 'https://example.teamcity.com' + integration.enable_ssl_verification = false + + expect(integration.enable_ssl_verification).to be(false) + end + end + end + describe 'Validations' do context 'when integration is active' do before do @@ -140,22 +186,22 @@ RSpec.describe Integrations::Teamcity, :use_clean_rails_memory_store_caching do it 'returns a specific URL when status is 500' do stub_request(status: 500) - is_expected.to eq('http://gitlab.com/teamcity/viewLog.html?buildTypeId=foo') + is_expected.to eq("#{teamcity_url}/viewLog.html?buildTypeId=foo") end it 'returns a build URL when teamcity_url has no trailing slash' do stub_request(body: %q({"build":{"id":"666"}})) - is_expected.to eq('http://gitlab.com/teamcity/viewLog.html?buildId=666&buildTypeId=foo') + is_expected.to eq("#{teamcity_url}/viewLog.html?buildId=666&buildTypeId=foo") end context 'teamcity_url has trailing slash' do - let(:teamcity_url) { 'http://gitlab.com/teamcity/' } + let(:teamcity_url) { 'https://gitlab.teamcity.com/' } it 'returns a build URL' do stub_request(body: %q({"build":{"id":"666"}})) - is_expected.to eq('http://gitlab.com/teamcity/viewLog.html?buildId=666&buildTypeId=foo') + is_expected.to eq('https://gitlab.teamcity.com/viewLog.html?buildId=666&buildTypeId=foo') end end @@ -299,7 +345,7 @@ RSpec.describe Integrations::Teamcity, :use_clean_rails_memory_store_caching do end def stub_post_to_build_queue(branch:) - teamcity_full_url = 'http://gitlab.com/teamcity/httpAuth/app/rest/buildQueue' + teamcity_full_url = "#{teamcity_url}/httpAuth/app/rest/buildQueue" body ||= %Q(<build branchName=\"#{branch}\"><buildType id=\"foo\"/></build>) auth = %w(mic password) diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb index 122340f7bec..0cb92f81da2 100644 --- a/spec/models/packages/package_spec.rb +++ b/spec/models/packages/package_spec.rb @@ -1201,6 +1201,30 @@ RSpec.describe Packages::Package, type: :model do end end + describe '#mark_package_files_for_destruction' do + let_it_be(:package) { create(:npm_package, :pending_destruction) } + + subject { package.mark_package_files_for_destruction } + + it 'enqueues a sync worker job' do + expect(::Packages::MarkPackageFilesForDestructionWorker) + .to receive(:perform_async).with(package.id) + + subject + end + + context 'for a package non pending destruction' do + let_it_be(:package) { create(:npm_package) } + + it 'does not enqueues a sync worker job' do + expect(::Packages::MarkPackageFilesForDestructionWorker) + .not_to receive(:perform_async).with(package.id) + + subject + end + end + end + describe '#create_build_infos!' do let_it_be(:package) { create(:package) } let_it_be(:pipeline) { create(:ci_pipeline) } diff --git a/spec/requests/api/graphql/mutations/packages/destroy_file_spec.rb b/spec/requests/api/graphql/mutations/packages/destroy_file_spec.rb index 7be629f8f4b..cd25aba9e00 100644 --- a/spec/requests/api/graphql/mutations/packages/destroy_file_spec.rb +++ b/spec/requests/api/graphql/mutations/packages/destroy_file_spec.rb @@ -24,19 +24,16 @@ RSpec.describe 'Destroying a package file' do let(:mutation_response) { graphql_mutation_response(:destroyPackageFile) } shared_examples 'destroying the package file' do - it 'destroy the package file' do - expect { mutation_request }.to change { ::Packages::PackageFile.count }.by(-1) + it 'marks the package file as pending destruction' do + expect { mutation_request }.to change { ::Packages::PackageFile.pending_destruction.count }.by(1) end it_behaves_like 'returning response status', :success end shared_examples 'denying the mutation request' do - it 'does not destroy the package file' do - expect(::Packages::PackageFile) - .not_to receive(:destroy) - - expect { mutation_request }.not_to change { ::Packages::PackageFile.count } + it 'does not mark the package file as pending destruction' do + expect { mutation_request }.not_to change { ::Packages::PackageFile.pending_destruction.count } expect(mutation_response).to be_nil end @@ -71,7 +68,7 @@ RSpec.describe 'Destroying a package file' do it_behaves_like 'denying the mutation request' end - context 'when an error occures' do + context 'when an error occurs' do let(:error_messages) { ['some error'] } before do @@ -80,7 +77,7 @@ RSpec.describe 'Destroying a package file' do it 'returns the errors in the response' do allow_next_found_instance_of(::Packages::PackageFile) do |package_file| - allow(package_file).to receive(:destroy).and_return(false) + allow(package_file).to receive(:update).with(status: :pending_destruction).and_return(false) allow(package_file).to receive_message_chain(:errors, :full_messages).and_return(error_messages) end diff --git a/spec/requests/api/graphql/mutations/packages/destroy_spec.rb b/spec/requests/api/graphql/mutations/packages/destroy_spec.rb index e5ced419ecf..2340a6a36d8 100644 --- a/spec/requests/api/graphql/mutations/packages/destroy_spec.rb +++ b/spec/requests/api/graphql/mutations/packages/destroy_spec.rb @@ -24,22 +24,27 @@ RSpec.describe 'Destroying a package' do let(:mutation_response) { graphql_mutation_response(:destroyPackage) } shared_examples 'destroying the package' do - it 'destroy the package' do - expect(::Packages::DestroyPackageService) + it 'marks the package as pending destruction' do + expect(::Packages::MarkPackageForDestructionService) .to receive(:new).with(container: package, current_user: user).and_call_original + expect_next_found_instance_of(::Packages::Package) do |package| + expect(package).to receive(:mark_package_files_for_destruction) + end - expect { mutation_request }.to change { ::Packages::Package.count }.by(-1) + expect { mutation_request } + .to change { ::Packages::Package.pending_destruction.count }.by(1) end it_behaves_like 'returning response status', :success end shared_examples 'denying the mutation request' do - it 'does not destroy the package' do - expect(::Packages::DestroyPackageService) + it 'does not mark the package as pending destruction' do + expect(::Packages::MarkPackageForDestructionService) .not_to receive(:new).with(container: package, current_user: user) - expect { mutation_request }.not_to change { ::Packages::Package.count } + expect { mutation_request } + .to not_change { ::Packages::Package.pending_destruction.count } expect(mutation_response).to be_nil end @@ -81,12 +86,12 @@ RSpec.describe 'Destroying a package' do it 'returns the errors in the response' do allow_next_found_instance_of(::Packages::Package) do |package| - allow(package).to receive(:destroy!).and_raise(StandardError) + allow(package).to receive(:pending_destruction!).and_raise(StandardError) end mutation_request - expect(mutation_response['errors']).to eq(['Failed to remove the package']) + expect(mutation_response['errors']).to match_array(['Failed to mark the package as pending destruction']) end end end diff --git a/spec/requests/api/graphql/project/cluster_agents_spec.rb b/spec/requests/api/graphql/project/cluster_agents_spec.rb index 585126f3849..c9900fea277 100644 --- a/spec/requests/api/graphql/project/cluster_agents_spec.rb +++ b/spec/requests/api/graphql/project/cluster_agents_spec.rb @@ -126,7 +126,7 @@ RSpec.describe 'Project.cluster_agents' do }) end - it 'preloads associations to prevent N+1 queries' do + it 'preloads associations to prevent N+1 queries', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/350868' do user = create(:user) token = create(:cluster_agent_token, agent: agents.second) create(:agent_activity_event, agent: agents.second, agent_token: token, user: user) diff --git a/spec/requests/api/package_files_spec.rb b/spec/requests/api/package_files_spec.rb index 7a6b1599154..a7e6a97fd0e 100644 --- a/spec/requests/api/package_files_spec.rb +++ b/spec/requests/api/package_files_spec.rb @@ -114,14 +114,14 @@ RSpec.describe API::PackageFiles do let(:user) { nil } it 'returns 403 for non authenticated user', :aggregate_failures do - expect { api_request }.not_to change { package.package_files.count } + expect { api_request }.not_to change { package.package_files.pending_destruction.count } expect(response).to have_gitlab_http_status(:forbidden) end end it 'returns 403 for a user without access to the project', :aggregate_failures do - expect { api_request }.not_to change { package.package_files.count } + expect { api_request }.not_to change { package.package_files.pending_destruction.count } expect(response).to have_gitlab_http_status(:forbidden) end @@ -131,7 +131,7 @@ RSpec.describe API::PackageFiles do let_it_be_with_refind(:project) { create(:project, :private) } it 'returns 404 for a user without access to the project', :aggregate_failures do - expect { api_request }.not_to change { package.package_files.count } + expect { api_request }.not_to change { package.package_files.pending_destruction.count } expect(response).to have_gitlab_http_status(:not_found) end @@ -139,7 +139,7 @@ RSpec.describe API::PackageFiles do it 'returns 403 for a user without enough permissions', :aggregate_failures do project.add_developer(user) - expect { api_request }.not_to change { package.package_files.count } + expect { api_request }.not_to change { package.package_files.pending_destruction.count } expect(response).to have_gitlab_http_status(:forbidden) end @@ -147,7 +147,7 @@ RSpec.describe API::PackageFiles do it 'returns 204', :aggregate_failures do project.add_maintainer(user) - expect { api_request }.to change { package.package_files.count }.by(-1) + expect { api_request }.to change { package.package_files.pending_destruction.count }.by(1) expect(response).to have_gitlab_http_status(:no_content) end @@ -156,7 +156,7 @@ RSpec.describe API::PackageFiles do let(:user) { nil } it 'returns 404 for non authenticated user', :aggregate_failures do - expect { api_request }.not_to change { package.package_files.count } + expect { api_request }.not_to change { package.package_files.pending_destruction.count } expect(response).to have_gitlab_http_status(:not_found) end @@ -168,7 +168,7 @@ RSpec.describe API::PackageFiles do it 'returns 404 when the package file does not exist', :aggregate_failures do project.add_maintainer(user) - expect { api_request }.not_to change { package.package_files.count } + expect { api_request }.not_to change { package.package_files.pending_destruction.count } expect(response).to have_gitlab_http_status(:not_found) end @@ -182,7 +182,7 @@ RSpec.describe API::PackageFiles do end it 'can not be accessed', :aggregate_failures do - expect { api_request }.not_to change { package.package_files.count } + expect { api_request }.not_to change { package.package_files.pending_destruction.count } expect(response).to have_gitlab_http_status(:not_found) end @@ -193,7 +193,7 @@ RSpec.describe API::PackageFiles do end it 'can be accessed', :aggregate_failures do - expect { api_request }.to change { package.package_files.count }.by(-1) + expect { api_request }.not_to change { package.package_files.pending_destruction.count } expect(response).to have_gitlab_http_status(:no_content) end diff --git a/spec/requests/api/project_packages_spec.rb b/spec/requests/api/project_packages_spec.rb index 9b7538547f6..5f4b8899a33 100644 --- a/spec/requests/api/project_packages_spec.rb +++ b/spec/requests/api/project_packages_spec.rb @@ -293,13 +293,13 @@ RSpec.describe API::ProjectPackages do context 'without the need for a license' do context 'project is public' do it 'returns 403 for non authenticated user' do - delete api(package_url) + expect { delete api(package_url) }.not_to change { ::Packages::Package.pending_destruction.count } expect(response).to have_gitlab_http_status(:forbidden) end it 'returns 403 for a user without access to the project' do - delete api(package_url, user) + expect { delete api(package_url, user) }.not_to change { ::Packages::Package.pending_destruction.count } expect(response).to have_gitlab_http_status(:forbidden) end @@ -313,13 +313,13 @@ RSpec.describe API::ProjectPackages do end it 'returns 404 for non authenticated user' do - delete api(package_url) + expect { delete api(package_url) }.not_to change { ::Packages::Package.pending_destruction.count } expect(response).to have_gitlab_http_status(:not_found) end it 'returns 404 for a user without access to the project' do - delete api(package_url, user) + expect { delete api(package_url, user) }.not_to change { ::Packages::Package.pending_destruction.count } expect(response).to have_gitlab_http_status(:not_found) end @@ -327,7 +327,7 @@ RSpec.describe API::ProjectPackages do it 'returns 404 when the package does not exist' do project.add_maintainer(user) - delete api(no_package_url, user) + expect { delete api(no_package_url, user) }.not_to change { ::Packages::Package.pending_destruction.count } expect(response).to have_gitlab_http_status(:not_found) end @@ -335,7 +335,7 @@ RSpec.describe API::ProjectPackages do it 'returns 404 for the package from a different project' do project.add_maintainer(user) - delete api(wrong_package_url, user) + expect { delete api(wrong_package_url, user) }.not_to change { ::Packages::Package.pending_destruction.count } expect(response).to have_gitlab_http_status(:not_found) end @@ -343,7 +343,7 @@ RSpec.describe API::ProjectPackages do it 'returns 403 for a user without enough permissions' do project.add_developer(user) - delete api(package_url, user) + expect { delete api(package_url, user) }.not_to change { ::Packages::Package.pending_destruction.count } expect(response).to have_gitlab_http_status(:forbidden) end @@ -351,7 +351,7 @@ RSpec.describe API::ProjectPackages do it 'returns 204' do project.add_maintainer(user) - delete api(package_url, user) + expect { delete api(package_url, user) }.to change { ::Packages::Package.pending_destruction.count }.by(1) expect(response).to have_gitlab_http_status(:no_content) end diff --git a/spec/requests/jira_connect/users_controller_spec.rb b/spec/requests/jira_connect/users_controller_spec.rb new file mode 100644 index 00000000000..c648d28c1bc --- /dev/null +++ b/spec/requests/jira_connect/users_controller_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe JiraConnect::UsersController do + describe 'GET /-/jira_connect/users' do + let_it_be(:user) { create(:user) } + + before do + sign_in(user) + end + + context 'with a valid host' do + let(:return_to) { 'https://testcompany.atlassian.net/plugins/servlet/ac/gitlab-jira-connect-staging.gitlab.com/gitlab-configuration' } + + it 'includes a return url' do + get '/-/jira_connect/users', params: { return_to: return_to } + + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to include('Return to GitLab') + end + end + + context 'with an invalid host' do + let(:return_to) { 'https://evil.com' } + + it 'does not include a return url' do + get '/-/jira_connect/users', params: { return_to: return_to } + + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).not_to include('Return to GitLab') + end + end + end +end diff --git a/spec/services/packages/mark_package_files_for_destruction_service_spec.rb b/spec/services/packages/mark_package_files_for_destruction_service_spec.rb new file mode 100644 index 00000000000..a836de1f7f6 --- /dev/null +++ b/spec/services/packages/mark_package_files_for_destruction_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::MarkPackageFilesForDestructionService, :aggregate_failures do + let(:service) { described_class.new(package_files) } + + describe '#execute', :aggregate_failures do + subject { service.execute } + + shared_examples 'executing successfully' do + it 'marks package files for destruction' do + expect { subject } + .to change { ::Packages::PackageFile.pending_destruction.count }.by(package_files.size) + end + + it 'executes successfully' do + expect(subject).to be_success + expect(subject.message).to eq('Package files are now pending destruction') + end + end + + context 'with no package files' do + let_it_be(:package_files) { ::Packages::PackageFile.none } + + it_behaves_like 'executing successfully' + end + + context 'with a single package file' do + let_it_be(:package_file) { create(:package_file) } + let_it_be(:package_files) { ::Packages::PackageFile.id_in(package_file.id) } + + it_behaves_like 'executing successfully' + end + + context 'with many package files' do + let_it_be(:package_files) { ::Packages::PackageFile.id_in(create_list(:package_file, 3).map(&:id)) } + + it_behaves_like 'executing successfully' + end + + context 'with an error during the update' do + let_it_be(:package_files) { ::Packages::PackageFile.none } + + before do + expect(package_files).to receive(:each_batch).and_raise('error!') + end + + it 'raises the error' do + expect { subject } + .to raise_error('error!') + .and not_change { ::Packages::PackageFile.pending_destruction.count } + end + end + end +end diff --git a/spec/services/packages/destroy_package_service_spec.rb b/spec/services/packages/mark_package_for_destruction_service_spec.rb index 92db8da968c..125ec53ad61 100644 --- a/spec/services/packages/destroy_package_service_spec.rb +++ b/spec/services/packages/mark_package_for_destruction_service_spec.rb @@ -2,10 +2,9 @@ require 'spec_helper' -RSpec.describe Packages::DestroyPackageService do +RSpec.describe Packages::MarkPackageForDestructionService do let_it_be(:user) { create(:user) } - - let!(:package) { create(:npm_package) } + let_it_be_with_reload(:package) { create(:npm_package) } describe '#execute' do subject(:service) { described_class.new(container: package, current_user: user) } @@ -15,10 +14,11 @@ RSpec.describe Packages::DestroyPackageService do package.project.add_maintainer(user) end - context 'when the destroy is successfull' do - it 'destroy the package' do + context 'when it is successful' do + it 'marks the package and package files as pending destruction' do expect(package).to receive(:sync_maven_metadata).and_call_original - expect { service.execute }.to change { Packages::Package.count }.by(-1) + expect(package).to receive(:mark_package_files_for_destruction).and_call_original + expect { service.execute }.to change { package.status }.from('default').to('pending_destruction') end it 'returns a success ServiceResponse' do @@ -26,13 +26,13 @@ RSpec.describe Packages::DestroyPackageService do expect(response).to be_a(ServiceResponse) expect(response).to be_success - expect(response.message).to eq("Package was successfully deleted") + expect(response.message).to eq("Package was successfully marked as pending destruction") end end - context 'when the destroy is not successful' do + context 'when it is not successful' do before do - allow(package).to receive(:destroy!).and_raise(StandardError, "test") + allow(package).to receive(:pending_destruction!).and_raise(StandardError, "test") end it 'returns an error ServiceResponse' do @@ -41,7 +41,7 @@ RSpec.describe Packages::DestroyPackageService do expect(package).not_to receive(:sync_maven_metadata) expect(response).to be_a(ServiceResponse) expect(response).to be_error - expect(response.message).to eq("Failed to remove the package") + expect(response.message).to eq("Failed to mark the package as pending destruction") expect(response.status).to eq(:error) end end diff --git a/spec/services/protected_branches/create_service_spec.rb b/spec/services/protected_branches/create_service_spec.rb index 0bea3edf203..3ac42d41377 100644 --- a/spec/services/protected_branches/create_service_spec.rb +++ b/spec/services/protected_branches/create_service_spec.rb @@ -24,38 +24,14 @@ RSpec.describe ProtectedBranches::CreateService do expect(project.protected_branches.last.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) end - context 'when name has escaped HTML' do - let(:name) { 'feature->test' } + context 'when protecting a branch with a name that contains HTML tags' do + let(:name) { 'foo<b>bar<\b>' } - it 'creates the new protected branch matching the unescaped version' do - expect { service.execute }.to change(ProtectedBranch, :count).by(1) - expect(project.protected_branches.last.name).to eq('feature->test') - end - - context 'and name contains HTML tags' do - let(:name) { '<b>master</b>' } - - it 'creates the new protected branch with sanitized name' do - expect { service.execute }.to change(ProtectedBranch, :count).by(1) - expect(project.protected_branches.last.name).to eq('master') - end - - context 'and contains unsafe HTML' do - let(:name) { '<script>alert('foo');</script>' } + subject(:service) { described_class.new(project, user, params) } - it 'does not create the new protected branch' do - expect { service.execute }.not_to change(ProtectedBranch, :count) - end - end - end - - context 'when name contains unescaped HTML tags' do - let(:name) { '<b>master</b>' } - - it 'creates the new protected branch with sanitized name' do - expect { service.execute }.to change(ProtectedBranch, :count).by(1) - expect(project.protected_branches.last.name).to eq('master') - end + it 'creates a new protected branch' do + expect { service.execute }.to change(ProtectedBranch, :count).by(1) + expect(project.protected_branches.last.name).to eq(name) end end diff --git a/spec/services/protected_branches/update_service_spec.rb b/spec/services/protected_branches/update_service_spec.rb index 3d9b77dcfc0..4405af35c37 100644 --- a/spec/services/protected_branches/update_service_spec.rb +++ b/spec/services/protected_branches/update_service_spec.rb @@ -18,35 +18,14 @@ RSpec.describe ProtectedBranches::UpdateService do expect(result.reload.name).to eq(params[:name]) end - context 'when name has escaped HTML' do - let(:new_name) { 'feature->test' } + context 'when updating name of a protected branch to one that contains HTML tags' do + let(:new_name) { 'foo<b>bar<\b>' } + let(:result) { service.execute(protected_branch) } - it 'updates protected branch name with unescaped HTML' do - expect(result.reload.name).to eq('feature->test') - end - - context 'and name contains HTML tags' do - let(:new_name) { '<b>master</b>' } - - it 'updates protected branch name with sanitized name' do - expect(result.reload.name).to eq('master') - end - - context 'and contains unsafe HTML' do - let(:new_name) { '<script>alert('foo');</script>' } - - it 'does not update the protected branch' do - expect(result.reload.name).to eq(protected_branch.name) - end - end - end - end - - context 'when name contains unescaped HTML tags' do - let(:new_name) { '<b>master</b>' } + subject(:service) { described_class.new(project, user, params) } - it 'updates protected branch name with sanitized name' do - expect(result.reload.name).to eq('master') + it 'updates a protected branch' do + expect(result.reload.name).to eq(new_name) end end diff --git a/spec/services/protected_tags/create_service_spec.rb b/spec/services/protected_tags/create_service_spec.rb index 31059d17f10..a0b99b595e3 100644 --- a/spec/services/protected_tags/create_service_spec.rb +++ b/spec/services/protected_tags/create_service_spec.rb @@ -22,38 +22,14 @@ RSpec.describe ProtectedTags::CreateService do expect(project.protected_tags.last.create_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) end - context 'when name has escaped HTML' do - let(:name) { 'tag->test' } + context 'protecting a tag with a name that contains HTML tags' do + let(:name) { 'foo<b>bar<\b>' } - it 'creates the new protected tag matching the unescaped version' do - expect { service.execute }.to change(ProtectedTag, :count).by(1) - expect(project.protected_tags.last.name).to eq('tag->test') - end - - context 'and name contains HTML tags' do - let(:name) { '<b>tag</b>' } - - it 'creates the new protected tag with sanitized name' do - expect { service.execute }.to change(ProtectedTag, :count).by(1) - expect(project.protected_tags.last.name).to eq('tag') - end - - context 'and contains unsafe HTML' do - let(:name) { '<script>alert('foo');</script>' } + subject(:service) { described_class.new(project, user, params) } - it 'does not create the new protected tag' do - expect { service.execute }.not_to change(ProtectedTag, :count) - end - end - end - - context 'when name contains unescaped HTML tags' do - let(:name) { '<b>tag</b>' } - - it 'creates the new protected tag with sanitized name' do - expect { service.execute }.to change(ProtectedTag, :count).by(1) - expect(project.protected_tags.last.name).to eq('tag') - end + it 'creates a new protected tag' do + expect { service.execute }.to change(ProtectedTag, :count).by(1) + expect(project.protected_tags.last.name).to eq(name) end end end diff --git a/spec/services/protected_tags/update_service_spec.rb b/spec/services/protected_tags/update_service_spec.rb index 8d301dcd825..4b6e726bb6e 100644 --- a/spec/services/protected_tags/update_service_spec.rb +++ b/spec/services/protected_tags/update_service_spec.rb @@ -18,35 +18,14 @@ RSpec.describe ProtectedTags::UpdateService do expect(result.reload.name).to eq(params[:name]) end - context 'when name has escaped HTML' do - let(:new_name) { 'tag->test' } + context 'when updating protected tag with a name that contains HTML tags' do + let(:new_name) { 'foo<b>bar<\b>' } + let(:result) { service.execute(protected_tag) } - it 'updates protected tag name with unescaped HTML' do - expect(result.reload.name).to eq('tag->test') - end - - context 'and name contains HTML tags' do - let(:new_name) { '<b>tag</b>' } - - it 'updates protected tag name with sanitized name' do - expect(result.reload.name).to eq('tag') - end - - context 'and contains unsafe HTML' do - let(:new_name) { '<script>alert('foo');</script>' } - - it 'does not update the protected tag' do - expect(result.reload.name).to eq(protected_tag.name) - end - end - end - end - - context 'when name contains unescaped HTML tags' do - let(:new_name) { '<b>tag</b>' } + subject(:service) { described_class.new(project, user, params) } - it 'updates protected tag name with sanitized name' do - expect(result.reload.name).to eq('tag') + it 'updates a protected tag' do + expect(result.reload.name).to eq(new_name) end end diff --git a/spec/support/shared_contexts/models/concerns/integrations/enable_ssl_verification_shared_context.rb b/spec/support/shared_contexts/models/concerns/integrations/enable_ssl_verification_shared_context.rb new file mode 100644 index 00000000000..c698e06c2a2 --- /dev/null +++ b/spec/support/shared_contexts/models/concerns/integrations/enable_ssl_verification_shared_context.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +RSpec.shared_context Integrations::EnableSslVerification do + # This is added to the global setup, to make sure all calls to + # `Gitlab::HTTP` in the main model spec are passing the `verify:` option. + before do + allow(Gitlab::HTTP).to receive(:perform_request) + .with(anything, anything, include(verify: true)) + .and_call_original + end + + describe 'accessors' do + it { is_expected.to respond_to(:enable_ssl_verification) } + it { is_expected.to respond_to(:enable_ssl_verification?) } + end + + describe '#initialize_properties' do + it 'enables the setting by default' do + expect(integration.enable_ssl_verification).to be(true) + end + + it 'does not enable the setting if the record is already persisted' do + allow(integration).to receive(:new_record?).and_return(false) + + integration.enable_ssl_verification = false + integration.send(:initialize_properties) + + expect(integration.enable_ssl_verification).to be(false) + end + + it 'does not enable the setting if a custom value was set' do + integration = described_class.new(enable_ssl_verification: false) + + expect(integration.enable_ssl_verification).to be(false) + end + end + + describe '#fields' do + it 'inserts the checkbox field after the first URL field, or at the end' do + names = integration.fields.pluck(:name) + url_index = names.index { |name| name.ends_with?('_url') } + insert_index = url_index ? url_index + 1 : names.size - 1 + + expect(names.index('enable_ssl_verification')).to eq insert_index + end + end +end diff --git a/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb b/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb index 1fa340a0cf4..ae72cb6ec5d 100644 --- a/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb +++ b/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb @@ -37,6 +37,16 @@ RSpec.shared_examples Integrations::HasWebHook do it 'returns a boolean' do expect(integration.hook_ssl_verification).to be_in([true, false]) end + + it 'delegates to #enable_ssl_verification if the concern is included' do + next unless integration.is_a?(Integrations::EnableSslVerification) + + [true, false].each do |value| + integration.enable_ssl_verification = value + + expect(integration.hook_ssl_verification).to be(value) + end + end end describe '#update_web_hook!' do diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index bb4e2981070..4f9c207f976 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -364,6 +364,7 @@ RSpec.describe 'Every Sidekiq worker' do 'Packages::CleanupPackageFileWorker' => 0, 'Packages::Composer::CacheUpdateWorker' => false, 'Packages::Go::SyncPackagesWorker' => 3, + 'Packages::MarkPackageFilesForDestructionWorker' => 3, 'Packages::Maven::Metadata::SyncWorker' => 3, 'Packages::Nuget::ExtractionWorker' => 3, 'Packages::Rubygems::ExtractionWorker' => 3, diff --git a/spec/workers/packages/cleanup_package_file_worker_spec.rb b/spec/workers/packages/cleanup_package_file_worker_spec.rb index b423c4d3f06..33f89826312 100644 --- a/spec/workers/packages/cleanup_package_file_worker_spec.rb +++ b/spec/workers/packages/cleanup_package_file_worker_spec.rb @@ -23,6 +23,7 @@ RSpec.describe Packages::CleanupPackageFileWorker do expect(worker).to receive(:log_extra_metadata_on_done).twice expect { subject }.to change { Packages::PackageFile.count }.by(-1) + .and not_change { Packages::Package.count } end end @@ -38,6 +39,17 @@ RSpec.describe Packages::CleanupPackageFileWorker do expect(package_file.reload).to be_error end end + + context 'removing the last package file' do + let_it_be(:package_file) { create(:package_file, :pending_destruction, package: package) } + + it 'deletes the package file and the package' do + expect(worker).to receive(:log_extra_metadata_on_done).twice + + expect { subject }.to change { Packages::PackageFile.count }.by(-1) + .and change { Packages::Package.count }.by(-1) + end + end end describe '#max_running_jobs' do diff --git a/spec/workers/packages/mark_package_files_for_destruction_worker_spec.rb b/spec/workers/packages/mark_package_files_for_destruction_worker_spec.rb new file mode 100644 index 00000000000..15d9e4c347b --- /dev/null +++ b/spec/workers/packages/mark_package_files_for_destruction_worker_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::MarkPackageFilesForDestructionWorker, :aggregate_failures do + describe '#perform' do + let_it_be(:package) { create(:package) } + let_it_be(:package_files) { create_list(:package_file, 3, package: package) } + + let(:worker) { described_class.new } + let(:job_args) { [package.id] } + + subject { worker.perform(*job_args) } + + context 'with a valid package id' do + it_behaves_like 'an idempotent worker' + + it 'marks all package files as pending_destruction' do + package_files_count = package.package_files.count + + expect(package.package_files.pending_destruction.count).to eq(0) + expect(package.package_files.default.count).to eq(package_files_count) + + subject + + expect(package.package_files.default.count).to eq(0) + expect(package.package_files.pending_destruction.count).to eq(package_files_count) + end + end + + context 'with an invalid package id' do + let(:job_args) { [non_existing_record_id] } + + it_behaves_like 'an idempotent worker' + + it 'marks no packag files' do + expect(::Packages::MarkPackageFilesForDestructionService).not_to receive(:new) + + expect { subject }.not_to change { ::Packages::PackageFile.pending_destruction.count } + end + end + + context 'with a nil package id' do + let(:job_args) { [nil] } + + it_behaves_like 'an idempotent worker' + + it 'marks no packag files' do + expect(::Packages::MarkPackageFilesForDestructionService).not_to receive(:new) + + expect { subject }.not_to change { ::Packages::PackageFile.pending_destruction.count } + end + end + end +end |