diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index d7c1241698a..67783866c3f 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -3,7 +3,6 @@ class UsersController < ApplicationController skip_before_action :authenticate_user! before_action :user, except: [:exists] - before_action :authorize_read_user!, except: [:exists] def show respond_to do |format| @@ -93,14 +92,17 @@ class UsersController < ApplicationController private - def authorize_read_user! - render_404 unless can?(current_user, :read_user, user) - - ensure_canonical_path(user.namespace, params[:username]) - end - def user - @user ||= User.find_by_full_path(params[:username], follow_redirects: true) + 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 end def contributed_projects diff --git a/app/models/route.rb b/app/models/route.rb index accc423ae46..3d798ce937b 100644 --- a/app/models/route.rb +++ b/app/models/route.rb @@ -16,22 +16,22 @@ class Route < ActiveRecord::Base scope :direct_descendant_routes, -> (path) { where('routes.path LIKE ? AND routes.path NOT LIKE ?', "#{sanitize_sql_like(path)}/%", "#{sanitize_sql_like(path)}/%/%") } def rename_direct_descendant_routes - if path_changed? || name_changed? - direct_descendant_routes = self.class.direct_descendant_routes(path_was) + return if !path_changed? && !name_changed? - direct_descendant_routes.each do |route| - attributes = {} + direct_descendant_routes = self.class.direct_descendant_routes(path_was) - if path_changed? && route.path.present? - attributes[:path] = route.path.sub(path_was, path) - end + direct_descendant_routes.each do |route| + attributes = {} - if name_changed? && name_was.present? && route.name.present? - attributes[:name] = route.name.sub(name_was, name) - end - - route.update(attributes) unless attributes.empty? + 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 + + route.update(attributes) unless attributes.empty? end end diff --git a/app/models/user.rb b/app/models/user.rb index c6354b989ad..7da92d03427 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -335,7 +335,7 @@ class User < ActiveRecord::Base def find_by_full_path(path, follow_redirects: false) namespace = Namespace.find_by_full_path(path, follow_redirects: follow_redirects) - namespace.owner if namespace && namespace.owner + namespace&.owner end def reference_prefix diff --git a/db/migrate/20170503184421_add_index_to_redirect_routes.rb b/db/migrate/20170503184421_add_index_to_redirect_routes.rb index 5991f6ab6a1..9062cf19a73 100644 --- a/db/migrate/20170503184421_add_index_to_redirect_routes.rb +++ b/db/migrate/20170503184421_add_index_to_redirect_routes.rb @@ -1,7 +1,6 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. -# rubocop:disable RemoveIndex class AddIndexToRedirectRoutes < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers @@ -16,7 +15,7 @@ class AddIndexToRedirectRoutes < ActiveRecord::Migration end def down - remove_index(:redirect_routes, :path) if index_exists?(:redirect_routes, :path) - remove_index(:redirect_routes, [:source_type, :source_id]) if index_exists?(:redirect_routes, [:source_type, :source_id]) + 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/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 73fd5b6f90d..73f448d69ed 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -84,6 +84,24 @@ describe UsersController do expect(response).to redirect_to(user) end end + + 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 + get :show, username: 'nonexistent' + expect(response).to have_http_status(404) + 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