Use the To header when sending pipeline emails

A missing To header can cause issues with spam filters. And since we no
longer render job logs in these emails, generating these emails
individually isn't expensive anymore.

Changelog: fixed

app/mailers/emails/pipelines.rb

@@ -2,21 +2,23 @@
 
 module Emails
   module Pipelines
-    def pipeline_success_email(pipeline, recipients)
-      pipeline_mail(pipeline, recipients, 'Successful')
+    def pipeline_success_email(pipeline, recipient)
+      pipeline_mail(pipeline, recipient, 'Successful')
     end
 
-    def pipeline_failed_email(pipeline, recipients)
-      pipeline_mail(pipeline, recipients, 'Failed')
+    def pipeline_failed_email(pipeline, recipient)
+      pipeline_mail(pipeline, recipient, 'Failed')
     end
 
-    def pipeline_fixed_email(pipeline, recipients)
-      pipeline_mail(pipeline, recipients, 'Fixed')
+    def pipeline_fixed_email(pipeline, recipient)
+      pipeline_mail(pipeline, recipient, 'Fixed')
     end
 
     private
 
-    def pipeline_mail(pipeline, recipients, status)
+    def pipeline_mail(pipeline, recipient, status)
+      raise ArgumentError if recipient.is_a?(Array)
+
       @project = pipeline.project
       @pipeline = pipeline
 
@@ -28,10 +30,7 @@ module Emails
 
       add_headers
 
