Put publish of attachements behind flag

Since this is a major change that could prevent status page
from publishing, put it behind a feature flag. Remove
helpers in favor of more descriptive class names. Return
an error response

app/finders/uploader_finder.rb

@@ -16,11 +16,8 @@ class UploaderFinder
     retrieve_file_state!
 
     uploader
-  rescue ::Gitlab::Utils::PathTraversalAttackError, StandardError => e
-    # Send any unexpected errors to sentry and return nil
-    Raven.capture_exception(e) unless e.is_a?(::Gitlab::Utils::PathTraversalAttackError)
-
-    nil
+  rescue ::Gitlab::Utils::PathTraversalAttackError
+    nil # no-op if for incorrect files
   end
 
   def prevent_path_traversal_attack!

ee/app/services/status_page/publication_service_helpers.rb → ee/app/services/concerns/status_page/publication_service_responses.rb

 # frozen_string_literal: true
 
 module StatusPage
-  module PublicationServiceHelpers
+  module PublicationServiceResponses
+    extend ActiveSupport::Concern
+
     def error(message, payload = {})
       ServiceResponse.error(message: message, payload: payload)
     end

ee/app/services/status_page/publish_attachments_service.rb

@@ -4,8 +4,8 @@ module StatusPage
   # Publishes Attachments from incident comments and descriptions to s3
   # Should only be called from publish details or a service that inherits from the publish_base_service
   class PublishAttachmentsService
-    include Gitlab::Utils::StrongMemoize
-    include StatusPage::PublicationServiceHelpers
+    include ::Gitlab::Utils::StrongMemoize
+    include ::StatusPage::PublicationServiceResponses
 
     def initialize(project:, issue:, user_notes:, storage_client:)
       @project = project
@@ -13,12 +13,15 @@ module StatusPage
       @user_notes = user_notes
       @storage_client = storage_client
       @total_uploads = existing_keys.size
+      @has_errors = false
     end
 
     def execute
       publish_description_attachments
       publish_user_note_attachments
 
+      return file_upload_error if @has_errors
+
       success
     end
 
@@ -60,10 +63,11 @@ module StatusPage
         storage_client.multipart_upload(key, open_file)
         @total_uploads += 1
       end
-    rescue Error => e
-      # Continue uploading other files if one fails
-      # But report the failure to Sentry
-      Raven.capture_exception(e)
+    rescue StatusPage::Storage::Error => e
+      # In production continue uploading other files if one fails But report the failure to Sentry
+      # raise errors in development and test
+      @has_errors = true
+      Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
     end
 
     def existing_keys
@@ -84,5 +88,9 @@ module StatusPage
       # Uploader object behaves like a file with an 'open' method
       UploaderFinder.new(project, secret, file_name).execute
     end
+
+    def file_upload_error
+      error('One or more files did not upload properly')
+    end
   end
 end

ee/app/services/status_page/publish_base_service.rb

@@ -3,7 +3,7 @@
 module StatusPage
   class PublishBaseService
     include Gitlab::Utils::StrongMemoize
-    include StatusPage::PublicationServiceHelpers
+    include StatusPage::PublicationServiceResponses
 
     def initialize(project:)
       @project = project

ee/app/services/status_page/publish_details_service.rb

@@ -14,7 +14,8 @@ module StatusPage
       response = publish_json(issue, user_notes)
       return response if response.error?
 
-      publish_attachments(issue, user_notes)
+      response = publish_attachments(issue, user_notes)
+      return response if response.error?
 
       success
     end
@@ -39,6 +40,8 @@ module StatusPage
     end
 
     def publish_attachments(issue, user_notes)
+      return success unless attachements_enabled?
+
       StatusPage::PublishAttachmentsService.new(
         project: @project,
         issue: issue,
@@ -46,5 +49,9 @@ module StatusPage
         storage_client: storage_client
       ).execute
     end
+
+    def attachements_enabled?
+      Feature.enabled?(:status_page_attachments, @project, default_enabled: true)
+    end
   end
 end

