Dry up routable lookups. Fixes #30317

Note: This changes the behavior of user lookups (see the spec change) so it acts the same way as groups and projects. Unauthenticated clients attempting to access a user page will be redirected to login whether the user exists and is publicly restricted, or does not exist at all.

5 files changed, 58 insertions, 55 deletions

diff --git a/app/controllers/concerns/routable_actions.rb b/app/controllers/concerns/routable_actions.rb
index 1714bc25e52..ba236fa5459 100644
--- a/app/controllers/concerns/routable_actions.rb
+++ b/app/controllers/concerns/routable_actions.rb
@@ -1,12 +1,36 @@
 module RoutableActions
   extend ActiveSupport::Concern
 
+  def find_routable!(routable_klass, requested_full_path, extra_authorization_method: nil)
+    routable = routable_klass.find_by_full_path(requested_full_path, follow_redirects: request.get?)
+
+    if authorized?(routable_klass, routable, extra_authorization_method)
+      ensure_canonical_path(routable, requested_full_path)
+      routable
+    else
+      route_not_found
+      nil
+    end
+  end
+
+  def authorized?(routable_klass, routable, extra_authorization_method)
+    action = :"read_#{routable_klass.to_s.underscore}"
+    return false unless can?(current_user, action, routable)
+
+    if extra_authorization_method
+      send(extra_authorization_method, routable)
+    else
+      true
+    end
+  end
+
   def ensure_canonical_path(routable, requested_path)
     return unless request.get?
 
-    if routable.full_path != requested_path
+    canonical_path = routable.try(:full_path) || routable.namespace.full_path
+    if canonical_path != requested_path
       flash[:notice] = 'This project has moved to this location. Please update your links and bookmarks.'
-      redirect_to request.original_url.sub(requested_path, routable.full_path)
+      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 209d8b1a08a..2157a56dea2 100644
--- a/app/controllers/groups/application_controller.rb
+++ b/app/controllers/groups/application_controller.rb
@@ -9,20 +9,11 @@ class Groups::ApplicationController < ApplicationController
   private
 
   def group
-    unless @group
-      given_path = params[:group_id] || params[:id]
-      @group = Group.find_by_full_path(given_path, follow_redirects: request.get?)
-
-      if @group && can?(current_user, :read_group, @group)
-        ensure_canonical_path(@group, given_path)
-      else
-        @group = nil
-
-        route_not_found
-      end
-    end
+    @group ||= find_routable!(Group, requested_full_path)
+  end
 
-    @group
+  def requested_full_path
+    params[:group_id] || params[:id]
   end
 
   def group_projects
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index dbdf68776f1..2301e1cca77 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -2,6 +2,7 @@ class Projects::ApplicationController < ApplicationController
   include RoutableActions
 
   skip_before_action :authenticate_user!
+  before_action :redirect_git_extension
   before_action :project
   before_action :repository
   layout 'project'
@@ -10,34 +11,30 @@ class Projects::ApplicationController < ApplicationController
 
   private
 
-  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, follow_redirects: request.get?)
-
-      if can?(current_user, :read_project, @project) && !@project.pending_delete?
-        ensure_canonical_path(@project, project_path)
-      else
-        @project = nil
-
-        route_not_found
-      end
+  def redirect_git_extension
+    # Redirect from
+    #   localhost/group/project.git
+    # to
+    #   localhost/group/project
+    #
+    if params[:format] == 'git'
+      redirect_to request.original_url.gsub(/\.git\/?\Z/, '')
+      return
     end
+  end
+
+  def project
+    @project ||= find_routable!(Project, requested_full_path, extra_authorization_method: :project_not_being_deleted?)
+  end
+
+  def requested_full_path
+    namespace = params[:namespace_id]
+    id = params[:project_id] || params[:id]
+    "#{namespace}/#{id}"
+  end
 
-    @project
+  def project_not_being_deleted?(project)
+    !project.pending_delete?
   end
 
   def repository
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 67783866c3f..ca89ed221c6 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -93,16 +93,7 @@ class UsersController < ApplicationController
   private
 
   def user
-    return @user if @user
-
-    @user = User.find_by_full_path(params[:username], follow_redirects: true)
-
-    return render_404 unless @user
-    return render_404 unless can?(current_user, :read_user, @user)
-
-    ensure_canonical_path(@user.namespace, params[:username])
-
-    @user
+    @user ||= find_routable!(User, params[:username])
   end
 
   def contributed_projects
diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb
index 5e8caa89cb7..9b6b9358a40 100644
--- a/spec/controllers/users_controller_spec.rb
+++ b/spec/controllers/users_controller_spec.rb
@@ -36,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
 
@@ -88,9 +88,9 @@ describe UsersController do
 
     context 'when a user by that username does not exist' do
       context 'when logged out' do
-        it 'renders 404 (does not redirect to login)' do
+        it 'redirects to login page' do
           get :show, username: 'nonexistent'
-          expect(response).to have_http_status(404)
+          expect(response).to redirect_to new_user_session_path
         end
       end