-      # We use bcc here because we don't want to generate these emails for a
-      # thousand times. This could be potentially expensive in a loop, and
-      # recipients would contain all project watchers so it could be a lot.
-      mail(bcc: recipients,
+      mail(to: recipient,
            subject: subject(pipeline_subject(status))) do |format|
         format.html { render layout: 'mailer' }
         format.text { render layout: 'mailer' }

app/models/integrations/emails_on_push.rb

@@ -97,7 +97,7 @@ module Integrations
       return if recipients.blank?
 
       if self.class.valid_recipients(recipients).size > RECIPIENTS_LIMIT
-        errors.add(:recipients, s_("EmailsOnPushService|can't exceed %{recipients_limit}") % { recipients_limit: RECIPIENTS_LIMIT })
+        errors.add(:recipients, s_("Integrations|can't exceed %{recipients_limit}") % { recipients_limit: RECIPIENTS_LIMIT })
       end
     end
   end

app/models/integrations/pipelines_email.rb

@@ -4,9 +4,12 @@ module Integrations
   class PipelinesEmail < Integration
     include NotificationBranchSelection
 
+    RECIPIENTS_LIMIT = 30
+
     prop_accessor :recipients, :branches_to_be_notified
     boolean_accessor :notify_only_broken_pipelines, :notify_only_default_branch
     validates :recipients, presence: true, if: :validate_recipients?
+    validate :number_of_recipients_within_limit, if: :validate_recipients?
 
     def initialize_properties
       if properties.nil?
@@ -49,7 +52,7 @@ module Integrations
       return unless supported_events.include?(data[:object_kind])
       return unless force || should_pipeline_be_notified?(data)
 
-      all_recipients = retrieve_recipients(data)
+      all_recipients = retrieve_recipients
 
       return unless all_recipients.any?
 
@@ -99,8 +102,18 @@ module Integrations
       end
     end
 
-    def retrieve_recipients(data)
+    def retrieve_recipients
       recipients.to_s.split(/[,\r\n ]+/).reject(&:empty?)
     end
+
+    private
+
+    def number_of_recipients_within_limit
+      return if recipients.blank?
+
+      if retrieve_recipients.size > RECIPIENTS_LIMIT
+        errors.add(:recipients, s_("Integrations|can't exceed %{recipients_limit}") % { recipients_limit: RECIPIENTS_LIMIT })
+      end
+    end
   end
 end

app/services/notification_service.rb

@@ -598,8 +598,8 @@ class NotificationService
       user.notification_email_for(pipeline.project.group)
     end
 
-    if recipients.any?
-      mailer.public_send(email_template, pipeline, recipients).deliver_later
+    recipients.each do |recipient|
+      mailer.public_send(email_template, pipeline, recipient).deliver_later
     end
   end
 

locale/gitlab.pot

@@ -12637,9 +12637,6 @@ msgstr ""
 msgid "EmailsOnPushService|Send notifications from the committer's email address if the domain matches the domain used by your GitLab instance (such as %{domains})."
 msgstr ""
 
-msgid "EmailsOnPushService|can't exceed %{recipients_limit}"
-msgstr ""
-
 msgid "EmailsOnPushService|tanuki@example.com gitlab@example.com"
 msgstr ""
 
@@ -18602,6 +18599,9 @@ msgstr ""
 msgid "Integrations|ZenTao issues display here when you create issues in your project in ZenTao."
 msgstr ""
 
+msgid "Integrations|can't exceed %{recipients_limit}"
+msgstr ""
+
 msgid "Interactive mode"
 msgstr ""
 

spec/mailers/emails/pipelines_spec.rb

@@ -71,10 +71,19 @@ RSpec.describe Emails::Pipelines do
     end
   end
 
+  shared_examples_for 'only accepts a single recipient' do
+    let(:recipient) { ['test@gitlab.com', 'test2@gitlab.com'] }
+
+    it 'raises an ArgumentError' do
+      expect { subject.deliver_now }.to raise_error(ArgumentError)
+    end
+  end
+
   describe '#pipeline_success_email' do
-    subject { Notify.pipeline_success_email(pipeline, pipeline.user.try(:email)) }
+    subject { Notify.pipeline_success_email(pipeline, recipient) }
 
     let(:pipeline) { create(:ci_pipeline, project: project, ref: ref, sha: sha) }
+    let(:recipient) { pipeline.user.try(:email) }
     let(:ref) { 'master' }
     let(:sha) { project.commit(ref).sha }
 
@@ -93,12 +102,15 @@ RSpec.describe Emails::Pipelines do
         stub_config_setting(email_subject_suffix: email_subject_suffix)
       end
     end
+
+    it_behaves_like 'only accepts a single recipient'
   end
 
   describe '#pipeline_failed_email' do
-    subject { Notify.pipeline_failed_email(pipeline, pipeline.user.try(:email)) }
+    subject { Notify.pipeline_failed_email(pipeline, recipient) }
 
     let(:pipeline) { create(:ci_pipeline, project: project, ref: ref, sha: sha) }
+    let(:recipient) { pipeline.user.try(:email) }
     let(:ref) { 'master' }
     let(:sha) { project.commit(ref).sha }
 
@@ -106,12 +118,15 @@ RSpec.describe Emails::Pipelines do
       let(:status) { 'Failed' }
       let(:status_text) { "Pipeline ##{pipeline.id} has failed!" }
     end
+
+    it_behaves_like 'only accepts a single recipient'
   end
 
   describe '#pipeline_fixed_email' do
     subject { Notify.pipeline_fixed_email(pipeline, pipeline.user.try(:email)) }
 
     let(:pipeline) { create(:ci_pipeline, project: project, ref: ref, sha: sha) }
+    let(:recipient) { pipeline.user.try(:email) }
     let(:ref) { 'master' }
     let(:sha) { project.commit(ref).sha }
 
@@ -119,5 +134,7 @@ RSpec.describe Emails::Pipelines do
       let(:status) { 'Fixed' }
       let(:status_text) { "Pipeline has been fixed and ##{pipeline.id} has passed!" }
     end
+
+    it_behaves_like 'only accepts a single recipient'
   end
 end

spec/models/ci/pipeline_spec.rb

@@ -3361,7 +3361,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
 
     shared_examples 'sending a notification' do
       it 'sends an email', :sidekiq_might_not_need_inline do
-        should_only_email(pipeline.user, kind: :bcc)
+        should_only_email(pipeline.user)
       end
     end
 

spec/models/integrations/pipelines_email_spec.rb

@@ -35,6 +35,42 @@ RSpec.describe Integrations::PipelinesEmail, :mailer do
 
       it { is_expected.not_to validate_presence_of(:recipients) }
     end
+
+    describe 'validates number of recipients' do
+      before do
+        stub_const("#{described_class}::RECIPIENTS_LIMIT", 2)
+      end
+
+      subject(:integration) { described_class.new(project: project, recipients: recipients, active: true) }
+
+      context 'valid number of recipients' do
+        let(:recipients) { 'foo@bar.com, , ' }
+
+        it 'does not count empty emails' do
+          is_expected.to be_valid
+        end
+      end
+
+      context 'invalid number of recipients' do
+        let(:recipients) { 'foo@bar.com bar@foo.com bob@gitlab.com' }
+
+        it { is_expected.not_to be_valid }
+
+        it 'adds an error message' do
+          integration.valid?
+
+          expect(integration.errors).to contain_exactly('Recipients can\'t exceed 2')
+        end
+
+        context 'when integration is not active' do
+          before do
+            integration.active = false
+          end
+
+          it { is_expected.to be_valid }
+        end
+      end
+    end
   end
 
   shared_examples 'sending email' do |branches_to_be_notified: nil|
@@ -50,7 +86,7 @@ RSpec.describe Integrations::PipelinesEmail, :mailer do
     it 'sends email' do
       emails = receivers.map { |r| double(notification_email_or_default: r) }
 
-      should_only_email(*emails, kind: :bcc)
+      should_only_email(*emails)
     end
   end
 

spec/services/notification_service_spec.rb

@@ -3040,7 +3040,7 @@ RSpec.describe NotificationService, :mailer do
           it 'emails only the creator' do
             notification.pipeline_finished(pipeline)
 
-            should_only_email(u_custom_notification_enabled, kind: :bcc)
+            should_only_email(u_custom_notification_enabled)
           end
 
           it_behaves_like 'project emails are disabled' do
@@ -3063,7 +3063,7 @@ RSpec.describe NotificationService, :mailer do
             it 'sends to group notification email' do
               notification.pipeline_finished(pipeline)
 
-              expect(email_recipients(kind: :bcc).first).to eq(group_notification_email)
+              expect(email_recipients.first).to eq(group_notification_email)
             end
           end
         end
@@ -3076,7 +3076,7 @@ RSpec.describe NotificationService, :mailer do
           it 'emails only the creator' do
             notification.pipeline_finished(pipeline)
 
-            should_only_email(u_member, kind: :bcc)
+            should_only_email(u_member)
           end
 
           it_behaves_like 'project emails are disabled' do
@@ -3098,7 +3098,7 @@ RSpec.describe NotificationService, :mailer do
             it 'sends to group notification email' do
               notification.pipeline_finished(pipeline)
 
-              expect(email_recipients(kind: :bcc).first).to eq(group_notification_email)
+              expect(email_recipients.first).to eq(group_notification_email)
             end
           end
         end
@@ -3110,7 +3110,7 @@ RSpec.describe NotificationService, :mailer do
           end
 
           it 'emails only the creator' do
-            should_only_email(u_watcher, kind: :bcc)
+            should_only_email(u_watcher)
           end
         end
 
@@ -3121,7 +3121,7 @@ RSpec.describe NotificationService, :mailer do
           end
 
           it 'emails only the creator' do
-            should_only_email(u_custom_notification_unset, kind: :bcc)
+            should_only_email(u_custom_notification_unset)
           end
         end
 
@@ -3143,7 +3143,7 @@ RSpec.describe NotificationService, :mailer do
           end
 
           it 'emails only the creator' do
-            should_only_email(u_custom_notification_enabled, kind: :bcc)
+            should_only_email(u_custom_notification_enabled)
           end
         end
 
@@ -3170,7 +3170,7 @@ RSpec.describe NotificationService, :mailer do
           it 'emails only the creator' do
             notification.pipeline_finished(pipeline, ref_status: ref_status)
 
-            should_only_email(u_member, kind: :bcc)
+            should_only_email(u_member)
           end
 
           it_behaves_like 'project emails are disabled' do
@@ -3192,7 +3192,7 @@ RSpec.describe NotificationService, :mailer do
             it 'sends to group notification email' do
               notification.pipeline_finished(pipeline, ref_status: ref_status)
 
-              expect(email_recipients(kind: :bcc).first).to eq(group_notification_email)
+              expect(email_recipients.first).to eq(group_notification_email)
             end
           end
         end
@@ -3204,7 +3204,7 @@ RSpec.describe NotificationService, :mailer do
           end
 
           it 'emails only the creator' do
-            should_only_email(u_watcher, kind: :bcc)
+            should_only_email(u_watcher)
           end
         end
 
@@ -3215,7 +3215,7 @@ RSpec.describe NotificationService, :mailer do
           end
 
           it 'emails only the creator' do
-            should_only_email(u_custom_notification_unset, kind: :bcc)
+            should_only_email(u_custom_notification_unset)
           end
         end
 
@@ -3236,7 +3236,7 @@ RSpec.describe NotificationService, :mailer do
 
             notification.pipeline_finished(pipeline, ref_status: ref_status)
 
-            should_only_email(u_custom_notification_enabled, kind: :bcc)
+            should_only_email(u_custom_notification_enabled)
           end
         end
       end