Validate User username only on Namespace, and bubble up appropriately

12 files changed, 23 insertions, 90 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index da56446b6ab..05c93d3cb17 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -151,9 +151,7 @@ class User < ActiveRecord::Base
   validates :projects_limit,
     presence: true,
     numericality: { greater_than_or_equal_to: 0, less_than_or_equal_to: Gitlab::Database::MAX_INT_VALUE }
-  validates :username,
-    user_path: true,
-    presence: true
+  validates :username, presence: true
 
   validates :namespace, presence: true
   validate :namespace_move_dir_allowed, if: :username_changed?
diff --git a/app/validators/abstract_path_validator.rb b/app/validators/abstract_path_validator.rb
index adbccb65a84..e43b66cbe3a 100644
--- a/app/validators/abstract_path_validator.rb
+++ b/app/validators/abstract_path_validator.rb
@@ -13,10 +13,6 @@ class AbstractPathValidator < ActiveModel::EachValidator
     raise NotImplementedError
   end
 
-  def self.full_path(record, value)
-    value
-  end
-
   def self.valid_path?(path)
     encode!(path)
     "#{path}/" =~ path_regex
@@ -28,7 +24,7 @@ class AbstractPathValidator < ActiveModel::EachValidator
       return
     end
 
-    full_path = self.class.full_path(record, value)
+    full_path = record.build_full_path
     return unless full_path
 
     unless self.class.valid_path?(full_path)
diff --git a/app/validators/namespace_path_validator.rb b/app/validators/namespace_path_validator.rb
index 4a0aa64ae0c..7b0ae4db5d4 100644
--- a/app/validators/namespace_path_validator.rb
+++ b/app/validators/namespace_path_validator.rb
@@ -12,8 +12,4 @@ class NamespacePathValidator < AbstractPathValidator
   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
index 829b596ad3c..424fd77a6a3 100644
--- a/app/validators/project_path_validator.rb
+++ b/app/validators/project_path_validator.rb
@@ -12,8 +12,4 @@ class ProjectPathValidator < AbstractPathValidator
   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
deleted file mode 100644
index adf02901802..00000000000
--- a/app/validators/user_path_validator.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-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/lib/constraints/user_url_constrainer.rb b/lib/constraints/user_url_constrainer.rb
index b7633aa7cbb..3b3ed1c6ddb 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 UserPathValidator.valid_path?(full_path)
+    return false unless NamespacePathValidator.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 e40a001d20c..a3e1c66c19f 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -178,7 +178,7 @@ module Gitlab
         valid_username = ::Namespace.clean_path(username)
 
         uniquify = Uniquify.new
-        valid_username = uniquify.string(valid_username) { |s| !UserPathValidator.valid_path?(s) }
+        valid_username = uniquify.string(valid_username) { |s| !NamespacePathValidator.valid_path?(s) }
 
         name = auth_hash.name
         name = valid_username if name.strip.empty?
diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb
index 7e5dfd33502..ef3f786686a 100644
--- a/lib/gitlab/path_regex.rb
+++ b/lib/gitlab/path_regex.rb
@@ -171,26 +171,14 @@ module Gitlab
       @project_git_route_regex ||= /#{project_route_regex}\.git/.freeze
     end
 
-    def root_namespace_path_regex
-      @root_namespace_path_regex ||= %r{\A#{root_namespace_route_regex}/\z}
-    end
-
     def full_namespace_path_regex
       @full_namespace_path_regex ||= %r{\A#{full_namespace_route_regex}/\z}
     end
 
-    def project_path_regex
-      @project_path_regex ||= %r{\A#{project_route_regex}/\z}
-    end
-
     def full_project_path_regex
       @full_project_path_regex ||= %r{\A#{full_namespace_route_regex}/#{project_route_regex}/\z}
     end
 
-    def full_namespace_format_regex
-      @namespace_format_regex ||= /A#{FULL_NAMESPACE_FORMAT_REGEX}\z/.freeze
-    end
-
     def namespace_format_regex
       @namespace_format_regex ||= /\A#{NAMESPACE_FORMAT_REGEX}\z/.freeze
     end
diff --git a/spec/lib/gitlab/path_regex_spec.rb b/spec/lib/gitlab/path_regex_spec.rb
index 85991c38363..a40330d853f 100644
--- a/spec/lib/gitlab/path_regex_spec.rb
+++ b/spec/lib/gitlab/path_regex_spec.rb
@@ -194,8 +194,8 @@ describe Gitlab::PathRegex do
     end
   end
 
-  describe '.root_namespace_path_regex' do
-    subject { described_class.root_namespace_path_regex }
+  describe '.root_namespace_route_regex' do
+    subject { %r{\A#{described_class.root_namespace_route_regex}/\z} }
 
     it 'rejects top level routes' do
       expect(subject).not_to match('admin/')
@@ -318,8 +318,8 @@ describe Gitlab::PathRegex do
     end
   end
 
-  describe '.project_path_regex' do
-    subject { described_class.project_path_regex }
+  describe '.project_route_regex' do
+    subject { %r{\A#{described_class.project_route_regex}/\z} }
 
     it 'accepts top level routes' do
       expect(subject).to match('admin/')
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index a52dc93befe..cb02d526a98 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -140,7 +140,19 @@ describe User do
           user = build(:user, username: username)
 
           expect(user).not_to be_valid
-          expect(user.errors.messages[:"namespace.route.path"].first).to eq('foo has been taken before. Please use another one')
+          expect(user.errors.full_messages).to eq(['Username has been taken before'])
+        end
+      end
+
+      context 'when the username is in use by another user' do
+        let(:username) { 'foo' }
+        let!(:other_user) { create(:user, username: username) }
+
+        it 'is invalid' do
+          user = build(:user, username: username)
+
+          expect(user).not_to be_valid
+          expect(user.errors.full_messages).to eq(['Username has already been taken'])
         end
       end
     end
@@ -2634,7 +2646,7 @@ describe User do
 
       it 'should raise an ActiveRecord::RecordInvalid exception' do
         user2 = build(:user, username: 'foo')
-        expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Namespace route path foo has been taken before/)
+        expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Username has been taken before/)
       end
     end
 
diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb
index bcc01b087f3..e1c873f8c1e 100644
--- a/spec/services/groups/transfer_service_spec.rb
+++ b/spec/services/groups/transfer_service_spec.rb
@@ -177,7 +177,7 @@ describe Groups::TransferService, :postgresql do
 
         it 'should add an error on group' do
           transfer_service.execute(new_parent_group)
-          expect(transfer_service.error).to eq('Transfer failed: Validation failed: Route path has already been taken, Route is invalid')
+          expect(transfer_service.error).to eq('Transfer failed: Validation failed: Path has already been taken')
         end
       end
 
diff --git a/spec/validators/user_path_validator_spec.rb b/spec/validators/user_path_validator_spec.rb
deleted file mode 100644
index a46089cc24f..00000000000
--- a/spec/validators/user_path_validator_spec.rb
+++ /dev/null
@@ -1,38 +0,0 @@
-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