Merge branch 'dm-route-path-validation' into 'master'

Validate user, group and project paths consistently, and only once

See merge request gitlab-org/gitlab-ce!16903

app/models/concerns/routable.rb

@@ -7,11 +7,12 @@ module Routable
     has_one :route, as: :source, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
     has_many :redirect_routes, as: :source, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
-    validates_associated :route
     validates :route, presence: true
 
     scope :with_route, -> { includes(:route) }
 
+    after_validation :set_path_errors
+
     before_validation do
       if full_path_changed? || full_name_changed?
         prepare_route
@@ -125,6 +126,11 @@ module Routable
 
   private
 
+  def set_path_errors
+    route_path_errors = self.errors.delete(:"route.path")
+    self.errors[:path].concat(route_path_errors) if route_path_errors
+  end
+
   def uncached_full_path
     if route && route.path.present?
       @full_path ||= route.path # rubocop:disable Gitlab/ModuleWithInstanceVariables

app/models/namespace.rb

@@ -32,7 +32,6 @@ class Namespace < ActiveRecord::Base
   validates :owner, presence: true, unless: ->(n) { n.type == "Group" }
   validates :name,
     presence: true,
-    uniqueness: { scope: :parent_id },
     length: { maximum: 255 },
     namespace_name: true
 

app/models/project.rb

@@ -245,8 +245,7 @@ class Project < ActiveRecord::Base
   validates :path,
     presence: true,
     project_path: true,
-    length: { maximum: 255 },
-    uniqueness: { scope: :namespace_id }
+    length: { maximum: 255 }
 
   validates :namespace, presence: true
   validates :name, uniqueness: { scope: :namespace_id }

app/models/route.rb

@@ -75,7 +75,7 @@ class Route < ActiveRecord::Base
   def ensure_permanent_paths
     return if path.nil?
 
-    errors.add(:path, "#{path} has been taken before. Please use another one") if conflicting_redirect_exists?
+    errors.add(:path, "has been taken before") if conflicting_redirect_exists?
   end
 
   def conflicting_redirect_exists?

app/models/user.rb

@@ -151,12 +151,9 @@ 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,
-    uniqueness: { case_sensitive: false }
+  validates :username, presence: true
 
-  validate :namespace_uniq, if: :username_changed?
+  validates :namespace, presence: true
   validate :namespace_move_dir_allowed, if: :username_changed?
 
   validate :unique_email, if: :email_changed?
@@ -172,6 +169,7 @@ class User < ActiveRecord::Base
   before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
   before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record? }
   before_validation :ensure_namespace_correct
+  after_validation :set_username_errors
   after_update :username_changed_hook, if: :username_changed?
   after_destroy :post_destroy_hook
   after_destroy :remove_key_cache
@@ -505,17 +503,6 @@ class User < ActiveRecord::Base
     end
   end
 
-  def namespace_uniq
-    # Return early if username already failed the first uniqueness validation
-    return if errors.key?(:username) &&
-        errors[:username].include?('has already been taken')
-
-    existing_namespace = Namespace.by_path(username)
-    if existing_namespace && existing_namespace != namespace
-      errors.add(:username, 'has already been taken')
-    end
-  end
-
   def namespace_move_dir_allowed
     if namespace&.any_project_has_container_registry_tags?
       errors.add(:username, 'cannot be changed if a personal project has container registry tags.')
@@ -891,6 +878,11 @@ class User < ActiveRecord::Base
     end
   end
 
+  def set_username_errors
+    namespace_path_errors = self.errors.delete(:"namespace.path")
+    self.errors[:username].concat(namespace_path_errors) if namespace_path_errors
+  end
+
   def username_changed_hook
     system_hook_service.execute_hooks_for(self, :rename)
   end

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)

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

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

app/validators/user_path_validator.rb

-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

changelogs/unreleased/dm-route-path-validation.yml

+---
+title: Validate user, group and project paths consistently, and only once
+merge_request:
+author:
+type: fixed

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

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?

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

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/')

spec/models/concerns/routable_spec.rb

@@ -39,7 +39,7 @@ describe Group, 'Routable' do
       create(:group, parent: group, path: 'xyz')
       duplicate = build(:project, namespace: group, path: 'xyz')
 
-      expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Route path has already been taken, Route is invalid')
+      expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Path has already been taken')
     end
   end
 

