Read about our upcoming Code of Conduct on this issue

Commit 7e00c1ac authored by Douwe Maan's avatar Douwe Maan
Browse files

Validate User username only on Namespace, and bubble up appropriately

parent 744ac38e166e
......@@ -151,9 +151,7 @@ def update_tracked_fields!(request)
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?
......
......@@ -13,10 +13,6 @@ 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
......@@ -28,7 +24,7 @@ def validate_each(record, attribute, value)
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)
......
......@@ -12,8 +12,4 @@ def self.format_regex
def self.format_error_message
Gitlab::PathRegex.namespace_format_message
end
def self.full_path(record, value)
record.build_full_path
end
end
......@@ -12,8 +12,4 @@ def self.format_regex
def self.format_error_message
Gitlab::PathRegex.project_path_format_message
end
def self.full_path(record, value)
record.build_full_path
end
end
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
......@@ -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
......
......@@ -178,7 +178,7 @@ def user_attributes
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?
......
......@@ -171,26 +171,14 @@ def project_git_route_regex
@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
......
......@@ -194,8 +194,8 @@ def failure_message(constant_name, migration_helper, missing_words: [], addition
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 @@ def failure_message(constant_name, migration_helper, missing_words: [], addition
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/')
......
......@@ -140,7 +140,19 @@
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 @@ def access_levels(groups)
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
......
......@@ -177,7 +177,7 @@
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
......
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
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment