Start adding Gitlab::HookData::IssuableBuilder

Signed-off-by: Rémy Coutable <remy@rymai.me>

6 files changed, 132 insertions, 60 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index e8a6c37d0b9..27f4dedffd3 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -256,29 +256,22 @@ module Issuable
     participants(user).include?(user)
   end
 
-  def to_hook_data(user, old_labels: [])
+  def to_hook_data(user, old_labels: [], old_assignees: [])
     changes = previous_changes
+
     if old_labels != labels
-      changes[:labels] = [old_labels.map(&:name), labels.map(&:name)]
+      changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)]
     end
 
-    hook_data = {
-      object_kind: self.class.name.underscore,
-      user: user.hook_attrs,
-      project: project.hook_attrs,
-      object_attributes: hook_attrs,
-      labels: labels.map(&:hook_attrs),
-      changes: changes,
-      # DEPRECATED
-      repository: project.hook_attrs.slice(:name, :url, :description, :homepage)
-    }
-    if self.is_a?(Issue)
-      hook_data[:assignees] = assignees.map(&:hook_attrs) if assignees.any?
-    else
-      hook_data[:assignee] = assignee.hook_attrs if assignee
+    if old_assignees != assignees
+      if self.is_a?(Issue)
+        changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)]
+      else
+        changes[:assignee] = [old_assignees&.first&.hook_attrs, assignee&.hook_attrs]
+      end
     end
 
-    hook_data
+    Gitlab::HookData::IssuableBuilder.new(self).build(user: user, changes: changes)
   end
 
   def labels_array
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 058ee144ee4..2527622a989 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -18,6 +18,36 @@ class Issue < ActiveRecord::Base
   DueThisWeek   = DueDateStruct.new('Due This Week', 'week').freeze
   DueThisMonth  = DueDateStruct.new('Due This Month', 'month').freeze
 
+  SAFE_HOOK_ATTRIBUTES = %i[
+    assignee_id
+    author_id
+    branch_name
+    closed_at
+    confidential
+    created_at
+    deleted_at
+    description
+    due_date
+    id
+    iid
+    last_edited_at
+    last_edited_by_id
+    milestone_id
+    moved_to_id
+    project_id
+    relative_position
+    state
+    time_estimate
+    title
+    updated_at
+    updated_by_id
+  ].freeze
+
+  SAFE_HOOK_RELATIONS = %i[
+    assignees
+    labels
+  ].freeze
+
   belongs_to :project
   belongs_to :moved_to, class_name: 'Issue'
 
@@ -74,21 +104,6 @@ class Issue < ActiveRecord::Base
     end
   end
 
-  def hook_attrs
-    assignee_ids = self.assignee_ids
-
-    attrs = {
-      url: Gitlab::UrlBuilder.build(self),
-      total_time_spent: total_time_spent,
-      human_total_time_spent: human_total_time_spent,
-      human_time_estimate: human_time_estimate,
-      assignee_ids: assignee_ids,
-      assignee_id: assignee_ids.first # This key is deprecated
-    }
-
-    attributes.merge!(attrs)
-  end
-
   def self.reference_prefix
     '#'
   end
@@ -132,6 +147,30 @@ class Issue < ActiveRecord::Base
               "id DESC")
   end
 
