From 56e031d3032996233f2c71ba7b5e3fc398da0b53 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 3 Apr 2017 14:07:15 +0200 Subject: [PATCH 01/38] Disallow some more namespaces These routes seem to be taken --- app/validators/namespace_validator.rb | 16 ++++++++++++++++ .../30272-bvl-reject-more-namespaces.yml | 4 ++++ 2 files changed, 20 insertions(+) create mode 100644 changelogs/unreleased/30272-bvl-reject-more-namespaces.yml diff --git a/app/validators/namespace_validator.rb b/app/validators/namespace_validator.rb index 77ca033e97f..9601013f2d2 100644 --- a/app/validators/namespace_validator.rb +++ b/app/validators/namespace_validator.rb @@ -33,6 +33,22 @@ class NamespaceValidator < ActiveModel::EachValidator u unsubscribes users + api + autocomplete + search + member + explore + uploads + import + notification_settings + abuse_reports + invites + help + koding + health_check + jwt + oauth + sent_notifications ].freeze WILDCARD_ROUTES = %w[tree commits wikis new edit create update logs_tree diff --git a/changelogs/unreleased/30272-bvl-reject-more-namespaces.yml b/changelogs/unreleased/30272-bvl-reject-more-namespaces.yml new file mode 100644 index 00000000000..56bce084546 --- /dev/null +++ b/changelogs/unreleased/30272-bvl-reject-more-namespaces.yml @@ -0,0 +1,4 @@ +--- +title: Improve validation of namespace & project paths +merge_request: 10413 +author: From f76a5abb3462a4bfeacca254c0cbda4f313d4ecd Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 3 Apr 2017 15:10:30 +0200 Subject: [PATCH 02/38] Add migration to rename all namespaces with forbidden name This is based on a migration in https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2073 Rename forbidden child namespaces --- ...121055_rename_forbidden_root_namespaces.rb | 247 ++++++++++++++++++ ...52317_rename_forbidden_child_namespaces.rb | 242 +++++++++++++++++ ...405111106_rename_wildcard_project_names.rb | 85 ++++++ .../rename_forbidden_child_namespaces_spec.rb | 187 +++++++++++++ .../rename_forbidden_root_namespaces_spec.rb | 197 ++++++++++++++ 5 files changed, 958 insertions(+) create mode 100644 db/post_migrate/20170403121055_rename_forbidden_root_namespaces.rb create mode 100644 db/post_migrate/20170404152317_rename_forbidden_child_namespaces.rb create mode 100644 db/post_migrate/20170405111106_rename_wildcard_project_names.rb create mode 100644 spec/migrations/rename_forbidden_child_namespaces_spec.rb create mode 100644 spec/migrations/rename_forbidden_root_namespaces_spec.rb diff --git a/db/post_migrate/20170403121055_rename_forbidden_root_namespaces.rb b/db/post_migrate/20170403121055_rename_forbidden_root_namespaces.rb new file mode 100644 index 00000000000..fb475cae465 --- /dev/null +++ b/db/post_migrate/20170403121055_rename_forbidden_root_namespaces.rb @@ -0,0 +1,247 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RenameForbiddenRootNamespaces < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + include Gitlab::ShellAdapter + disable_ddl_transaction! + + class Namespace < ActiveRecord::Base + self.table_name = 'namespaces' + belongs_to :parent, class_name: "Namespace" + has_one :route, as: :source, autosave: true + has_many :children, class_name: "Namespace", foreign_key: :parent_id + has_many :projects + belongs_to :owner, class_name: "User" + + def full_path + if route && route.path.present? + @full_path ||= route.path + else + update_route if persisted? + + build_full_path + end + end + + def build_full_path + if parent && path + parent.full_path + '/' + path + else + path + end + end + + def update_route + prepare_route + route.save + end + + def prepare_route + route || build_route(source: self) + route.path = build_full_path + route.name = build_full_name + @full_path = nil + @full_name = nil + end + + def build_full_name + if parent && name + parent.human_name + ' / ' + name + else + name + end + end + + def human_name + owner&.name + end + end + + class Route < ActiveRecord::Base + self.table_name = 'routes' + belongs_to :source, polymorphic: true + + validates :source, presence: true + + validates :path, + length: { within: 1..255 }, + presence: true, + uniqueness: { case_sensitive: false } + end + + class Project < ActiveRecord::Base + self.table_name = 'projects' + + def repository_storage_path + Gitlab.config.repositories.storages[repository_storage]['path'] + end + end + + DOWNTIME = false + DISALLOWED_PATHS = %w[ + api + autocomplete + search + member + explore + uploads + import + notification_settings + abuse_reports + invites + help + koding + health_check + jwt + oauth + sent_notifications + ] + + def up + DISALLOWED_PATHS.each do |path| + say "Renaming namespaces called #{path}" + forbidden_namespaces_with_path(path).each do |namespace| + rename_namespace(namespace) + end + end + end + + def down + # nothing to do + end + + def rename_namespace(namespace) + old_path = namespace.path + old_full_path = namespace.full_path + # Only remove the last occurrence of the path name to get the parent namespace path + namespace_path = remove_last_occurrence(old_full_path, old_path) + new_path = rename_path(namespace_path, old_path) + new_full_path = if namespace_path.present? + File.join(namespace_path, new_path) + else + new_path + end + + Namespace.where(id: namespace).update_all(path: new_path) # skips callbacks & validations + + replace_statement = replace_sql(Route.arel_table[:path], old_full_path, new_full_path) + + update_column_in_batches(:routes, :path, replace_statement) do |table, query| + query.where(Route.arel_table[:path].matches("#{old_full_path}%")) + end + + clear_cache_for_namespace(namespace) + + # tasks here are based on `Namespace#move_dir` + move_repositories(namespace, old_full_path, new_full_path) + move_namespace_folders(uploads_dir, old_full_path, new_full_path) if file_storage? + move_namespace_folders(pages_dir, old_full_path, new_full_path) + end + + # This will replace the first occurance of a string in a column with + # the replacement + # On postgresql we can use `regexp_replace` for that. + # On mysql we remove the pattern from the beginning of the string, and + # concatenate the remaining part tot the replacement. + def replace_sql(column, pattern, replacement) + if Gitlab::Database.mysql? + substr = Arel::Nodes::NamedFunction.new("substring", [column, pattern.to_s.size + 1]) + concat = Arel::Nodes::NamedFunction.new("concat", [Arel::Nodes::Quoted.new(replacement.to_s), substr]) + Arel::Nodes::SqlLiteral.new(concat.to_sql) + else + replace = Arel::Nodes::NamedFunction.new("regexp_replace", [column, Arel::Nodes::Quoted.new(pattern.to_s), Arel::Nodes::Quoted.new(replacement.to_s)]) + Arel::Nodes::SqlLiteral.new(replace.to_sql) + end + end + + def remove_last_occurrence(string, pattern) + string.reverse.sub(pattern.reverse, "").reverse + end + + def move_namespace_folders(directory, old_relative_path, new_relative_path) + old_path = File.join(directory, old_relative_path) + return unless File.directory?(old_path) + + new_path = File.join(directory, new_relative_path) + FileUtils.mv(old_path, new_path) + end + + def move_repositories(namespace, old_full_path, new_full_path) + repo_paths_for_namespace(namespace).each do |repository_storage_path| + # Ensure old directory exists before moving it + gitlab_shell.add_namespace(repository_storage_path, old_full_path) + + unless gitlab_shell.mv_namespace(repository_storage_path, old_full_path, new_full_path) + say "Exception moving path #{repository_storage_path} from #{old_full_path} to #{new_full_path}" + end + end + end + + def rename_path(namespace_path, path_was) + counter = 0 + path = "#{path_was}#{counter}" + + while route_exists?(File.join(namespace_path, path)) + counter += 1 + path = "#{path_was}#{counter}" + end + + path + end + + def route_exists?(full_path) + Route.where(Route.arel_table[:path].matches(full_path)).any? + end + + def forbidden_namespaces_with_path(name) + Namespace.where(arel_table[:path].matches(name).and(arel_table[:parent_id].eq(nil))) + end + + def clear_cache_for_namespace(namespace) + project_ids = project_ids_for_namespace(namespace) + scopes = { "Project" => { id: project_ids }, + "Issue" => { project_id: project_ids }, + "MergeRequest" => { target_project_id: project_ids }, + "Note" => { project_id: project_ids } } + + ClearDatabaseCacheWorker.perform_async(scopes) + rescue => e + Rails.logger.error ["Couldn't clear the markdown cache: #{e.message}", e.backtrace.join("\n")].join("\n") + end + + def project_ids_for_namespace(namespace) + namespace_ids = child_ids_for_parent(namespace, ids: [namespace.id]) + namespace_or_children = Project.arel_table[:namespace_id].in(namespace_ids) + Project.unscoped.where(namespace_or_children).pluck(:id) + end + + # This won't scale to huge trees, but it should do for a handful of namespaces + def child_ids_for_parent(namespace, ids: []) + namespace.children.each do |child| + ids << child.id + child_ids_for_parent(child, ids: ids) if child.children.any? + end + ids + end + + def repo_paths_for_namespace(namespace) + namespace.projects.unscoped.select('distinct(repository_storage)').to_a.map(&:repository_storage_path) + end + + def uploads_dir + File.join(Rails.root, "public", "uploads") + end + + def pages_dir + Settings.pages.path + end + + def file_storage? + CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File + end + + def arel_table + Namespace.arel_table + end +end diff --git a/db/post_migrate/20170404152317_rename_forbidden_child_namespaces.rb b/db/post_migrate/20170404152317_rename_forbidden_child_namespaces.rb new file mode 100644 index 00000000000..8b082a892d4 --- /dev/null +++ b/db/post_migrate/20170404152317_rename_forbidden_child_namespaces.rb @@ -0,0 +1,242 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RenameForbiddenChildNamespaces < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + include Gitlab::ShellAdapter + disable_ddl_transaction! + + class Namespace < ActiveRecord::Base + self.table_name = 'namespaces' + belongs_to :parent, class_name: "Namespace" + has_one :route, as: :source, autosave: true + has_many :children, class_name: "Namespace", foreign_key: :parent_id + has_many :projects + belongs_to :owner, class_name: "User" + + def full_path + if route && route.path.present? + @full_path ||= route.path + else + update_route if persisted? + + build_full_path + end + end + + def build_full_path + if parent && path + parent.full_path + '/' + path + else + path + end + end + + def update_route + prepare_route + route.save + end + + def prepare_route + route || build_route(source: self) + route.path = build_full_path + route.name = build_full_name + @full_path = nil + @full_name = nil + end + + def build_full_name + if parent && name + parent.human_name + ' / ' + name + else + name + end + end + + def human_name + owner&.name + end + end + + class Route < ActiveRecord::Base + self.table_name = 'routes' + belongs_to :source, polymorphic: true + + validates :source, presence: true + + validates :path, + length: { within: 1..255 }, + presence: true, + uniqueness: { case_sensitive: false } + end + + class Project < ActiveRecord::Base + self.table_name = 'projects' + + def repository_storage_path + Gitlab.config.repositories.storages[repository_storage]['path'] + end + end + + DOWNTIME = false + DISALLOWED_PATHS = %w[info git-upload-pack + git-receive-pack gitlab-lfs autocomplete_sources + templates avatar commit pages compare network snippets + services mattermost deploy_keys forks import merge_requests + branches merged_branches tags protected_branches variables + triggers pipelines environments cycle_analytics builds + hooks container_registry milestones labels issues + project_members group_links notes noteable boards todos + uploads runners runner_projects settings repository + transfer remove_fork archive unarchive housekeeping + toggle_star preview_markdown export remove_export + generate_new_export download_export activity + new_issue_address] + + def up + DISALLOWED_PATHS.each do |path| + say "Renaming namespaces called #{path}" + forbidden_namespaces_with_path(path).each do |namespace| + rename_namespace(namespace) + end + end + end + + def down + # nothing to do + end + + def rename_namespace(namespace) + old_path = namespace.path + old_full_path = namespace.full_path + # Only remove the last occurrence of the path name to get the parent namespace path + namespace_path = remove_last_occurrence(old_full_path, old_path) + new_path = rename_path(namespace_path, old_path) + new_full_path = if namespace_path.present? + File.join(namespace_path, new_path) + else + new_path + end + + Namespace.where(id: namespace).update_all(path: new_path) # skips callbacks & validations + + replace_statement = replace_sql(Route.arel_table[:path], old_full_path, new_full_path) + + update_column_in_batches(:routes, :path, replace_statement) do |table, query| + query.where(Route.arel_table[:path].matches("#{old_full_path}%")) + end + + clear_cache_for_namespace(namespace) + + # tasks here are based on `Namespace#move_dir` + move_repositories(namespace, old_full_path, new_full_path) + move_namespace_folders(uploads_dir, old_full_path, new_full_path) if file_storage? + move_namespace_folders(pages_dir, old_full_path, new_full_path) + end + + # This will replace the first occurance of a string in a column with + # the replacement + # On postgresql we can use `regexp_replace` for that. + # On mysql we remove the pattern from the beginning of the string, and + # concatenate the remaining part tot the replacement. + def replace_sql(column, pattern, replacement) + if Gitlab::Database.mysql? + substr = Arel::Nodes::NamedFunction.new("substring", [column, pattern.to_s.size + 1]) + concat = Arel::Nodes::NamedFunction.new("concat", [Arel::Nodes::Quoted.new(replacement.to_s), substr]) + Arel::Nodes::SqlLiteral.new(concat.to_sql) + else + replace = Arel::Nodes::NamedFunction.new("regexp_replace", [column, Arel::Nodes::Quoted.new(pattern.to_s), Arel::Nodes::Quoted.new(replacement.to_s)]) + Arel::Nodes::SqlLiteral.new(replace.to_sql) + end + end + + def remove_last_occurrence(string, pattern) + string.reverse.sub(pattern.reverse, "").reverse + end + + def move_namespace_folders(directory, old_relative_path, new_relative_path) + old_path = File.join(directory, old_relative_path) + return unless File.directory?(old_path) + + new_path = File.join(directory, new_relative_path) + FileUtils.mv(old_path, new_path) + end + + def move_repositories(namespace, old_full_path, new_full_path) + repo_paths_for_namespace(namespace).each do |repository_storage_path| + # Ensure old directory exists before moving it + gitlab_shell.add_namespace(repository_storage_path, old_full_path) + + unless gitlab_shell.mv_namespace(repository_storage_path, old_full_path, new_full_path) + say "Exception moving path #{repository_storage_path} from #{old_full_path} to #{new_full_path}" + end + end + end + + def rename_path(namespace_path, path_was) + counter = 0 + path = "#{path_was}#{counter}" + + while route_exists?(File.join(namespace_path, path)) + counter += 1 + path = "#{path_was}#{counter}" + end + + path + end + + def route_exists?(full_path) + Route.where(Route.arel_table[:path].matches(full_path)).any? + end + + def forbidden_namespaces_with_path(path) + Namespace.where(arel_table[:parent_id].eq(nil).not).where(arel_table[:path].matches(path)) + end + + def clear_cache_for_namespace(namespace) + project_ids = project_ids_for_namespace(namespace) + scopes = { "Project" => { id: project_ids }, + "Issue" => { project_id: project_ids }, + "MergeRequest" => { target_project_id: project_ids }, + "Note" => { project_id: project_ids } } + + ClearDatabaseCacheWorker.perform_async(scopes) + rescue => e + Rails.logger.error ["Couldn't clear the markdown cache: #{e.message}", e.backtrace.join("\n")].join("\n") + end + + def project_ids_for_namespace(namespace) + namespace_ids = child_ids_for_parent(namespace, ids: [namespace.id]) + namespace_or_children = Project.arel_table[:namespace_id].in(namespace_ids) + Project.unscoped.where(namespace_or_children).pluck(:id) + end + + # This won't scale to huge trees, but it should do for a handful of namespaces + def child_ids_for_parent(namespace, ids: []) + namespace.children.each do |child| + ids << child.id + child_ids_for_parent(child, ids: ids) if child.children.any? + end + ids + end + + def repo_paths_for_namespace(namespace) + namespace.projects.unscoped.select('distinct(repository_storage)').to_a.map(&:repository_storage_path) + end + + def uploads_dir + File.join(Rails.root, "public", "uploads") + end + + def pages_dir + Settings.pages.path + end + + def file_storage? + CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File + end + + def arel_table + Namespace.arel_table + end +end diff --git a/db/post_migrate/20170405111106_rename_wildcard_project_names.rb b/db/post_migrate/20170405111106_rename_wildcard_project_names.rb new file mode 100644 index 00000000000..1b8d2a40e99 --- /dev/null +++ b/db/post_migrate/20170405111106_rename_wildcard_project_names.rb @@ -0,0 +1,85 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RenameWildcardProjectNames < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + include Gitlab::ShellAdapter + disable_ddl_transaction! + + DOWNTIME = false + KNOWN_PATHS = %w[info git-upload-pack + git-receive-pack gitlab-lfs autocomplete_sources + templates avatar commit pages compare network snippets + services mattermost deploy_keys forks import merge_requests + branches merged_branches tags protected_branches variables + triggers pipelines environments cycle_analytics builds + hooks container_registry milestones labels issues + project_members group_links notes noteable boards todos + uploads runners runner_projects settings repository + transfer remove_fork archive unarchive housekeeping + toggle_star preview_markdown export remove_export + generate_new_export download_export activity + new_issue_address].freeze + + def up + reserved_projects.find_in_batches(batch_size: 100) do |slice| + rename_projects(slice) + end + end + + def down + # nothing to do here + end + + private + + def reserved_projects + Project.unscoped. + includes(:namespace). + where('EXISTS (SELECT 1 FROM namespaces WHERE projects.namespace_id = namespaces.id)'). + where('projects.path' => KNOWN_PATHS) + end + + def route_exists?(full_path) + quoted_path = ActiveRecord::Base.connection.quote_string(full_path.downcase) + + ActiveRecord::Base.connection. + select_all("SELECT id, path FROM routes WHERE lower(path) = '#{quoted_path}'").present? + end + + # Adds number to the end of the path that is not taken by other route + def rename_path(namespace_path, path_was) + counter = 0 + path = "#{path_was}#{counter}" + + while route_exists?("#{namespace_path}/#{path}") + counter += 1 + path = "#{path_was}#{counter}" + end + + path + end + + def rename_projects(projects) + projects.each do |project| + id = project.id + path_was = project.path + namespace_path = project.namespace.path + path = rename_path(namespace_path, path_was) + + begin + # Because project path update is quite complex operation we can't safely + # copy-paste all code from GitLab. As exception we use Rails code here + project.rename_repo if rename_project_row(project, path) + rescue Exception => e # rubocop: disable Lint/RescueException + Rails.logger.error "Exception when renaming project #{id}: #{e.message}" + end + end + end + + def rename_project_row(project, path) + project.respond_to?(:update_attributes) && + project.update_attributes(path: path) && + project.respond_to?(:rename_repo) + end +end diff --git a/spec/migrations/rename_forbidden_child_namespaces_spec.rb b/spec/migrations/rename_forbidden_child_namespaces_spec.rb new file mode 100644 index 00000000000..c5486e18052 --- /dev/null +++ b/spec/migrations/rename_forbidden_child_namespaces_spec.rb @@ -0,0 +1,187 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20170404152317_rename_forbidden_child_namespaces.rb') + +describe RenameForbiddenChildNamespaces, truncate: true do + let(:migration) { described_class.new } + let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_namespaces_test') } + let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } + let(:forbidden_namespace) do + namespace = build(:group, path: 'info') + namespace.parent = create(:group, path: 'parent') + namespace.save(validate: false) + namespace + end + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + FileUtils.remove_dir(TestEnv.repos_path) if File.directory?(TestEnv.repos_path) + allow(migration).to receive(:say) + allow(migration).to receive(:uploads_dir).and_return(uploads_dir) + end + + describe '#forbidden_namespaces_with_path' do + let(:other_namespace) { create(:group, path: 'info') } + before do + forbidden_namespace + other_namespace + end + + it 'includes namespaces called with path `info`' do + expect(migration.forbidden_namespaces_with_path('info').map(&:id)).to contain_exactly(forbidden_namespace.id) + end + end + + describe '#up' do + before do + forbidden_namespace + end + + it 'renames namespaces called info' do + migration.up + + expect(forbidden_namespace.reload.path).to eq('info0') + end + + it 'renames the route to the namespace' do + migration.up + + expect(forbidden_namespace.reload.full_path).to eq('parent/info0') + end + + it 'renames the route for projects of the namespace' do + project = create(:project, path: 'project-path', namespace: forbidden_namespace) + + migration.up + + expect(project.route.reload.path).to eq('parent/info0/project-path') + end + + it 'moves the the repository for a project in the namespace' do + create(:project, namespace: forbidden_namespace, path: 'info-project') + expected_repo = File.join(TestEnv.repos_path, 'parent/info0', 'info-project.git') + + migration.up + + expect(File.directory?(expected_repo)).to be(true) + end + + it 'moves the uploads for the namespace' do + allow(migration).to receive(:move_namespace_folders).with(Settings.pages.path, 'parent/info', 'parent/info0') + expect(migration).to receive(:move_namespace_folders).with(uploads_dir, 'parent/info', 'parent/info0') + + migration.up + end + + it 'moves the pages for the namespace' do + allow(migration).to receive(:move_namespace_folders).with(uploads_dir, 'parent/info', 'parent/info0') + expect(migration).to receive(:move_namespace_folders).with(Settings.pages.path, 'parent/info', 'parent/info0') + + migration.up + end + + it 'clears the markdown cache for projects in the forbidden namespace' do + project = create(:project, namespace: forbidden_namespace) + scopes = { 'Project' => { id: [project.id] }, + 'Issue' => { project_id: [project.id] }, + 'MergeRequest' => { target_project_id: [project.id] }, + 'Note' => { project_id: [project.id] } } + + expect(ClearDatabaseCacheWorker).to receive(:perform_async).with(scopes) + + migration.up + end + + context 'forbidden namespace -> subgroup -> info0 project' do + it 'updates the route of the project correctly' do + subgroup = create(:group, path: 'subgroup', parent: forbidden_namespace) + project = create(:project, path: 'info0', namespace: subgroup) + + migration.up + + expect(project.route.reload.path).to eq('parent/info0/subgroup/info0') + end + end + end + + describe '#move_repositories' do + let(:namespace) { create(:group, name: 'hello-group') } + it 'moves a project for a namespace' do + create(:project, namespace: namespace, path: 'hello-project') + expected_path = File.join(TestEnv.repos_path, 'bye-group', 'hello-project.git') + + migration.move_repositories(namespace, 'hello-group', 'bye-group') + + expect(File.directory?(expected_path)).to be(true) + end + + it 'moves a namespace in a subdirectory correctly' do + child_namespace = create(:group, name: 'sub-group', parent: namespace) + create(:project, namespace: child_namespace, path: 'hello-project') + + expected_path = File.join(TestEnv.repos_path, 'hello-group', 'renamed-sub-group', 'hello-project.git') + + migration.move_repositories(child_namespace, 'hello-group/sub-group', 'hello-group/renamed-sub-group') + + expect(File.directory?(expected_path)).to be(true) + end + + it 'moves a parent namespace with subdirectories' do + child_namespace = create(:group, name: 'sub-group', parent: namespace) + create(:project, namespace: child_namespace, path: 'hello-project') + expected_path = File.join(TestEnv.repos_path, 'renamed-group', 'sub-group', 'hello-project.git') + + migration.move_repositories(child_namespace, 'hello-group', 'renamed-group') + + expect(File.directory?(expected_path)).to be(true) + end + end + + describe '#move_namespace_folders' do + it 'moves a namespace with files' do + source = File.join(uploads_dir, 'parent-group', 'sub-group') + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, 'parent-group', 'moved-group') + FileUtils.touch(File.join(source, 'test.txt')) + expected_file = File.join(destination, 'test.txt') + + migration.move_namespace_folders(uploads_dir, File.join('parent-group', 'sub-group'), File.join('parent-group', 'moved-group')) + + expect(File.exist?(expected_file)).to be(true) + end + + it 'moves a parent namespace uploads' do + source = File.join(uploads_dir, 'parent-group', 'sub-group') + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, 'moved-parent', 'sub-group') + FileUtils.touch(File.join(source, 'test.txt')) + expected_file = File.join(destination, 'test.txt') + + migration.move_namespace_folders(uploads_dir, 'parent-group', 'moved-parent') + + expect(File.exist?(expected_file)).to be(true) + end + end + + describe '#child_ids_for_parent' do + it 'collects child ids for all levels' do + parent = create(:namespace) + first_child = create(:namespace, parent: parent) + second_child = create(:namespace, parent: parent) + third_child = create(:namespace, parent: second_child) + all_ids = [parent.id, first_child.id, second_child.id, third_child.id] + + collected_ids = migration.child_ids_for_parent(parent, ids: [parent.id]) + + expect(collected_ids).to contain_exactly(*all_ids) + end + end + + describe '#remove_last_ocurrence' do + it 'removes only the last occurance of a string' do + input = 'this/is/info/namespace/with/info' + + expect(migration.remove_last_occurrence(input, 'info')).to eq('this/is/info/namespace/with/') + end + end +end diff --git a/spec/migrations/rename_forbidden_root_namespaces_spec.rb b/spec/migrations/rename_forbidden_root_namespaces_spec.rb new file mode 100644 index 00000000000..ca806e08475 --- /dev/null +++ b/spec/migrations/rename_forbidden_root_namespaces_spec.rb @@ -0,0 +1,197 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20170403121055_rename_forbidden_root_namespaces.rb') + +describe RenameForbiddenRootNamespaces, truncate: true do + let(:migration) { described_class.new } + let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_namespaces_test') } + let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } + let(:forbidden_namespace) do + namespace = build(:namespace, path: 'api') + namespace.save(validate: false) + namespace + end + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + FileUtils.remove_dir(TestEnv.repos_path) if File.directory?(TestEnv.repos_path) + allow(migration).to receive(:say) + allow(migration).to receive(:uploads_dir).and_return(uploads_dir) + end + + describe '#forbidden_namespaces_with_path' do + before do + forbidden_namespace + end + + it 'includes namespaces called with path `api`' do + expect(migration.forbidden_namespaces_with_path('api').map(&:id)).to include(forbidden_namespace.id) + end + end + + describe '#up' do + before do + forbidden_namespace + end + + it 'renames namespaces called api' do + migration.up + + expect(forbidden_namespace.reload.path).to eq('api0') + end + + it 'renames the route to the namespace' do + migration.up + + expect(forbidden_namespace.reload.full_path).to eq('api0') + end + + it 'renames the route for projects of the namespace' do + project = create(:project, path: 'project-path', namespace: forbidden_namespace) + + migration.up + + expect(project.route.reload.path).to eq('api0/project-path') + end + + it 'moves the the repository for a project in the namespace' do + create(:project, namespace: forbidden_namespace, path: 'api-project') + expected_repo = File.join(TestEnv.repos_path, 'api0', 'api-project.git') + + migration.up + + expect(File.directory?(expected_repo)).to be(true) + end + + it 'moves the uploads for the namespace' do + allow(migration).to receive(:move_namespace_folders).with(Settings.pages.path, 'api', 'api0') + expect(migration).to receive(:move_namespace_folders).with(uploads_dir, 'api', 'api0') + + migration.up + end + + it 'moves the pages for the namespace' do + allow(migration).to receive(:move_namespace_folders).with(uploads_dir, 'api', 'api0') + expect(migration).to receive(:move_namespace_folders).with(Settings.pages.path, 'api', 'api0') + + migration.up + end + + it 'clears the markdown cache for projects in the forbidden namespace' do + project = create(:project, namespace: forbidden_namespace) + scopes = { 'Project' => { id: [project.id] }, + 'Issue' => { project_id: [project.id] }, + 'MergeRequest' => { target_project_id: [project.id] }, + 'Note' => { project_id: [project.id] } } + + expect(ClearDatabaseCacheWorker).to receive(:perform_async).with(scopes) + + migration.up + end + + context 'forbidden namespace -> subgroup -> api0 project' do + it 'updates the route of the project correctly' do + subgroup = create(:group, path: 'subgroup', parent: forbidden_namespace) + project = create(:project, path: 'api0', namespace: subgroup) + + migration.up + + expect(project.route.reload.path).to eq('api0/subgroup/api0') + end + end + + context 'for a sub-namespace' do + before do + forbidden_namespace.parent = create(:namespace, path: 'parent') + forbidden_namespace.save(validate: false) + end + + it "doesn't rename child-namespace paths" do + migration.up + + expect(forbidden_namespace.reload.full_path).to eq('parent/api') + end + end + end + + describe '#move_repositories' do + let(:namespace) { create(:group, name: 'hello-group') } + it 'moves a project for a namespace' do + create(:project, namespace: namespace, path: 'hello-project') + expected_path = File.join(TestEnv.repos_path, 'bye-group', 'hello-project.git') + + migration.move_repositories(namespace, 'hello-group', 'bye-group') + + expect(File.directory?(expected_path)).to be(true) + end + + it 'moves a namespace in a subdirectory correctly' do + child_namespace = create(:group, name: 'sub-group', parent: namespace) + create(:project, namespace: child_namespace, path: 'hello-project') + + expected_path = File.join(TestEnv.repos_path, 'hello-group', 'renamed-sub-group', 'hello-project.git') + + migration.move_repositories(child_namespace, 'hello-group/sub-group', 'hello-group/renamed-sub-group') + + expect(File.directory?(expected_path)).to be(true) + end + + it 'moves a parent namespace with subdirectories' do + child_namespace = create(:group, name: 'sub-group', parent: namespace) + create(:project, namespace: child_namespace, path: 'hello-project') + expected_path = File.join(TestEnv.repos_path, 'renamed-group', 'sub-group', 'hello-project.git') + + migration.move_repositories(child_namespace, 'hello-group', 'renamed-group') + + expect(File.directory?(expected_path)).to be(true) + end + end + + describe '#move_namespace_folders' do + it 'moves a namespace with files' do + source = File.join(uploads_dir, 'parent-group', 'sub-group') + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, 'parent-group', 'moved-group') + FileUtils.touch(File.join(source, 'test.txt')) + expected_file = File.join(destination, 'test.txt') + + migration.move_namespace_folders(uploads_dir, File.join('parent-group', 'sub-group'), File.join('parent-group', 'moved-group')) + + expect(File.exist?(expected_file)).to be(true) + end + + it 'moves a parent namespace uploads' do + source = File.join(uploads_dir, 'parent-group', 'sub-group') + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, 'moved-parent', 'sub-group') + FileUtils.touch(File.join(source, 'test.txt')) + expected_file = File.join(destination, 'test.txt') + + migration.move_namespace_folders(uploads_dir, 'parent-group', 'moved-parent') + + expect(File.exist?(expected_file)).to be(true) + end + end + + describe '#child_ids_for_parent' do + it 'collects child ids for all levels' do + parent = create(:namespace) + first_child = create(:namespace, parent: parent) + second_child = create(:namespace, parent: parent) + third_child = create(:namespace, parent: second_child) + all_ids = [parent.id, first_child.id, second_child.id, third_child.id] + + collected_ids = migration.child_ids_for_parent(parent, ids: [parent.id]) + + expect(collected_ids).to contain_exactly(*all_ids) + end + end + + describe '#remove_last_ocurrence' do + it 'removes only the last occurance of a string' do + input = 'this/is/api/namespace/with/api' + + expect(migration.remove_last_occurrence(input, 'api')).to eq('this/is/api/namespace/with/') + end + end +end From 536f2bdfd17ac3bab38851de2973dd1c89dccc3f Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 5 Apr 2017 13:44:23 +0200 Subject: [PATCH 03/38] Add forbidden paths to the namespace validator --- app/validators/namespace_validator.rb | 14 +++++++++- spec/validators/namespace_validator_spec.rb | 29 +++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 spec/validators/namespace_validator_spec.rb diff --git a/app/validators/namespace_validator.rb b/app/validators/namespace_validator.rb index 9601013f2d2..2aef4204e31 100644 --- a/app/validators/namespace_validator.rb +++ b/app/validators/namespace_validator.rb @@ -53,7 +53,19 @@ class NamespaceValidator < ActiveModel::EachValidator WILDCARD_ROUTES = %w[tree commits wikis new edit create update logs_tree preview blob blame raw files create_dir find_file - artifacts graphs refs badges].freeze + artifacts graphs refs badges info git-upload-pack + git-receive-pack gitlab-lfs autocomplete_sources + templates avatar commit pages compare network snippets + services mattermost deploy_keys forks import merge_requests + branches merged_branches tags protected_branches variables + triggers pipelines environments cycle_analytics builds + hooks container_registry milestones labels issues + project_members group_links notes noteable boards todos + uploads runners runner_projects settings repository + transfer remove_fork archive unarchive housekeeping + toggle_star preview_markdown export remove_export + generate_new_export download_export activity + new_issue_address registry].freeze STRICT_RESERVED = (RESERVED + WILDCARD_ROUTES).freeze diff --git a/spec/validators/namespace_validator_spec.rb b/spec/validators/namespace_validator_spec.rb new file mode 100644 index 00000000000..e21b8ef5abd --- /dev/null +++ b/spec/validators/namespace_validator_spec.rb @@ -0,0 +1,29 @@ +require 'spec_helper' + +describe NamespaceValidator do + describe 'RESERVED' do + it 'includes all the top level namespaces' do + all_top_level_routes = Rails.application.routes.routes.routes. + map { |r| r.path.spec.to_s }. + select { |p| p !~ %r{^/[:*]} }. + map { |p| p.split('/')[1] }. + compact. + map { |p| p.split('(', 2)[0] }. + uniq + + expect(described_class::RESERVED).to include(*all_top_level_routes) + end + end + + describe 'WILDCARD_ROUTES' do + it 'includes all paths that can be used after a namespace/project path' do + all_wildcard_paths = Rails.application.routes.routes.routes. + map { |r| r.path.spec.to_s }. + select { |p| p =~ %r{^/\*namespace_id/:(project_)?id/[^:*]} }. + map { |p| p.split('/')[3].split('(', 2)[0] }. + uniq + + expect(described_class::WILDCARD_ROUTES).to include(*all_wildcard_paths) + end + end +end From 74fcccaab30ac0f9e11ed9a076c008ade13a50d0 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 5 Apr 2017 15:41:00 +0200 Subject: [PATCH 04/38] Streamline the path validation in groups & projects `Project` uses `ProjectPathValidator` which is now a `NamespaceValidator` that skips the format validation. That way we're sure we are using the same collection of reserved paths. I updated the path constraints to reflect the changes: We now allow some values that are only used on a top level namespace as a name for a nested group/project. --- app/models/project.rb | 5 +- app/validators/namespace_validator.rb | 93 +++++++++++++------ app/validators/project_path_validator.rb | 22 ++--- lib/constraints/group_url_constrainer.rb | 10 +- lib/gitlab/etag_caching/router.rb | 24 +++-- .../constraints/group_url_constrainer_spec.rb | 7 ++ spec/models/group_spec.rb | 20 ++++ spec/models/project_spec.rb | 14 +++ spec/validators/namespace_validator_spec.rb | 33 +++++++ 9 files changed, 167 insertions(+), 61 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index c7dc562c238..205b080c353 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -199,10 +199,11 @@ class Project < ActiveRecord::Base project_path: true, length: { maximum: 255 }, format: { with: Gitlab::Regex.project_path_regex, - message: Gitlab::Regex.project_path_regex_message } + message: Gitlab::Regex.project_path_regex_message }, + uniqueness: { scope: :namespace_id } + validates :namespace, presence: true validates :name, uniqueness: { scope: :namespace_id } - validates :path, uniqueness: { scope: :namespace_id } validates :import_url, addressable_url: true, if: :external_import? validates :import_url, importable_url: true, if: [:external_import?, :import_url_changed?] validates :star_count, numericality: { greater_than_or_equal_to: 0 } diff --git a/app/validators/namespace_validator.rb b/app/validators/namespace_validator.rb index 2aef4204e31..5a8b482d3db 100644 --- a/app/validators/namespace_validator.rb +++ b/app/validators/namespace_validator.rb @@ -5,7 +5,16 @@ # Values are checked for formatting and exclusion from a list of reserved path # names. class NamespaceValidator < ActiveModel::EachValidator - RESERVED = %w[ + # All routes that appear on the top level must be listed here. + # This will make sure that groups cannot be created with these names + # as these routes would be masked by the paths already in place. + # + # Example: + # /api/api-project + # + # the path `api` shouldn't be allowed because it would be masked by `api/*` + # + TOP_LEVEL_ROUTES = Set.new(%w[ .well-known admin all @@ -49,35 +58,56 @@ class NamespaceValidator < ActiveModel::EachValidator jwt oauth sent_notifications - ].freeze + ]).freeze - WILDCARD_ROUTES = %w[tree commits wikis new edit create update logs_tree - preview blob blame raw files create_dir find_file - artifacts graphs refs badges info git-upload-pack - git-receive-pack gitlab-lfs autocomplete_sources - templates avatar commit pages compare network snippets - services mattermost deploy_keys forks import merge_requests - branches merged_branches tags protected_branches variables - triggers pipelines environments cycle_analytics builds - hooks container_registry milestones labels issues - project_members group_links notes noteable boards todos - uploads runners runner_projects settings repository - transfer remove_fork archive unarchive housekeeping - toggle_star preview_markdown export remove_export - generate_new_export download_export activity - new_issue_address registry].freeze + # All project routes with wildcard argument must be listed here. + # Otherwise it can lead to routing issues when route considered as project name. + # + # Example: + # /group/project/tree/deploy_keys + # + # without tree as reserved name routing can match 'group/project' as group name, + # 'tree' as project name and 'deploy_keys' as route. + # + WILDCARD_ROUTES = Set.new(%w[tree commits wikis new edit create update logs_tree + preview blob blame raw files create_dir find_file + artifacts graphs refs badges info git-upload-pack + git-receive-pack gitlab-lfs autocomplete_sources + templates avatar commit pages compare network snippets + services mattermost deploy_keys forks import merge_requests + branches merged_branches tags protected_branches variables + triggers pipelines environments cycle_analytics builds + hooks container_registry milestones labels issues + project_members group_links notes noteable boards todos + uploads runners runner_projects settings repository + transfer remove_fork archive unarchive housekeeping + toggle_star preview_markdown export remove_export + generate_new_export download_export activity + new_issue_address registry]) - STRICT_RESERVED = (RESERVED + WILDCARD_ROUTES).freeze + STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES) - def self.valid?(value) - !reserved?(value) && follow_format?(value) + def self.valid_full_path?(full_path) + pieces = full_path.split('/') + first_part = pieces.first + pieces.all? do |namespace| + type = first_part == namespace ? :top_level : :wildcard + valid?(namespace, type: type) + end end - def self.reserved?(value, strict: false) - if strict - STRICT_RESERVED.include?(value) + def self.valid?(value, type: :strict) + !reserved?(value, type: type) && follow_format?(value) + end + + def self.reserved?(value, type: :strict) + case type + when :wildcard + WILDCARD_ROUTES.include?(value) + when :top_level + TOP_LEVEL_ROUTES.include?(value) else - RESERVED.include?(value) + STRICT_RESERVED.include?(value) end end @@ -92,10 +122,19 @@ class NamespaceValidator < ActiveModel::EachValidator record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) end - strict = record.is_a?(Group) && record.parent_id - - if reserved?(value, strict: strict) + if reserved?(value, type: validation_type(record)) record.errors.add(attribute, "#{value} is a reserved name") end end + + def validation_type(record) + case record + when Group + record.parent_id ? :wildcard : :top_level + when Project + :wildcard + else + :strict + end + end end diff --git a/app/validators/project_path_validator.rb b/app/validators/project_path_validator.rb index ee2ae65be7b..d41bdaeab84 100644 --- a/app/validators/project_path_validator.rb +++ b/app/validators/project_path_validator.rb @@ -4,25 +4,17 @@ # # Values are checked for formatting and exclusion from a list of reserved path # names. -class ProjectPathValidator < ActiveModel::EachValidator - # All project routes with wildcard argument must be listed here. - # Otherwise it can lead to routing issues when route considered as project name. - # - # Example: - # /group/project/tree/deploy_keys - # - # without tree as reserved name routing can match 'group/project' as group name, - # 'tree' as project name and 'deploy_keys' as route. - # - RESERVED = (NamespaceValidator::STRICT_RESERVED - - %w[dashboard help ci admin search notes services assets profile public]).freeze - +# +# This is basically the same as the `NamespaceValidator` but it skips the validation +# of the format with `Gitlab::Regex.namespace_regex`. The format of projects +# is validated in the class itself. +class ProjectPathValidator < NamespaceValidator def self.valid?(value) !reserved?(value) end - def self.reserved?(value) - RESERVED.include?(value) + def self.reserved?(value, type: :wildcard) + super(value, type: :wildcard) end delegate :reserved?, to: :class diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb index bae4db1ca4d..559f510944a 100644 --- a/lib/constraints/group_url_constrainer.rb +++ b/lib/constraints/group_url_constrainer.rb @@ -2,16 +2,8 @@ class GroupUrlConstrainer def matches?(request) id = request.params[:id] - return false unless valid?(id) + return false unless NamespaceValidator.valid_full_path?(id) Group.find_by_full_path(id).present? end - - private - - def valid?(id) - id.split('/').all? do |namespace| - NamespaceValidator.valid?(namespace) - end - end end diff --git a/lib/gitlab/etag_caching/router.rb b/lib/gitlab/etag_caching/router.rb index f6e4f279c06..67aa237f2f1 100644 --- a/lib/gitlab/etag_caching/router.rb +++ b/lib/gitlab/etag_caching/router.rb @@ -2,31 +2,39 @@ module Gitlab module EtagCaching class Router Route = Struct.new(:regexp, :name) - - RESERVED_WORDS = NamespaceValidator::WILDCARD_ROUTES.map { |word| "/#{word}/" }.join('|') + # We enable an ETag for every request matching the regex. + # To match a regex the path needs to match the following: + # - Don't contain a reserved word (expect for the words used in the + # regex itself) + # - Ending in `noteable/issue//notes` for the `issue_notes` route + # - Ending in `issues/id`/rendered_title` for the `issue_title` route + USED_IN_ROUTES = %w[noteable issue notes issues renderred_title + commit pipelines merge_requests new].freeze + RESERVED_WORDS = NamespaceValidator::WILDCARD_ROUTES - USED_IN_ROUTES + RESERVED_WORDS_REGEX = Regexp.union(*RESERVED_WORDS) ROUTES = [ Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/noteable/issue/\d+/notes\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/noteable/issue/\d+/notes\z), 'issue_notes' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/issues/\d+/rendered_title\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/issues/\d+/rendered_title\z), 'issue_title' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/commit/\S+/pipelines\.json\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/commit/\S+/pipelines\.json\z), 'commit_pipelines' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/merge_requests/new\.json\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/merge_requests/new\.json\z), 'new_merge_request_pipelines' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/merge_requests/\d+/pipelines\.json\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/merge_requests/\d+/pipelines\.json\z), 'merge_request_pipelines' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/pipelines\.json\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/pipelines\.json\z), 'project_pipelines' ) ].freeze diff --git a/spec/lib/constraints/group_url_constrainer_spec.rb b/spec/lib/constraints/group_url_constrainer_spec.rb index 96dacdc5cd2..f95adf3a84b 100644 --- a/spec/lib/constraints/group_url_constrainer_spec.rb +++ b/spec/lib/constraints/group_url_constrainer_spec.rb @@ -17,6 +17,13 @@ describe GroupUrlConstrainer, lib: true do it { expect(subject.matches?(request)).to be_truthy } end + context 'valid request for nested group with reserved top level name' do + let!(:nested_group) { create(:group, path: 'api', parent: group) } + let!(:request) { build_request('gitlab/api') } + + it { expect(subject.matches?(request)).to be_truthy } + end + context 'invalid request' do let(:request) { build_request('foo') } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 8ffde6f7fbb..7b076d09585 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -57,6 +57,26 @@ describe Group, models: true do it { is_expected.not_to validate_presence_of :owner } it { is_expected.to validate_presence_of :two_factor_grace_period } it { is_expected.to validate_numericality_of(:two_factor_grace_period).is_greater_than_or_equal_to(0) } + + describe 'path validation' do + it 'rejects paths reserved on the root namespace when the group has no parent' do + group = build(:group, path: 'api') + + expect(group).not_to be_valid + end + + it 'allows root paths when the group has a parent' do + group = build(:group, path: 'api', parent: create(:group)) + + expect(group).to be_valid + end + + it 'rejects any wildcard paths when not a top level group' do + group = build(:group, path: 'tree', parent: create(:group)) + + expect(group).not_to be_valid + end + end end describe '.visible_to_user' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 92d420337f9..726dade93f6 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -253,6 +253,20 @@ describe Project, models: true do expect(new_project.errors.full_messages.first).to eq('The project is still being deleted. Please try again later.') end end + + describe 'path validation' do + it 'allows paths reserved on the root namespace' do + project = build(:project, path: 'api') + + expect(project).to be_valid + end + + it 'rejects paths reserved on another level' do + project = build(:project, path: 'tree') + + expect(project).not_to be_valid + end + end end describe 'default_scope' do diff --git a/spec/validators/namespace_validator_spec.rb b/spec/validators/namespace_validator_spec.rb index e21b8ef5abd..4b5dd59e048 100644 --- a/spec/validators/namespace_validator_spec.rb +++ b/spec/validators/namespace_validator_spec.rb @@ -1,6 +1,7 @@ require 'spec_helper' describe NamespaceValidator do + let(:validator) { described_class.new(attributes: [:path]) } describe 'RESERVED' do it 'includes all the top level namespaces' do all_top_level_routes = Rails.application.routes.routes.routes. @@ -26,4 +27,36 @@ describe NamespaceValidator do expect(described_class::WILDCARD_ROUTES).to include(*all_wildcard_paths) end end + + describe '#validation_type' do + it 'uses top level validation for groups without parent' do + group = build(:group) + + type = validator.validation_type(group) + + expect(type).to eq(:top_level) + end + + it 'uses wildcard validation for groups with a parent' do + group = build(:group, parent: create(:group)) + + type = validator.validation_type(group) + + expect(type).to eq(:wildcard) + end + + it 'uses wildcard validation for a project' do + project = build(:project) + + type = validator.validation_type(project) + + expect(type).to eq(:wildcard) + end + + it 'uses strict validation for everything else' do + type = validator.validation_type(double) + + expect(type).to eq(:strict) + end + end end From e4f5b7ca2184473985ef216df676ddb737fb26af Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 10 Apr 2017 19:27:19 +0200 Subject: [PATCH 05/38] Improve detection of reserved words from routes --- app/validators/namespace_validator.rb | 15 +--- spec/validators/namespace_validator_spec.rb | 83 +++++++++++++++++---- 2 files changed, 70 insertions(+), 28 deletions(-) diff --git a/app/validators/namespace_validator.rb b/app/validators/namespace_validator.rb index 5a8b482d3db..4d99b09e98f 100644 --- a/app/validators/namespace_validator.rb +++ b/app/validators/namespace_validator.rb @@ -69,21 +69,10 @@ class NamespaceValidator < ActiveModel::EachValidator # without tree as reserved name routing can match 'group/project' as group name, # 'tree' as project name and 'deploy_keys' as route. # + WILDCARD_ROUTES = Set.new(%w[tree commits wikis new edit create update logs_tree preview blob blame raw files create_dir find_file - artifacts graphs refs badges info git-upload-pack - git-receive-pack gitlab-lfs autocomplete_sources - templates avatar commit pages compare network snippets - services mattermost deploy_keys forks import merge_requests - branches merged_branches tags protected_branches variables - triggers pipelines environments cycle_analytics builds - hooks container_registry milestones labels issues - project_members group_links notes noteable boards todos - uploads runners runner_projects settings repository - transfer remove_fork archive unarchive housekeeping - toggle_star preview_markdown export remove_export - generate_new_export download_export activity - new_issue_address registry]) + artifacts graphs refs badges objects folders file]) STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES) diff --git a/spec/validators/namespace_validator_spec.rb b/spec/validators/namespace_validator_spec.rb index 4b5dd59e048..7ddce74939d 100644 --- a/spec/validators/namespace_validator_spec.rb +++ b/spec/validators/namespace_validator_spec.rb @@ -2,27 +2,80 @@ require 'spec_helper' describe NamespaceValidator do let(:validator) { described_class.new(attributes: [:path]) } - describe 'RESERVED' do - it 'includes all the top level namespaces' do - all_top_level_routes = Rails.application.routes.routes.routes. - map { |r| r.path.spec.to_s }. - select { |p| p !~ %r{^/[:*]} }. - map { |p| p.split('/')[1] }. - compact. - map { |p| p.split('(', 2)[0] }. - uniq - expect(described_class::RESERVED).to include(*all_top_level_routes) + # Pass in a full path to remove the format segment: + # `/ci/lint(.:format)` -> `/ci/lint` + def without_format(path) + path.split('(', 2)[0] + end + + # Pass in a full path and get the last segment before a wildcard + # That's not a parameter + # `/*namespace_id/:project_id/builds/artifacts/*ref_name_and_path` + # -> 'artifacts' + def segment_before_last_wildcard(path) + path_segments = path.split('/').reject { |segment| segment.empty? } + last_wildcard_index = path_segments.rindex { |part| part.starts_with?('*') } + + index_of_non_param_segment = last_wildcard_index - 1 + part_before_wildcard = path_segments[index_of_non_param_segment] + while parameter?(part_before_wildcard) + index_of_non_param_segment -= 1 + part_before_wildcard = path_segments[index_of_non_param_segment] + end + + part_before_wildcard + end + + def parameter?(path_segment) + path_segment.starts_with?(':') || path_segment.starts_with?('*') + end + + let(:all_routes) do + Rails.application.routes.routes.routes. + map { |r| r.path.spec.to_s } + end + + let(:routes_without_format) { all_routes.map { |path| without_format(path) } } + + # Routes not starting with `/:` or `/*` + # all routes not starting with a param + let(:routes_not_starting_in_wildcard) { routes_without_format.select { |p| p !~ %r{^/[:*]} } } + + # All routes that start with a namespaced path, that have 1 or more + # path-segments before having another wildcard parameter. + # - Starting with paths: + # - `/*namespace_id/:project_id/` + # - `/*namespace_id/:id/` + # - Followed by one or more path-parts not starting with `:` or `/` + # - Followed by a path-part that includes a wildcard parameter `*` + # At the time of writing these routes match: http://rubular.com/r/QDxulzZlxZ + STARTING_WITH_NAMESPACE = /^\/\*namespace_id\/:(project_)?id/ + NON_PARAM_PARTS = /[^:*][a-z\-_\/]*/ + ANY_OTHER_PATH_PART = /[a-z\-_\/:]*/ + WILDCARD_SEGMENT = /\*/ + let(:namespaced_wildcard_routes) do + routes_without_format.select do |p| + p =~ %r{#{STARTING_WITH_NAMESPACE}\/#{NON_PARAM_PARTS}\/#{ANY_OTHER_PATH_PART}#{WILDCARD_SEGMENT}} + end + end + + describe 'TOP_LEVEL_ROUTES' do + it 'includes all the top level namespaces' do + top_level_words = routes_not_starting_in_wildcard. + map { |p| p.split('/')[1] }. # Get the first part of the path + compact. + uniq + + expect(described_class::TOP_LEVEL_ROUTES).to include(*top_level_words) end end describe 'WILDCARD_ROUTES' do it 'includes all paths that can be used after a namespace/project path' do - all_wildcard_paths = Rails.application.routes.routes.routes. - map { |r| r.path.spec.to_s }. - select { |p| p =~ %r{^/\*namespace_id/:(project_)?id/[^:*]} }. - map { |p| p.split('/')[3].split('(', 2)[0] }. - uniq + all_wildcard_paths = namespaced_wildcard_routes.map do |path| + segment_before_last_wildcard(path) + end.uniq expect(described_class::WILDCARD_ROUTES).to include(*all_wildcard_paths) end From f7511caa5f26c071c61908a48440a95c5f45eb69 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 11 Apr 2017 15:51:33 +0200 Subject: [PATCH 06/38] Split off validating full paths The first part of a full path needs to be validated as a `top_level` while the rest need to be validated as `wildcard` --- app/validators/namespace_validator.rb | 14 ++++++++------ spec/validators/namespace_validator_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/app/validators/namespace_validator.rb b/app/validators/namespace_validator.rb index 4d99b09e98f..8a0e18612ec 100644 --- a/app/validators/namespace_validator.rb +++ b/app/validators/namespace_validator.rb @@ -77,12 +77,14 @@ class NamespaceValidator < ActiveModel::EachValidator STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES) def self.valid_full_path?(full_path) - pieces = full_path.split('/') - first_part = pieces.first - pieces.all? do |namespace| - type = first_part == namespace ? :top_level : :wildcard - valid?(namespace, type: type) - end + path_segments = full_path.split('/') + root_segment = path_segments.shift + + valid?(root_segment, type: :top_level) && valid_wildcard_segments?(path_segments) + end + + def self.valid_wildcard_segments?(segments) + segments.all? { |segment| valid?(segment, type: :wildcard) } end def self.valid?(value, type: :strict) diff --git a/spec/validators/namespace_validator_spec.rb b/spec/validators/namespace_validator_spec.rb index 7ddce74939d..589175a2ced 100644 --- a/spec/validators/namespace_validator_spec.rb +++ b/spec/validators/namespace_validator_spec.rb @@ -81,6 +81,26 @@ describe NamespaceValidator do end end + describe '#valid_full_path' do + it "isn't valid when the top level is reserved" do + test_path = 'u/should-be-a/reserved-word' + + expect(described_class.valid_full_path?(test_path)).to be(false) + end + + it "isn't valid if any of the path segments is reserved" do + test_path = 'the-wildcard/wikis/is-a-reserved-path' + + expect(described_class.valid_full_path?(test_path)).to be(false) + end + + it "is valid if the path doen't contain reserved words" do + test_path = 'there-are/no-wildcards/in-this-path' + + expect(described_class.valid_full_path?(test_path)).to be(true) + end + end + describe '#validation_type' do it 'uses top level validation for groups without parent' do group = build(:group) From 1498a9cb0fb4500737c9d9e88f38c0ddf2b42791 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 11 Apr 2017 16:21:52 +0200 Subject: [PATCH 07/38] Check `has_parent?` for determining validation type --- app/models/namespace.rb | 4 ++++ app/validators/namespace_validator.rb | 6 +++--- spec/models/namespace_spec.rb | 1 + 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 9bfa731785f..1570470d63f 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -220,6 +220,10 @@ class Namespace < ActiveRecord::Base Project.inside_path(full_path) end + def has_parent? + parent.present? + end + private def repository_storage_paths diff --git a/app/validators/namespace_validator.rb b/app/validators/namespace_validator.rb index 8a0e18612ec..ed71d5ad5b5 100644 --- a/app/validators/namespace_validator.rb +++ b/app/validators/namespace_validator.rb @@ -74,7 +74,7 @@ class NamespaceValidator < ActiveModel::EachValidator preview blob blame raw files create_dir find_file artifacts graphs refs badges objects folders file]) - STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES) + STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES).freeze def self.valid_full_path?(full_path) path_segments = full_path.split('/') @@ -120,8 +120,8 @@ class NamespaceValidator < ActiveModel::EachValidator def validation_type(record) case record - when Group - record.parent_id ? :wildcard : :top_level + when Namespace + record.has_parent? ? :wildcard : :top_level when Project :wildcard else diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index e406d0a16bd..6775fd2f1b8 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -47,6 +47,7 @@ describe Namespace, models: true do describe "Respond to" do it { is_expected.to respond_to(:human_name) } it { is_expected.to respond_to(:to_param) } + it { is_expected.to respond_to(:has_parent?) } end describe '#to_param' do From 3143a5d2602de521b432231d701aedcc2844c088 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 12 Apr 2017 09:44:05 +0200 Subject: [PATCH 08/38] Use the namespace validator for validating all paths Since the namespacevalidator now knows the difference between a top-level and another path, this could all be handled there. --- app/models/project.rb | 2 +- app/validators/project_path_validator.rb | 27 ---------------------- lib/constraints/project_url_constrainer.rb | 2 +- 3 files changed, 2 insertions(+), 29 deletions(-) delete mode 100644 app/validators/project_path_validator.rb diff --git a/app/models/project.rb b/app/models/project.rb index 205b080c353..0ed4b4fddd6 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -196,7 +196,7 @@ class Project < ActiveRecord::Base message: Gitlab::Regex.project_name_regex_message } validates :path, presence: true, - project_path: true, + namespace: true, length: { maximum: 255 }, format: { with: Gitlab::Regex.project_path_regex, message: Gitlab::Regex.project_path_regex_message }, diff --git a/app/validators/project_path_validator.rb b/app/validators/project_path_validator.rb deleted file mode 100644 index d41bdaeab84..00000000000 --- a/app/validators/project_path_validator.rb +++ /dev/null @@ -1,27 +0,0 @@ -# ProjectPathValidator -# -# Custom validator for GitLab project path values. -# -# Values are checked for formatting and exclusion from a list of reserved path -# names. -# -# This is basically the same as the `NamespaceValidator` but it skips the validation -# of the format with `Gitlab::Regex.namespace_regex`. The format of projects -# is validated in the class itself. -class ProjectPathValidator < NamespaceValidator - def self.valid?(value) - !reserved?(value) - end - - def self.reserved?(value, type: :wildcard) - super(value, type: :wildcard) - end - - delegate :reserved?, to: :class - - def validate_each(record, attribute, value) - if reserved?(value) - record.errors.add(attribute, "#{value} is a reserved name") - end - end -end diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb index a10b4657d7d..f83fa1618a5 100644 --- a/lib/constraints/project_url_constrainer.rb +++ b/lib/constraints/project_url_constrainer.rb @@ -4,7 +4,7 @@ class ProjectUrlConstrainer project_path = request.params[:project_id] || request.params[:id] full_path = namespace_path + '/' + project_path - unless ProjectPathValidator.valid?(project_path) + unless NamespaceValidator.valid_full_path?(full_path) return false end From bccf8d86c57a141aeb60d96bc3139fe2b5dc9240 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 12 Apr 2017 11:28:23 +0200 Subject: [PATCH 09/38] Rename `NamespaceValidator` to `DynamicPathValidator` This reflects better that it validates paths instead of a namespace model --- app/models/namespace.rb | 2 +- app/models/project.rb | 2 +- app/models/user.rb | 2 +- ...{namespace_validator.rb => dynamic_path_validator.rb} | 9 ++++++--- lib/constraints/group_url_constrainer.rb | 2 +- lib/constraints/project_url_constrainer.rb | 2 +- lib/gitlab/etag_caching/router.rb | 2 +- ..._validator_spec.rb => dynamic_path_validator_spec.rb} | 2 +- 8 files changed, 13 insertions(+), 10 deletions(-) rename app/validators/{namespace_validator.rb => dynamic_path_validator.rb} (92%) rename spec/validators/{namespace_validator_spec.rb => dynamic_path_validator_spec.rb} (99%) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 1570470d63f..397dc7a25ab 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -33,7 +33,7 @@ class Namespace < ActiveRecord::Base validates :path, presence: true, length: { maximum: 255 }, - namespace: true + dynamic_path: true validate :nesting_level_allowed diff --git a/app/models/project.rb b/app/models/project.rb index 0ed4b4fddd6..9ebb9638ca4 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -196,7 +196,7 @@ class Project < ActiveRecord::Base message: Gitlab::Regex.project_name_regex_message } validates :path, presence: true, - namespace: true, + dynamic_path: true, length: { maximum: 255 }, format: { with: Gitlab::Regex.project_path_regex, message: Gitlab::Regex.project_path_regex_message }, diff --git a/app/models/user.rb b/app/models/user.rb index bd9c9f99663..2b7ebe6c1a7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -118,7 +118,7 @@ class User < ActiveRecord::Base presence: true, numericality: { greater_than_or_equal_to: 0, less_than_or_equal_to: Gitlab::Database::MAX_INT_VALUE } validates :username, - namespace: true, + dynamic_path: true, presence: true, uniqueness: { case_sensitive: false } diff --git a/app/validators/namespace_validator.rb b/app/validators/dynamic_path_validator.rb similarity index 92% rename from app/validators/namespace_validator.rb rename to app/validators/dynamic_path_validator.rb index ed71d5ad5b5..ce363c052d8 100644 --- a/app/validators/namespace_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -1,10 +1,11 @@ -# NamespaceValidator +# DynamicPathValidator # -# Custom validator for GitLab namespace values. +# Custom validator for GitLab path values. +# These paths are assigned to `Namespace` (& `Group` as a subclass) & `Project` # # Values are checked for formatting and exclusion from a list of reserved path # names. -class NamespaceValidator < ActiveModel::EachValidator +class DynamicPathValidator < ActiveModel::EachValidator # All routes that appear on the top level must be listed here. # This will make sure that groups cannot be created with these names # as these routes would be masked by the paths already in place. @@ -124,6 +125,8 @@ class NamespaceValidator < ActiveModel::EachValidator record.has_parent? ? :wildcard : :top_level when Project :wildcard + when User + :top_level else :strict end diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb index 559f510944a..48a33690cb2 100644 --- a/lib/constraints/group_url_constrainer.rb +++ b/lib/constraints/group_url_constrainer.rb @@ -2,7 +2,7 @@ class GroupUrlConstrainer def matches?(request) id = request.params[:id] - return false unless NamespaceValidator.valid_full_path?(id) + return false unless DynamicPathValidator.valid_full_path?(id) Group.find_by_full_path(id).present? end diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb index f83fa1618a5..72c457d0369 100644 --- a/lib/constraints/project_url_constrainer.rb +++ b/lib/constraints/project_url_constrainer.rb @@ -4,7 +4,7 @@ class ProjectUrlConstrainer project_path = request.params[:project_id] || request.params[:id] full_path = namespace_path + '/' + project_path - unless NamespaceValidator.valid_full_path?(full_path) + unless DynamicPathValidator.valid_full_path?(full_path) return false end diff --git a/lib/gitlab/etag_caching/router.rb b/lib/gitlab/etag_caching/router.rb index 67aa237f2f1..c7da8b07a56 100644 --- a/lib/gitlab/etag_caching/router.rb +++ b/lib/gitlab/etag_caching/router.rb @@ -10,7 +10,7 @@ module Gitlab # - Ending in `issues/id`/rendered_title` for the `issue_title` route USED_IN_ROUTES = %w[noteable issue notes issues renderred_title commit pipelines merge_requests new].freeze - RESERVED_WORDS = NamespaceValidator::WILDCARD_ROUTES - USED_IN_ROUTES + RESERVED_WORDS = DynamicPathValidator::WILDCARD_ROUTES - USED_IN_ROUTES RESERVED_WORDS_REGEX = Regexp.union(*RESERVED_WORDS) ROUTES = [ Gitlab::EtagCaching::Router::Route.new( diff --git a/spec/validators/namespace_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb similarity index 99% rename from spec/validators/namespace_validator_spec.rb rename to spec/validators/dynamic_path_validator_spec.rb index 589175a2ced..6c490b1be2e 100644 --- a/spec/validators/namespace_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe NamespaceValidator do +describe DynamicPathValidator do let(:validator) { described_class.new(attributes: [:path]) } # Pass in a full path to remove the format segment: From 2f95e6a0c469b2ac7647eef02810ecf2b5ce2021 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Sat, 8 Apr 2017 00:07:57 +0200 Subject: [PATCH 10/38] Move `replace_sql` into `Database::MigrationHelpers` --- lib/gitlab/database/migration_helpers.rb | 23 +++++++++++++ .../gitlab/database/migration_helpers_spec.rb | 33 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 6dabbe0264c..298b1a1f4e6 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -498,6 +498,29 @@ module Gitlab columns(table).find { |column| column.name == name } end + + # This will replace the first occurance of a string in a column with + # the replacement + # On postgresql we can use `regexp_replace` for that. + # On mysql we find the location of the pattern, and overwrite it + # with the replacement + def replace_sql(column, pattern, replacement) + quoted_pattern = Arel::Nodes::Quoted.new(pattern.to_s) + quoted_replacement = Arel::Nodes::Quoted.new(replacement.to_s) + + if Database.mysql? + locate = Arel::Nodes::NamedFunction. + new('locate', [quoted_pattern, column]) + insert_in_place = Arel::Nodes::NamedFunction. + new('insert', [column, locate, pattern.size, quoted_replacement]) + + Arel::Nodes::SqlLiteral.new(insert_in_place.to_sql) + else + replace = Arel::Nodes::NamedFunction. + new("regexp_replace", [column, quoted_pattern, quoted_replacement]) + Arel::Nodes::SqlLiteral.new(replace.to_sql) + end + end end end end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index a044b871730..737fac14f92 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -726,4 +726,37 @@ describe Gitlab::Database::MigrationHelpers, lib: true do expect(model.column_for(:users, :kittens)).to be_nil end end + + describe '#replace_sql' do + context 'using postgres' do + before do + allow(Gitlab::Database).to receive(:mysql?).and_return(false) + end + + it 'builds the sql with correct functions' do + expect(model.replace_sql(Arel::Table.new(:users)[:first_name], "Alice", "Eve").to_s). + to include('regexp_replace') + end + end + + context 'using mysql' do + before do + allow(Gitlab::Database).to receive(:mysql?).and_return(true) + end + + it 'builds the sql with the correct functions' do + expect(model.replace_sql(Arel::Table.new(:users)[:first_name], "Alice", "Eve").to_s). + to include('locate', 'insert') + end + end + + describe 'results' do + let!(:user) { create(:user, name: 'Kathy Alice Aliceson') } + + it 'replaces the correct part of the string' do + model.update_column_in_batches(:users, :name, model.replace_sql(Arel::Table.new(:users)[:name], 'Alice', 'Eve')) + expect(user.reload.name).to eq('Kathy Eve Aliceson') + end + end + end end From 9fb9414ec0787a0414c912bb7b62103f96c48d34 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 12 Apr 2017 13:26:52 +0200 Subject: [PATCH 11/38] Reject `-` as a path --- app/validators/dynamic_path_validator.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index ce363c052d8..7587a3f20e1 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -59,6 +59,7 @@ class DynamicPathValidator < ActiveModel::EachValidator jwt oauth sent_notifications + - ]).freeze # All project routes with wildcard argument must be listed here. From 58bc628d3051d6c97b9592985b43aa741a87d086 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 12 Apr 2017 20:14:22 +0200 Subject: [PATCH 12/38] Rename namespace-paths in a migration helper --- .../rename_reserved_paths_migration.rb | 35 +++ .../migration_classes.rb | 84 +++++++ .../namespaces.rb | 113 ++++++++++ .../namespaces_spec.rb | 208 ++++++++++++++++++ .../rename_reserved_paths_migration_spec.rb | 29 +++ 5 files changed, 469 insertions(+) create mode 100644 lib/gitlab/database/rename_reserved_paths_migration.rb create mode 100644 lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb create mode 100644 lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb create mode 100644 spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb create mode 100644 spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb diff --git a/lib/gitlab/database/rename_reserved_paths_migration.rb b/lib/gitlab/database/rename_reserved_paths_migration.rb new file mode 100644 index 00000000000..ad5570b4c72 --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration.rb @@ -0,0 +1,35 @@ +module Gitlab + module Database + module RenameReservedPathsMigration + include MigrationHelpers + include Namespaces + include Projects + + def rename_wildcard_paths(one_or_more_paths) + paths = Array(one_or_more_paths) + rename_namespaces(paths, type: :wildcard) + end + + def rename_root_paths(paths) + paths = Array(paths) + rename_namespaces(paths, type: :top_level) + end + + def rename_path(namespace_path, path_was) + counter = 0 + path = "#{path_was}#{counter}" + + while route_exists?(File.join(namespace_path, path)) + counter += 1 + path = "#{path_was}#{counter}" + end + + path + end + + def route_exists?(full_path) + MigrationClasses::Route.where(Route.arel_table[:path].matches(full_path)).any? + end + end + end +end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb b/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb new file mode 100644 index 00000000000..a919d250541 --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb @@ -0,0 +1,84 @@ +module Gitlab + module Database + module RenameReservedPathsMigration + module MigrationClasses + class User < ActiveRecord::Base + self.table_name = 'users' + end + + class Namespace < ActiveRecord::Base + self.table_name = 'namespaces' + belongs_to :parent, + class_name: "#{MigrationClasses.name}::Namespace" + has_one :route, as: :source + has_many :children, + class_name: "#{MigrationClasses.name}::Namespace", + foreign_key: :parent_id + belongs_to :owner, + class_name: "#{MigrationClasses.name}::User" + + # Overridden to have the correct `source_type` for the `route` relation + def self.name + 'Namespace' + end + + def full_path + if route && route.path.present? + @full_path ||= route.path + else + update_route if persisted? + + build_full_path + end + end + + def build_full_path + if parent && path + parent.full_path + '/' + path + else + path + end + end + + def update_route + prepare_route + route.save + end + + def prepare_route + route || build_route(source: self) + route.path = build_full_path + route.name = build_full_name + @full_path = nil + @full_name = nil + end + + def build_full_name + if parent && name + parent.human_name + ' / ' + name + else + name + end + end + + def human_name + owner&.name + end + end + + class Route < ActiveRecord::Base + self.table_name = 'routes' + belongs_to :source, polymorphic: true + end + + class Project < ActiveRecord::Base + self.table_name = 'projects' + + def repository_storage_path + Gitlab.config.repositories.storages[repository_storage]['path'] + end + end + end + end + end +end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb b/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb new file mode 100644 index 00000000000..f8fbeaa990a --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb @@ -0,0 +1,113 @@ +module Gitlab + module Database + module RenameReservedPathsMigration + module Namespaces + include Gitlab::ShellAdapter + + def rename_namespaces(paths, type:) + namespaces_for_paths(paths, type: type).each do |namespace| + rename_namespace(namespace) + end + end + + def namespaces_for_paths(paths, type:) + namespaces = if type == :wildcard + MigrationClasses::Namespace.where.not(parent_id: nil) + elsif type == :top_level + MigrationClasses::Namespace.where(parent_id: nil) + end + namespaces.where(path: paths.map(&:downcase)) + end + + def rename_namespace(namespace) + old_path = namespace.path + old_full_path = namespace.full_path + # Only remove the last occurrence of the path name to get the parent namespace path + namespace_path = remove_last_occurrence(old_full_path, old_path) + new_path = rename_path(namespace_path, old_path) + new_full_path = if namespace_path.present? + File.join(namespace_path, new_path) + else + new_path + end + + # skips callbacks & validations + MigrationClasses::Namespace.where(id: namespace). + update_all(path: new_path) + + replace_statement = replace_sql(Route.arel_table[:path], + old_full_path, + new_full_path) + + update_column_in_batches(:routes, :path, replace_statement) do |table, query| + query.where(MigrationClasses::Route.arel_table[:path].matches("#{old_full_path}%")) + end + + move_repositories(namespace, old_full_path, new_full_path) + move_namespace_folders(uploads_dir, old_full_path, new_full_path) if file_storage? + move_namespace_folders(pages_dir, old_full_path, new_full_path) + end + + def move_namespace_folders(directory, old_relative_path, new_relative_path) + old_path = File.join(directory, old_relative_path) + return unless File.directory?(old_path) + + new_path = File.join(directory, new_relative_path) + FileUtils.mv(old_path, new_path) + end + + def move_repositories(namespace, old_full_path, new_full_path) + repo_paths_for_namespace(namespace).each do |repository_storage_path| + # Ensure old directory exists before moving it + gitlab_shell.add_namespace(repository_storage_path, old_full_path) + + unless gitlab_shell.mv_namespace(repository_storage_path, old_full_path, new_full_path) + message = "Exception moving path #{repository_storage_path} \ + from #{old_full_path} to #{new_full_path}" + Rails.logger.error message + end + end + end + + def repo_paths_for_namespace(namespace) + projects_for_namespace(namespace). + select('distinct(repository_storage)').map(&:repository_storage_path) + end + + def projects_for_namespace(namespace) + namespace_ids = child_ids_for_parent(namespace, ids: [namespace.id]) + namespace_or_children = MigrationClasses::Project. + arel_table[:namespace_id]. + in(namespace_ids) + MigrationClasses::Project.unscoped.where(namespace_or_children) + end + + # This won't scale to huge trees, but it should do for a handful of + # namespaces called `system`. + def child_ids_for_parent(namespace, ids: []) + namespace.children.each do |child| + ids << child.id + child_ids_for_parent(child, ids: ids) if child.children.any? + end + ids + end + + def remove_last_occurrence(string, pattern) + string.reverse.sub(pattern.reverse, "").reverse + end + + def file_storage? + CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File + end + + def uploads_dir + File.join(CarrierWave.root, "uploads") + end + + def pages_dir + Settings.pages.path + end + end + end + end +end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb new file mode 100644 index 00000000000..ea31e373647 --- /dev/null +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb @@ -0,0 +1,208 @@ +require 'spec_helper' + +describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate do + let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_namespaces_test') } + let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } + let(:subject) do + ActiveRecord::Migration.new.extend( + Gitlab::Database::RenameReservedPathsMigration + ) + end + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + FileUtils.remove_dir(TestEnv.repos_path) if File.directory?(TestEnv.repos_path) + allow(subject).to receive(:uploads_dir).and_return(uploads_dir) + allow(subject).to receive(:say) + end + + def migration_namespace(namespace) + Gitlab::Database::RenameReservedPathsMigration::MigrationClasses:: + Namespace.find(namespace.id) + end + + describe '#namespaces_for_paths' do + context 'for wildcard namespaces' do + it 'only returns child namespaces with the correct path' do + _root_namespace = create(:namespace, path: 'the-path') + _other_path = create(:namespace, + path: 'other', + parent: create(:namespace)) + namespace = create(:namespace, + path: 'the-path', + parent: create(:namespace)) + + found_ids = subject.namespaces_for_paths(['the-path'], type: :wildcard). + pluck(:id) + expect(found_ids).to contain_exactly(namespace.id) + end + end + + context 'for top level namespaces' do + it 'only returns child namespaces with the correct path' do + root_namespace = create(:namespace, path: 'the-path') + _other_path = create(:namespace, path: 'other') + _child_namespace = create(:namespace, + path: 'the-path', + parent: create(:namespace)) + + found_ids = subject.namespaces_for_paths(['the-path'], type: :top_level). + pluck(:id) + expect(found_ids).to contain_exactly(root_namespace.id) + end + end + end + + describe '#move_repositories' do + let(:namespace) { create(:group, name: 'hello-group') } + it 'moves a project for a namespace' do + create(:project, namespace: namespace, path: 'hello-project') + expected_path = File.join(TestEnv.repos_path, 'bye-group', 'hello-project.git') + + subject.move_repositories(namespace, 'hello-group', 'bye-group') + + expect(File.directory?(expected_path)).to be(true) + end + + it 'moves a namespace in a subdirectory correctly' do + child_namespace = create(:group, name: 'sub-group', parent: namespace) + create(:project, namespace: child_namespace, path: 'hello-project') + + expected_path = File.join(TestEnv.repos_path, 'hello-group', 'renamed-sub-group', 'hello-project.git') + + subject.move_repositories(child_namespace, 'hello-group/sub-group', 'hello-group/renamed-sub-group') + + expect(File.directory?(expected_path)).to be(true) + end + + it 'moves a parent namespace with subdirectories' do + child_namespace = create(:group, name: 'sub-group', parent: namespace) + create(:project, namespace: child_namespace, path: 'hello-project') + expected_path = File.join(TestEnv.repos_path, 'renamed-group', 'sub-group', 'hello-project.git') + + subject.move_repositories(child_namespace, 'hello-group', 'renamed-group') + + expect(File.directory?(expected_path)).to be(true) + end + end + + describe '#move_namespace_folders' do + it 'moves a namespace with files' do + source = File.join(uploads_dir, 'parent-group', 'sub-group') + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, 'parent-group', 'moved-group') + FileUtils.touch(File.join(source, 'test.txt')) + expected_file = File.join(destination, 'test.txt') + + subject.move_namespace_folders(uploads_dir, File.join('parent-group', 'sub-group'), File.join('parent-group', 'moved-group')) + + expect(File.exist?(expected_file)).to be(true) + end + + it 'moves a parent namespace uploads' do + source = File.join(uploads_dir, 'parent-group', 'sub-group') + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, 'moved-parent', 'sub-group') + FileUtils.touch(File.join(source, 'test.txt')) + expected_file = File.join(destination, 'test.txt') + + subject.move_namespace_folders(uploads_dir, 'parent-group', 'moved-parent') + + expect(File.exist?(expected_file)).to be(true) + end + end + + describe "#child_ids_for_parent" do + it "collects child ids for all levels" do + parent = create(:namespace) + first_child = create(:namespace, parent: parent) + second_child = create(:namespace, parent: parent) + third_child = create(:namespace, parent: second_child) + all_ids = [parent.id, first_child.id, second_child.id, third_child.id] + + collected_ids = subject.child_ids_for_parent(parent, ids: [parent.id]) + + expect(collected_ids).to contain_exactly(*all_ids) + end + end + + describe "#remove_last_ocurrence" do + it "removes only the last occurance of a string" do + input = "this/is/a-word-to-replace/namespace/with/a-word-to-replace" + + expect(subject.remove_last_occurrence(input, "a-word-to-replace")) + .to eq("this/is/a-word-to-replace/namespace/with/") + end + end + + describe "#rename_namespace" do + let(:namespace) { create(:namespace, path: 'the-path') } + it "renames namespaces called the-path" do + subject.rename_namespace(namespace) + + expect(namespace.reload.path).to eq("the-path0") + end + + it "renames the route to the namespace" do + subject.rename_namespace(namespace) + + expect(Namespace.find(namespace.id).full_path).to eq("the-path0") + end + + it "renames the route for projects of the namespace" do + project = create(:project, path: "project-path", namespace: namespace) + + subject.rename_namespace(namespace) + + expect(project.route.reload.path).to eq("the-path0/project-path") + end + + it "moves the the repository for a project in the namespace" do + create(:project, namespace: namespace, path: "the-path-project") + expected_repo = File.join(TestEnv.repos_path, "the-path0", "the-path-project.git") + + subject.rename_namespace(namespace) + + expect(File.directory?(expected_repo)).to be(true) + end + + it "moves the uploads for the namespace" do + allow(subject).to receive(:move_namespace_folders).with(Settings.pages.path, "the-path", "the-path0") + expect(subject).to receive(:move_namespace_folders).with(uploads_dir, "the-path", "the-path0") + + subject.rename_namespace(namespace) + end + + it "moves the pages for the namespace" do + allow(subject).to receive(:move_namespace_folders).with(uploads_dir, "the-path", "the-path0") + expect(subject).to receive(:move_namespace_folders).with(Settings.pages.path, "the-path", "the-path0") + + subject.rename_namespace(namespace) + end + + context "the-path namespace -> subgroup -> the-path0 project" do + it "updates the route of the project correctly" do + subgroup = create(:group, path: "subgroup", parent: namespace) + project = create(:project, path: "the-path0", namespace: subgroup) + + subject.rename_namespace(namespace) + + expect(project.route.reload.path).to eq("the-path0/subgroup/the-path0") + end + end + end + + describe '#rename_namespaces' do + context 'top level namespaces' do + let!(:namespace) { create(:namespace, path: 'the-path') } + + it 'should rename the namespace' do + expect(subject).to receive(:rename_namespace). + with(migration_namespace(namespace)) + + subject.rename_namespaces(['the-path'], type: :top_level) + end + end + end +end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb new file mode 100644 index 00000000000..8b4ab6703c6 --- /dev/null +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb @@ -0,0 +1,29 @@ +require 'spec_helper' + +describe Gitlab::Database::RenameReservedPathsMigration do + let(:subject) do + ActiveRecord::Migration.new.extend( + Gitlab::Database::RenameReservedPathsMigration + ) + end + + describe '#rename_wildcard_paths' do + it 'should rename namespaces' do + expect(subject).to receive(:rename_namespaces). + with(['first-path', 'second-path'], type: :wildcard) + + subject.rename_wildcard_paths(['first-path', 'second-path']) + end + + it 'should rename projects' + end + + describe '#rename_root_paths' do + it 'should rename namespaces' do + expect(subject).to receive(:rename_namespaces). + with(['the-path'], type: :top_level) + + subject.rename_root_paths('the-path') + end + end +end From e3d6957812e6cf399c208b1109ccc81ee1ff9144 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 12 Apr 2017 20:26:02 +0200 Subject: [PATCH 13/38] Rename forbidden paths in a single migration --- ...121055_rename_forbidden_root_namespaces.rb | 247 ------------------ ...52317_rename_forbidden_child_namespaces.rb | 242 ----------------- ...405111106_rename_wildcard_project_names.rb | 85 ------ ...412174900_rename_reserved_dynamic_paths.rb | 39 +++ .../rename_forbidden_child_namespaces_spec.rb | 187 ------------- .../rename_forbidden_root_namespaces_spec.rb | 197 -------------- 6 files changed, 39 insertions(+), 958 deletions(-) delete mode 100644 db/post_migrate/20170403121055_rename_forbidden_root_namespaces.rb delete mode 100644 db/post_migrate/20170404152317_rename_forbidden_child_namespaces.rb delete mode 100644 db/post_migrate/20170405111106_rename_wildcard_project_names.rb create mode 100644 db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb delete mode 100644 spec/migrations/rename_forbidden_child_namespaces_spec.rb delete mode 100644 spec/migrations/rename_forbidden_root_namespaces_spec.rb diff --git a/db/post_migrate/20170403121055_rename_forbidden_root_namespaces.rb b/db/post_migrate/20170403121055_rename_forbidden_root_namespaces.rb deleted file mode 100644 index fb475cae465..00000000000 --- a/db/post_migrate/20170403121055_rename_forbidden_root_namespaces.rb +++ /dev/null @@ -1,247 +0,0 @@ -# See http://doc.gitlab.com/ce/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class RenameForbiddenRootNamespaces < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - include Gitlab::ShellAdapter - disable_ddl_transaction! - - class Namespace < ActiveRecord::Base - self.table_name = 'namespaces' - belongs_to :parent, class_name: "Namespace" - has_one :route, as: :source, autosave: true - has_many :children, class_name: "Namespace", foreign_key: :parent_id - has_many :projects - belongs_to :owner, class_name: "User" - - def full_path - if route && route.path.present? - @full_path ||= route.path - else - update_route if persisted? - - build_full_path - end - end - - def build_full_path - if parent && path - parent.full_path + '/' + path - else - path - end - end - - def update_route - prepare_route - route.save - end - - def prepare_route - route || build_route(source: self) - route.path = build_full_path - route.name = build_full_name - @full_path = nil - @full_name = nil - end - - def build_full_name - if parent && name - parent.human_name + ' / ' + name - else - name - end - end - - def human_name - owner&.name - end - end - - class Route < ActiveRecord::Base - self.table_name = 'routes' - belongs_to :source, polymorphic: true - - validates :source, presence: true - - validates :path, - length: { within: 1..255 }, - presence: true, - uniqueness: { case_sensitive: false } - end - - class Project < ActiveRecord::Base - self.table_name = 'projects' - - def repository_storage_path - Gitlab.config.repositories.storages[repository_storage]['path'] - end - end - - DOWNTIME = false - DISALLOWED_PATHS = %w[ - api - autocomplete - search - member - explore - uploads - import - notification_settings - abuse_reports - invites - help - koding - health_check - jwt - oauth - sent_notifications - ] - - def up - DISALLOWED_PATHS.each do |path| - say "Renaming namespaces called #{path}" - forbidden_namespaces_with_path(path).each do |namespace| - rename_namespace(namespace) - end - end - end - - def down - # nothing to do - end - - def rename_namespace(namespace) - old_path = namespace.path - old_full_path = namespace.full_path - # Only remove the last occurrence of the path name to get the parent namespace path - namespace_path = remove_last_occurrence(old_full_path, old_path) - new_path = rename_path(namespace_path, old_path) - new_full_path = if namespace_path.present? - File.join(namespace_path, new_path) - else - new_path - end - - Namespace.where(id: namespace).update_all(path: new_path) # skips callbacks & validations - - replace_statement = replace_sql(Route.arel_table[:path], old_full_path, new_full_path) - - update_column_in_batches(:routes, :path, replace_statement) do |table, query| - query.where(Route.arel_table[:path].matches("#{old_full_path}%")) - end - - clear_cache_for_namespace(namespace) - - # tasks here are based on `Namespace#move_dir` - move_repositories(namespace, old_full_path, new_full_path) - move_namespace_folders(uploads_dir, old_full_path, new_full_path) if file_storage? - move_namespace_folders(pages_dir, old_full_path, new_full_path) - end - - # This will replace the first occurance of a string in a column with - # the replacement - # On postgresql we can use `regexp_replace` for that. - # On mysql we remove the pattern from the beginning of the string, and - # concatenate the remaining part tot the replacement. - def replace_sql(column, pattern, replacement) - if Gitlab::Database.mysql? - substr = Arel::Nodes::NamedFunction.new("substring", [column, pattern.to_s.size + 1]) - concat = Arel::Nodes::NamedFunction.new("concat", [Arel::Nodes::Quoted.new(replacement.to_s), substr]) - Arel::Nodes::SqlLiteral.new(concat.to_sql) - else - replace = Arel::Nodes::NamedFunction.new("regexp_replace", [column, Arel::Nodes::Quoted.new(pattern.to_s), Arel::Nodes::Quoted.new(replacement.to_s)]) - Arel::Nodes::SqlLiteral.new(replace.to_sql) - end - end - - def remove_last_occurrence(string, pattern) - string.reverse.sub(pattern.reverse, "").reverse - end - - def move_namespace_folders(directory, old_relative_path, new_relative_path) - old_path = File.join(directory, old_relative_path) - return unless File.directory?(old_path) - - new_path = File.join(directory, new_relative_path) - FileUtils.mv(old_path, new_path) - end - - def move_repositories(namespace, old_full_path, new_full_path) - repo_paths_for_namespace(namespace).each do |repository_storage_path| - # Ensure old directory exists before moving it - gitlab_shell.add_namespace(repository_storage_path, old_full_path) - - unless gitlab_shell.mv_namespace(repository_storage_path, old_full_path, new_full_path) - say "Exception moving path #{repository_storage_path} from #{old_full_path} to #{new_full_path}" - end - end - end - - def rename_path(namespace_path, path_was) - counter = 0 - path = "#{path_was}#{counter}" - - while route_exists?(File.join(namespace_path, path)) - counter += 1 - path = "#{path_was}#{counter}" - end - - path - end - - def route_exists?(full_path) - Route.where(Route.arel_table[:path].matches(full_path)).any? - end - - def forbidden_namespaces_with_path(name) - Namespace.where(arel_table[:path].matches(name).and(arel_table[:parent_id].eq(nil))) - end - - def clear_cache_for_namespace(namespace) - project_ids = project_ids_for_namespace(namespace) - scopes = { "Project" => { id: project_ids }, - "Issue" => { project_id: project_ids }, - "MergeRequest" => { target_project_id: project_ids }, - "Note" => { project_id: project_ids } } - - ClearDatabaseCacheWorker.perform_async(scopes) - rescue => e - Rails.logger.error ["Couldn't clear the markdown cache: #{e.message}", e.backtrace.join("\n")].join("\n") - end - - def project_ids_for_namespace(namespace) - namespace_ids = child_ids_for_parent(namespace, ids: [namespace.id]) - namespace_or_children = Project.arel_table[:namespace_id].in(namespace_ids) - Project.unscoped.where(namespace_or_children).pluck(:id) - end - - # This won't scale to huge trees, but it should do for a handful of namespaces - def child_ids_for_parent(namespace, ids: []) - namespace.children.each do |child| - ids << child.id - child_ids_for_parent(child, ids: ids) if child.children.any? - end - ids - end - - def repo_paths_for_namespace(namespace) - namespace.projects.unscoped.select('distinct(repository_storage)').to_a.map(&:repository_storage_path) - end - - def uploads_dir - File.join(Rails.root, "public", "uploads") - end - - def pages_dir - Settings.pages.path - end - - def file_storage? - CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File - end - - def arel_table - Namespace.arel_table - end -end diff --git a/db/post_migrate/20170404152317_rename_forbidden_child_namespaces.rb b/db/post_migrate/20170404152317_rename_forbidden_child_namespaces.rb deleted file mode 100644 index 8b082a892d4..00000000000 --- a/db/post_migrate/20170404152317_rename_forbidden_child_namespaces.rb +++ /dev/null @@ -1,242 +0,0 @@ -# See http://doc.gitlab.com/ce/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class RenameForbiddenChildNamespaces < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - include Gitlab::ShellAdapter - disable_ddl_transaction! - - class Namespace < ActiveRecord::Base - self.table_name = 'namespaces' - belongs_to :parent, class_name: "Namespace" - has_one :route, as: :source, autosave: true - has_many :children, class_name: "Namespace", foreign_key: :parent_id - has_many :projects - belongs_to :owner, class_name: "User" - - def full_path - if route && route.path.present? - @full_path ||= route.path - else - update_route if persisted? - - build_full_path - end - end - - def build_full_path - if parent && path - parent.full_path + '/' + path - else - path - end - end - - def update_route - prepare_route - route.save - end - - def prepare_route - route || build_route(source: self) - route.path = build_full_path - route.name = build_full_name - @full_path = nil - @full_name = nil - end - - def build_full_name - if parent && name - parent.human_name + ' / ' + name - else - name - end - end - - def human_name - owner&.name - end - end - - class Route < ActiveRecord::Base - self.table_name = 'routes' - belongs_to :source, polymorphic: true - - validates :source, presence: true - - validates :path, - length: { within: 1..255 }, - presence: true, - uniqueness: { case_sensitive: false } - end - - class Project < ActiveRecord::Base - self.table_name = 'projects' - - def repository_storage_path - Gitlab.config.repositories.storages[repository_storage]['path'] - end - end - - DOWNTIME = false - DISALLOWED_PATHS = %w[info git-upload-pack - git-receive-pack gitlab-lfs autocomplete_sources - templates avatar commit pages compare network snippets - services mattermost deploy_keys forks import merge_requests - branches merged_branches tags protected_branches variables - triggers pipelines environments cycle_analytics builds - hooks container_registry milestones labels issues - project_members group_links notes noteable boards todos - uploads runners runner_projects settings repository - transfer remove_fork archive unarchive housekeeping - toggle_star preview_markdown export remove_export - generate_new_export download_export activity - new_issue_address] - - def up - DISALLOWED_PATHS.each do |path| - say "Renaming namespaces called #{path}" - forbidden_namespaces_with_path(path).each do |namespace| - rename_namespace(namespace) - end - end - end - - def down - # nothing to do - end - - def rename_namespace(namespace) - old_path = namespace.path - old_full_path = namespace.full_path - # Only remove the last occurrence of the path name to get the parent namespace path - namespace_path = remove_last_occurrence(old_full_path, old_path) - new_path = rename_path(namespace_path, old_path) - new_full_path = if namespace_path.present? - File.join(namespace_path, new_path) - else - new_path - end - - Namespace.where(id: namespace).update_all(path: new_path) # skips callbacks & validations - - replace_statement = replace_sql(Route.arel_table[:path], old_full_path, new_full_path) - - update_column_in_batches(:routes, :path, replace_statement) do |table, query| - query.where(Route.arel_table[:path].matches("#{old_full_path}%")) - end - - clear_cache_for_namespace(namespace) - - # tasks here are based on `Namespace#move_dir` - move_repositories(namespace, old_full_path, new_full_path) - move_namespace_folders(uploads_dir, old_full_path, new_full_path) if file_storage? - move_namespace_folders(pages_dir, old_full_path, new_full_path) - end - - # This will replace the first occurance of a string in a column with - # the replacement - # On postgresql we can use `regexp_replace` for that. - # On mysql we remove the pattern from the beginning of the string, and - # concatenate the remaining part tot the replacement. - def replace_sql(column, pattern, replacement) - if Gitlab::Database.mysql? - substr = Arel::Nodes::NamedFunction.new("substring", [column, pattern.to_s.size + 1]) - concat = Arel::Nodes::NamedFunction.new("concat", [Arel::Nodes::Quoted.new(replacement.to_s), substr]) - Arel::Nodes::SqlLiteral.new(concat.to_sql) - else - replace = Arel::Nodes::NamedFunction.new("regexp_replace", [column, Arel::Nodes::Quoted.new(pattern.to_s), Arel::Nodes::Quoted.new(replacement.to_s)]) - Arel::Nodes::SqlLiteral.new(replace.to_sql) - end - end - - def remove_last_occurrence(string, pattern) - string.reverse.sub(pattern.reverse, "").reverse - end - - def move_namespace_folders(directory, old_relative_path, new_relative_path) - old_path = File.join(directory, old_relative_path) - return unless File.directory?(old_path) - - new_path = File.join(directory, new_relative_path) - FileUtils.mv(old_path, new_path) - end - - def move_repositories(namespace, old_full_path, new_full_path) - repo_paths_for_namespace(namespace).each do |repository_storage_path| - # Ensure old directory exists before moving it - gitlab_shell.add_namespace(repository_storage_path, old_full_path) - - unless gitlab_shell.mv_namespace(repository_storage_path, old_full_path, new_full_path) - say "Exception moving path #{repository_storage_path} from #{old_full_path} to #{new_full_path}" - end - end - end - - def rename_path(namespace_path, path_was) - counter = 0 - path = "#{path_was}#{counter}" - - while route_exists?(File.join(namespace_path, path)) - counter += 1 - path = "#{path_was}#{counter}" - end - - path - end - - def route_exists?(full_path) - Route.where(Route.arel_table[:path].matches(full_path)).any? - end - - def forbidden_namespaces_with_path(path) - Namespace.where(arel_table[:parent_id].eq(nil).not).where(arel_table[:path].matches(path)) - end - - def clear_cache_for_namespace(namespace) - project_ids = project_ids_for_namespace(namespace) - scopes = { "Project" => { id: project_ids }, - "Issue" => { project_id: project_ids }, - "MergeRequest" => { target_project_id: project_ids }, - "Note" => { project_id: project_ids } } - - ClearDatabaseCacheWorker.perform_async(scopes) - rescue => e - Rails.logger.error ["Couldn't clear the markdown cache: #{e.message}", e.backtrace.join("\n")].join("\n") - end - - def project_ids_for_namespace(namespace) - namespace_ids = child_ids_for_parent(namespace, ids: [namespace.id]) - namespace_or_children = Project.arel_table[:namespace_id].in(namespace_ids) - Project.unscoped.where(namespace_or_children).pluck(:id) - end - - # This won't scale to huge trees, but it should do for a handful of namespaces - def child_ids_for_parent(namespace, ids: []) - namespace.children.each do |child| - ids << child.id - child_ids_for_parent(child, ids: ids) if child.children.any? - end - ids - end - - def repo_paths_for_namespace(namespace) - namespace.projects.unscoped.select('distinct(repository_storage)').to_a.map(&:repository_storage_path) - end - - def uploads_dir - File.join(Rails.root, "public", "uploads") - end - - def pages_dir - Settings.pages.path - end - - def file_storage? - CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File - end - - def arel_table - Namespace.arel_table - end -end diff --git a/db/post_migrate/20170405111106_rename_wildcard_project_names.rb b/db/post_migrate/20170405111106_rename_wildcard_project_names.rb deleted file mode 100644 index 1b8d2a40e99..00000000000 --- a/db/post_migrate/20170405111106_rename_wildcard_project_names.rb +++ /dev/null @@ -1,85 +0,0 @@ -# See http://doc.gitlab.com/ce/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class RenameWildcardProjectNames < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - include Gitlab::ShellAdapter - disable_ddl_transaction! - - DOWNTIME = false - KNOWN_PATHS = %w[info git-upload-pack - git-receive-pack gitlab-lfs autocomplete_sources - templates avatar commit pages compare network snippets - services mattermost deploy_keys forks import merge_requests - branches merged_branches tags protected_branches variables - triggers pipelines environments cycle_analytics builds - hooks container_registry milestones labels issues - project_members group_links notes noteable boards todos - uploads runners runner_projects settings repository - transfer remove_fork archive unarchive housekeeping - toggle_star preview_markdown export remove_export - generate_new_export download_export activity - new_issue_address].freeze - - def up - reserved_projects.find_in_batches(batch_size: 100) do |slice| - rename_projects(slice) - end - end - - def down - # nothing to do here - end - - private - - def reserved_projects - Project.unscoped. - includes(:namespace). - where('EXISTS (SELECT 1 FROM namespaces WHERE projects.namespace_id = namespaces.id)'). - where('projects.path' => KNOWN_PATHS) - end - - def route_exists?(full_path) - quoted_path = ActiveRecord::Base.connection.quote_string(full_path.downcase) - - ActiveRecord::Base.connection. - select_all("SELECT id, path FROM routes WHERE lower(path) = '#{quoted_path}'").present? - end - - # Adds number to the end of the path that is not taken by other route - def rename_path(namespace_path, path_was) - counter = 0 - path = "#{path_was}#{counter}" - - while route_exists?("#{namespace_path}/#{path}") - counter += 1 - path = "#{path_was}#{counter}" - end - - path - end - - def rename_projects(projects) - projects.each do |project| - id = project.id - path_was = project.path - namespace_path = project.namespace.path - path = rename_path(namespace_path, path_was) - - begin - # Because project path update is quite complex operation we can't safely - # copy-paste all code from GitLab. As exception we use Rails code here - project.rename_repo if rename_project_row(project, path) - rescue Exception => e # rubocop: disable Lint/RescueException - Rails.logger.error "Exception when renaming project #{id}: #{e.message}" - end - end - end - - def rename_project_row(project, path) - project.respond_to?(:update_attributes) && - project.update_attributes(path: path) && - project.respond_to?(:rename_repo) - end -end diff --git a/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb new file mode 100644 index 00000000000..fcab298eb09 --- /dev/null +++ b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb @@ -0,0 +1,39 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RenameReservedDynamicPaths < ActiveRecord::Migration + include Gitlab::Database::RenameReservedPathsMigration + + DOWNTIME = false + + disable_ddl_transaction! + + DISALLOWED_ROOT_PATHS = %w[ + api + autocomplete + member + explore + uploads + import + notification_settings + abuse_reports + invites + koding + health_check + jwt + oauth + sent_notifications + - + ] + + DISALLOWED_WILDCARD_PATHS = %w[objects folders file] + + def up + rename_root_paths(DISALLOWED_ROOT_PATHS) + rename_wildcard_paths(DISALLOWED_WILDCARD_PATHS) + end + + def down + # nothing to do + end +end diff --git a/spec/migrations/rename_forbidden_child_namespaces_spec.rb b/spec/migrations/rename_forbidden_child_namespaces_spec.rb deleted file mode 100644 index c5486e18052..00000000000 --- a/spec/migrations/rename_forbidden_child_namespaces_spec.rb +++ /dev/null @@ -1,187 +0,0 @@ -require 'spec_helper' -require Rails.root.join('db', 'post_migrate', '20170404152317_rename_forbidden_child_namespaces.rb') - -describe RenameForbiddenChildNamespaces, truncate: true do - let(:migration) { described_class.new } - let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_namespaces_test') } - let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } - let(:forbidden_namespace) do - namespace = build(:group, path: 'info') - namespace.parent = create(:group, path: 'parent') - namespace.save(validate: false) - namespace - end - - before do - FileUtils.remove_dir(test_dir) if File.directory?(test_dir) - FileUtils.mkdir_p(uploads_dir) - FileUtils.remove_dir(TestEnv.repos_path) if File.directory?(TestEnv.repos_path) - allow(migration).to receive(:say) - allow(migration).to receive(:uploads_dir).and_return(uploads_dir) - end - - describe '#forbidden_namespaces_with_path' do - let(:other_namespace) { create(:group, path: 'info') } - before do - forbidden_namespace - other_namespace - end - - it 'includes namespaces called with path `info`' do - expect(migration.forbidden_namespaces_with_path('info').map(&:id)).to contain_exactly(forbidden_namespace.id) - end - end - - describe '#up' do - before do - forbidden_namespace - end - - it 'renames namespaces called info' do - migration.up - - expect(forbidden_namespace.reload.path).to eq('info0') - end - - it 'renames the route to the namespace' do - migration.up - - expect(forbidden_namespace.reload.full_path).to eq('parent/info0') - end - - it 'renames the route for projects of the namespace' do - project = create(:project, path: 'project-path', namespace: forbidden_namespace) - - migration.up - - expect(project.route.reload.path).to eq('parent/info0/project-path') - end - - it 'moves the the repository for a project in the namespace' do - create(:project, namespace: forbidden_namespace, path: 'info-project') - expected_repo = File.join(TestEnv.repos_path, 'parent/info0', 'info-project.git') - - migration.up - - expect(File.directory?(expected_repo)).to be(true) - end - - it 'moves the uploads for the namespace' do - allow(migration).to receive(:move_namespace_folders).with(Settings.pages.path, 'parent/info', 'parent/info0') - expect(migration).to receive(:move_namespace_folders).with(uploads_dir, 'parent/info', 'parent/info0') - - migration.up - end - - it 'moves the pages for the namespace' do - allow(migration).to receive(:move_namespace_folders).with(uploads_dir, 'parent/info', 'parent/info0') - expect(migration).to receive(:move_namespace_folders).with(Settings.pages.path, 'parent/info', 'parent/info0') - - migration.up - end - - it 'clears the markdown cache for projects in the forbidden namespace' do - project = create(:project, namespace: forbidden_namespace) - scopes = { 'Project' => { id: [project.id] }, - 'Issue' => { project_id: [project.id] }, - 'MergeRequest' => { target_project_id: [project.id] }, - 'Note' => { project_id: [project.id] } } - - expect(ClearDatabaseCacheWorker).to receive(:perform_async).with(scopes) - - migration.up - end - - context 'forbidden namespace -> subgroup -> info0 project' do - it 'updates the route of the project correctly' do - subgroup = create(:group, path: 'subgroup', parent: forbidden_namespace) - project = create(:project, path: 'info0', namespace: subgroup) - - migration.up - - expect(project.route.reload.path).to eq('parent/info0/subgroup/info0') - end - end - end - - describe '#move_repositories' do - let(:namespace) { create(:group, name: 'hello-group') } - it 'moves a project for a namespace' do - create(:project, namespace: namespace, path: 'hello-project') - expected_path = File.join(TestEnv.repos_path, 'bye-group', 'hello-project.git') - - migration.move_repositories(namespace, 'hello-group', 'bye-group') - - expect(File.directory?(expected_path)).to be(true) - end - - it 'moves a namespace in a subdirectory correctly' do - child_namespace = create(:group, name: 'sub-group', parent: namespace) - create(:project, namespace: child_namespace, path: 'hello-project') - - expected_path = File.join(TestEnv.repos_path, 'hello-group', 'renamed-sub-group', 'hello-project.git') - - migration.move_repositories(child_namespace, 'hello-group/sub-group', 'hello-group/renamed-sub-group') - - expect(File.directory?(expected_path)).to be(true) - end - - it 'moves a parent namespace with subdirectories' do - child_namespace = create(:group, name: 'sub-group', parent: namespace) - create(:project, namespace: child_namespace, path: 'hello-project') - expected_path = File.join(TestEnv.repos_path, 'renamed-group', 'sub-group', 'hello-project.git') - - migration.move_repositories(child_namespace, 'hello-group', 'renamed-group') - - expect(File.directory?(expected_path)).to be(true) - end - end - - describe '#move_namespace_folders' do - it 'moves a namespace with files' do - source = File.join(uploads_dir, 'parent-group', 'sub-group') - FileUtils.mkdir_p(source) - destination = File.join(uploads_dir, 'parent-group', 'moved-group') - FileUtils.touch(File.join(source, 'test.txt')) - expected_file = File.join(destination, 'test.txt') - - migration.move_namespace_folders(uploads_dir, File.join('parent-group', 'sub-group'), File.join('parent-group', 'moved-group')) - - expect(File.exist?(expected_file)).to be(true) - end - - it 'moves a parent namespace uploads' do - source = File.join(uploads_dir, 'parent-group', 'sub-group') - FileUtils.mkdir_p(source) - destination = File.join(uploads_dir, 'moved-parent', 'sub-group') - FileUtils.touch(File.join(source, 'test.txt')) - expected_file = File.join(destination, 'test.txt') - - migration.move_namespace_folders(uploads_dir, 'parent-group', 'moved-parent') - - expect(File.exist?(expected_file)).to be(true) - end - end - - describe '#child_ids_for_parent' do - it 'collects child ids for all levels' do - parent = create(:namespace) - first_child = create(:namespace, parent: parent) - second_child = create(:namespace, parent: parent) - third_child = create(:namespace, parent: second_child) - all_ids = [parent.id, first_child.id, second_child.id, third_child.id] - - collected_ids = migration.child_ids_for_parent(parent, ids: [parent.id]) - - expect(collected_ids).to contain_exactly(*all_ids) - end - end - - describe '#remove_last_ocurrence' do - it 'removes only the last occurance of a string' do - input = 'this/is/info/namespace/with/info' - - expect(migration.remove_last_occurrence(input, 'info')).to eq('this/is/info/namespace/with/') - end - end -end diff --git a/spec/migrations/rename_forbidden_root_namespaces_spec.rb b/spec/migrations/rename_forbidden_root_namespaces_spec.rb deleted file mode 100644 index ca806e08475..00000000000 --- a/spec/migrations/rename_forbidden_root_namespaces_spec.rb +++ /dev/null @@ -1,197 +0,0 @@ -require 'spec_helper' -require Rails.root.join('db', 'post_migrate', '20170403121055_rename_forbidden_root_namespaces.rb') - -describe RenameForbiddenRootNamespaces, truncate: true do - let(:migration) { described_class.new } - let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_namespaces_test') } - let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } - let(:forbidden_namespace) do - namespace = build(:namespace, path: 'api') - namespace.save(validate: false) - namespace - end - - before do - FileUtils.remove_dir(test_dir) if File.directory?(test_dir) - FileUtils.mkdir_p(uploads_dir) - FileUtils.remove_dir(TestEnv.repos_path) if File.directory?(TestEnv.repos_path) - allow(migration).to receive(:say) - allow(migration).to receive(:uploads_dir).and_return(uploads_dir) - end - - describe '#forbidden_namespaces_with_path' do - before do - forbidden_namespace - end - - it 'includes namespaces called with path `api`' do - expect(migration.forbidden_namespaces_with_path('api').map(&:id)).to include(forbidden_namespace.id) - end - end - - describe '#up' do - before do - forbidden_namespace - end - - it 'renames namespaces called api' do - migration.up - - expect(forbidden_namespace.reload.path).to eq('api0') - end - - it 'renames the route to the namespace' do - migration.up - - expect(forbidden_namespace.reload.full_path).to eq('api0') - end - - it 'renames the route for projects of the namespace' do - project = create(:project, path: 'project-path', namespace: forbidden_namespace) - - migration.up - - expect(project.route.reload.path).to eq('api0/project-path') - end - - it 'moves the the repository for a project in the namespace' do - create(:project, namespace: forbidden_namespace, path: 'api-project') - expected_repo = File.join(TestEnv.repos_path, 'api0', 'api-project.git') - - migration.up - - expect(File.directory?(expected_repo)).to be(true) - end - - it 'moves the uploads for the namespace' do - allow(migration).to receive(:move_namespace_folders).with(Settings.pages.path, 'api', 'api0') - expect(migration).to receive(:move_namespace_folders).with(uploads_dir, 'api', 'api0') - - migration.up - end - - it 'moves the pages for the namespace' do - allow(migration).to receive(:move_namespace_folders).with(uploads_dir, 'api', 'api0') - expect(migration).to receive(:move_namespace_folders).with(Settings.pages.path, 'api', 'api0') - - migration.up - end - - it 'clears the markdown cache for projects in the forbidden namespace' do - project = create(:project, namespace: forbidden_namespace) - scopes = { 'Project' => { id: [project.id] }, - 'Issue' => { project_id: [project.id] }, - 'MergeRequest' => { target_project_id: [project.id] }, - 'Note' => { project_id: [project.id] } } - - expect(ClearDatabaseCacheWorker).to receive(:perform_async).with(scopes) - - migration.up - end - - context 'forbidden namespace -> subgroup -> api0 project' do - it 'updates the route of the project correctly' do - subgroup = create(:group, path: 'subgroup', parent: forbidden_namespace) - project = create(:project, path: 'api0', namespace: subgroup) - - migration.up - - expect(project.route.reload.path).to eq('api0/subgroup/api0') - end - end - - context 'for a sub-namespace' do - before do - forbidden_namespace.parent = create(:namespace, path: 'parent') - forbidden_namespace.save(validate: false) - end - - it "doesn't rename child-namespace paths" do - migration.up - - expect(forbidden_namespace.reload.full_path).to eq('parent/api') - end - end - end - - describe '#move_repositories' do - let(:namespace) { create(:group, name: 'hello-group') } - it 'moves a project for a namespace' do - create(:project, namespace: namespace, path: 'hello-project') - expected_path = File.join(TestEnv.repos_path, 'bye-group', 'hello-project.git') - - migration.move_repositories(namespace, 'hello-group', 'bye-group') - - expect(File.directory?(expected_path)).to be(true) - end - - it 'moves a namespace in a subdirectory correctly' do - child_namespace = create(:group, name: 'sub-group', parent: namespace) - create(:project, namespace: child_namespace, path: 'hello-project') - - expected_path = File.join(TestEnv.repos_path, 'hello-group', 'renamed-sub-group', 'hello-project.git') - - migration.move_repositories(child_namespace, 'hello-group/sub-group', 'hello-group/renamed-sub-group') - - expect(File.directory?(expected_path)).to be(true) - end - - it 'moves a parent namespace with subdirectories' do - child_namespace = create(:group, name: 'sub-group', parent: namespace) - create(:project, namespace: child_namespace, path: 'hello-project') - expected_path = File.join(TestEnv.repos_path, 'renamed-group', 'sub-group', 'hello-project.git') - - migration.move_repositories(child_namespace, 'hello-group', 'renamed-group') - - expect(File.directory?(expected_path)).to be(true) - end - end - - describe '#move_namespace_folders' do - it 'moves a namespace with files' do - source = File.join(uploads_dir, 'parent-group', 'sub-group') - FileUtils.mkdir_p(source) - destination = File.join(uploads_dir, 'parent-group', 'moved-group') - FileUtils.touch(File.join(source, 'test.txt')) - expected_file = File.join(destination, 'test.txt') - - migration.move_namespace_folders(uploads_dir, File.join('parent-group', 'sub-group'), File.join('parent-group', 'moved-group')) - - expect(File.exist?(expected_file)).to be(true) - end - - it 'moves a parent namespace uploads' do - source = File.join(uploads_dir, 'parent-group', 'sub-group') - FileUtils.mkdir_p(source) - destination = File.join(uploads_dir, 'moved-parent', 'sub-group') - FileUtils.touch(File.join(source, 'test.txt')) - expected_file = File.join(destination, 'test.txt') - - migration.move_namespace_folders(uploads_dir, 'parent-group', 'moved-parent') - - expect(File.exist?(expected_file)).to be(true) - end - end - - describe '#child_ids_for_parent' do - it 'collects child ids for all levels' do - parent = create(:namespace) - first_child = create(:namespace, parent: parent) - second_child = create(:namespace, parent: parent) - third_child = create(:namespace, parent: second_child) - all_ids = [parent.id, first_child.id, second_child.id, third_child.id] - - collected_ids = migration.child_ids_for_parent(parent, ids: [parent.id]) - - expect(collected_ids).to contain_exactly(*all_ids) - end - end - - describe '#remove_last_ocurrence' do - it 'removes only the last occurance of a string' do - input = 'this/is/api/namespace/with/api' - - expect(migration.remove_last_occurrence(input, 'api')).to eq('this/is/api/namespace/with/') - end - end -end From 7508ee56670dd960275b6438be91471020ea62ab Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 13 Apr 2017 16:54:48 +0200 Subject: [PATCH 14/38] Make renaming records in the database reusable So we can use it for projects --- .../rename_reserved_paths_migration.rb | 41 +++++++++- .../migration_classes.rb | 49 +++++++----- .../namespaces.rb | 29 +------ .../namespaces_spec.rb | 71 ++++++---------- .../rename_reserved_paths_migration_spec.rb | 80 +++++++++++++++++++ 5 files changed, 175 insertions(+), 95 deletions(-) diff --git a/lib/gitlab/database/rename_reserved_paths_migration.rb b/lib/gitlab/database/rename_reserved_paths_migration.rb index ad5570b4c72..2cfc01ab2f5 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration.rb @@ -15,11 +15,38 @@ module Gitlab rename_namespaces(paths, type: :top_level) end + def rename_path_for_routable(routable) + old_path = routable.path + old_full_path = routable.full_path + # Only remove the last occurrence of the path name to get the parent namespace path + namespace_path = remove_last_occurrence(old_full_path, old_path) + new_path = rename_path(namespace_path, old_path) + new_full_path = join_routable_path(namespace_path, new_path) + + # skips callbacks & validations + routable.class.where(id: routable). + update_all(path: new_path) + + rename_routes(old_full_path, new_full_path) + + [old_full_path, new_full_path] + end + + def rename_routes(old_full_path, new_full_path) + replace_statement = replace_sql(Route.arel_table[:path], + old_full_path, + new_full_path) + + update_column_in_batches(:routes, :path, replace_statement) do |table, query| + query.where(MigrationClasses::Route.arel_table[:path].matches("#{old_full_path}%")) + end + end + def rename_path(namespace_path, path_was) counter = 0 path = "#{path_was}#{counter}" - while route_exists?(File.join(namespace_path, path)) + while route_exists?(join_routable_path(namespace_path, path)) counter += 1 path = "#{path_was}#{counter}" end @@ -27,6 +54,18 @@ module Gitlab path end + def remove_last_occurrence(string, pattern) + string.reverse.sub(pattern.reverse, "").reverse + end + + def join_routable_path(namespace_path, top_level) + if namespace_path.present? + File.join(namespace_path, top_level) + else + top_level + end + end + def route_exists?(full_path) MigrationClasses::Route.where(Route.arel_table[:path].matches(full_path)).any? end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb b/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb index a919d250541..d725402ace4 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb @@ -2,26 +2,7 @@ module Gitlab module Database module RenameReservedPathsMigration module MigrationClasses - class User < ActiveRecord::Base - self.table_name = 'users' - end - - class Namespace < ActiveRecord::Base - self.table_name = 'namespaces' - belongs_to :parent, - class_name: "#{MigrationClasses.name}::Namespace" - has_one :route, as: :source - has_many :children, - class_name: "#{MigrationClasses.name}::Namespace", - foreign_key: :parent_id - belongs_to :owner, - class_name: "#{MigrationClasses.name}::User" - - # Overridden to have the correct `source_type` for the `route` relation - def self.name - 'Namespace' - end - + module Routable def full_path if route && route.path.present? @full_path ||= route.path @@ -66,17 +47,45 @@ module Gitlab end end + class User < ActiveRecord::Base + self.table_name = 'users' + end + + class Namespace < ActiveRecord::Base + include MigrationClasses::Routable + self.table_name = 'namespaces' + belongs_to :parent, + class_name: "#{MigrationClasses.name}::Namespace" + has_one :route, as: :source + has_many :children, + class_name: "#{MigrationClasses.name}::Namespace", + foreign_key: :parent_id + belongs_to :owner, + class_name: "#{MigrationClasses.name}::User" + + # Overridden to have the correct `source_type` for the `route` relation + def self.name + 'Namespace' + end + end + class Route < ActiveRecord::Base self.table_name = 'routes' belongs_to :source, polymorphic: true end class Project < ActiveRecord::Base + include MigrationClasses::Routable self.table_name = 'projects' def repository_storage_path Gitlab.config.repositories.storages[repository_storage]['path'] end + + # Overridden to have the correct `source_type` for the `route` relation + def self.name + 'Project' + end end end end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb b/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb index f8fbeaa990a..b4f2a67fd06 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb @@ -16,32 +16,11 @@ module Gitlab elsif type == :top_level MigrationClasses::Namespace.where(parent_id: nil) end - namespaces.where(path: paths.map(&:downcase)) + namespaces.where('lower(path) in (?)', paths.map(&:downcase)) end def rename_namespace(namespace) - old_path = namespace.path - old_full_path = namespace.full_path - # Only remove the last occurrence of the path name to get the parent namespace path - namespace_path = remove_last_occurrence(old_full_path, old_path) - new_path = rename_path(namespace_path, old_path) - new_full_path = if namespace_path.present? - File.join(namespace_path, new_path) - else - new_path - end - - # skips callbacks & validations - MigrationClasses::Namespace.where(id: namespace). - update_all(path: new_path) - - replace_statement = replace_sql(Route.arel_table[:path], - old_full_path, - new_full_path) - - update_column_in_batches(:routes, :path, replace_statement) do |table, query| - query.where(MigrationClasses::Route.arel_table[:path].matches("#{old_full_path}%")) - end + old_full_path, new_full_path = rename_path_for_routable(namespace) move_repositories(namespace, old_full_path, new_full_path) move_namespace_folders(uploads_dir, old_full_path, new_full_path) if file_storage? @@ -92,10 +71,6 @@ module Gitlab ids end - def remove_last_occurrence(string, pattern) - string.reverse.sub(pattern.reverse, "").reverse - end - def file_storage? CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb index ea31e373647..3e5f981c095 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb @@ -25,7 +25,7 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d describe '#namespaces_for_paths' do context 'for wildcard namespaces' do it 'only returns child namespaces with the correct path' do - _root_namespace = create(:namespace, path: 'the-path') + _root_namespace = create(:namespace, path: 'THE-path') _other_path = create(:namespace, path: 'other', parent: create(:namespace)) @@ -33,13 +33,13 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d path: 'the-path', parent: create(:namespace)) - found_ids = subject.namespaces_for_paths(['the-path'], type: :wildcard). - pluck(:id) + found_ids = subject.namespaces_for_paths(['the-PATH'], type: :wildcard). + map(&:id) expect(found_ids).to contain_exactly(namespace.id) end end - context 'for top level namespaces' do + context 'for top levelnamespaces' do it 'only returns child namespaces with the correct path' do root_namespace = create(:namespace, path: 'the-path') _other_path = create(:namespace, path: 'other') @@ -48,7 +48,7 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d parent: create(:namespace)) found_ids = subject.namespaces_for_paths(['the-path'], type: :top_level). - pluck(:id) + map(&:id) expect(found_ids).to contain_exactly(root_namespace.id) end end @@ -127,35 +127,15 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d end end - describe "#remove_last_ocurrence" do - it "removes only the last occurance of a string" do - input = "this/is/a-word-to-replace/namespace/with/a-word-to-replace" - - expect(subject.remove_last_occurrence(input, "a-word-to-replace")) - .to eq("this/is/a-word-to-replace/namespace/with/") - end - end - describe "#rename_namespace" do let(:namespace) { create(:namespace, path: 'the-path') } - it "renames namespaces called the-path" do - subject.rename_namespace(namespace) - expect(namespace.reload.path).to eq("the-path0") - end - - it "renames the route to the namespace" do - subject.rename_namespace(namespace) - - expect(Namespace.find(namespace.id).full_path).to eq("the-path0") - end - - it "renames the route for projects of the namespace" do - project = create(:project, path: "project-path", namespace: namespace) + it 'renames paths & routes for the namesapce' do + expect(subject).to receive(:rename_path_for_routable). + with(namespace). + and_call_original subject.rename_namespace(namespace) - - expect(project.route.reload.path).to eq("the-path0/project-path") end it "moves the the repository for a project in the namespace" do @@ -180,29 +160,26 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d subject.rename_namespace(namespace) end - - context "the-path namespace -> subgroup -> the-path0 project" do - it "updates the route of the project correctly" do - subgroup = create(:group, path: "subgroup", parent: namespace) - project = create(:project, path: "the-path0", namespace: subgroup) - - subject.rename_namespace(namespace) - - expect(project.route.reload.path).to eq("the-path0/subgroup/the-path0") - end - end end describe '#rename_namespaces' do - context 'top level namespaces' do - let!(:namespace) { create(:namespace, path: 'the-path') } + let!(:top_level_namespace) { create(:namespace, path: 'the-path') } + let!(:child_namespace) do + create(:namespace, path: 'the-path', parent: create(:namespace)) + end - it 'should rename the namespace' do - expect(subject).to receive(:rename_namespace). - with(migration_namespace(namespace)) + it 'renames top level namespaces the namespace' do + expect(subject).to receive(:rename_namespace). + with(migration_namespace(top_level_namespace)) - subject.rename_namespaces(['the-path'], type: :top_level) - end + subject.rename_namespaces(['the-path'], type: :top_level) + end + + it 'renames child namespaces' do + expect(subject).to receive(:rename_namespace). + with(migration_namespace(child_namespace)) + + subject.rename_namespaces(['the-path'], type: :wildcard) end end end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb index 8b4ab6703c6..8fc15c8f716 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb @@ -7,6 +7,10 @@ describe Gitlab::Database::RenameReservedPathsMigration do ) end + before do + allow(subject).to receive(:say) + end + describe '#rename_wildcard_paths' do it 'should rename namespaces' do expect(subject).to receive(:rename_namespaces). @@ -26,4 +30,80 @@ describe Gitlab::Database::RenameReservedPathsMigration do subject.rename_root_paths('the-path') end end + + describe "#remove_last_ocurrence" do + it "removes only the last occurance of a string" do + input = "this/is/a-word-to-replace/namespace/with/a-word-to-replace" + + expect(subject.remove_last_occurrence(input, "a-word-to-replace")) + .to eq("this/is/a-word-to-replace/namespace/with/") + end + end + + describe '#rename_path_for_routable' do + context 'for namespaces' do + let(:namespace) { create(:namespace, path: 'the-path') } + it "renames namespaces called the-path" do + subject.rename_path_for_routable(namespace) + + expect(namespace.reload.path).to eq("the-path0") + end + + it "renames the route to the namespace" do + subject.rename_path_for_routable(namespace) + + expect(Namespace.find(namespace.id).full_path).to eq("the-path0") + end + + it "renames the route for projects of the namespace" do + project = create(:project, path: "project-path", namespace: namespace) + + subject.rename_path_for_routable(namespace) + + expect(project.route.reload.path).to eq("the-path0/project-path") + end + + it 'returns the old & the new path' do + old_path, new_path = subject.rename_path_for_routable(namespace) + + expect(old_path).to eq('the-path') + expect(new_path).to eq('the-path0') + end + + context "the-path namespace -> subgroup -> the-path0 project" do + it "updates the route of the project correctly" do + subgroup = create(:group, path: "subgroup", parent: namespace) + project = create(:project, path: "the-path0", namespace: subgroup) + + subject.rename_path_for_routable(namespace) + + expect(project.route.reload.path).to eq("the-path0/subgroup/the-path0") + end + end + end + + context 'for projects' do + let(:parent) { create(:namespace, path: 'the-parent') } + let(:project) { create(:empty_project, path: 'the-path', namespace: parent) } + + it 'renames the project called `the-path`' do + subject.rename_path_for_routable(project) + + expect(project.reload.path).to eq('the-path0') + end + + it 'renames the route for the project' do + subject.rename_path_for_routable(project) + + expect(project.reload.route.path).to eq('the-parent/the-path0') + end + + it 'returns the old & new path' do + old_path, new_path = subject.rename_path_for_routable(project) + + expect(old_path).to eq('the-parent/the-path') + expect(new_path).to eq('the-parent/the-path0') + end + end + end end From 579d8891d550cfbbcb433ed4966c6de37c710e83 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 13 Apr 2017 18:50:36 +0200 Subject: [PATCH 15/38] Rename projects in a migrationhelper --- .../rename_reserved_paths_migration.rb | 31 ++++++++ .../namespaces.rb | 28 ++----- .../projects.rb | 38 ++++++++++ .../namespaces_spec.rb | 42 +--------- .../projects_spec.rb | 76 +++++++++++++++++++ .../rename_reserved_paths_migration_spec.rb | 48 +++++++++++- 6 files changed, 201 insertions(+), 62 deletions(-) create mode 100644 lib/gitlab/database/rename_reserved_paths_migration/projects.rb create mode 100644 spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb diff --git a/lib/gitlab/database/rename_reserved_paths_migration.rb b/lib/gitlab/database/rename_reserved_paths_migration.rb index 2cfc01ab2f5..5314f933435 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration.rb @@ -8,6 +8,7 @@ module Gitlab def rename_wildcard_paths(one_or_more_paths) paths = Array(one_or_more_paths) rename_namespaces(paths, type: :wildcard) + rename_projects(paths) end def rename_root_paths(paths) @@ -69,6 +70,36 @@ module Gitlab def route_exists?(full_path) MigrationClasses::Route.where(Route.arel_table[:path].matches(full_path)).any? end + + def move_pages(old_path, new_path) + move_folders(pages_dir, old_path, new_path) + end + + def move_uploads(old_path, new_path) + return unless file_storage? + + move_folders(uploads_dir, old_path, new_path) + end + + def move_folders(directory, old_relative_path, new_relative_path) + old_path = File.join(directory, old_relative_path) + return unless File.directory?(old_path) + + new_path = File.join(directory, new_relative_path) + FileUtils.mv(old_path, new_path) + end + + def file_storage? + CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File + end + + def uploads_dir + File.join(CarrierWave.root, "uploads") + end + + def pages_dir + Settings.pages.path + end end end end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb b/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb index b4f2a67fd06..2ef2629f4c2 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb @@ -16,23 +16,17 @@ module Gitlab elsif type == :top_level MigrationClasses::Namespace.where(parent_id: nil) end - namespaces.where('lower(path) in (?)', paths.map(&:downcase)) + with_paths = MigrationClasses::Namespace.arel_table[:path]. + matches_any(paths) + namespaces.where(with_paths) end def rename_namespace(namespace) old_full_path, new_full_path = rename_path_for_routable(namespace) move_repositories(namespace, old_full_path, new_full_path) - move_namespace_folders(uploads_dir, old_full_path, new_full_path) if file_storage? - move_namespace_folders(pages_dir, old_full_path, new_full_path) - end - - def move_namespace_folders(directory, old_relative_path, new_relative_path) - old_path = File.join(directory, old_relative_path) - return unless File.directory?(old_path) - - new_path = File.join(directory, new_relative_path) - FileUtils.mv(old_path, new_path) + move_uploads(old_full_path, new_full_path) + move_pages(old_full_path, new_full_path) end def move_repositories(namespace, old_full_path, new_full_path) @@ -70,18 +64,6 @@ module Gitlab end ids end - - def file_storage? - CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File - end - - def uploads_dir - File.join(CarrierWave.root, "uploads") - end - - def pages_dir - Settings.pages.path - end end end end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/projects.rb b/lib/gitlab/database/rename_reserved_paths_migration/projects.rb new file mode 100644 index 00000000000..a2c9354e430 --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/projects.rb @@ -0,0 +1,38 @@ +module Gitlab + module Database + module RenameReservedPathsMigration + module Projects + include Gitlab::ShellAdapter + + def rename_projects(paths) + projects_for_paths(paths).each do |project| + rename_project(project) + end + end + + def rename_project(project) + old_full_path, new_full_path = rename_path_for_routable(project) + + move_repository(project, old_full_path, new_full_path) + move_repository(project, "#{old_full_path}.wiki", "#{new_full_path}.wiki") + move_uploads(old_full_path, new_full_path) + move_pages(old_full_path, new_full_path) + end + + def move_repository(project, old_path, new_path) + unless gitlab_shell.mv_repository(project.repository_storage_path, + old_path, + new_path) + Rails.logger.error "Error moving #{old_path} to #{new_path}" + end + end + + def projects_for_paths(paths) + with_paths = MigrationClasses::Project.arel_table[:path] + .matches_any(paths) + MigrationClasses::Project.where(with_paths) + end + end + end + end +end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb index 3e5f981c095..f6d95f04073 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb @@ -1,8 +1,6 @@ require 'spec_helper' describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate do - let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_namespaces_test') } - let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } let(:subject) do ActiveRecord::Migration.new.extend( Gitlab::Database::RenameReservedPathsMigration @@ -10,10 +8,6 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d end before do - FileUtils.remove_dir(test_dir) if File.directory?(test_dir) - FileUtils.mkdir_p(uploads_dir) - FileUtils.remove_dir(TestEnv.repos_path) if File.directory?(TestEnv.repos_path) - allow(subject).to receive(:uploads_dir).and_return(uploads_dir) allow(subject).to receive(:say) end @@ -44,8 +38,8 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d root_namespace = create(:namespace, path: 'the-path') _other_path = create(:namespace, path: 'other') _child_namespace = create(:namespace, - path: 'the-path', - parent: create(:namespace)) + path: 'the-path', + parent: create(:namespace)) found_ids = subject.namespaces_for_paths(['the-path'], type: :top_level). map(&:id) @@ -87,32 +81,6 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d end end - describe '#move_namespace_folders' do - it 'moves a namespace with files' do - source = File.join(uploads_dir, 'parent-group', 'sub-group') - FileUtils.mkdir_p(source) - destination = File.join(uploads_dir, 'parent-group', 'moved-group') - FileUtils.touch(File.join(source, 'test.txt')) - expected_file = File.join(destination, 'test.txt') - - subject.move_namespace_folders(uploads_dir, File.join('parent-group', 'sub-group'), File.join('parent-group', 'moved-group')) - - expect(File.exist?(expected_file)).to be(true) - end - - it 'moves a parent namespace uploads' do - source = File.join(uploads_dir, 'parent-group', 'sub-group') - FileUtils.mkdir_p(source) - destination = File.join(uploads_dir, 'moved-parent', 'sub-group') - FileUtils.touch(File.join(source, 'test.txt')) - expected_file = File.join(destination, 'test.txt') - - subject.move_namespace_folders(uploads_dir, 'parent-group', 'moved-parent') - - expect(File.exist?(expected_file)).to be(true) - end - end - describe "#child_ids_for_parent" do it "collects child ids for all levels" do parent = create(:namespace) @@ -148,15 +116,13 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d end it "moves the uploads for the namespace" do - allow(subject).to receive(:move_namespace_folders).with(Settings.pages.path, "the-path", "the-path0") - expect(subject).to receive(:move_namespace_folders).with(uploads_dir, "the-path", "the-path0") + expect(subject).to receive(:move_uploads).with("the-path", "the-path0") subject.rename_namespace(namespace) end it "moves the pages for the namespace" do - allow(subject).to receive(:move_namespace_folders).with(uploads_dir, "the-path", "the-path0") - expect(subject).to receive(:move_namespace_folders).with(Settings.pages.path, "the-path", "the-path0") + expect(subject).to receive(:move_pages).with("the-path", "the-path0") subject.rename_namespace(namespace) end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb new file mode 100644 index 00000000000..7d3344faa10 --- /dev/null +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb @@ -0,0 +1,76 @@ +require 'spec_helper' + +describe Gitlab::Database::RenameReservedPathsMigration::Projects, :truncate do + let(:subject) do + ActiveRecord::Migration.new.extend( + Gitlab::Database::RenameReservedPathsMigration + ) + end + + before do + allow(subject).to receive(:say) + end + + describe '#projects_for_paths' do + it 'includes the correct projects' do + project = create(:empty_project, path: 'THE-path') + _other_project = create(:empty_project) + + result_ids = subject.projects_for_paths(['the-PATH']).map(&:id) + + expect(result_ids).to contain_exactly(project.id) + end + end + + describe '#rename_project' do + let(:project) do + create(:empty_project, + path: 'the-path', + namespace: create(:namespace, path: 'known-parent' )) + end + + it 'renames path & route for the project' do + expect(subject).to receive(:rename_path_for_routable). + with(project). + and_call_original + + subject.rename_project(project) + end + + it 'moves the wiki & the repo' do + expect(subject).to receive(:move_repository). + with(project, 'known-parent/the-path.wiki', 'known-parent/the-path0.wiki') + expect(subject).to receive(:move_repository). + with(project, 'known-parent/the-path', 'known-parent/the-path0') + + subject.rename_project(project) + end + + it 'moves uploads' do + expect(subject).to receive(:move_uploads). + with('known-parent/the-path', 'known-parent/the-path0') + + subject.rename_project(project) + end + + it 'moves pages' do + expect(subject).to receive(:move_pages). + with('known-parent/the-path', 'known-parent/the-path0') + + subject.rename_project(project) + end + end + + describe '#move_repository' do + let(:known_parent) { create(:namespace, path: 'known-parent') } + let(:project) { create(:project, path: 'the-path', namespace: known_parent) } + + it 'moves the repository for a project' do + expected_path = File.join(TestEnv.repos_path, 'known-parent', 'new-repo.git') + + subject.move_repository(project, 'known-parent/the-path', 'known-parent/new-repo') + + expect(File.directory?(expected_path)).to be(true) + end + end +end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb index 8fc15c8f716..d3021d330be 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb @@ -19,7 +19,11 @@ describe Gitlab::Database::RenameReservedPathsMigration do subject.rename_wildcard_paths(['first-path', 'second-path']) end - it 'should rename projects' + it 'should rename projects' do + expect(subject).to receive(:rename_projects).with(['the-path']) + + subject.rename_wildcard_paths(['the-path']) + end end describe '#rename_root_paths' do @@ -106,4 +110,46 @@ describe Gitlab::Database::RenameReservedPathsMigration do end end end + + describe "#move_uploads" do + let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_reserved_paths') } + let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } + + it 'moves subdirectories in the uploads folder' do + expect(subject).to receive(:uploads_dir).and_return(uploads_dir) + expect(subject).to receive(:move_folders).with(uploads_dir, 'old_path', 'new_path') + + subject.move_uploads('old_path', 'new_path') + end + + it "doesn't move uploads when they are stored in object storage" do + expect(subject).to receive(:file_storage?).and_return(false) + expect(subject).not_to receive(:move_folders) + + subject.move_uploads('old_path', 'new_path') + end + end + + describe '#move_folders' do + let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_reserved_paths') } + let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + allow(subject).to receive(:uploads_dir).and_return(uploads_dir) + end + + it 'moves a folder with files' do + source = File.join(uploads_dir, 'parent-group', 'sub-group') + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, 'parent-group', 'moved-group') + FileUtils.touch(File.join(source, 'test.txt')) + expected_file = File.join(destination, 'test.txt') + + subject.move_folders(uploads_dir, File.join('parent-group', 'sub-group'), File.join('parent-group', 'moved-group')) + + expect(File.exist?(expected_file)).to be(true) + end + end end From 0369ef14525cac86b015a21fa0d01b1cad627fc1 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 14 Apr 2017 13:55:09 +0200 Subject: [PATCH 16/38] Add a WIP spec for clearing the cache --- .../database/rename_reserved_paths_migration/namespaces_spec.rb | 2 ++ .../database/rename_reserved_paths_migration/projects_spec.rb | 2 ++ 2 files changed, 4 insertions(+) diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb index f6d95f04073..fb24800e1f1 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb @@ -126,6 +126,8 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d subject.rename_namespace(namespace) end + + it 'invalidates the markdown cache of related projects' end describe '#rename_namespaces' do diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb index 7d3344faa10..1812e3ea7f4 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb @@ -59,6 +59,8 @@ describe Gitlab::Database::RenameReservedPathsMigration::Projects, :truncate do subject.rename_project(project) end + + it 'invalidates the markdown cache of related projects' end describe '#move_repository' do From 27f54bebb29a1e56251ac2d669c2883aeaf1cb1c Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 14 Apr 2017 19:29:22 +0200 Subject: [PATCH 17/38] Use objects for renaming namespaces and projects --- .../rename_reserved_paths_migration.rb | 97 +----------- .../migration_classes.rb | 1 + .../rename_base.rb | 103 ++++++++++++ .../{namespaces.rb => rename_namespaces.rb} | 10 +- .../{projects.rb => rename_projects.rb} | 8 +- .../rename_base_spec.rb | 147 ++++++++++++++++++ ...aces_spec.rb => rename_namespaces_spec.rb} | 23 ++- ...ojects_spec.rb => rename_projects_spec.rb} | 15 +- .../rename_reserved_paths_migration_spec.rb | 147 +++--------------- spec/support/fake_migration_classes.rb | 3 + 10 files changed, 306 insertions(+), 248 deletions(-) create mode 100644 lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb rename lib/gitlab/database/rename_reserved_paths_migration/{namespaces.rb => rename_namespaces.rb} (90%) rename lib/gitlab/database/rename_reserved_paths_migration/{projects.rb => rename_projects.rb} (88%) create mode 100644 spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb rename spec/lib/gitlab/database/rename_reserved_paths_migration/{namespaces_spec.rb => rename_namespaces_spec.rb} (89%) rename spec/lib/gitlab/database/rename_reserved_paths_migration/{projects_spec.rb => rename_projects_spec.rb} (85%) create mode 100644 spec/support/fake_migration_classes.rb diff --git a/lib/gitlab/database/rename_reserved_paths_migration.rb b/lib/gitlab/database/rename_reserved_paths_migration.rb index 5314f933435..0507ae4da51 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration.rb @@ -1,104 +1,19 @@ module Gitlab module Database module RenameReservedPathsMigration - include MigrationHelpers - include Namespaces - include Projects + def self.included(kls) + kls.include(MigrationHelpers) + end def rename_wildcard_paths(one_or_more_paths) paths = Array(one_or_more_paths) - rename_namespaces(paths, type: :wildcard) - rename_projects(paths) + RenameNamespaces.new(paths, self).rename_namespaces(type: :wildcard) + RenameProjects.new(paths, self).rename_projects end def rename_root_paths(paths) paths = Array(paths) - rename_namespaces(paths, type: :top_level) - end - - def rename_path_for_routable(routable) - old_path = routable.path - old_full_path = routable.full_path - # Only remove the last occurrence of the path name to get the parent namespace path - namespace_path = remove_last_occurrence(old_full_path, old_path) - new_path = rename_path(namespace_path, old_path) - new_full_path = join_routable_path(namespace_path, new_path) - - # skips callbacks & validations - routable.class.where(id: routable). - update_all(path: new_path) - - rename_routes(old_full_path, new_full_path) - - [old_full_path, new_full_path] - end - - def rename_routes(old_full_path, new_full_path) - replace_statement = replace_sql(Route.arel_table[:path], - old_full_path, - new_full_path) - - update_column_in_batches(:routes, :path, replace_statement) do |table, query| - query.where(MigrationClasses::Route.arel_table[:path].matches("#{old_full_path}%")) - end - end - - def rename_path(namespace_path, path_was) - counter = 0 - path = "#{path_was}#{counter}" - - while route_exists?(join_routable_path(namespace_path, path)) - counter += 1 - path = "#{path_was}#{counter}" - end - - path - end - - def remove_last_occurrence(string, pattern) - string.reverse.sub(pattern.reverse, "").reverse - end - - def join_routable_path(namespace_path, top_level) - if namespace_path.present? - File.join(namespace_path, top_level) - else - top_level - end - end - - def route_exists?(full_path) - MigrationClasses::Route.where(Route.arel_table[:path].matches(full_path)).any? - end - - def move_pages(old_path, new_path) - move_folders(pages_dir, old_path, new_path) - end - - def move_uploads(old_path, new_path) - return unless file_storage? - - move_folders(uploads_dir, old_path, new_path) - end - - def move_folders(directory, old_relative_path, new_relative_path) - old_path = File.join(directory, old_relative_path) - return unless File.directory?(old_path) - - new_path = File.join(directory, new_relative_path) - FileUtils.mv(old_path, new_path) - end - - def file_storage? - CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File - end - - def uploads_dir - File.join(CarrierWave.root, "uploads") - end - - def pages_dir - Settings.pages.path + RenameNamespaces.new(paths, self).rename_namespaces(type: :top_level) end end end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb b/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb index d725402ace4..9133b97d239 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb @@ -76,6 +76,7 @@ module Gitlab class Project < ActiveRecord::Base include MigrationClasses::Routable + has_one :route, as: :source self.table_name = 'projects' def repository_storage_path diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb new file mode 100644 index 00000000000..367348a9a42 --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb @@ -0,0 +1,103 @@ +module Gitlab + module Database + module RenameReservedPathsMigration + class RenameBase + attr_reader :paths, :migration + + delegate :update_column_in_batches, + :replace_sql, + to: :migration + + def initialize(paths, migration) + @paths = paths + @migration = migration + end + + def rename_path_for_routable(routable) + old_path = routable.path + old_full_path = routable.full_path + # Only remove the last occurrence of the path name to get the parent namespace path + namespace_path = remove_last_occurrence(old_full_path, old_path) + new_path = rename_path(namespace_path, old_path) + new_full_path = join_routable_path(namespace_path, new_path) + + # skips callbacks & validations + routable.class.where(id: routable). + update_all(path: new_path) + + rename_routes(old_full_path, new_full_path) + + [old_full_path, new_full_path] + end + + def rename_routes(old_full_path, new_full_path) + replace_statement = replace_sql(Route.arel_table[:path], + old_full_path, + new_full_path) + + update_column_in_batches(:routes, :path, replace_statement) do |table, query| + query.where(MigrationClasses::Route.arel_table[:path].matches("#{old_full_path}%")) + end + end + + def rename_path(namespace_path, path_was) + counter = 0 + path = "#{path_was}#{counter}" + + while route_exists?(join_routable_path(namespace_path, path)) + counter += 1 + path = "#{path_was}#{counter}" + end + + path + end + + def remove_last_occurrence(string, pattern) + string.reverse.sub(pattern.reverse, "").reverse + end + + def join_routable_path(namespace_path, top_level) + if namespace_path.present? + File.join(namespace_path, top_level) + else + top_level + end + end + + def route_exists?(full_path) + MigrationClasses::Route.where(Route.arel_table[:path].matches(full_path)).any? + end + + def move_pages(old_path, new_path) + move_folders(pages_dir, old_path, new_path) + end + + def move_uploads(old_path, new_path) + return unless file_storage? + + move_folders(uploads_dir, old_path, new_path) + end + + def move_folders(directory, old_relative_path, new_relative_path) + old_path = File.join(directory, old_relative_path) + return unless File.directory?(old_path) + + new_path = File.join(directory, new_relative_path) + FileUtils.mv(old_path, new_path) + end + + def file_storage? + CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File + end + + def uploads_dir + File.join(CarrierWave.root, "uploads") + end + + def pages_dir + Settings.pages.path + end + end + end + end +end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb similarity index 90% rename from lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb rename to lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb index 2ef2629f4c2..80e8135ea93 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/namespaces.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb @@ -1,16 +1,16 @@ module Gitlab module Database module RenameReservedPathsMigration - module Namespaces + class RenameNamespaces < RenameBase include Gitlab::ShellAdapter - def rename_namespaces(paths, type:) - namespaces_for_paths(paths, type: type).each do |namespace| + def rename_namespaces(type:) + namespaces_for_paths(type: type).each do |namespace| rename_namespace(namespace) end end - def namespaces_for_paths(paths, type:) + def namespaces_for_paths(type:) namespaces = if type == :wildcard MigrationClasses::Namespace.where.not(parent_id: nil) elsif type == :top_level @@ -52,7 +52,7 @@ module Gitlab namespace_or_children = MigrationClasses::Project. arel_table[:namespace_id]. in(namespace_ids) - MigrationClasses::Project.unscoped.where(namespace_or_children) + MigrationClasses::Project.where(namespace_or_children) end # This won't scale to huge trees, but it should do for a handful of diff --git a/lib/gitlab/database/rename_reserved_paths_migration/projects.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb similarity index 88% rename from lib/gitlab/database/rename_reserved_paths_migration/projects.rb rename to lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb index a2c9354e430..02f10d8e951 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/projects.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb @@ -1,11 +1,11 @@ module Gitlab module Database module RenameReservedPathsMigration - module Projects + class RenameProjects < RenameBase include Gitlab::ShellAdapter - def rename_projects(paths) - projects_for_paths(paths).each do |project| + def rename_projects + projects_for_paths.each do |project| rename_project(project) end end @@ -27,7 +27,7 @@ module Gitlab end end - def projects_for_paths(paths) + def projects_for_paths with_paths = MigrationClasses::Project.arel_table[:path] .matches_any(paths) MigrationClasses::Project.where(with_paths) diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb new file mode 100644 index 00000000000..48234170d31 --- /dev/null +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb @@ -0,0 +1,147 @@ +require 'spec_helper' + +describe Gitlab::Database::RenameReservedPathsMigration::RenameBase do + let(:migration) { FakeRenameReservedPathMigration.new } + let(:subject) { described_class.new(['the-path'], migration) } + + before do + allow(migration).to receive(:say) + end + + def migration_namespace(namespace) + Gitlab::Database::RenameReservedPathsMigration::MigrationClasses:: + Namespace.find(namespace.id) + end + + def migration_project(project) + Gitlab::Database::RenameReservedPathsMigration::MigrationClasses:: + Project.find(project.id) + end + + describe "#remove_last_ocurrence" do + it "removes only the last occurance of a string" do + input = "this/is/a-word-to-replace/namespace/with/a-word-to-replace" + + expect(subject.remove_last_occurrence(input, "a-word-to-replace")) + .to eq("this/is/a-word-to-replace/namespace/with/") + end + end + + describe '#rename_path_for_routable' do + context 'for namespaces' do + let(:namespace) { create(:namespace, path: 'the-path') } + it "renames namespaces called the-path" do + subject.rename_path_for_routable(migration_namespace(namespace)) + + expect(namespace.reload.path).to eq("the-path0") + end + + it "renames the route to the namespace" do + subject.rename_path_for_routable(migration_namespace(namespace)) + + expect(Namespace.find(namespace.id).full_path).to eq("the-path0") + end + + it "renames the route for projects of the namespace" do + project = create(:project, path: "project-path", namespace: namespace) + + subject.rename_path_for_routable(migration_namespace(namespace)) + + expect(project.route.reload.path).to eq("the-path0/project-path") + end + + it 'returns the old & the new path' do + old_path, new_path = subject.rename_path_for_routable(migration_namespace(namespace)) + + expect(old_path).to eq('the-path') + expect(new_path).to eq('the-path0') + end + + context "the-path namespace -> subgroup -> the-path0 project" do + it "updates the route of the project correctly" do + subgroup = create(:group, path: "subgroup", parent: namespace) + project = create(:project, path: "the-path0", namespace: subgroup) + + subject.rename_path_for_routable(migration_namespace(namespace)) + + expect(project.route.reload.path).to eq("the-path0/subgroup/the-path0") + end + end + end + + context 'for projects' do + let(:parent) { create(:namespace, path: 'the-parent') } + let(:project) { create(:empty_project, path: 'the-path', namespace: parent) } + + it 'renames the project called `the-path`' do + subject.rename_path_for_routable(migration_project(project)) + + expect(project.reload.path).to eq('the-path0') + end + + it 'renames the route for the project' do + subject.rename_path_for_routable(project) + + expect(project.reload.route.path).to eq('the-parent/the-path0') + end + + it 'returns the old & new path' do + old_path, new_path = subject.rename_path_for_routable(migration_project(project)) + + expect(old_path).to eq('the-parent/the-path') + expect(new_path).to eq('the-parent/the-path0') + end + end + end + + describe '#move_pages' do + it 'moves the pages directory' do + expect(subject).to receive(:move_folders) + .with(TestEnv.pages_path, 'old-path', 'new-path') + + subject.move_pages('old-path', 'new-path') + end + end + + describe "#move_uploads" do + let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_reserved_paths') } + let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } + + it 'moves subdirectories in the uploads folder' do + expect(subject).to receive(:uploads_dir).and_return(uploads_dir) + expect(subject).to receive(:move_folders).with(uploads_dir, 'old_path', 'new_path') + + subject.move_uploads('old_path', 'new_path') + end + + it "doesn't move uploads when they are stored in object storage" do + expect(subject).to receive(:file_storage?).and_return(false) + expect(subject).not_to receive(:move_folders) + + subject.move_uploads('old_path', 'new_path') + end + end + + describe '#move_folders' do + let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_reserved_paths') } + let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + allow(subject).to receive(:uploads_dir).and_return(uploads_dir) + end + + it 'moves a folder with files' do + source = File.join(uploads_dir, 'parent-group', 'sub-group') + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, 'parent-group', 'moved-group') + FileUtils.touch(File.join(source, 'test.txt')) + expected_file = File.join(destination, 'test.txt') + + subject.move_folders(uploads_dir, File.join('parent-group', 'sub-group'), File.join('parent-group', 'moved-group')) + + expect(File.exist?(expected_file)).to be(true) + end + end +end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb similarity index 89% rename from spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb rename to spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb index fb24800e1f1..ee481e4610c 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/namespaces_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb @@ -1,14 +1,11 @@ require 'spec_helper' -describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate do - let(:subject) do - ActiveRecord::Migration.new.extend( - Gitlab::Database::RenameReservedPathsMigration - ) - end +describe Gitlab::Database::RenameReservedPathsMigration::RenameNamespaces do + let(:migration) { FakeRenameReservedPathMigration.new } + let(:subject) { described_class.new(['the-path'], migration) } before do - allow(subject).to receive(:say) + allow(migration).to receive(:say) end def migration_namespace(namespace) @@ -27,7 +24,7 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d path: 'the-path', parent: create(:namespace)) - found_ids = subject.namespaces_for_paths(['the-PATH'], type: :wildcard). + found_ids = subject.namespaces_for_paths(type: :wildcard). map(&:id) expect(found_ids).to contain_exactly(namespace.id) end @@ -41,7 +38,7 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d path: 'the-path', parent: create(:namespace)) - found_ids = subject.namespaces_for_paths(['the-path'], type: :top_level). + found_ids = subject.namespaces_for_paths(type: :top_level). map(&:id) expect(found_ids).to contain_exactly(root_namespace.id) end @@ -98,12 +95,14 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d describe "#rename_namespace" do let(:namespace) { create(:namespace, path: 'the-path') } - it 'renames paths & routes for the namesapce' do + it 'renames paths & routes for the namespace' do expect(subject).to receive(:rename_path_for_routable). with(namespace). and_call_original subject.rename_namespace(namespace) + + expect(namespace.reload.path).to eq('the-path0') end it "moves the the repository for a project in the namespace" do @@ -140,14 +139,14 @@ describe Gitlab::Database::RenameReservedPathsMigration::Namespaces, :truncate d expect(subject).to receive(:rename_namespace). with(migration_namespace(top_level_namespace)) - subject.rename_namespaces(['the-path'], type: :top_level) + subject.rename_namespaces(type: :top_level) end it 'renames child namespaces' do expect(subject).to receive(:rename_namespace). with(migration_namespace(child_namespace)) - subject.rename_namespaces(['the-path'], type: :wildcard) + subject.rename_namespaces(type: :wildcard) end end end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb similarity index 85% rename from spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb rename to spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb index 1812e3ea7f4..4a572133b69 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/projects_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb @@ -1,14 +1,11 @@ require 'spec_helper' -describe Gitlab::Database::RenameReservedPathsMigration::Projects, :truncate do - let(:subject) do - ActiveRecord::Migration.new.extend( - Gitlab::Database::RenameReservedPathsMigration - ) - end +describe Gitlab::Database::RenameReservedPathsMigration::RenameProjects do + let(:migration) { FakeRenameReservedPathMigration.new } + let(:subject) { described_class.new(['the-path'], migration) } before do - allow(subject).to receive(:say) + allow(migration).to receive(:say) end describe '#projects_for_paths' do @@ -16,7 +13,7 @@ describe Gitlab::Database::RenameReservedPathsMigration::Projects, :truncate do project = create(:empty_project, path: 'THE-path') _other_project = create(:empty_project) - result_ids = subject.projects_for_paths(['the-PATH']).map(&:id) + result_ids = subject.projects_for_paths.map(&:id) expect(result_ids).to contain_exactly(project.id) end @@ -35,6 +32,8 @@ describe Gitlab::Database::RenameReservedPathsMigration::Projects, :truncate do and_call_original subject.rename_project(project) + + expect(project.reload.path).to eq('the-path0') end it 'moves the wiki & the repo' do diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb index d3021d330be..b46595ba628 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb @@ -1,11 +1,7 @@ require 'spec_helper' describe Gitlab::Database::RenameReservedPathsMigration do - let(:subject) do - ActiveRecord::Migration.new.extend( - Gitlab::Database::RenameReservedPathsMigration - ) - end + let(:subject) { FakeRenameReservedPathMigration.new } before do allow(subject).to receive(:say) @@ -13,14 +9,23 @@ describe Gitlab::Database::RenameReservedPathsMigration do describe '#rename_wildcard_paths' do it 'should rename namespaces' do - expect(subject).to receive(:rename_namespaces). - with(['first-path', 'second-path'], type: :wildcard) + rename_namespaces = double + expect(Gitlab::Database::RenameReservedPathsMigration::RenameNamespaces). + to receive(:new).with(['first-path', 'second-path'], subject). + and_return(rename_namespaces) + expect(rename_namespaces).to receive(:rename_namespaces). + with(type: :wildcard) subject.rename_wildcard_paths(['first-path', 'second-path']) end it 'should rename projects' do - expect(subject).to receive(:rename_projects).with(['the-path']) + rename_projects = double + expect(Gitlab::Database::RenameReservedPathsMigration::RenameProjects). + to receive(:new).with(['the-path'], subject). + and_return(rename_projects) + + expect(rename_projects).to receive(:rename_projects) subject.rename_wildcard_paths(['the-path']) end @@ -28,128 +33,14 @@ describe Gitlab::Database::RenameReservedPathsMigration do describe '#rename_root_paths' do it 'should rename namespaces' do - expect(subject).to receive(:rename_namespaces). - with(['the-path'], type: :top_level) + rename_namespaces = double + expect(Gitlab::Database::RenameReservedPathsMigration::RenameNamespaces). + to receive(:new).with(['the-path'], subject). + and_return(rename_namespaces) + expect(rename_namespaces).to receive(:rename_namespaces). + with(type: :top_level) subject.rename_root_paths('the-path') end end - - describe "#remove_last_ocurrence" do - it "removes only the last occurance of a string" do - input = "this/is/a-word-to-replace/namespace/with/a-word-to-replace" - - expect(subject.remove_last_occurrence(input, "a-word-to-replace")) - .to eq("this/is/a-word-to-replace/namespace/with/") - end - end - - describe '#rename_path_for_routable' do - context 'for namespaces' do - let(:namespace) { create(:namespace, path: 'the-path') } - it "renames namespaces called the-path" do - subject.rename_path_for_routable(namespace) - - expect(namespace.reload.path).to eq("the-path0") - end - - it "renames the route to the namespace" do - subject.rename_path_for_routable(namespace) - - expect(Namespace.find(namespace.id).full_path).to eq("the-path0") - end - - it "renames the route for projects of the namespace" do - project = create(:project, path: "project-path", namespace: namespace) - - subject.rename_path_for_routable(namespace) - - expect(project.route.reload.path).to eq("the-path0/project-path") - end - - it 'returns the old & the new path' do - old_path, new_path = subject.rename_path_for_routable(namespace) - - expect(old_path).to eq('the-path') - expect(new_path).to eq('the-path0') - end - - context "the-path namespace -> subgroup -> the-path0 project" do - it "updates the route of the project correctly" do - subgroup = create(:group, path: "subgroup", parent: namespace) - project = create(:project, path: "the-path0", namespace: subgroup) - - subject.rename_path_for_routable(namespace) - - expect(project.route.reload.path).to eq("the-path0/subgroup/the-path0") - end - end - end - - context 'for projects' do - let(:parent) { create(:namespace, path: 'the-parent') } - let(:project) { create(:empty_project, path: 'the-path', namespace: parent) } - - it 'renames the project called `the-path`' do - subject.rename_path_for_routable(project) - - expect(project.reload.path).to eq('the-path0') - end - - it 'renames the route for the project' do - subject.rename_path_for_routable(project) - - expect(project.reload.route.path).to eq('the-parent/the-path0') - end - - it 'returns the old & new path' do - old_path, new_path = subject.rename_path_for_routable(project) - - expect(old_path).to eq('the-parent/the-path') - expect(new_path).to eq('the-parent/the-path0') - end - end - end - - describe "#move_uploads" do - let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_reserved_paths') } - let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } - - it 'moves subdirectories in the uploads folder' do - expect(subject).to receive(:uploads_dir).and_return(uploads_dir) - expect(subject).to receive(:move_folders).with(uploads_dir, 'old_path', 'new_path') - - subject.move_uploads('old_path', 'new_path') - end - - it "doesn't move uploads when they are stored in object storage" do - expect(subject).to receive(:file_storage?).and_return(false) - expect(subject).not_to receive(:move_folders) - - subject.move_uploads('old_path', 'new_path') - end - end - - describe '#move_folders' do - let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'rename_reserved_paths') } - let(:uploads_dir) { File.join(test_dir, 'public', 'uploads') } - - before do - FileUtils.remove_dir(test_dir) if File.directory?(test_dir) - FileUtils.mkdir_p(uploads_dir) - allow(subject).to receive(:uploads_dir).and_return(uploads_dir) - end - - it 'moves a folder with files' do - source = File.join(uploads_dir, 'parent-group', 'sub-group') - FileUtils.mkdir_p(source) - destination = File.join(uploads_dir, 'parent-group', 'moved-group') - FileUtils.touch(File.join(source, 'test.txt')) - expected_file = File.join(destination, 'test.txt') - - subject.move_folders(uploads_dir, File.join('parent-group', 'sub-group'), File.join('parent-group', 'moved-group')) - - expect(File.exist?(expected_file)).to be(true) - end - end end diff --git a/spec/support/fake_migration_classes.rb b/spec/support/fake_migration_classes.rb new file mode 100644 index 00000000000..7ff36fe434c --- /dev/null +++ b/spec/support/fake_migration_classes.rb @@ -0,0 +1,3 @@ +class FakeRenameReservedPathMigration < ActiveRecord::Migration + include Gitlab::Database::RenameReservedPathsMigration +end From c5059cb4f705191074d3371beb81a3f0a67473af Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 14 Apr 2017 20:06:20 +0200 Subject: [PATCH 18/38] Make path validation case-insensitive --- app/validators/dynamic_path_validator.rb | 1 + spec/validators/dynamic_path_validator_spec.rb | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index 7587a3f20e1..3b93a0d0b28 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -94,6 +94,7 @@ class DynamicPathValidator < ActiveModel::EachValidator end def self.reserved?(value, type: :strict) + value = value.to_s.downcase case type when :wildcard WILDCARD_ROUTES.include?(value) diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb index 6c490b1be2e..b931a4a2dc3 100644 --- a/spec/validators/dynamic_path_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -81,7 +81,13 @@ describe DynamicPathValidator do end end - describe '#valid_full_path' do + describe ".valid?" do + it 'is not case sensitive' do + expect(described_class.valid?("Users", type: :top_level)).to be(false) + end + end + + describe '.valid_full_path' do it "isn't valid when the top level is reserved" do test_path = 'u/should-be-a/reserved-word' From ab5f9027fb2a78f9c15b3f4d0fcd20ed998e5272 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 14 Apr 2017 20:07:34 +0200 Subject: [PATCH 19/38] Rename namespaces called `Users` This should rename the already created namespace that snuck trough because the validation was case sensitive --- db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb index fcab298eb09..73a59ef0d74 100644 --- a/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb +++ b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb @@ -24,6 +24,7 @@ class RenameReservedDynamicPaths < ActiveRecord::Migration oauth sent_notifications - + users ] DISALLOWED_WILDCARD_PATHS = %w[objects folders file] From e50f4bc066e4477e9c59708f978383b071dc2959 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 18 Apr 2017 16:27:11 +0200 Subject: [PATCH 20/38] The dynamic path validator can block out partial paths So we can block `objects` only when it is contained in `info/lfs` or `gitlab-lfs` --- app/validators/dynamic_path_validator.rb | 61 ++++---- lib/constraints/group_url_constrainer.rb | 2 +- lib/constraints/project_url_constrainer.rb | 2 +- spec/models/namespace_spec.rb | 7 + spec/models/project_spec.rb | 7 + .../validators/dynamic_path_validator_spec.rb | 146 ++++++++++-------- 6 files changed, 125 insertions(+), 100 deletions(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index 3b93a0d0b28..6f04263d4d5 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -71,40 +71,42 @@ class DynamicPathValidator < ActiveModel::EachValidator # without tree as reserved name routing can match 'group/project' as group name, # 'tree' as project name and 'deploy_keys' as route. # - WILDCARD_ROUTES = Set.new(%w[tree commits wikis new edit create update logs_tree preview blob blame raw files create_dir find_file - artifacts graphs refs badges objects folders file]) + artifacts graphs refs badges info/lfs/objects + gitlab-lfs/objects environments/folders]) STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES).freeze - def self.valid_full_path?(full_path) - path_segments = full_path.split('/') - root_segment = path_segments.shift + def self.valid?(path) + path_segments = path.split('/') - valid?(root_segment, type: :top_level) && valid_wildcard_segments?(path_segments) + !reserved?(path) && path_segments.all? { |value| follow_format?(value) } end - def self.valid_wildcard_segments?(segments) - segments.all? { |segment| valid?(segment, type: :wildcard) } + def self.reserved?(path) + path = path.to_s.downcase + top_level, wildcard_part = path.split('/', 2) + + includes_reserved_top_level?(top_level) || includes_reserved_wildcard?(wildcard_part) end - def self.valid?(value, type: :strict) - !reserved?(value, type: type) && follow_format?(value) - end - - def self.reserved?(value, type: :strict) - value = value.to_s.downcase - case type - when :wildcard - WILDCARD_ROUTES.include?(value) - when :top_level - TOP_LEVEL_ROUTES.include?(value) - else - STRICT_RESERVED.include?(value) + def self.includes_reserved_wildcard?(path) + WILDCARD_ROUTES.any? do |reserved_word| + contains_path_part?(path, reserved_word) end end + def self.includes_reserved_top_level?(path) + TOP_LEVEL_ROUTES.any? do |reserved_route| + contains_path_part?(path, reserved_route) + end + end + + def self.contains_path_part?(path, part) + path =~ /(.*\/|\A)#{Regexp.quote(part)}(\/.*|\z)/ + end + def self.follow_format?(value) value =~ Gitlab::Regex.namespace_regex end @@ -116,21 +118,10 @@ class DynamicPathValidator < ActiveModel::EachValidator record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) end - if reserved?(value, type: validation_type(record)) + full_path = record.respond_to?(:full_path) ? record.full_path : value + + if reserved?(full_path) record.errors.add(attribute, "#{value} is a reserved name") end end - - def validation_type(record) - case record - when Namespace - record.has_parent? ? :wildcard : :top_level - when Project - :wildcard - when User - :top_level - else - :strict - end - end end diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb index 48a33690cb2..1501f64d537 100644 --- a/lib/constraints/group_url_constrainer.rb +++ b/lib/constraints/group_url_constrainer.rb @@ -2,7 +2,7 @@ class GroupUrlConstrainer def matches?(request) id = request.params[:id] - return false unless DynamicPathValidator.valid_full_path?(id) + return false unless DynamicPathValidator.valid?(id) Group.find_by_full_path(id).present? end diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb index 72c457d0369..064e1bd78e3 100644 --- a/lib/constraints/project_url_constrainer.rb +++ b/lib/constraints/project_url_constrainer.rb @@ -4,7 +4,7 @@ class ProjectUrlConstrainer project_path = request.params[:project_id] || request.params[:id] full_path = namespace_path + '/' + project_path - unless DynamicPathValidator.valid_full_path?(full_path) + unless DynamicPathValidator.valid?(full_path) return false end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 6775fd2f1b8..8624616316c 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -34,6 +34,13 @@ describe Namespace, models: true do let(:group) { build(:group, :nested, path: 'tree') } it { expect(group).not_to be_valid } + + it 'rejects nested paths' do + parent = create(:group, :nested, path: 'environments') + namespace = build(:project, path: 'folders', namespace: parent) + + expect(namespace).not_to be_valid + end end context 'top-level group' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 726dade93f6..04c90b651a0 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -266,6 +266,13 @@ describe Project, models: true do expect(project).not_to be_valid end + + it 'rejects nested paths' do + parent = create(:group, :nested, path: 'environments') + project = build(:project, path: 'folders', namespace: parent) + + expect(project).not_to be_valid + end end end diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb index b931a4a2dc3..71098b17dfd 100644 --- a/spec/validators/dynamic_path_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -12,23 +12,30 @@ describe DynamicPathValidator do # Pass in a full path and get the last segment before a wildcard # That's not a parameter # `/*namespace_id/:project_id/builds/artifacts/*ref_name_and_path` - # -> 'artifacts' - def segment_before_last_wildcard(path) - path_segments = path.split('/').reject { |segment| segment.empty? } - last_wildcard_index = path_segments.rindex { |part| part.starts_with?('*') } + # -> 'builds/artifacts' + def path_between_wildcards(path) + path = path.gsub(STARTING_WITH_NAMESPACE, "") + path_segments = path.split('/').reject(&:empty?) + wildcard_index = path_segments.index { |segment| segment.starts_with?('*') } - index_of_non_param_segment = last_wildcard_index - 1 - part_before_wildcard = path_segments[index_of_non_param_segment] - while parameter?(part_before_wildcard) - index_of_non_param_segment -= 1 - part_before_wildcard = path_segments[index_of_non_param_segment] + segments_before_wildcard = path_segments[0..wildcard_index - 1] + + param_index = segments_before_wildcard.index { |segment| segment.starts_with?(':') } + if param_index + segments_before_wildcard = segments_before_wildcard[param_index + 1..-1] end - part_before_wildcard + segments_before_wildcard.join('/') end - def parameter?(path_segment) - path_segment.starts_with?(':') || path_segment.starts_with?('*') + # If the path is reserved. Then no conflicting paths can# be created for any + # route using this reserved word. + # + # Both `builds/artifacts` & `artifacts/file` are covered by reserving the word + # `artifacts` + def wildcards_include?(path) + described_class::WILDCARD_ROUTES.include?(path) || + path.split('/').any? { |segment| described_class::WILDCARD_ROUTES.include?(segment) } end let(:all_routes) do @@ -42,14 +49,20 @@ describe DynamicPathValidator do # all routes not starting with a param let(:routes_not_starting_in_wildcard) { routes_without_format.select { |p| p !~ %r{^/[:*]} } } + let(:top_level_words) do + routes_not_starting_in_wildcard.map do |route| + route.split('/')[1] + end.compact.uniq + end + # All routes that start with a namespaced path, that have 1 or more # path-segments before having another wildcard parameter. # - Starting with paths: # - `/*namespace_id/:project_id/` # - `/*namespace_id/:id/` - # - Followed by one or more path-parts not starting with `:` or `/` + # - Followed by one or more path-parts not starting with `:` or `*` # - Followed by a path-part that includes a wildcard parameter `*` - # At the time of writing these routes match: http://rubular.com/r/QDxulzZlxZ + # At the time of writing these routes match: http://rubular.com/r/Rv2pDE5Dvw STARTING_WITH_NAMESPACE = /^\/\*namespace_id\/:(project_)?id/ NON_PARAM_PARTS = /[^:*][a-z\-_\/]*/ ANY_OTHER_PATH_PART = /[a-z\-_\/:]*/ @@ -60,82 +73,89 @@ describe DynamicPathValidator do end end + # This will return all paths that are used in a namespaced route + # before another wildcard path: + # + # /*namespace_id/:project_id/builds/artifacts/*ref_name_and_path + # /*namespace_id/:project_id/info/lfs/objects/*oid + # /*namespace_id/:project_id/commits/*id + # /*namespace_id/:project_id/builds/:build_id/artifacts/file/*path + # -> ['builds/artifacts', 'info/lfs/objects', 'commits', 'artifacts/file'] + let(:all_wildcard_paths) do + namespaced_wildcard_routes.map do |route| + path_between_wildcards(route) + end.uniq + end + describe 'TOP_LEVEL_ROUTES' do it 'includes all the top level namespaces' do - top_level_words = routes_not_starting_in_wildcard. - map { |p| p.split('/')[1] }. # Get the first part of the path - compact. - uniq - expect(described_class::TOP_LEVEL_ROUTES).to include(*top_level_words) end end describe 'WILDCARD_ROUTES' do it 'includes all paths that can be used after a namespace/project path' do - all_wildcard_paths = namespaced_wildcard_routes.map do |path| - segment_before_last_wildcard(path) - end.uniq + aggregate_failures do + all_wildcard_paths.each do |path| + expect(wildcards_include?(path)).to be(true), "Expected #{path} to be rejected" + end + end + end + end - expect(described_class::WILDCARD_ROUTES).to include(*all_wildcard_paths) + describe '.contains_path_part?' do + it 'recognizes a path part' do + expect(described_class.contains_path_part?('user/some/path', 'user')). + to be_truthy + end + + it 'recognizes a path ending in the path part' do + expect(described_class.contains_path_part?('some/path/user', 'user')). + to be_truthy + end + + it 'skips partial path matchies' do + expect(described_class.contains_path_part?('some/user1/path', 'user')). + to be_falsy + end + + it 'can recognise combined paths' do + expect(described_class.contains_path_part?('some/user/admin/path', 'user/admin')). + to be_truthy + end + + it 'only recognizes full paths' do + expect(described_class.contains_path_part?('frontend-fixtures', 's')). + to be_falsy + end + + it 'handles regex chars gracefully' do + expect(described_class.contains_path_part?('frontend-fixtures', '-')). + to be_falsy end end describe ".valid?" do it 'is not case sensitive' do - expect(described_class.valid?("Users", type: :top_level)).to be(false) + expect(described_class.valid?("Users")).to be(false) end - end - describe '.valid_full_path' do it "isn't valid when the top level is reserved" do test_path = 'u/should-be-a/reserved-word' - expect(described_class.valid_full_path?(test_path)).to be(false) + expect(described_class.valid?(test_path)).to be(false) end it "isn't valid if any of the path segments is reserved" do - test_path = 'the-wildcard/wikis/is-a-reserved-path' + test_path = 'the-wildcard/wikis/is-not-allowed' - expect(described_class.valid_full_path?(test_path)).to be(false) + expect(described_class.valid?(test_path)).to be(false) end - it "is valid if the path doen't contain reserved words" do + it "is valid if the path doesn't contain reserved words" do test_path = 'there-are/no-wildcards/in-this-path' - expect(described_class.valid_full_path?(test_path)).to be(true) - end - end - - describe '#validation_type' do - it 'uses top level validation for groups without parent' do - group = build(:group) - - type = validator.validation_type(group) - - expect(type).to eq(:top_level) - end - - it 'uses wildcard validation for groups with a parent' do - group = build(:group, parent: create(:group)) - - type = validator.validation_type(group) - - expect(type).to eq(:wildcard) - end - - it 'uses wildcard validation for a project' do - project = build(:project) - - type = validator.validation_type(project) - - expect(type).to eq(:wildcard) - end - - it 'uses strict validation for everything else' do - type = validator.validation_type(double) - - expect(type).to eq(:strict) + expect(described_class.valid?(test_path)).to be(true) end end end From 389057f00184a3549a1874174cbb81c807abfd49 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 18 Apr 2017 17:16:48 +0200 Subject: [PATCH 21/38] Rename Projects & Namespaces based on entire paths --- .../20170412174900_rename_reserved_dynamic_paths.rb | 3 ++- .../rename_reserved_paths_migration/rename_base.rb | 4 ++++ .../rename_namespaces.rb | 10 +++++----- .../rename_projects.rb | 7 ++++--- .../rename_namespaces_spec.rb | 13 +++++++++++++ .../rename_projects_spec.rb | 10 ++++++++++ 6 files changed, 38 insertions(+), 9 deletions(-) diff --git a/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb index 73a59ef0d74..88eca39c716 100644 --- a/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb +++ b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb @@ -27,7 +27,8 @@ class RenameReservedDynamicPaths < ActiveRecord::Migration users ] - DISALLOWED_WILDCARD_PATHS = %w[objects folders file] + DISALLOWED_WILDCARD_PATHS = %w[info/lfs/objects gitlab-lfs/objects + environments/folders] def up rename_root_paths(DISALLOWED_ROOT_PATHS) diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb index 367348a9a42..4d454fd8ea0 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb @@ -13,6 +13,10 @@ module Gitlab @migration = migration end + def path_patterns + @path_patterns ||= paths.map { |path| "%#{path}" } + end + def rename_path_for_routable(routable) old_path = routable.path old_full_path = routable.full_path diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb index 80e8135ea93..847b6e56955 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb @@ -16,9 +16,9 @@ module Gitlab elsif type == :top_level MigrationClasses::Namespace.where(parent_id: nil) end - with_paths = MigrationClasses::Namespace.arel_table[:path]. - matches_any(paths) - namespaces.where(with_paths) + with_paths = MigrationClasses::Route.arel_table[:path]. + matches_any(path_patterns) + namespaces.joins(:route).where(with_paths) end def rename_namespace(namespace) @@ -43,8 +43,8 @@ module Gitlab end def repo_paths_for_namespace(namespace) - projects_for_namespace(namespace). - select('distinct(repository_storage)').map(&:repository_storage_path) + projects_for_namespace(namespace).distinct.select(:repository_storage). + map(&:repository_storage_path) end def projects_for_namespace(namespace) diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb index 02f10d8e951..49b9453b134 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb @@ -28,9 +28,10 @@ module Gitlab end def projects_for_paths - with_paths = MigrationClasses::Project.arel_table[:path] - .matches_any(paths) - MigrationClasses::Project.where(with_paths) + with_paths = MigrationClasses::Route.arel_table[:path] + .matches_any(path_patterns) + + MigrationClasses::Project.joins(:route).where(with_paths) end end end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb index ee481e4610c..00d6cf0105c 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb @@ -14,6 +14,19 @@ describe Gitlab::Database::RenameReservedPathsMigration::RenameNamespaces do end describe '#namespaces_for_paths' do + context 'nested namespaces' do + let(:subject) { described_class.new(['parent/the-Path'], migration) } + + it 'includes the namespace' do + parent = create(:namespace, path: 'parent') + child = create(:namespace, path: 'the-path', parent: parent) + + found_ids = subject.namespaces_for_paths(type: :wildcard). + map(&:id) + expect(found_ids).to contain_exactly(child.id) + end + end + context 'for wildcard namespaces' do it 'only returns child namespaces with the correct path' do _root_namespace = create(:namespace, path: 'THE-path') diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb index 4a572133b69..173ebecb676 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb @@ -9,6 +9,16 @@ describe Gitlab::Database::RenameReservedPathsMigration::RenameProjects do end describe '#projects_for_paths' do + it 'searches using nested paths' do + namespace = create(:namespace, path: 'hello') + project = create(:empty_project, path: 'THE-path', namespace: namespace) + + result_ids = described_class.new(['Hello/the-path'], migration). + projects_for_paths.map(&:id) + + expect(result_ids).to contain_exactly(project.id) + end + it 'includes the correct projects' do project = create(:empty_project, path: 'THE-path') _other_project = create(:empty_project) From 39efd0c03018089366d5301583a605626e97e9d7 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 19 Apr 2017 10:12:09 +0200 Subject: [PATCH 22/38] Clear cached markdown after renaming projects --- .../rename_base.rb | 18 +++++++++ .../rename_namespaces.rb | 3 +- .../rename_projects.rb | 6 ++- .../rename_base_spec.rb | 40 +++++++++++++++++++ .../rename_namespaces_spec.rb | 8 +++- .../rename_projects_spec.rb | 19 ++++++++- 6 files changed, 88 insertions(+), 6 deletions(-) diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb index 4d454fd8ea0..fa0c93084e3 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb @@ -90,6 +90,24 @@ module Gitlab FileUtils.mv(old_path, new_path) end + def remove_cached_html_for_projects(project_ids) + update_column_in_batches(:projects, :description_html, nil) do |table, query| + query.where(table[:id].in(project_ids)) + end + + update_column_in_batches(:issues, :description_html, nil) do |table, query| + query.where(table[:project_id].in(project_ids)) + end + + update_column_in_batches(:merge_requests, :description_html, nil) do |table, query| + query.where(table[:target_project_id].in(project_ids)) + end + + update_column_in_batches(:notes, :note_html, nil) do |table, query| + query.where(table[:project_id].in(project_ids)) + end + end + def file_storage? CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb index 847b6e56955..4937dfe2d31 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb @@ -27,6 +27,7 @@ module Gitlab move_repositories(namespace, old_full_path, new_full_path) move_uploads(old_full_path, new_full_path) move_pages(old_full_path, new_full_path) + remove_cached_html_for_projects(projects_for_namespace(namespace).map(&:id)) end def move_repositories(namespace, old_full_path, new_full_path) @@ -55,8 +56,6 @@ module Gitlab MigrationClasses::Project.where(namespace_or_children) end - # This won't scale to huge trees, but it should do for a handful of - # namespaces called `system`. def child_ids_for_parent(namespace, ids: []) namespace.children.each do |child| ids << child.id diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb index 49b9453b134..4f35c3371d8 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb @@ -8,6 +8,8 @@ module Gitlab projects_for_paths.each do |project| rename_project(project) end + + remove_cached_html_for_projects(projects_for_paths.map(&:id)) end def rename_project(project) @@ -28,10 +30,12 @@ module Gitlab end def projects_for_paths + return @projects_for_paths if @projects_for_paths + with_paths = MigrationClasses::Route.arel_table[:path] .matches_any(path_patterns) - MigrationClasses::Project.joins(:route).where(with_paths) + @projects_for_paths = MigrationClasses::Project.joins(:route).where(with_paths) end end end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb index 48234170d31..8aad88edb2b 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb @@ -27,6 +27,46 @@ describe Gitlab::Database::RenameReservedPathsMigration::RenameBase do end end + describe '#remove_cached_html_for_projects' do + let(:project) { create(:empty_project, description_html: 'Project description') } + + it 'removes description_html from projects' do + subject.remove_cached_html_for_projects([project.id]) + + expect(project.reload.description_html).to be_nil + end + + it 'removes issue descriptions' do + issue = create(:issue, project: project, description_html: 'Issue description') + + subject.remove_cached_html_for_projects([project.id]) + + expect(issue.reload.description_html).to be_nil + end + + it 'removes merge request descriptions' do + merge_request = create(:merge_request, + source_project: project, + target_project: project, + description_html: 'MergeRequest description') + + subject.remove_cached_html_for_projects([project.id]) + + expect(merge_request.reload.description_html).to be_nil + end + + it 'removes note html' do + note = create(:note, + project: project, + noteable: create(:issue, project: project), + note_html: 'note description') + + subject.remove_cached_html_for_projects([project.id]) + + expect(note.reload.note_html).to be_nil + end + end + describe '#rename_path_for_routable' do context 'for namespaces' do let(:namespace) { create(:namespace, path: 'the-path') } diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb index 00d6cf0105c..8bb272e7595 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb @@ -139,7 +139,13 @@ describe Gitlab::Database::RenameReservedPathsMigration::RenameNamespaces do subject.rename_namespace(namespace) end - it 'invalidates the markdown cache of related projects' + it 'invalidates the markdown cache of related projects' do + project = create(:empty_project, namespace: namespace, path: "the-path-project") + + expect(subject).to receive(:remove_cached_html_for_projects).with([project.id]) + + subject.rename_namespace(namespace) + end end describe '#rename_namespaces' do diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb index 173ebecb676..f4be5494c4a 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb @@ -29,6 +29,23 @@ describe Gitlab::Database::RenameReservedPathsMigration::RenameProjects do end end + describe '#rename_projects' do + let!(:projects) { create_list(:empty_project, 2, path: 'the-path') } + + it 'renames each project' do + expect(subject).to receive(:rename_project).twice + + subject.rename_projects + end + + it 'invalidates the markdown cache of related projects' do + expect(subject).to receive(:remove_cached_html_for_projects). + with(projects.map(&:id)) + + subject.rename_projects + end + end + describe '#rename_project' do let(:project) do create(:empty_project, @@ -68,8 +85,6 @@ describe Gitlab::Database::RenameReservedPathsMigration::RenameProjects do subject.rename_project(project) end - - it 'invalidates the markdown cache of related projects' end describe '#move_repository' do From ea8e86dac856fdf6545e020df346533231379b93 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 24 Apr 2017 12:22:51 +0200 Subject: [PATCH 23/38] Use `%r{}` regexes to avoid having to escape `/` --- app/validators/dynamic_path_validator.rb | 2 +- spec/validators/dynamic_path_validator_spec.rb | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index 6f04263d4d5..ae2bd5984c5 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -104,7 +104,7 @@ class DynamicPathValidator < ActiveModel::EachValidator end def self.contains_path_part?(path, part) - path =~ /(.*\/|\A)#{Regexp.quote(part)}(\/.*|\z)/ + path =~ %r{(/|\A)#{Regexp.quote(part)}(/|\z)} end def self.follow_format?(value) diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb index 71098b17dfd..960a6204c90 100644 --- a/spec/validators/dynamic_path_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -63,13 +63,13 @@ describe DynamicPathValidator do # - Followed by one or more path-parts not starting with `:` or `*` # - Followed by a path-part that includes a wildcard parameter `*` # At the time of writing these routes match: http://rubular.com/r/Rv2pDE5Dvw - STARTING_WITH_NAMESPACE = /^\/\*namespace_id\/:(project_)?id/ - NON_PARAM_PARTS = /[^:*][a-z\-_\/]*/ - ANY_OTHER_PATH_PART = /[a-z\-_\/:]*/ - WILDCARD_SEGMENT = /\*/ + STARTING_WITH_NAMESPACE = %r{^/\*namespace_id/:(project_)?id} + NON_PARAM_PARTS = %r{[^:*][a-z\-_/]*} + ANY_OTHER_PATH_PART = %r{[a-z\-_/:]*} + WILDCARD_SEGMENT = %r{\*} let(:namespaced_wildcard_routes) do routes_without_format.select do |p| - p =~ %r{#{STARTING_WITH_NAMESPACE}\/#{NON_PARAM_PARTS}\/#{ANY_OTHER_PATH_PART}#{WILDCARD_SEGMENT}} + p =~ %r{#{STARTING_WITH_NAMESPACE}/#{NON_PARAM_PARTS}/#{ANY_OTHER_PATH_PART}#{WILDCARD_SEGMENT}} end end From 12735eefcd7876435cd05f35a9f26bfe2836e09f Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 24 Apr 2017 12:38:09 +0200 Subject: [PATCH 24/38] Minor style adjustments --- app/validators/dynamic_path_validator.rb | 60 ++++++++++++------- lib/constraints/project_url_constrainer.rb | 4 +- .../rename_namespaces.rb | 7 ++- lib/gitlab/etag_caching/router.rb | 2 +- 4 files changed, 45 insertions(+), 28 deletions(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index ae2bd5984c5..5d2d3228baa 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -16,20 +16,33 @@ class DynamicPathValidator < ActiveModel::EachValidator # the path `api` shouldn't be allowed because it would be masked by `api/*` # TOP_LEVEL_ROUTES = Set.new(%w[ + - .well-known + abuse_reports admin all + api assets + autocomplete ci dashboard + explore files groups + health_check help hooks + import + invites issues + jwt + koding + member merge_requests new notes + notification_settings + oauth profile projects public @@ -37,29 +50,14 @@ class DynamicPathValidator < ActiveModel::EachValidator robots.txt s search + sent_notifications services snippets teams u unsubscribes - users - api - autocomplete - search - member - explore uploads - import - notification_settings - abuse_reports - invites - help - koding - health_check - jwt - oauth - sent_notifications - - + users ]).freeze # All project routes with wildcard argument must be listed here. @@ -71,10 +69,30 @@ class DynamicPathValidator < ActiveModel::EachValidator # without tree as reserved name routing can match 'group/project' as group name, # 'tree' as project name and 'deploy_keys' as route. # - WILDCARD_ROUTES = Set.new(%w[tree commits wikis new edit create update logs_tree - preview blob blame raw files create_dir find_file - artifacts graphs refs badges info/lfs/objects - gitlab-lfs/objects environments/folders]) + WILDCARD_ROUTES = Set.new(%w[ + artifacts + badges + blame + blob + commits + create + create_dir + edit + environments/folders + files + find_file + gitlab-lfs/objects + graphs + info/lfs/objects + logs_tree + new + preview + raw + refs + tree + update + wikis + ]).freeze STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES).freeze diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb index 064e1bd78e3..d0ce2caffff 100644 --- a/lib/constraints/project_url_constrainer.rb +++ b/lib/constraints/project_url_constrainer.rb @@ -4,9 +4,7 @@ class ProjectUrlConstrainer project_path = request.params[:project_id] || request.params[:id] full_path = namespace_path + '/' + project_path - unless DynamicPathValidator.valid?(full_path) - return false - end + return false unless DynamicPathValidator.valid?(full_path) Project.find_by_full_path(full_path).present? end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb index 4937dfe2d31..4143e0edc62 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb @@ -11,13 +11,14 @@ module Gitlab end def namespaces_for_paths(type:) - namespaces = if type == :wildcard + namespaces = case type + when :wildcard MigrationClasses::Namespace.where.not(parent_id: nil) - elsif type == :top_level + when :top_level MigrationClasses::Namespace.where(parent_id: nil) end with_paths = MigrationClasses::Route.arel_table[:path]. - matches_any(path_patterns) + matches_any(path_patterns) namespaces.joins(:route).where(with_paths) end diff --git a/lib/gitlab/etag_caching/router.rb b/lib/gitlab/etag_caching/router.rb index c7da8b07a56..aac210f19e8 100644 --- a/lib/gitlab/etag_caching/router.rb +++ b/lib/gitlab/etag_caching/router.rb @@ -8,7 +8,7 @@ module Gitlab # regex itself) # - Ending in `noteable/issue//notes` for the `issue_notes` route # - Ending in `issues/id`/rendered_title` for the `issue_title` route - USED_IN_ROUTES = %w[noteable issue notes issues renderred_title + USED_IN_ROUTES = %w[noteable issue notes issues rendered_title commit pipelines merge_requests new].freeze RESERVED_WORDS = DynamicPathValidator::WILDCARD_ROUTES - USED_IN_ROUTES RESERVED_WORDS_REGEX = Regexp.union(*RESERVED_WORDS) From 99a03fd6e912f594f96176f581de47e1731d3459 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 25 Apr 2017 14:10:04 +0200 Subject: [PATCH 25/38] Move ReservedPathsMigration into V1 namespace --- ...412174900_rename_reserved_dynamic_paths.rb | 25 ++-- .../rename_reserved_paths_migration.rb | 20 --- .../migration_classes.rb | 94 ------------- .../rename_base.rb | 125 ----------------- .../rename_namespaces.rb | 70 ---------- .../rename_projects.rb | 43 ------ .../rename_reserved_paths_migration/v1.rb | 22 +++ .../v1/migration_classes.rb | 96 +++++++++++++ .../v1/rename_base.rb | 127 ++++++++++++++++++ .../v1/rename_namespaces.rb | 72 ++++++++++ .../v1/rename_projects.rb | 45 +++++++ .../{ => v1}/rename_base_spec.rb | 8 +- .../{ => v1}/rename_namespaces_spec.rb | 6 +- .../{ => v1}/rename_projects_spec.rb | 4 +- .../rename_reserved_paths_migration_spec.rb | 10 +- spec/support/fake_migration_classes.rb | 4 +- 16 files changed, 392 insertions(+), 379 deletions(-) delete mode 100644 lib/gitlab/database/rename_reserved_paths_migration.rb delete mode 100644 lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb delete mode 100644 lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb delete mode 100644 lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb delete mode 100644 lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb create mode 100644 lib/gitlab/database/rename_reserved_paths_migration/v1.rb create mode 100644 lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb create mode 100644 lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base.rb create mode 100644 lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces.rb create mode 100644 lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects.rb rename spec/lib/gitlab/database/rename_reserved_paths_migration/{ => v1}/rename_base_spec.rb (95%) rename spec/lib/gitlab/database/rename_reserved_paths_migration/{ => v1}/rename_namespaces_spec.rb (96%) rename spec/lib/gitlab/database/rename_reserved_paths_migration/{ => v1}/rename_projects_spec.rb (95%) diff --git a/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb index 88eca39c716..0fe04a23959 100644 --- a/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb +++ b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb @@ -2,33 +2,36 @@ # for more information on how to write migrations for GitLab. class RenameReservedDynamicPaths < ActiveRecord::Migration - include Gitlab::Database::RenameReservedPathsMigration + include Gitlab::Database::RenameReservedPathsMigration::V1 DOWNTIME = false disable_ddl_transaction! DISALLOWED_ROOT_PATHS = %w[ + - + abuse_reports api autocomplete - member explore - uploads - import - notification_settings - abuse_reports - invites - koding health_check + import + invites jwt + koding + member + notification_settings oauth sent_notifications - - + uploads users ] - DISALLOWED_WILDCARD_PATHS = %w[info/lfs/objects gitlab-lfs/objects - environments/folders] + DISALLOWED_WILDCARD_PATHS = %w[ + environments/folders + gitlab-lfs/objects + info/lfs/objects + ] def up rename_root_paths(DISALLOWED_ROOT_PATHS) diff --git a/lib/gitlab/database/rename_reserved_paths_migration.rb b/lib/gitlab/database/rename_reserved_paths_migration.rb deleted file mode 100644 index 0507ae4da51..00000000000 --- a/lib/gitlab/database/rename_reserved_paths_migration.rb +++ /dev/null @@ -1,20 +0,0 @@ -module Gitlab - module Database - module RenameReservedPathsMigration - def self.included(kls) - kls.include(MigrationHelpers) - end - - def rename_wildcard_paths(one_or_more_paths) - paths = Array(one_or_more_paths) - RenameNamespaces.new(paths, self).rename_namespaces(type: :wildcard) - RenameProjects.new(paths, self).rename_projects - end - - def rename_root_paths(paths) - paths = Array(paths) - RenameNamespaces.new(paths, self).rename_namespaces(type: :top_level) - end - end - end -end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb b/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb deleted file mode 100644 index 9133b97d239..00000000000 --- a/lib/gitlab/database/rename_reserved_paths_migration/migration_classes.rb +++ /dev/null @@ -1,94 +0,0 @@ -module Gitlab - module Database - module RenameReservedPathsMigration - module MigrationClasses - module Routable - def full_path - if route && route.path.present? - @full_path ||= route.path - else - update_route if persisted? - - build_full_path - end - end - - def build_full_path - if parent && path - parent.full_path + '/' + path - else - path - end - end - - def update_route - prepare_route - route.save - end - - def prepare_route - route || build_route(source: self) - route.path = build_full_path - route.name = build_full_name - @full_path = nil - @full_name = nil - end - - def build_full_name - if parent && name - parent.human_name + ' / ' + name - else - name - end - end - - def human_name - owner&.name - end - end - - class User < ActiveRecord::Base - self.table_name = 'users' - end - - class Namespace < ActiveRecord::Base - include MigrationClasses::Routable - self.table_name = 'namespaces' - belongs_to :parent, - class_name: "#{MigrationClasses.name}::Namespace" - has_one :route, as: :source - has_many :children, - class_name: "#{MigrationClasses.name}::Namespace", - foreign_key: :parent_id - belongs_to :owner, - class_name: "#{MigrationClasses.name}::User" - - # Overridden to have the correct `source_type` for the `route` relation - def self.name - 'Namespace' - end - end - - class Route < ActiveRecord::Base - self.table_name = 'routes' - belongs_to :source, polymorphic: true - end - - class Project < ActiveRecord::Base - include MigrationClasses::Routable - has_one :route, as: :source - self.table_name = 'projects' - - def repository_storage_path - Gitlab.config.repositories.storages[repository_storage]['path'] - end - - # Overridden to have the correct `source_type` for the `route` relation - def self.name - 'Project' - end - end - end - end - end -end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb deleted file mode 100644 index fa0c93084e3..00000000000 --- a/lib/gitlab/database/rename_reserved_paths_migration/rename_base.rb +++ /dev/null @@ -1,125 +0,0 @@ -module Gitlab - module Database - module RenameReservedPathsMigration - class RenameBase - attr_reader :paths, :migration - - delegate :update_column_in_batches, - :replace_sql, - to: :migration - - def initialize(paths, migration) - @paths = paths - @migration = migration - end - - def path_patterns - @path_patterns ||= paths.map { |path| "%#{path}" } - end - - def rename_path_for_routable(routable) - old_path = routable.path - old_full_path = routable.full_path - # Only remove the last occurrence of the path name to get the parent namespace path - namespace_path = remove_last_occurrence(old_full_path, old_path) - new_path = rename_path(namespace_path, old_path) - new_full_path = join_routable_path(namespace_path, new_path) - - # skips callbacks & validations - routable.class.where(id: routable). - update_all(path: new_path) - - rename_routes(old_full_path, new_full_path) - - [old_full_path, new_full_path] - end - - def rename_routes(old_full_path, new_full_path) - replace_statement = replace_sql(Route.arel_table[:path], - old_full_path, - new_full_path) - - update_column_in_batches(:routes, :path, replace_statement) do |table, query| - query.where(MigrationClasses::Route.arel_table[:path].matches("#{old_full_path}%")) - end - end - - def rename_path(namespace_path, path_was) - counter = 0 - path = "#{path_was}#{counter}" - - while route_exists?(join_routable_path(namespace_path, path)) - counter += 1 - path = "#{path_was}#{counter}" - end - - path - end - - def remove_last_occurrence(string, pattern) - string.reverse.sub(pattern.reverse, "").reverse - end - - def join_routable_path(namespace_path, top_level) - if namespace_path.present? - File.join(namespace_path, top_level) - else - top_level - end - end - - def route_exists?(full_path) - MigrationClasses::Route.where(Route.arel_table[:path].matches(full_path)).any? - end - - def move_pages(old_path, new_path) - move_folders(pages_dir, old_path, new_path) - end - - def move_uploads(old_path, new_path) - return unless file_storage? - - move_folders(uploads_dir, old_path, new_path) - end - - def move_folders(directory, old_relative_path, new_relative_path) - old_path = File.join(directory, old_relative_path) - return unless File.directory?(old_path) - - new_path = File.join(directory, new_relative_path) - FileUtils.mv(old_path, new_path) - end - - def remove_cached_html_for_projects(project_ids) - update_column_in_batches(:projects, :description_html, nil) do |table, query| - query.where(table[:id].in(project_ids)) - end - - update_column_in_batches(:issues, :description_html, nil) do |table, query| - query.where(table[:project_id].in(project_ids)) - end - - update_column_in_batches(:merge_requests, :description_html, nil) do |table, query| - query.where(table[:target_project_id].in(project_ids)) - end - - update_column_in_batches(:notes, :note_html, nil) do |table, query| - query.where(table[:project_id].in(project_ids)) - end - end - - def file_storage? - CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File - end - - def uploads_dir - File.join(CarrierWave.root, "uploads") - end - - def pages_dir - Settings.pages.path - end - end - end - end -end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb deleted file mode 100644 index 4143e0edc62..00000000000 --- a/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces.rb +++ /dev/null @@ -1,70 +0,0 @@ -module Gitlab - module Database - module RenameReservedPathsMigration - class RenameNamespaces < RenameBase - include Gitlab::ShellAdapter - - def rename_namespaces(type:) - namespaces_for_paths(type: type).each do |namespace| - rename_namespace(namespace) - end - end - - def namespaces_for_paths(type:) - namespaces = case type - when :wildcard - MigrationClasses::Namespace.where.not(parent_id: nil) - when :top_level - MigrationClasses::Namespace.where(parent_id: nil) - end - with_paths = MigrationClasses::Route.arel_table[:path]. - matches_any(path_patterns) - namespaces.joins(:route).where(with_paths) - end - - def rename_namespace(namespace) - old_full_path, new_full_path = rename_path_for_routable(namespace) - - move_repositories(namespace, old_full_path, new_full_path) - move_uploads(old_full_path, new_full_path) - move_pages(old_full_path, new_full_path) - remove_cached_html_for_projects(projects_for_namespace(namespace).map(&:id)) - end - - def move_repositories(namespace, old_full_path, new_full_path) - repo_paths_for_namespace(namespace).each do |repository_storage_path| - # Ensure old directory exists before moving it - gitlab_shell.add_namespace(repository_storage_path, old_full_path) - - unless gitlab_shell.mv_namespace(repository_storage_path, old_full_path, new_full_path) - message = "Exception moving path #{repository_storage_path} \ - from #{old_full_path} to #{new_full_path}" - Rails.logger.error message - end - end - end - - def repo_paths_for_namespace(namespace) - projects_for_namespace(namespace).distinct.select(:repository_storage). - map(&:repository_storage_path) - end - - def projects_for_namespace(namespace) - namespace_ids = child_ids_for_parent(namespace, ids: [namespace.id]) - namespace_or_children = MigrationClasses::Project. - arel_table[:namespace_id]. - in(namespace_ids) - MigrationClasses::Project.where(namespace_or_children) - end - - def child_ids_for_parent(namespace, ids: []) - namespace.children.each do |child| - ids << child.id - child_ids_for_parent(child, ids: ids) if child.children.any? - end - ids - end - end - end - end -end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb b/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb deleted file mode 100644 index 4f35c3371d8..00000000000 --- a/lib/gitlab/database/rename_reserved_paths_migration/rename_projects.rb +++ /dev/null @@ -1,43 +0,0 @@ -module Gitlab - module Database - module RenameReservedPathsMigration - class RenameProjects < RenameBase - include Gitlab::ShellAdapter - - def rename_projects - projects_for_paths.each do |project| - rename_project(project) - end - - remove_cached_html_for_projects(projects_for_paths.map(&:id)) - end - - def rename_project(project) - old_full_path, new_full_path = rename_path_for_routable(project) - - move_repository(project, old_full_path, new_full_path) - move_repository(project, "#{old_full_path}.wiki", "#{new_full_path}.wiki") - move_uploads(old_full_path, new_full_path) - move_pages(old_full_path, new_full_path) - end - - def move_repository(project, old_path, new_path) - unless gitlab_shell.mv_repository(project.repository_storage_path, - old_path, - new_path) - Rails.logger.error "Error moving #{old_path} to #{new_path}" - end - end - - def projects_for_paths - return @projects_for_paths if @projects_for_paths - - with_paths = MigrationClasses::Route.arel_table[:path] - .matches_any(path_patterns) - - @projects_for_paths = MigrationClasses::Project.joins(:route).where(with_paths) - end - end - end - end -end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1.rb new file mode 100644 index 00000000000..6296e964187 --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1.rb @@ -0,0 +1,22 @@ +module Gitlab + module Database + module RenameReservedPathsMigration + module V1 + def self.included(kls) + kls.include(MigrationHelpers) + end + + def rename_wildcard_paths(one_or_more_paths) + paths = Array(one_or_more_paths) + RenameNamespaces.new(paths, self).rename_namespaces(type: :wildcard) + RenameProjects.new(paths, self).rename_projects + end + + def rename_root_paths(paths) + paths = Array(paths) + RenameNamespaces.new(paths, self).rename_namespaces(type: :top_level) + end + end + end + end +end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb new file mode 100644 index 00000000000..e620b62e2ac --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb @@ -0,0 +1,96 @@ +module Gitlab + module Database + module RenameReservedPathsMigration + module V1 + module MigrationClasses + module Routable + def full_path + if route && route.path.present? + @full_path ||= route.path + else + update_route if persisted? + + build_full_path + end + end + + def build_full_path + if parent && path + parent.full_path + '/' + path + else + path + end + end + + def update_route + prepare_route + route.save + end + + def prepare_route + route || build_route(source: self) + route.path = build_full_path + route.name = build_full_name + @full_path = nil + @full_name = nil + end + + def build_full_name + if parent && name + parent.human_name + ' / ' + name + else + name + end + end + + def human_name + owner&.name + end + end + + class User < ActiveRecord::Base + self.table_name = 'users' + end + + class Namespace < ActiveRecord::Base + include MigrationClasses::Routable + self.table_name = 'namespaces' + belongs_to :parent, + class_name: "#{MigrationClasses.name}::Namespace" + has_one :route, as: :source + has_many :children, + class_name: "#{MigrationClasses.name}::Namespace", + foreign_key: :parent_id + belongs_to :owner, + class_name: "#{MigrationClasses.name}::User" + + # Overridden to have the correct `source_type` for the `route` relation + def self.name + 'Namespace' + end + end + + class Route < ActiveRecord::Base + self.table_name = 'routes' + belongs_to :source, polymorphic: true + end + + class Project < ActiveRecord::Base + include MigrationClasses::Routable + has_one :route, as: :source + self.table_name = 'projects' + + def repository_storage_path + Gitlab.config.repositories.storages[repository_storage]['path'] + end + + # Overridden to have the correct `source_type` for the `route` relation + def self.name + 'Project' + end + end + end + end + end + end +end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base.rb new file mode 100644 index 00000000000..7838951c703 --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base.rb @@ -0,0 +1,127 @@ +module Gitlab + module Database + module RenameReservedPathsMigration + module V1 + class RenameBase + attr_reader :paths, :migration + + delegate :update_column_in_batches, + :replace_sql, + to: :migration + + def initialize(paths, migration) + @paths = paths + @migration = migration + end + + def path_patterns + @path_patterns ||= paths.map { |path| "%#{path}" } + end + + def rename_path_for_routable(routable) + old_path = routable.path + old_full_path = routable.full_path + # Only remove the last occurrence of the path name to get the parent namespace path + namespace_path = remove_last_occurrence(old_full_path, old_path) + new_path = rename_path(namespace_path, old_path) + new_full_path = join_routable_path(namespace_path, new_path) + + # skips callbacks & validations + routable.class.where(id: routable). + update_all(path: new_path) + + rename_routes(old_full_path, new_full_path) + + [old_full_path, new_full_path] + end + + def rename_routes(old_full_path, new_full_path) + replace_statement = replace_sql(Route.arel_table[:path], + old_full_path, + new_full_path) + + update_column_in_batches(:routes, :path, replace_statement) do |table, query| + query.where(MigrationClasses::Route.arel_table[:path].matches("#{old_full_path}%")) + end + end + + def rename_path(namespace_path, path_was) + counter = 0 + path = "#{path_was}#{counter}" + + while route_exists?(join_routable_path(namespace_path, path)) + counter += 1 + path = "#{path_was}#{counter}" + end + + path + end + + def remove_last_occurrence(string, pattern) + string.reverse.sub(pattern.reverse, "").reverse + end + + def join_routable_path(namespace_path, top_level) + if namespace_path.present? + File.join(namespace_path, top_level) + else + top_level + end + end + + def route_exists?(full_path) + MigrationClasses::Route.where(Route.arel_table[:path].matches(full_path)).any? + end + + def move_pages(old_path, new_path) + move_folders(pages_dir, old_path, new_path) + end + + def move_uploads(old_path, new_path) + return unless file_storage? + + move_folders(uploads_dir, old_path, new_path) + end + + def move_folders(directory, old_relative_path, new_relative_path) + old_path = File.join(directory, old_relative_path) + return unless File.directory?(old_path) + + new_path = File.join(directory, new_relative_path) + FileUtils.mv(old_path, new_path) + end + + def remove_cached_html_for_projects(project_ids) + update_column_in_batches(:projects, :description_html, nil) do |table, query| + query.where(table[:id].in(project_ids)) + end + + update_column_in_batches(:issues, :description_html, nil) do |table, query| + query.where(table[:project_id].in(project_ids)) + end + + update_column_in_batches(:merge_requests, :description_html, nil) do |table, query| + query.where(table[:target_project_id].in(project_ids)) + end + + update_column_in_batches(:notes, :note_html, nil) do |table, query| + query.where(table[:project_id].in(project_ids)) + end + end + + def file_storage? + CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File + end + + def uploads_dir + File.join(CarrierWave.root, "uploads") + end + + def pages_dir + Settings.pages.path + end + end + end + end + end +end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces.rb new file mode 100644 index 00000000000..d863de90844 --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces.rb @@ -0,0 +1,72 @@ +module Gitlab + module Database + module RenameReservedPathsMigration + module V1 + class RenameNamespaces < RenameBase + include Gitlab::ShellAdapter + + def rename_namespaces(type:) + namespaces_for_paths(type: type).each do |namespace| + rename_namespace(namespace) + end + end + + def namespaces_for_paths(type:) + namespaces = case type + when :wildcard + MigrationClasses::Namespace.where.not(parent_id: nil) + when :top_level + MigrationClasses::Namespace.where(parent_id: nil) + end + with_paths = MigrationClasses::Route.arel_table[:path]. + matches_any(path_patterns) + namespaces.joins(:route).where(with_paths) + end + + def rename_namespace(namespace) + old_full_path, new_full_path = rename_path_for_routable(namespace) + + move_repositories(namespace, old_full_path, new_full_path) + move_uploads(old_full_path, new_full_path) + move_pages(old_full_path, new_full_path) + remove_cached_html_for_projects(projects_for_namespace(namespace).map(&:id)) + end + + def move_repositories(namespace, old_full_path, new_full_path) + repo_paths_for_namespace(namespace).each do |repository_storage_path| + # Ensure old directory exists before moving it + gitlab_shell.add_namespace(repository_storage_path, old_full_path) + + unless gitlab_shell.mv_namespace(repository_storage_path, old_full_path, new_full_path) + message = "Exception moving path #{repository_storage_path} \ + from #{old_full_path} to #{new_full_path}" + Rails.logger.error message + end + end + end + + def repo_paths_for_namespace(namespace) + projects_for_namespace(namespace).distinct.select(:repository_storage). + map(&:repository_storage_path) + end + + def projects_for_namespace(namespace) + namespace_ids = child_ids_for_parent(namespace, ids: [namespace.id]) + namespace_or_children = MigrationClasses::Project. + arel_table[:namespace_id]. + in(namespace_ids) + MigrationClasses::Project.where(namespace_or_children) + end + + def child_ids_for_parent(namespace, ids: []) + namespace.children.each do |child| + ids << child.id + child_ids_for_parent(child, ids: ids) if child.children.any? + end + ids + end + end + end + end + end +end diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects.rb new file mode 100644 index 00000000000..448717eb744 --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects.rb @@ -0,0 +1,45 @@ +module Gitlab + module Database + module RenameReservedPathsMigration + module V1 + class RenameProjects < RenameBase + include Gitlab::ShellAdapter + + def rename_projects + projects_for_paths.each do |project| + rename_project(project) + end + + remove_cached_html_for_projects(projects_for_paths.map(&:id)) + end + + def rename_project(project) + old_full_path, new_full_path = rename_path_for_routable(project) + + move_repository(project, old_full_path, new_full_path) + move_repository(project, "#{old_full_path}.wiki", "#{new_full_path}.wiki") + move_uploads(old_full_path, new_full_path) + move_pages(old_full_path, new_full_path) + end + + def move_repository(project, old_path, new_path) + unless gitlab_shell.mv_repository(project.repository_storage_path, + old_path, + new_path) + Rails.logger.error "Error moving #{old_path} to #{new_path}" + end + end + + def projects_for_paths + return @projects_for_paths if @projects_for_paths + + with_paths = MigrationClasses::Route.arel_table[:path] + .matches_any(path_patterns) + + @projects_for_paths = MigrationClasses::Project.joins(:route).where(with_paths) + end + end + end + end + end +end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb similarity index 95% rename from spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb rename to spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb index 8aad88edb2b..7704f1dbead 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_base_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' -describe Gitlab::Database::RenameReservedPathsMigration::RenameBase do - let(:migration) { FakeRenameReservedPathMigration.new } +describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase do + let(:migration) { FakeRenameReservedPathMigrationV1.new } let(:subject) { described_class.new(['the-path'], migration) } before do @@ -9,12 +9,12 @@ describe Gitlab::Database::RenameReservedPathsMigration::RenameBase do end def migration_namespace(namespace) - Gitlab::Database::RenameReservedPathsMigration::MigrationClasses:: + Gitlab::Database::RenameReservedPathsMigration::V1::MigrationClasses:: Namespace.find(namespace.id) end def migration_project(project) - Gitlab::Database::RenameReservedPathsMigration::MigrationClasses:: + Gitlab::Database::RenameReservedPathsMigration::V1::MigrationClasses:: Project.find(project.id) end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb similarity index 96% rename from spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb rename to spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb index 8bb272e7595..6d58413edf2 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_namespaces_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' -describe Gitlab::Database::RenameReservedPathsMigration::RenameNamespaces do - let(:migration) { FakeRenameReservedPathMigration.new } +describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces do + let(:migration) { FakeRenameReservedPathMigrationV1.new } let(:subject) { described_class.new(['the-path'], migration) } before do @@ -9,7 +9,7 @@ describe Gitlab::Database::RenameReservedPathsMigration::RenameNamespaces do end def migration_namespace(namespace) - Gitlab::Database::RenameReservedPathsMigration::MigrationClasses:: + Gitlab::Database::RenameReservedPathsMigration::V1::MigrationClasses:: Namespace.find(namespace.id) end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb similarity index 95% rename from spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb rename to spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb index f4be5494c4a..59e8de2712d 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/rename_projects_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' -describe Gitlab::Database::RenameReservedPathsMigration::RenameProjects do - let(:migration) { FakeRenameReservedPathMigration.new } +describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects do + let(:migration) { FakeRenameReservedPathMigrationV1.new } let(:subject) { described_class.new(['the-path'], migration) } before do diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb index b46595ba628..743054e0efc 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' -describe Gitlab::Database::RenameReservedPathsMigration do - let(:subject) { FakeRenameReservedPathMigration.new } +describe Gitlab::Database::RenameReservedPathsMigration::V1 do + let(:subject) { FakeRenameReservedPathMigrationV1.new } before do allow(subject).to receive(:say) @@ -10,7 +10,7 @@ describe Gitlab::Database::RenameReservedPathsMigration do describe '#rename_wildcard_paths' do it 'should rename namespaces' do rename_namespaces = double - expect(Gitlab::Database::RenameReservedPathsMigration::RenameNamespaces). + expect(described_class::RenameNamespaces). to receive(:new).with(['first-path', 'second-path'], subject). and_return(rename_namespaces) expect(rename_namespaces).to receive(:rename_namespaces). @@ -21,7 +21,7 @@ describe Gitlab::Database::RenameReservedPathsMigration do it 'should rename projects' do rename_projects = double - expect(Gitlab::Database::RenameReservedPathsMigration::RenameProjects). + expect(described_class::RenameProjects). to receive(:new).with(['the-path'], subject). and_return(rename_projects) @@ -34,7 +34,7 @@ describe Gitlab::Database::RenameReservedPathsMigration do describe '#rename_root_paths' do it 'should rename namespaces' do rename_namespaces = double - expect(Gitlab::Database::RenameReservedPathsMigration::RenameNamespaces). + expect(described_class::RenameNamespaces). to receive(:new).with(['the-path'], subject). and_return(rename_namespaces) expect(rename_namespaces).to receive(:rename_namespaces). diff --git a/spec/support/fake_migration_classes.rb b/spec/support/fake_migration_classes.rb index 7ff36fe434c..3de0460c3ca 100644 --- a/spec/support/fake_migration_classes.rb +++ b/spec/support/fake_migration_classes.rb @@ -1,3 +1,3 @@ -class FakeRenameReservedPathMigration < ActiveRecord::Migration - include Gitlab::Database::RenameReservedPathsMigration +class FakeRenameReservedPathMigrationV1 < ActiveRecord::Migration + include Gitlab::Database::RenameReservedPathsMigration::V1 end From ce0102d8b752a57131692d5dec620b50b2e76658 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 25 Apr 2017 14:32:13 +0200 Subject: [PATCH 26/38] Remove dependecy on `User` --- .../v1/migration_classes.rb | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb index e620b62e2ac..4fdcb682c2f 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb @@ -30,26 +30,8 @@ module Gitlab def prepare_route route || build_route(source: self) route.path = build_full_path - route.name = build_full_name @full_path = nil - @full_name = nil end - - def build_full_name - if parent && name - parent.human_name + ' / ' + name - else - name - end - end - - def human_name - owner&.name - end - end - - class User < ActiveRecord::Base - self.table_name = 'users' end class Namespace < ActiveRecord::Base @@ -61,8 +43,6 @@ module Gitlab has_many :children, class_name: "#{MigrationClasses.name}::Namespace", foreign_key: :parent_id - belongs_to :owner, - class_name: "#{MigrationClasses.name}::User" # Overridden to have the correct `source_type` for the `route` relation def self.name From b85f2fa4c7dca83d9fabb702562c3b4244231741 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 25 Apr 2017 14:46:13 +0200 Subject: [PATCH 27/38] Clear html cache for a projects milestones --- .../rename_reserved_paths_migration/v1/rename_base.rb | 4 ++++ .../v1/rename_base_spec.rb | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base.rb index 7838951c703..de4e6e7c404 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base.rb @@ -107,6 +107,10 @@ module Gitlab update_column_in_batches(:notes, :note_html, nil) do |table, query| query.where(table[:project_id].in(project_ids)) end + + update_column_in_batches(:milestones, :description_html, nil) do |table, query| + query.where(table[:project_id].in(project_ids)) + end end def file_storage? diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb index 7704f1dbead..64bc5fc0429 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb @@ -65,6 +65,16 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase do expect(note.reload.note_html).to be_nil end + + it 'removes milestone description' do + milestone = create(:milestone, + project: project, + description_html: 'milestone description') + + subject.remove_cached_html_for_projects([project.id]) + + expect(milestone.reload.description_html).to be_nil + end end describe '#rename_path_for_routable' do From 2c7ca43bdd8d24153e50216308bc7d88e73209cf Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 26 Apr 2017 13:46:03 +0200 Subject: [PATCH 28/38] Allow `graphs` & `refs` project names --- app/validators/dynamic_path_validator.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index 5d2d3228baa..18680b8975f 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -82,13 +82,11 @@ class DynamicPathValidator < ActiveModel::EachValidator files find_file gitlab-lfs/objects - graphs info/lfs/objects logs_tree new preview raw - refs tree update wikis From 1e14c3c8525c4e9db6f83da6c037ed94205f65f0 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 28 Apr 2017 17:53:28 +0200 Subject: [PATCH 29/38] Reject paths following namespace for paths including 2 `*` Reject the part following `/*namespace_id/:project_id` for paths containing 2 wildcard parameters --- app/validators/dynamic_path_validator.rb | 4 +-- .../validators/dynamic_path_validator_spec.rb | 26 ++++++++++--------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index 18680b8975f..96da92b5d76 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -70,10 +70,10 @@ class DynamicPathValidator < ActiveModel::EachValidator # 'tree' as project name and 'deploy_keys' as route. # WILDCARD_ROUTES = Set.new(%w[ - artifacts badges blame blob + builds commits create create_dir @@ -83,10 +83,10 @@ class DynamicPathValidator < ActiveModel::EachValidator find_file gitlab-lfs/objects info/lfs/objects - logs_tree new preview raw + refs tree update wikis diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb index 960a6204c90..5053a299bc3 100644 --- a/spec/validators/dynamic_path_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -13,29 +13,28 @@ describe DynamicPathValidator do # That's not a parameter # `/*namespace_id/:project_id/builds/artifacts/*ref_name_and_path` # -> 'builds/artifacts' - def path_between_wildcards(path) + def path_before_wildcard(path) path = path.gsub(STARTING_WITH_NAMESPACE, "") path_segments = path.split('/').reject(&:empty?) - wildcard_index = path_segments.index { |segment| segment.starts_with?('*') } + wildcard_index = path_segments.index { |segment| parameter?(segment) } segments_before_wildcard = path_segments[0..wildcard_index - 1] - param_index = segments_before_wildcard.index { |segment| segment.starts_with?(':') } - if param_index - segments_before_wildcard = segments_before_wildcard[param_index + 1..-1] - end - segments_before_wildcard.join('/') end + def parameter?(segment) + segment =~ /[*:]/ + end + # If the path is reserved. Then no conflicting paths can# be created for any # route using this reserved word. # - # Both `builds/artifacts` & `artifacts/file` are covered by reserving the word - # `artifacts` + # Both `builds/artifacts` & `build` are covered by reserving the word + # `build` def wildcards_include?(path) described_class::WILDCARD_ROUTES.include?(path) || - path.split('/').any? { |segment| described_class::WILDCARD_ROUTES.include?(segment) } + described_class::WILDCARD_ROUTES.include?(path.split('/').first) end let(:all_routes) do @@ -83,7 +82,10 @@ describe DynamicPathValidator do # -> ['builds/artifacts', 'info/lfs/objects', 'commits', 'artifacts/file'] let(:all_wildcard_paths) do namespaced_wildcard_routes.map do |route| - path_between_wildcards(route) + path_before_wildcard(route) + end.uniq + end + end.uniq end @@ -114,7 +116,7 @@ describe DynamicPathValidator do to be_truthy end - it 'skips partial path matchies' do + it 'skips partial path matches' do expect(described_class.contains_path_part?('some/user1/path', 'user')). to be_falsy end From 08b1bc3489e8d4e6d5786221bad090f16a1c021f Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 28 Apr 2017 18:09:01 +0200 Subject: [PATCH 30/38] Reject group-routes as names of child namespaces --- app/validators/dynamic_path_validator.rb | 89 +++++++++++++++---- spec/models/group_spec.rb | 6 ++ spec/models/project_spec.rb | 7 ++ spec/models/user_spec.rb | 12 +++ .../validators/dynamic_path_validator_spec.rb | 78 ++++++++++------ 5 files changed, 148 insertions(+), 44 deletions(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index 96da92b5d76..c4ed5ac85eb 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -55,6 +55,7 @@ class DynamicPathValidator < ActiveModel::EachValidator snippets teams u + unicorn_test unsubscribes uploads users @@ -92,7 +93,52 @@ class DynamicPathValidator < ActiveModel::EachValidator wikis ]).freeze - STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES).freeze + # These are all the paths that follow `/groups/*id/ or `/groups/*group_id` + # We need to reject these because we have a `/groups/*id` page that is the same + # as the `/*id`. + # + # If we would allow a subgroup to be created with the name `activity` then + # this group would not be accessible through `/groups/parent/activity` since + # this would map to the activity-page of it's parent. + GROUP_ROUTES = Set.new(%w[ + activity + avatar + edit + group_members + issues + labels + merge_requests + milestones + projects + subgroups + ]) + + CHILD_ROUTES = (WILDCARD_ROUTES | GROUP_ROUTES).freeze + + def self.without_reserved_wildcard_paths_regex + @full_path_without_wildcard_regex ||= regex_excluding_child_paths(WILDCARD_ROUTES) + end + + def self.without_reserved_child_paths_regex + @full_path_without_child_routes_regex ||= regex_excluding_child_paths(CHILD_ROUTES) + end + + # This is used to validate a full path. + # It doesn't match paths + # - Starting with one of the top level words + # - Containing one of the child level words in the middle of a path + def self.regex_excluding_child_paths(child_routes) + reserved_top_level_words = Regexp.union(TOP_LEVEL_ROUTES.to_a) + not_starting_in_reserved_word = %r{^(/?)(?!(#{reserved_top_level_words})(/|$))} + + reserved_child_level_words = Regexp.union(child_routes.to_a) + not_containing_reserved_child = %r{(?!(\S+)/(#{reserved_child_level_words})(/|$))} + + @full_path_regex = %r{ + #{not_starting_in_reserved_word} + #{not_containing_reserved_child} + #{Gitlab::Regex::FULL_NAMESPACE_REGEX_STR}}x + end def self.valid?(path) path_segments = path.split('/') @@ -102,41 +148,48 @@ class DynamicPathValidator < ActiveModel::EachValidator def self.reserved?(path) path = path.to_s.downcase - top_level, wildcard_part = path.split('/', 2) + _project_parts, namespace_parts = path.reverse.split('/', 2).map(&:reverse) - includes_reserved_top_level?(top_level) || includes_reserved_wildcard?(wildcard_part) + wildcard_reserved?(path) || any_reserved?(namespace_parts) end - def self.includes_reserved_wildcard?(path) - WILDCARD_ROUTES.any? do |reserved_word| - contains_path_part?(path, reserved_word) - end + def self.any_reserved?(path) + return false unless path + + path !~ without_reserved_child_paths_regex end - def self.includes_reserved_top_level?(path) - TOP_LEVEL_ROUTES.any? do |reserved_route| - contains_path_part?(path, reserved_route) - end - end + def self.wildcard_reserved?(path) + return false unless path - def self.contains_path_part?(path, part) - path =~ %r{(/|\A)#{Regexp.quote(part)}(/|\z)} + path !~ without_reserved_wildcard_paths_regex end def self.follow_format?(value) value =~ Gitlab::Regex.namespace_regex end - delegate :reserved?, :follow_format?, to: :class + delegate :reserved?, + :any_reserved?, + :follow_format?, to: :class + + def valid_full_path?(record, value) + full_path = record.respond_to?(:full_path) ? record.full_path : value + + case record + when Project || User + reserved?(full_path) + else + any_reserved?(full_path) + end + end def validate_each(record, attribute, value) unless follow_format?(value) record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) end - full_path = record.respond_to?(:full_path) ? record.full_path : value - - if reserved?(full_path) + if valid_full_path?(record, value) record.errors.add(attribute, "#{value} is a reserved name") end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 7b076d09585..a11805926cc 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -76,6 +76,12 @@ describe Group, models: true do expect(group).not_to be_valid end + + it 'rejects reserved group paths' do + group = build(:group, path: 'activity', parent: create(:group)) + + expect(group).not_to be_valid + end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 04c90b651a0..2168099ee82 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -273,6 +273,13 @@ describe Project, models: true do expect(project).not_to be_valid end + + it 'allows a reserved group name' do + parent = create(:group) + project = build(:project, path: 'avatar', namespace: parent) + + expect(project).to be_valid + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0bcebc27598..1c2df4c9d97 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -97,6 +97,18 @@ describe User, models: true do expect(user.errors.values).to eq [['dashboard is a reserved name']] end + it 'allows child names' do + user = build(:user, username: 'avatar') + + expect(user).to be_valid + end + + it 'allows wildcard names' do + user = build(:user, username: 'blob') + + expect(user).to be_valid + end + it 'validates uniqueness' do expect(subject).to validate_uniqueness_of(:username).case_insensitive end diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb index 5053a299bc3..f43b4892456 100644 --- a/spec/validators/dynamic_path_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -86,6 +86,16 @@ describe DynamicPathValidator do end.uniq end + STARTING_WITH_GROUP = %r{^/groups/\*(group_)?id/} + let(:group_routes) do + routes_without_format.select do |path| + path =~ STARTING_WITH_GROUP + end + end + + let(:paths_after_group_id) do + group_routes.map do |route| + route.gsub(STARTING_WITH_GROUP, '').split('/').first end.uniq end @@ -95,6 +105,12 @@ describe DynamicPathValidator do end end + describe 'GROUP_ROUTES' do + it "don't contain a second wildcard" do + expect(described_class::GROUP_ROUTES).to include(*paths_after_group_id) + end + end + describe 'WILDCARD_ROUTES' do it 'includes all paths that can be used after a namespace/project path' do aggregate_failures do @@ -105,59 +121,69 @@ describe DynamicPathValidator do end end - describe '.contains_path_part?' do - it 'recognizes a path part' do - expect(described_class.contains_path_part?('user/some/path', 'user')). - to be_truthy + describe '.without_reserved_wildcard_paths_regex' do + subject { described_class.without_reserved_wildcard_paths_regex } + + it 'rejects paths starting with a reserved top level' do + expect(subject).not_to match('dashboard/hello/world') + expect(subject).not_to match('dashboard') end - it 'recognizes a path ending in the path part' do - expect(described_class.contains_path_part?('some/path/user', 'user')). - to be_truthy + it 'rejects paths containing a wildcard reserved word' do + expect(subject).not_to match('hello/edit') + expect(subject).not_to match('hello/edit/in-the-middle') + expect(subject).not_to match('foo/bar1/refs/master/logs_tree') end - it 'skips partial path matches' do - expect(described_class.contains_path_part?('some/user1/path', 'user')). - to be_falsy + it 'matches valid paths' do + expect(subject).to match('parent/child/project-path') + expect(subject).to match('/parent/child/project-path') end + end - it 'can recognise combined paths' do - expect(described_class.contains_path_part?('some/user/admin/path', 'user/admin')). - to be_truthy - end + describe '.without_reserved_child_paths_regex' do + it 'rejects paths containing a child reserved word' do + subject = described_class.without_reserved_child_paths_regex - it 'only recognizes full paths' do - expect(described_class.contains_path_part?('frontend-fixtures', 's')). - to be_falsy - end - - it 'handles regex chars gracefully' do - expect(described_class.contains_path_part?('frontend-fixtures', '-')). - to be_falsy + expect(subject).not_to match('hello/group_members') + expect(subject).not_to match('hello/activity/in-the-middle') + expect(subject).not_to match('foo/bar1/refs/master/logs_tree') end end describe ".valid?" do it 'is not case sensitive' do - expect(described_class.valid?("Users")).to be(false) + expect(described_class.valid?("Users")).to be_falsey end it "isn't valid when the top level is reserved" do test_path = 'u/should-be-a/reserved-word' - expect(described_class.valid?(test_path)).to be(false) + expect(described_class.valid?(test_path)).to be_falsey end it "isn't valid if any of the path segments is reserved" do test_path = 'the-wildcard/wikis/is-not-allowed' - expect(described_class.valid?(test_path)).to be(false) + expect(described_class.valid?(test_path)).to be_falsey end it "is valid if the path doesn't contain reserved words" do test_path = 'there-are/no-wildcards/in-this-path' - expect(described_class.valid?(test_path)).to be(true) + expect(described_class.valid?(test_path)).to be_truthy + end + + it 'allows allows a child path on the last spot' do + test_path = 'there/can-be-a/project-called/labels' + + expect(described_class.valid?(test_path)).to be_truthy + end + + it 'rejects a child path somewhere else' do + test_path = 'there/can-be-no/labels/group' + + expect(described_class.valid?(test_path)).to be_falsey end end end From 2e2a63c8669a084ed3a3aa5e770158ea2cb43a9d Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Sun, 30 Apr 2017 20:06:11 +0200 Subject: [PATCH 31/38] Rename child namespaces in migrationhelpers --- ...412174900_rename_reserved_dynamic_paths.rb | 11 ++++++++ .../rename_reserved_paths_migration/v1.rb | 7 ++++- .../v1/rename_namespaces.rb | 2 +- .../v1/rename_namespaces_spec.rb | 8 +++--- .../v1_spec.rb} | 28 ++++++++++++------- 5 files changed, 40 insertions(+), 16 deletions(-) rename spec/lib/gitlab/database/{rename_reserved_paths_migration_spec.rb => rename_reserved_paths_migration/v1_spec.rb} (62%) diff --git a/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb index 0fe04a23959..a23f83205f1 100644 --- a/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb +++ b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb @@ -23,6 +23,7 @@ class RenameReservedDynamicPaths < ActiveRecord::Migration notification_settings oauth sent_notifications + unicorn_test uploads users ] @@ -33,9 +34,19 @@ class RenameReservedDynamicPaths < ActiveRecord::Migration info/lfs/objects ] + DISSALLOWED_GROUP_PATHS = %w[ + activity + avatar + group_members + labels + milestones + subgroups + ] + def up rename_root_paths(DISALLOWED_ROOT_PATHS) rename_wildcard_paths(DISALLOWED_WILDCARD_PATHS) + rename_child_paths(DISSALLOWED_GROUP_PATHS) end def down diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1.rb index 6296e964187..1966f5c1cec 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/v1.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1.rb @@ -7,11 +7,16 @@ module Gitlab end def rename_wildcard_paths(one_or_more_paths) + rename_child_paths(one_or_more_paths) paths = Array(one_or_more_paths) - RenameNamespaces.new(paths, self).rename_namespaces(type: :wildcard) RenameProjects.new(paths, self).rename_projects end + def rename_child_paths(one_or_more_paths) + paths = Array(one_or_more_paths) + RenameNamespaces.new(paths, self).rename_namespaces(type: :child) + end + def rename_root_paths(paths) paths = Array(paths) RenameNamespaces.new(paths, self).rename_namespaces(type: :top_level) diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces.rb index d863de90844..b9f4f3cff3c 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces.rb @@ -13,7 +13,7 @@ module Gitlab def namespaces_for_paths(type:) namespaces = case type - when :wildcard + when :child MigrationClasses::Namespace.where.not(parent_id: nil) when :top_level MigrationClasses::Namespace.where(parent_id: nil) diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb index 6d58413edf2..a25c5da488a 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb @@ -21,13 +21,13 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces do parent = create(:namespace, path: 'parent') child = create(:namespace, path: 'the-path', parent: parent) - found_ids = subject.namespaces_for_paths(type: :wildcard). + found_ids = subject.namespaces_for_paths(type: :child). map(&:id) expect(found_ids).to contain_exactly(child.id) end end - context 'for wildcard namespaces' do + context 'for child namespaces' do it 'only returns child namespaces with the correct path' do _root_namespace = create(:namespace, path: 'THE-path') _other_path = create(:namespace, @@ -37,7 +37,7 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces do path: 'the-path', parent: create(:namespace)) - found_ids = subject.namespaces_for_paths(type: :wildcard). + found_ids = subject.namespaces_for_paths(type: :child). map(&:id) expect(found_ids).to contain_exactly(namespace.id) end @@ -165,7 +165,7 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces do expect(subject).to receive(:rename_namespace). with(migration_namespace(child_namespace)) - subject.rename_namespaces(type: :wildcard) + subject.rename_namespaces(type: :child) end end end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb similarity index 62% rename from spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb rename to spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb index 743054e0efc..f8cc1eb91ec 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb @@ -1,5 +1,18 @@ require 'spec_helper' +shared_examples 'renames child namespaces' do |type| + it 'renames namespaces' do + rename_namespaces = double + expect(described_class::RenameNamespaces). + to receive(:new).with(['first-path', 'second-path'], subject). + and_return(rename_namespaces) + expect(rename_namespaces).to receive(:rename_namespaces). + with(type: :child) + + subject.rename_wildcard_paths(['first-path', 'second-path']) + end +end + describe Gitlab::Database::RenameReservedPathsMigration::V1 do let(:subject) { FakeRenameReservedPathMigrationV1.new } @@ -7,17 +20,12 @@ describe Gitlab::Database::RenameReservedPathsMigration::V1 do allow(subject).to receive(:say) end - describe '#rename_wildcard_paths' do - it 'should rename namespaces' do - rename_namespaces = double - expect(described_class::RenameNamespaces). - to receive(:new).with(['first-path', 'second-path'], subject). - and_return(rename_namespaces) - expect(rename_namespaces).to receive(:rename_namespaces). - with(type: :wildcard) + describe '#rename_child_paths' do + it_behaves_like 'renames child namespaces' + end - subject.rename_wildcard_paths(['first-path', 'second-path']) - end + describe '#rename_wildcard_paths' do + it_behaves_like 'renames child namespaces' it 'should rename projects' do rename_projects = double From c853dd6158af0f77721ce59c03f5f05e98eeadba Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 2 May 2017 09:13:10 +0200 Subject: [PATCH 32/38] Reuse Gitlab::Regex.full_namespace_regex in the DynamicPathValidator --- app/validators/dynamic_path_validator.rb | 43 ++++++++----------- lib/gitlab/regex.rb | 4 ++ spec/lib/gitlab/regex_spec.rb | 4 +- .../validators/dynamic_path_validator_spec.rb | 11 ++++- 4 files changed, 34 insertions(+), 28 deletions(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index c4ed5ac85eb..ce90e6f01dd 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -15,7 +15,7 @@ class DynamicPathValidator < ActiveModel::EachValidator # # the path `api` shouldn't be allowed because it would be masked by `api/*` # - TOP_LEVEL_ROUTES = Set.new(%w[ + TOP_LEVEL_ROUTES = %w[ - .well-known abuse_reports @@ -59,7 +59,7 @@ class DynamicPathValidator < ActiveModel::EachValidator unsubscribes uploads users - ]).freeze + ].freeze # All project routes with wildcard argument must be listed here. # Otherwise it can lead to routing issues when route considered as project name. @@ -70,7 +70,7 @@ class DynamicPathValidator < ActiveModel::EachValidator # without tree as reserved name routing can match 'group/project' as group name, # 'tree' as project name and 'deploy_keys' as route. # - WILDCARD_ROUTES = Set.new(%w[ + WILDCARD_ROUTES = %w[ badges blame blob @@ -91,7 +91,7 @@ class DynamicPathValidator < ActiveModel::EachValidator tree update wikis - ]).freeze + ].freeze # These are all the paths that follow `/groups/*id/ or `/groups/*group_id` # We need to reject these because we have a `/groups/*id` page that is the same @@ -100,7 +100,7 @@ class DynamicPathValidator < ActiveModel::EachValidator # If we would allow a subgroup to be created with the name `activity` then # this group would not be accessible through `/groups/parent/activity` since # this would map to the activity-page of it's parent. - GROUP_ROUTES = Set.new(%w[ + GROUP_ROUTES = %w[ activity avatar edit @@ -111,16 +111,16 @@ class DynamicPathValidator < ActiveModel::EachValidator milestones projects subgroups - ]) + ].freeze CHILD_ROUTES = (WILDCARD_ROUTES | GROUP_ROUTES).freeze def self.without_reserved_wildcard_paths_regex - @full_path_without_wildcard_regex ||= regex_excluding_child_paths(WILDCARD_ROUTES) + @without_reserved_wildcard_paths_regex ||= regex_excluding_child_paths(WILDCARD_ROUTES) end def self.without_reserved_child_paths_regex - @full_path_without_child_routes_regex ||= regex_excluding_child_paths(CHILD_ROUTES) + @without_reserved_child_paths_regex ||= regex_excluding_child_paths(CHILD_ROUTES) end # This is used to validate a full path. @@ -128,22 +128,19 @@ class DynamicPathValidator < ActiveModel::EachValidator # - Starting with one of the top level words # - Containing one of the child level words in the middle of a path def self.regex_excluding_child_paths(child_routes) - reserved_top_level_words = Regexp.union(TOP_LEVEL_ROUTES.to_a) - not_starting_in_reserved_word = %r{^(/?)(?!(#{reserved_top_level_words})(/|$))} + reserved_top_level_words = Regexp.union(TOP_LEVEL_ROUTES) + not_starting_in_reserved_word = %r{\A/?(?!(#{reserved_top_level_words})(/|\z))} - reserved_child_level_words = Regexp.union(child_routes.to_a) - not_containing_reserved_child = %r{(?!(\S+)/(#{reserved_child_level_words})(/|$))} + reserved_child_level_words = Regexp.union(child_routes) + not_containing_reserved_child = %r{(?!\S+/(#{reserved_child_level_words})(/|\z))} - @full_path_regex = %r{ - #{not_starting_in_reserved_word} - #{not_containing_reserved_child} - #{Gitlab::Regex::FULL_NAMESPACE_REGEX_STR}}x + %r{#{not_starting_in_reserved_word} + #{not_containing_reserved_child} + #{Gitlab::Regex.full_namespace_regex}}x end def self.valid?(path) - path_segments = path.split('/') - - !reserved?(path) && path_segments.all? { |value| follow_format?(value) } + path =~ Gitlab::Regex.full_namespace_regex && !reserved?(path) end def self.reserved?(path) @@ -165,13 +162,9 @@ class DynamicPathValidator < ActiveModel::EachValidator path !~ without_reserved_wildcard_paths_regex end - def self.follow_format?(value) - value =~ Gitlab::Regex.namespace_regex - end - delegate :reserved?, :any_reserved?, - :follow_format?, to: :class + to: :class def valid_full_path?(record, value) full_path = record.respond_to?(:full_path) ? record.full_path : value @@ -185,7 +178,7 @@ class DynamicPathValidator < ActiveModel::EachValidator end def validate_each(record, attribute, value) - unless follow_format?(value) + unless value =~ Gitlab::Regex.namespace_regex record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) end diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 08b061d5e31..b7fef5dd068 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -22,6 +22,10 @@ module Gitlab @namespace_regex ||= /\A#{NAMESPACE_REGEX_STR}\z/.freeze end + def full_namespace_regex + @full_namespace_regex ||= %r{\A#{FULL_NAMESPACE_REGEX_STR}\z} + end + def namespace_route_regex @namespace_route_regex ||= /#{NAMESPACE_REGEX_STR}/.freeze end diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb index 127cd8c78d8..72e947f2cc2 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -45,8 +45,8 @@ describe Gitlab::Regex, lib: true do it { is_expected.not_to match('foo-') } end - describe 'FULL_NAMESPACE_REGEX_STR' do - subject { %r{\A#{Gitlab::Regex::FULL_NAMESPACE_REGEX_STR}\z} } + describe '.full_namespace_regex' do + subject { described_class.full_namespace_regex } it { is_expected.to match('gitlab.org') } it { is_expected.to match('gitlab.org/gitlab-git') } diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb index f43b4892456..4924706b88e 100644 --- a/spec/validators/dynamic_path_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -129,6 +129,10 @@ describe DynamicPathValidator do expect(subject).not_to match('dashboard') end + it 'matches valid paths with a toplevel word in a different place' do + expect(subject).to match('parent/dashboard/project-path') + end + it 'rejects paths containing a wildcard reserved word' do expect(subject).not_to match('hello/edit') expect(subject).not_to match('hello/edit/in-the-middle') @@ -137,7 +141,6 @@ describe DynamicPathValidator do it 'matches valid paths' do expect(subject).to match('parent/child/project-path') - expect(subject).to match('/parent/child/project-path') end end @@ -185,5 +188,11 @@ describe DynamicPathValidator do expect(described_class.valid?(test_path)).to be_falsey end + + it 'rejects paths that are in an incorrect format' do + test_path = 'incorrect/format.git' + + expect(described_class.valid?(test_path)).to be_falsey + end end end From a035ebbe069fa6b203c279c8944b447f8fe896c5 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 2 May 2017 10:32:31 +0200 Subject: [PATCH 33/38] Update path validation & specs --- app/validators/dynamic_path_validator.rb | 31 +++++++------ .../validators/dynamic_path_validator_spec.rb | 43 +++++++++++++++++-- 2 files changed, 58 insertions(+), 16 deletions(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index ce90e6f01dd..ba142ea06a6 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -140,17 +140,17 @@ class DynamicPathValidator < ActiveModel::EachValidator end def self.valid?(path) - path =~ Gitlab::Regex.full_namespace_regex && !reserved?(path) + path =~ Gitlab::Regex.full_namespace_regex && !full_path_reserved?(path) end - def self.reserved?(path) + def self.full_path_reserved?(path) path = path.to_s.downcase - _project_parts, namespace_parts = path.reverse.split('/', 2).map(&:reverse) + _project_part, namespace_parts = path.reverse.split('/', 2).map(&:reverse) - wildcard_reserved?(path) || any_reserved?(namespace_parts) + wildcard_reserved?(path) || child_reserved?(namespace_parts) end - def self.any_reserved?(path) + def self.child_reserved?(path) return false unless path path !~ without_reserved_child_paths_regex @@ -162,18 +162,23 @@ class DynamicPathValidator < ActiveModel::EachValidator path !~ without_reserved_wildcard_paths_regex end - delegate :reserved?, - :any_reserved?, + delegate :full_path_reserved?, + :child_reserved?, to: :class - def valid_full_path?(record, value) + def path_reserved_for_record?(record, value) full_path = record.respond_to?(:full_path) ? record.full_path : value - case record - when Project || User - reserved?(full_path) + # For group paths the entire path cannot contain a reserved child word + # The path doesn't contain the last `_project_part` so we need to validate + # if the entire path. + # Example: + # A *group* with full path `parent/activity` is reserved. + # A *project* with full path `parent/activity` is allowed. + if record.is_a? Group + child_reserved?(full_path) else - any_reserved?(full_path) + full_path_reserved?(full_path) end end @@ -182,7 +187,7 @@ class DynamicPathValidator < ActiveModel::EachValidator record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) end - if valid_full_path?(record, value) + if path_reserved_for_record?(record, value) record.errors.add(attribute, "#{value} is a reserved name") end end diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb index 4924706b88e..deeff477193 100644 --- a/spec/validators/dynamic_path_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -144,14 +144,19 @@ describe DynamicPathValidator do end end - describe '.without_reserved_child_paths_regex' do - it 'rejects paths containing a child reserved word' do - subject = described_class.without_reserved_child_paths_regex + describe '.regex_excluding_child_paths' do + let(:subject) { described_class.without_reserved_child_paths_regex } + it 'rejects paths containing a child reserved word' do expect(subject).not_to match('hello/group_members') expect(subject).not_to match('hello/activity/in-the-middle') expect(subject).not_to match('foo/bar1/refs/master/logs_tree') end + + it 'allows a child path on the top level' do + expect(subject).to match('activity/foo') + expect(subject).to match('avatar') + end end describe ".valid?" do @@ -195,4 +200,36 @@ describe DynamicPathValidator do expect(described_class.valid?(test_path)).to be_falsey end end + + describe '#path_reserved_for_record?' do + it 'reserves a sub-group named activity' do + group = build(:group, :nested, path: 'activity') + + expect(validator.path_reserved_for_record?(group, 'activity')).to be_truthy + end + + it "doesn't reserve a project called activity" do + project = build(:project, path: 'activity') + + expect(validator.path_reserved_for_record?(project, 'activity')).to be_falsey + end + end + + describe '#validates_each' do + it 'adds a message when the path is not in the correct format' do + group = build(:group) + + validator.validate_each(group, :path, "Path with spaces, and comma's!") + + expect(group.errors[:path]).to include(Gitlab::Regex.namespace_regex_message) + end + + it 'adds a message when the path is not in the correct format' do + group = build(:group, path: 'users') + + validator.validate_each(group, :path, 'users') + + expect(group.errors[:path]).to include('users is a reserved name') + end + end end From e2b9420c11cc5f328fc4014f5bfe66bacd3c8028 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 2 May 2017 11:48:54 +0200 Subject: [PATCH 34/38] Add a better error message when a certain path is missing --- .../validators/dynamic_path_validator_spec.rb | 37 +++++++++++++++++-- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb index deeff477193..b114bfc1bca 100644 --- a/spec/validators/dynamic_path_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -37,6 +37,24 @@ describe DynamicPathValidator do described_class::WILDCARD_ROUTES.include?(path.split('/').first) end + def failure_message(missing_words, constant_name, migration_helper) + missing_words = Array(missing_words) + <<-MSG + Found new routes that could cause conflicts with existing namespaced routes + for groups or projects. + + Add <#{missing_words.join(', ')}> to `DynamicPathValidator::#{constant_name} + to make sure no projects or namespaces can be created with those paths. + + To rename any existing records with those paths you can use the + `Gitlab::Database::RenameReservedpathsMigration::.#{migration_helper}` + migration helper. + + Make sure to make a note of the renamed records in the release blog post. + + MSG + end + let(:all_routes) do Rails.application.routes.routes.routes. map { |r| r.path.spec.to_s } @@ -101,13 +119,25 @@ describe DynamicPathValidator do describe 'TOP_LEVEL_ROUTES' do it 'includes all the top level namespaces' do - expect(described_class::TOP_LEVEL_ROUTES).to include(*top_level_words) + failure_block = lambda do + missing_words = top_level_words - described_class::TOP_LEVEL_ROUTES + failure_message(missing_words, 'TOP_LEVEL_ROUTES', 'rename_root_paths') + end + + expect(described_class::TOP_LEVEL_ROUTES) + .to include(*top_level_words), failure_block end end describe 'GROUP_ROUTES' do it "don't contain a second wildcard" do - expect(described_class::GROUP_ROUTES).to include(*paths_after_group_id) + failure_block = lambda do + missing_words = paths_after_group_id - described_class::GROUP_ROUTES + failure_message(missing_words, 'GROUP_ROUTES', 'rename_child_paths') + end + + expect(described_class::GROUP_ROUTES) + .to include(*paths_after_group_id), failure_block end end @@ -115,7 +145,8 @@ describe DynamicPathValidator do it 'includes all paths that can be used after a namespace/project path' do aggregate_failures do all_wildcard_paths.each do |path| - expect(wildcards_include?(path)).to be(true), "Expected #{path} to be rejected" + expect(wildcards_include?(path)) + .to be(true), failure_message(path, 'WILDCARD_ROUTES', 'rename_wildcard_paths') end end end From 29f200110245443454fd4358ed8c71ff8607fdd7 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 2 May 2017 12:36:58 +0200 Subject: [PATCH 35/38] Update comments --- app/validators/dynamic_path_validator.rb | 23 +++++++++++++++---- .../rename_reserved_paths_migration/v1.rb | 8 +++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index ba142ea06a6..7a14aed0c3e 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -61,15 +61,28 @@ class DynamicPathValidator < ActiveModel::EachValidator users ].freeze - # All project routes with wildcard argument must be listed here. - # Otherwise it can lead to routing issues when route considered as project name. + # This list should contain all words following `/*namespace_id/:project_id` in + # routes that contain a second wildcard. # # Example: - # /group/project/tree/deploy_keys + # /*namespace_id/:project_id/badges/*ref/build # - # without tree as reserved name routing can match 'group/project' as group name, - # 'tree' as project name and 'deploy_keys' as route. + # If `badges` was allowed as a project/group name, we would not be able to access the + # `badges` route for those projects: # + # Consider a namespace with path `foo/bar` and a project called `badges`. + # The route to the build badge would then be `/foo/bar/badges/badges/master/build.svg` + # + # When accessing this path the route would be matched to the `badges` path + # with the following params: + # - namespace_id: `foo` + # - project_id: `bar` + # - ref: `badges/master` + # + # Failing to find the project, this would result in a 404. + # + # By rejecting `badges` the router can _count_ on the fact that `badges` will + # be preceded by the `namespace/project`. WILDCARD_ROUTES = %w[ badges blame diff --git a/lib/gitlab/database/rename_reserved_paths_migration/v1.rb b/lib/gitlab/database/rename_reserved_paths_migration/v1.rb index 1966f5c1cec..89530082cd2 100644 --- a/lib/gitlab/database/rename_reserved_paths_migration/v1.rb +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1.rb @@ -1,3 +1,11 @@ +# This module can be included in migrations to make it easier to rename paths +# of `Namespace` & `Project` models certain paths would become `reserved`. +# +# If the way things are stored on the filesystem related to namespaces and +# projects ever changes. Don't update this module, or anything nested in `V1`, +# since it needs to keep functioning for all migrations using it using the state +# that the data is in at the time. Instead, create a `V2` module that implements +# the new way of reserving paths. module Gitlab module Database module RenameReservedPathsMigration From ffc486a58547b6984a44a43097183b34592be608 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 2 May 2017 14:10:55 +0200 Subject: [PATCH 36/38] Add some documentation for the new migration helpers --- doc/development/migration_style_guide.md | 25 ++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 3e8b709c18f..77ba2a5fd87 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -270,3 +270,28 @@ end When doing so be sure to explicitly set the model's table name so it's not derived from the class name or namespace. + +### Renaming reserved paths + +When a new route for projects is introduced that could conflict with any +existing records. The path for this records should be renamed, and the +related data should be moved on disk. + +Since we had to do this a few times already, there are now some helpers to help +with this. + +To use this you can include `Gitlab::Database::RenameReservedPathsMigration::V1` +in your migration. This will provide 3 methods which you can pass one or more +paths that need to be rejected. + +**`rename_root_paths`**: This will rename the path of all _namespaces_ with the +given name that don't have a `parent_id`. + +**`rename_child_paths`**: This will rename the path of all _namespaces_ with the +given name that have a `parent_id`. + +**`rename_wildcard_paths`**: This will rename the path of all _projects_, and all +_namespaces_ that have a `project_id`. + +The `path` column for these rows will be renamed to their previous value followed +by an integer. For example: `users` would turn into `users0` From 3dd2476eb395ca706763210ef1f37978889c595d Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 2 May 2017 15:24:41 +0200 Subject: [PATCH 37/38] Refresh the markdown cache if it was `nil` If the cached html_field for a markdown_field is `nil` while the mfarkdown_field is not, it needs to be refreshed. --- app/models/concerns/cache_markdown_field.rb | 3 +++ spec/models/concerns/cache_markdown_field_spec.rb | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index f033028c4e5..eb32bf3d32a 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -78,6 +78,9 @@ module CacheMarkdownField def cached_html_up_to_date?(markdown_field) html_field = cached_markdown_fields.html_field(markdown_field) + cached = !cached_html_for(markdown_field).nil? && !__send__(markdown_field).nil? + return false unless cached + markdown_changed = attribute_changed?(markdown_field) || false html_changed = attribute_changed?(html_field) || false diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 4edafbc4e32..40bbb10eaac 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -170,6 +170,12 @@ describe CacheMarkdownField do is_expected.to be_truthy end + + it 'returns false if the markdown field is set but the html is not' do + thing.foo_html = nil + + is_expected.to be_falsy + end end describe '#refresh_markdown_cache!' do From 49a8e5f510723eb39a948efe87e1af3b0abb49f6 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 2 May 2017 17:26:32 +0200 Subject: [PATCH 38/38] Don't validate reserved words if the format doesn't match Because it also won't match the sophisticated format we have for detecting reserved names. We don't want to confuse the user with 2 error messages --- app/validators/dynamic_path_validator.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index 7a14aed0c3e..226eb6b313c 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -198,6 +198,7 @@ class DynamicPathValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) unless value =~ Gitlab::Regex.namespace_regex record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) + return end if path_reserved_for_record?(record, value)