ee/lib/status_page/storage/s3_client.rb

@@ -4,7 +4,7 @@ module StatusPage
   module Storage
     # Implements a minimal AWS S3 client.
     class S3Client
-      include StatusPage::Storage::S3Helpers
+      include StatusPage::Storage::WrapsStorageErrors
 
       def initialize(region:, bucket_name:, access_key_id:, secret_access_key:)
         @bucket_name = bucket_name

ee/lib/status_page/storage/s3_multipart_upload.rb

@@ -4,7 +4,7 @@ module StatusPage
   module Storage
     # Implements multipart upload in s3
     class S3MultipartUpload
-      include StatusPage::Storage::S3Helpers
+      include StatusPage::Storage::WrapsStorageErrors
       # 5 megabytes is the minimum part size specified in the amazon SDK
       MULTIPART_UPLOAD_PART_SIZE = 5.megabytes
 
@@ -28,7 +28,7 @@ module StatusPage
           begin
             parts = upload_part(upload_id)
             complete_upload(upload_id, parts)
-            # Rescue on Exception since even on keyboard inturrupt we want to abort the upload and re-raise
+            # Rescue on Exception since even on keyboard interrupt we want to abort the upload and re-raise
             # abort clears the already uploaded parts so that they do not cost the bucket owner
             # The status page bucket lifecycle policy will clear out unaborted parts if
             # this fails without an exception (power failures etc.)

ee/lib/status_page/storage/s3_helpers.rb → ee/lib/status_page/storage/wraps_storage_errors.rb

@@ -2,7 +2,7 @@
 
 module StatusPage
   module Storage
-    module S3Helpers
+    module WrapsStorageErrors
       def wrap_errors(**args)
         yield
       rescue Aws::Errors::ServiceError => e

ee/spec/lib/status_page/storage/s3_multipart_upload_spec.rb

@@ -15,18 +15,21 @@ describe StatusPage::Storage::S3MultipartUpload, :aws_s3 do
     )
   end
 
-  describe 'multipart_upload' do
+  describe '#call' do
     let(:key) { '123' }
-    let(:file) { Tempfile.new('foo') }
+    let(:file) do
+      Tempfile.new('foo').tap do |file|
+        file.open
+        file.write('hello world')
+        file.rewind
+      end
+    end
+
     let(:upload_id) { '123456789' }
 
     subject(:result) { described_class.new(client: s3_client, bucket_name: bucket_name, key: key, open_file: file).call }
 
     before do
