diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-05-19 22:28:38 +0300 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2017-05-19 22:28:38 +0300 |
commit | 5d5e695473bf65d89f12f330374f7bb36548e876 (patch) | |
tree | 94a80beb5f1b89debab74aff79d9e0bf0b93ef40 /spec | |
parent | bdf62a19d2cdbdfe554eb73500fd5fe4744161b1 (diff) | |
parent | 49697bc8df613dfe8e88f5f7cd8eae57e26c786f (diff) |
Merge branch 'fix-issue-32506' into 'master'
Fix redirects modifying the host
Closes #32506
See merge request !11498
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/groups/milestones_controller_spec.rb | 135 | ||||
-rw-r--r-- | spec/controllers/groups_controller_spec.rb | 257 | ||||
-rw-r--r-- | spec/controllers/projects/labels_controller_spec.rb | 70 | ||||
-rw-r--r-- | spec/controllers/projects_controller_spec.rb | 199 | ||||
-rw-r--r-- | spec/controllers/users_controller_spec.rb | 253 | ||||
-rw-r--r-- | spec/support/milestone_tabs_examples.rb | 2 |
6 files changed, 608 insertions, 308 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 15dae3231ca..4626f1ebc29 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -84,26 +84,6 @@ describe GroupsController do expect(assigns(:issues)).to eq [issue_2, issue_1] end end - - context 'when requesting the canonical path 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 - - 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 :issues, id: redirect_route.path - - expect(response).to redirect_to(issues_group_path(group.to_param)) - expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group)) - end - end end describe 'GET #merge_requests' do @@ -129,26 +109,6 @@ describe GroupsController do expect(assigns(:merge_requests)).to eq [merge_request_2, merge_request_1] end end - - context 'when requesting the canonical path with different casing' do - it 'redirects to the correct casing' do - get :merge_requests, id: group.to_param.upcase - - expect(response).to redirect_to(merge_requests_group_path(group.to_param)) - expect(controller).not_to set_flash[:notice] - 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, id: redirect_route.path - - expect(response).to redirect_to(merge_requests_group_path(group.to_param)) - expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group)) - end - end end describe 'DELETE #destroy' do @@ -178,30 +138,6 @@ describe GroupsController do expect(response).to redirect_to(root_path) end - - context 'when requesting the canonical path with different casing' do - it 'does not 404' do - delete :destroy, id: group.to_param.upcase - - expect(response).not_to have_http_status(404) - end - - it 'does not redirect to the correct casing' do - delete :destroy, id: group.to_param.upcase - - expect(response).not_to redirect_to(group_path(group.to_param)) - end - end - - context 'when requesting a redirected path' do - let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } - - it 'returns not found' do - delete :destroy, id: redirect_route.path - - expect(response).to have_http_status(404) - end - end end end @@ -224,28 +160,197 @@ describe GroupsController do expect(assigns(:group).errors).not_to be_empty expect(assigns(:group).path).not_to eq('new_path') end + end + + describe '#ensure_canonical_path' do + before do + sign_in(user) + end + + context 'for a GET request' do + context 'when requesting groups at the root path' do + before do + allow(request).to receive(:original_fullpath).and_return("/#{group_full_path}") + get :show, id: group_full_path + end + + context 'when requesting the canonical path with different casing' do + let(:group_full_path) { group.to_param.upcase } + + it 'redirects to the correct casing' do + expect(response).to redirect_to(group) + expect(controller).not_to set_flash[:notice] + end + end + + context 'when requesting a redirected path' do + let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } + let(:group_full_path) { redirect_route.path } + + it 'redirects to the canonical path' do + expect(response).to redirect_to(group) + 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 + expect(response).to redirect_to(group) + 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 + expect(response).to redirect_to(group) + expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group)) + end + end + end + end + + context 'when requesting groups under the /groups path' 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 :issues, 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 :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 + + 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 :issues, id: redirect_route.path + + expect(response).to redirect_to(issues_group_path(group.to_param)) + 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 :issues, id: redirect_route.path + + expect(response).to redirect_to(issues_group_path(group.to_param)) + 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 :issues, id: redirect_route.path + + expect(response).to redirect_to(issues_group_path(group.to_param)) + 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 :issues, id: redirect_route.path + + expect(response).to redirect_to(issues_group_path(group.to_param)) + expect(controller).to set_flash[:notice].to(group_moved_message(redirect_route, group)) + end + end + end + end + end - context 'when requesting the canonical path with different casing' do - it 'does not 404' do - post :update, id: group.to_param.upcase, group: { path: 'new_path' } + context 'for a POST request' do + context 'when requesting the canonical path with different casing' do + it 'does not 404' do + post :update, id: group.to_param.upcase, group: { path: 'new_path' } + + expect(response).not_to have_http_status(404) + end + + it 'does not redirect to the correct casing' do + post :update, id: group.to_param.upcase, group: { path: 'new_path' } - expect(response).not_to have_http_status(404) + expect(response).not_to have_http_status(301) + end end - it 'does not redirect to the correct casing' do - post :update, id: group.to_param.upcase, group: { path: 'new_path' } + context 'when requesting a redirected path' do + let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } - expect(response).not_to redirect_to(group_path(group.to_param)) + it 'returns not found' do + post :update, id: redirect_route.path, group: { path: 'new_path' } + + expect(response).to have_http_status(404) + end end end - context 'when requesting a redirected path' do - let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } + context 'for a DELETE request' do + context 'when requesting the canonical path with different casing' do + it 'does not 404' do + delete :destroy, id: group.to_param.upcase + + expect(response).not_to have_http_status(404) + end + + it 'does not redirect to the correct casing' do + delete :destroy, id: group.to_param.upcase - it 'returns not found' do - post :update, id: redirect_route.path, group: { path: 'new_path' } + expect(response).not_to have_http_status(301) + end + end - expect(response).to have_http_status(404) + context 'when requesting a redirected path' do + let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } + + it 'returns not found' do + delete :destroy, id: redirect_route.path + + expect(response).to have_http_status(404) + 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/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index e230944d52e..a8be6768a47 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -169,27 +169,6 @@ describe ProjectsController do end end - context "when requested with case sensitive namespace and project path" do - context "when there is a match with the same casing" do - it "loads the project" do - get :show, namespace_id: public_project.namespace, id: public_project - - expect(assigns(:project)).to eq(public_project) - expect(response).to have_http_status(200) - end - end - - context "when there is a match with different casing" do - it "redirects to the normalized path" do - get :show, namespace_id: public_project.namespace, id: public_project.path.upcase - - expect(assigns(:project)).to eq(public_project) - expect(response).to redirect_to("/#{public_project.full_path}") - expect(controller).not_to set_flash[:notice] - end - end - end - context "when the url contains .atom" do let(:public_project_with_dot_atom) { build(:empty_project, :public, name: 'my.atom', path: 'my.atom') } @@ -219,17 +198,6 @@ describe ProjectsController do expect(response).to redirect_to(namespace_project_path) end end - - context 'when requesting a redirected path' do - let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") } - - it 'redirects to the canonical path' do - get :show, namespace_id: 'foo', id: 'bar' - - expect(response).to redirect_to(public_project) - expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project)) - end - end end describe "#update" do @@ -256,34 +224,6 @@ describe ProjectsController do expect(assigns(:repository).path).to eq(project.repository.path) expect(response).to have_http_status(302) end - - context 'when requesting the canonical path' do - it "is case-insensitive" do - controller.instance_variable_set(:@project, project) - - put :update, - namespace_id: 'FOo', - id: 'baR', - project: project_params - - expect(project.repository.path).to include(new_path) - expect(assigns(:repository).path).to eq(project.repository.path) - expect(response).to have_http_status(302) - end - end - - context 'when requesting a redirected path' do - let!(:redirect_route) { project.redirect_routes.create!(path: "foo/bar") } - - it 'returns not found' do - put :update, - namespace_id: 'foo', - id: 'bar', - project: project_params - - expect(response).to have_http_status(404) - end - end end describe "#destroy" do @@ -319,31 +259,6 @@ describe ProjectsController do expect(merge_request.reload.state).to eq('closed') end end - - context 'when requesting the canonical path' do - it "is case-insensitive" do - controller.instance_variable_set(:@project, project) - sign_in(admin) - - orig_id = project.id - delete :destroy, namespace_id: project.namespace, id: project.path.upcase - - expect { Project.find(orig_id) }.to raise_error(ActiveRecord::RecordNotFound) - expect(response).to have_http_status(302) - expect(response).to redirect_to(dashboard_projects_path) - end - end - - context 'when requesting a redirected path' do - let!(:redirect_route) { project.redirect_routes.create!(path: "foo/bar") } - - it 'returns not found' do - sign_in(admin) - delete :destroy, namespace_id: 'foo', id: 'bar' - - expect(response).to have_http_status(404) - end - end end describe 'PUT #new_issue_address' do @@ -465,17 +380,6 @@ describe ProjectsController do expect(parsed_body["Tags"]).to include("v1.0.0") expect(parsed_body["Commits"]).to include("123456") end - - context 'when requesting a redirected path' do - let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") } - - it 'redirects to the canonical path' do - get :refs, namespace_id: 'foo', id: 'bar' - - expect(response).to redirect_to(refs_namespace_project_path(namespace_id: public_project.namespace, id: public_project)) - expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project)) - end - end end describe 'POST #preview_markdown' do @@ -488,6 +392,109 @@ describe ProjectsController do 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 "with exactly matching casing" do + it "loads the project" do + get :show, namespace_id: public_project.namespace, id: public_project + + expect(assigns(:project)).to eq(public_project) + expect(response).to have_http_status(200) + end + end + + context "with different casing" do + it "redirects to the normalized path" do + get :show, namespace_id: public_project.namespace, id: public_project.path.upcase + + expect(assigns(:project)).to eq(public_project) + expect(response).to redirect_to("/#{public_project.full_path}") + expect(controller).not_to set_flash[:notice] + end + end + end + + context 'when requesting a redirected path' do + let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") } + + it 'redirects to the canonical path' do + get :show, namespace_id: 'foo', id: 'bar' + + expect(response).to redirect_to(public_project) + expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project)) + end + + it 'redirects to the canonical path (testing non-show action)' do + get :refs, namespace_id: 'foo', id: 'bar' + + expect(response).to redirect_to(refs_namespace_project_path(namespace_id: public_project.namespace, id: public_project)) + expect(controller).to set_flash[:notice].to(project_moved_message(redirect_route, public_project)) + end + end + end + + context 'for a POST request' do + context 'when requesting the canonical path with different casing' do + it 'does not 404' do + post :toggle_star, namespace_id: public_project.namespace, id: public_project.path.upcase + + expect(response).not_to have_http_status(404) + end + + it 'does not redirect to the correct casing' do + post :toggle_star, namespace_id: public_project.namespace, id: public_project.path.upcase + + expect(response).not_to have_http_status(301) + end + end + + context 'when requesting a redirected path' do + let!(:redirect_route) { public_project.redirect_routes.create!(path: "foo/bar") } + + it 'returns not found' do + post :toggle_star, namespace_id: 'foo', id: 'bar' + + expect(response).to have_http_status(404) + end + end + end + + context 'for a DELETE request' do + before do + sign_in(create(:admin)) + end + + context 'when requesting the canonical path with different casing' do + it 'does not 404' do + delete :destroy, namespace_id: project.namespace, id: project.path.upcase + + expect(response).not_to have_http_status(404) + end + + it 'does not redirect to the correct casing' do + delete :destroy, namespace_id: project.namespace, id: project.path.upcase + + 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: "foo/bar") } + + it 'returns not found' do + delete :destroy, namespace_id: 'foo', id: 'bar' + + expect(response).to have_http_status(404) + end + 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 diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 1d61719f1d0..d33e2ba1e53 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -53,40 +53,6 @@ describe UsersController do end end - context 'when requesting the canonical path' do - let(:user) { create(:user, username: 'CamelCaseUser') } - - before { sign_in(user) } - - context 'with exactly matching casing' do - it 'responds with success' do - get :show, username: user.username - - expect(response).to be_success - end - end - - context 'with different casing' do - it 'redirects to the correct casing' do - get :show, username: user.username.downcase - - expect(response).to redirect_to(user) - expect(controller).not_to set_flash[:notice] - end - end - end - - context 'when requesting a redirected path' do - let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') } - - it 'redirects to the canonical path' do - get :show, username: redirect_route.path - - expect(response).to redirect_to(user) - expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user)) - end - end - context 'when a user by that username does not exist' do context 'when logged out' do it 'redirects to login page' do @@ -131,40 +97,6 @@ describe UsersController do expect(assigns(:contributions_calendar).projects.count).to eq(2) end end - - context 'when requesting the canonical path' do - let(:user) { create(:user, username: 'CamelCaseUser') } - - before { sign_in(user) } - - context 'with exactly matching casing' do - it 'responds with success' do - get :calendar, username: user.username - - expect(response).to be_success - end - end - - context 'with different casing' do - it 'redirects to the correct casing' do - get :calendar, username: user.username.downcase - - expect(response).to redirect_to(user_calendar_path(user)) - expect(controller).not_to set_flash[:notice] - end - end - end - - context 'when requesting a redirected path' do - let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') } - - it 'redirects to the canonical path' do - get :calendar, username: redirect_route.path - - expect(response).to redirect_to(user_calendar_path(user)) - expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user)) - end - end end describe 'GET #calendar_activities' do @@ -187,38 +119,6 @@ describe UsersController do get :calendar_activities, username: user.username expect(response).to render_template('calendar_activities') end - - context 'when requesting the canonical path' do - let(:user) { create(:user, username: 'CamelCaseUser') } - - context 'with exactly matching casing' do - it 'responds with success' do - get :calendar_activities, username: user.username - - expect(response).to be_success - end - end - - context 'with different casing' do - it 'redirects to the correct casing' do - get :calendar_activities, username: user.username.downcase - - expect(response).to redirect_to(user_calendar_activities_path(user)) - expect(controller).not_to set_flash[:notice] - end - end - end - - context 'when requesting a redirected path' do - let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') } - - it 'redirects to the canonical path' do - get :calendar_activities, username: redirect_route.path - - expect(response).to redirect_to(user_calendar_activities_path(user)) - expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user)) - end - end end describe 'GET #snippets' do @@ -241,38 +141,6 @@ describe UsersController do expect(JSON.parse(response.body)).to have_key('html') end end - - context 'when requesting the canonical path' do - let(:user) { create(:user, username: 'CamelCaseUser') } - - context 'with exactly matching casing' do - it 'responds with success' do - get :snippets, username: user.username - - expect(response).to be_success - end - end - - context 'with different casing' do - it 'redirects to the correct casing' do - get :snippets, username: user.username.downcase - - expect(response).to redirect_to(user_snippets_path(user)) - expect(controller).not_to set_flash[:notice] - end - end - end - - context 'when requesting a redirected path' do - let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') } - - it 'redirects to the canonical path' do - get :snippets, username: redirect_route.path - - expect(response).to redirect_to(user_snippets_path(user)) - expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user)) - end - end end describe 'GET #exists' do @@ -321,6 +189,127 @@ describe UsersController do end end + describe '#ensure_canonical_path' do + before do + sign_in(user) + end + + context 'for a GET request' do + context 'when requesting users at the root path' do + context 'when requesting the canonical path' do + let(:user) { create(:user, username: 'CamelCaseUser') } + + context 'with exactly matching casing' do + it 'responds with success' do + get :show, username: user.username + + expect(response).to be_success + end + end + + context 'with different casing' do + it 'redirects to the correct casing' do + get :show, username: user.username.downcase + + expect(response).to redirect_to(user) + expect(controller).not_to set_flash[:notice] + end + end + end + + context 'when requesting a redirected path' do + let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-path') } + + it 'redirects to the canonical path' do + get :show, username: redirect_route.path + + expect(response).to redirect_to(user) + expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user)) + end + + context 'when the old path is a substring of the scheme or host' do + let(:redirect_route) { user.namespace.redirect_routes.create(path: 'http') } + + it 'does not modify the requested host' do + get :show, username: redirect_route.path + + expect(response).to redirect_to(user) + expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user)) + end + end + + context 'when the old path is substring of users' do + let(:redirect_route) { user.namespace.redirect_routes.create(path: 'ser') } + + it 'redirects to the canonical path' do + get :show, username: redirect_route.path + + expect(response).to redirect_to(user) + expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user)) + end + end + end + end + + context 'when requesting users under the /users path' do + context 'when requesting the canonical path' do + let(:user) { create(:user, username: 'CamelCaseUser') } + + context 'with exactly matching casing' do + it 'responds with success' do + get :projects, username: user.username + + expect(response).to be_success + end + end + + context 'with different casing' do + it 'redirects to the correct casing' do + get :projects, username: user.username.downcase + + expect(response).to redirect_to(user_projects_path(user)) + expect(controller).not_to set_flash[:notice] + end + end + end + + context 'when requesting a redirected path' do + let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-path') } + + it 'redirects to the canonical path' do + get :projects, username: redirect_route.path + + expect(response).to redirect_to(user_projects_path(user)) + expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user)) + end + + context 'when the old path is a substring of the scheme or host' do + let(:redirect_route) { user.namespace.redirect_routes.create(path: 'http') } + + it 'does not modify the requested host' do + get :projects, username: redirect_route.path + + expect(response).to redirect_to(user_projects_path(user)) + expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user)) + end + end + + context 'when the old path is substring of users' do + let(:redirect_route) { user.namespace.redirect_routes.create(path: 'ser') } + + # I.e. /users/ser should not become /ufoos/ser + it 'does not modify the /users part of the path' do + get :projects, username: redirect_route.path + + expect(response).to redirect_to(user_projects_path(user)) + expect(controller).to set_flash[:notice].to(user_moved_message(redirect_route, user)) + end + end + end + end + end + end + def user_moved_message(redirect_route, user) "User '#{redirect_route.path}' was moved to '#{user.full_path}'. Please update any links and bookmarks that may still have the old path." 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 |