Commit 613b1c39 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Free up reserved words that were used under the `groups` route

Free up `labels` as a group name

Free up `avatar`, `group_members` and `milestones` as paths

Free up some group reserved words

Update failure message when finding new routes in `PathRegex`

Check redirecting with a querystring

Remove EE-specific group paths

redirect the EE specific routes
parent 032a2e36
---
title: Free up some reserved group names
merge_request: 15052
author:
type: other
......@@ -8,16 +8,19 @@ constraints(GroupUrlConstrainer.new) do
scope(path: 'groups/*id',
controller: :groups,
constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ }) do
scope(path: '-') do
get :edit, as: :edit_group
get :issues, as: :issues_group
get :merge_requests, as: :merge_requests_group
get :projects, as: :projects_group
get :activity, as: :activity_group
get :subgroups, as: :subgroups_group
end
get '/', action: :show, as: :group_canonical
end
scope(path: 'groups/*group_id',
scope(path: 'groups/*group_id/-',
module: :groups,
as: :group,
constraints: { group_id: Gitlab::PathRegex.full_namespace_route_regex }) do
......@@ -69,7 +72,6 @@ constraints(GroupUrlConstrainer.new) do
post :toggle_subscription, on: :member
end
scope path: '-' do
namespace :settings do
resource :ci_cd, only: [:show], controller: 'ci_cd'
end
......@@ -88,17 +90,6 @@ constraints(GroupUrlConstrainer.new) do
end
end
## EE-specific
legacy_ee_group_boards_redirect = redirect do |params, request|
path = "/groups/#{params[:group_id]}/-/boards"
path << "/#{params[:extra_params]}" if params[:extra_params].present?
path << "?#{request.query_string}" if request.query_string.present?
path
end
get 'boards(/*extra_params)', as: :legacy_ee_group_boards_redirect, to: legacy_ee_group_boards_redirect
end
scope(path: '*id',
as: :group,
constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ },
......@@ -108,4 +99,18 @@ constraints(GroupUrlConstrainer.new) do
put '/', action: :update
delete '/', action: :destroy
end
# Legacy paths should be defined last, so they would be ignored if routes with
# one of the previously reserved words exist.
scope(path: 'groups/*group_id') do
Gitlab::Routing.redirect_legacy_paths(self, :labels, :milestones, :group_members,
:edit, :issues, :merge_requests, :projects,
:activity)
## EE-specific
Gitlab::Routing.redirect_legacy_paths(self, :analytics, :ldap, :ldap_group_links,
:notification_setting, :audit_events,
:pipeline_quota, :hooks, :boards)
## EE-specific
end
end
......@@ -114,23 +114,6 @@ module Gitlab
# this would map to the activity-page of its parent.
GROUP_ROUTES = %w[
-
activity
analytics
audit_events
avatar
edit
group_members
hooks
issues
labels
ldap
ldap_group_links
merge_requests
milestones
notification_setting
pipeline_quota
projects
subgroups
].freeze
ILLEGAL_PROJECT_PATH_WORDS = PROJECT_WILDCARD_ROUTES
......
......@@ -40,5 +40,24 @@ module Gitlab
def self.url_helpers
@url_helpers ||= Gitlab::Application.routes.url_helpers
end
def self.redirect_legacy_paths(router, *paths)
build_redirect_path = lambda do |request, _params, path|
# Only replace the last occurence of `path`.
#
# `request.fullpath` includes the querystring
path = request.path.sub(%r{/#{path}/*(?!.*#{path})}, "/-/#{path}/")
path << "?#{request.query_string}" if request.query_string.present?
path
end
paths.each do |path|
router.match "/#{path}(/*rest)",
via: [:get, :post, :patch, :delete],
to: router.redirect { |params, request| build_redirect_path.call(request, params, path) },
as: "legacy_#{path}_redirect"
end
end
end
end
require 'spec_helper'
describe 'Group routing', "routing" do
include RSpec::Rails::RoutingExampleGroup
describe 'subgroup "boards"' do
it 'shows group show page' do
allow(Group).to receive(:find_by_full_path).with('gitlabhq/boards', any_args).and_return(true)
expect(get('/groups/gitlabhq/boards')).to route_to('groups#show', id: 'gitlabhq/boards')
end
it 'shows boards index page' do
allow(Group).to receive(:find_by_full_path).with('gitlabhq', any_args).and_return(true)
expect(get('/groups/gitlabhq/-/boards')).to route_to('groups/boards#index', group_id: 'gitlabhq')
end
end
describe 'legacy redirection' do
%w(analytics
boards
ldap
ldap_group_links
notification_setting
audit_events
pipeline_quota hooks).each do |legacy_reserved_path|
describe legacy_reserved_path do
it_behaves_like 'redirecting a legacy path',
"/groups/complex.group-namegit/#{legacy_reserved_path}",
"/groups/complex.group-namegit/-/#{legacy_reserved_path}/" do
let!(:parent) { create(:group, path: 'complex.group-namegit') }
let(:resource) { create(:group, parent: parent, path: legacy_reserved_path) }
end
end
end
context 'multiple redirects' do
include RSpec::Rails::RequestExampleGroup
let!(:parent) { create(:group, path: 'complex.group-namegit') }
it 'follows multiple redirects' do
expect(get('/groups/complex.group-namegit/boards/issues'))
.to redirect_to('/groups/complex.group-namegit/-/boards/issues')
end
it 'redirects when the nested group does not exist' do
create(:group, path: 'boards', parent: parent)
expect(get('/groups/complex.group-namegit/boards/issues/'))
.to redirect_to('/groups/complex.group-namegit/boards/-/issues/')
end
it 'does not redirect when the nested group exists' do
boards_group = create(:group, path: 'boards', parent: parent)
create(:group, path: 'issues', parent: boards_group)
expect(get('/groups/complex.group-namegit/boards/issues'))
.to eq(200)
end
end
end
end
......@@ -24,7 +24,7 @@ describe LabelsHelper do
let(:group) { build(:group, name: 'bar') }
it 'links to group issues page' do
expect(link_to_label(label, subject: group)).to match %r{<a href="/groups/bar/issues\?label_name%5B%5D=#{label.name}">.*</a>}
expect(link_to_label(label, subject: group)).to match %r{<a href="/groups/bar/-/issues\?label_name%5B%5D=#{label.name}">.*</a>}
end
end
......
......@@ -45,21 +45,16 @@ describe Gitlab::PathRegex do
Found new routes that could cause conflicts with existing namespaced routes
for groups or projects.
Add <#{missing_words.join(', ')}> to `Gitlab::PathRegex::#{constant_name}
to make sure no projects or namespaces can be created with those paths.
To rename any existing records with those paths you can use the
`Gitlab::Database::RenameReservedpathsMigration::<VERSION>.#{migration_helper}`
migration helper.
Make sure to make a note of the renamed records in the release blog post.
Nest <#{missing_words.join(', ')}> in a route containing `-`, that way
we know there will be no conflicts with groups or projects created with those
paths.
MISSING
end
if additional_words.any?
message += <<-ADDITIONAL
Why are <#{additional_words.join(', ')}> in `#{constant_name}`?
Is <#{additional_words.join(', ')}> in `#{constant_name}` required?
If they are really required, update these specs to reflect that.
ADDITIONAL
......@@ -216,8 +211,6 @@ describe Gitlab::PathRegex do
it 'accepts group routes' do
expect(subject).to match('activity/')
expect(subject).to match('group_members/')
expect(subject).to match('labels/')
end
it 'is not case sensitive' do
......@@ -249,8 +242,6 @@ describe Gitlab::PathRegex do
it 'accepts group routes' do
expect(subject).to match('activity/')
expect(subject).to match('group_members/')
expect(subject).to match('labels/')
end
end
......@@ -271,8 +262,6 @@ describe Gitlab::PathRegex do
it 'accepts group routes' do
expect(subject).to match('activity/more/')
expect(subject).to match('group_members/more/')
expect(subject).to match('labels/more/')
end
end
end
......@@ -294,9 +283,7 @@ describe Gitlab::PathRegex do
end
it 'rejects group routes' do
expect(subject).not_to match('root/activity/')
expect(subject).not_to match('root/group_members/')
expect(subject).not_to match('root/labels/')
expect(subject).not_to match('root/-/')
end
end
......@@ -316,9 +303,7 @@ describe Gitlab::PathRegex do
end
it 'rejects group routes' do
expect(subject).not_to match('root/activity/more/')
expect(subject).not_to match('root/group_members/more/')
expect(subject).not_to match('root/labels/more/')
expect(subject).not_to match('root/-/')
end
end
end
......@@ -351,9 +336,7 @@ describe Gitlab::PathRegex do
end
it 'accepts group routes' do
expect(subject).to match('activity/')
expect(subject).to match('group_members/')
expect(subject).to match('labels/')
expect(subject).to match('analytics/')
end
it 'is not case sensitive' do
......@@ -384,9 +367,7 @@ describe Gitlab::PathRegex do
end
it 'accepts group routes' do
expect(subject).to match('root/activity/')
expect(subject).to match('root/group_members/')
expect(subject).to match('root/labels/')
expect(subject).to match('root/analytics/')
end
it 'is not case sensitive' do
......
......@@ -64,12 +64,6 @@ describe Group do
expect(group).not_to be_valid
end
it 'rejects reserved group paths' do
group = build(:group, path: 'activity', parent: create(:group))
expect(group).not_to be_valid
end
end
describe '#visibility_level_allowed_by_parent' do
......
......@@ -7,7 +7,7 @@ describe 'Deprecated boards paths' do
it 'redirects to boards page' do
get('/groups/gitlabhq/boards')
expect(response).to redirect_to(group_boards_path(group))
expect(response).to redirect_to('/groups/gitlabhq/-/boards/')
end
it 'redirects to the boards page with additional params' do
......@@ -29,7 +29,7 @@ describe 'Deprecated boards paths' do
end
it 'does not redirect to the boards page with additional params' do
get('/groups/gitlabhq/boards/issues')
get('/groups/gitlabhq/boards/-/issues')
expect(response).to have_gitlab_http_status(200)
end
......
require 'spec_helper'
describe 'Group routing' do
describe 'subgroup "boards"' do
it 'shows group show page' do
allow(Group).to receive(:find_by_full_path).with('gitlabhq/boards', any_args).and_return(true)
describe "Groups", "routing" do
let(:group_path) { 'complex.group-namegit' }
let!(:group) { create(:group, path: group_path) }
expect(get('/groups/gitlabhq/boards')).to route_to('groups#show', id: 'gitlabhq/boards')
it "to #show" do
expect(get("/groups/#{group_path}")).to route_to('groups#show', id: group_path)
end
it 'shows boards index page' do
allow(Group).to receive(:find_by_full_path).with('gitlabhq', any_args).and_return(true)
it "also supports nested groups" do
nested_group = create(:group, parent: group)
expect(get("/#{group_path}/#{nested_group.path}")).to route_to('groups#show', id: "#{group_path}/#{nested_group.path}")
end
it "also display group#show on the short path" do
expect(get("/#{group_path}")).to route_to('groups#show', id: group_path)
end
it "to #activity" do
expect(get("/groups/#{group_path}/-/activity")).to route_to('groups#activity', id: group_path)
end
it "to #issues" do
expect(get("/groups/#{group_path}/-/issues")).to route_to('groups#issues', id: group_path)
end
it "to #members" do
expect(get("/groups/#{group_path}/-/group_members")).to route_to('groups/group_members#index', group_id: group_path)
end
it "to #labels" do
expect(get("/groups/#{group_path}/-/labels")).to route_to('groups/labels#index', group_id: group_path)
end
it "to #milestones" do
expect(get("/groups/#{group_path}/-/milestones")).to route_to('groups/milestones#index', group_id: group_path)
end
describe 'legacy redirection' do
describe 'labels' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/labels", "/groups/complex.group-namegit/-/labels/" do
let(:resource) { create(:group, parent: group, path: 'labels') }
end
end
describe 'group_members' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/group_members", "/groups/complex.group-namegit/-/group_members/" do
let(:resource) { create(:group, parent: group, path: 'group_members') }
end
end
describe 'avatar' do
it 'routes to the avatars controller' do
expect(delete("/groups/#{group_path}/-/avatar"))
.to route_to(group_id: group_path,
controller: 'groups/avatars',
action: 'destroy')
end
end
describe 'milestones' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/milestones", "/groups/complex.group-namegit/-/milestones/" do
let(:resource) { create(:group, parent: group, path: 'milestones') }
end
context 'nested routes' do
include RSpec::Rails::RequestExampleGroup
let(:milestone) { create(:milestone, group: group) }
expect(get('/groups/gitlabhq/-/boards')).to route_to('groups/boards#index', group_id: 'gitlabhq')
it 'redirects the nested routes' do
request = get("/groups/#{group_path}/milestones/#{milestone.id}/merge_requests")
expect(request).to redirect_to("/groups/#{group_path}/-/milestones/#{milestone.id}/merge_requests")
end
end
context 'with a query string' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/milestones?hello=world", "/groups/complex.group-namegit/-/milestones/?hello=world" do
let(:resource) { create(:group, parent: group, path: 'milestones') }
end
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/milestones?milestones=/milestones", "/groups/complex.group-namegit/-/milestones/?milestones=/milestones" do
let(:resource) { create(:group, parent: group, path: 'milestones') }
end
end
end
describe 'edit' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/edit", "/groups/complex.group-namegit/-/edit/" do
let(:resource) do
pending('still rejected because of the wildcard reserved word')
create(:group, parent: group, path: 'edit')
end
end
end
describe 'issues' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/issues", "/groups/complex.group-namegit/-/issues/" do
let(:resource) { create(:group, parent: group, path: 'issues') }
end
end
describe 'merge_requests' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/merge_requests", "/groups/complex.group-namegit/-/merge_requests/" do
let(:resource) { create(:group, parent: group, path: 'merge_requests') }
end
end
describe 'projects' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/projects", "/groups/complex.group-namegit/-/projects/" do
let(:resource) { create(:group, parent: group, path: 'projects') }
end
end
describe 'activity' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/activity", "/groups/complex.group-namegit/-/activity/" do
let(:resource) { create(:group, parent: group, path: 'activity') }
end
it_behaves_like 'redirecting a legacy path', "/groups/activity/activity", "/groups/activity/-/activity/" do
let!(:parent) { create(:group, path: 'activity') }
let(:resource) { create(:group, parent: parent, path: 'activity') }
end
end
end
end
......@@ -278,36 +278,6 @@ describe "Authentication", "routing" do
end
end
describe "Groups", "routing" do
let(:name) { 'complex.group-namegit' }
let!(:group) { create(:group, name: name) }
it "to #show" do
expect(get("/groups/#{name}")).to route_to('groups#show', id: name)
end
it "also supports nested groups" do
nested_group = create(:group, parent: group)
expect(get("/#{name}/#{nested_group.name}")).to route_to('groups#show', id: "#{name}/#{nested_group.name}")
end
it "also display group#show on the short path" do
expect(get("/#{name}")).to route_to('groups#show', id: name)
end
it "to #activity" do
expect(get("/groups/#{name}/activity")).to route_to('groups#activity', id: name)
end
it "to #issues" do
expect(get("/groups/#{name}/issues")).to route_to('groups#issues', id: name)
end
it "to #members" do
expect(get("/groups/#{name}/group_members")).to route_to('groups/group_members#index', group_id: name)
end
end
describe HealthCheckController, 'routing' do
it 'to #index' do
expect(get('/health_check')).to route_to('health_check#index')
......
shared_examples 'redirecting a legacy path' do |source, target|
include RSpec::Rails::RequestExampleGroup
it "redirects #{source} to #{target} when the resource does not exist" do
expect(get(source)).to redirect_to(target)
end
it "does not redirect #{source} to #{target} when the resource exists" do
resource
expect(get(source)).not_to redirect_to(target)
end
end
......@@ -36,16 +36,6 @@ describe DynamicPathValidator do
end
end
context 'for group' do
it 'calls valid_group_path?' do
group = build(:group, :nested, path: 'activity')
expect(described_class).to receive(:valid_group_path?).with(group.full_path).and_call_original
expect(validator.path_valid_for_record?(group, 'activity')).to be_falsey
end
end
context 'for user' do
it 'calls valid_user_path?' do
user = build(:user, username: 'activity')
......
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