Reject group-routes as names of child namespaces

5 files changed, 147 insertions, 43 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
-  end
+  def self.any_reserved?(path)
+    return false unless path
 
-  def self.includes_reserved_top_level?(path)
-    TOP_LEVEL_ROUTES.any? do |reserved_route|
-      contains_path_part?(path, reserved_route)
-    end
+    path !~ without_reserved_child_paths_regex
   end
 
-  def self.contains_path_part?(path, part)
-    path =~ %r{(/|\A)#{Regexp.quote(part)}(/|\z)}
+  def self.wildcard_reserved?(path)
+    return false unless path
+
+    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
-    end
+  describe '.without_reserved_wildcard_paths_regex' do
+    subject { described_class.without_reserved_wildcard_paths_regex }
 
-    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 starting with a reserved top level' do
+      expect(subject).not_to match('dashboard/hello/world')
+      expect(subject).not_to match('dashboard')
     end
 
-    it 'skips partial path matches' do
-      expect(described_class.contains_path_part?('some/user1/path', 'user')).
-        to be_falsy
+    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 'can recognise combined paths' do
-      expect(described_class.contains_path_part?('some/user/admin/path', 'user/admin')).
-        to be_truthy
+    it 'matches valid paths' do
+      expect(subject).to match('parent/child/project-path')
+      expect(subject).to match('/parent/child/project-path')
     end
+  end
 
-    it 'only recognizes full paths' do
-      expect(described_class.contains_path_part?('frontend-fixtures', 's')).
-        to be_falsy
-    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 '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