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.

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/<id>/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