Merge branch 'fix-confidential-issues-webhooks' into 'master'

Scope webhooks/services that will run for confidential issues

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/20661

See merge request !1986

CHANGELOG

@@ -56,6 +56,7 @@ v 8.11.4 (unreleased)
   - Fix resolving conflicts on forks
   - Fix diff commenting on merge requests created prior to 8.10
   - Don't create groups for unallowed users when importing projects
+  - Scope webhooks/services that will run for confidential issues
   - Fix issue boards leak private label names and descriptions
 
 v 8.11.3

app/controllers/concerns/service_params.rb

@@ -13,7 +13,7 @@ module ServiceParams
                     # `issue_events` and `merge_request_events` (singular!)
                     # See app/helpers/services_helper.rb for how we
                     # make those event names plural as special case.
-                    :issues_events, :merge_requests_events,
+                    :issues_events, :confidential_issues_events, :merge_requests_events,
                     :notify_only_broken_builds, :notify_only_broken_pipelines,
                     :add_pusher, :send_from_committer_email, :disable_diffs,
                     :external_wiki_url, :notify, :color,

app/controllers/projects/hooks_controller.rb

@@ -59,6 +59,7 @@ class Projects::HooksController < Projects::ApplicationController
       :pipeline_events,
       :enable_ssl_verification,
       :issues_events,
+      :confidential_issues_events,
       :merge_requests_events,
       :note_events,
       :push_events,

app/helpers/services_helper.rb

@@ -8,7 +8,9 @@ module ServicesHelper
     when "note"
       "Event will be triggered when someone adds a comment"
     when "issue"
-      "Event will be triggered when an issue is created/updated/merged"
+      "Event will be triggered when an issue is created/updated/closed"
+    when "confidential_issue"
+      "Event will be triggered when a confidential issue is created/updated/closed"
     when "merge_request"
       "Event will be triggered when a merge request is created/updated/merged"
     when "build"
@@ -19,7 +21,7 @@ module ServicesHelper
   end
 
   def service_event_field_name(event)
-    event = event.pluralize if %w[merge_request issue].include?(event)
+    event = event.pluralize if %w[merge_request issue confidential_issue].include?(event)
     "#{event}_events"
   end
 end

app/models/hooks/project_hook.rb

@@ -2,6 +2,7 @@ class ProjectHook < WebHook
   belongs_to :project
 
   scope :issue_hooks, -> { where(issues_events: true) }
+  scope :confidential_issue_hooks, -> { where(confidential_issues_events: true) }
   scope :note_hooks, -> { where(note_events: true) }
   scope :merge_request_hooks, -> { where(merge_requests_events: true) }
   scope :build_hooks, -> { where(build_events: true) }

app/models/hooks/web_hook.rb

@@ -4,6 +4,7 @@ class WebHook < ActiveRecord::Base
 
   default_value_for :push_events, true
   default_value_for :issues_events, false
+  default_value_for :confidential_issues_events, false
   default_value_for :note_events, false
   default_value_for :merge_requests_events, false
   default_value_for :tag_push_events, false

app/models/project_services/hipchat_service.rb

@@ -39,7 +39,7 @@ class HipchatService < Service
   end
 
   def supported_events
-    %w(push issue merge_request note tag_push build)
+    %w(push issue confidential_issue merge_request note tag_push build)
   end
 
   def execute(data)

app/models/project_services/slack_service.rb

@@ -44,7 +44,7 @@ class SlackService < Service
   end
 
   def supported_events
-    %w(push issue merge_request note tag_push build wiki_page)
+    %w(push issue confidential_issue merge_request note tag_push build wiki_page)
   end
 
   def execute(data)

app/models/service.rb

@@ -7,6 +7,7 @@ class Service < ActiveRecord::Base
   default_value_for :active, false
   default_value_for :push_events, true
   default_value_for :issues_events, true
+  default_value_for :confidential_issues_events, true
   default_value_for :merge_requests_events, true
   default_value_for :tag_push_events, true
   default_value_for :note_events, true
@@ -33,6 +34,7 @@ class Service < ActiveRecord::Base
   scope :push_hooks, -> { where(push_events: true, active: true) }
   scope :tag_push_hooks, -> { where(tag_push_events: true, active: true) }
   scope :issue_hooks, -> { where(issues_events: true, active: true) }
+  scope :confidential_issue_hooks, -> { where(confidential_issues_events: true, active: true) }
   scope :merge_request_hooks, -> { where(merge_requests_events: true, active: true) }
   scope :note_hooks, -> { where(note_events: true, active: true) }
   scope :build_hooks, -> { where(build_events: true, active: true) }
@@ -100,7 +102,7 @@ class Service < ActiveRecord::Base
   end
 
   def supported_events
