Commit 08b1bc34 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Reject group-routes as names of child namespaces

parent 1e14c3c8
...@@ -55,6 +55,7 @@ class DynamicPathValidator < ActiveModel::EachValidator ...@@ -55,6 +55,7 @@ class DynamicPathValidator < ActiveModel::EachValidator
snippets snippets
teams teams
u u
unicorn_test
unsubscribes unsubscribes
uploads uploads
users users
...@@ -92,7 +93,52 @@ class DynamicPathValidator < ActiveModel::EachValidator ...@@ -92,7 +93,52 @@ class DynamicPathValidator < ActiveModel::EachValidator
wikis wikis
]).freeze ]).freeze
STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES).freeze # These are all the paths that follow `/groups/*id/ or `/groups/*group_id`
# We need to reject these because we have a `/groups/*id` page that is the same
# as the `/*id`.
#
# If we would allow a subgroup to be created with the name `activity` then
# this group would not be accessible through `/groups/parent/activity` since
# this would map to the activity-page of it's parent.
GROUP_ROUTES = Set.new(%w[
activity
avatar
edit
group_members
issues
labels
merge_requests
milestones
projects
subgroups
])
CHILD_ROUTES = (WILDCARD_ROUTES | GROUP_ROUTES).freeze
def self.without_reserved_wildcard_paths_regex
@full_path_without_wildcard_regex ||= regex_excluding_child_paths(WILDCARD_ROUTES)
end
def self.without_reserved_child_paths_regex
@full_path_without_child_routes_regex ||= regex_excluding_child_paths(CHILD_ROUTES)
end
# This is used to validate a full path.
# It doesn't match paths
# - Starting with one of the top level words
# - Containing one of the child level words in the middle of a path
def self.regex_excluding_child_paths(child_routes)
reserved_top_level_words = Regexp.union(TOP_LEVEL_ROUTES.to_a)
not_starting_in_reserved_word = %r{^(/?)(?!(#{reserved_top_level_words})(/|$))}
reserved_child_level_words = Regexp.union(child_routes.to_a)
not_containing_reserved_child = %r{(?!(\S+)/(#{reserved_child_level_words})(/|$))}
@full_path_regex = %r{
#{not_starting_in_reserved_word}
#{not_containing_reserved_child}
#{Gitlab::Regex::FULL_NAMESPACE_REGEX_STR}}x
end
def self.valid?(path) def self.valid?(path)
path_segments = path.split('/') path_segments = path.split('/')
...@@ -102,41 +148,48 @@ class DynamicPathValidator < ActiveModel::EachValidator ...@@ -102,41 +148,48 @@ class DynamicPathValidator < ActiveModel::EachValidator
def self.reserved?(path) def self.reserved?(path)
path = path.to_s.downcase path = path.to_s.downcase
top_level, wildcard_part = path.split('/', 2) _project_parts, namespace_parts = path.reverse.split('/', 2).map(&:reverse)
includes_reserved_top_level?(top_level) || includes_reserved_wildcard?(wildcard_part) wildcard_reserved?(path) || any_reserved?(namespace_parts)
end end
def self.includes_reserved_wildcard?(path) def self.any_reserved?(path)
WILDCARD_ROUTES.any? do |reserved_word| return false unless path
contains_path_part?(path, reserved_word)
end
end
def self.includes_reserved_top_level?(path) path !~ without_reserved_child_paths_regex
TOP_LEVEL_ROUTES.any? do |reserved_route|
contains_path_part?(path, reserved_route)
end
end end
def self.contains_path_part?(path, part) def self.wildcard_reserved?(path)
path =~ %r{(/|\A)#{Regexp.quote(part)}(/|\z)} return false unless path
path !~ without_reserved_wildcard_paths_regex
end end
def self.follow_format?(value) def self.follow_format?(value)
value =~ Gitlab::Regex.namespace_regex value =~ Gitlab::Regex.namespace_regex
end end
delegate :reserved?, :follow_format?, to: :class delegate :reserved?,
:any_reserved?,
:follow_format?, to: :class
def valid_full_path?(record, value)
full_path = record.respond_to?(:full_path) ? record.full_path : value
case record
when Project || User
reserved?(full_path)
else
any_reserved?(full_path)
end
end
def validate_each(record, attribute, value) def validate_each(record, attribute, value)
unless follow_format?(value) unless follow_format?(value)
record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) record.errors.add(attribute, Gitlab::Regex.namespace_regex_message)
end end
full_path = record.respond_to?(:full_path) ? record.full_path : value if valid_full_path?(record, value)
if reserved?(full_path)
record.errors.add(attribute, "#{value} is a reserved name") record.errors.add(attribute, "#{value} is a reserved name")
end end
end end
......
...@@ -76,6 +76,12 @@ describe Group, models: true do ...@@ -76,6 +76,12 @@ describe Group, models: true do
expect(group).not_to be_valid expect(group).not_to be_valid
end end
it 'rejects reserved group paths' do
group = build(:group, path: 'activity', parent: create(:group))
expect(group).not_to be_valid
end
end end
end end
......
...@@ -273,6 +273,13 @@ describe Project, models: true do ...@@ -273,6 +273,13 @@ describe Project, models: true do
expect(project).not_to be_valid expect(project).not_to be_valid
end end
it 'allows a reserved group name' do
parent = create(:group)
project = build(:project, path: 'avatar', namespace: parent)
expect(project).to be_valid
end
end end
end end
......
...@@ -97,6 +97,18 @@ describe User, models: true do ...@@ -97,6 +97,18 @@ describe User, models: true do
expect(user.errors.values).to eq [['dashboard is a reserved name']] expect(user.errors.values).to eq [['dashboard is a reserved name']]
end end
it 'allows child names' do
user = build(:user, username: 'avatar')
expect(user).to be_valid
end
it 'allows wildcard names' do
user = build(:user, username: 'blob')
expect(user).to be_valid
end
it 'validates uniqueness' do it 'validates uniqueness' do
expect(subject).to validate_uniqueness_of(:username).case_insensitive expect(subject).to validate_uniqueness_of(:username).case_insensitive
end end
......
...@@ -86,6 +86,16 @@ describe DynamicPathValidator do ...@@ -86,6 +86,16 @@ describe DynamicPathValidator do
end.uniq end.uniq
end end
STARTING_WITH_GROUP = %r{^/groups/\*(group_)?id/}
let(:group_routes) do
routes_without_format.select do |path|
path =~ STARTING_WITH_GROUP
end
end
let(:paths_after_group_id) do
group_routes.map do |route|
route.gsub(STARTING_WITH_GROUP, '').split('/').first
end.uniq end.uniq
end end
...@@ -95,6 +105,12 @@ describe DynamicPathValidator do ...@@ -95,6 +105,12 @@ describe DynamicPathValidator do
end end
end end
describe 'GROUP_ROUTES' do
it "don't contain a second wildcard" do
expect(described_class::GROUP_ROUTES).to include(*paths_after_group_id)
end
end
describe 'WILDCARD_ROUTES' do describe 'WILDCARD_ROUTES' do
it 'includes all paths that can be used after a namespace/project path' do it 'includes all paths that can be used after a namespace/project path' do
aggregate_failures do aggregate_failures do
...@@ -105,59 +121,69 @@ describe DynamicPathValidator do ...@@ -105,59 +121,69 @@ describe DynamicPathValidator do
end end
end end
describe '.contains_path_part?' do describe '.without_reserved_wildcard_paths_regex' do
it 'recognizes a path part' do subject { described_class.without_reserved_wildcard_paths_regex }
expect(described_class.contains_path_part?('user/some/path', 'user')).
to be_truthy
end
it 'recognizes a path ending in the path part' do it 'rejects paths starting with a reserved top level' do
expect(described_class.contains_path_part?('some/path/user', 'user')). expect(subject).not_to match('dashboard/hello/world')
to be_truthy expect(subject).not_to match('dashboard')
end end
it 'skips partial path matches' do it 'rejects paths containing a wildcard reserved word' do
expect(described_class.contains_path_part?('some/user1/path', 'user')). expect(subject).not_to match('hello/edit')
to be_falsy expect(subject).not_to match('hello/edit/in-the-middle')
expect(subject).not_to match('foo/bar1/refs/master/logs_tree')
end end
it 'can recognise combined paths' do it 'matches valid paths' do
expect(described_class.contains_path_part?('some/user/admin/path', 'user/admin')). expect(subject).to match('parent/child/project-path')
to be_truthy expect(subject).to match('/parent/child/project-path')
end end
end
it 'only recognizes full paths' do describe '.without_reserved_child_paths_regex' do
expect(described_class.contains_path_part?('frontend-fixtures', 's')). it 'rejects paths containing a child reserved word' do
to be_falsy subject = described_class.without_reserved_child_paths_regex
end
it 'handles regex chars gracefully' do expect(subject).not_to match('hello/group_members')
expect(described_class.contains_path_part?('frontend-fixtures', '-')). expect(subject).not_to match('hello/activity/in-the-middle')
to be_falsy expect(subject).not_to match('foo/bar1/refs/master/logs_tree')
end end
end end
describe ".valid?" do describe ".valid?" do
it 'is not case sensitive' do it 'is not case sensitive' do
expect(described_class.valid?("Users")).to be(false) expect(described_class.valid?("Users")).to be_falsey
end end
it "isn't valid when the top level is reserved" do it "isn't valid when the top level is reserved" do
test_path = 'u/should-be-a/reserved-word' test_path = 'u/should-be-a/reserved-word'
expect(described_class.valid?(test_path)).to be(false) expect(described_class.valid?(test_path)).to be_falsey
end end
it "isn't valid if any of the path segments is reserved" do it "isn't valid if any of the path segments is reserved" do
test_path = 'the-wildcard/wikis/is-not-allowed' test_path = 'the-wildcard/wikis/is-not-allowed'
expect(described_class.valid?(test_path)).to be(false) expect(described_class.valid?(test_path)).to be_falsey
end end
it "is valid if the path doesn't contain reserved words" do it "is valid if the path doesn't contain reserved words" do
test_path = 'there-are/no-wildcards/in-this-path' test_path = 'there-are/no-wildcards/in-this-path'
expect(described_class.valid?(test_path)).to be(true) expect(described_class.valid?(test_path)).to be_truthy
end
it 'allows allows a child path on the last spot' do
test_path = 'there/can-be-a/project-called/labels'
expect(described_class.valid?(test_path)).to be_truthy
end
it 'rejects a child path somewhere else' do
test_path = 'there/can-be-no/labels/group'
expect(described_class.valid?(test_path)).to be_falsey
end end
end 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