diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2017-05-06 05:45:56 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2017-05-06 05:45:56 +0000 |
commit | 56fb7823f9740d4595d90dcb224180c950a27048 (patch) | |
tree | ebf2f5498f164477cfe594ea6b820202087532a9 /app | |
parent | 6ce1df41e175c7d62ca760b1e66cf1bf86150284 (diff) | |
parent | b0ee22609a89572d6e3f98eebccf9fb2335dd939 (diff) | |
download | gitlab-ce-56fb7823f9740d4595d90dcb224180c950a27048.tar.gz |
Merge branch '17361-redirect-renamed-paths' into 'master'
Resolve "Redirect to new project link after a rename"
Closes #17361 and #30317
See merge request !11136
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/application_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/concerns/routable_actions.rb | 38 | ||||
-rw-r--r-- | app/controllers/groups/application_controller.rb | 24 | ||||
-rw-r--r-- | app/controllers/groups_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/application_controller.rb | 49 | ||||
-rw-r--r-- | app/controllers/users_controller.rb | 9 | ||||
-rw-r--r-- | app/models/concerns/routable.rb | 24 | ||||
-rw-r--r-- | app/models/redirect_route.rb | 12 | ||||
-rw-r--r-- | app/models/route.rb | 55 | ||||
-rw-r--r-- | app/models/user.rb | 9 |
10 files changed, 150 insertions, 74 deletions
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] - - # Redirect from - # localhost/group/project.git - # to - # localhost/group/project - # - if params[:format] == 'git' - redirect_to request.original_url.gsub(/\.git\/?\Z/, '') - return - end - - 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 + return @project if @project + + path = File.join(params[:namespace_id], params[:project_id] || params[:id]) + auth_proc = ->(project) { !project.pending_delete? } - @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)" - - where_full_path_in([path]).reorder(order_sql).take + found = where_full_path_in([path]).reorder(order_sql).take + return found if found + + 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 |