+  def self.safe_hook_attributes
+    SAFE_HOOK_ATTRIBUTES
+  end
+
+  def self.safe_hook_relations
+    SAFE_HOOK_RELATIONS
+  end
+
+  def hook_attrs
+    assignee_ids = self.assignee_ids
+
+    attrs = {
+      url: Gitlab::UrlBuilder.build(self),
+      total_time_spent: total_time_spent,
+      human_total_time_spent: human_total_time_spent,
+      human_time_estimate: human_time_estimate,
+      assignee_ids: assignee_ids,
+      assignee_id: assignee_ids.first # This key is deprecated
+    }
+
+    attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes)
+      .merge!(attrs)
+  end
+
   # Returns a Hash of attributes to be used for Twitter card metadata
   def card_attributes
     {
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 52a6c31503a..ce32b7cb05e 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -7,6 +7,41 @@ class MergeRequest < ActiveRecord::Base
   include IgnorableColumn
   include CreatedAtFilterable
 
+  SAFE_HOOK_ATTRIBUTES = %i[
+    assignee_id
+    author_id
+    created_at
+    deleted_at
+    description
+    head_pipeline_id
+    id
+    iid
+    last_edited_at
+    last_edited_by_id
+    merge_commit_sha
+    merge_error
+    merge_params
+    merge_status
+    merge_user_id
+    merge_when_pipeline_succeeds
+    milestone_id
+    ref_fetched
+    source_branch
+    source_project_id
+    state
+    target_branch
+    target_project_id
+    time_estimate
+    title
+    updated_at
+    updated_by_id
+  ].freeze
+
+  SAFE_HOOK_RELATIONS = %i[
+    assignee
+    labels
+  ].freeze
+
   ignore_column :locked_at
 
   belongs_to :target_project, class_name: "Project"
@@ -179,6 +214,30 @@ class MergeRequest < ActiveRecord::Base
     work_in_progress?(title) ? title : "WIP: #{title}"
   end
 
+  def self.safe_hook_attributes
+    SAFE_HOOK_ATTRIBUTES
+  end
+
+  def self.safe_hook_relations
+    SAFE_HOOK_RELATIONS
+  end
+
+  def hook_attrs
+    attrs = {
+      url: Gitlab::UrlBuilder.build(self),
+      source: source_project.try(:hook_attrs),
+      target: target_project.hook_attrs,
+      last_commit: diff_head_commit&.hook_attrs,
+      work_in_progress: work_in_progress?,
+      total_time_spent: total_time_spent,
+      human_total_time_spent: human_total_time_spent,
+      human_time_estimate: human_time_estimate
+    }
+
+    attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes)
+      .merge!(attrs)
+  end
+
   # Returns a Hash of attributes to be used for Twitter card metadata
   def card_attributes
     {
@@ -587,25 +646,6 @@ class MergeRequest < ActiveRecord::Base
     !discussions_to_be_resolved?
   end
 
-  def hook_attrs
-    attrs = {
-      url: Gitlab::UrlBuilder.build(self),
-      source: source_project.try(:hook_attrs),
-      target: target_project.hook_attrs,
-      last_commit: nil,
-      work_in_progress: work_in_progress?,
-      total_time_spent: total_time_spent,
-      human_total_time_spent: human_total_time_spent,
-      human_time_estimate: human_time_estimate
-    }
-
-    if diff_head_commit
-      attrs[:last_commit] = diff_head_commit.hook_attrs
-    end
-
-    attributes.merge!(attrs)
-  end
-
   def for_fork?
     target_project != source_project
   end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 1d6b48ccf56..d61a342ebad 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -255,7 +255,7 @@ class IssuableBaseService < BaseService
         invalidate_cache_counts(issuable, users: affected_assignees.compact)
         after_update(issuable)
         issuable.create_new_cross_references!(current_user)
-        execute_hooks(issuable, 'update', old_labels: old_labels)
+        execute_hooks(issuable, 'update', old_labels: old_labels, old_assignees: old_assignees)
 
         issuable.update_project_counter_caches if update_project_counters
       end
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index f4fff4bf646..735257c4779 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -1,7 +1,7 @@
 module Issues
   class BaseService < ::IssuableBaseService
-    def hook_data(issue, action, old_labels: [])
-      hook_data = issue.to_hook_data(current_user, old_labels: old_labels)
+    def hook_data(issue, action, old_labels: [], old_assignees: [])
+      hook_data = issue.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees)
       hook_data[:object_attributes][:action] = action
 
       hook_data
@@ -22,8 +22,8 @@ module Issues
         issue, issue.project, current_user, old_assignees)
     end
 
-    def execute_hooks(issue, action = 'open', old_labels: [])
-      issue_data  = hook_data(issue, action, old_labels: old_labels)
+    def execute_hooks(issue, action = 'open', old_labels: [], old_assignees: [])
+      issue_data  = hook_data(issue, action, old_labels: old_labels, old_assignees: old_assignees)
       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)
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index 7cdb45ca552..112606a82d7 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -18,8 +18,8 @@ module MergeRequests
       super if changed_title
     end
 
-    def hook_data(merge_request, action, old_rev: nil, old_labels: [])
-      hook_data = merge_request.to_hook_data(current_user, old_labels: old_labels)
+    def hook_data(merge_request, action, old_rev: nil, old_labels: [], old_assignees: [])
+      hook_data = merge_request.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees)
       hook_data[:object_attributes][:action] = action
       if old_rev && !Gitlab::Git.blank_ref?(old_rev)
         hook_data[:object_attributes][:oldrev] = old_rev
@@ -28,9 +28,9 @@ module MergeRequests
       hook_data
     end
 
-    def execute_hooks(merge_request, action = 'open', old_rev: nil, old_labels: [])
+    def execute_hooks(merge_request, action = 'open', old_rev: nil, old_labels: [], old_assignees: [])
       if merge_request.project
-        merge_data = hook_data(merge_request, action, old_rev: old_rev, old_labels: old_labels)
+        merge_data = hook_data(merge_request, action, old_rev: old_rev, old_labels: old_labels, old_assignees: old_assignees)
         merge_request.project.execute_hooks(merge_data, :merge_request_hooks)
         merge_request.project.execute_services(merge_data, :merge_request_hooks)
       end