Merge branch '337608-multiple-issues-sd-prevention' into 'master'

Create a note when replied to the email creating the service desk issue

See merge request gitlab-org/gitlab!71749

app/models/issue.rb

@@ -63,6 +63,7 @@ class Issue < ApplicationRecord
 
   has_many :issue_assignees
   has_many :issue_email_participants
+  has_one :email
   has_many :assignees, class_name: "User", through: :issue_assignees
   has_many :zoom_meetings
   has_many :user_mentions, class_name: "IssueUserMention", dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent

app/models/issue/email.rb

+# frozen_string_literal: true
+
+class Issue::Email < ApplicationRecord
+  self.table_name = 'issue_emails'
+
+  belongs_to :issue
+
+  validates :email_message_id, uniqueness: true, presence: true, length: { maximum: 1000 }
+  validates :issue, presence: true, uniqueness: true
+end

db/migrate/20211110092710_create_issue_emails.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class CreateIssueEmails < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def up
+    create_table :issue_emails do |t|
+      t.references :issue, index: true, null: false, unique: true, foreign_key: { on_delete: :cascade }
+      t.text :email_message_id, null: false, limit: 1000
+
+      t.index :email_message_id
+    end
+  end
+
+  def down
+    drop_table :issue_emails
+  end
+end

db/schema_migrations/20211110092710

+f6312d56d2ac77537383c8671d73ad202fed9bb8eddba4bdb24d19dbe821cdf3
\ No newline at end of file

db/structure.sql

@@ -15278,6 +15278,22 @@ CREATE SEQUENCE issue_email_participants_id_seq
 
 ALTER SEQUENCE issue_email_participants_id_seq OWNED BY issue_email_participants.id;
 
