Revert "Merge branch 'remove-unnecessary-sql-queries-from-avatar-requests' into 'master'"

This reverts merge request !19383

app/controllers/application_controller.rb

@@ -20,11 +20,11 @@ class ApplicationController < ActionController::Base
   before_action :authenticate_user!
   before_action :enforce_terms!, if: :should_enforce_terms?
   before_action :validate_user_service_ticket!
-  before_action :check_password_expiration, if: :html_request?
+  before_action :check_password_expiration
   before_action :ldap_security_check
   before_action :sentry_context
   before_action :default_headers
-  before_action :add_gon_variables, if: :html_request?
+  before_action :add_gon_variables, unless: [:peek_request?, :json_request?]
   before_action :configure_permitted_parameters, if: :devise_controller?
   before_action :require_email, unless: :devise_controller?
   before_action :active_user_check, unless: :devise_controller?
@@ -449,8 +449,8 @@ class ApplicationController < ActionController::Base
     response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
   end
 
-  def html_request?
-    request.format.html?
+  def peek_request?
+    request.path.start_with?('/-/peek')
   end
 
   def json_request?
@@ -460,7 +460,7 @@ class ApplicationController < ActionController::Base
   def should_enforce_terms?
     return false unless Gitlab::CurrentSettings.current_application_settings.enforce_terms
 
-    html_request? && !devise_controller?
+    !(peek_request? || devise_controller?)
   end
 
   def set_usage_stats_consent_flag

app/controllers/concerns/confirm_email_warning.rb

@@ -4,18 +4,15 @@ module ConfirmEmailWarning
   extend ActiveSupport::Concern
 
   included do
-    before_action :set_confirm_warning, if: :show_confirm_warning?
+    before_action :set_confirm_warning, if: -> { Feature.enabled?(:soft_email_confirmation) }
   end
 
   protected
 
-  def show_confirm_warning?
-    html_request? && request.get? && Feature.enabled?(:soft_email_confirmation)
-  end
-
   def set_confirm_warning
     return unless current_user
     return if current_user.confirmed?
+    return if peek_request? || json_request? || !request.get?
 
     email = current_user.unconfirmed_email || current_user.email
 

app/controllers/concerns/uploads_actions.rb

 # frozen_string_literal: true
 
 module UploadsActions
-  extend ActiveSupport::Concern
   include Gitlab::Utils::StrongMemoize
   include SendFileUpload
 
   UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze
 
-  included do
-    prepend_before_action :set_request_format_from_path_extension
-  end
-
   def create
     uploader = UploadService.new(model, params[:file], uploader_class).execute
 
@@ -69,18 +64,6 @@ module UploadsActions
 
   private
 
-  # From ActionDispatch::Http::MimeNegotiation. We have an initializer that
-  # monkey-patches this method out (so that repository paths don't guess a
-  # format based on extension), but we do want this behaviour when serving
-  # uploads.
-  def set_request_format_from_path_extension
-    path = request.headers['action_dispatch.original_path'] || request.headers['PATH_INFO']
-
-    if match = path&.match(/\.(\w+)\z/)
-      request.format = match.captures.first
-    end
-  end
-
   def uploader_class
     raise NotImplementedError
   end

app/controllers/uploads_controller.rb

@@ -20,7 +20,7 @@ class UploadsController < ApplicationController
 
   skip_before_action :authenticate_user!
   before_action :upload_mount_satisfied?
-  before_action :model
+  before_action :find_model
   before_action :authorize_access!, only: [:show]
   before_action :authorize_create_access!, only: [:create, :authorize]
   before_action :verify_workhorse_api!, only: [:authorize]

spec/controllers/application_controller_spec.rb

@@ -90,16 +90,18 @@ describe ApplicationController do
       let(:format) { :html }
 
       it_behaves_like 'setting gon variables'
-    end
 
-    context 'with json format' do
-      let(:format) { :json }
+      context 'for peek requests' do
+        before do
+          request.path = '/-/peek'
+        end
 
-      it_behaves_like 'not setting gon variables'
+        it_behaves_like 'not setting gon variables'
+      end
     end
 
-    context 'with atom format' do
-      let(:format) { :atom }
+    context 'with json format' do
+      let(:format) { :json }
 
       it_behaves_like 'not setting gon variables'
     end

spec/controllers/uploads_controller_spec.rb

@@ -228,10 +228,10 @@ describe UploadsController do
             user.block
           end
 
-          it "responds with status 401" do
+          it "redirects to the sign in page" do
             get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" }
 
-            expect(response).to have_gitlab_http_status(401)
+            expect(response).to redirect_to(new_user_session_path)
           end
         end
 
@@ -320,10 +320,10 @@ describe UploadsController do
         end
 
         context "when not signed in" do
-          it "responds with status 401" do
+          it "redirects to the sign in page" do
             get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
 
-            expect(response).to have_gitlab_http_status(401)
+            expect(response).to redirect_to(new_user_session_path)
           end
         end
 
@@ -343,10 +343,10 @@ describe UploadsController do
                 project.add_maintainer(user)
               end
 
-              it "responds with status 401" do
+              it "redirects to the sign in page" do
                 get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
 
-                expect(response).to have_gitlab_http_status(401)
+                expect(response).to redirect_to(new_user_session_path)
               end
             end
 
@@ -439,10 +439,10 @@ describe UploadsController do
                 user.block
               end
 
-              it "responds with status 401" do
+              it "redirects to the sign in page" do
                 get :show, params: { model: "group", mounted_as: "avatar", id: group.id, filename: "dk.png" }
 
-                expect(response).to have_gitlab_http_status(401)
+                expect(response).to redirect_to(new_user_session_path)
               end
             end
 
@@ -526,10 +526,10 @@ describe UploadsController do
         end
 
         context "when not signed in" do
-          it "responds with status 401" do
+          it "redirects to the sign in page" do
             get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
 
-            expect(response).to have_gitlab_http_status(401)
+            expect(response).to redirect_to(new_user_session_path)
           end
         end
 
@@ -549,10 +549,10 @@ describe UploadsController do
                 project.add_maintainer(user)
               end
 
-              it "responds with status 401" do
+              it "redirects to the sign in page" do
                 get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
 
-                expect(response).to have_gitlab_http_status(401)
+                expect(response).to redirect_to(new_user_session_path)
               end
             end
 

spec/requests/user_avatar_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe 'Loading a user avatar' do
-  let(:user) { create(:user, :with_avatar) }
-
-  context 'when logged in' do
-    # The exact query count will vary depending on the 2FA settings of the
-    # instance, group, and user. Removing those extra 2FA queries in this case
-    # may not be a good idea, so we just set up the ideal case.
-    before do
-      stub_application_setting(require_two_factor_authentication: true)
-
-      login_as(create(:user, :two_factor))
-    end
-
-    # One each for: current user, avatar user, and upload record
-    it 'only performs three SQL queries' do
-      get user.avatar_url # Skip queries on first application load
-
-      expect(response).to have_gitlab_http_status(200)
-      expect { get user.avatar_url }.not_to exceed_query_limit(3)
-    end
-  end
-
-  context 'when logged out' do
-    # One each for avatar user and upload record
-    it 'only performs two SQL queries' do
-      get user.avatar_url # Skip queries on first application load
-
-      expect(response).to have_gitlab_http_status(200)
-      expect { get user.avatar_url }.not_to exceed_query_limit(2)
-    end
-  end
-end