spec/models/group_spec.rb

@@ -41,7 +41,6 @@ describe Group do
 
   describe 'validations' do
     it { is_expected.to validate_presence_of :name }
-    it { is_expected.to validate_uniqueness_of(:name).scoped_to(:parent_id) }
     it { is_expected.to validate_presence_of :path }
     it { is_expected.not_to validate_presence_of :owner }
     it { is_expected.to validate_presence_of :two_factor_grace_period }

spec/models/namespace_spec.rb

@@ -15,7 +15,6 @@ describe Namespace do
 
   describe 'validations' do
     it { is_expected.to validate_presence_of(:name) }
-    it { is_expected.to validate_uniqueness_of(:name).scoped_to(:parent_id) }
     it { is_expected.to validate_length_of(:name).is_at_most(255) }
     it { is_expected.to validate_length_of(:description).is_at_most(255) }
     it { is_expected.to validate_presence_of(:path) }

spec/models/project_spec.rb

@@ -129,7 +129,6 @@ describe Project do
     it { is_expected.to validate_length_of(:name).is_at_most(255) }
 
     it { is_expected.to validate_presence_of(:path) }
-    it { is_expected.to validate_uniqueness_of(:path).scoped_to(:namespace_id) }
     it { is_expected.to validate_length_of(:path).is_at_most(255) }
 
     it { is_expected.to validate_length_of(:description).is_at_most(2000) }

spec/models/route_spec.rb

@@ -27,7 +27,7 @@ describe Route do
             redirect.save!(validate: false)
 
             expect(new_route.valid?).to be_falsey
-            expect(new_route.errors.first[1]).to eq('foo has been taken before. Please use another one')
+            expect(new_route.errors.first[1]).to eq('has been taken before')
           end
         end
 
@@ -49,7 +49,7 @@ describe Route do
             redirect.save!(validate: false)
 
             expect(route.valid?).to be_falsey
-            expect(route.errors.first[1]).to eq('foo has been taken before. Please use another one')
+            expect(route.errors.first[1]).to eq('has been taken before')
           end
         end
 
@@ -368,7 +368,7 @@ describe Route do
 
         group2.path = 'foo'
         group2.valid?
-        expect(group2.errors["route.path"].first).to eq('foo has been taken before. Please use another one')
+        expect(group2.errors[:path]).to eq(['has been taken before'])
       end
     end
 

spec/models/user_spec.rb

@@ -116,12 +116,6 @@ describe User do
         expect(user).to be_valid
       end
 
-      it 'validates uniqueness' do
-        user = build(:user)
-
-        expect(user).to validate_uniqueness_of(:username).case_insensitive
-      end
-
       context 'when username is changed' do
         let(:user) { build_stubbed(:user, username: 'old_path', namespace: build_stubbed(:namespace)) }
 
@@ -146,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
@@ -2287,17 +2293,17 @@ describe User do
           end
 
           context 'when there is a validation error (namespace name taken) while updating namespace' do
-            let!(:conflicting_namespace) { create(:group, name: new_username, path: 'quz') }
+            let!(:conflicting_namespace) { create(:group, path: new_username) }
 
             it 'causes the user save to fail' do
               expect(user.update_attributes(username: new_username)).to be_falsey
-              expect(user.namespace.errors.messages[:name].first).to eq('has already been taken')
+              expect(user.namespace.errors.messages[:path].first).to eq('has already been taken')
             end
 
             it 'adds the namespace errors to the user' do
               user.update_attributes(username: new_username)
 
-              expect(user.errors.full_messages.first).to eq('Namespace name has already been taken')
+              expect(user.errors.full_messages.first).to eq('Username has already been taken')
             end
           end
         end
@@ -2640,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
 

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
 

spec/services/users/update_service_spec.rb

@@ -21,13 +21,13 @@ describe Users::UpdateService do
     end
 
     it 'includes namespace error messages' do
-      create(:group, name: 'taken', path: 'something_else')
+      create(:group, path: 'taken')
       result = {}
       expect do
         result = update_user(user, { username: 'taken' })
       end.not_to change { user.reload.username }
       expect(result[:status]).to eq(:error)
-      expect(result[:message]).to eq('Namespace name has already been taken')
+      expect(result[:message]).to eq('Username has already been taken')
     end
 
     def update_user(user, opts)

spec/validators/user_path_validator_spec.rb

-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