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

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

app/controllers/concerns/routable_actions.rb

+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

app/controllers/groups/application_controller.rb

 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

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]

app/controllers/projects/application_controller.rb

 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
-    @project
+    return @project if @project
+
+    path = File.join(params[:namespace_id], params[:project_id] || params[:id])
+    auth_proc = ->(project) { !project.pending_delete? }
+    @project = find_routable!(Project, path, extra_authorization_proc: auth_proc)
   end
   def repository

app/controllers/users_controller.rb

 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

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.

app/models/redirect_route.rb

+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

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)
-      descendants.each do |route|
-        attributes = {}
-        if path_changed? && route.path.present?
-          attributes[:path] = route.path.sub(path_was, path)
-        end
-        if name_changed? && name_was.present? && route.name.present?
-          attributes[:name] = route.name.sub(name_was, name)
-        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?
+    return unless path_changed? || name_changed?
+    descendant_routes = self.class.inside_path(path_was)
+    descendant_routes.each do |route|
+      attributes = {}
+      if path_changed? && route.path.present?
+        attributes[:path] = route.path.sub(path_was, path)
+      end
+      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

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

changelogs/unreleased/17361-redirect-renamed-paths.yml

+---
+title: Redirect old links after renaming a user/group/project.
+merge_request: 10370
+author:

db/migrate/20170427215854_create_redirect_routes.rb

+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

db/migrate/20170503184421_add_index_to_redirect_routes.rb

+# 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

db/migrate/20170503185032_index_redirect_routes_path_for_like.rb

+# 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

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"

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

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

lib/constraints/user_url_constrainer.rb

 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

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

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