Merge branch 'pl-status-page-mvc-delete-confidential' into 'master'

Unpublish details for confidential issues

See merge request gitlab-org/gitlab!27263

config/sidekiq_queues.yml

@@ -234,7 +234,7 @@
   - 2
 - - service_desk_email_receiver
   - 1
-- - status_page_publish_incident
+- - status_page_publish
   - 1
 - - sync_seat_link_request
   - 1

ee/app/finders/status_page/incidents_finder.rb

@@ -10,10 +10,12 @@
 #
 #     finder = StatusPage::IncidentsFinder.new(project_id: project_id)
 #
-#     # A single issue
-#     issue, user_notes = finder.find_by_id(issue_id)
+#     # A single issue which includes confidential issues by default)
+#     issue = finder.find_by_id(issue_id)
+#     # Find a "public only" issue
+#     issue = finder.find_by_id(issue_id, include_confidential: false)
 #
-#     # Most recent 20 issues
+#     # Most recent 20 non-confidential issues
 #     issues = finder.all
 #
 module StatusPage
@@ -24,24 +26,23 @@ module StatusPage
       @project_id = project_id
     end
 
-    def find_by_id(issue_id)
-      execute.find_by_id(issue_id)
+    def find_by_id(issue_id, include_confidential: true)
+      execute(include_confidential: include_confidential)
+        .find_by_id(issue_id)
     end
 
     def all
-      @sorted = true
-
-      execute
+      execute(sorted: true)
         .limit(MAX_LIMIT) # rubocop: disable CodeReuse/ActiveRecord
     end
 
     private
 
-    attr_reader :project_id, :with_user_notes, :sorted
+    attr_reader :project_id
 
-    def execute
+    def execute(sorted: false, include_confidential: false)
       issues = init_collection
-      issues = public_only(issues)
+      issues = public_only(issues) unless include_confidential
       issues = by_project(issues)
       issues = reverse_chronological(issues) if sorted
       issues

ee/app/services/status_page/publish_base_service.rb

@@ -12,14 +12,14 @@ module StatusPage
       return error_feature_not_available unless feature_available?
       return error_no_storage_client unless storage_client
 
-      publish(*args)
+      process(*args)
     end
 
     private
 
     attr_reader :project
 
-    def publish(*args)
+    def process(*args)
       raise NotImplementedError
     end
 
@@ -53,6 +53,12 @@ module StatusPage
       success(object_key: key)
     end
 
+    def delete(key)
+      storage_client.delete_object(key)
+
+      success(object_key: key)
+    end
+
     def limit_exceeded?(json)
       !Gitlab::Utils::DeepSize
         .new(json, max_size: Storage::JSON_MAX_SIZE)

ee/app/services/status_page/publish_details_service.rb

@@ -4,13 +4,13 @@ module StatusPage
   # Render an issue as incident details and publish them to CDN.
   #
   # This is an internal service which is part of
-  # +StatusPage::PublishIncidentService+ and is not meant to be called directly.
+  # +StatusPage::PublishService+ and is not meant to be called directly.
   #
-  # Consider calling +StatusPage::PublishIncidentService+ instead.
+  # Consider calling +StatusPage::PublishService+ instead.
   class PublishDetailsService < PublishBaseService
     private
 
-    def publish(issue, user_notes)
+    def process(issue, user_notes)
       json = serialize(issue, user_notes)
       key = object_key(json)
       return error('Missing object key') unless key

ee/app/services/status_page/publish_list_service.rb

@@ -4,13 +4,13 @@ module StatusPage
   # Render a list of issues as incidents and publish them to CDN.
   #
   # This is an internal service which is part of
-  # +StatusPage::PublishIncidentService+ and is not meant to be called directly.
+  # +StatusPage::PublishService+ and is not meant to be called directly.
   #
-  # Consider calling +StatusPage::PublishIncidentService+ instead.
+  # Consider calling +StatusPage::PublishService+ instead.
   class PublishListService < PublishBaseService
     private
 
-    def publish(issues)
+    def process(issues)
       json = serialize(issues)
 
       upload(object_key, json)

ee/app/services/status_page/publish_incident_service.rb → ee/app/services/status_page/publish_service.rb

@@ -9,8 +9,9 @@ module StatusPage
   #
   # This services calls:
   # * StatusPage::PublishDetailsService