-    %w(push tag_push issue merge_request wiki_page)
+    %w(push tag_push issue confidential_issue merge_request wiki_page)
   end
 
   def execute(data)

app/services/issues/base_service.rb

@@ -15,8 +15,9 @@ module Issues
 
     def execute_hooks(issue, action = 'open')
       issue_data  = hook_data(issue, action)
-      issue.project.execute_hooks(issue_data, :issue_hooks)
-      issue.project.execute_services(issue_data, :issue_hooks)
+      hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks
+      issue.project.execute_hooks(issue_data, hooks_scope)
+      issue.project.execute_services(issue_data, hooks_scope)
     end
   end
 end

app/views/projects/hooks/_project_hook.html.haml

@@ -3,7 +3,7 @@
     .col-md-8.col-lg-7
       %strong.light-header= hook.url
       %div
-        - %w(push_events tag_push_events issues_events note_events merge_requests_events build_events pipeline_events wiki_page_events).each do |trigger|
+        - %w(push_events tag_push_events issues_events confidential_issues_events note_events merge_requests_events build_events pipeline_events wiki_page_events).each do |trigger|
           - if hook.send(trigger)
             %span.label.label-gray.deploy-project-label= trigger.titleize
     .col-md-4.col-lg-5.text-right-lg.prepend-top-5

app/views/shared/web_hooks/_form.html.haml

@@ -51,6 +51,13 @@
                 %strong Issues events
               %p.light
                 This URL will be triggered when an issue is created/updated/merged
+          %li
+            = f.check_box :confidential_issues_events, class: 'pull-left'
+            .prepend-left-20
+              = f.label :confidential_issues_events, class: 'list-label' do
+                %strong Confidential Issues events
+              %p.light
+                This URL will be triggered when a confidential issue is created/updated/merged
           %li
             = f.check_box :merge_requests_events, class: 'pull-left'
             .prepend-left-20

db/migrate/20160830203109_add_confidential_issues_events_to_web_hooks.rb

+class AddConfidentialIssuesEventsToWebHooks < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :web_hooks, :confidential_issues_events, :boolean, default: false, allow_null: false
+  end
+
+  def down
+    remove_column :web_hooks, :confidential_issues_events
+  end
+end

db/migrate/20160830211132_add_confidential_issues_events_to_services.rb

+class AddConfidentialIssuesEventsToServices < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :services, :confidential_issues_events, :boolean, default: true, allow_null: false
+  end
+
+  def down
+    remove_column :services, :confidential_issues_events
+  end
+end

db/migrate/20160901141443_set_confidential_issues_events_on_webhooks.rb

+class SetConfidentialIssuesEventsOnWebhooks < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    update_column_in_batches(:web_hooks, :confidential_issues_events, true) do |table, query|
+      query.where(table[:issues_events].eq(true))
+    end
+  end
+
+  def down
+    # noop
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160830232601) do
+ActiveRecord::Schema.define(version: 20160901141443) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -914,6 +914,7 @@ ActiveRecord::Schema.define(version: 20160830232601) do
     t.boolean  "default",                    default: false
     t.boolean  "wiki_page_events",           default: true
     t.boolean  "pipeline_events",            default: false,    null: false
+    t.boolean  "confidential_issues_events", default: true,     null: false
   end
 
   add_index "services", ["project_id"], name: "index_services_on_project_id", using: :btree
@@ -1129,6 +1130,7 @@ ActiveRecord::Schema.define(version: 20160830232601) do
     t.boolean  "wiki_page_events",                        default: false,         null: false
     t.string   "token"
     t.boolean  "pipeline_events",                         default: false,         null: false
+    t.boolean  "confidential_issues_events",              default: false,         null: false
   end
 
   add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree

spec/services/issues/close_service_spec.rb

@@ -18,12 +18,12 @@ describe Issues::CloseService, services: true do
     context "valid params" do
       before do
         perform_enqueued_jobs do
-          @issue = described_class.new(project, user, {}).execute(issue)
+          described_class.new(project, user).execute(issue)
         end
       end
 
-      it { expect(@issue).to be_valid }
-      it { expect(@issue).to be_closed }
+      it { expect(issue).to be_valid }
+      it { expect(issue).to be_closed }
 
       it 'sends email to user2 about assign of new issue' do
         email = ActionMailer::Base.deliveries.last
@@ -32,7 +32,7 @@ describe Issues::CloseService, services: true do
       end
 
       it 'creates system note about issue reassign' do
-        note = @issue.notes.last
+        note = issue.notes.last
         expect(note.note).to include "Status changed to closed"
       end
 
