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/app/models/namespace.rb b/app/models/namespace.rb index 9bfa731785f..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 @@ -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/models/project.rb b/app/models/project.rb index 9d64e5d406d..6a8f8c3500f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -196,13 +196,14 @@ class Project < ActiveRecord::Base message: Gitlab::Regex.project_name_regex_message } validates :path, presence: true, - project_path: true, + dynamic_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/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/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb new file mode 100644 index 00000000000..226eb6b313c --- /dev/null +++ b/app/validators/dynamic_path_validator.rb @@ -0,0 +1,208 @@ +# DynamicPathValidator +# +# 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 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. + # + # Example: + # /api/api-project + # + # the path `api` shouldn't be allowed because it would be masked by `api/*` + # + TOP_LEVEL_ROUTES = %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 + repository + robots.txt + s + search + sent_notifications + services + snippets + teams + u + unicorn_test + unsubscribes + uploads + users + ].freeze + + # This list should contain all words following `/*namespace_id/:project_id` in + # routes that contain a second wildcard. + # + # Example: + # /*namespace_id/:project_id/badges/*ref/build + # + # 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 + blob + builds + commits + create + create_dir + edit + environments/folders + files + find_file + gitlab-lfs/objects + info/lfs/objects + new + preview + raw + refs + tree + update + wikis + ].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 = %w[ + activity + avatar + edit + group_members + issues + labels + merge_requests + milestones + projects + subgroups + ].freeze + + CHILD_ROUTES = (WILDCARD_ROUTES | GROUP_ROUTES).freeze + + def self.without_reserved_wildcard_paths_regex + @without_reserved_wildcard_paths_regex ||= regex_excluding_child_paths(WILDCARD_ROUTES) + end + + def self.without_reserved_child_paths_regex + @without_reserved_child_paths_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) + not_starting_in_reserved_word = %r{\A/?(?!(#{reserved_top_level_words})(/|\z))} + + reserved_child_level_words = Regexp.union(child_routes) + not_containing_reserved_child = %r{(?!\S+/(#{reserved_child_level_words})(/|\z))} + + %r{#{not_starting_in_reserved_word} + #{not_containing_reserved_child} + #{Gitlab::Regex.full_namespace_regex}}x + end + + def self.valid?(path) + path =~ Gitlab::Regex.full_namespace_regex && !full_path_reserved?(path) + end + + def self.full_path_reserved?(path) + path = path.to_s.downcase + _project_part, namespace_parts = path.reverse.split('/', 2).map(&:reverse) + + wildcard_reserved?(path) || child_reserved?(namespace_parts) + end + + def self.child_reserved?(path) + return false unless path + + path !~ without_reserved_child_paths_regex + end + + def self.wildcard_reserved?(path) + return false unless path + + path !~ without_reserved_wildcard_paths_regex + end + + delegate :full_path_reserved?, + :child_reserved?, + to: :class + + def path_reserved_for_record?(record, value) + full_path = record.respond_to?(:full_path) ? record.full_path : value + + # 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 + full_path_reserved?(full_path) + end + end + + 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) + record.errors.add(attribute, "#{value} is a reserved name") + end + end +end diff --git a/app/validators/namespace_validator.rb b/app/validators/namespace_validator.rb deleted file mode 100644 index 77ca033e97f..00000000000 --- a/app/validators/namespace_validator.rb +++ /dev/null @@ -1,73 +0,0 @@ -# NamespaceValidator -# -# Custom validator for GitLab namespace values. -# -# Values are checked for formatting and exclusion from a list of reserved path -# names. -class NamespaceValidator < ActiveModel::EachValidator - RESERVED = %w[ - .well-known - admin - all - assets - ci - dashboard - files - groups - help - hooks - issues - merge_requests - new - notes - profile - projects - public - repository - robots.txt - s - search - services - snippets - teams - u - unsubscribes - users - ].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].freeze - - STRICT_RESERVED = (RESERVED + WILDCARD_ROUTES).freeze - - def self.valid?(value) - !reserved?(value) && follow_format?(value) - end - - def self.reserved?(value, strict: false) - if strict - STRICT_RESERVED.include?(value) - else - RESERVED.include?(value) - end - end - - def self.follow_format?(value) - value =~ Gitlab::Regex.namespace_regex - end - - delegate :reserved?, :follow_format?, to: :class - - def validate_each(record, attribute, value) - unless follow_format?(value) - record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) - end - - strict = record.is_a?(Group) && record.parent_id - - if reserved?(value, strict: strict) - record.errors.add(attribute, "#{value} is a reserved name") - end - end -end diff --git a/app/validators/project_path_validator.rb b/app/validators/project_path_validator.rb deleted file mode 100644 index ee2ae65be7b..00000000000 --- a/app/validators/project_path_validator.rb +++ /dev/null @@ -1,35 +0,0 @@ -# ProjectPathValidator -# -# Custom validator for GitLab project path values. -# -# 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 - - def self.valid?(value) - !reserved?(value) - end - - def self.reserved?(value) - RESERVED.include?(value) - 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/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: 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..a23f83205f1 --- /dev/null +++ b/db/post_migrate/20170412174900_rename_reserved_dynamic_paths.rb @@ -0,0 +1,55 @@ +# 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::V1 + + DOWNTIME = false + + disable_ddl_transaction! + + DISALLOWED_ROOT_PATHS = %w[ + - + abuse_reports + api + autocomplete + explore + health_check + import + invites + jwt + koding + member + notification_settings + oauth + sent_notifications + unicorn_test + uploads + users + ] + + DISALLOWED_WILDCARD_PATHS = %w[ + environments/folders + gitlab-lfs/objects + 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 + # nothing to do + end +end 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` diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb index bae4db1ca4d..1501f64d537 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 DynamicPathValidator.valid?(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/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb index a10b4657d7d..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 ProjectPathValidator.valid?(project_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/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/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..89530082cd2 --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1.rb @@ -0,0 +1,35 @@ +# 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 + module V1 + def self.included(kls) + kls.include(MigrationHelpers) + end + + def rename_wildcard_paths(one_or_more_paths) + rename_child_paths(one_or_more_paths) + paths = Array(one_or_more_paths) + 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) + 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..4fdcb682c2f --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1/migration_classes.rb @@ -0,0 +1,76 @@ +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 + @full_path = nil + end + 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 + + # 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..de4e6e7c404 --- /dev/null +++ b/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base.rb @@ -0,0 +1,131 @@ +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 + + update_column_in_batches(:milestones, :description_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..b9f4f3cff3c --- /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 :child + 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/lib/gitlab/etag_caching/router.rb b/lib/gitlab/etag_caching/router.rb index f6e4f279c06..aac210f19e8 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 rendered_title + commit pipelines merge_requests new].freeze + RESERVED_WORDS = DynamicPathValidator::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/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/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/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 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 new file mode 100644 index 00000000000..64bc5fc0429 --- /dev/null +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb @@ -0,0 +1,197 @@ +require 'spec_helper' + +describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase do + let(:migration) { FakeRenameReservedPathMigrationV1.new } + let(:subject) { described_class.new(['the-path'], migration) } + + before do + allow(migration).to receive(:say) + end + + def migration_namespace(namespace) + Gitlab::Database::RenameReservedPathsMigration::V1::MigrationClasses:: + Namespace.find(namespace.id) + end + + def migration_project(project) + Gitlab::Database::RenameReservedPathsMigration::V1::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 '#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 + + 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 + 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/v1/rename_namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb new file mode 100644 index 00000000000..a25c5da488a --- /dev/null +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb @@ -0,0 +1,171 @@ +require 'spec_helper' + +describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces do + let(:migration) { FakeRenameReservedPathMigrationV1.new } + let(:subject) { described_class.new(['the-path'], migration) } + + before do + allow(migration).to receive(:say) + end + + def migration_namespace(namespace) + Gitlab::Database::RenameReservedPathsMigration::V1::MigrationClasses:: + Namespace.find(namespace.id) + 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: :child). + map(&:id) + expect(found_ids).to contain_exactly(child.id) + end + end + + 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, + path: 'other', + parent: create(:namespace)) + namespace = create(:namespace, + path: 'the-path', + parent: create(:namespace)) + + found_ids = subject.namespaces_for_paths(type: :child). + map(&:id) + expect(found_ids).to contain_exactly(namespace.id) + end + end + + 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') + _child_namespace = create(:namespace, + path: 'the-path', + parent: create(:namespace)) + + found_ids = subject.namespaces_for_paths(type: :top_level). + map(&: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 "#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 "#rename_namespace" do + let(:namespace) { create(:namespace, path: 'the-path') } + + 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 + 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 + expect(subject).to receive(:move_uploads).with("the-path", "the-path0") + + subject.rename_namespace(namespace) + end + + it "moves the pages for the namespace" do + expect(subject).to receive(:move_pages).with("the-path", "the-path0") + + subject.rename_namespace(namespace) + end + + 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 + let!(:top_level_namespace) { create(:namespace, path: 'the-path') } + let!(:child_namespace) do + create(:namespace, path: 'the-path', parent: create(:namespace)) + end + + it 'renames top level namespaces the namespace' do + expect(subject).to receive(:rename_namespace). + with(migration_namespace(top_level_namespace)) + + 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(type: :child) + end + end +end diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb new file mode 100644 index 00000000000..59e8de2712d --- /dev/null +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb @@ -0,0 +1,102 @@ +require 'spec_helper' + +describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects do + let(:migration) { FakeRenameReservedPathMigrationV1.new } + let(:subject) { described_class.new(['the-path'], migration) } + + before do + allow(migration).to receive(:say) + 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) + + result_ids = subject.projects_for_paths.map(&:id) + + expect(result_ids).to contain_exactly(project.id) + 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, + 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) + + expect(project.reload.path).to eq('the-path0') + 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/v1_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb new file mode 100644 index 00000000000..f8cc1eb91ec --- /dev/null +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1_spec.rb @@ -0,0 +1,54 @@ +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 } + + before do + allow(subject).to receive(:say) + end + + describe '#rename_child_paths' do + it_behaves_like 'renames child namespaces' + end + + describe '#rename_wildcard_paths' do + it_behaves_like 'renames child namespaces' + + it 'should rename projects' do + rename_projects = double + expect(described_class::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 + end + + describe '#rename_root_paths' do + it 'should rename namespaces' do + rename_namespaces = double + expect(described_class::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 +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/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 diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 8ffde6f7fbb..a11805926cc 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -57,6 +57,32 @@ 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 + + it 'rejects reserved group paths' do + group = build(:group, path: 'activity', parent: create(:group)) + + expect(group).not_to be_valid + end + end end describe '.visible_to_user' do diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index e406d0a16bd..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 @@ -47,6 +54,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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index d9244657953..49455303096 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -253,6 +253,34 @@ 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 + + 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 + + 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 describe 'default_scope' do 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/support/fake_migration_classes.rb b/spec/support/fake_migration_classes.rb new file mode 100644 index 00000000000..3de0460c3ca --- /dev/null +++ b/spec/support/fake_migration_classes.rb @@ -0,0 +1,3 @@ +class FakeRenameReservedPathMigrationV1 < ActiveRecord::Migration + include Gitlab::Database::RenameReservedPathsMigration::V1 +end diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb new file mode 100644 index 00000000000..b114bfc1bca --- /dev/null +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -0,0 +1,266 @@ +require 'spec_helper' + +describe DynamicPathValidator do + let(:validator) { described_class.new(attributes: [:path]) } + + # 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` + # -> 'builds/artifacts' + def path_before_wildcard(path) + path = path.gsub(STARTING_WITH_NAMESPACE, "") + path_segments = path.split('/').reject(&:empty?) + wildcard_index = path_segments.index { |segment| parameter?(segment) } + + segments_before_wildcard = path_segments[0..wildcard_index - 1] + + 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` & `build` are covered by reserving the word + # `build` + def wildcards_include?(path) + described_class::WILDCARD_ROUTES.include?(path) || + 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 } + 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{^/[:*]} } } + + 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 a path-part that includes a wildcard parameter `*` + # At the time of writing these routes match: http://rubular.com/r/Rv2pDE5Dvw + 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}} + 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_before_wildcard(route) + 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 + + describe 'TOP_LEVEL_ROUTES' do + it 'includes all the top level namespaces' do + 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 + 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 + + describe 'WILDCARD_ROUTES' 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), failure_message(path, 'WILDCARD_ROUTES', 'rename_wildcard_paths') + end + end + end + end + + 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 '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') + expect(subject).not_to match('foo/bar1/refs/master/logs_tree') + end + + it 'matches valid paths' do + expect(subject).to match('parent/child/project-path') + end + end + + 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 + it 'is not case sensitive' do + 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_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_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_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 + + 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 + + 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