+  # * StatusPage::UnpublishDetailsService
   # * StatusPage::PublishListService
-  class PublishIncidentService
+  class PublishService
     include Gitlab::Utils::StrongMemoize
 
     def initialize(user:, project:, issue_id:)
@@ -23,22 +24,34 @@ module StatusPage
       return error_permission_denied unless can_publish?
       return error_issue_not_found unless issue
 
-      response = publish_details
+      response = process_details
       return response if response.error?
 
-      publish_list
+      process_list
     end
 
     private
 
     attr_reader :user, :project, :issue_id
 
+    def process_details
+      if issue.confidential?
+        unpublish_details
+      else
+        publish_details
+      end
+    end
+
+    def process_list
+      PublishListService.new(project: project).execute(issues)
+    end
+
     def publish_details
       PublishDetailsService.new(project: project).execute(issue, user_notes)
     end
 
-    def publish_list
-      PublishListService.new(project: project).execute(issues)
+    def unpublish_details
+      UnpublishDetailsService.new(project: project).execute(issue)
     end
 
     def issue

ee/app/services/status_page/trigger_publish_service.rb

@@ -15,8 +15,7 @@ module StatusPage
       return unless can_publish?
       return unless status_page_enabled?
 
-      StatusPage::PublishIncidentWorker
-        .perform_async(user.id, project.id, issue_id)
+      StatusPage::PublishWorker.perform_async(user.id, project.id, issue_id)
     end
 
     private

ee/app/services/status_page/unpublish_details_service.rb

+# frozen_string_literal: true
+
+module StatusPage
+  # Unpublish incident details from CDN.
+  #
+  # Example: An issue becomes confidential so it must be removed from CDN.
+  #
+  # This is an internal service which is part of
+  # +StatusPage::PublishService+ and is not meant to be called directly.
+  #
+  # Consider calling +StatusPage::PublishService+ instead.
+  class UnpublishDetailsService < PublishBaseService
+    private
+
+    def process(issue)
+      key = object_key(issue)
+
+      delete(key)
+    end
+
+    def object_key(issue)
+      StatusPage::Storage.details_path(issue.iid)
+    end
+  end
+end

ee/app/workers/all_queues.yml

@@ -577,7 +577,7 @@
   :resource_boundary: :unknown
   :weight: 1
   :idempotent: 
-- :name: status_page_publish_incident
+- :name: status_page_publish
   :feature_category: :status_page
   :has_external_dependencies: true
   :urgency: :low

ee/app/workers/status_page/publish_incident_worker.rb → ee/app/workers/status_page/publish_worker.rb

 # frozen_string_literal: true
 
 module StatusPage
-  class PublishIncidentWorker
+  class PublishWorker
     include ApplicationWorker
     include Gitlab::Utils::StrongMemoize
 
@@ -26,7 +26,7 @@ module StatusPage
     attr_reader :user_id, :project_id, :issue_id
 
     def publish
-      result = PublishIncidentService
+      result = PublishService
         .new(user: user, project: project, issue_id: issue_id)
         .execute
 

ee/lib/status_page/storage/s3_client.rb

@@ -25,6 +25,17 @@ module StatusPage
         true
       end
 
+      # Deletes +key+ from storage
+      #
+      # Note, this operation succeeds even if +key+ does not exist in storage.
+      def delete_object(key)
+        wrap_errors(key: key) do
+          client.delete_object(bucket: bucket_name, key: key)
+        end
+
+        true
+      end
+
       private
 
       attr_reader :client, :bucket_name

ee/spec/finders/status_page/incidents_finder_spec.rb

@@ -17,7 +17,10 @@ describe StatusPage::IncidentsFinder do
   let(:finder) { described_class.new(project_id: project.id) }
 
   describe '#find_by_id' do
-    subject { finder.find_by_id(issue.id) }
+    subject { finder.find_by_id(issue.id, **params) }
+
+    context 'without params' do
+      let(:params) { {} }
 
       context 'for public issue' do
         let(:issue) { public_issues.first }
@@ -28,7 +31,7 @@ describe StatusPage::IncidentsFinder do
       context 'for confidential issue' do
         let(:issue) { issues.fetch(:confidential) }
 
-      it { is_expected.to be_nil }
+        it { is_expected.to eq(issue) }
       end
 
       context 'for unrelated issue' do
