diff options
| -rw-r--r-- | app/controllers/concerns/sends_blob.rb | 69 | ||||
| -rw-r--r-- | app/controllers/projects/avatars_controller.rb | 12 | ||||
| -rw-r--r-- | app/controllers/projects/raw_controller.rb | 37 | ||||
| -rw-r--r-- | app/helpers/blob_helper.rb | 22 | ||||
| -rw-r--r-- | app/serializers/diff_file_entity.rb | 1 | ||||
| -rw-r--r-- | changelogs/unreleased/fj-47229-fix-logo-lfs-tracked.yml | 5 | ||||
| -rw-r--r-- | spec/controllers/projects/avatars_controller_spec.rb | 53 | ||||
| -rw-r--r-- | spec/controllers/projects/raw_controller_spec.rb | 101 | ||||
| -rw-r--r-- | spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb | 89 |
9 files changed, 225 insertions, 164 deletions
diff --git a/app/controllers/concerns/sends_blob.rb b/app/controllers/concerns/sends_blob.rb new file mode 100644 index 00000000000..971390d9118 --- /dev/null +++ b/app/controllers/concerns/sends_blob.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +module SendsBlob + extend ActiveSupport::Concern + + included do + include BlobHelper + include SendFileUpload + end + + def send_blob(blob, params = {}) + if blob + headers['X-Content-Type-Options'] = 'nosniff' + + return if cached_blob?(blob) + + if blob.stored_externally? + send_lfs_object(blob) + else + send_git_blob(repository, blob, params) + end + else + render_404 + end + end + + private + + def cached_blob?(blob) + stale = stale?(etag: blob.id) # The #stale? method sets cache headers. + + # Because we are opinionated we set the cache headers ourselves. + response.cache_control[:public] = project.public? + + response.cache_control[:max_age] = + if @ref && @commit && @ref == @commit.id # rubocop:disable Gitlab/ModuleWithInstanceVariables + # This is a link to a commit by its commit SHA. That means that the blob + # is immutable. The only reason to invalidate the cache is if the commit + # was deleted or if the user lost access to the repository. + Blob::CACHE_TIME_IMMUTABLE + else + # A branch or tag points at this blob. That means that the expected blob + # value may change over time. + Blob::CACHE_TIME + end + + response.etag = blob.id + !stale + end + + def send_lfs_object(blob) + lfs_object = find_lfs_object(blob) + + if lfs_object && lfs_object.project_allowed_access?(project) + send_upload(lfs_object.file, attachment: blob.name) + else + render_404 + end + end + + def find_lfs_object(blob) + lfs_object = LfsObject.find_by_oid(blob.lfs_oid) + if lfs_object && lfs_object.file.exists? + lfs_object + else + nil + end + end +end diff --git a/app/controllers/projects/avatars_controller.rb b/app/controllers/projects/avatars_controller.rb index 53fdc5843b5..878c82cd183 100644 --- a/app/controllers/projects/avatars_controller.rb +++ b/app/controllers/projects/avatars_controller.rb @@ -1,24 +1,16 @@ class Projects::AvatarsController < Projects::ApplicationController - include BlobHelper + include SendsBlob before_action :authorize_admin_project!, only: [:destroy] def show @blob = @repository.blob_at_branch(@repository.root_ref, @project.avatar_in_git) - if @blob - headers['X-Content-Type-Options'] = 'nosniff' - return if cached_blob? - - send_git_blob @repository, @blob - else - render_404 - end + send_blob(@blob) end def destroy @project.remove_avatar! - @project.save redirect_to edit_project_path(@project, anchor: 'js-general-project-settings'), status: :found diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb index 1cba0011304..91cf35bc70b 100644 --- a/app/controllers/projects/raw_controller.rb +++ b/app/controllers/projects/raw_controller.rb @@ -1,8 +1,7 @@ # Controller for viewing a file's raw class Projects::RawController < Projects::ApplicationController include ExtractsPath - include BlobHelper - include SendFileUpload + include SendsBlob before_action :require_non_empty_project before_action :assign_ref_vars @@ -10,39 +9,7 @@ class Projects::RawController < Projects::ApplicationController def show @blob = @repository.blob_at(@commit.id, @path) - if @blob - headers['X-Content-Type-Options'] = 'nosniff' - return if cached_blob? - - if @blob.stored_externally? - send_lfs_object - else - send_git_blob @repository, @blob, inline: (params[:inline] != 'false') - end - else - render_404 - end - end - - private - - def send_lfs_object - lfs_object = find_lfs_object - - if lfs_object && lfs_object.project_allowed_access?(@project) - send_upload(lfs_object.file, attachment: @blob.name) - else - render_404 - end - end - - def find_lfs_object - lfs_object = LfsObject.find_by_oid(@blob.lfs_oid) - if lfs_object && lfs_object.file.exists? - lfs_object - else - nil - end + send_blob(@blob, inline: (params[:inline] != 'false')) end end diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index b61cbd5418a..00ebafd177b 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -157,28 +157,6 @@ module BlobHelper end end - def cached_blob? - stale = stale?(etag: @blob.id) # The #stale? method sets cache headers. - - # Because we are opionated we set the cache headers ourselves. - response.cache_control[:public] = @project.public? - - response.cache_control[:max_age] = - if @ref && @commit && @ref == @commit.id - # This is a link to a commit by its commit SHA. That means that the blob - # is immutable. The only reason to invalidate the cache is if the commit - # was deleted or if the user lost access to the repository. - Blob::CACHE_TIME_IMMUTABLE - else - # A branch or tag points at this blob. That means that the expected blob - # value may change over time. - Blob::CACHE_TIME - end - - response.etag = @blob.id - !stale - end - def licenses_for_select return @licenses_for_select if defined?(@licenses_for_select) diff --git a/app/serializers/diff_file_entity.rb b/app/serializers/diff_file_entity.rb index 79844c9210a..d49d4895d89 100644 --- a/app/serializers/diff_file_entity.rb +++ b/app/serializers/diff_file_entity.rb @@ -2,7 +2,6 @@ class DiffFileEntity < Grape::Entity include RequestAwareEntity - include BlobHelper include CommitsHelper include DiffHelper include SubmoduleHelper diff --git a/changelogs/unreleased/fj-47229-fix-logo-lfs-tracked.yml b/changelogs/unreleased/fj-47229-fix-logo-lfs-tracked.yml new file mode 100644 index 00000000000..ed2af81f779 --- /dev/null +++ b/changelogs/unreleased/fj-47229-fix-logo-lfs-tracked.yml @@ -0,0 +1,5 @@ +--- +title: Fixed bug when the project logo file is stored in LFS +merge_request: 20948 +author: +type: fixed diff --git a/spec/controllers/projects/avatars_controller_spec.rb b/spec/controllers/projects/avatars_controller_spec.rb index 17c9a61f339..14059cff74c 100644 --- a/spec/controllers/projects/avatars_controller_spec.rb +++ b/spec/controllers/projects/avatars_controller_spec.rb @@ -1,24 +1,55 @@ require 'spec_helper' describe Projects::AvatarsController do - let(:project) { create(:project, :repository, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) } - let(:user) { create(:user) } + let(:project) { create(:project, :repository) } before do - sign_in(user) - project.add_maintainer(user) controller.instance_variable_set(:@project, project) end - it 'GET #show' do - get :show, namespace_id: project.namespace.id, project_id: project.id + describe 'GET #show' do + subject { get :show, namespace_id: project.namespace, project_id: project } - expect(response).to have_gitlab_http_status(404) + context 'when repository has no avatar' do + it 'shows 404' do + subject + + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when repository has an avatar' do + before do + allow(project).to receive(:avatar_in_git).and_return(filepath) + end + + context 'when the avatar is stored in the repository' do + let(:filepath) { 'files/images/logo-white.png' } + + it 'sends the avatar' do + subject + + expect(response).to have_gitlab_http_status(200) + expect(response.header['Content-Type']).to eq('image/png') + expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:') + end + end + + context 'when the avatar is stored in lfs' do + it_behaves_like 'repository lfs file load' do + let(:filename) { 'lfs_object.iso' } + let(:filepath) { "files/lfs/#{filename}" } + end + end + end end - it 'removes avatar from DB by calling destroy' do - delete :destroy, namespace_id: project.namespace.id, project_id: project.id - expect(project.avatar.present?).to be_falsey - expect(project).to be_valid + describe 'DELETE #destroy' do + it 'removes avatar from DB by calling destroy' do + delete :destroy, namespace_id: project.namespace.id, project_id: project.id + + expect(project.avatar.present?).to be_falsey + expect(project).to be_valid + end end end diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb index c3468536ae1..6b658bf5295 100644 --- a/spec/controllers/projects/raw_controller_spec.rb +++ b/spec/controllers/projects/raw_controller_spec.rb @@ -1,14 +1,21 @@ require 'spec_helper' describe Projects::RawController do - let(:public_project) { create(:project, :public, :repository) } + let(:project) { create(:project, :public, :repository) } + + describe 'GET #show' do + subject do + get(:show, + namespace_id: project.namespace, + project_id: project, + id: filepath) + end - describe '#show' do context 'regular filename' do - let(:id) { 'master/README.md' } + let(:filepath) { 'master/README.md' } it 'delivers ASCII file' do - get_show(public_project, id) + subject expect(response).to have_gitlab_http_status(200) expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8') @@ -19,10 +26,10 @@ describe Projects::RawController do end context 'image header' do - let(:id) { 'master/files/images/6049019_460s.jpg' } + let(:filepath) { 'master/files/images/6049019_460s.jpg' } it 'sets image content type header' do - get_show(public_project, id) + subject expect(response).to have_gitlab_http_status(200) expect(response.header['Content-Type']).to eq('image/jpeg') @@ -30,85 +37,9 @@ describe Projects::RawController do end end - context 'lfs object' do - let(:id) { 'be93687/files/lfs/lfs_object.iso' } - let!(:lfs_object) { create(:lfs_object, oid: '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897', size: '1575078') } - - context 'when lfs is enabled' do - before do - allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(true) - end - - context 'when project has access' do - before do - public_project.lfs_objects << lfs_object - allow_any_instance_of(LfsObjectUploader).to receive(:exists?).and_return(true) - allow(controller).to receive(:send_file) { controller.head :ok } - end - - it 'serves the file' do - expect(controller).to receive(:send_file).with("#{LfsObjectUploader.root}/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: 'lfs_object.iso', disposition: 'attachment') - get_show(public_project, id) - - expect(response).to have_gitlab_http_status(200) - end - - context 'and lfs uses object storage' do - let(:lfs_object) { create(:lfs_object, :with_file, oid: '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897', size: '1575078') } - - before do - stub_lfs_object_storage - lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE) - end - - it 'responds with redirect to file' do - get_show(public_project, id) - - expect(response).to have_gitlab_http_status(302) - expect(response.location).to include(lfs_object.reload.file.path) - end - - it 'sets content disposition' do - get_show(public_project, id) - - file_uri = URI.parse(response.location) - params = CGI.parse(file_uri.query) - - expect(params["response-content-disposition"].first).to eq 'attachment;filename="lfs_object.iso"' - end - end - end - - context 'when project does not have access' do - it 'does not serve the file' do - get_show(public_project, id) - - expect(response).to have_gitlab_http_status(404) - end - end - end - - context 'when lfs is not enabled' do - before do - allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(false) - end - - it 'delivers ASCII file' do - get_show(public_project, id) - - expect(response).to have_gitlab_http_status(200) - expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8') - expect(response.header['Content-Disposition']) - .to eq('inline') - expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:') - end - end + it_behaves_like 'repository lfs file load' do + let(:filename) { 'lfs_object.iso' } + let(:filepath) { "be93687/files/lfs/#{filename}" } end end - - def get_show(project, id) - get(:show, namespace_id: project.namespace.to_param, - project_id: project, - id: id) - end end diff --git a/spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb b/spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb new file mode 100644 index 00000000000..a3d31e26498 --- /dev/null +++ b/spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +# Shared examples for controllers that load and send files from the git repository +# (like Projects::RawController or Projects::AvatarsController) + +# These examples requires the following variables: +# - `project` +# - `filename`: filename of the file +# - `filepath`: path of the file (contains filename) +# - `subject`: the request to be made to the controller. Example: +# subject { get :show, namespace_id: project.namespace, project_id: project } +shared_examples 'repository lfs file load' do + context 'when file is stored in lfs' do + let(:lfs_oid) { '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897' } + let(:lfs_size) { '1575078' } + let!(:lfs_object) { create(:lfs_object, oid: lfs_oid, size: lfs_size) } + + context 'when lfs is enabled' do + before do + allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(true) + end + + context 'when project has access' do + before do + project.lfs_objects << lfs_object + allow_any_instance_of(LfsObjectUploader).to receive(:exists?).and_return(true) + allow(controller).to receive(:send_file) { controller.head :ok } + end + + it 'serves the file' do + expect(controller).to receive(:send_file).with("#{LfsObjectUploader.root}/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: filename, disposition: 'attachment') + + subject + + expect(response).to have_gitlab_http_status(200) + end + + context 'and lfs uses object storage' do + let(:lfs_object) { create(:lfs_object, :with_file, oid: lfs_oid, size: lfs_size) } + + before do + stub_lfs_object_storage + lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE) + end + + it 'responds with redirect to file' do + subject + + expect(response).to have_gitlab_http_status(302) + expect(response.location).to include(lfs_object.reload.file.path) + end + + it 'sets content disposition' do + subject + + file_uri = URI.parse(response.location) + params = CGI.parse(file_uri.query) + + expect(params["response-content-disposition"].first).to eq "attachment;filename=\"#{filename}\"" + end + end + end + + context 'when project does not have access' do + it 'does not serve the file' do + subject + + expect(response).to have_gitlab_http_status(404) + end + end + end + + context 'when lfs is not enabled' do + before do + allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(false) + end + + it 'delivers ASCII file' do + subject + + expect(response).to have_gitlab_http_status(200) + expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8') + expect(response.header['Content-Disposition']) + .to eq('inline') + expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:') + end + end + end +end |