Commit ba52bfa9 authored by Sean McGivern's avatar Sean McGivern

Revert "Merge branch 'optimise-avatar-requests-part-two' into 'master'"

This reverts merge request !19822
parent a7e2d31e
...@@ -20,11 +20,11 @@ class ApplicationController < ActionController::Base ...@@ -20,11 +20,11 @@ class ApplicationController < ActionController::Base
before_action :authenticate_user!, except: [:route_not_found] before_action :authenticate_user!, except: [:route_not_found]
before_action :enforce_terms!, if: :should_enforce_terms? before_action :enforce_terms!, if: :should_enforce_terms?
before_action :validate_user_service_ticket! 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 :ldap_security_check
before_action :sentry_context before_action :sentry_context
before_action :default_headers 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 :configure_permitted_parameters, if: :devise_controller?
before_action :require_email, unless: :devise_controller? before_action :require_email, unless: :devise_controller?
before_action :active_user_check, unless: :devise_controller? before_action :active_user_check, unless: :devise_controller?
...@@ -455,8 +455,8 @@ class ApplicationController < ActionController::Base ...@@ -455,8 +455,8 @@ class ApplicationController < ActionController::Base
response.headers['Page-Title'] = URI.escape(page_title('GitLab')) response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
end end
def html_request? def peek_request?
request.format.html? request.path.start_with?('/-/peek')
end end
def json_request? def json_request?
...@@ -466,7 +466,7 @@ class ApplicationController < ActionController::Base ...@@ -466,7 +466,7 @@ class ApplicationController < ActionController::Base
def should_enforce_terms? def should_enforce_terms?
return false unless Gitlab::CurrentSettings.current_application_settings.enforce_terms return false unless Gitlab::CurrentSettings.current_application_settings.enforce_terms
html_request? && !devise_controller? !(peek_request? || devise_controller?)
end end
def set_usage_stats_consent_flag def set_usage_stats_consent_flag
......
...@@ -4,18 +4,15 @@ module ConfirmEmailWarning ...@@ -4,18 +4,15 @@ module ConfirmEmailWarning
extend ActiveSupport::Concern extend ActiveSupport::Concern
included do included do
before_action :set_confirm_warning, if: :show_confirm_warning? before_action :set_confirm_warning, if: -> { Feature.enabled?(:soft_email_confirmation) }
end end
protected protected
def show_confirm_warning?
html_request? && request.get? && Feature.enabled?(:soft_email_confirmation)
end
def set_confirm_warning def set_confirm_warning
return unless current_user return unless current_user
return if current_user.confirmed? return if current_user.confirmed?
return if peek_request? || json_request? || !request.get?
email = current_user.unconfirmed_email || current_user.email email = current_user.unconfirmed_email || current_user.email
......
# frozen_string_literal: true # frozen_string_literal: true
module UploadsActions module UploadsActions
extend ActiveSupport::Concern
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
include SendFileUpload include SendFileUpload
UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze 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 def create
uploader = UploadService.new(model, params[:file], uploader_class).execute uploader = UploadService.new(model, params[:file], uploader_class).execute
...@@ -69,18 +64,6 @@ module UploadsActions ...@@ -69,18 +64,6 @@ module UploadsActions
private 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 def uploader_class
raise NotImplementedError raise NotImplementedError
end end
......
...@@ -20,7 +20,7 @@ class UploadsController < ApplicationController ...@@ -20,7 +20,7 @@ class UploadsController < ApplicationController
skip_before_action :authenticate_user! skip_before_action :authenticate_user!
before_action :upload_mount_satisfied? before_action :upload_mount_satisfied?
before_action :model before_action :find_model
before_action :authorize_access!, only: [:show] before_action :authorize_access!, only: [:show]
before_action :authorize_create_access!, only: [:create, :authorize] before_action :authorize_create_access!, only: [:create, :authorize]
before_action :verify_workhorse_api!, only: [:authorize] before_action :verify_workhorse_api!, only: [:authorize]
......
...@@ -90,16 +90,18 @@ describe ApplicationController do ...@@ -90,16 +90,18 @@ describe ApplicationController do
let(:format) { :html } let(:format) { :html }
it_behaves_like 'setting gon variables' it_behaves_like 'setting gon variables'
end
context 'with json format' do context 'for peek requests' do
let(:format) { :json } before do
request.path = '/-/peek'
end
it_behaves_like 'not setting gon variables' it_behaves_like 'not setting gon variables'
end
end end
context 'with atom format' do context 'with json format' do
let(:format) { :atom } let(:format) { :json }
it_behaves_like 'not setting gon variables' it_behaves_like 'not setting gon variables'
end end
......
...@@ -228,10 +228,10 @@ describe UploadsController do ...@@ -228,10 +228,10 @@ describe UploadsController do
user.block user.block
end 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" } 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
end end
...@@ -320,10 +320,10 @@ describe UploadsController do ...@@ -320,10 +320,10 @@ describe UploadsController do
end end
context "when not signed in" do 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" } 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
end end
...@@ -343,10 +343,10 @@ describe UploadsController do ...@@ -343,10 +343,10 @@ describe UploadsController do
project.add_maintainer(user) project.add_maintainer(user)
end 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" } 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
end end
...@@ -439,10 +439,10 @@ describe UploadsController do ...@@ -439,10 +439,10 @@ describe UploadsController do
user.block user.block
end 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" } 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
end end
...@@ -526,10 +526,10 @@ describe UploadsController do ...@@ -526,10 +526,10 @@ describe UploadsController do
end end
context "when not signed in" do 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" } 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
end end
...@@ -549,10 +549,10 @@ describe UploadsController do ...@@ -549,10 +549,10 @@ describe UploadsController do
project.add_maintainer(user) project.add_maintainer(user)
end 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" } 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
end end
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment