diff --git a/app/models/bulk_imports/entity.rb b/app/models/bulk_imports/entity.rb index ae2d375811070f593161cb80333c488eaf995b35..d5d1d38784ea6d82ac69ef2775ecb7cc65d85a45 100644 --- a/app/models/bulk_imports/entity.rb +++ b/app/models/bulk_imports/entity.rb @@ -44,23 +44,18 @@ class BulkImports::Entity < ApplicationRecord validates :source_full_path, presence: true, format: { with: Gitlab::Regex.bulk_import_source_full_path_regex, - message: Gitlab::Regex.bulk_import_destination_namespace_path_regex_message } + message: Gitlab::Regex.bulk_import_source_full_path_regex_message } validates :destination_name, presence: true, - format: { with: Gitlab::Regex.group_path_regex, - message: Gitlab::Regex.group_path_regex_message } + if: -> { group || project } validates :destination_namespace, exclusion: [nil], - format: { with: Gitlab::Regex.bulk_import_destination_namespace_path_regex, - message: Gitlab::Regex.bulk_import_destination_namespace_path_regex_message }, if: :group validates :destination_namespace, presence: true, - format: { with: Gitlab::Regex.bulk_import_destination_namespace_path_regex, - message: Gitlab::Regex.bulk_import_destination_namespace_path_regex_message }, if: :project validate :validate_parent_is_a_group, if: :parent diff --git a/app/services/bulk_imports/create_service.rb b/app/services/bulk_imports/create_service.rb index 348280030f4273dd5f6565eaf4b5e2a7f9312283..aec32209b19eea0550045ce81c3d73da3e32a4b6 100644 --- a/app/services/bulk_imports/create_service.rb +++ b/app/services/bulk_imports/create_service.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Entry point of the BulkImport feature. +# Entry point of the BulkImport/Direct Transfer feature. # This service receives a Gitlab Instance connection params # and a list of groups to be imported. # @@ -84,6 +84,8 @@ def create_bulk_import Array.wrap(params).each do |entity_params| track_access_level(entity_params) + validate_destination_namespace(entity_params[:destination_namespace]) + validate_destination_slug(entity_params[:destination_slug] || entity_params[:destination_name]) validate_destination_full_path(entity_params) BulkImports::Entity.create!( @@ -135,6 +137,18 @@ def source_equals_destination? credentials[:url].starts_with?(Settings.gitlab.base_url) end + def validate_destination_namespace(destination_namespace) + return if destination_namespace =~ Gitlab::Regex.bulk_import_destination_namespace_path_regex + + raise BulkImports::Error.destination_namespace_validation_failure + end + + def validate_destination_slug(destination_slug) + return if destination_slug =~ Gitlab::Regex.oci_repository_path_regex + + raise BulkImports::Error.destination_slug_validation_failure + end + def validate_destination_full_path(entity_params) source_type = entity_params[:source_type] diff --git a/lib/api/validations/validators/bulk_imports.rb b/lib/api/validations/validators/bulk_imports.rb index 4625f2f39cd954597c33ebf7b1e8f289d92e5cd5..bff3424a0ac6f83c07da3cdc137cab9aecb13551 100644 --- a/lib/api/validations/validators/bulk_imports.rb +++ b/lib/api/validations/validators/bulk_imports.rb @@ -6,13 +6,25 @@ module Validators module BulkImports class DestinationSlugPath < Grape::Validations::Base def validate_param!(attr_name, params) - unless params[attr_name] =~ Gitlab::Regex.group_path_regex # rubocop: disable Style/GuardClause + if Feature.disabled?(:restrict_special_characters_in_namespace_path) + return if params[attr_name] =~ Gitlab::Regex.group_path_regex + raise Grape::Exceptions::Validation.new( params: [@scope.full_name(attr_name)], - message: "cannot start with a dash or forward slash, or end with a period or forward slash. " \ + message: "#{Gitlab::Regex.group_path_regex_message} " \ "It can only contain alphanumeric characters, periods, underscores, and dashes. " \ - "E.g. 'destination_namespace' not 'destination/namespace'" + "For example, 'destination_namespace' not 'destination/namespace'" ) + else + return if params[attr_name] =~ Gitlab::Regex.oci_repository_path_regex + + raise Grape::Exceptions::Validation.new( + params: [@scope.full_name(attr_name)], + message: "#{Gitlab::Regex.oci_repository_path_regex_message} " \ + "It can only contain alphanumeric characters, periods, underscores, and dashes. " \ + "For example, 'destination_namespace' not 'destination/namespace'" + ) + end end end @@ -21,26 +33,24 @@ class DestinationNamespacePath < Grape::Validations::Base def validate_param!(attr_name, params) return if params[attr_name].blank? - unless params[attr_name] =~ Gitlab::Regex.bulk_import_destination_namespace_path_regex # rubocop: disable Style/GuardClause - raise Grape::Exceptions::Validation.new( - params: [@scope.full_name(attr_name)], - message: "cannot start with a dash or forward slash, or end with a period or forward slash. " \ - "It can only contain alphanumeric characters, periods, underscores, forward slashes " \ - "and dashes. E.g. 'destination_namespace' or 'destination/namespace'" - ) - end + return if params[attr_name] =~ Gitlab::Regex.bulk_import_destination_namespace_path_regex + + raise Grape::Exceptions::Validation.new( + params: [@scope.full_name(attr_name)], + message: Gitlab::Regex.bulk_import_destination_namespace_path_regex_message + ) end end class SourceFullPath < Grape::Validations::Base def validate_param!(attr_name, params) - unless params[attr_name] =~ Gitlab::Regex.bulk_import_source_full_path_regex # rubocop: disable Style/GuardClause - raise Grape::Exceptions::Validation.new( - params: [@scope.full_name(attr_name)], - message: "must be a relative path and not include protocol, sub-domain, or domain information. " \ - "E.g. 'source/full/path' not 'https://example.com/source/full/path'" \ - ) - end + return if params[attr_name] =~ Gitlab::Regex.bulk_import_source_full_path_regex + + raise Grape::Exceptions::Validation.new( + params: [@scope.full_name(attr_name)], + message: "must be a relative path and not include protocol, sub-domain, or domain information. " \ + "For example, 'source/full/path' not 'https://example.com/source/full/path'" \ + ) end end end diff --git a/lib/bulk_imports/error.rb b/lib/bulk_imports/error.rb index fb72cb61de07fd863db24aaafcbadcc641bfb2b6..616b58d1852cd2b7c2e5218967a5b400470b73e0 100644 --- a/lib/bulk_imports/error.rb +++ b/lib/bulk_imports/error.rb @@ -7,16 +7,26 @@ def self.unsupported_gitlab_version end def self.scope_validation_failure - self.new("Import aborted as the provided personal access token does not have the required 'api' scope or " \ - "is no longer valid.") + self.new("Personal access token does not have the required " \ + "'api' scope or is no longer valid.") end def self.invalid_url self.new("Invalid source URL. Enter only the base URL of the source GitLab instance.") end + def self.destination_namespace_validation_failure + self.new("Import failed. Destination group or subgroup path " \ + "#{Gitlab::Regex.bulk_import_destination_namespace_path_regex_message}") + end + + def self.destination_slug_validation_failure + self.new("Import failed. Destination URL " \ + "#{Gitlab::Regex.oci_repository_path_regex_message}") + end + def self.destination_full_path_validation_failure(full_path) - self.new("Import aborted as '#{full_path}' already exists. Change the destination and try again.") + self.new("Import failed. '#{full_path}' already exists. Change the destination and try again.") end def self.setting_not_enabled diff --git a/lib/bulk_imports/groups/stage.rb b/lib/bulk_imports/groups/stage.rb index 1cdd3bb1d65087431d048210e9c4e0ddfa44f21d..bc9d490162c9722c2431bf1e5db8b37bfe421449 100644 --- a/lib/bulk_imports/groups/stage.rb +++ b/lib/bulk_imports/groups/stage.rb @@ -19,6 +19,8 @@ class Stage < ::BulkImports::Stage # `maximum_source_version` is equal to 15.1.0, the pipeline will be executed when importing from source instances # running versions 15.1.1 (patch), 15.1.0, 15.0.1, 15.0.0, 14.10.0, etc. And it won't be executed when the source # instance version is 15.2.0, 15.2.1, 16.0.0, etc. + # + # SubGroup Entities must be imported in later stage than Project Entities to avoid `full_path` naming conflicts. def config { @@ -30,10 +32,6 @@ def config pipeline: BulkImports::Groups::Pipelines::GroupAttributesPipeline, stage: 1 }, - subgroups: { - pipeline: BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline, - stage: 1 - }, namespace_settings: { pipeline: BulkImports::Groups::Pipelines::NamespaceSettingsPipeline, stage: 1, @@ -55,6 +53,11 @@ def config pipeline: BulkImports::Common::Pipelines::BadgesPipeline, stage: 1 }, + subgroups: { + pipeline: BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline, + stage: 2 # SubGroup Entities must be imported in later stage + # to Project Entities to avoid `full_path` naming conflicts. + }, boards: { pipeline: BulkImports::Common::Pipelines::BoardsPipeline, stage: 2 diff --git a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb index 18ef460385c5d867c336eb5765e2b7746faad519..85b52117dbc1293358c7abd70a9d159f14e96f6f 100644 --- a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb +++ b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb @@ -5,6 +5,8 @@ module Groups module Transformers class GroupAttributesTransformer include BulkImports::VisibilityLevel + include BulkImports::PathNormalization + include BulkImports::Uniquify # rubocop: disable Style/IfUnlessModifier def transform(context, data) @@ -14,9 +16,11 @@ def transform(context, data) namespace = Namespace.find_by_full_path(import_entity.destination_namespace) end + path = normalize_path(import_entity.destination_slug) + params = { - 'name' => group_name(namespace, data), - 'path' => import_entity.destination_slug.parameterize, + 'name' => uniquify(namespace, data['name'], :name), + 'path' => uniquify(namespace, path, :path), 'description' => data['description'], 'lfs_enabled' => data['lfs_enabled'], 'emails_disabled' => data['emails_disabled'], @@ -57,23 +61,6 @@ def transform(context, data) params.with_indifferent_access end # rubocop: enable Style/IfUnlessModifier - - private - - def group_name(namespace, data) - if namespace.present? - namespace_children_names = namespace.children.pluck(:name) # rubocop: disable CodeReuse/ActiveRecord - - if namespace_children_names.include?(data['name']) - data['name'] = - Gitlab::Utils::Uniquify.new(1).string(-> (counter) { "#{data['name']}(#{counter})" }) do |base| - namespace_children_names.include?(base) - end - end - end - - data['name'] - end end end end diff --git a/lib/bulk_imports/path_normalization.rb b/lib/bulk_imports/path_normalization.rb new file mode 100644 index 0000000000000000000000000000000000000000..dfeef330ff88fd1bcc8c06681b277b429cd0e9be --- /dev/null +++ b/lib/bulk_imports/path_normalization.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module BulkImports + module PathNormalization + private + + def normalize_path(path) + path = path.parameterize.downcase + return path if path =~ Gitlab::Regex.oci_repository_path_regex + + # remove invalid characters from end and start of path + delete_invalid_edge_characters(delete_invalid_edge_characters(path)) + # remove invalid multiplied characters + delete_invalid_multiple_characters(path) + end + + def delete_invalid_edge_characters(path) + path.reverse! + path.each_char do |char| + break path unless char.match(Gitlab::Regex.oci_repository_path_regex).nil? + + path.delete_prefix!(char) + end + end + + def delete_invalid_multiple_characters(path) + path.gsub!('-_', '-') if path.include?('-_') + path.gsub!('_-', '-') if path.include?('_-') + path + end + end +end diff --git a/lib/bulk_imports/projects/transformers/project_attributes_transformer.rb b/lib/bulk_imports/projects/transformers/project_attributes_transformer.rb index c5ed9d42e4443bd0de42efa6c574565eaf94ae17..d36b0af88291e3ffcf7bb648a38a7aadb2e288bf 100644 --- a/lib/bulk_imports/projects/transformers/project_attributes_transformer.rb +++ b/lib/bulk_imports/projects/transformers/project_attributes_transformer.rb @@ -5,6 +5,8 @@ module Projects module Transformers class ProjectAttributesTransformer include BulkImports::VisibilityLevel + include BulkImports::PathNormalization + include BulkImports::Uniquify PROJECT_IMPORT_TYPE = 'gitlab_project_migration' @@ -12,9 +14,10 @@ def transform(context, data) project = {} entity = context.entity namespace = Namespace.find_by_full_path(entity.destination_namespace) + path = normalize_path(entity.destination_slug) - project[:name] = entity.destination_slug - project[:path] = entity.destination_slug.parameterize + project[:name] = uniquify(namespace, entity.destination_slug, :name) + project[:path] = uniquify(namespace, path, :path) project[:created_at] = data['created_at'] project[:import_type] = PROJECT_IMPORT_TYPE project[:visibility_level] = visibility_level(entity, namespace, data['visibility']) diff --git a/lib/bulk_imports/uniquify.rb b/lib/bulk_imports/uniquify.rb new file mode 100644 index 0000000000000000000000000000000000000000..a4290eb86bfdc7c9ae22c7479e5b1e062fb94d4e --- /dev/null +++ b/lib/bulk_imports/uniquify.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module BulkImports + module Uniquify + private + + def uniquify(namespace, data_item, data_type) + return data_item unless namespace.present? + + children_items = Set.new + + # index_namespaces_on_parent_id_and_id index supports this + Namespace.by_parent(namespace).each_batch do |relation| + children_items.merge(relation.pluck(data_type).to_set) # rubocop: disable CodeReuse/ActiveRecord + end + + return data_item unless children_items.include?(data_item) + + data_item = Gitlab::Utils::Uniquify.new(1).string(->(counter) { "#{data_item}_#{counter}" }) do |base| + children_items.include?(base) + end + end + end +end diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 5b235639ae8a7487c5b023bfd1306697fd0f1153..0ff7cbf24a52c80b1a0fd62d4144062f2e12aefc 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -258,38 +258,45 @@ def conan_name_regex end end - extend self - extend Packages + module BulkImports + def bulk_import_destination_namespace_path_regex + # This regexp validates the string conforms to rules for a destination_namespace path: + # i.e does not start with a non-alphanumeric character, + # contains only alphanumeric characters, forward slashes, periods, and underscores, + # does not end with a period or forward slash, and has a relative path structure + # with no http protocol chars or leading or trailing forward slashes + # eg 'source/full/path' or 'destination_namespace' not 'https://example.com/destination/namespace/path' + # the regex also allows for an empty string ('') to be accepted as this is allowed in + # a bulk_import POST request + @bulk_import_destination_namespace_path_regex ||= %r/((\A\z)|(\A[0-9a-z]*(-_.)?[0-9a-z])(\/?[0-9a-z]*[-_.]?[0-9a-z])+\z)/i + end - def bulk_import_destination_namespace_path_regex - # This regexp validates the string conforms to rules for a destination_namespace path: - # i.e does not start with a non-alphanumeric character except for periods or underscores, - # contains only alphanumeric characters, forward slashes, periods, and underscores, - # does not end with a period or forward slash, and has a relative path structure - # with no http protocol chars or leading or trailing forward slashes - # eg 'source/full/path' or 'destination_namespace' not 'https://example.com/destination/namespace/path' - # the regex also allows for an empty string ('') to be accepted as this is allowed in - # a bulk_import POST request - @bulk_import_destination_namespace_path_regex ||= %r/((\A\z)|\A([.]?)\w*([0-9a-z][-_]*)(\/?[.]?[0-9a-z][-_]*)+\z)/i - end + def bulk_import_source_full_path_regex + # This regexp validates the string conforms to rules for a source_full_path path: + # i.e does not start with a non-alphanumeric character except for periods or underscores, + # contains only alphanumeric characters, forward slashes, periods, and underscores, + # does not end with a period or forward slash, and has a relative path structure + # with no http protocol chars or leading or trailing forward slashes + # eg 'source/full/path' or 'destination_namespace' not 'https://example.com/source/full/path' + @bulk_import_source_full_path_regex ||= %r/\A([.]?)[^\W](\/?([-_.+]*)*[0-9a-z][-_]*)+\z/i + end - def bulk_import_source_full_path_regex - # This regexp validates the string conforms to rules for a source_full_path path: - # i.e does not start with a non-alphanumeric character except for periods or underscores, - # contains only alphanumeric characters, forward slashes, periods, and underscores, - # does not end with a period or forward slash, and has a relative path structure - # with no http protocol chars or leading or trailing forward slashes - # eg 'source/full/path' or 'destination_namespace' not 'https://example.com/source/full/path' - @bulk_import_source_full_path_regex ||= %r/\A([.]?)[^\W](\/?[.]?[0-9a-z][-_]*)+\z/i - end + def bulk_import_source_full_path_regex_message + bulk_import_destination_namespace_path_regex_message + end - def bulk_import_destination_namespace_path_regex_message - "cannot start with a non-alphanumeric character except for periods or underscores, " \ - "can contain only alphanumeric characters, forward slashes, periods, and underscores, " \ - "cannot end with a period or forward slash, and has a relative path structure " \ - "with no http protocol chars or leading or trailing forward slashes" \ + def bulk_import_destination_namespace_path_regex_message + "must have a relative path structure " \ + "with no HTTP protocol characters, or leading or trailing forward slashes. " \ + "Path segments must not start or end with a special character, " \ + "and must not contain consecutive special characters." + end end + extend self + extend Packages + extend BulkImports + def group_path_regex # This regexp validates the string conforms to rules for a group slug: # i.e does not start with a non-alphanumeric character except for periods or underscores, @@ -302,7 +309,7 @@ def group_path_regex def group_path_regex_message "cannot start with a non-alphanumeric character except for periods or underscores, " \ "can contain only alphanumeric characters, periods, and underscores, " \ - "cannot end with a period or forward slash, and has no leading or trailing forward slashes" \ + "cannot end with a period or forward slash, and has no leading or trailing forward slashes." \ end def project_name_regex diff --git a/spec/lib/bulk_imports/clients/http_spec.rb b/spec/lib/bulk_imports/clients/http_spec.rb index 40261947750bcc8d1a420cfd4a4401e958b736ed..aff049408e2b832685679d158cf858a861b0ccf4 100644 --- a/spec/lib/bulk_imports/clients/http_spec.rb +++ b/spec/lib/bulk_imports/clients/http_spec.rb @@ -261,7 +261,7 @@ def stub_http_get(path, query, response) .to_return(status: 401, body: "", headers: { 'Content-Type' => 'application/json' }) expect { subject.instance_version }.to raise_exception(BulkImports::Error, - "Import aborted as the provided personal access token does not have the required 'api' scope or " \ + "Personal access token does not have the required 'api' scope or " \ "is no longer valid.") end end @@ -273,7 +273,7 @@ def stub_http_get(path, query, response) .to_return(status: 403, body: "", headers: { 'Content-Type' => 'application/json' }) expect { subject.instance_version }.to raise_exception(BulkImports::Error, - "Import aborted as the provided personal access token does not have the required 'api' scope or " \ + "Personal access token does not have the required 'api' scope or " \ "is no longer valid.") end end diff --git a/spec/lib/bulk_imports/groups/pipelines/project_entities_pipeline_spec.rb b/spec/lib/bulk_imports/groups/pipelines/project_entities_pipeline_spec.rb index 395f3568913579cf198971e7198a6e5f902fc9cb..0155dc8053e7c6c2a5eb8e0dd9926aec92dea9cc 100644 --- a/spec/lib/bulk_imports/groups/pipelines/project_entities_pipeline_spec.rb +++ b/spec/lib/bulk_imports/groups/pipelines/project_entities_pipeline_spec.rb @@ -17,18 +17,18 @@ let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } - let(:extracted_data) do - BulkImports::Pipeline::ExtractedData.new(data: { - 'id' => 'gid://gitlab/Project/1234567', - 'name' => 'My Project', - 'path' => 'my-project', - 'full_path' => 'group/my-project' - }) - end - subject { described_class.new(context) } describe '#run' do + let(:extracted_data) do + BulkImports::Pipeline::ExtractedData.new(data: { + 'id' => 'gid://gitlab/Project/1234567', + 'name' => 'My Project', + 'path' => 'my-project', + 'full_path' => 'group/my-project' + }) + end + before do allow_next_instance_of(BulkImports::Common::Extractors::GraphqlExtractor) do |extractor| allow(extractor).to receive(:extract).and_return(extracted_data) diff --git a/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb b/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb index 138a92a7e6b99cb7806ac8e82d61368b3c0f7446..ab0d57f574b4f42c80d3391c01c8713d88ca802c 100644 --- a/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb +++ b/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb @@ -85,6 +85,22 @@ end end + context 'when the destination_slug has invalid characters' do + let(:entity) do + build_stubbed( + :bulk_import_entity, + bulk_import: bulk_import, + source_full_path: 'source/full/path', + destination_slug: '____destination-_slug-path----__', + destination_namespace: destination_namespace + ) + end + + it 'normalizes the path' do + expect(transformed_data[:path]).to eq('destination-slug-path') + end + end + describe 'parent group transformation' do it 'sets parent id' do expect(transformed_data['parent_id']).to eq(destination_group.id) @@ -101,45 +117,62 @@ end end - describe 'group name transformation' do - context 'when destination namespace is empty' do - before do - entity.destination_namespace = '' - end + context 'when destination namespace is empty' do + before do + entity.destination_namespace = '' + end + it 'does not transform name' do + expect(transformed_data['name']).to eq('Source Group Name') + end + end + + context 'when destination namespace is present' do + context 'when destination namespace does not have a group or project with same path' do it 'does not transform name' do expect(transformed_data['name']).to eq('Source Group Name') end end - context 'when destination namespace is present' do - context 'when destination namespace does not have a group with same name' do - it 'does not transform name' do - expect(transformed_data['name']).to eq('Source Group Name') - end + context 'when destination namespace already has a group or project with the same name' do + before do + create(:project, group: destination_group, name: 'Source Project Name', path: 'project') + create(:group, parent: destination_group, name: 'Source Group Name', path: 'group') + create(:group, parent: destination_group, name: 'Source Group Name_1', path: 'group_1') + create(:group, parent: destination_group, name: 'Source Group Name_2', path: 'group_2') end - context 'when destination namespace already have a group with the same name' do - before do - create(:group, parent: destination_group, name: 'Source Group Name', path: 'group_1') - create(:group, parent: destination_group, name: 'Source Group Name(1)', path: 'group_2') - create(:group, parent: destination_group, name: 'Source Group Name(2)', path: 'group_3') - create(:group, parent: destination_group, name: 'Source Group Name(1)(1)', path: 'group_4') - end + it 'makes the name unique by appending a counter', :aggregate_failures do + transformed_data = described_class.new.transform(context, data.merge('name' => 'Source Group Name')) + expect(transformed_data['name']).to eq('Source Group Name_3') + + transformed_data = described_class.new.transform(context, data.merge('name' => 'Source Group Name_1')) + expect(transformed_data['name']).to eq('Source Group Name_1_1') + + transformed_data = described_class.new.transform(context, data.merge('name' => 'Source Group Name_2')) + expect(transformed_data['name']).to eq('Source Group Name_2_1') - it 'makes the name unique by appeding a counter', :aggregate_failures do - transformed_data = described_class.new.transform(context, data.merge('name' => 'Source Group Name')) - expect(transformed_data['name']).to eq('Source Group Name(3)') + transformed_data = described_class.new.transform(context, data.merge('name' => 'Source Project Name')) + expect(transformed_data['name']).to eq('Source Project Name_1') + end + end + + context 'when destination namespace already has a group or project with the same path' do + before do + create(:project, group: destination_group, name: 'Source Project Name', path: 'destination-slug-path') + create(:group, parent: destination_group, name: 'Source Group Name_4', path: 'destination-slug-path_4') + create(:group, parent: destination_group, name: 'Source Group Name_2', path: 'destination-slug-path_2') + create(:group, parent: destination_group, name: 'Source Group Name_3', path: 'destination-slug-path_3') + end - transformed_data = described_class.new.transform(context, data.merge('name' => 'Source Group Name(2)')) - expect(transformed_data['name']).to eq('Source Group Name(2)(1)') + it 'makes the path unique by appending a counter', :aggregate_failures do + transformed_data = described_class.new.transform(context, data) + expect(transformed_data['path']).to eq('destination-slug-path_1') - transformed_data = described_class.new.transform(context, data.merge('name' => 'Source Group Name(1)')) - expect(transformed_data['name']).to eq('Source Group Name(1)(2)') + create(:group, parent: destination_group, name: 'Source Group Name_1', path: 'destination-slug-path_1') - transformed_data = described_class.new.transform(context, data.merge('name' => 'Source Group Name(1)(1)')) - expect(transformed_data['name']).to eq('Source Group Name(1)(1)(1)') - end + transformed_data = described_class.new.transform(context, data) + expect(transformed_data['path']).to eq('destination-slug-path_5') end end end diff --git a/spec/lib/bulk_imports/projects/pipelines/project_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/project_pipeline_spec.rb index 09385a261b6b64922d63213c71e83d218c0b03f6..82b8bb3958ae15e7da5428e1331486c22702d53c 100644 --- a/spec/lib/bulk_imports/projects/pipelines/project_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/project_pipeline_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe BulkImports::Projects::Pipelines::ProjectPipeline do +RSpec.describe BulkImports::Projects::Pipelines::ProjectPipeline, feature_category: :importers do describe '#run' do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } diff --git a/spec/lib/bulk_imports/projects/transformers/project_attributes_transformer_spec.rb b/spec/lib/bulk_imports/projects/transformers/project_attributes_transformer_spec.rb index 36dc63a9331dffea5a418479bf77573d46e62128..d64a0114f7b424894a6cffa547e86d33fff01b94 100644 --- a/spec/lib/bulk_imports/projects/transformers/project_attributes_transformer_spec.rb +++ b/spec/lib/bulk_imports/projects/transformers/project_attributes_transformer_spec.rb @@ -36,8 +36,8 @@ expect(transformed_data[:name]).to eq(entity.destination_slug) end - it 'adds path as parameterized name' do - expect(transformed_data[:path]).to eq(entity.destination_slug.parameterize) + it 'adds path as normalized name' do + expect(transformed_data[:path]).to eq(entity.destination_slug.downcase) end it 'adds import type' do @@ -86,6 +86,58 @@ end end + context 'when destination_slug has invalid characters' do + let(:entity) do + create( + :bulk_import_entity, + source_type: :project_entity, + bulk_import: bulk_import, + source_full_path: 'source/full/path', + destination_slug: '------------Destination_-Project-_Name------------', + destination_namespace: destination_namespace + ) + end + + it 'parameterizes the path' do + expect(transformed_data[:path]).to eq('destination-project-name') + end + end + + context 'when destination namespace already has a group or project with the same name' do + before do + create(:project, group: destination_group, name: 'Destination-Project-Name', path: 'project') + create(:project, group: destination_group, name: 'Destination-Project-Name_1', path: 'project_1') + end + + it 'makes the name unique by appending a counter' do + transformed_data = described_class.new.transform(context, data) + expect(transformed_data['name']).to eq('Destination-Project-Name_2') + end + end + + context 'when destination namespace already has a project with the same path' do + let(:entity) do + create( + :bulk_import_entity, + source_type: :project_entity, + bulk_import: bulk_import, + source_full_path: 'source/full/path', + destination_slug: 'destination-slug-path', + destination_namespace: destination_namespace + ) + end + + before do + create(:project, group: destination_group, name: 'Source Project Name', path: 'destination-slug-path') + create(:project, group: destination_group, name: 'Source Project Name_1', path: 'destination-slug-path_1') + end + + it 'makes the path unique by appending a counter' do + transformed_data = described_class.new.transform(context, data) + expect(transformed_data['path']).to eq('destination-slug-path_2') + end + end + describe 'visibility level' do include_examples 'visibility level settings' end diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb index d885051b93bd62c496b01d4ba2c1df7946f7e988..3192d6be737217374c12f8aa4b7de05a74da83c1 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -79,10 +79,10 @@ it { is_expected - .to eq("cannot start with a non-alphanumeric character except for periods or underscores, " \ - "can contain only alphanumeric characters, forward slashes, periods, and underscores, " \ - "cannot end with a period or forward slash, and has a relative path structure " \ - "with no http protocol chars or leading or trailing forward slashes") + .to eq("must have a relative path structure with no HTTP " \ + "protocol characters, or leading or trailing forward slashes. Path segments must not start or " \ + "end with a special character, and must not contain consecutive special characters." + ) } end @@ -101,13 +101,14 @@ it { is_expected.not_to match('good_for+you') } it { is_expected.not_to match('source/') } it { is_expected.not_to match('.source/full./path') } + it { is_expected.not_to match('.source/.full/.path') } + it { is_expected.not_to match('_source') } + it { is_expected.not_to match('.source') } it { is_expected.to match('source') } - it { is_expected.to match('.source') } - it { is_expected.to match('_source') } it { is_expected.to match('source/full') } it { is_expected.to match('source/full/path') } - it { is_expected.to match('.source/.full/.path') } + it { is_expected.to match('sou_rce/fu-ll/pa.th') } it { is_expected.to match('domain_namespace') } it { is_expected.to match('gitlab-migration-test') } it { is_expected.to match('1-project-path') } @@ -115,10 +116,22 @@ it { is_expected.to match('') } # it is possible to pass an empty string for destination_namespace in bulk_import POST request end + describe '.bulk_import_source_full_path_regex_message' do + subject { described_class.bulk_import_source_full_path_regex_message } + + it { + is_expected + .to eq( + "must have a relative path structure with no HTTP " \ + "protocol characters, or leading or trailing forward slashes. Path segments must not start or " \ + "end with a special character, and must not contain consecutive special characters." + ) + } + end + describe '.bulk_import_source_full_path_regex' do subject { described_class.bulk_import_source_full_path_regex } - it { is_expected.not_to match('?gitlab') } it { is_expected.not_to match("Users's something") } it { is_expected.not_to match('/source') } it { is_expected.not_to match('http:') } @@ -126,20 +139,32 @@ it { is_expected.not_to match('example.com/?stuff=true') } it { is_expected.not_to match('example.com:5000/?stuff=true') } it { is_expected.not_to match('http://gitlab.example/gitlab-org/manage/import/gitlab-migration-test') } - it { is_expected.not_to match('_good_for_me!') } - it { is_expected.not_to match('good_for+you') } it { is_expected.not_to match('source/') } - it { is_expected.not_to match('.source/full./path') } it { is_expected.not_to match('') } + it { is_expected.not_to match('.source/full./path') } + it { is_expected.not_to match('?gitlab') } + it { is_expected.not_to match('_good_for_me!') } + it { is_expected.not_to match('group/@*%_my_other-project-----') } + it { is_expected.not_to match('_foog-for-me!') } + it { is_expected.not_to match('.source/full/path.') } + it { is_expected.to match('good_for+you') } it { is_expected.to match('source') } it { is_expected.to match('.source') } it { is_expected.to match('_source') } it { is_expected.to match('source/full') } it { is_expected.to match('source/full/path') } - it { is_expected.to match('.source/.full/.path') } it { is_expected.to match('domain_namespace') } it { is_expected.to match('gitlab-migration-test') } + it { is_expected.to match('source/full/path-') } + it { is_expected.to match('.source/full/path') } + it { is_expected.to match('.source/.full/.path') } + it { is_expected.to match('source/full/.path') } + it { is_expected.to match('source/full/..path') } + it { is_expected.to match('source/full/---1path') } + it { is_expected.to match('source/full/-___path') } + it { is_expected.to match('source/full/path---') } + it { is_expected.to match('group/__my_other-project-----') } end describe '.group_path_regex' do diff --git a/spec/models/bulk_imports/entity_spec.rb b/spec/models/bulk_imports/entity_spec.rb index 45f120e6773bad73d8332fa42a5c83176f47d7a9..c7ace3d2b78b7e05fa99d3740bb87ee959dad440 100644 --- a/spec/models/bulk_imports/entity_spec.rb +++ b/spec/models/bulk_imports/entity_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe BulkImports::Entity, type: :model, feature_category: :importers do + subject { described_class.new(group: Group.new) } + describe 'associations' do it { is_expected.to belong_to(:bulk_import).required } it { is_expected.to belong_to(:parent) } @@ -23,35 +25,8 @@ it { is_expected.to define_enum_for(:source_type).with_values(%i[group_entity project_entity]) } context 'when formatting with regexes' do - subject { described_class.new(group: Group.new) } - - it { is_expected.to allow_values('namespace', 'parent/namespace', 'parent/group/subgroup', '').for(:destination_namespace) } - it { is_expected.not_to allow_values('parent/namespace/', '/namespace', 'parent group/subgroup', '@namespace').for(:destination_namespace) } - it { is_expected.to allow_values('source', 'source/path', 'source/full/path').for(:source_full_path) } it { is_expected.not_to allow_values('/source', 'http://source/path', 'sou rce/full/path', '').for(:source_full_path) } - - it { is_expected.to allow_values('destination', 'destination-slug', 'new-destination-slug').for(:destination_slug) } - - # it { is_expected.not_to allow_values('destination/slug', '/destination-slug', 'destination slug').for(:destination_slug) } <-- this test should - # succeed but it's failing possibly due to rspec caching. To ensure this case is covered see the more cumbersome test below: - context 'when destination_slug is invalid' do - let(:invalid_slugs) { ['destination/slug', '/destination-slug', 'destination slug'] } - let(:error_message) do - 'cannot start with a non-alphanumeric character except for periods or underscores, ' \ - 'can contain only alphanumeric characters, periods, and underscores, ' \ - 'cannot end with a period or forward slash, and has no ' \ - 'leading or trailing forward slashes' - end - - it 'raises an error' do - invalid_slugs.each do |slug| - entity = build(:bulk_import_entity, :group_entity, group: build(:group), project: nil, destination_slug: slug) - expect(entity).not_to be_valid - expect(entity.errors.errors[0].message).to include(error_message) - end - end - end end context 'when associated with a group and project' do diff --git a/spec/requests/api/bulk_imports_spec.rb b/spec/requests/api/bulk_imports_spec.rb index 70c581716ceabcc5635ebb80b72bbf5ecdfa03fa..ff76b9369af658c1ec293baa57508a9743f5b2ee 100644 --- a/spec/requests/api/bulk_imports_spec.rb +++ b/spec/requests/api/bulk_imports_spec.rb @@ -219,7 +219,7 @@ request expect(response).to have_gitlab_http_status(:bad_request) expect(json_response['error']).to eq("entities[0][source_full_path] must be a relative path and not include protocol, sub-domain, " \ - "or domain information. E.g. 'source/full/path' not 'https://example.com/source/full/path'") + "or domain information. For example, 'source/full/path' not 'https://example.com/source/full/path'") end end @@ -229,10 +229,11 @@ request expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to eq("entities[0][destination_namespace] cannot start with a dash or forward slash, " \ - "or end with a period or forward slash. It can only contain alphanumeric " \ - "characters, periods, underscores, forward slashes and dashes. " \ - "E.g. 'destination_namespace' or 'destination/namespace'") + expect(json_response['error']).to eq("entities[0][destination_namespace] must have a relative " \ + "path structure with no HTTP protocol characters, or leading or " \ + "trailing forward slashes. Path segments must not start or " \ + "end with a special character, and must not contain " \ + "consecutive special characters.") end end @@ -248,15 +249,35 @@ end context 'when the destination_slug is invalid' do - it 'returns invalid error' do + it 'returns invalid error when restricting special characters is disabled' do + Feature.disable(:restrict_special_characters_in_namespace_path) + + params[:entities][0][:destination_slug] = 'des?tin?atoi-slugg' + + request + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to include("entities[0][destination_slug] cannot start with " \ + "a non-alphanumeric character except for periods or " \ + "underscores, can contain only alphanumeric characters, " \ + "periods, and underscores, cannot end with a period or " \ + "forward slash, and has no leading or trailing forward " \ + "slashes. It can only contain alphanumeric characters, " \ + "periods, underscores, and dashes. For example, " \ + "'destination_namespace' not 'destination/namespace'") + end + + it 'returns invalid error when restricting special characters is enabled' do + Feature.enable(:restrict_special_characters_in_namespace_path) + params[:entities][0][:destination_slug] = 'des?tin?atoi-slugg' request expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to include("entities[0][destination_slug] cannot start with a dash " \ - "or forward slash, or end with a period or forward slash. " \ - "It can only contain alphanumeric characters, periods, underscores, and dashes. " \ - "E.g. 'destination_namespace' not 'destination/namespace'") + expect(json_response['error']).to include("entities[0][destination_slug] must not start or " \ + "end with a special character and must not contain " \ + "consecutive special characters. It can only contain " \ + "alphanumeric characters, periods, underscores, and " \ + "dashes. For example, 'destination_namespace' not 'destination/namespace'") end end diff --git a/spec/serializers/import/bulk_import_entity_spec.rb b/spec/serializers/import/bulk_import_entity_spec.rb index 3dfc659daf79cc3c2b360b1f7ae196e2071e2868..f2f8854174a7b45ccc2a0262c9a5d0b18813d90c 100644 --- a/spec/serializers/import/bulk_import_entity_spec.rb +++ b/spec/serializers/import/bulk_import_entity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Import::BulkImportEntity do +RSpec.describe Import::BulkImportEntity, feature_category: :importers do let(:importable_data) do { 'id' => 1, diff --git a/spec/services/bulk_imports/create_service_spec.rb b/spec/services/bulk_imports/create_service_spec.rb index 6fe9343769439a386c91c0c5e8601bca252309de..c68030a89a82813bd931ad7a7e4b4d9a6a097874 100644 --- a/spec/services/bulk_imports/create_service_spec.rb +++ b/spec/services/bulk_imports/create_service_spec.rb @@ -114,8 +114,8 @@ expect(result).to be_error expect(result.message) .to eq( - "Import aborted as the provided personal access token does not have the required 'api' scope or is " \ - "no longer valid." + "Personal access token does not " \ + "have the required 'api' scope or is no longer valid." ) end end @@ -306,11 +306,10 @@ expect(result).to be_a(ServiceResponse) expect(result).to be_error expect(result.message).to eq("Validation failed: Source full path can't be blank, " \ - "Source full path cannot start with a non-alphanumeric character except " \ - "for periods or underscores, can contain only alphanumeric characters, " \ - "forward slashes, periods, and underscores, cannot end with " \ - "a period or forward slash, and has a relative path structure " \ - "with no http protocol chars or leading or trailing forward slashes") + "Source full path must have a relative path structure with " \ + "no HTTP protocol characters, or leading or trailing forward slashes. " \ + "Path segments must not start or end with a special character, and " \ + "must not contain consecutive special characters.") end describe '#user-role' do @@ -503,6 +502,65 @@ end end + describe '.validate_destination_namespace' do + context 'when the destination_namespace is invalid' do + let(:params) do + [ + { + source_type: 'group_entity', + source_full_path: 'full/path/to/source', + destination_slug: 'destination-slug', + destination_namespace: '---destination----namespace---', + migrate_projects: migrate_projects + } + ] + end + + it 'returns ServiceResponse with an error message' do + result = subject.execute + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + expect(result.message) + .to eq( + "Import failed. Destination group or subgroup path " \ + "must have a relative path structure with no HTTP protocol characters, or leading " \ + "or trailing forward slashes. Path segments must not start or end with a special " \ + "character, and must not contain consecutive special characters." + ) + end + end + end + + describe '.validate_destination_slug' do + context 'when the destination_slug is invalid' do + let(:params) do + [ + { + source_type: 'group_entity', + source_full_path: 'full/path/to/source', + destination_slug: 'destin-*-ation-slug', + destination_namespace: 'destination_namespace', + migrate_projects: migrate_projects + } + ] + end + + it 'returns ServiceResponse with an error message' do + result = subject.execute + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + expect(result.message) + .to eq( + "Import failed. Destination URL " \ + "must not start or end with a special character and must " \ + "not contain consecutive special characters." + ) + end + end + end + describe '.validate_destination_full_path' do context 'when the source_type is a group' do context 'when the provided destination_slug already exists in the destination_namespace' do @@ -527,7 +585,7 @@ expect(result).to be_error expect(result.message) .to eq( - "Import aborted as 'parent-group/existing-subgroup' already exists. " \ + "Import failed. 'parent-group/existing-subgroup' already exists. " \ "Change the destination and try again." ) end @@ -554,7 +612,7 @@ expect(result).to be_error expect(result.message) .to eq( - "Import aborted as 'top-level-group' already exists. " \ + "Import failed. 'top-level-group' already exists. " \ "Change the destination and try again." ) end @@ -605,7 +663,7 @@ expect(result).to be_error expect(result.message) .to eq( - "Import aborted as 'existing-group/existing-project' already exists. " \ + "Import failed. 'existing-group/existing-project' already exists. " \ "Change the destination and try again." ) end