From c3a940000ea20d6682313640e1a0fda9ff68dbdf Mon Sep 17 00:00:00 2001
From: Pawel Chojnacki <pawel@chojnacki.ws>
Date: Wed, 12 Oct 2016 19:07:36 +0200
Subject: [PATCH] Handles unsubscribe from notifications via email
- allows unsubscription processing of email in format "reply+%{key}+unsubscribe@acme.com" (example)
- if config.address includes %{key} and replies are enabled every unsubscriable message will include mailto: link in its List-Unsubscribe header
---
app/mailers/notify.rb | 20 ++++--
...ddress-to-unsubscribe-list-header-in-email | 4 ++
lib/gitlab/email/handler.rb | 3 +-
lib/gitlab/email/handler/base_handler.rb | 43 +------------
.../email/handler/create_issue_handler.rb | 1 +
.../email/handler/create_note_handler.rb | 7 ++-
lib/gitlab/email/handler/reply_processing.rb | 54 ++++++++++++++++
.../email/handler/unsubscribe_handler.rb | 32 ++++++++++
lib/gitlab/incoming_email.rb | 9 ++-
spec/lib/gitlab/email/email_shared_blocks.rb | 2 +-
.../handler/create_issue_handler_spec.rb | 2 +-
.../email/handler/create_note_handler_spec.rb | 2 +-
.../email/handler/unsubscribe_handler_spec.rb | 61 +++++++++++++++++++
spec/lib/gitlab/incoming_email_spec.rb | 42 +++++++++++++
spec/support/notify_shared_examples.rb | 17 +++++-
15 files changed, 243 insertions(+), 56 deletions(-)
create mode 100644 changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email
create mode 100644 lib/gitlab/email/handler/reply_processing.rb
create mode 100644 lib/gitlab/email/handler/unsubscribe_handler.rb
create mode 100644 spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb
diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb
index 0bc1c19e9cd..0cd3456b4de 100644
--- a/app/mailers/notify.rb
+++ b/app/mailers/notify.rb
@@ -107,15 +107,11 @@ class Notify < BaseMailer
def mail_thread(model, headers = {})
add_project_headers
+ add_unsubscription_headers_and_links
+
headers["X-GitLab-#{model.class.name}-ID"] = model.id
headers['X-GitLab-Reply-Key'] = reply_key
- if !@labels_url && @sent_notification && @sent_notification.unsubscribable?
- headers['List-Unsubscribe'] = "<#{unsubscribe_sent_notification_url(@sent_notification, force: true)}>"
-
- @sent_notification_url = unsubscribe_sent_notification_url(@sent_notification)
- end
-
if Gitlab::IncomingEmail.enabled?
address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key))
address.display_name = @project.name_with_namespace
@@ -171,4 +167,16 @@ class Notify < BaseMailer
headers['X-GitLab-Project-Id'] = @project.id
headers['X-GitLab-Project-Path'] = @project.path_with_namespace
end
+
+ def add_unsubscription_headers_and_links
+ return unless !@labels_url && @sent_notification && @sent_notification.unsubscribable?
+
+ list_unsubscribe_methods = [unsubscribe_sent_notification_url(@sent_notification, force: true)]
+ if Gitlab::IncomingEmail.enabled? && Gitlab::IncomingEmail.supports_wildcard?
+ list_unsubscribe_methods << "mailto:#{Gitlab::IncomingEmail.unsubscribe_address(reply_key)}"
+ end
+
+ headers['List-Unsubscribe'] = list_unsubscribe_methods.map { |e| "<#{e}>" }.join(',')
+ @sent_notification_url = unsubscribe_sent_notification_url(@sent_notification)
+ end
end
diff --git a/changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email b/changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email
new file mode 100644
index 00000000000..f4011b756a5
--- /dev/null
+++ b/changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email
@@ -0,0 +1,4 @@
+---
+title: Handle unsubscribe from email notifications via replying to reply+%{key}+unsubscribe@ address
+merge_request: 6597
+author:
diff --git a/lib/gitlab/email/handler.rb b/lib/gitlab/email/handler.rb
index bd3267e2a80..bd2f5d3615e 100644
--- a/lib/gitlab/email/handler.rb
+++ b/lib/gitlab/email/handler.rb
@@ -1,10 +1,11 @@
require 'gitlab/email/handler/create_note_handler'
require 'gitlab/email/handler/create_issue_handler'
+require 'gitlab/email/handler/unsubscribe_handler'
module Gitlab
module Email
module Handler
- HANDLERS = [CreateNoteHandler, CreateIssueHandler]
+ HANDLERS = [UnsubscribeHandler, CreateNoteHandler, CreateIssueHandler]
def self.for(mail, mail_key)
HANDLERS.find do |klass|
diff --git a/lib/gitlab/email/handler/base_handler.rb b/lib/gitlab/email/handler/base_handler.rb
index 7cccf465334..3f6ace0311a 100644
--- a/lib/gitlab/email/handler/base_handler.rb
+++ b/lib/gitlab/email/handler/base_handler.rb
@@ -9,52 +9,13 @@ module Gitlab
@mail_key = mail_key
end
- def message
- @message ||= process_message
- end
-
- def author
+ def can_execute?
raise NotImplementedError
end
- def project
+ def execute
raise NotImplementedError
end
-
- private
-
- def validate_permission!(permission)
- raise UserNotFoundError unless author
- raise UserBlockedError if author.blocked?
- raise ProjectNotFound unless author.can?(:read_project, project)
- raise UserNotAuthorizedError unless author.can?(permission, project)
- end
-
- def process_message
- message = ReplyParser.new(mail).execute.strip
- add_attachments(message)
- end
-
- def add_attachments(reply)
- attachments = Email::AttachmentUploader.new(mail).execute(project)
-
- reply + attachments.map do |link|
- "\n\n#{link[:markdown]}"
- end.join
- end
-
- def verify_record!(record:, invalid_exception:, record_name:)
- return if record.persisted?
- return if record.errors.key?(:commands_only)
-
- error_title = "The #{record_name} could not be created for the following reasons:"
-
- msg = error_title + record.errors.full_messages.map do |error|
- "\n\n- #{error}"
- end.join
-
- raise invalid_exception, msg
- end
end
end
end
diff --git a/lib/gitlab/email/handler/create_issue_handler.rb b/lib/gitlab/email/handler/create_issue_handler.rb
index 9f90a3ec2b2..127fae159d5 100644
--- a/lib/gitlab/email/handler/create_issue_handler.rb
+++ b/lib/gitlab/email/handler/create_issue_handler.rb
@@ -5,6 +5,7 @@ module Gitlab
module Email
module Handler
class CreateIssueHandler < BaseHandler
+ include ReplyProcessing
attr_reader :project_path, :incoming_email_token
def initialize(mail, mail_key)
diff --git a/lib/gitlab/email/handler/create_note_handler.rb b/lib/gitlab/email/handler/create_note_handler.rb
index 447c7a6a6b9..d87ba427f4b 100644
--- a/lib/gitlab/email/handler/create_note_handler.rb
+++ b/lib/gitlab/email/handler/create_note_handler.rb
@@ -1,10 +1,13 @@
require 'gitlab/email/handler/base_handler'
+require 'gitlab/email/handler/reply_processing'
module Gitlab
module Email
module Handler
class CreateNoteHandler < BaseHandler
+ include ReplyProcessing
+
def can_handle?
mail_key =~ /\A\w+\z/
end
@@ -24,6 +27,8 @@ module Gitlab
record_name: 'comment')
end
+ private
+
def author
sent_notification.recipient
end
@@ -36,8 +41,6 @@ module Gitlab
@sent_notification ||= SentNotification.for(mail_key)
end
- private
-
def create_note
Notes::CreateService.new(
project,
diff --git a/lib/gitlab/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb
new file mode 100644
index 00000000000..32c5caf93e8
--- /dev/null
+++ b/lib/gitlab/email/handler/reply_processing.rb
@@ -0,0 +1,54 @@
+module Gitlab
+ module Email
+ module Handler
+ module ReplyProcessing
+ private
+
+ def author
+ raise NotImplementedError
+ end
+
+ def project
+ raise NotImplementedError
+ end
+
+ def message
+ @message ||= process_message
+ end
+
+ def process_message
+ message = ReplyParser.new(mail).execute.strip
+ add_attachments(message)
+ end
+
+ def add_attachments(reply)
+ attachments = Email::AttachmentUploader.new(mail).execute(project)
+
+ reply + attachments.map do |link|
+ "\n\n#{link[:markdown]}"
+ end.join
+ end
+
+ def validate_permission!(permission)
+ raise UserNotFoundError unless author
+ raise UserBlockedError if author.blocked?
+ raise ProjectNotFound unless author.can?(:read_project, project)
+ raise UserNotAuthorizedError unless author.can?(permission, project)
+ end
+
+ def verify_record!(record:, invalid_exception:, record_name:)
+ return if record.persisted?
+ return if record.errors.key?(:commands_only)
+
+ error_title = "The #{record_name} could not be created for the following reasons:"
+
+ msg = error_title + record.errors.full_messages.map do |error|
+ "\n\n- #{error}"
+ end.join
+
+ raise invalid_exception, msg
+ end
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/email/handler/unsubscribe_handler.rb b/lib/gitlab/email/handler/unsubscribe_handler.rb
new file mode 100644
index 00000000000..97d7a8d65ff
--- /dev/null
+++ b/lib/gitlab/email/handler/unsubscribe_handler.rb
@@ -0,0 +1,32 @@
+require 'gitlab/email/handler/base_handler'
+
+module Gitlab
+ module Email
+ module Handler
+ class UnsubscribeHandler < BaseHandler
+ def can_handle?
+ mail_key =~ /\A\w+#{Regexp.escape(Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX)}\z/
+ end
+
+ def execute
+ raise SentNotificationNotFoundError unless sent_notification
+ return unless sent_notification.unsubscribable?
+
+ noteable = sent_notification.noteable
+ raise NoteableNotFoundError unless noteable
+ noteable.unsubscribe(sent_notification.recipient)
+ end
+
+ private
+
+ def sent_notification
+ @sent_notification ||= SentNotification.for(reply_key)
+ end
+
+ def reply_key
+ mail_key.sub(Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX, '')
+ end
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/incoming_email.rb b/lib/gitlab/incoming_email.rb
index 801dfde9a36..b91012d6405 100644
--- a/lib/gitlab/incoming_email.rb
+++ b/lib/gitlab/incoming_email.rb
@@ -1,5 +1,6 @@
module Gitlab
module IncomingEmail
+ UNSUBSCRIBE_SUFFIX = '+unsubscribe'.freeze
WILDCARD_PLACEHOLDER = '%{key}'.freeze
class << self
@@ -18,7 +19,11 @@ module Gitlab
end
def reply_address(key)
- config.address.gsub(WILDCARD_PLACEHOLDER, key)
+ config.address.sub(WILDCARD_PLACEHOLDER, key)
+ end
+
+ def unsubscribe_address(key)
+ config.address.sub(WILDCARD_PLACEHOLDER, "#{key}#{UNSUBSCRIBE_SUFFIX}")
end
def key_from_address(address)
@@ -49,7 +54,7 @@ module Gitlab
return nil unless wildcard_address
regex = Regexp.escape(wildcard_address)
- regex = regex.gsub(Regexp.escape('%{key}'), "(.+)")
+ regex = regex.sub(Regexp.escape(WILDCARD_PLACEHOLDER), '(.+)')
Regexp.new(regex).freeze
end
end
diff --git a/spec/lib/gitlab/email/email_shared_blocks.rb b/spec/lib/gitlab/email/email_shared_blocks.rb
index 19298e261e3..9d806fc524d 100644
--- a/spec/lib/gitlab/email/email_shared_blocks.rb
+++ b/spec/lib/gitlab/email/email_shared_blocks.rb
@@ -18,7 +18,7 @@ shared_context :email_shared_context do
end
end
-shared_examples :email_shared_examples do
+shared_examples :reply_processing_shared_examples do
context "when the user could not be found" do
before do
user.destroy
diff --git a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb
index cb3651e3845..08897a4c310 100644
--- a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb
+++ b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb
@@ -3,7 +3,7 @@ require_relative '../email_shared_blocks'
describe Gitlab::Email::Handler::CreateIssueHandler, lib: true do
include_context :email_shared_context
- it_behaves_like :email_shared_examples
+ it_behaves_like :reply_processing_shared_examples
before do
stub_incoming_email_setting(enabled: true, address: "incoming+%{key}@appmail.adventuretime.ooo")
diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
index 48660d1dd1b..cebbeff50cf 100644
--- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
+++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
@@ -3,7 +3,7 @@ require_relative '../email_shared_blocks'
describe Gitlab::Email::Handler::CreateNoteHandler, lib: true do
include_context :email_shared_context
- it_behaves_like :email_shared_examples
+ it_behaves_like :reply_processing_shared_examples
before do
stub_incoming_email_setting(enabled: true, address: "reply+%{key}@appmail.adventuretime.ooo")
diff --git a/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb b/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb
new file mode 100644
index 00000000000..a444257754b
--- /dev/null
+++ b/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb
@@ -0,0 +1,61 @@
+require 'spec_helper'
+require_relative '../email_shared_blocks'
+
+describe Gitlab::Email::Handler::UnsubscribeHandler, lib: true do
+ include_context :email_shared_context
+
+ before do
+ stub_incoming_email_setting(enabled: true, address: 'reply+%{key}@appmail.adventuretime.ooo')
+ stub_config_setting(host: 'localhost')
+ end
+
+ let(:email_raw) { fixture_file('emails/valid_reply.eml').gsub(mail_key, "#{mail_key}+unsubscribe") }
+ let(:project) { create(:project, :public) }
+ let(:user) { create(:user) }
+ let(:noteable) { create(:issue, project: project) }
+
+ let!(:sent_notification) { SentNotification.record(noteable, user.id, mail_key) }
+
+ context 'when notification concerns a commit' do
+ let(:commit) { create(:commit, project: project) }
+ let!(:sent_notification) { SentNotification.record(commit, user.id, mail_key) }
+
+ it 'handler does not raise an error' do
+ expect { receiver.execute }.not_to raise_error
+ end
+ end
+
+ context 'user is unsubscribed' do
+ it 'leaves user unsubscribed' do
+ expect { receiver.execute }.not_to change { noteable.subscribed?(user) }.from(false)
+ end
+ end
+
+ context 'user is subscribed' do
+ before do
+ noteable.subscribe(user)
+ end
+
+ it 'unsubscribes user from notable' do
+ expect { receiver.execute }.to change { noteable.subscribed?(user) }.from(true).to(false)
+ end
+ end
+
+ context 'when the noteable could not be found' do
+ before do
+ noteable.destroy
+ end
+
+ it 'raises a NoteableNotFoundError' do
+ expect { receiver.execute }.to raise_error(Gitlab::Email::NoteableNotFoundError)
+ end
+ end
+
+ context 'when no sent notification for the mail key could be found' do
+ let(:email_raw) { fixture_file('emails/wrong_mail_key.eml') }
+
+ it 'raises a SentNotificationNotFoundError' do
+ expect { receiver.execute }.to raise_error(Gitlab::Email::SentNotificationNotFoundError)
+ end
+ end
+end
diff --git a/spec/lib/gitlab/incoming_email_spec.rb b/spec/lib/gitlab/incoming_email_spec.rb
index 1dcf2c0668b..7e951e3fcdd 100644
--- a/spec/lib/gitlab/incoming_email_spec.rb
+++ b/spec/lib/gitlab/incoming_email_spec.rb
@@ -23,6 +23,48 @@ describe Gitlab::IncomingEmail, lib: true do
end
end
+ describe 'self.supports_wildcard?' do
+ context 'address contains the wildard placeholder' do
+ before do
+ stub_incoming_email_setting(address: 'replies+%{key}@example.com')
+ end
+
+ it 'confirms that wildcard is supported' do
+ expect(described_class.supports_wildcard?).to be_truthy
+ end
+ end
+
+ context "address doesn't contain the wildcard placeholder" do
+ before do
+ stub_incoming_email_setting(address: 'replies@example.com')
+ end
+
+ it 'returns that wildcard is not supported' do
+ expect(described_class.supports_wildcard?).to be_falsey
+ end
+ end
+
+ context 'address is not set' do
+ before do
+ stub_incoming_email_setting(address: nil)
+ end
+
+ it 'returns that wildard is not supported' do
+ expect(described_class.supports_wildcard?).to be_falsey
+ end
+ end
+ end
+
+ context 'self.unsubscribe_address' do
+ before do
+ stub_incoming_email_setting(address: 'replies+%{key}@example.com')
+ end
+
+ it 'returns the address with interpolated reply key and unsubscribe suffix' do
+ expect(described_class.unsubscribe_address('key')).to eq('replies+key+unsubscribe@example.com')
+ end
+ end
+
context "self.reply_address" do
before do
stub_incoming_email_setting(address: "replies+%{key}@example.com")
diff --git a/spec/support/notify_shared_examples.rb b/spec/support/notify_shared_examples.rb
index 49867aa5cc4..a3724b801b3 100644
--- a/spec/support/notify_shared_examples.rb
+++ b/spec/support/notify_shared_examples.rb
@@ -179,9 +179,24 @@ shared_examples 'it should show Gmail Actions View Commit link' do
end
shared_examples 'an unsubscribeable thread' do
+ it_behaves_like 'an unsubscribeable thread with incoming address without %{key}'
+
+ it 'has a List-Unsubscribe header in the correct format' do
+ is_expected.to have_header 'List-Unsubscribe', /unsubscribe/
+ is_expected.to have_header 'List-Unsubscribe', /mailto/
+ is_expected.to have_header 'List-Unsubscribe', /^<.+,.+>$/
+ end
+
+ it { is_expected.to have_body_text /unsubscribe/ }
+end
+
+shared_examples 'an unsubscribeable thread with incoming address without %{key}' do
+ include_context 'reply-by-email is enabled with incoming address without %{key}'
+
it 'has a List-Unsubscribe header in the correct format' do
is_expected.to have_header 'List-Unsubscribe', /unsubscribe/
- is_expected.to have_header 'List-Unsubscribe', /^<.+>$/
+ is_expected.not_to have_header 'List-Unsubscribe', /mailto/
+ is_expected.to have_header 'List-Unsubscribe', /^<[^,]+>$/
end
it { is_expected.to have_body_text /unsubscribe/ }
--
GitLab