From a035ebbe069fa6b203c279c8944b447f8fe896c5 Mon Sep 17 00:00:00 2001
From: Bob Van Landuyt <bob@gitlab.com>
Date: Tue, 2 May 2017 10:32:31 +0200
Subject: [PATCH] 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