Reallow project paths ending in periodsdm-reallow-project-path-ending-in-period

18 files changed, 223 insertions, 159 deletions

diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 0601a61a926..4d401e7ba18 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -36,7 +36,7 @@ class Namespace < ActiveRecord::Base
   validates :path,
     presence: true,
     length: { maximum: 255 },
-    dynamic_path: true
+    namespace_path: true
 
   validate :nesting_level_allowed
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 2f9b80d0514..5a0cbfeb282 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -240,10 +240,8 @@ class Project < ActiveRecord::Base
               message: Gitlab::Regex.project_name_regex_message }
   validates :path,
     presence: true,
-    dynamic_path: true,
+    project_path: true,
     length: { maximum: 255 },
-    format: { with: Gitlab::PathRegex.project_path_format_regex,
-              message: Gitlab::PathRegex.project_path_format_message },
     uniqueness: { scope: :namespace_id }
 
   validates :namespace, presence: true
diff --git a/app/models/user.rb b/app/models/user.rb
index bcda4564595..87a6c63cfaa 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -146,7 +146,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,
-    dynamic_path: true,
+    user_path: true,
     presence: true,
     uniqueness: { case_sensitive: false }
 
diff --git a/app/validators/abstract_path_validator.rb b/app/validators/abstract_path_validator.rb
new file mode 100644
index 00000000000..adbccb65a84
--- /dev/null
+++ b/app/validators/abstract_path_validator.rb
@@ -0,0 +1,38 @@
+class AbstractPathValidator < ActiveModel::EachValidator
+  extend Gitlab::EncodingHelper
+
+  def self.path_regex
+    raise NotImplementedError
+  end
+
+  def self.format_regex
+    raise NotImplementedError
+  end
+
+  def self.format_error_message
+    raise NotImplementedError
+  end
+
+  def self.full_path(record, value)
+    value
+  end
+
+  def self.valid_path?(path)
+    encode!(path)
+    "#{path}/" =~ path_regex
+  end
+
+  def validate_each(record, attribute, value)
+    unless value =~ self.class.format_regex
+      record.errors.add(attribute, self.class.format_error_message)
+      return
+    end
+
+    full_path = self.class.full_path(record, value)
+    return unless full_path
+
+    unless self.class.valid_path?(full_path)
+      record.errors.add(attribute, "#{value} is a reserved name")
+    end
+  end
+end
diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb
deleted file mode 100644
index 4688aabc2a8..00000000000
--- a/app/validators/dynamic_path_validator.rb
+++ /dev/null
@@ -1,53 +0,0 @@
-# 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 illegal path
-# names.
-class DynamicPathValidator < ActiveModel::EachValidator
-  extend Gitlab::EncodingHelper
-
-  class << self
-    def valid_user_path?(path)
-      encode!(path)
-      "#{path}/" =~ Gitlab::PathRegex.root_namespace_path_regex
-    end
-
-    def valid_group_path?(path)
-      encode!(path)
-      "#{path}/" =~ Gitlab::PathRegex.full_namespace_path_regex
-    end
-
-    def valid_project_path?(path)
-      encode!(path)
-      "#{path}/" =~ Gitlab::PathRegex.full_project_path_regex
-    end
-  end
-
-  def path_valid_for_record?(record, value)
-    full_path = record.respond_to?(:build_full_path) ? record.build_full_path : value
-
-    return true unless full_path
-
-    case record
-    when Project
-      self.class.valid_project_path?(full_path)
-    when Group
-      self.class.valid_group_path?(full_path)
-    else # User or non-Group Namespace
-      self.class.valid_user_path?(full_path)
-    end
-  end
-
-  def validate_each(record, attribute, value)
-    unless value =~ Gitlab::PathRegex.namespace_format_regex
-      record.errors.add(attribute, Gitlab::PathRegex.namespace_format_message)
-      return
-    end
-
-    unless path_valid_for_record?(record, value)
-      record.errors.add(attribute, "#{value} is a reserved name")
-    end
-  end
-end
diff --git a/app/validators/namespace_path_validator.rb b/app/validators/namespace_path_validator.rb
new file mode 100644
index 00000000000..4a0aa64ae0c
--- /dev/null
+++ b/app/validators/namespace_path_validator.rb
@@ -0,0 +1,19 @@
+class NamespacePathValidator < AbstractPathValidator
+  extend Gitlab::EncodingHelper
+
+  def self.path_regex
+    Gitlab::PathRegex.full_namespace_path_regex
+  end
+
+  def self.format_regex
+    Gitlab::PathRegex.namespace_format_regex
+  end
+
+  def self.format_error_message
+    Gitlab::PathRegex.namespace_format_message
+  end
+
+  def self.full_path(record, value)
+    record.build_full_path
+  end
+end
diff --git a/app/validators/project_path_validator.rb b/app/validators/project_path_validator.rb
new file mode 100644
index 00000000000..829b596ad3c
--- /dev/null
+++ b/app/validators/project_path_validator.rb
@@ -0,0 +1,19 @@
+class ProjectPathValidator < AbstractPathValidator
+  extend Gitlab::EncodingHelper
+
+  def self.path_regex
+    Gitlab::PathRegex.full_project_path_regex
+  end
+
+  def self.format_regex
+    Gitlab::PathRegex.project_path_format_regex
+  end
+
+  def self.format_error_message
+    Gitlab::PathRegex.project_path_format_message
+  end
+
+  def self.full_path(record, value)
+    record.build_full_path
+  end
+end
diff --git a/app/validators/user_path_validator.rb b/app/validators/user_path_validator.rb
new file mode 100644
index 00000000000..adf02901802
--- /dev/null
+++ b/app/validators/user_path_validator.rb
@@ -0,0 +1,15 @@
+class UserPathValidator < AbstractPathValidator
+  extend Gitlab::EncodingHelper
+
+  def self.path_regex
+    Gitlab::PathRegex.root_namespace_path_regex
+  end
+
+  def self.format_regex
+    Gitlab::PathRegex.namespace_format_regex
+  end
+
+  def self.format_error_message
+    Gitlab::PathRegex.namespace_format_message
+  end
+end
diff --git a/changelogs/unreleased/dm-reallow-project-path-ending-in-period.yml b/changelogs/unreleased/dm-reallow-project-path-ending-in-period.yml
new file mode 100644
index 00000000000..ad41d9b84c3
--- /dev/null
+++ b/changelogs/unreleased/dm-reallow-project-path-ending-in-period.yml
@@ -0,0 +1,5 @@
+---
+title: Reallow project paths ending in periods
+merge_request:
+author:
+type: fixed
diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb
index 6fc1d56d7a0..fd2ac2db0a9 100644
--- a/lib/constraints/group_url_constrainer.rb
+++ b/lib/constraints/group_url_constrainer.rb
@@ -2,7 +2,7 @@ class GroupUrlConstrainer
   def matches?(request)
     full_path = request.params[:group_id] || request.params[:id]
 
