From 58d1d6a5c7e0a45c9aa8a9d4d1be24dbdce5a08a Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 27 Oct 2017 11:29:51 +0200 Subject: Free up some group reserved words --- config/routes/group.rb | 57 +++++++++++++++-------------- lib/gitlab/path_regex.rb | 5 --- lib/gitlab/routing.rb | 10 +++++- spec/helpers/labels_helper_spec.rb | 2 +- spec/lib/gitlab/path_regex_spec.rb | 8 ++--- spec/models/group_spec.rb | 6 ---- spec/routing/group_routing_spec.rb | 73 +++++++++++++++++++++++++++++--------- 7 files changed, 100 insertions(+), 61 deletions(-) diff --git a/config/routes/group.rb b/config/routes/group.rb index 11124eaec51..db99e10bb9a 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -8,45 +8,46 @@ 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 - scope path: '-' do - namespace :settings do - resource :ci_cd, only: [:show], controller: 'ci_cd' - end + namespace :settings do + resource :ci_cd, only: [:show], controller: 'ci_cd' + end - resources :variables, only: [:index, :show, :update, :create, :destroy] + resources :variables, only: [:index, :show, :update, :create, :destroy] - resources :children, only: [:index] + resources :children, only: [:index] - resources :labels, except: [:show] do - post :toggle_subscription, on: :member - end + resources :labels, except: [:show] do + post :toggle_subscription, on: :member + end - resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :edit, :update, :new, :create] do - member do - get :merge_requests - get :participants - get :labels - end + resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :edit, :update, :new, :create] do + member do + get :merge_requests + get :participants + get :labels end + end - resource :avatar, only: [:destroy] + resource :avatar, only: [:destroy] - resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do - post :resend_invite, on: :member - delete :leave, on: :collection - end + resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do + post :resend_invite, on: :member + delete :leave, on: :collection end end @@ -63,6 +64,8 @@ constraints(GroupUrlConstrainer.new) do # 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) + 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 12e54013be1..6b8a4442ebd 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -112,18 +112,13 @@ module Gitlab # this would map to the activity-page of its parent. GROUP_ROUTES = %w[ - - activity analytics audit_events - edit hooks - issues ldap ldap_group_links - merge_requests 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 abfd413b7ea..defb47663cb 100644 --- a/lib/gitlab/routing.rb +++ b/lib/gitlab/routing.rb @@ -42,10 +42,18 @@ module Gitlab end def self.redirect_legacy_paths(router, *paths) + build_redirect_path = lambda do |request, _params, path| + # Only replace the last occurence of `path`. + path = request.fullpath.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| request.fullpath.gsub(%r{/#{path}/*}, "/-/#{path}/") }, + to: router.redirect { |params, request| build_redirect_path.call(request, params, path) }, as: "legacy_#{path}_redirect" 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{.*} + expect(link_to_label(label, subject: group)).to match %r{.*} end end diff --git a/spec/lib/gitlab/path_regex_spec.rb b/spec/lib/gitlab/path_regex_spec.rb index c76084114cb..98fcc84c416 100644 --- a/spec/lib/gitlab/path_regex_spec.rb +++ b/spec/lib/gitlab/path_regex_spec.rb @@ -297,7 +297,7 @@ describe Gitlab::PathRegex do end it 'rejects group routes' do - expect(subject).not_to match('root/activity/') + expect(subject).not_to match('root/analytics/') end end @@ -317,7 +317,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/analytics/more/') end end end @@ -350,7 +350,7 @@ describe Gitlab::PathRegex do end it 'accepts group routes' do - expect(subject).to match('activity/') + expect(subject).to match('analytics/') end it 'is not case sensitive' do @@ -381,7 +381,7 @@ describe Gitlab::PathRegex do end it 'accepts group routes' do - expect(subject).to match('root/activity/') + 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 index b3e55f5e3c9..3e56d34bc9d 100644 --- a/spec/routing/group_routing_spec.rb +++ b/spec/routing/group_routing_spec.rb @@ -18,37 +18,36 @@ describe "Groups", "routing" do end it "to #activity" do - expect(get("/groups/#{group_path}/activity")).to route_to('groups#activity', id: group_path) + 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) + 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 - describe 'legacy redirection' do - shared_examples 'canonical groups route' do |path| - it "#{path} routes to the correct controller" do - expect(get("/groups/#{group_path}/-/#{path}")) - .to route_to(group_id: group_path, - controller: "groups/#{path}", - action: 'index') - end + it "to #labels" do + expect(get("/groups/#{group_path}/-/labels")).to route_to('groups/labels#index', group_id: group_path) + end - it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/#{path}", "/groups/complex.group-namegit/-/#{path}/" do - let(:resource) { create(:group, parent: group, path: path) } - end - 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 'canonical groups route', 'labels' + 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 'canonical groups route', 'group_members' + 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 @@ -61,7 +60,9 @@ describe "Groups", "routing" do end describe 'milestones' do - it_behaves_like 'canonical groups route', 'milestones' + 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 @@ -74,5 +75,43 @@ describe "Groups", "routing" do 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 -- cgit v1.2.1