diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 85602aa72deb31eb32e58462010e51ad4816b259..13b226a6a7106ac4766e4aeafb2b7fbd6aaf120f 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -20,11 +20,11 @@ class ApplicationController < ActionController::Base
before_action :authenticate_user!, except: [:route_not_found]
before_action :enforce_terms!, if: :should_enforce_terms?
before_action :validate_user_service_ticket!
- before_action :check_password_expiration
+ before_action :check_password_expiration, if: :html_request?
before_action :ldap_security_check
before_action :sentry_context
before_action :default_headers
- before_action :add_gon_variables, unless: [:peek_request?, :json_request?]
+ before_action :add_gon_variables, if: :html_request?
before_action :configure_permitted_parameters, if: :devise_controller?
before_action :require_email, unless: :devise_controller?
before_action :active_user_check, unless: :devise_controller?
@@ -455,8 +455,8 @@ def set_page_title_header
response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
end
- def peek_request?
- request.path.start_with?('/-/peek')
+ def html_request?
+ request.format.html?
end
def json_request?
@@ -466,7 +466,7 @@ def json_request?
def should_enforce_terms?
return false unless Gitlab::CurrentSettings.current_application_settings.enforce_terms
- !(peek_request? || devise_controller?)
+ html_request? && !devise_controller?
end
def set_usage_stats_consent_flag
diff --git a/app/controllers/concerns/confirm_email_warning.rb b/app/controllers/concerns/confirm_email_warning.rb
index 86df0010665367e70f473ce63d48efd1bc4c82d7..32e1a46e580ffee8ea3364089620cf23ae1ab541 100644
--- a/app/controllers/concerns/confirm_email_warning.rb
+++ b/app/controllers/concerns/confirm_email_warning.rb
@@ -4,15 +4,18 @@ module ConfirmEmailWarning
extend ActiveSupport::Concern
included do
- before_action :set_confirm_warning, if: -> { Feature.enabled?(:soft_email_confirmation) }
+ before_action :set_confirm_warning, if: :show_confirm_warning?
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
diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb
index b87779c22d316da2f2f01f8573228a263fd59621..023c41821da1bf20d252d6c7cbcadd709e0aa662 100644
--- a/app/controllers/concerns/uploads_actions.rb
+++ b/app/controllers/concerns/uploads_actions.rb
@@ -1,11 +1,16 @@
# 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
@@ -64,6 +69,18 @@ def authorize
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
diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index bb7f5ec2b28fa1b6f9d3ca2a97138eca6b2f3ad3..7893b0812c77f68c151935ce911fc609bda41853 100644
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -90,14 +90,6 @@ def index
let(:format) { :html }
it_behaves_like 'setting gon variables'
-
- context 'for peek requests' do
- before do
- request.path = '/-/peek'
- end
-
- it_behaves_like 'not setting gon variables'
- end
end
context 'with json format' do
@@ -105,6 +97,12 @@ def index
it_behaves_like 'not setting gon variables'
end
+
+ context 'with atom format' do
+ let(:format) { :atom }
+
+ it_behaves_like 'not setting gon variables'
+ end
end
describe 'session expiration' do
diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb
index 1bcf3bb106b86c342a4734cd17e5ba3c4cc08368..f35babc1b56361025552e4d1252cb2106ebff5f8 100644
--- a/spec/controllers/uploads_controller_spec.rb
+++ b/spec/controllers/uploads_controller_spec.rb
@@ -228,10 +228,10 @@
user.block
end
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
@@ -320,10 +320,10 @@
end
context "when not signed in" do
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
@@ -343,10 +343,10 @@
project.add_maintainer(user)
end
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
@@ -439,10 +439,10 @@
user.block
end
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "group", mounted_as: "avatar", id: group.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
@@ -526,10 +526,10 @@
end
context "when not signed in" do
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
@@ -549,10 +549,10 @@
project.add_maintainer(user)
end
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
diff --git a/spec/requests/user_avatar_spec.rb b/spec/requests/user_avatar_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..9451674161c9dd376da7e5bcf90cb9caa2b3d76f
--- /dev/null
+++ b/spec/requests/user_avatar_spec.rb
@@ -0,0 +1,36 @@
+# 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