-    return false unless DynamicPathValidator.valid_group_path?(full_path)
+    return false unless NamespacePathValidator.valid_path?(full_path)
 
     Group.find_by_full_path(full_path, follow_redirects: request.get?).present?
   end
diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb
index 5bef29eb1da..e90ecb5ec69 100644
--- a/lib/constraints/project_url_constrainer.rb
+++ b/lib/constraints/project_url_constrainer.rb
@@ -4,7 +4,7 @@ class ProjectUrlConstrainer
     project_path = request.params[:project_id] || request.params[:id]
     full_path = [namespace_path, project_path].join('/')
 
-    return false unless DynamicPathValidator.valid_project_path?(full_path)
+    return false unless ProjectPathValidator.valid_path?(full_path)
 
     # We intentionally allow SELECT(*) here so result of this query can be used
     # as cache for further Project.find_by_full_path calls within request
diff --git a/lib/constraints/user_url_constrainer.rb b/lib/constraints/user_url_constrainer.rb
index d16ae7f3f40..b7633aa7cbb 100644
--- a/lib/constraints/user_url_constrainer.rb
+++ b/lib/constraints/user_url_constrainer.rb
@@ -2,7 +2,7 @@ class UserUrlConstrainer
   def matches?(request)
     full_path = request.params[:username]
 