@@ -38,8 +41,20 @@ describe StatusPage::IncidentsFinder do
       end
     end
 
+    context 'with include_confidential' do
+      let(:params) { { include_confidential: false } }
+
+      context 'for confidential issue' do
+        let(:issue) { issues.fetch(:confidential) }
+
+        it { is_expected.to be_nil }
+      end
+    end
+  end
+
   describe '#all' do
     let(:sorted_issues) { public_issues.sort_by(&:created_at).reverse }
+    let(:limit) { public_issues.size }
 
     subject { finder.all }
 
@@ -58,5 +73,13 @@ describe StatusPage::IncidentsFinder do
 
       it { is_expected.to eq(sorted_issues.first(1)) }
     end
+
+    context 'when combined with other finder methods' do
+      before do
+        finder.find_by_id(public_issues.first.id)
+      end
+
+      it { is_expected.to eq(sorted_issues) }
+    end
   end
 end

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

@@ -41,6 +41,29 @@ describe StatusPage::Storage::S3Client, :aws_s3 do
     end
   end
 
+  describe '#delete_object' do
+    let(:key) { 'key' }
+
+    subject(:result) { client.delete_object(key) }
+
+    it 'returns true' do
+      stub_responses(:delete_object)
+
+      expect(result).to eq(true)
+    end
+
+    context 'when failed' do
+      let(:aws_error) { 'SomeError' }
+
+      it 'raises an error' do
+        stub_responses(:delete_object, aws_error)
+
+        msg = error_message(aws_error, key: key)
+        expect { result }.to raise_error(StatusPage::Storage::Error, msg)
+      end
+    end
+  end
+
   private
 
   def stub_responses(*args)

ee/spec/services/status_page/publish_incident_service_spec.rb → ee/spec/services/status_page/publish_service_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe StatusPage::PublishIncidentService do
+describe StatusPage::PublishService do
   let_it_be(:user) { create(:user) }
   let_it_be(:project, refind: true) { create(:project) }
   let_it_be(:issue) { create(:issue, project: project) }
@@ -23,8 +23,9 @@ describe StatusPage::PublishIncidentService do
         .and_return(user_can_publish)
     end
 
-    context 'when publishing succeeds' do
-      it 'returns uploads incidents details and list' do
+    describe 'publish details' do
+      context 'when upload succeeds' do
+        it 'uploads incident details and list' do
           expect_to_upload_details(issue)
           expect_to_upload_list
 
@@ -32,15 +33,38 @@ describe StatusPage::PublishIncidentService do
         end
       end
 
-    context 'when uploading details fails' do
+      context 'when upload fails' do
         it 'propagates the exception' do
           expect_to_upload_details(issue, status: 404)
 
           expect { result }.to raise_error(StatusPage::Storage::Error)
         end
       end
+    end
+
+    describe 'unpublish details' do
+      let_it_be(:issue) { create(:issue, :confidential, project: project) }
+
+      context 'when deletion succeeds' do
+        it 'deletes incident details and upload list' do
+          expect_to_delete_details(issue)
+          expect_to_upload_list
+
+          expect(result).to be_success
+        end
+      end
+
+      context 'when deletion fails' do
+        it 'propagates the exception' do
+          expect_to_delete_details(issue, status: 403)
 
-    context 'when uploading list fails' do
+          expect { result }.to raise_error(StatusPage::Storage::Error)
+        end
+      end
+    end
+
+    describe 'publish list' do
+      context 'when upload fails' do
         it 'returns error and skip list upload' do
           expect_to_upload_details(issue)
           expect_to_upload_list(status: 404)
@@ -48,6 +72,7 @@ describe StatusPage::PublishIncidentService do
           expect { result }.to raise_error(StatusPage::Storage::Error)
         end
       end
+    end
 
     context 'with unrelated issue' do
       let(:issue) { create(:issue) }
@@ -71,14 +96,18 @@ describe StatusPage::PublishIncidentService do
   private
 
   def expect_to_upload_details(issue, **kwargs)
-    stub_upload_request(StatusPage::Storage.details_path(issue.iid), **kwargs)
+    stub_aws_request(:put, StatusPage::Storage.details_path(issue.iid), **kwargs)
+  end
+
+  def expect_to_delete_details(issue, **kwargs)
+    stub_aws_request(:delete, StatusPage::Storage.details_path(issue.iid), **kwargs)
   end
 
   def expect_to_upload_list(**kwargs)
