From a03d29da1dbbef3c202899cef3cd89b453a8b76f Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 31 Jan 2018 11:39:03 -0600 Subject: [PATCH] Validate User username only on Namespace, and bubble up appropriately --- app/models/user.rb | 4 +- app/validators/abstract_path_validator.rb | 6 +-- app/validators/namespace_path_validator.rb | 4 -- app/validators/project_path_validator.rb | 4 -- app/validators/user_path_validator.rb | 15 -------- lib/constraints/user_url_constrainer.rb | 2 +- lib/gitlab/o_auth/user.rb | 2 +- lib/gitlab/path_regex.rb | 12 ------ spec/lib/gitlab/path_regex_spec.rb | 8 ++-- spec/models/user_spec.rb | 16 +++++++- spec/services/groups/transfer_service_spec.rb | 2 +- spec/validators/user_path_validator_spec.rb | 38 ------------------- 12 files changed, 23 insertions(+), 90 deletions(-) delete mode 100644 app/validators/user_path_validator.rb delete mode 100644 spec/validators/user_path_validator_spec.rb 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