From 7849683766e93cfd91e0c864f3deb08500ea35d9 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 7 Mar 2017 18:57:30 +0100 Subject: [PATCH] Do not show LFS object when LFS is disabled Do not display a 404, when a user tries to retrieve the raw content of an LFS file (pointer) if the config option "lfs_enabled" is set to false. Instead, display the LFS pointer file directly. --- app/controllers/projects/raw_controller.rb | 2 +- app/models/blob.rb | 8 ++- app/views/projects/blob/_blob.html.haml | 2 +- changelogs/unreleased/feature-custom-lfs.yml | 4 ++ features/steps/project/source/browse_files.rb | 1 + .../projects/raw_controller_spec.rb | 67 +++++++++++++------ .../projects/files/browse_files_spec.rb | 14 +++- spec/models/blob_spec.rb | 25 ++++--- 8 files changed, 86 insertions(+), 37 deletions(-) create mode 100644 changelogs/unreleased/feature-custom-lfs.yml diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb index 10d24da16d7..c55b37ae0dd 100644 --- a/app/controllers/projects/raw_controller.rb +++ b/app/controllers/projects/raw_controller.rb @@ -15,7 +15,7 @@ class Projects::RawController < Projects::ApplicationController return if cached_blob? - if @blob.lfs_pointer? + if @blob.lfs_pointer? && project.lfs_enabled? send_lfs_object else send_git_blob @repository, @blob diff --git a/app/models/blob.rb b/app/models/blob.rb index ab92e820335..1376b86fdad 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -54,9 +54,13 @@ class Blob < SimpleDelegator UploaderHelper::VIDEO_EXT.include?(extname.downcase.delete('.')) end - def to_partial_path + def to_partial_path(project) if lfs_pointer? - 'download' + if project.lfs_enabled? + 'download' + else + 'text' + end elsif image? || svg? 'image' elsif text? diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml index 24ff74ecb3b..bf8801bb1e3 100644 --- a/app/views/projects/blob/_blob.html.haml +++ b/app/views/projects/blob/_blob.html.haml @@ -33,4 +33,4 @@ = number_to_human_size(blob_size(blob)) .file-actions.hidden-xs = render "actions" - = render blob, blob: blob + = render blob.to_partial_path(@project), blob: blob diff --git a/changelogs/unreleased/feature-custom-lfs.yml b/changelogs/unreleased/feature-custom-lfs.yml new file mode 100644 index 00000000000..ec968386a6f --- /dev/null +++ b/changelogs/unreleased/feature-custom-lfs.yml @@ -0,0 +1,4 @@ +--- +title: Do not show LFS object when LFS is disabled +merge_request: 9779 +author: Christopher Bartz diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb index e84cd9193da..6845f75f22f 100644 --- a/features/steps/project/source/browse_files.rb +++ b/features/steps/project/source/browse_files.rb @@ -337,6 +337,7 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps end step 'I click on "files/lfs/lfs_object.iso" file in repo' do + allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(true) visit namespace_project_tree_path(@project.namespace, @project, "lfs") click_link 'files' click_link "lfs" diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb index 4cebe3884bf..952071af57f 100644 --- a/spec/controllers/projects/raw_controller_spec.rb +++ b/spec/controllers/projects/raw_controller_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Projects::RawController do let(:public_project) { create(:project, :public, :repository) } - describe "#show" do + describe '#show' do context 'regular filename' do let(:id) { 'master/README.md' } @@ -16,8 +16,8 @@ describe Projects::RawController do expect(response).to have_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:") + to eq('inline') + expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:') end end @@ -32,7 +32,7 @@ describe Projects::RawController do expect(response).to have_http_status(200) expect(response.header['Content-Type']).to eq('image/jpeg') - expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("git-blob:") + expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:') end end @@ -40,32 +40,57 @@ describe Projects::RawController do let(:id) { 'be93687/files/lfs/lfs_object.iso' } let!(:lfs_object) { create(:lfs_object, oid: '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897', size: '1575078') } - context 'when project has access' do + context 'when lfs is enabled' 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 } + allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(true) end - it 'serves the file' do - expect(controller).to receive(:send_file).with("#{Gitlab.config.shared.path}/lfs-objects/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: "lfs_object.iso", disposition: 'attachment') + 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("#{Gitlab.config.shared.path}/lfs-objects/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: 'lfs_object.iso', disposition: 'attachment') + get(:show, + namespace_id: public_project.namespace.to_param, + project_id: public_project, + id: id) + + expect(response).to have_http_status(200) + end + end + + context 'when project does not have access' do + it 'does not serve the file' do + get(:show, + namespace_id: public_project.namespace.to_param, + project_id: public_project, + id: id) + + expect(response).to have_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, namespace_id: public_project.namespace.to_param, project_id: public_project, id: id) expect(response).to have_http_status(200) - end - end - - context 'when project does not have access' do - it 'does not serve the file' do - get(:show, - namespace_id: public_project.namespace.to_param, - project_id: public_project, - id: id) - - expect(response).to have_http_status(404) + 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 diff --git a/spec/features/projects/files/browse_files_spec.rb b/spec/features/projects/files/browse_files_spec.rb index 69295e450d0..d281043caa3 100644 --- a/spec/features/projects/files/browse_files_spec.rb +++ b/spec/features/projects/files/browse_files_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -feature 'user checks git blame', feature: true do +feature 'user browses project', feature: true do let(:project) { create(:project) } let(:user) { create(:user) } @@ -18,4 +18,16 @@ feature 'user checks git blame', feature: true do expect(page).to have_content "Dmitriy Zaporozhets" expect(page).to have_content "Initial commit" end + + scenario 'can see raw content of LFS pointer with LFS disabled' do + allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(false) + click_link 'files' + click_link 'lfs' + click_link 'lfs_object.iso' + + expect(page).not_to have_content 'Download (1.5 MB)' + expect(page).to have_content 'version https://git-lfs.github.com/spec/v1' + expect(page).to have_content 'oid sha256:91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897' + expect(page).to have_content 'size 1575078' + end end diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb index 03d02b4d382..94c25a454aa 100644 --- a/spec/models/blob_spec.rb +++ b/spec/models/blob_spec.rb @@ -70,6 +70,8 @@ describe Blob do end describe '#to_partial_path' do + let(:project) { double(lfs_enabled?: true) } + def stubbed_blob(overrides = {}) overrides.reverse_merge!( image?: false, @@ -84,34 +86,35 @@ describe Blob do end end - it 'handles LFS pointers' do - blob = stubbed_blob(lfs_pointer?: true) + it 'handles LFS pointers with LFS enabled' do + blob = stubbed_blob(lfs_pointer?: true, text?: true) + expect(blob.to_partial_path(project)).to eq 'download' + end - expect(blob.to_partial_path).to eq 'download' + it 'handles LFS pointers with LFS disabled' do + blob = stubbed_blob(lfs_pointer?: true, text?: true) + project = double(lfs_enabled?: false) + expect(blob.to_partial_path(project)).to eq 'text' end it 'handles SVGs' do blob = stubbed_blob(text?: true, svg?: true) - - expect(blob.to_partial_path).to eq 'image' + expect(blob.to_partial_path(project)).to eq 'image' end it 'handles images' do blob = stubbed_blob(image?: true) - - expect(blob.to_partial_path).to eq 'image' + expect(blob.to_partial_path(project)).to eq 'image' end it 'handles text' do blob = stubbed_blob(text?: true) - - expect(blob.to_partial_path).to eq 'text' + expect(blob.to_partial_path(project)).to eq 'text' end it 'defaults to download' do blob = stubbed_blob - - expect(blob.to_partial_path).to eq 'download' + expect(blob.to_partial_path(project)).to eq 'download' end end