-    stub_upload_request(StatusPage::Storage.list_path, **kwargs)
+    stub_aws_request(:put, StatusPage::Storage.list_path, **kwargs)
   end
 
-  def stub_upload_request(path, status: 200)
-    stub_request(:put, %r{amazonaws.com/#{path}}).to_return(status: status)
+  def stub_aws_request(method, path, status: 200)
+    stub_request(method, %r{amazonaws.com/#{path}}).to_return(status: status)
   end
 end

ee/spec/services/status_page/trigger_publish_service_spec.rb

@@ -7,7 +7,7 @@ describe StatusPage::TriggerPublishService do
   let_it_be(:project, refind: true) { create(:project) }
   let_it_be(:issue) { create(:issue, project: project) }
   let(:service) { described_class.new(user: user, project: project) }
-  let(:worker) { StatusPage::PublishIncidentWorker }
+  let(:worker) { StatusPage::PublishWorker }
 
   let_it_be(:status_page_setting) do
     create(:status_page_setting, :enabled, project: project)

ee/spec/services/status_page/unpublish_details_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe StatusPage::UnpublishDetailsService do
+  let_it_be(:project, refind: true) { create(:project) }
+  let(:issue) { instance_double(Issue, iid: incident_id) }
+  let(:incident_id) { 1 }
+  let(:key) { StatusPage::Storage.details_path(incident_id) }
+
+  let(:service) { described_class.new(project: project) }
+
+  subject(:result) { service.execute(issue) }
+
+  describe '#execute' do
+    let(:status_page_setting_enabled) { true }
+    let(:storage_client) { instance_double(StatusPage::Storage::S3Client) }
+
+    let(:status_page_setting) do
+      instance_double(StatusPageSetting, enabled?: status_page_setting_enabled,
+                      storage_client: storage_client)
+    end
+
+    before do
+      stub_licensed_features(status_page: true)
+
+      allow(project).to receive(:status_page_setting)
+        .and_return(status_page_setting)
+    end
+
+    context 'when deletion succeeds' do
+      before do
+        allow(storage_client).to receive(:delete_object).with(key)
+      end
+
+      it 'removes details from CDN' do
+        expect(result).to be_success
+        expect(result.payload).to eq(object_key: key)
+      end
+    end
+
+    context 'when upload fails due to exception' do
+      let(:bucket) { 'bucket_name' }
+      let(:error) { StandardError.new }
+
+      let(:exception) do
+        StatusPage::Storage::Error.new(bucket: bucket, error: error)
+      end
+
+      before do
+        allow(storage_client).to receive(:delete_object).with(key)
+          .and_raise(exception)
+      end
+
+      it 'propagates the exception' do
+        expect { result }.to raise_error(exception)
+      end
+    end
+
+    context 'when status page setting is not enabled' do
+      let(:status_page_setting_enabled) { false }
+
+      it 'returns feature not available error' do
+        expect(result).to be_error
+        expect(result.message).to eq('Feature not available')
+      end
+    end
+  end
+end

ee/spec/workers/status_page/publish_incident_worker_spec.rb → ee/spec/workers/status_page/publish_worker_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe StatusPage::PublishIncidentWorker do
+describe StatusPage::PublishWorker do
   include ExclusiveLeaseHelpers
 
   let_it_be(:user) { create(:user) }
@@ -11,11 +11,11 @@ describe StatusPage::PublishIncidentWorker do
 
   let(:worker) { described_class.new }
   let(:logger) { worker.send(:logger) }
-  let(:service) { instance_double(StatusPage::PublishIncidentService) }
+  let(:service) { instance_double(StatusPage::PublishService) }
   let(:service_result) { ServiceResponse.success }
 
   before do
-    allow(StatusPage::PublishIncidentService)
+    allow(StatusPage::PublishService)
       .to receive(:new).with(user: user, project: project, issue_id: issue.id)
       .and_return(service)
     allow(service).to receive(:execute)
@@ -40,7 +40,7 @@ describe StatusPage::PublishIncidentWorker do
         let(:project) { build(:project) }
 
         it 'does not execute the service' do
-          expect(StatusPage::PublishIncidentService).not_to receive(:execute)
+          expect(StatusPage::PublishService).not_to receive(:execute)
 
           subject
         end