+CREATE TABLE issue_emails (
+    id bigint NOT NULL,
+    issue_id bigint NOT NULL,
+    email_message_id text NOT NULL,
+    CONSTRAINT check_5abf3e6aea CHECK ((char_length(email_message_id) <= 1000))
+);
+
+CREATE SEQUENCE issue_emails_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE issue_emails_id_seq OWNED BY issue_emails.id;
+
 CREATE TABLE issue_links (
     id integer NOT NULL,
     source_id integer NOT NULL,
@@ -21565,6 +21581,8 @@ ALTER TABLE ONLY issue_customer_relations_contacts ALTER COLUMN id SET DEFAULT n
 
 ALTER TABLE ONLY issue_email_participants ALTER COLUMN id SET DEFAULT nextval('issue_email_participants_id_seq'::regclass);
 
+ALTER TABLE ONLY issue_emails ALTER COLUMN id SET DEFAULT nextval('issue_emails_id_seq'::regclass);
+
 ALTER TABLE ONLY issue_links ALTER COLUMN id SET DEFAULT nextval('issue_links_id_seq'::regclass);
 
 ALTER TABLE ONLY issue_metrics ALTER COLUMN id SET DEFAULT nextval('issue_metrics_id_seq'::regclass);
@@ -23238,6 +23256,9 @@ ALTER TABLE ONLY issue_customer_relations_contacts
 ALTER TABLE ONLY issue_email_participants
     ADD CONSTRAINT issue_email_participants_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY issue_emails
+    ADD CONSTRAINT issue_emails_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY issue_links
     ADD CONSTRAINT issue_links_pkey PRIMARY KEY (id);
 
@@ -26159,6 +26180,10 @@ CREATE INDEX index_issue_customer_relations_contacts_on_contact_id ON issue_cust
 
 CREATE UNIQUE INDEX index_issue_email_participants_on_issue_id_and_lower_email ON issue_email_participants USING btree (issue_id, lower(email));
 
+CREATE INDEX index_issue_emails_on_email_message_id ON issue_emails USING btree (email_message_id);
+
+CREATE INDEX index_issue_emails_on_issue_id ON issue_emails USING btree (issue_id);
+
 CREATE INDEX index_issue_links_on_source_id ON issue_links USING btree (source_id);
 
 CREATE UNIQUE INDEX index_issue_links_on_source_id_and_target_id ON issue_links USING btree (source_id, target_id);
@@ -30992,6 +31017,9 @@ ALTER TABLE ONLY packages_packages
 ALTER TABLE ONLY cluster_platforms_kubernetes
     ADD CONSTRAINT fk_rails_e1e2cf841a FOREIGN KEY (cluster_id) REFERENCES clusters(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY issue_emails
+    ADD CONSTRAINT fk_rails_e2ee00a8f7 FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY vulnerability_finding_evidences
     ADD CONSTRAINT fk_rails_e3205a0c65 FOREIGN KEY (vulnerability_occurrence_id) REFERENCES vulnerability_occurrences(id) ON DELETE CASCADE;
 

lib/gitlab/database/gitlab_schemas.yml

@@ -261,6 +261,7 @@ issuable_severities: :gitlab_main
 issuable_slas: :gitlab_main
 issue_assignees: :gitlab_main
 issue_customer_relations_contacts: :gitlab_main
+issue_emails: :gitlab_main
 issue_email_participants: :gitlab_main
 issue_links: :gitlab_main
 issue_metrics: :gitlab_main

lib/gitlab/email/handler/service_desk_handler.rb

@@ -32,11 +32,11 @@ module Gitlab
         def execute
           raise ProjectNotFound if project.nil?
 
-          create_issue!
+          create_issue_or_note
 
           if from_address
             add_email_participant
-            send_thank_you_email
+            send_thank_you_email unless reply_email?
           end
         end
 
@@ -82,6 +82,14 @@ module Gitlab
           project.present? && slug == project.full_path_slug
         end
 
+        def create_issue_or_note
+          if reply_email?
+            create_note_from_reply_email
+          else
+            create_issue!
+          end
+        end
+
         def create_issue!
           @issue = ::Issues::CreateService.new(
             project: project,
@@ -97,11 +105,35 @@ module Gitlab
 
           raise InvalidIssueError unless @issue.persisted?
 
+          begin
+            ::Issue::Email.create!(issue: @issue, email_message_id: mail.message_id)
+          rescue StandardError => e
+            Gitlab::ErrorTracking.log_exception(e)
+          end
+
           if service_desk_setting&.issue_template_missing?
-            create_template_not_found_note(@issue)
+            create_template_not_found_note
+          end
+        end
+
+        def issue_from_reply_to
+          strong_memoize(:issue_from_reply_to) do
+            next unless mail.in_reply_to
+
+            Issue::Email.find_by_email_message_id(mail.in_reply_to)&.issue
           end
         end
 
+        def reply_email?
+          issue_from_reply_to.present?
+        end
+
+        def create_note_from_reply_email
+          @issue = issue_from_reply_to
+
+          create_note(message_including_reply)
+        end
+
         def send_thank_you_email
           Notify.service_desk_thank_you_email(@issue.id).deliver_later
           Gitlab::Metrics::BackgroundTransaction.current&.add_event(:service_desk_thank_you_email)
@@ -124,7 +156,7 @@ module Gitlab
           end
         end
 
-        def create_template_not_found_note(issue)
+        def create_template_not_found_note
           issue_template_key = service_desk_setting&.issue_template_key
 
           warning_note = <<-MD.strip_heredoc
@@ -132,15 +164,15 @@ module Gitlab
             Please check service desk settings and update the file to be used.
           MD
 
-          note_params = {
-            noteable: issue,
-            note: warning_note
-          }
+          create_note(warning_note)
+        end
 
+        def create_note(note)
           ::Notes::CreateService.new(
             project,
             User.support_bot,
-            note_params
+            noteable: @issue,
+            note: note
           ).execute
         end
 
@@ -157,6 +189,8 @@ module Gitlab
         end
 
         def add_email_participant
+          return if reply_email? && !Feature.enabled?(:issue_email_participants, @issue.project)
+
           @issue.issue_email_participants.create(email: from_address)
         end
       end

lib/gitlab/import_export/project/import_export.yml

@@ -747,6 +747,7 @@ excluded_attributes:
     - :service_desk_reply_to
     - :upvotes_count
     - :work_item_type_id
+    - :email_message_id
   merge_request: &merge_request_excluded_definition
     - :milestone_id
     - :sprint_id

spec/db/schema_spec.rb

@@ -53,6 +53,7 @@ RSpec.describe 'Database schema' do
     identities: %w[user_id],
     import_failures: %w[project_id],
     issues: %w[last_edited_by_id state_id],
+    issue_emails: %w[email_message_id],
     jira_tracker_data: %w[jira_issue_transition_id],
     keys: %w[user_id],
     label_links: %w[target_id],

spec/factories/issue_emails.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :issue_email, class: 'Issue::Email' do
+    issue
+    email_message_id { generate(:short_text) }
+  end
+end

spec/factories/sequences.rb

@@ -21,4 +21,5 @@ FactoryBot.define do
   sequence(:jira_branch) { |n| "feature/PROJ-#{n}" }
   sequence(:job_name) { |n| "job #{n}" }
   sequence(:work_item_type_name) { |n| "bug#{n}" }
+  sequence(:short_text) { |n| "someText#{n}" }
 end

spec/fixtures/emails/service_desk_forwarded.eml

@@ -8,7 +8,7 @@ Date: Thu, 13 Jun 2013 17:03:48 -0400
 From: Jake the Dog <jake.g@adventuretime.ooo>
 To: support@adventuretime.ooo
 Delivered-To: support@adventuretime.ooo
-Message-ID: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>
+Message-ID: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=fdskbsf@mail.gmail.com>
 Subject: The message subject! @all
 Mime-Version: 1.0
 Content-Type: text/plain;

spec/fixtures/emails/service_desk_reply.eml

+Return-Path: <alan@adventuretime.ooo>
+Received: from iceking.adventuretime.ooo ([unix socket]) by iceking (Cyrus v2.2.13-Debian-2.2.13-19+squeeze3) with LMTPA; Thu, 13 Jun 2013 17:03:50 -0400
+Received: from mail-ie0-x234.google.com (mail-ie0-x234.google.com [IPv6:2607:f8b0:4001:c03::234]) by iceking.adventuretime.ooo (8.14.3/8.14.3/Debian-9.4) with ESMTP id r5DL3nFJ016967 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for <incoming+gitlabhq/gitlabhq@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 17:03:50 -0400
+Received: by mail-ie0-f180.google.com with SMTP id f4so21977375iea.25 for <incoming+email-test-project_id-issue-@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 14:03:48 -0700
+Received: by 10.0.0.1 with HTTP; Thu, 13 Jun 2013 14:03:48 -0700
+Date: Thu, 13 Jun 2013 17:03:48 -0400
+From: Jake the Dog <alan@adventuretime.ooo>
+To: incoming+email-test-project_id-issue-@appmail.adventuretime.ooo
+Message-ID: <CAH_Wr+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>
+In-Reply-To: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>
+Subject: The message subject! @all
+Mime-Version: 1.0
+Content-Type: text/plain;
+ charset=ISO-8859-1
+Content-Transfer-Encoding: 7bit
+X-Sieve: CMU Sieve 2.2
+X-Received: by 10.0.0.1 with SMTP id n7mr11234144ipb.85.1371157428600; Thu,
+ 13 Jun 2013 14:03:48 -0700 (PDT)
+X-Scanned-By: MIMEDefang 2.69 on IPv6:2001:470:1d:165::1
+
+Service desk reply!
+
+/label ~label2

spec/lib/gitlab/email/handler/service_desk_handler_spec.rb

@@ -12,6 +12,8 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
 
   let(:email_raw) { email_fixture('emails/service_desk.eml') }
   let(:author_email) { 'jake@adventuretime.ooo' }
+  let(:message_id) { 'CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com' }
+
   let_it_be(:group) { create(:group, :private, name: "email") }
 
   let(:expected_description) do
@@ -40,6 +42,7 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
         expect(new_issue.all_references.all).to be_empty
         expect(new_issue.title).to eq("The message subject! @all")
         expect(new_issue.description).to eq(expected_description.strip)
+        expect(new_issue.email&.email_message_id).to eq(message_id)
       end
 
       it 'creates an issue_email_participant' do
@@ -72,6 +75,95 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
         it_behaves_like 'a new issue request'
       end
 
+      context 'when replying to issue creation email' do
+        def receive_reply
+          reply_email_raw = email_fixture('emails/service_desk_reply.eml')
+
+          second_receiver = Gitlab::Email::Receiver.new(reply_email_raw)
+          second_receiver.execute
+        end
+
+        context 'when an issue with message_id has been found' do
+          before do
+            receiver.execute
+          end
+
+          subject do
+            receive_reply
+          end
+
+          it 'does not create an additional issue' do
+            expect { subject }.not_to change { Issue.count }
+          end
+
+          it 'adds a comment to the created issue' do
+            subject
+
+            notes = Issue.last.notes
+            new_note = notes.first
+
+            expect(notes.count).to eq(1)
+            expect(new_note.note).to eq("Service desk reply!\n\n`/label ~label2`")
+            expect(new_note.author).to eql(User.support_bot)
+          end
+
+          it 'does not send thank you email' do
+            expect(Notify).not_to receive(:service_desk_thank_you_email)
+
+            subject
+          end
+
+          context 'when issue_email_participants FF is enabled' do
+            it 'creates 2 issue_email_participants' do
+              subject
+
+              expect(Issue.last.issue_email_participants.map(&:email))
+                .to match_array(%w(alan@adventuretime.ooo jake@adventuretime.ooo))
+            end
+          end
+
+          context 'when issue_email_participants FF is disabled' do
+            before do
+              stub_feature_flags(issue_email_participants: false)
+            end
+
+            it 'creates only 1 issue_email_participant' do
+              subject
+
+              expect(Issue.last.issue_email_participants.map(&:email))
+                .to match_array(%w(jake@adventuretime.ooo))
+            end
+          end
+        end
+
+        context 'when an issue with message_id has not been found' do
+          subject do
+            receive_reply
+          end
+
+          it 'creates a new issue correctly' do
+            expect { subject }.to change { Issue.count }.by(1)
+
+            issue = Issue.last
+
+            expect(issue.description).to eq("Service desk reply!\n\n`/label ~label2`")
+          end
+
+          it 'sends thank you email once' do
+            expect(Notify).to receive(:service_desk_thank_you_email).once.and_return(double(deliver_later: true))
+
+            subject
+          end
+
+          it 'creates 1 issue_email_participant' do
+            subject
+
+            expect(Issue.last.issue_email_participants.map(&:email))
+              .to match_array(%w(alan@adventuretime.ooo))
+          end
+        end
+      end
+
       context 'when using issue templates' do
         let_it_be(:user) { create(:user) }
 
@@ -270,6 +362,20 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
       end
     end
 
+    context 'when issue email creation fails' do
+      before do
+        allow(::Issue::Email).to receive(:create!).and_raise(StandardError)
+      end
+
+      it 'still creates a new issue' do
+        expect { receiver.execute }.to change { Issue.count }.by(1)
+      end
+
+      it 'does not create issue email record' do
+        expect { receiver.execute }.not_to change { Issue::Email.count }
+      end
+    end
+
     context 'when rate limiting is in effect', :freeze_time, :clean_gitlab_redis_rate_limiting do
       let(:receiver) { Gitlab::Email::Receiver.new(email_raw) }
 
@@ -291,19 +397,19 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
           rescue RateLimitedService::RateLimitedError
           end.to change { Issue.count }.by(1)
         end
+      end
 
-        context 'when requests are sent by different users' do
-          let(:email_raw_2) { email_fixture('emails/service_desk_forwarded.eml') }
-          let(:receiver2) { Gitlab::Email::Receiver.new(email_raw_2) }
+      context 'when requests are sent by different users' do
+        let(:email_raw_2) { email_fixture('emails/service_desk_forwarded.eml') }
+        let(:receiver2) { Gitlab::Email::Receiver.new(email_raw_2) }
 
-          subject do
-            receiver.execute
-            receiver2.execute
-          end
+        subject do
+          receiver.execute
+          receiver2.execute
+        end
 
-          it 'creates 2 issues' do
-            expect { subject }.to change { Issue.count }.by(2)
-          end
+        it 'creates 2 issues' do
+          expect { subject }.to change { Issue.count }.by(2)
         end
       end
 
@@ -389,6 +495,7 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do
     context 'when the email is forwarded through an alias' do
       let(:author_email) { 'jake.g@adventuretime.ooo' }
       let(:email_raw) { email_fixture('emails/service_desk_forwarded.eml') }
+      let(:message_id) { 'CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=fdskbsf@mail.gmail.com' }
 
       it_behaves_like 'a new issue request'
     end

spec/lib/gitlab/import_export/all_models.yml

@@ -61,6 +61,7 @@ issues:
 - pending_escalations
 - customer_relations_contacts
 - issue_customer_relations_contacts
+- email
 work_item_type:
 - issues
 events:

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -33,6 +33,7 @@ Issue:
 - health_status
 - external_key
 - issue_type
+- email_message_id
 Event:
 - id
 - target_type

spec/models/issue/email_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issue::Email do
+  describe 'Associations' do
+    it { is_expected.to belong_to(:issue) }
+  end
+
+  describe 'Validations' do
+    subject { build(:issue_email) }
+
+    it { is_expected.to validate_presence_of(:issue) }
+    it { is_expected.to validate_uniqueness_of(:issue) }
+    it { is_expected.to validate_uniqueness_of(:email_message_id) }
+    it { is_expected.to validate_length_of(:email_message_id).is_at_most(1000) }
+    it { is_expected.to validate_presence_of(:email_message_id) }
+  end
+end

spec/models/issue_spec.rb

@@ -32,6 +32,7 @@ RSpec.describe Issue do
     it { is_expected.to have_and_belong_to_many(:self_managed_prometheus_alert_events) }
     it { is_expected.to have_many(:prometheus_alerts) }
     it { is_expected.to have_many(:issue_email_participants) }
+    it { is_expected.to have_one(:email) }
     it { is_expected.to have_many(:timelogs).autosave(true) }
     it { is_expected.to have_one(:incident_management_issuable_escalation_status) }
     it { is_expected.to have_many(:issue_customer_relations_contacts) }