Merge branch 'always-proxy-reports' into 'master'

backport: Always proxy reports downloads

See merge request gitlab-org/gitlab-ce!22889

app/controllers/concerns/send_file_upload.rb

 # frozen_string_literal: true
 
 module SendFileUpload
-  def send_upload(file_upload, send_params: {}, redirect_params: {}, attachment: nil, disposition: 'attachment')
+  def send_upload(file_upload, send_params: {}, redirect_params: {}, attachment: nil, proxy: false, disposition: 'attachment')
     if attachment
       # Response-Content-Type will not override an existing Content-Type in
       # Google Cloud Storage, so the metadata needs to be cleared on GCS for
@@ -17,7 +17,7 @@ module SendFileUpload
 
     if file_upload.file_storage?
       send_file file_upload.path, send_params
-    elsif file_upload.class.proxy_download_enabled?
+    elsif file_upload.class.proxy_download_enabled? || proxy
       headers.store(*Gitlab::Workhorse.send_url(file_upload.url(**redirect_params)))
       head :ok
     else

app/controllers/projects/artifacts_controller.rb

@@ -16,7 +16,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
   def download
     return render_404 unless artifacts_file
 
-    send_upload(artifacts_file, attachment: artifacts_file.filename)
+    send_upload(artifacts_file, attachment: artifacts_file.filename, proxy: params[:proxy])
   end
 
   def browse

spec/controllers/concerns/send_file_upload_spec.rb

@@ -28,8 +28,9 @@ describe SendFileUpload do
   describe '#send_upload' do
     let(:controller) { controller_class.new }
     let(:temp_file) { Tempfile.new('test') }
+    let(:params) { {} }
 
-    subject { controller.send_upload(uploader) }
+    subject { controller.send_upload(uploader, **params) }
 
     before do
       FileUtils.touch(temp_file)
@@ -52,7 +53,7 @@ describe SendFileUpload do
     end
 
     context 'with attachment' do
-      let(:send_attachment) { controller.send_upload(uploader, attachment: 'test.js') }
+      let(:params) { { attachment: 'test.js' } }
 
       it 'sends a file with content-type of text/plain' do
         expected_params = {
@@ -62,7 +63,7 @@ describe SendFileUpload do
         }
         expect(controller).to receive(:send_file).with(uploader.path, expected_params)
 
-        send_attachment
+        subject
       end
 
       context 'with a proxied file in object storage' do
@@ -83,7 +84,7 @@ describe SendFileUpload do
           expect(controller).to receive(:headers) { headers }
           expect(controller).to receive(:head).with(:ok)
 
-          send_attachment
+          subject
         end
       end
     end
@@ -95,11 +96,7 @@ describe SendFileUpload do
         uploader.store!(temp_file)
       end
 
-      context 'and proxying is enabled' do
-        before do
-          allow(Gitlab.config.uploads.object_store).to receive(:proxy_download) { true }
-        end
-
+      shared_examples 'proxied file' do
         it 'sends a file' do
           headers = double
           expect(Gitlab::Workhorse).not_to receive(:send_url).with(/response-content-disposition/)
@@ -115,6 +112,14 @@ describe SendFileUpload do
         end
       end
 
+      context 'and proxying is enabled' do
+        before do
+          allow(Gitlab.config.uploads.object_store).to receive(:proxy_download) { true }
+        end
+
+        it_behaves_like 'proxied file'
+      end
+
       context 'and proxying is disabled' do
         before do
           allow(Gitlab.config.uploads.object_store).to receive(:proxy_download) { false }
@@ -125,6 +130,12 @@ describe SendFileUpload do
 
           subject
         end
+
+        context 'with proxy requested' do
+          let(:params) { { proxy: true } }
+
+          it_behaves_like 'proxied file'
+        end
       end
     end
   end

spec/controllers/projects/artifacts_controller_spec.rb

@@ -47,14 +47,37 @@ describe Projects::ArtifactsController do
       context 'when codequality file type is supplied' do
         let(:file_type) { 'codequality' }
 
-        before do
-          create(:ci_job_artifact, :codequality, job: job)
+        context 'when file is stored locally' do
+          before do
+            create(:ci_job_artifact, :codequality, job: job)
+          end
+
+          it 'sends the codequality report' do
+            expect(controller).to receive(:send_file).with(job.job_artifacts_codequality.file.path, hash_including(disposition: 'attachment')).and_call_original
+
+            download_artifact(file_type: file_type)
+          end
         end
 
-        it 'sends the codequality report' do
-          expect(controller).to receive(:send_file).with(job.job_artifacts_codequality.file.path, hash_including(disposition: 'attachment')).and_call_original
+        context 'when file is stored remotely' do
+          before do
+            stub_artifacts_object_storage
+            create(:ci_job_artifact, :remote_store, :codequality, job: job)
+          end
+
+          it 'sends the codequality report' do
+            expect(controller).to receive(:redirect_to).and_call_original
 
-          download_artifact(file_type: file_type)
+            download_artifact(file_type: file_type)
+          end
+
+          context 'when proxied' do
+            it 'sends the codequality report' do
+              expect(Gitlab::Workhorse).to receive(:send_url).and_call_original
+
+              download_artifact(file_type: file_type, proxy: true)
+            end
+          end
         end
       end
     end