-    return false unless DynamicPathValidator.valid_user_path?(full_path)
+    return false unless UserPathValidator.valid_path?(full_path)
 
     User.find_by_full_path(full_path, follow_redirects: request.get?).present?
   end
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index 47c2a422387..b4b3b00c84d 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -179,7 +179,7 @@ module Gitlab
         valid_username = ::Namespace.clean_path(username)
 
         uniquify = Uniquify.new
-        valid_username = uniquify.string(valid_username) { |s| !DynamicPathValidator.valid_user_path?(s) }
+        valid_username = uniquify.string(valid_username) { |s| !UserPathValidator.valid_path?(s) }
 
         name = auth_hash.name
         name = valid_username if name.strip.empty?
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index e8588975118..0e50909988b 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -276,6 +276,12 @@ describe Project do
 
         expect(project).to be_valid
       end
+
+      it 'allows a path ending in a period' do
+        project = build(:project, path: 'foo.')
+
+        expect(project).to be_valid
+      end
     end
   end
 
diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb
deleted file mode 100644
index 08e1c5a728a..00000000000
--- a/spec/validators/dynamic_path_validator_spec.rb
+++ /dev/null
@@ -1,97 +0,0 @@
-require 'spec_helper'
-
-describe DynamicPathValidator do
-  let(:validator) { described_class.new(attributes: [:path]) }
-
-  def expect_handles_invalid_utf8
-    expect { yield('\255invalid') }.to be_falsey
-  end
-
-  describe '.valid_user_path' do
-    it 'handles invalid utf8' do
-      expect(described_class.valid_user_path?("a\0weird\255path")).to be_falsey
-    end
-  end
-
-  describe '.valid_group_path' do
-    it 'handles invalid utf8' do
-      expect(described_class.valid_group_path?("a\0weird\255path")).to be_falsey
-    end
-  end
-
-  describe '.valid_project_path' do
-    it 'handles invalid utf8' do
-      expect(described_class.valid_project_path?("a\0weird\255path")).to be_falsey
-    end
-  end
-
-  describe '#path_valid_for_record?' do
-    context 'for project' do
-      it 'calls valid_project_path?' do
-        project = build(:project, path: 'activity')
-
-        expect(described_class).to receive(:valid_project_path?).with(project.full_path).and_call_original
-
-        expect(validator.path_valid_for_record?(project, 'activity')).to be_truthy
-      end
-    end
-
-    context 'for group' do
-      it 'calls valid_group_path?' do
-        group = build(:group, :nested, path: 'activity')
-
-        expect(described_class).to receive(:valid_group_path?).with(group.full_path).and_call_original
-
-        expect(validator.path_valid_for_record?(group, 'activity')).to be_falsey
-      end
-    end
-
-    context 'for user' do
-      it 'calls valid_user_path?' do
-        user = build(:user, username: 'activity')
-
-        expect(described_class).to receive(:valid_user_path?).with(user.full_path).and_call_original
-
-        expect(validator.path_valid_for_record?(user, 'activity')).to be_truthy
-      end
-    end
-
-    context 'for user namespace' do
-      it 'calls valid_user_path?' do
-        user = create(:user, username: 'activity')
-        namespace = user.namespace
-
-        expect(described_class).to receive(:valid_user_path?).with(namespace.full_path).and_call_original
-
-        expect(validator.path_valid_for_record?(namespace, 'activity')).to be_truthy
-      end
-    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::PathRegex.namespace_format_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
-
-    it 'updating to an invalid path is not allowed' do
-      project = create(:project)
-      project.path = 'update'
-
-      validator.validate_each(project, :path, 'update')
-
-      expect(project.errors[:path]).to include('update is a reserved name')
-    end
-  end
-end
diff --git a/spec/validators/namespace_path_validator_spec.rb b/spec/validators/namespace_path_validator_spec.rb
new file mode 100644
index 00000000000..61e2845f35f
--- /dev/null
+++ b/spec/validators/namespace_path_validator_spec.rb
@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+describe NamespacePathValidator do
+  let(:validator) { described_class.new(attributes: [:path]) }
+
+  describe '.valid_path?' do
+    it 'handles invalid utf8' do
+      expect(described_class.valid_path?("a\0weird\255path")).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::PathRegex.namespace_format_message)
+    end
+
+    it 'adds a message when the path is reserved when creating' do
+      group = build(:group, path: 'help')
+
+      validator.validate_each(group, :path, 'help')
+
+      expect(group.errors[:path]).to include('help is a reserved name')
+    end
+
+    it 'adds a message when the path is reserved when updating' do
+      group = create(:group)
+      group.path = 'help'
+
+      validator.validate_each(group, :path, 'help')
+
+      expect(group.errors[:path]).to include('help is a reserved name')
+    end
+  end
+end
diff --git a/spec/validators/project_path_validator_spec.rb b/spec/validators/project_path_validator_spec.rb
new file mode 100644
index 00000000000..8bb5e72dc22
--- /dev/null
+++ b/spec/validators/project_path_validator_spec.rb
@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+describe ProjectPathValidator do
+  let(:validator) { described_class.new(attributes: [:path]) }
+
+  describe '.valid_path?' do
+    it 'handles invalid utf8' do
+      expect(described_class.valid_path?("a\0weird\255path")).to be_falsey
+    end
+  end
+
+  describe '#validates_each' do
+    it 'adds a message when the path is not in the correct format' do
+      project = build(:project)
+
+      validator.validate_each(project, :path, "Path with spaces, and comma's!")
+
+      expect(project.errors[:path]).to include(Gitlab::PathRegex.project_path_format_message)
+    end
+
+    it 'adds a message when the path is reserved when creating' do
+      project = build(:project, path: 'blob')
+
+      validator.validate_each(project, :path, 'blob')
+
+      expect(project.errors[:path]).to include('blob is a reserved name')
+    end
+
+    it 'adds a message when the path is reserved when updating' do
+      project = create(:project)
+      project.path = 'blob'
+
+      validator.validate_each(project, :path, 'blob')
+
+      expect(project.errors[:path]).to include('blob is a reserved name')
+    end
+  end
+end
diff --git a/spec/validators/user_path_validator_spec.rb b/spec/validators/user_path_validator_spec.rb
new file mode 100644
index 00000000000..a46089cc24f
--- /dev/null
+++ b/spec/validators/user_path_validator_spec.rb
@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+describe UserPathValidator do
+  let(:validator) { described_class.new(attributes: [:username]) }
+
+  describe '.valid_path?' do
+    it 'handles invalid utf8' do
+      expect(described_class.valid_path?("a\0weird\255path")).to be_falsey
+    end
+  end
+
+  describe '#validates_each' do
+    it 'adds a message when the path is not in the correct format' do
+      user = build(:user)
+
+      validator.validate_each(user, :username, "Path with spaces, and comma's!")
+
+      expect(user.errors[:username]).to include(Gitlab::PathRegex.namespace_format_message)
+    end
+
+    it 'adds a message when the path is reserved when creating' do
+      user = build(:user, username: 'help')
+
+      validator.validate_each(user, :username, 'help')
+
+      expect(user.errors[:username]).to include('help is a reserved name')
+    end
+
+    it 'adds a message when the path is reserved when updating' do
+      user = create(:user)
+      user.username = 'help'
+
+      validator.validate_each(user, :username, 'help')
+
+      expect(user.errors[:username]).to include('help is a reserved name')
+    end
+  end
+end