diff options
author | Michael Kozono <mkozono@gmail.com> | 2017-05-18 16:23:05 -0700 |
---|---|---|
committer | Michael Kozono <mkozono@gmail.com> | 2017-05-19 09:13:27 -0700 |
commit | 49697bc8df613dfe8e88f5f7cd8eae57e26c786f (patch) | |
tree | da5888be64878d8b35d1727dc802d609960fd631 /spec | |
parent | f9785dcec34c4205732871523f95b9743db00965 (diff) | |
download | gitlab-ce-49697bc8df613dfe8e88f5f7cd8eae57e26c786f.tar.gz |
Refactor to more robust implementationfix-issue-32506
In order to avoid string manipulation or modify route params (to make them unambiguous for `url_for`), we are accepting a behavior change:
When being redirected to the canonical path for a group, if you requested a group show path starting with `/groups/…` then you’ll now be redirected to the group at root `/…`.
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/groups/milestones_controller_spec.rb | 135 | ||||
-rw-r--r-- | spec/controllers/groups_controller_spec.rb | 41 | ||||
-rw-r--r-- | spec/controllers/projects/labels_controller_spec.rb | 70 | ||||
-rw-r--r-- | spec/support/milestone_tabs_examples.rb | 2 |
4 files changed, 239 insertions, 9 deletions
diff --git a/spec/controllers/groups/milestones_controller_spec.rb b/spec/controllers/groups/milestones_controller_spec.rb index 7cf2996ffd0..f3263bc177d 100644 --- a/spec/controllers/groups/milestones_controller_spec.rb +++ b/spec/controllers/groups/milestones_controller_spec.rb @@ -21,7 +21,6 @@ describe Groups::MilestonesController do sign_in(user) group.add_owner(user) project.team << [user, :master] - controller.instance_variable_set(:@group, group) end it_behaves_like 'milestone tabs' @@ -29,7 +28,7 @@ describe Groups::MilestonesController do describe "#create" do it "creates group milestone with Chinese title" do post :create, - group_id: group.id, + group_id: group.to_param, milestone: { project_ids: [project.id, project2.id], title: title } expect(response).to redirect_to(group_milestone_path(group, title.to_slug.to_s, title: title)) @@ -37,9 +36,139 @@ describe Groups::MilestonesController do end it "redirects to new when there are no project ids" do - post :create, group_id: group.id, milestone: { title: title, project_ids: [""] } + post :create, group_id: group.to_param, milestone: { title: title, project_ids: [""] } expect(response).to render_template :new expect(assigns(:milestone).errors).not_to be_nil end end + + describe '#ensure_canonical_path' do + before do + sign_in(user) + end + + context 'for a GET request' do + context 'when requesting the canonical path' do + context 'non-show path' do + context 'with exactly matching casing' do + it 'does not redirect' do + get :index, group_id: group.to_param + + expect(response).not_to have_http_status(301) + end + end + + context 'with different casing' do + it 'redirects to the correct casing' do + get :index, group_id: group.to_param.upcase + + expect(response).to redirect_to(group_milestones_path(group.to_param)) + expect(controller).not_to set_flash[:notice] + end + end + end + + context 'show path' do + context 'with exactly matching casing' do + it 'does not redirect' do + get :show, group_id: group.to_param, id: title + + expect(response).not_to have_http_status(301) + end + end + + context 'with different casing' do + it 'redirects to the correct casing' do + get :show, group_id: group.to_param.upcase, id: title + + expect(response).to redirect_to(group_milestone_path(group.to_param, title)) + expect(controller).not_to set_flash[:notice] + end + end + end + end + + context 'when requesting a redirected path' do + let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } + + it 'redirects to the canonical path' do + get :merge_requests, group_id: redirect_route.path, id: title + + expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title)) + expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group)) + end + + context 'when the old group path is a substring of the scheme or host' do + let(:redirect_route) { group.redirect_routes.create(path: 'http') } + + it 'does not modify the requested host' do + get :merge_requests, group_id: redirect_route.path, id: title + + expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title)) + expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group)) + end + end + + context 'when the old group path is substring of groups' do + # I.e. /groups/oups should not become /grfoo/oups + let(:redirect_route) { group.redirect_routes.create(path: 'oups') } + + it 'does not modify the /groups part of the path' do + get :merge_requests, group_id: redirect_route.path, id: title + + expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title)) + expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group)) + end + end + + context 'when the old group path is substring of groups plus the new path' do + # I.e. /groups/oups/oup should not become /grfoos + let(:redirect_route) { group.redirect_routes.create(path: 'oups/oup') } + + it 'does not modify the /groups part of the path' do + get :merge_requests, group_id: redirect_route.path, id: title + + expect(response).to redirect_to(merge_requests_group_milestone_path(group.to_param, title)) + expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group)) + end + end + end + end + end + + context 'for a non-GET request' do + context 'when requesting the canonical path with different casing' do + it 'does not 404' do + post :create, + group_id: group.to_param, + milestone: { project_ids: [project.id, project2.id], title: title } + + expect(response).not_to have_http_status(404) + end + + it 'does not redirect to the correct casing' do + post :create, + group_id: group.to_param, + milestone: { project_ids: [project.id, project2.id], title: title } + + expect(response).not_to have_http_status(301) + end + end + + context 'when requesting a redirected path' do + let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } + + it 'returns not found' do + post :create, + group_id: redirect_route.path, + milestone: { project_ids: [project.id, project2.id], title: title } + + expect(response).to have_http_status(404) + end + end + end + + def group_moved_message(redirect_route, group) + "Group '#{redirect_route.path}' was moved to '#{group.full_path}'. Please update any links and bookmarks that may still have the old path." + end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 993654fddaa..4626f1ebc29 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -214,12 +214,43 @@ describe GroupsController do end context 'when requesting groups under the /groups path' do - context 'when requesting the canonical path with different casing' do - it 'redirects to the correct casing' do - get :issues, id: group.to_param.upcase + context 'when requesting the canonical path' do + context 'non-show path' do + context 'with exactly matching casing' do + it 'does not redirect' do + get :issues, id: group.to_param + + expect(response).not_to have_http_status(301) + end + end - expect(response).to redirect_to(issues_group_path(group.to_param)) - expect(controller).not_to set_flash[:notice] + context 'with different casing' do + it 'redirects to the correct casing' do + get :issues, id: group.to_param.upcase + + expect(response).to redirect_to(issues_group_path(group.to_param)) + expect(controller).not_to set_flash[:notice] + end + end + end + + context 'show path' do + context 'with exactly matching casing' do + it 'does not redirect' do + get :show, id: group.to_param + + expect(response).not_to have_http_status(301) + end + end + + context 'with different casing' do + it 'redirects to the correct casing at the root path' do + get :show, id: group.to_param.upcase + + expect(response).to redirect_to(group) + expect(controller).not_to set_flash[:notice] + end + end end end diff --git a/spec/controllers/projects/labels_controller_spec.rb b/spec/controllers/projects/labels_controller_spec.rb index 05999431d8f..130b0b744b5 100644 --- a/spec/controllers/projects/labels_controller_spec.rb +++ b/spec/controllers/projects/labels_controller_spec.rb @@ -157,4 +157,74 @@ describe Projects::LabelsController do end end end + + describe '#ensure_canonical_path' do + before do + sign_in(user) + end + + context 'for a GET request' do + context 'when requesting the canonical path' do + context 'non-show path' do + context 'with exactly matching casing' do + it 'does not redirect' do + get :index, namespace_id: project.namespace, project_id: project.to_param + + expect(response).not_to have_http_status(301) + end + end + + context 'with different casing' do + it 'redirects to the correct casing' do + get :index, namespace_id: project.namespace, project_id: project.to_param.upcase + + expect(response).to redirect_to(namespace_project_labels_path(project.namespace, project)) + expect(controller).not_to set_flash[:notice] + end + end + end + end + + context 'when requesting a redirected path' do + let!(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') } + + it 'redirects to the canonical path' do + get :index, namespace_id: project.namespace, project_id: project.to_param + 'old' + + expect(response).to redirect_to(namespace_project_labels_path(project.namespace, project)) + expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, project)) + end + end + end + end + + context 'for a non-GET request' do + context 'when requesting the canonical path with different casing' do + it 'does not 404' do + post :generate, namespace_id: project.namespace, project_id: project + + expect(response).not_to have_http_status(404) + end + + it 'does not redirect to the correct casing' do + post :generate, namespace_id: project.namespace, project_id: project + + expect(response).not_to have_http_status(301) + end + end + + context 'when requesting a redirected path' do + let!(:redirect_route) { project.redirect_routes.create(path: project.full_path + 'old') } + + it 'returns not found' do + post :generate, namespace_id: project.namespace, project_id: project.to_param + 'old' + + expect(response).to have_http_status(404) + end + end + end + + def project_moved_message(redirect_route, project) + "Project '#{redirect_route.path}' was moved to '#{project.full_path}'. Please update any links and bookmarks that may still have the old path." + end end diff --git a/spec/support/milestone_tabs_examples.rb b/spec/support/milestone_tabs_examples.rb index c69f8e11008..4ad8b0a16e1 100644 --- a/spec/support/milestone_tabs_examples.rb +++ b/spec/support/milestone_tabs_examples.rb @@ -1,7 +1,7 @@ shared_examples 'milestone tabs' do def go(path, extra_params = {}) params = if milestone.is_a?(GlobalMilestone) - { group_id: group.id, id: milestone.safe_title, title: milestone.title } + { group_id: group.to_param, id: milestone.safe_title, title: milestone.title } else { namespace_id: project.namespace.to_param, project_id: project, id: milestone.iid } end |