Validate User username only on Namespace, and bubble up appropriately
This commit is contained in:
parent
75144b1e03
commit
a03d29da1d
|
@ -151,9 +151,7 @@ class User < ActiveRecord::Base
|
||||||
validates :projects_limit,
|
validates :projects_limit,
|
||||||
presence: true,
|
presence: true,
|
||||||
numericality: { greater_than_or_equal_to: 0, less_than_or_equal_to: Gitlab::Database::MAX_INT_VALUE }
|
numericality: { greater_than_or_equal_to: 0, less_than_or_equal_to: Gitlab::Database::MAX_INT_VALUE }
|
||||||
validates :username,
|
validates :username, presence: true
|
||||||
user_path: true,
|
|
||||||
presence: true
|
|
||||||
|
|
||||||
validates :namespace, presence: true
|
validates :namespace, presence: true
|
||||||
validate :namespace_move_dir_allowed, if: :username_changed?
|
validate :namespace_move_dir_allowed, if: :username_changed?
|
||||||
|
|
|
@ -13,10 +13,6 @@ class AbstractPathValidator < ActiveModel::EachValidator
|
||||||
raise NotImplementedError
|
raise NotImplementedError
|
||||||
end
|
end
|
||||||
|
|
||||||
def self.full_path(record, value)
|
|
||||||
value
|
|
||||||
end
|
|
||||||
|
|
||||||
def self.valid_path?(path)
|
def self.valid_path?(path)
|
||||||
encode!(path)
|
encode!(path)
|
||||||
"#{path}/" =~ path_regex
|
"#{path}/" =~ path_regex
|
||||||
|
@ -28,7 +24,7 @@ class AbstractPathValidator < ActiveModel::EachValidator
|
||||||
return
|
return
|
||||||
end
|
end
|
||||||
|
|
||||||
full_path = self.class.full_path(record, value)
|
full_path = record.build_full_path
|
||||||
return unless full_path
|
return unless full_path
|
||||||
|
|
||||||
unless self.class.valid_path?(full_path)
|
unless self.class.valid_path?(full_path)
|
||||||
|
|
|
@ -12,8 +12,4 @@ class NamespacePathValidator < AbstractPathValidator
|
||||||
def self.format_error_message
|
def self.format_error_message
|
||||||
Gitlab::PathRegex.namespace_format_message
|
Gitlab::PathRegex.namespace_format_message
|
||||||
end
|
end
|
||||||
|
|
||||||
def self.full_path(record, value)
|
|
||||||
record.build_full_path
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
|
|
|
@ -12,8 +12,4 @@ class ProjectPathValidator < AbstractPathValidator
|
||||||
def self.format_error_message
|
def self.format_error_message
|
||||||
Gitlab::PathRegex.project_path_format_message
|
Gitlab::PathRegex.project_path_format_message
|
||||||
end
|
end
|
||||||
|
|
||||||
def self.full_path(record, value)
|
|
||||||
record.build_full_path
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
|
|
|
@ -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
|
|
|
@ -2,7 +2,7 @@ class UserUrlConstrainer
|
||||||
def matches?(request)
|
def matches?(request)
|
||||||
full_path = request.params[:username]
|
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?
|
User.find_by_full_path(full_path, follow_redirects: request.get?).present?
|
||||||
end
|
end
|
||||||
|
|
|
@ -178,7 +178,7 @@ module Gitlab
|
||||||
valid_username = ::Namespace.clean_path(username)
|
valid_username = ::Namespace.clean_path(username)
|
||||||
|
|
||||||
uniquify = Uniquify.new
|
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 = auth_hash.name
|
||||||
name = valid_username if name.strip.empty?
|
name = valid_username if name.strip.empty?
|
||||||
|
|
|
@ -171,26 +171,14 @@ module Gitlab
|
||||||
@project_git_route_regex ||= /#{project_route_regex}\.git/.freeze
|
@project_git_route_regex ||= /#{project_route_regex}\.git/.freeze
|
||||||
end
|
end
|
||||||
|
|
||||||
def root_namespace_path_regex
|
|
||||||
@root_namespace_path_regex ||= %r{\A#{root_namespace_route_regex}/\z}
|
|
||||||
end
|
|
||||||
|
|
||||||
def full_namespace_path_regex
|
def full_namespace_path_regex
|
||||||
@full_namespace_path_regex ||= %r{\A#{full_namespace_route_regex}/\z}
|
@full_namespace_path_regex ||= %r{\A#{full_namespace_route_regex}/\z}
|
||||||
end
|
end
|
||||||
|
|
||||||
def project_path_regex
|
|
||||||
@project_path_regex ||= %r{\A#{project_route_regex}/\z}
|
|
||||||
end
|
|
||||||
|
|
||||||
def full_project_path_regex
|
def full_project_path_regex
|
||||||
@full_project_path_regex ||= %r{\A#{full_namespace_route_regex}/#{project_route_regex}/\z}
|
@full_project_path_regex ||= %r{\A#{full_namespace_route_regex}/#{project_route_regex}/\z}
|
||||||
end
|
end
|
||||||
|
|
||||||
def full_namespace_format_regex
|
|
||||||
@namespace_format_regex ||= /A#{FULL_NAMESPACE_FORMAT_REGEX}\z/.freeze
|
|
||||||
end
|
|
||||||
|
|
||||||
def namespace_format_regex
|
def namespace_format_regex
|
||||||
@namespace_format_regex ||= /\A#{NAMESPACE_FORMAT_REGEX}\z/.freeze
|
@namespace_format_regex ||= /\A#{NAMESPACE_FORMAT_REGEX}\z/.freeze
|
||||||
end
|
end
|
||||||
|
|
|
@ -194,8 +194,8 @@ describe Gitlab::PathRegex do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe '.root_namespace_path_regex' do
|
describe '.root_namespace_route_regex' do
|
||||||
subject { described_class.root_namespace_path_regex }
|
subject { %r{\A#{described_class.root_namespace_route_regex}/\z} }
|
||||||
|
|
||||||
it 'rejects top level routes' do
|
it 'rejects top level routes' do
|
||||||
expect(subject).not_to match('admin/')
|
expect(subject).not_to match('admin/')
|
||||||
|
@ -318,8 +318,8 @@ describe Gitlab::PathRegex do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe '.project_path_regex' do
|
describe '.project_route_regex' do
|
||||||
subject { described_class.project_path_regex }
|
subject { %r{\A#{described_class.project_route_regex}/\z} }
|
||||||
|
|
||||||
it 'accepts top level routes' do
|
it 'accepts top level routes' do
|
||||||
expect(subject).to match('admin/')
|
expect(subject).to match('admin/')
|
||||||
|
|
|
@ -140,7 +140,19 @@ describe User do
|
||||||
user = build(:user, username: username)
|
user = build(:user, username: username)
|
||||||
|
|
||||||
expect(user).not_to be_valid
|
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
|
end
|
||||||
end
|
end
|
||||||
|
@ -2634,7 +2646,7 @@ describe User do
|
||||||
|
|
||||||
it 'should raise an ActiveRecord::RecordInvalid exception' do
|
it 'should raise an ActiveRecord::RecordInvalid exception' do
|
||||||
user2 = build(:user, username: 'foo')
|
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
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -177,7 +177,7 @@ describe Groups::TransferService, :postgresql do
|
||||||
|
|
||||||
it 'should add an error on group' do
|
it 'should add an error on group' do
|
||||||
transfer_service.execute(new_parent_group)
|
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
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -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
|
|
Loading…
Reference in New Issue