Merge branch '22619-add-an-email-address-to-unsubscribe-list-header-in-email' into 'master'

Handle unsubscribe notification via email

Closes #22619

See merge request !6597

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

changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email

+---
+title: Handle unsubscribe from email notifications via replying to reply+%{key}+unsubscribe@ address
+merge_request: 6597
+author:

lib/gitlab/email/handler.rb

 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|

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

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)

lib/gitlab/email/handler/create_note_handler.rb

 
 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,

lib/gitlab/email/handler/reply_processing.rb

+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

lib/gitlab/email/handler/unsubscribe_handler.rb

+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

lib/gitlab/incoming_email.rb

 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

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

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")

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")

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

+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

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")

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/ }