Merge branch 'fix_email_images' into 'master'

Fix email images

## Dependencies:
This MR Depends on gitlab-org/html-pipeline-gitlab!4 being **merged and published** to rubygems.org

## What does this MR do?
This MR fixes the broken images in emails that occured scince we introduced access control for all attachments in 7.8.
The access control lead to broken images, because the user usually isn't logged into gitlab when opening the email in his mail client.
The solution to fix this, is to replace all images that were uploaded to gitlab as attachemnts with inline images in emails.

I only added one test for images in notes, because all notes share the same view. If it works fine for a note on a commit, then it'll also work the same for notes on a MR or on issues.

@DouweM can you review this please?

Closes #1161

See merge request !373

CHANGELOG

@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 7.9.0 (unreleased)
+  - Fix broken email images (Hannes Rosenögger)
   - Fix mass SQL statements on initial push (Hannes Rosenögger)
   - Add tag push notifications and normalize HipChat and Slack messages to be consistent (Stan Hu)
   - Add comment notification events to HipChat and Slack services (Stan Hu)

app/helpers/emails_helper.rb

@@ -1,3 +1,6 @@
+require 'html/pipeline'
+require 'html/pipeline/gitlab'
+
 module EmailsHelper
 
   # Google Actions
@@ -39,4 +42,26 @@ module EmailsHelper
     lexer = Rugments::Lexers::Diff.new
     raw formatter.format(lexer.lex(diffcontent))
   end
+
+  def replace_image_links_with_base64(text, project)
+    # Used pipelines in GitLab:
+    # GitlabEmailImageFilter - replaces images that have been uploaded as attachments with inline images in emails.
+    #
+    # see https://gitlab.com/gitlab-org/html-pipeline-gitlab for more filters
+    filters = [
+      HTML::Pipeline::Gitlab::GitlabEmailImageFilter
+    ]
+
+    context = {
+      base_url: File.join(Gitlab.config.gitlab.url, project.path_with_namespace, 'uploads'),
+      upload_path: File.join(Rails.root, 'public', 'uploads', project.path_with_namespace),
+    }
+
+    pipeline = HTML::Pipeline::Gitlab.new(filters).pipeline
+
+    result = pipeline.call(text, context)
+    text = result[:output].to_html(save_with: 0)
+
+    text.html_safe
+  end
 end

app/views/notify/_note_message.html.haml

@@ -1,2 +1,2 @@
 %div
-  = markdown(@note.note)
+  = replace_image_links_with_base64(markdown(@note.note), @note.project)

app/views/notify/new_issue_email.html.haml

@@ -1,5 +1,5 @@
 -if @issue.description
-  = markdown(@issue.description)
+  = replace_image_links_with_base64(markdown(@issue.description), @issue.project)
 
 - if @issue.assignee_id.present?
   %p

app/views/notify/new_merge_request_email.html.haml

@@ -6,4 +6,4 @@
     Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
 
 -if @merge_request.description
-  = markdown(@merge_request.description)
+  = replace_image_links_with_base64(markdown(@merge_request.description), @merge_request.project)

spec/mailers/notify_spec.rb

@@ -183,6 +183,13 @@ describe Notify do
       context 'for issues' do
         let(:issue) { create(:issue, author: current_user, assignee: assignee, project: project) }
         let(:issue_with_description) { create(:issue, author: current_user, assignee: assignee, project: project, description: Faker::Lorem.sentence) }
+        let(:issue_with_image) do
+          create(:issue,
+                 author: current_user,
+                 assignee: assignee,
+                 project: project,
+                 description: "![test](#{Gitlab.config.gitlab.url}/#{project.path_with_namespace}/uploads/12345/test.jpg)")
+        end
 
         describe 'that are new' do
           subject { Notify.new_issue_email(issue.assignee_id, issue.id) }
@@ -207,6 +214,22 @@ describe Notify do
           end
         end
 