@@ -44,23 +44,43 @@ describe Issues::CloseService, services: true do
     context 'current user is not authorized to close issue' do
       before do
         perform_enqueued_jobs do
-          @issue = described_class.new(project, guest).execute(issue)
+          described_class.new(project, guest).execute(issue)
         end
       end
 
       it 'does not close the issue' do
-        expect(@issue).to be_open
+        expect(issue).to be_open
       end
     end
 
-    context "external issue tracker" do
+    context 'when issue is not confidential' do
+      it 'executes issue hooks' do
+        expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
+        expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks)
+
+        described_class.new(project, user).execute(issue)
+      end
+    end
+
+    context 'when issue is confidential' do
+      it 'executes confidential issue hooks' do
+        issue = create(:issue, :confidential, project: project)
+
+        expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks)
+        expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks)
+
+        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)
+        described_class.new(project, user).execute(issue)
       end
 
-      it { expect(@issue).to be_valid }
-      it { expect(@issue).to be_opened }
+      it { expect(issue).to be_valid }
+      it { expect(issue).to be_opened }
       it { expect(todo.reload).to be_pending }
     end
   end

spec/services/issues/create_service_spec.rb

@@ -72,6 +72,24 @@ describe Issues::CreateService, services: true do
           expect(issue.milestone).not_to eq milestone
         end
       end
+
+      it 'executes issue hooks when issue is not confidential' do
+        opts = { title: 'Title', description: 'Description', confidential: false }
+
+        expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
+        expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks)
+
+        described_class.new(project, user, opts).execute
+      end
+
+      it 'executes confidential issue hooks when issue is confidential' do
+        opts = { title: 'Title', description: 'Description', confidential: true }
+
+        expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks)
+        expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks)
+
+        described_class.new(project, user, opts).execute
+      end
     end
 
     it_behaves_like 'new issuable record that supports slash commands'

spec/services/issues/reopen_service_spec.rb

 require 'spec_helper'
 
 describe Issues::ReopenService, services: true do
-  let(:guest) { create(:user) }
-  let(:issue) { create(:issue, :closed) }
-  let(:project) { issue.project }
+  let(:project) { create(:empty_project) }
+  let(:issue) { create(:issue, :closed, project: project) }
 
+  describe '#execute' do
+    context 'when user is not authorized to reopen issue' do
       before do
+        guest = create(:user)
         project.team << [guest, :guest]
-  end
 
-  describe '#execute' do
-    context 'current user is not authorized to reopen issue' do
-      before do
         perform_enqueued_jobs do
-          @issue = described_class.new(project, guest).execute(issue)
+          described_class.new(project, guest).execute(issue)
         end
       end
 
       it 'does not reopen the issue' do
-        expect(@issue).to be_closed
+        expect(issue).to be_closed
+      end
+    end
+
+    context 'when user is authrized to reopen issue' do
+      let(:user) { create(:user) }
+
+      before do
+        project.team << [user, :master]
+      end
+
+      context 'when issue is not confidential' do
+        it 'executes issue hooks' do
+          expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
+          expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks)
+
+          described_class.new(project, user).execute(issue)
+        end
+      end
+
+      context 'when issue is confidential' do
+        it 'executes confidential issue hooks' do
+          issue = create(:issue, :confidential, :closed, project: project)
+
+          expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks)
+          expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks)
+
+          described_class.new(project, user).execute(issue)
+        end
       end
     end
   end

spec/services/issues/update_service_spec.rb

@@ -23,11 +23,15 @@ describe Issues::UpdateService, services: true do
 
   describe 'execute' do
     def find_note(starting_with)
-      @issue.notes.find do |note|
+      issue.notes.find do |note|
         note && note.note.start_with?(starting_with)
       end
     end
 
+    def update_issue(opts)
+      described_class.new(project, user, opts).execute(issue)
+    end
+
     context "valid params" do
       before do
         opts = {
@@ -35,23 +39,20 @@ 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)
+          update_issue(opts)
         end
-
-        @issue.reload
       end
 
-      it { expect(@issue).to be_valid }
-      it { expect(@issue.title).to eq('New title') }
-      it { expect(@issue.assignee).to eq(user2) }
-      it { expect(@issue).to be_closed }
-      it { expect(@issue.labels.count).to eq(1) }
-      it { expect(@issue.labels.first.title).to eq(label.name) }
+      it { expect(issue).to be_valid }
+      it { expect(issue.title).to eq('New title') }
+      it { expect(issue.assignee).to eq(user2) }
+      it { expect(issue).to be_closed }
+      it { expect(issue.labels.count).to eq(1) }
+      it { expect(issue.labels.first.title).to eq(label.name) }
 
       it 'sends email to user2 about assign of new issue and email to user3 about issue unassignment' do
         deliveries = ActionMailer::Base.deliveries