-      file.open
-      file.write('hello world')
-      file.rewind
-
       stub_responses(
         :create_multipart_upload,
         instance_double(Aws::S3::Types::CreateMultipartUploadOutput, { to_h: { upload_id: upload_id } })
@@ -74,9 +77,10 @@ describe StatusPage::Storage::S3MultipartUpload, :aws_s3 do
           stub_responses(:upload_part, aws_error)
         end
 
-        it 'raises an error' do
-          expect(s3_client).to receive(:abort_multipart_upload)
+        it 'aborts the upload and raises an error' do
           msg = error_message(aws_error, key: key)
+
+          expect(s3_client).to receive(:abort_multipart_upload)
           expect { result }.to raise_error(StatusPage::Storage::Error, msg)
         end
       end
@@ -87,9 +91,10 @@ describe StatusPage::Storage::S3MultipartUpload, :aws_s3 do
           stub_responses(:complete_multipart_upload, aws_error)
         end
 
-        it 'raises an error' do
-          expect(s3_client).to receive(:abort_multipart_upload)
+        it 'aborts the upload and raises an error' do
           msg = error_message(aws_error, key: key)
+
+          expect(s3_client).to receive(:abort_multipart_upload)
           expect { result }.to raise_error(StatusPage::Storage::Error, msg)
         end
       end

ee/spec/services/status_page/publish_attachments_service_spec.rb

@@ -71,6 +71,17 @@ describe StatusPage::PublishAttachmentsService do
           expect(subject.payload).to eq({})
         end
 
+        context 'when upload to storage throws an error' do
+          it 'returns an error response' do
+            storage_error = StatusPage::Storage::Error.new(bucket: '', error: StandardError.new)
+            # no raise to mimic prod behavior
+            allow(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception)
+            allow(storage_client).to receive(:multipart_upload).and_raise(storage_error)
+
+            expect(subject.error?).to be true
+          end
+        end
+
         context 'user notes uploads' do
           let(:user_note) { instance_double(Note, note: markdown_field) }
           let(:user_notes) { [user_note] }

ee/spec/services/status_page/publish_details_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe StatusPage::PublishDetailsService do
+  include ::StatusPage::PublicationServiceResponses
+
   let_it_be(:project, refind: true) { create(:project) }
   let(:user_notes) { [] }
   let(:incident_id) { 1 }
@@ -31,16 +33,45 @@ describe StatusPage::PublishDetailsService do
       end
     end
 
-    context 'publishes attachments' do
-      it 'sends attachements to s3' do
-        allow(storage_client).to receive(:upload_object)
+    context 'publishing attachments' do
+      before do
+        allow(storage_client).to receive(:upload_object).and_return(success)
         allow(storage_client).to receive(:list_object_keys).and_return([])
+      end
+
+      context 'when feature flag disabled' do
+        before do
+          stub_feature_flags(status_page_attachments: false)
+        end
 
-        expect_next_instance_of(StatusPage::PublishAttachmentsService) do |service|
-          expect(service).to receive(:execute)
+        it 'does not publish attachments and returns success' do
+          expect(StatusPage::PublishAttachmentsService).not_to receive(:new)
+          expect(subject.success?).to be true
         end
+      end
+
+      context 'when successful' do
+        let(:success_response) { double(error?: false, success?: true) }
+
+        it 'sends attachments to storage and returns success' do
+          expect_next_instance_of(StatusPage::PublishAttachmentsService) do |service|
+            expect(service).to receive(:execute).and_return(success_response)
+          end
 
-        subject
+          expect(subject.success?).to be true
+        end
+      end
+
+      context 'when error returned from PublishAttachmentsService' do
+        let(:error_response) { double(error?: true, success?: false) }
+
+        it 'returns an error' do
+          expect_next_instance_of(StatusPage::PublishAttachmentsService) do |service|
+            expect(service).to receive(:execute).and_return(error_response)
+          end
+
+          expect(subject.success?).to be false
+        end
       end
     end
   end

lib/api/validations/validators/file_path.rb

@@ -8,7 +8,7 @@ module API
           path = params[attr_name]
 
           Gitlab::Utils.check_path_traversal!(path)
-        rescue Gitlab::Utils::PathTraversalAttackError
+        rescue ::Gitlab::Utils::PathTraversalAttackError
           raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
                                                message: "should be a valid file path"
         end

lib/gitlab/utils.rb

@@ -3,8 +3,7 @@
 module Gitlab
   module Utils
     extend self
-    # disable Cop/CustomErrorClass because recommended style causes 'already initialized constant' warning
-    class PathTraversalAttackError < StandardError; end # rubocop:disable Cop/CustomErrorClass
+    PathTraversalAttackError ||= Class.new(StandardError)
 
     # Ensure that the relative path will not traverse outside the base directory
     # We url decode the path to avoid passing invalid paths forward in url encoded format.

spec/finders/uploader_finder_spec.rb

@@ -15,7 +15,7 @@ describe UploaderFinder do
       upload.save
     end
 
-    context 'when sucessful' do
+    context 'when successful' do
       before do
         allow_next_instance_of(FileUploader) do |uploader|
           allow(uploader).to receive(:retrieve_from_store!).with(upload.path).and_return(uploader)
@@ -48,7 +48,7 @@ describe UploaderFinder do
       end
 
       it 'returns nil when unexpected error is raised' do
-        expect(subject).to be(nil)
+        expect { subject }.to raise_error(StandardError)
       end
     end
   end