+        describe 'that contain images' do
+          let(:png) { File.read("#{Rails.root}/spec/fixtures/dk.png") }
+          let(:png_encoded) { Base64::encode64(png) }
+
+          before :each do
+            file_path = File.join(Rails.root, 'public', 'uploads', issue_with_image.project.path_with_namespace, '12345/test.jpg')
+            allow(File).to receive(:file?).with(file_path).and_return(true)
+            allow(File).to receive(:read).with(file_path).and_return(png)
+          end
+
+          subject { Notify.new_issue_email(issue_with_image.assignee_id, issue_with_image.id) }
+          it 'replaces attached images with inline images' do
+            is_expected.to have_body_text URI.encode(png_encoded)
+          end
+        end
+
         describe 'that have been reassigned' do
           subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id, current_user) }
 
@@ -271,6 +294,14 @@ describe Notify do
         let(:merge_author) { create(:user) }
         let(:merge_request) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project) }
         let(:merge_request_with_description) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project, description: Faker::Lorem.sentence) }
+        let(:merge_request_with_image) do
+           create(:merge_request,
+                  author: current_user,
+                  assignee: assignee,
+                  source_project: project,
+                  target_project: project,
+                  description: "![test](#{Gitlab.config.gitlab.url}/#{project.path_with_namespace}/uploads/12345/test.jpg)")
+         end
 
         describe 'that are new' do
           subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) }
@@ -307,6 +338,22 @@ describe Notify do
           end
         end
 
+        describe 'that are new and contain contain images in the description' do
+          let(:png) {File.read("#{Rails.root}/spec/fixtures/dk.png")}
+          let(:png_encoded) { Base64::encode64(png) }
+
+          before :each do
+            file_path = File.join(Rails.root, 'public', 'uploads', merge_request_with_image.project.path_with_namespace, '/12345/test.jpg')
+            allow(File).to receive(:file?).with(file_path).and_return(true)
+            allow(File).to receive(:read).with(file_path).and_return(png)
+          end
+
+          subject { Notify.new_merge_request_email(merge_request_with_image.assignee_id, merge_request_with_image.id) }
+          it 'replaces attached images with inline images' do
+            is_expected.to have_body_text URI.encode(png_encoded)
+          end
+        end
+
         describe 'that are reassigned' do
           subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id) }
 
@@ -415,9 +462,12 @@ describe Notify do
     describe 'project access changed' do
       let(:project) { create(:project) }
       let(:user) { create(:user) }
-      let(:project_member) { create(:project_member,
-                                   project: project,
-                                   user: user) }
+      let(:project_member) do
+        create(:project_member,
+               project: project,
+               user: user)
+      end
+
       subject { Notify.project_access_granted_email(project_member.id) }
 
       it_behaves_like 'an email sent from GitLab'
@@ -457,6 +507,32 @@ describe Notify do
         end
       end
 
+      describe 'on a commit that contains an image' do
+        let(:commit) { project.repository.commit }
+        let(:note_with_image) do
+          create(:note,
+                 project: project,
+                 author: note_author,
+                 note: "![test](#{Gitlab.config.gitlab.url}/#{project.path_with_namespace}/uploads/12345/test.jpg)")
+        end
+
+        let(:png) {File.read("#{Rails.root}/spec/fixtures/dk.png")}
+        let(:png_encoded) { Base64::encode64(png) }
+
+        before :each do
+          file_path = File.join(Rails.root, 'public', 'uploads', note_with_image.project.path_with_namespace, '12345/test.jpg')
+          allow(File).to receive(:file?).with(file_path).and_return(true)
+          allow(File).to receive(:read).with(file_path).and_return(png)
+          allow(Note).to receive(:find).with(note_with_image.id).and_return(note_with_image)
+          allow(note_with_image).to receive(:noteable).and_return(commit)
+        end
+
+        subject { Notify.note_commit_email(recipient.id, note_with_image.id) }
+        it 'replaces attached images with inline images' do
+          is_expected.to have_body_text URI.encode(png_encoded)
+        end
+      end
+
       describe 'on a commit' do
         let(:commit) { project.repository.commit }