diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d2c13da6917..65a1f640a76 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -58,7 +58,7 @@ class ApplicationController < ActionController::Base if current_user not_found else - redirect_to new_user_session_path + authenticate_user! end end diff --git a/app/controllers/concerns/routable_actions.rb b/app/controllers/concerns/routable_actions.rb new file mode 100644 index 00000000000..d4ab6782444 --- /dev/null +++ b/app/controllers/concerns/routable_actions.rb @@ -0,0 +1,38 @@ +module RoutableActions + extend ActiveSupport::Concern + + def find_routable!(routable_klass, requested_full_path, extra_authorization_proc: nil) + routable = routable_klass.find_by_full_path(requested_full_path, follow_redirects: request.get?) + + if routable_authorized?(routable_klass, routable, extra_authorization_proc) + ensure_canonical_path(routable, requested_full_path) + routable + else + route_not_found + nil + end + end + + def routable_authorized?(routable_klass, routable, extra_authorization_proc) + action = :"read_#{routable_klass.to_s.underscore}" + return false unless can?(current_user, action, routable) + + if extra_authorization_proc + extra_authorization_proc.call(routable) + else + true + end + end + + def ensure_canonical_path(routable, requested_path) + return unless request.get? + + canonical_path = routable.full_path + if canonical_path != requested_path + if canonical_path.casecmp(requested_path) != 0 + flash[:notice] = "Project '#{requested_path}' was moved to '#{canonical_path}'. Please update any links and bookmarks that may still have the old path." + end + redirect_to request.original_url.sub(requested_path, canonical_path) + end + end +end diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index 29ffaeb19c1..afffb813b44 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -1,4 +1,6 @@ class Groups::ApplicationController < ApplicationController + include RoutableActions + layout 'group' skip_before_action :authenticate_user! @@ -7,29 +9,17 @@ class Groups::ApplicationController < ApplicationController private def group - unless @group - id = params[:group_id] || params[:id] - @group = Group.find_by_full_path(id) - @group_merge_requests = MergeRequestsFinder.new(current_user, group_id: @group.id).execute - - unless @group && can?(current_user, :read_group, @group) - @group = nil - - if current_user.nil? - authenticate_user! - else - render_404 - end - end - end - - @group + @group ||= find_routable!(Group, params[:group_id] || params[:id]) end def group_projects @projects ||= GroupProjectsFinder.new(group: group, current_user: current_user).execute end + def group_merge_requests + @group_merge_requests = MergeRequestsFinder.new(current_user, group_id: @group.id).execute + end + def authorize_admin_group! unless can?(current_user, :admin_group, group) return render_404 diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 593001e6396..46c3ff10694 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -12,8 +12,8 @@ class GroupsController < Groups::ApplicationController before_action :authorize_admin_group!, only: [:edit, :update, :destroy, :projects] before_action :authorize_create_group!, only: [:new, :create] - # Load group projects before_action :group_projects, only: [:projects, :activity, :issues, :merge_requests] + before_action :group_merge_requests, only: [:merge_requests] before_action :event_filter, only: [:activity] before_action :user_actions, only: [:show, :subgroups] diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 89f1128ec36..b4b0dfc3eb8 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -1,5 +1,8 @@ class Projects::ApplicationController < ApplicationController + include RoutableActions + skip_before_action :authenticate_user! + before_action :redirect_git_extension before_action :project before_action :repository layout 'project' @@ -8,40 +11,22 @@ class Projects::ApplicationController < ApplicationController private + def redirect_git_extension + # Redirect from + # localhost/group/project.git + # to + # localhost/group/project + # + redirect_to url_for(params.merge(format: nil)) if params[:format] == 'git' + end + def project - unless @project - namespace = params[:namespace_id] - id = params[:project_id] || params[:id] + return @project if @project - # Redirect from - # localhost/group/project.git - # to - # localhost/group/project - # - if params[:format] == 'git' - redirect_to request.original_url.gsub(/\.git\/?\Z/, '') - return - end + path = File.join(params[:namespace_id], params[:project_id] || params[:id]) + auth_proc = ->(project) { !project.pending_delete? } - project_path = "#{namespace}/#{id}" - @project = Project.find_by_full_path(project_path) - - if can?(current_user, :read_project, @project) && !@project.pending_delete? - if @project.path_with_namespace != project_path - redirect_to request.original_url.gsub(project_path, @project.path_with_namespace) - end - else - @project = nil - - if current_user.nil? - authenticate_user! - else - render_404 - end - end - end - - @project + @project = find_routable!(Project, path, extra_authorization_proc: auth_proc) end def repository diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a452bbba422..ca89ed221c6 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,7 +1,8 @@ class UsersController < ApplicationController + include RoutableActions + skip_before_action :authenticate_user! before_action :user, except: [:exists] - before_action :authorize_read_user!, only: [:show] def show respond_to do |format| @@ -91,12 +92,8 @@ class UsersController < ApplicationController private - def authorize_read_user! - render_404 unless can?(current_user, :read_user, user) - end - def user - @user ||= User.find_by_username!(params[:username]) + @user ||= find_routable!(User, params[:username]) end def contributed_projects diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index b28e05d0c28..e351dbb45dd 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -5,6 +5,7 @@ module Routable included do has_one :route, as: :source, autosave: true, dependent: :destroy + has_many :redirect_routes, as: :source, autosave: true, dependent: :destroy validates_associated :route validates :route, presence: true @@ -26,16 +27,31 @@ module Routable # Klass.find_by_full_path('gitlab-org/gitlab-ce') # # Returns a single object, or nil. - def find_by_full_path(path) + def find_by_full_path(path, follow_redirects: false) # On MySQL we want to ensure the ORDER BY uses a case-sensitive match so # any literal matches come first, for this we have to use "BINARY". # Without this there's still no guarantee in what order MySQL will return # rows. + # + # Why do we do this? + # + # Even though we have Rails validation on Route for unique paths + # (case-insensitive), there are old projects in our DB (and possibly + # clients' DBs) that have the same path with different cases. + # See https://gitlab.com/gitlab-org/gitlab-ce/issues/18603. Also note that + # our unique index is case-sensitive in Postgres. binary = Gitlab::Database.mysql? ? 'BINARY' : '' - order_sql = "(CASE WHEN #{binary} routes.path = #{connection.quote(path)} THEN 0 ELSE 1 END)" + found = where_full_path_in([path]).reorder(order_sql).take + return found if found - where_full_path_in([path]).reorder(order_sql).take + if follow_redirects + if Gitlab::Database.postgresql? + joins(:redirect_routes).find_by("LOWER(redirect_routes.path) = LOWER(?)", path) + else + joins(:redirect_routes).find_by(path: path) + end + end end # Builds a relation to find multiple objects by their full paths. diff --git a/app/models/redirect_route.rb b/app/models/redirect_route.rb new file mode 100644 index 00000000000..99812bcde53 --- /dev/null +++ b/app/models/redirect_route.rb @@ -0,0 +1,12 @@ +class RedirectRoute < ActiveRecord::Base + belongs_to :source, polymorphic: true + + validates :source, presence: true + + validates :path, + length: { within: 1..255 }, + presence: true, + uniqueness: { case_sensitive: false } + + scope :matching_path_and_descendants, -> (path) { where('redirect_routes.path = ? OR redirect_routes.path LIKE ?', path, "#{sanitize_sql_like(path)}/%") } +end diff --git a/app/models/route.rb b/app/models/route.rb index 4b3efab5c3c..12a7fa3d01b 100644 --- a/app/models/route.rb +++ b/app/models/route.rb @@ -8,29 +8,58 @@ class Route < ActiveRecord::Base presence: true, uniqueness: { case_sensitive: false } + after_create :delete_conflicting_redirects + after_update :delete_conflicting_redirects, if: :path_changed? + after_update :create_redirect_for_old_path after_update :rename_descendants scope :inside_path, -> (path) { where('routes.path LIKE ?', "#{sanitize_sql_like(path)}/%") } def rename_descendants - if path_changed? || name_changed? - descendants = self.class.inside_path(path_was) + return unless path_changed? || name_changed? - descendants.each do |route| - attributes = {} + descendant_routes = self.class.inside_path(path_was) - if path_changed? && route.path.present? - attributes[:path] = route.path.sub(path_was, path) - end + descendant_routes.each do |route| + attributes = {} - if name_changed? && name_was.present? && route.name.present? - attributes[:name] = route.name.sub(name_was, name) - end + if path_changed? && route.path.present? + attributes[:path] = route.path.sub(path_was, path) + end - # Note that update_columns skips validation and callbacks. - # We need this to avoid recursive call of rename_descendants method - route.update_columns(attributes) unless attributes.empty? + if name_changed? && name_was.present? && route.name.present? + attributes[:name] = route.name.sub(name_was, name) + end + + if attributes.present? + old_path = route.path + + # Callbacks must be run manually + route.update_columns(attributes) + + # We are not calling route.delete_conflicting_redirects here, in hopes + # of avoiding deadlocks. The parent (self, in this method) already + # called it, which deletes conflicts for all descendants. + route.create_redirect(old_path) if attributes[:path] end end end + + def delete_conflicting_redirects + conflicting_redirects.delete_all + end + + def conflicting_redirects + RedirectRoute.matching_path_and_descendants(path) + end + + def create_redirect(path) + RedirectRoute.create(source: source, path: path) + end + + private + + def create_redirect_for_old_path + create_redirect(path_was) if path_changed? + end end diff --git a/app/models/user.rb b/app/models/user.rb index 59f2be3ba9d..accaa91b805 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -337,6 +337,11 @@ class User < ActiveRecord::Base find_by(id: Key.unscoped.select(:user_id).where(id: key_id)) end + def find_by_full_path(path, follow_redirects: false) + namespace = Namespace.find_by_full_path(path, follow_redirects: follow_redirects) + namespace&.owner + end + def reference_prefix '@' end @@ -359,6 +364,10 @@ class User < ActiveRecord::Base end end + def full_path + username + end + def self.internal_attributes [:ghost] end diff --git a/changelogs/unreleased/17361-redirect-renamed-paths.yml b/changelogs/unreleased/17361-redirect-renamed-paths.yml new file mode 100644 index 00000000000..7a33c9fb3ec --- /dev/null +++ b/changelogs/unreleased/17361-redirect-renamed-paths.yml @@ -0,0 +1,4 @@ +--- +title: Redirect old links after renaming a user/group/project. +merge_request: 10370 +author: diff --git a/db/migrate/20170427215854_create_redirect_routes.rb b/db/migrate/20170427215854_create_redirect_routes.rb new file mode 100644 index 00000000000..2bf086b3e30 --- /dev/null +++ b/db/migrate/20170427215854_create_redirect_routes.rb @@ -0,0 +1,14 @@ +class CreateRedirectRoutes < ActiveRecord::Migration + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def change + create_table :redirect_routes do |t| + t.integer :source_id, null: false + t.string :source_type, null: false + t.string :path, null: false + + t.timestamps null: false + end + end +end diff --git a/db/migrate/20170503184421_add_index_to_redirect_routes.rb b/db/migrate/20170503184421_add_index_to_redirect_routes.rb new file mode 100644 index 00000000000..9062cf19a73 --- /dev/null +++ b/db/migrate/20170503184421_add_index_to_redirect_routes.rb @@ -0,0 +1,21 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIndexToRedirectRoutes < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index(:redirect_routes, :path, unique: true) + add_concurrent_index(:redirect_routes, [:source_type, :source_id]) + end + + def down + remove_concurrent_index(:redirect_routes, :path) if index_exists?(:redirect_routes, :path) + remove_concurrent_index(:redirect_routes, [:source_type, :source_id]) if index_exists?(:redirect_routes, [:source_type, :source_id]) + end +end diff --git a/db/migrate/20170503185032_index_redirect_routes_path_for_like.rb b/db/migrate/20170503185032_index_redirect_routes_path_for_like.rb new file mode 100644 index 00000000000..5b8b6c828be --- /dev/null +++ b/db/migrate/20170503185032_index_redirect_routes_path_for_like.rb @@ -0,0 +1,29 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class IndexRedirectRoutesPathForLike < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + INDEX_NAME = 'index_redirect_routes_on_path_text_pattern_ops' + + disable_ddl_transaction! + + def up + return unless Gitlab::Database.postgresql? + + unless index_exists?(:redirect_routes, :path, name: INDEX_NAME) + execute("CREATE INDEX CONCURRENTLY #{INDEX_NAME} ON redirect_routes (path varchar_pattern_ops);") + end + end + + def down + return unless Gitlab::Database.postgresql? + + if index_exists?(:redirect_routes, :path, name: INDEX_NAME) + execute("DROP INDEX CONCURRENTLY #{INDEX_NAME};") + end + end +end diff --git a/db/schema.rb b/db/schema.rb index ebe802a3087..61af066ed3b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1052,6 +1052,18 @@ ActiveRecord::Schema.define(version: 20170504102911) do add_index "protected_tags", ["project_id"], name: "index_protected_tags_on_project_id", using: :btree + create_table "redirect_routes", force: :cascade do |t| + t.integer "source_id", null: false + t.string "source_type", null: false + t.string "path", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path", unique: true, using: :btree + add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path_text_pattern_ops", using: :btree, opclasses: {"path"=>"varchar_pattern_ops"} + add_index "redirect_routes", ["source_type", "source_id"], name: "index_redirect_routes_on_source_type_and_source_id", using: :btree + create_table "releases", force: :cascade do |t| t.string "tag" t.text "description" diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb index 1501f64d537..5f379756c11 100644 --- a/lib/constraints/group_url_constrainer.rb +++ b/lib/constraints/group_url_constrainer.rb @@ -4,6 +4,6 @@ class GroupUrlConstrainer return false unless DynamicPathValidator.valid?(id) - Group.find_by_full_path(id).present? + Group.find_by_full_path(id, follow_redirects: request.get?).present? end end diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb index d0ce2caffff..6f542f63f98 100644 --- a/lib/constraints/project_url_constrainer.rb +++ b/lib/constraints/project_url_constrainer.rb @@ -6,6 +6,6 @@ class ProjectUrlConstrainer return false unless DynamicPathValidator.valid?(full_path) - Project.find_by_full_path(full_path).present? + Project.find_by_full_path(full_path, follow_redirects: request.get?).present? end end diff --git a/lib/constraints/user_url_constrainer.rb b/lib/constraints/user_url_constrainer.rb index 9ab5bcb12ff..28159dc0dec 100644 --- a/lib/constraints/user_url_constrainer.rb +++ b/lib/constraints/user_url_constrainer.rb @@ -1,5 +1,5 @@ class UserUrlConstrainer def matches?(request) - User.find_by_username(request.params[:username]).present? + User.find_by_full_path(request.params[:username], follow_redirects: request.get?).present? end end diff --git a/lib/tasks/migrate/setup_postgresql.rake b/lib/tasks/migrate/setup_postgresql.rake index 8938bc515f5..1e00b47303d 100644 --- a/lib/tasks/migrate/setup_postgresql.rake +++ b/lib/tasks/migrate/setup_postgresql.rake @@ -11,4 +11,5 @@ task setup_postgresql: :environment do AddUsersLowerUsernameEmailIndexes.new.up AddLowerPathIndexToRoutes.new.up IndexRoutesPathForLike.new.up + IndexRedirectRoutesPathForLike.new.up end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 1bf0533ca24..d40aae04fc3 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -106,10 +106,9 @@ describe ApplicationController do controller.send(:route_not_found) end - it 'does redirect to login page if not authenticated' do + it 'does redirect to login page via authenticate_user! if not authenticated' do allow(controller).to receive(:current_user).and_return(nil) - expect(controller).to receive(:redirect_to) - expect(controller).to receive(:new_user_session_path) + expect(controller).to receive(:authenticate_user!) controller.send(:route_not_found) end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index cad82a34fb0..073b87a1cb4 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -49,6 +49,26 @@ 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(/moved/) + end + end end describe 'GET #merge_requests' do @@ -74,6 +94,26 @@ 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(/moved/) + end + end end describe 'DELETE #destroy' do @@ -81,7 +121,7 @@ describe GroupsController do it 'returns 404' do sign_in(create(:user)) - delete :destroy, id: group.path + delete :destroy, id: group.to_param expect(response.status).to eq(404) end @@ -94,15 +134,39 @@ describe GroupsController do it 'schedules a group destroy' do Sidekiq::Testing.fake! do - expect { delete :destroy, id: group.path }.to change(GroupDestroyWorker.jobs, :size).by(1) + expect { delete :destroy, id: group.to_param }.to change(GroupDestroyWorker.jobs, :size).by(1) end end it 'redirects to the root path' do - delete :destroy, id: group.path + delete :destroy, id: group.to_param 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 @@ -111,7 +175,7 @@ describe GroupsController do sign_in(user) end - it 'updates the path succesfully' do + it 'updates the path successfully' do post :update, id: group.to_param, group: { path: 'new_path' } expect(response).to have_http_status(302) @@ -125,5 +189,29 @@ describe GroupsController do expect(assigns(:group).errors).not_to be_empty expect(assigns(:group).path).not_to eq('new_path') 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' } + + 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 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 + post :update, id: redirect_route.path, group: { path: 'new_path' } + + expect(response).to have_http_status(404) + end + end end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index eafc2154568..e46ef447df2 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -185,6 +185,7 @@ describe ProjectsController do 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 @@ -218,19 +219,33 @@ 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(/moved/) + end + end end describe "#update" do render_views let(:admin) { create(:admin) } + let(:project) { create(:project, :repository) } + let(:new_path) { 'renamed_path' } + let(:project_params) { { path: new_path } } + + before do + sign_in(admin) + end it "sets the repository to the right path after a rename" do - project = create(:project, :repository) - new_path = 'renamed_path' - project_params = { path: new_path } controller.instance_variable_set(:@project, project) - sign_in(admin) put :update, namespace_id: project.namespace, @@ -241,6 +256,34 @@ 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 @@ -276,6 +319,31 @@ 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 @@ -397,6 +465,17 @@ 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(/moved/) + end + end end describe 'POST #preview_markdown' do diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index bbe9aaf737f..74c5aa44ba9 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -4,15 +4,6 @@ describe UsersController do let(:user) { create(:user) } describe 'GET #show' do - it 'is case-insensitive' do - user = create(:user, username: 'CamelCaseUser') - sign_in(user) - - get :show, username: user.username.downcase - - expect(response).to be_success - end - context 'with rendered views' do render_views @@ -45,9 +36,9 @@ describe UsersController do end context 'when logged out' do - it 'renders 404' do + it 'redirects to login page' do get :show, username: user.username - expect(response).to have_http_status(404) + expect(response).to redirect_to new_user_session_path end end @@ -61,6 +52,58 @@ describe UsersController do end 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(/moved/) + end + end + + context 'when a user by that username does not exist' do + context 'when logged out' do + it 'redirects to login page' do + get :show, username: 'nonexistent' + expect(response).to redirect_to new_user_session_path + end + end + + context 'when logged in' do + before { sign_in(user) } + + it 'renders 404' do + get :show, username: 'nonexistent' + expect(response).to have_http_status(404) + end + end + end end describe 'GET #calendar' do @@ -88,11 +131,45 @@ 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(/moved/) + end + end end describe 'GET #calendar_activities' do let!(:project) { create(:empty_project) } - let!(:user) { create(:user) } + let(:user) { create(:user) } before do allow_any_instance_of(User).to receive(:contributed_projects_ids).and_return([project.id]) @@ -110,6 +187,38 @@ 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(/moved/) + end + end end describe 'GET #snippets' do @@ -132,5 +241,83 @@ 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(/moved/) + end + end + end + + describe 'GET #exists' do + before do + sign_in(user) + end + + context 'when user exists' do + it 'returns JSON indicating the user exists' do + get :exists, username: user.username + + expected_json = { exists: true }.to_json + expect(response.body).to eq(expected_json) + end + + context 'when the casing is different' do + let(:user) { create(:user, username: 'CamelCaseUser') } + + it 'returns JSON indicating the user exists' do + get :exists, username: user.username.downcase + + expected_json = { exists: true }.to_json + expect(response.body).to eq(expected_json) + end + end + end + + context 'when the user does not exist' do + it 'returns JSON indicating the user does not exist' do + get :exists, username: 'foo' + + expected_json = { exists: false }.to_json + expect(response.body).to eq(expected_json) + end + + context 'when a user changed their username' do + let(:redirect_route) { user.namespace.redirect_routes.create(path: 'old-username') } + + it 'returns JSON indicating a user by that username does not exist' do + get :exists, username: 'old-username' + + expected_json = { exists: false }.to_json + expect(response.body).to eq(expected_json) + end + end + end end end diff --git a/spec/features/groups/group_settings_spec.rb b/spec/features/groups/group_settings_spec.rb new file mode 100644 index 00000000000..cc25db4ad60 --- /dev/null +++ b/spec/features/groups/group_settings_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' + +feature 'Edit group settings', feature: true do + given(:user) { create(:user) } + given(:group) { create(:group, path: 'foo') } + + background do + group.add_owner(user) + login_as(user) + end + + describe 'when the group path is changed' do + let(:new_group_path) { 'bar' } + let(:old_group_full_path) { "/#{group.path}" } + let(:new_group_full_path) { "/#{new_group_path}" } + + scenario 'the group is accessible via the new path' do + update_path(new_group_path) + visit new_group_full_path + expect(current_path).to eq(new_group_full_path) + expect(find('h1.group-title')).to have_content(new_group_path) + end + + scenario 'the old group path redirects to the new path' do + update_path(new_group_path) + visit old_group_full_path + expect(current_path).to eq(new_group_full_path) + expect(find('h1.group-title')).to have_content(new_group_path) + end + + context 'with a subgroup' do + given!(:subgroup) { create(:group, parent: group, path: 'subgroup') } + given(:old_subgroup_full_path) { "/#{group.path}/#{subgroup.path}" } + given(:new_subgroup_full_path) { "/#{new_group_path}/#{subgroup.path}" } + + scenario 'the subgroup is accessible via the new path' do + update_path(new_group_path) + visit new_subgroup_full_path + expect(current_path).to eq(new_subgroup_full_path) + expect(find('h1.group-title')).to have_content(subgroup.path) + end + + scenario 'the old subgroup path redirects to the new path' do + update_path(new_group_path) + visit old_subgroup_full_path + expect(current_path).to eq(new_subgroup_full_path) + expect(find('h1.group-title')).to have_content(subgroup.path) + end + end + + context 'with a project' do + given!(:project) { create(:project, group: group, path: 'project') } + given(:old_project_full_path) { "/#{group.path}/#{project.path}" } + given(:new_project_full_path) { "/#{new_group_path}/#{project.path}" } + + before(:context) { TestEnv.clean_test_path } + after(:example) { TestEnv.clean_test_path } + + scenario 'the project is accessible via the new path' do + update_path(new_group_path) + visit new_project_full_path + expect(current_path).to eq(new_project_full_path) + expect(find('h1.project-title')).to have_content(project.name) + end + + scenario 'the old project path redirects to the new path' do + update_path(new_group_path) + visit old_project_full_path + expect(current_path).to eq(new_project_full_path) + expect(find('h1.project-title')).to have_content(project.name) + end + end + end +end + +def update_path(new_group_path) + visit edit_group_path(group) + fill_in 'group_path', with: new_group_path + click_button 'Save group' +end diff --git a/spec/features/profiles/account_spec.rb b/spec/features/profiles/account_spec.rb new file mode 100644 index 00000000000..05a7587f8d4 --- /dev/null +++ b/spec/features/profiles/account_spec.rb @@ -0,0 +1,59 @@ +require 'rails_helper' + +feature 'Profile > Account', feature: true do + given(:user) { create(:user, username: 'foo') } + + before do + login_as(user) + end + + describe 'Change username' do + given(:new_username) { 'bar' } + given(:new_user_path) { "/#{new_username}" } + given(:old_user_path) { "/#{user.username}" } + + scenario 'the user is accessible via the new path' do + update_username(new_username) + visit new_user_path + expect(current_path).to eq(new_user_path) + expect(find('.user-info')).to have_content(new_username) + end + + scenario 'the old user path redirects to the new path' do + update_username(new_username) + visit old_user_path + expect(current_path).to eq(new_user_path) + expect(find('.user-info')).to have_content(new_username) + end + + context 'with a project' do + given!(:project) { create(:project, namespace: user.namespace, path: 'project') } + given(:new_project_path) { "/#{new_username}/#{project.path}" } + given(:old_project_path) { "/#{user.username}/#{project.path}" } + + before(:context) { TestEnv.clean_test_path } + after(:example) { TestEnv.clean_test_path } + + scenario 'the project is accessible via the new path' do + update_username(new_username) + visit new_project_path + expect(current_path).to eq(new_project_path) + expect(find('h1.project-title')).to have_content(project.name) + end + + scenario 'the old project path redirects to the new path' do + update_username(new_username) + visit old_project_path + expect(current_path).to eq(new_project_path) + expect(find('h1.project-title')).to have_content(project.name) + end + end + end +end + +def update_username(new_username) + allow(user.namespace).to receive(:move_dir) + visit profile_account_path + fill_in 'user_username', with: new_username + click_button 'Update username' +end diff --git a/spec/features/projects/features_visibility_spec.rb b/spec/features/projects/features_visibility_spec.rb index b080a8d500e..e1781cf320a 100644 --- a/spec/features/projects/features_visibility_spec.rb +++ b/spec/features/projects/features_visibility_spec.rb @@ -68,20 +68,23 @@ describe 'Edit Project Settings', feature: true do end describe 'project features visibility pages' do - before do - @tools = - { - builds: namespace_project_pipelines_path(project.namespace, project), - issues: namespace_project_issues_path(project.namespace, project), - wiki: namespace_project_wiki_path(project.namespace, project, :home), - snippets: namespace_project_snippets_path(project.namespace, project), - merge_requests: namespace_project_merge_requests_path(project.namespace, project), - } + let(:tools) do + { + builds: namespace_project_pipelines_path(project.namespace, project), + issues: namespace_project_issues_path(project.namespace, project), + wiki: namespace_project_wiki_path(project.namespace, project, :home), + snippets: namespace_project_snippets_path(project.namespace, project), + merge_requests: namespace_project_merge_requests_path(project.namespace, project), + } end context 'normal user' do + before do + login_as(member) + end + it 'renders 200 if tool is enabled' do - @tools.each do |method_name, url| + tools.each do |method_name, url| project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::ENABLED) visit url expect(page.status_code).to eq(200) @@ -89,7 +92,7 @@ describe 'Edit Project Settings', feature: true do end it 'renders 404 if feature is disabled' do - @tools.each do |method_name, url| + tools.each do |method_name, url| project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::DISABLED) visit url expect(page.status_code).to eq(404) @@ -99,21 +102,21 @@ describe 'Edit Project Settings', feature: true do it 'renders 404 if feature is enabled only for team members' do project.team.truncate - @tools.each do |method_name, url| + tools.each do |method_name, url| project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::PRIVATE) visit url expect(page.status_code).to eq(404) end end - it 'renders 200 if users is member of group' do + it 'renders 200 if user is member of group' do group = create(:group) project.group = group project.save group.add_owner(member) - @tools.each do |method_name, url| + tools.each do |method_name, url| project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::PRIVATE) visit url expect(page.status_code).to eq(200) @@ -128,7 +131,7 @@ describe 'Edit Project Settings', feature: true do end it 'renders 404 if feature is disabled' do - @tools.each do |method_name, url| + tools.each do |method_name, url| project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::DISABLED) visit url expect(page.status_code).to eq(404) @@ -138,7 +141,7 @@ describe 'Edit Project Settings', feature: true do it 'renders 200 if feature is enabled only for team members' do project.team.truncate - @tools.each do |method_name, url| + tools.each do |method_name, url| project.project_feature.update_attribute("#{method_name}_access_level", ProjectFeature::PRIVATE) visit url expect(page.status_code).to eq(200) diff --git a/spec/features/projects/project_settings_spec.rb b/spec/features/projects/project_settings_spec.rb index 5d0314d5c09..11dcab4d737 100644 --- a/spec/features/projects/project_settings_spec.rb +++ b/spec/features/projects/project_settings_spec.rb @@ -1,64 +1,158 @@ require 'spec_helper' describe 'Edit Project Settings', feature: true do + include Select2Helper + let(:user) { create(:user) } - let(:project) { create(:empty_project, path: 'gitlab', name: 'sample') } + let(:project) { create(:empty_project, namespace: user.namespace, path: 'gitlab', name: 'sample') } before do login_as(user) - project.team << [user, :master] end - describe 'Project settings', js: true do + describe 'Project settings section', js: true do it 'shows errors for invalid project name' do visit edit_namespace_project_path(project.namespace, project) - fill_in 'project_name_edit', with: 'foo&bar' - click_button 'Save changes' - expect(page).to have_field 'project_name_edit', with: 'foo&bar' expect(page).to have_content "Name can contain only letters, digits, emojis, '_', '.', dash, space. It must start with letter, digit, emoji or '_'." expect(page).to have_button 'Save changes' end - scenario 'shows a successful notice when the project is updated' do + it 'shows a successful notice when the project is updated' do visit edit_namespace_project_path(project.namespace, project) - fill_in 'project_name_edit', with: 'hello world' - click_button 'Save changes' - expect(page).to have_content "Project 'hello world' was successfully updated." end end - describe 'Rename repository' do - it 'shows errors for invalid project path/name' do - visit edit_namespace_project_path(project.namespace, project) + describe 'Rename repository section' do + context 'with invalid characters' do + it 'shows errors for invalid project path/name' do + rename_project(project, name: 'foo&bar', path: 'foo&bar') + expect(page).to have_field 'Project name', with: 'foo&bar' + expect(page).to have_field 'Path', with: 'foo&bar' + expect(page).to have_content "Name can contain only letters, digits, emojis, '_', '.', dash, space. It must start with letter, digit, emoji or '_'." + expect(page).to have_content "Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'" + end + end - fill_in 'project_name', with: 'foo&bar' - fill_in 'Path', with: 'foo&bar' + context 'when changing project name' do + it 'renames the repository' do + rename_project(project, name: 'bar') + expect(find('h1.title')).to have_content(project.name) + end - click_button 'Rename project' + context 'with emojis' do + it 'shows error for invalid project name' do + rename_project(project, name: '🚀 foo bar ☁️') + expect(page).to have_field 'Project name', with: '🚀 foo bar ☁️' + expect(page).not_to have_content "Name can contain only letters, digits, emojis '_', '.', dash and space. It must start with letter, digit, emoji or '_'." + end + end + end - expect(page).to have_field 'Project name', with: 'foo&bar' - expect(page).to have_field 'Path', with: 'foo&bar' - expect(page).to have_content "Name can contain only letters, digits, emojis, '_', '.', dash, space. It must start with letter, digit, emoji or '_'." - expect(page).to have_content "Path can contain only letters, digits, '_', '-' and '.'. Cannot start with '-', end in '.git' or end in '.atom'" + context 'when changing project path' do + # Not using empty project because we need a repo to exist + let(:project) { create(:project, namespace: user.namespace, name: 'gitlabhq') } + + before(:context) { TestEnv.clean_test_path } + after(:example) { TestEnv.clean_test_path } + + specify 'the project is accessible via the new path' do + rename_project(project, path: 'bar') + new_path = namespace_project_path(project.namespace, 'bar') + visit new_path + expect(current_path).to eq(new_path) + expect(find('h1.title')).to have_content(project.name) + end + + specify 'the project is accessible via a redirect from the old path' do + old_path = namespace_project_path(project.namespace, project) + rename_project(project, path: 'bar') + new_path = namespace_project_path(project.namespace, 'bar') + visit old_path + expect(current_path).to eq(new_path) + expect(find('h1.title')).to have_content(project.name) + end + + context 'and a new project is added with the same path' do + it 'overrides the redirect' do + old_path = namespace_project_path(project.namespace, project) + rename_project(project, path: 'bar') + new_project = create(:empty_project, namespace: user.namespace, path: 'gitlabhq', name: 'quz') + visit old_path + expect(current_path).to eq(old_path) + expect(find('h1.title')).to have_content(new_project.name) + end + end end end - describe 'Rename repository name with emojis' do - it 'shows error for invalid project name' do - visit edit_namespace_project_path(project.namespace, project) + describe 'Transfer project section', js: true do + # Not using empty project because we need a repo to exist + let!(:project) { create(:project, namespace: user.namespace, name: 'gitlabhq') } + let!(:group) { create(:group) } - fill_in 'project_name', with: '🚀 foo bar ☁️' + before(:context) { TestEnv.clean_test_path } + before(:example) { group.add_owner(user) } + after(:example) { TestEnv.clean_test_path } - click_button 'Rename project' + specify 'the project is accessible via the new path' do + transfer_project(project, group) + new_path = namespace_project_path(group, project) + visit new_path + expect(current_path).to eq(new_path) + expect(find('h1.title')).to have_content(project.name) + end - expect(page).to have_field 'Project name', with: '🚀 foo bar ☁️' - expect(page).not_to have_content "Name can contain only letters, digits, emojis '_', '.', dash and space. It must start with letter, digit, emoji or '_'." + specify 'the project is accessible via a redirect from the old path' do + old_path = namespace_project_path(project.namespace, project) + transfer_project(project, group) + new_path = namespace_project_path(group, project) + visit old_path + expect(current_path).to eq(new_path) + expect(find('h1.title')).to have_content(project.name) + end + + context 'and a new project is added with the same path' do + it 'overrides the redirect' do + old_path = namespace_project_path(project.namespace, project) + transfer_project(project, group) + new_project = create(:empty_project, namespace: user.namespace, path: 'gitlabhq', name: 'quz') + visit old_path + expect(current_path).to eq(old_path) + expect(find('h1.title')).to have_content(new_project.name) + end end end end + +def rename_project(project, name: nil, path: nil) + visit edit_namespace_project_path(project.namespace, project) + fill_in('project_name', with: name) if name + fill_in('Path', with: path) if path + click_button('Rename project') + wait_for_edit_project_page_reload + project.reload +end + +def transfer_project(project, namespace) + visit edit_namespace_project_path(project.namespace, project) + select2(namespace.id, from: '#new_namespace_id') + click_button('Transfer project') + confirm_transfer_modal + wait_for_edit_project_page_reload + project.reload +end + +def confirm_transfer_modal + fill_in('confirm_name_input', with: project.path) + click_button 'Confirm' +end + +def wait_for_edit_project_page_reload + expect(find('.project-edit-container')).to have_content('Rename repository') +end diff --git a/spec/lib/constraints/group_url_constrainer_spec.rb b/spec/lib/constraints/group_url_constrainer_spec.rb index f95adf3a84b..db680489a8d 100644 --- a/spec/lib/constraints/group_url_constrainer_spec.rb +++ b/spec/lib/constraints/group_url_constrainer_spec.rb @@ -29,9 +29,37 @@ describe GroupUrlConstrainer, lib: true do it { expect(subject.matches?(request)).to be_falsey } end + + context 'when the request matches a redirect route' do + context 'for a root group' do + let!(:redirect_route) { group.redirect_routes.create!(path: 'gitlabb') } + + context 'and is a GET request' do + let(:request) { build_request(redirect_route.path) } + + it { expect(subject.matches?(request)).to be_truthy } + end + + context 'and is NOT a GET request' do + let(:request) { build_request(redirect_route.path, 'POST') } + + it { expect(subject.matches?(request)).to be_falsey } + end + end + + context 'for a nested group' do + let!(:nested_group) { create(:group, path: 'nested', parent: group) } + let!(:redirect_route) { nested_group.redirect_routes.create!(path: 'gitlabb/nested') } + let(:request) { build_request(redirect_route.path) } + + it { expect(subject.matches?(request)).to be_truthy } + end + end end - def build_request(path) - double(:request, params: { id: path }) + def build_request(path, method = 'GET') + double(:request, + 'get?': (method == 'GET'), + params: { id: path }) end end diff --git a/spec/lib/constraints/project_url_constrainer_spec.rb b/spec/lib/constraints/project_url_constrainer_spec.rb index 4f25ad88960..b6884e37aa3 100644 --- a/spec/lib/constraints/project_url_constrainer_spec.rb +++ b/spec/lib/constraints/project_url_constrainer_spec.rb @@ -24,9 +24,26 @@ describe ProjectUrlConstrainer, lib: true do it { expect(subject.matches?(request)).to be_falsey } end end + + context 'when the request matches a redirect route' do + let(:old_project_path) { 'old_project_path' } + let!(:redirect_route) { project.redirect_routes.create!(path: "#{namespace.full_path}/#{old_project_path}") } + + context 'and is a GET request' do + let(:request) { build_request(namespace.full_path, old_project_path) } + it { expect(subject.matches?(request)).to be_truthy } + end + + context 'and is NOT a GET request' do + let(:request) { build_request(namespace.full_path, old_project_path, 'POST') } + it { expect(subject.matches?(request)).to be_falsey } + end + end end - def build_request(namespace, project) - double(:request, params: { namespace_id: namespace, id: project }) + def build_request(namespace, project, method = 'GET') + double(:request, + 'get?': (method == 'GET'), + params: { namespace_id: namespace, id: project }) end end diff --git a/spec/lib/constraints/user_url_constrainer_spec.rb b/spec/lib/constraints/user_url_constrainer_spec.rb index 207b6fe6c9e..ed69b830979 100644 --- a/spec/lib/constraints/user_url_constrainer_spec.rb +++ b/spec/lib/constraints/user_url_constrainer_spec.rb @@ -15,9 +15,26 @@ describe UserUrlConstrainer, lib: true do it { expect(subject.matches?(request)).to be_falsey } end + + context 'when the request matches a redirect route' do + let(:old_project_path) { 'old_project_path' } + let!(:redirect_route) { user.namespace.redirect_routes.create!(path: 'foo') } + + context 'and is a GET request' do + let(:request) { build_request(redirect_route.path) } + it { expect(subject.matches?(request)).to be_truthy } + end + + context 'and is NOT a GET request' do + let(:request) { build_request(redirect_route.path, 'POST') } + it { expect(subject.matches?(request)).to be_falsey } + end + end end - def build_request(username) - double(:request, params: { username: username }) + def build_request(username, method = 'GET') + double(:request, + 'get?': (method == 'GET'), + params: { username: username }) end end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index f7654f1aade..baa81870e81 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -229,6 +229,7 @@ project: - authorized_users - project_authorizations - route +- redirect_routes - statistics - container_repositories - uploads diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index 221647d7a48..49a4132f763 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -9,6 +9,7 @@ describe Group, 'Routable' do describe 'Associations' do it { is_expected.to have_one(:route).dependent(:destroy) } + it { is_expected.to have_many(:redirect_routes).dependent(:destroy) } end describe 'Callbacks' do @@ -35,10 +36,53 @@ describe Group, 'Routable' do describe '.find_by_full_path' do let!(:nested_group) { create(:group, parent: group) } - it { expect(described_class.find_by_full_path(group.to_param)).to eq(group) } - it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) } - it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) } - it { expect(described_class.find_by_full_path('unknown')).to eq(nil) } + context 'without any redirect routes' do + it { expect(described_class.find_by_full_path(group.to_param)).to eq(group) } + it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) } + it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) } + it { expect(described_class.find_by_full_path('unknown')).to eq(nil) } + end + + context 'with redirect routes' do + let!(:group_redirect_route) { group.redirect_routes.create!(path: 'bar') } + let!(:nested_group_redirect_route) { nested_group.redirect_routes.create!(path: nested_group.path.sub('foo', 'bar')) } + + context 'without follow_redirects option' do + context 'with the given path not matching any route' do + it { expect(described_class.find_by_full_path('unknown')).to eq(nil) } + end + + context 'with the given path matching the canonical route' do + it { expect(described_class.find_by_full_path(group.to_param)).to eq(group) } + it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) } + it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) } + end + + context 'with the given path matching a redirect route' do + it { expect(described_class.find_by_full_path(group_redirect_route.path)).to eq(nil) } + it { expect(described_class.find_by_full_path(group_redirect_route.path.upcase)).to eq(nil) } + it { expect(described_class.find_by_full_path(nested_group_redirect_route.path)).to eq(nil) } + end + end + + context 'with follow_redirects option set to true' do + context 'with the given path not matching any route' do + it { expect(described_class.find_by_full_path('unknown', follow_redirects: true)).to eq(nil) } + end + + context 'with the given path matching the canonical route' do + it { expect(described_class.find_by_full_path(group.to_param, follow_redirects: true)).to eq(group) } + it { expect(described_class.find_by_full_path(group.to_param.upcase, follow_redirects: true)).to eq(group) } + it { expect(described_class.find_by_full_path(nested_group.to_param, follow_redirects: true)).to eq(nested_group) } + end + + context 'with the given path matching a redirect route' do + it { expect(described_class.find_by_full_path(group_redirect_route.path, follow_redirects: true)).to eq(group) } + it { expect(described_class.find_by_full_path(group_redirect_route.path.upcase, follow_redirects: true)).to eq(group) } + it { expect(described_class.find_by_full_path(nested_group_redirect_route.path, follow_redirects: true)).to eq(nested_group) } + end + end + end end describe '.where_full_path_in' do diff --git a/spec/models/redirect_route_spec.rb b/spec/models/redirect_route_spec.rb new file mode 100644 index 00000000000..71827421dd7 --- /dev/null +++ b/spec/models/redirect_route_spec.rb @@ -0,0 +1,27 @@ +require 'rails_helper' + +describe RedirectRoute, models: true do + let(:group) { create(:group) } + let!(:redirect_route) { group.redirect_routes.create(path: 'gitlabb') } + + describe 'relationships' do + it { is_expected.to belong_to(:source) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:source) } + it { is_expected.to validate_presence_of(:path) } + it { is_expected.to validate_uniqueness_of(:path) } + end + + describe '.matching_path_and_descendants' do + let!(:redirect2) { group.redirect_routes.create(path: 'gitlabb/test') } + let!(:redirect3) { group.redirect_routes.create(path: 'gitlabb/test/foo') } + let!(:redirect4) { group.redirect_routes.create(path: 'gitlabb/test/foo/bar') } + let!(:redirect5) { group.redirect_routes.create(path: 'gitlabb/test/baz') } + + it 'returns correct routes' do + expect(RedirectRoute.matching_path_and_descendants('gitlabb/test')).to match_array([redirect2, redirect3, redirect4, redirect5]) + end + end +end diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb index 171a51fcc5b..c1fe1b06c52 100644 --- a/spec/models/route_spec.rb +++ b/spec/models/route_spec.rb @@ -1,19 +1,43 @@ require 'spec_helper' describe Route, models: true do - let!(:group) { create(:group, path: 'git_lab', name: 'git_lab') } - let!(:route) { group.route } + let(:group) { create(:group, path: 'git_lab', name: 'git_lab') } + let(:route) { group.route } describe 'relationships' do it { is_expected.to belong_to(:source) } end describe 'validations' do + before { route } it { is_expected.to validate_presence_of(:source) } it { is_expected.to validate_presence_of(:path) } it { is_expected.to validate_uniqueness_of(:path) } end + describe 'callbacks' do + context 'after update' do + it 'calls #create_redirect_for_old_path' do + expect(route).to receive(:create_redirect_for_old_path) + route.update_attributes(path: 'foo') + end + + it 'calls #delete_conflicting_redirects' do + expect(route).to receive(:delete_conflicting_redirects) + route.update_attributes(path: 'foo') + end + end + + context 'after create' do + it 'calls #delete_conflicting_redirects' do + route.destroy + new_route = Route.new(source: group, path: group.path) + expect(new_route).to receive(:delete_conflicting_redirects) + new_route.save! + end + end + end + describe '.inside_path' do let!(:nested_group) { create(:group, path: 'test', name: 'test', parent: group) } let!(:deep_nested_group) { create(:group, path: 'foo', name: 'foo', parent: nested_group) } @@ -37,7 +61,7 @@ describe Route, models: true do context 'when route name is set' do before { route.update_attributes(path: 'bar') } - it "updates children routes with new path" do + it 'updates children routes with new path' do expect(described_class.exists?(path: 'bar')).to be_truthy expect(described_class.exists?(path: 'bar/test')).to be_truthy expect(described_class.exists?(path: 'bar/test/foo')).to be_truthy @@ -56,10 +80,24 @@ describe Route, models: true do expect(route.update_attributes(path: 'bar')).to be_truthy end end + + context 'when conflicting redirects exist' do + let!(:conflicting_redirect1) { route.create_redirect('bar/test') } + let!(:conflicting_redirect2) { route.create_redirect('bar/test/foo') } + let!(:conflicting_redirect3) { route.create_redirect('gitlab-org') } + + it 'deletes the conflicting redirects' do + route.update_attributes(path: 'bar') + + expect(RedirectRoute.exists?(path: 'bar/test')).to be_falsey + expect(RedirectRoute.exists?(path: 'bar/test/foo')).to be_falsey + expect(RedirectRoute.exists?(path: 'gitlab-org')).to be_truthy + end + end end context 'name update' do - it "updates children routes with new path" do + it 'updates children routes with new path' do route.update_attributes(name: 'bar') expect(described_class.exists?(name: 'bar')).to be_truthy @@ -77,4 +115,72 @@ describe Route, models: true do end end end + + describe '#create_redirect_for_old_path' do + context 'if the path changed' do + it 'creates a RedirectRoute for the old path' do + redirect_scope = route.source.redirect_routes.where(path: 'git_lab') + expect(redirect_scope.exists?).to be_falsey + route.path = 'new-path' + route.save! + expect(redirect_scope.exists?).to be_truthy + end + end + end + + describe '#create_redirect' do + it 'creates a RedirectRoute with the same source' do + redirect_route = route.create_redirect('foo') + expect(redirect_route).to be_a(RedirectRoute) + expect(redirect_route).to be_persisted + expect(redirect_route.source).to eq(route.source) + expect(redirect_route.path).to eq('foo') + end + end + + describe '#delete_conflicting_redirects' do + context 'when a redirect route with the same path exists' do + let!(:redirect1) { route.create_redirect(route.path) } + + it 'deletes the redirect' do + route.delete_conflicting_redirects + expect(route.conflicting_redirects).to be_empty + end + + context 'when redirect routes with paths descending from the route path exists' do + let!(:redirect2) { route.create_redirect("#{route.path}/foo") } + let!(:redirect3) { route.create_redirect("#{route.path}/foo/bar") } + let!(:redirect4) { route.create_redirect("#{route.path}/baz/quz") } + let!(:other_redirect) { route.create_redirect("other") } + + it 'deletes all redirects with paths that descend from the route path' do + route.delete_conflicting_redirects + expect(route.conflicting_redirects).to be_empty + end + end + end + end + + describe '#conflicting_redirects' do + context 'when a redirect route with the same path exists' do + let!(:redirect1) { route.create_redirect(route.path) } + + it 'returns the redirect route' do + expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation) + expect(route.conflicting_redirects).to match_array([redirect1]) + end + + context 'when redirect routes with paths descending from the route path exists' do + let!(:redirect2) { route.create_redirect("#{route.path}/foo") } + let!(:redirect3) { route.create_redirect("#{route.path}/foo/bar") } + let!(:redirect4) { route.create_redirect("#{route.path}/baz/quz") } + let!(:other_redirect) { route.create_redirect("other") } + + it 'returns the redirect routes' do + expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation) + expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3, redirect4]) + end + end + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 401eaac07a1..63e71f5ff2f 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -849,6 +849,65 @@ describe User, models: true do end end + describe '.find_by_full_path' do + let!(:user) { create(:user) } + + context 'with a route matching the given path' do + let!(:route) { user.namespace.route } + + it 'returns the user' do + expect(User.find_by_full_path(route.path)).to eq(user) + end + + it 'is case-insensitive' do + expect(User.find_by_full_path(route.path.upcase)).to eq(user) + expect(User.find_by_full_path(route.path.downcase)).to eq(user) + end + end + + context 'with a redirect route matching the given path' do + let!(:redirect_route) { user.namespace.redirect_routes.create(path: 'foo') } + + context 'without the follow_redirects option' do + it 'returns nil' do + expect(User.find_by_full_path(redirect_route.path)).to eq(nil) + end + end + + context 'with the follow_redirects option set to true' do + it 'returns the user' do + expect(User.find_by_full_path(redirect_route.path, follow_redirects: true)).to eq(user) + end + + it 'is case-insensitive' do + expect(User.find_by_full_path(redirect_route.path.upcase, follow_redirects: true)).to eq(user) + expect(User.find_by_full_path(redirect_route.path.downcase, follow_redirects: true)).to eq(user) + end + end + end + + context 'without a route or a redirect route matching the given path' do + context 'without the follow_redirects option' do + it 'returns nil' do + expect(User.find_by_full_path('unknown')).to eq(nil) + end + end + context 'with the follow_redirects option set to true' do + it 'returns nil' do + expect(User.find_by_full_path('unknown', follow_redirects: true)).to eq(nil) + end + end + end + + context 'with a group route matching the given path' do + let!(:group) { create(:group, path: 'group_path') } + + it 'returns nil' do + expect(User.find_by_full_path('group_path')).to eq(nil) + end + end + end + describe 'all_ssh_keys' do it { is_expected.to have_many(:keys).dependent(:destroy) } diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index 163df072cf6..50e96d56191 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'project routing' do before do allow(Project).to receive(:find_by_full_path).and_return(false) - allow(Project).to receive(:find_by_full_path).with('gitlab/gitlabhq').and_return(true) + allow(Project).to receive(:find_by_full_path).with('gitlab/gitlabhq', any_args).and_return(true) end # Shared examples for a resource inside a Project @@ -93,13 +93,13 @@ describe 'project routing' do end context 'name with dot' do - before { allow(Project).to receive(:find_by_full_path).with('gitlab/gitlabhq.keys').and_return(true) } + before { allow(Project).to receive(:find_by_full_path).with('gitlab/gitlabhq.keys', any_args).and_return(true) } it { expect(get('/gitlab/gitlabhq.keys')).to route_to('projects#show', namespace_id: 'gitlab', id: 'gitlabhq.keys') } end context 'with nested group' do - before { allow(Project).to receive(:find_by_full_path).with('gitlab/subgroup/gitlabhq').and_return(true) } + before { allow(Project).to receive(:find_by_full_path).with('gitlab/subgroup/gitlabhq', any_args).and_return(true) } it { expect(get('/gitlab/subgroup/gitlabhq')).to route_to('projects#show', namespace_id: 'gitlab/subgroup', id: 'gitlabhq') } end