Fix confidential issues should not be passed to Webhooks

5 files changed, 77 insertions, 22 deletions

diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index 089b0f527e2..241efc44d36 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -14,6 +14,8 @@ module Issues
     end
 
     def execute_hooks(issue, action = 'open')
+      return if issue.confidential?
+
       issue_data = hook_data(issue, action)
       issue.project.execute_hooks(issue_data, :issue_hooks)
       issue.project.execute_services(issue_data, :issue_hooks)
diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb
index aff022a573e..229c8eb5b5b 100644
--- a/spec/services/issues/close_service_spec.rb
+++ b/spec/services/issues/close_service_spec.rb
@@ -18,7 +18,7 @@ describe Issues::CloseService, services: true do
     context "valid params" do
       before do
         perform_enqueued_jobs do
-          @issue = described_class.new(project, user, {}).execute(issue)
+          @issue = described_class.new(project, user).execute(issue)
         end
       end
 
@@ -53,10 +53,21 @@ describe Issues::CloseService, services: true do
       end
     end
 
-    context "external issue tracker" do
+    context 'when issue is confidential' do
+      it 'does not execute hooks' do
+        issue = create(:issue, :confidential, project: project)
+
+        expect(project).not_to receive(:execute_hooks)
+        expect(project).not_to receive(:execute_services)
+
+        described_class.new(project, user).execute(issue)
+      end
+    end
+
+    context 'external issue tracker' do
       before do
         allow(project).to receive(:default_issues_tracker?).and_return(false)
-        @issue = described_class.new(project, user, {}).execute(issue)
+        @issue = described_class.new(project, user).execute(issue)
       end
 
       it { expect(@issue).to be_valid }
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index fcc3c0a00bd..81beca47738 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -72,6 +72,15 @@ describe Issues::CreateService, services: true do
           expect(issue.milestone).not_to eq milestone
         end
       end
+
+      it 'does not execute hooks when issue is confidential' do
+        opts = { title: 'Title', description: 'Description', confidential: true }
+
+        expect(project).not_to receive(:execute_hooks)
+        expect(project).not_to receive(:execute_services)
+
+        described_class.new(project, user, opts).execute
+      end
     end
 
     it_behaves_like 'new issuable record that supports slash commands'
diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb
index 34a89fcd4e1..83c72e64c87 100644
--- a/spec/services/issues/reopen_service_spec.rb
+++ b/spec/services/issues/reopen_service_spec.rb
@@ -1,17 +1,15 @@
 require 'spec_helper'
 
 describe Issues::ReopenService, services: true do
-  let(:guest) { create(:user) }
-  let(:issue) { create(:issue, :closed) }
-  let(:project) { issue.project }
-
-  before do
-    project.team << [guest, :guest]
-  end
+  let(:project) { create(:empty_project) }
+  let(:issue) { create(:issue, :closed, project: project) }
 
   describe '#execute' do
     context 'current user is not authorized to reopen issue' do
       before do
+        guest = create(:user)
+        project.team << [guest, :guest]
+
         perform_enqueued_jobs do
           @issue = described_class.new(project, guest).execute(issue)
         end
@@ -21,5 +19,19 @@ describe Issues::ReopenService, services: true do
         expect(@issue).to be_closed
       end
     end
+
+    context 'when issue is confidential' do
+      it 'does not execute hooks' do
+        user = create(:user)
+        project.team << [user, :master]
+
+        issue = create(:issue, :confidential, :closed, project: project)
+
+        expect(project).not_to receive(:execute_hooks)
+        expect(project).not_to receive(:execute_services)
+
+        described_class.new(project, user).execute(issue)
+      end
+    end
   end
 end
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index 0313f424463..a4bd19d317b 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -28,6 +28,11 @@ describe Issues::UpdateService, services: true do
       end
     end
 
+    def update_issue(opts)
+      @issue = described_class.new(project, user, opts).execute(issue)
+      @issue.reload
+    end
+
     context "valid params" do
       before do
         opts = {
@@ -35,12 +40,11 @@ describe Issues::UpdateService, services: true do
           description: 'Also please fix',
           assignee_id: user2.id,
           state_event: 'close',
-          label_ids: [label.id],
-          confidential: true
+          label_ids: [label.id]
         }
 
         perform_enqueued_jobs do
-          @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+          @issue = described_class.new(project, user, opts).execute(issue)
         end
 
         @issue.reload
@@ -81,18 +85,35 @@ describe Issues::UpdateService, services: true do
         expect(note).not_to be_nil
         expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**'
       end
+    end
+
+    context 'when issue turns confidential' do
+      let(:opts) do
+        {
+          title: 'New title',
+          description: 'Also please fix',
+          assignee_id: user2.id,
+          state_event: 'close',
+          label_ids: [label.id],
+          confidential: true
+        }
+      end
 
       it 'creates system note about confidentiality change' do
+        update_issue({ confidential: true })
+
         note = find_note('Made the issue confidential')
 
         expect(note).not_to be_nil
         expect(note.note).to eq 'Made the issue confidential'
       end
-    end
 
-    def update_issue(opts)
-      @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
-      @issue.reload
+      it 'does not execute hooks' do
+        expect(project).not_to receive(:execute_hooks)
+        expect(project).not_to receive(:execute_services)
+
+        update_issue({ confidential: true })
+      end
     end
 
     context 'todos' do
@@ -176,7 +197,7 @@ describe Issues::UpdateService, services: true do
         opts = { label_ids: [label.id] }
 
         perform_enqueued_jobs do
-          @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+          @issue = described_class.new(project, user, opts).execute(issue)
         end
 
         should_email(subscriber)
@@ -190,7 +211,7 @@ describe Issues::UpdateService, services: true do
           opts = { label_ids: [label.id, label2.id] }
 
           perform_enqueued_jobs do
-            @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+            @issue = described_class.new(project, user, opts).execute(issue)
           end
 
           should_not_email(subscriber)
@@ -201,7 +222,7 @@ describe Issues::UpdateService, services: true do
           opts = { label_ids: [label2.id] }
 
           perform_enqueued_jobs do
-            @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+            @issue = described_class.new(project, user, opts).execute(issue)
           end
 
           should_not_email(subscriber)
@@ -210,7 +231,7 @@ describe Issues::UpdateService, services: true do
       end
     end
 
-    context 'when Issue has tasks' do
+    context 'when issue has tasks' do
       before { update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) }
 
       it { expect(@issue.tasks?).to eq(true) }
@@ -277,7 +298,7 @@ describe Issues::UpdateService, services: true do
 
     context 'updating labels' do
       let(:label3) { create(:label, project: project) }
-      let(:result) { Issues::UpdateService.new(project, user, params).execute(issue).reload }
+      let(:result) { described_class.new(project, user, params).execute(issue).reload }
 
       context 'when add_label_ids and label_ids are passed' do
         let(:params) { { label_ids: [label.id], add_label_ids: [label3.id] } }