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

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