@@ -81,18 +82,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 'executes confidential issue hooks' do
+        expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks)
+        expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks)
+
+        update_issue(confidential: true)
+      end
     end
 
     context 'todos' do
@@ -100,7 +118,7 @@ describe Issues::UpdateService, services: true do
 
       context 'when the title change' do
         before do
-          update_issue({ title: 'New title' })
+          update_issue(title: 'New title')
         end
 
         it 'marks pending todos as done' do
@@ -110,7 +128,7 @@ describe Issues::UpdateService, services: true do
 
       context 'when the description change' do
         before do
-          update_issue({ description: 'Also please fix' })
+          update_issue(description: 'Also please fix')
         end
 
         it 'marks todos as done' do
@@ -120,7 +138,7 @@ describe Issues::UpdateService, services: true do
 
       context 'when is reassigned' do
         before do
-          update_issue({ assignee: user2 })
+          update_issue(assignee: user2)
         end
 
         it 'marks previous assignee todos as done' do
@@ -144,7 +162,7 @@ describe Issues::UpdateService, services: true do
 
       context 'when the milestone change' do
         before do
-          update_issue({ milestone: create(:milestone) })
+          update_issue(milestone: create(:milestone))
         end
 
         it 'marks todos as done' do
@@ -154,7 +172,7 @@ describe Issues::UpdateService, services: true do
 
       context 'when the labels change' do
         before do
-          update_issue({ label_ids: [label.id] })
+          update_issue(label_ids: [label.id])
         end
 
         it 'marks todos as done' do
@@ -165,6 +183,7 @@ describe Issues::UpdateService, services: true do
 
     context 'when the issue is relabeled' do
       let!(:non_subscriber) { create(:user) }
+
       let!(:subscriber) do
         create(:user).tap do |u|
           label.toggle_subscription(u)
@@ -176,7 +195,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 +209,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 +220,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,13 +229,15 @@ describe Issues::UpdateService, services: true do
       end
     end
 
-    context 'when Issue has tasks' do
-      before { update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) }
+    context 'when issue has tasks' do
+      before do
+        update_issue(description: "- [ ] Task 1\n- [ ] Task 2")
+      end
 
-      it { expect(@issue.tasks?).to eq(true) }
+      it { expect(issue.tasks?).to eq(true) }
 
       context 'when tasks are marked as completed' do
-        before { update_issue({ description: "- [x] Task 1\n- [X] Task 2" }) }
+        before { update_issue(description: "- [x] Task 1\n- [X] Task 2") }
 
         it 'creates system note about task status change' do
           note1 = find_note('Marked the task **Task 1** as completed')
@@ -229,8 +250,8 @@ describe Issues::UpdateService, services: true do
 
       context 'when tasks are marked as incomplete' do
         before do
-          update_issue({ description: "- [x] Task 1\n- [X] Task 2" })
-          update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" })
+          update_issue(description: "- [x] Task 1\n- [X] Task 2")
+          update_issue(description: "- [ ] Task 1\n- [ ] Task 2")
         end
 
         it 'creates system note about task status change' do
@@ -244,8 +265,8 @@ describe Issues::UpdateService, services: true do
 
       context 'when tasks position has been modified' do
         before do
-          update_issue({ description: "- [x] Task 1\n- [X] Task 2" })
-          update_issue({ description: "- [x] Task 1\n- [ ] Task 3\n- [ ] Task 2" })
+          update_issue(description: "- [x] Task 1\n- [X] Task 2")
+          update_issue(description: "- [x] Task 1\n- [ ] Task 3\n- [ ] Task 2")
         end
 
         it 'does not create a system note' do
@@ -257,8 +278,8 @@ describe Issues::UpdateService, services: true do
 
       context 'when a Task list with a completed item is totally replaced' do
         before do
-          update_issue({ description: "- [ ] Task 1\n- [X] Task 2" })
-          update_issue({ description: "- [ ] One\n- [ ] Two\n- [ ] Three" })
+          update_issue(description: "- [ ] Task 1\n- [X] Task 2")
+          update_issue(description: "- [ ] One\n- [ ] Two\n- [ ] Three")
         end
 
         it 'does not create a system note referencing the position the old item' do
@@ -269,7 +290,7 @@ describe Issues::UpdateService, services: true do
 
         it 'does not generate a new note at all' do
           expect do
-            update_issue({ description: "- [ ] One\n- [ ] Two\n- [ ] Three" })
+            update_issue(description: "- [ ] One\n- [ ] Two\n- [ ] Three")
           end.not_to change { Note.count }
         end
       end
@@ -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] } }