diff options
-rw-r--r-- | changelogs/unreleased/bvl-free-paths.yml | 5 | ||||
-rw-r--r-- | config/routes/group.rb | 52 | ||||
-rw-r--r-- | lib/gitlab/path_regex.rb | 16 | ||||
-rw-r--r-- | lib/gitlab/routing.rb | 19 | ||||
-rw-r--r-- | spec/helpers/labels_helper_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/path_regex_spec.rb | 46 | ||||
-rw-r--r-- | spec/models/group_spec.rb | 6 | ||||
-rw-r--r-- | spec/routing/group_routing_spec.rb | 127 | ||||
-rw-r--r-- | spec/routing/routing_spec.rb | 30 | ||||
-rw-r--r-- | spec/support/legacy_path_redirect_shared_examples.rb | 13 |
10 files changed, 205 insertions, 111 deletions
diff --git a/changelogs/unreleased/bvl-free-paths.yml b/changelogs/unreleased/bvl-free-paths.yml new file mode 100644 index 00000000000..f15459cc788 --- /dev/null +++ b/changelogs/unreleased/bvl-free-paths.yml @@ -0,0 +1,5 @@ +--- +title: Free up some reserved group names +merge_request: 15052 +author: +type: other diff --git a/config/routes/group.rb b/config/routes/group.rb index f4d520a2518..db99e10bb9a 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -8,24 +8,33 @@ constraints(GroupUrlConstrainer.new) do scope(path: 'groups/*id', controller: :groups, constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ }) 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 + 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 + 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 - resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do - post :resend_invite, on: :member - delete :leave, on: :collection + namespace :settings do + resource :ci_cd, only: [:show], controller: 'ci_cd' + end + + resources :variables, only: [:index, :show, :update, :create, :destroy] + + resources :children, only: [:index] + + resources :labels, except: [:show] do + post :toggle_subscription, on: :member end - resource :avatar, only: [:destroy] resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :edit, :update, :new, :create] do member do get :merge_requests @@ -34,18 +43,11 @@ constraints(GroupUrlConstrainer.new) do end end - resources :labels, except: [:show] do - post :toggle_subscription, on: :member - end - - scope path: '-' do - namespace :settings do - resource :ci_cd, only: [:show], controller: 'ci_cd' - end - - resources :variables, only: [:index, :show, :update, :create, :destroy] + resource :avatar, only: [:destroy] - resources :children, only: [:index] + resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do + post :resend_invite, on: :member + delete :leave, on: :collection end end @@ -58,4 +60,12 @@ 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) + end end diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 22f8dd669d0..9a91f8bf96a 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -112,22 +112,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 ].freeze ILLEGAL_PROJECT_PATH_WORDS = PROJECT_WILDCARD_ROUTES diff --git a/lib/gitlab/routing.rb b/lib/gitlab/routing.rb index e57890f1143..910533076b0 100644 --- a/lib/gitlab/routing.rb +++ b/lib/gitlab/routing.rb @@ -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 diff --git a/spec/helpers/labels_helper_spec.rb b/spec/helpers/labels_helper_spec.rb index 36d6e495ed0..4ac4302adfd 100644 --- a/spec/helpers/labels_helper_spec.rb +++ b/spec/helpers/labels_helper_spec.rb @@ -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 diff --git a/spec/lib/gitlab/path_regex_spec.rb b/spec/lib/gitlab/path_regex_spec.rb index ee63c9338c5..0ae90069b7f 100644 --- a/spec/lib/gitlab/path_regex_spec.rb +++ b/spec/lib/gitlab/path_regex_spec.rb @@ -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 @@ -157,16 +152,7 @@ describe Gitlab::PathRegex do let(:paths_after_group_id) do group_routes.map do |route| route.gsub(STARTING_WITH_GROUP, '').split('/').first - end.uniq + ee_paths_after_group_id - end - - let(:ee_paths_after_group_id) do - %w(analytics - ldap - ldap_group_links - notification_setting - audit_events - pipeline_quota hooks) + end.uniq end describe 'TOP_LEVEL_ROUTES' do @@ -225,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 @@ -258,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 @@ -280,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 @@ -303,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 @@ -325,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 @@ -360,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 @@ -393,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 diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index c8caa11b8b0..d4052a64570 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -65,12 +65,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 diff --git a/spec/routing/group_routing_spec.rb b/spec/routing/group_routing_spec.rb new file mode 100644 index 00000000000..7a4c8304e62 --- /dev/null +++ b/spec/routing/group_routing_spec.rb @@ -0,0 +1,127 @@ +require 'spec_helper' + +describe "Groups", "routing" do + let(:group_path) { 'complex.group-namegit' } + let!(:group) { create(:group, path: group_path) } + + it "to #show" do + expect(get("/groups/#{group_path}")).to route_to('groups#show', id: group_path) + end + + 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) } + + 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 diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index 609481603af..32aa6e5ad52 100644 --- a/spec/routing/routing_spec.rb +++ b/spec/routing/routing_spec.rb @@ -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') diff --git a/spec/support/legacy_path_redirect_shared_examples.rb b/spec/support/legacy_path_redirect_shared_examples.rb new file mode 100644 index 00000000000..f300bdd48b1 --- /dev/null +++ b/spec/support/legacy_path_redirect_shared_examples.rb @@ -0,0 +1,13 @@ +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 |