From 3b5330e9bcf66d7f66ff5b51dbb60adcf2197f67 Mon Sep 17 00:00:00 2001
From: Heinrich Lee Yu <heinrich@gitlab.com>
Date: Wed, 3 Nov 2021 09:18:07 +0000
Subject: [PATCH] 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 | 21 +++++-----
app/models/integrations/emails_on_push.rb | 2 +-
app/models/integrations/pipelines_email.rb | 17 ++++++++-
app/services/notification_service.rb | 4 +-
locale/gitlab.pot | 6 +--
spec/mailers/emails/pipelines_spec.rb | 21 +++++++++-
spec/models/ci/pipeline_spec.rb | 2 +-
.../integrations/pipelines_email_spec.rb | 38 ++++++++++++++++++-
spec/services/notification_service_spec.rb | 24 ++++++------
9 files changed, 100 insertions(+), 35 deletions(-)
diff --git a/app/mailers/emails/pipelines.rb b/app/mailers/emails/pipelines.rb
index 1b27d06239136..5363ad637715f 100644
--- a/app/mailers/emails/pipelines.rb
+++ b/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 @@ def pipeline_mail(pipeline, recipients, status)
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' }
diff --git a/app/models/integrations/emails_on_push.rb b/app/models/integrations/emails_on_push.rb
index 26c47d53918ab..a9cd67550dc3e 100644
--- a/app/models/integrations/emails_on_push.rb
+++ b/app/models/integrations/emails_on_push.rb
@@ -97,7 +97,7 @@ def number_of_recipients_within_limit
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
diff --git a/app/models/integrations/pipelines_email.rb b/app/models/integrations/pipelines_email.rb
index e5d94bab2db5b..6dc41958daa8b 100644
--- a/app/models/integrations/pipelines_email.rb
+++ b/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 @@ def execute(data, force: false)
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 @@ def notify_for_pipeline?(data)
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
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index afc9015e758bf..6ad3a74b85d51 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -598,8 +598,8 @@ def pipeline_finished(pipeline, ref_status: nil, recipients: nil)
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
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 9a07fac83681a..86b5cbf638ce7 100644
--- a/locale/gitlab.pot
+++ b/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 ""
diff --git a/spec/mailers/emails/pipelines_spec.rb b/spec/mailers/emails/pipelines_spec.rb
index b9bc53625ac9b..3a2eb105964c9 100644
--- a/spec/mailers/emails/pipelines_spec.rb
+++ b/spec/mailers/emails/pipelines_spec.rb
@@ -71,10 +71,19 @@
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 @@
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 @@
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 @@
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
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 8d25502ebb510..e573a6ef780a8 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -3361,7 +3361,7 @@ def create_pipeline(status, ref, sha)
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
diff --git a/spec/models/integrations/pipelines_email_spec.rb b/spec/models/integrations/pipelines_email_spec.rb
index afd9d71ebc425..d70f104b965df 100644
--- a/spec/models/integrations/pipelines_email_spec.rb
+++ b/spec/models/integrations/pipelines_email_spec.rb
@@ -35,6 +35,42 @@
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 @@
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
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 48718cbc24a82..fbf5b1833658a 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -3040,7 +3040,7 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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 @@ def create_pipeline(user, status)
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
--
GitLab