Merge branch 'uploads_path_fix' into 'master'

Fix: Images cannot show when projects' path was changed

Fixes #1443

See merge request !1521

CHANGELOG

@@ -53,6 +53,7 @@ v 8.1.0 (unreleased)
   - Add "New Page" button to Wiki Pages tab (Stan Hu)
   - Only render 404 page from /public
   - Hide passwords from services API (Alex Lossent)
+  - Fix: Images cannot show when projects' path was changed
 
 v 8.0.4
   - Fix Message-ID header to be RFC 2111-compliant to prevent e-mails being dropped (Stan Hu)

app/models/namespace.rb

@@ -118,6 +118,8 @@ class Namespace < ActiveRecord::Base
     gitlab_shell.add_namespace(path_was)
 
     if gitlab_shell.mv_namespace(path_was, path)
+      Gitlab::UploadsTransfer.new.rename_namespace(path_was, path)
+
       # If repositories moved successfully we need to
       # send update instructions to users.
       # However we cannot allow rollback since we moved namespace dir

app/models/project.rb

@@ -656,6 +656,8 @@ class Project < ActiveRecord::Base
       # db changes in order to prevent out of sync between db and fs
       raise Exception.new('repository cannot be renamed')
     end
+
+    Gitlab::UploadsTransfer.new.rename_project(path_was, path, namespace.path)
   end
 
   def hook_attrs

app/services/projects/transfer_service.rb

@@ -27,6 +27,7 @@ module Projects
     def transfer(project, new_namespace)
       Project.transaction do
         old_path = project.path_with_namespace
+        old_namespace = project.namespace
         new_path = File.join(new_namespace.try(:path) || '', project.path)
 
         if Project.where(path: project.path, namespace_id: new_namespace.try(:id)).present?
@@ -51,6 +52,9 @@ module Projects
         # clear project cached events
         project.reset_events_cache
 
+        # Move uploads
+        Gitlab::UploadsTransfer.new.move_project(project.path, old_namespace.path, new_namespace.path)
+
         true
       end
     end

app/uploaders/file_uploader.rb

@@ -26,7 +26,7 @@ class FileUploader < CarrierWave::Uploader::Base
   end
 
   def secure_url
-    File.join(Gitlab.config.gitlab.url, @project.path_with_namespace, "uploads", @secret, file.filename)
+    File.join("/uploads", @secret, file.filename)
   end
 
   def file_storage?

features/steps/admin/projects.rb

@@ -41,6 +41,8 @@ class Spinach::Features::AdminProjects < Spinach::FeatureSteps
   end
 
   step 'I transfer project to group \'Web\'' do
+    allow_any_instance_of(Projects::TransferService).
+      to receive(:move_uploads_to_new_namespace).and_return(true)
     find(:xpath, "//input[@id='new_namespace_id']").set group.id
     click_button 'Transfer'
   end

lib/gitlab/markdown.rb

@@ -48,6 +48,7 @@ module Gitlab
     autoload :TableOfContentsFilter,        'gitlab/markdown/table_of_contents_filter'
     autoload :TaskListFilter,               'gitlab/markdown/task_list_filter'
     autoload :UserReferenceFilter,          'gitlab/markdown/user_reference_filter'
+    autoload :UploadLinkFilter,             'gitlab/markdown/upload_link_filter'
 
     # Public: Parse the provided text with GitLab-Flavored Markdown
     #
@@ -140,6 +141,7 @@ module Gitlab
         Gitlab::Markdown::SyntaxHighlightFilter,
         Gitlab::Markdown::SanitizationFilter,
 
+        Gitlab::Markdown::UploadLinkFilter,
         Gitlab::Markdown::RelativeLinkFilter,
         Gitlab::Markdown::EmojiFilter,
         Gitlab::Markdown::TableOfContentsFilter,

lib/gitlab/markdown/upload_link_filter.rb

@@ -0,0 +1,47 @@
+require 'gitlab/markdown'
+require 'html/pipeline/filter'
+require 'uri'
+
+module Gitlab
+  module Markdown
+    # HTML filter that "fixes" relative upload links to files.
+    # Context options:
+    #   :project (required) - Current project
+    #
+    class UploadLinkFilter < HTML::Pipeline::Filter
+      def call
+        doc.search('a').each do |el|
+          process_link_attr el.attribute('href')
+        end
+
+        doc.search('img').each do |el|
+          process_link_attr el.attribute('src')
+        end
+
+        doc
+      end
+
+      protected
+
+      def process_link_attr(html_attr)
+        return if html_attr.blank?
+
+        uri = html_attr.value
+        if uri.starts_with?("/uploads/")
+          html_attr.value = build_url(uri).to_s
+        end
+      end
+
+      def build_url(uri)
+        File.join(Gitlab.config.gitlab.url, context[:project].path_with_namespace, uri)
+      end
+
+      # Ensure that a :project key exists in context
+      #
+      # Note that while the key might exist, its value could be nil!
+      def validate
+        needs :project
+      end
+    end
+  end
+end

lib/gitlab/uploads_transfer.rb

@@ -0,0 +1,35 @@
+module Gitlab
+  class UploadsTransfer
+    def move_project(project_path, namespace_path_was, namespace_path)
+      new_namespace_folder = File.join(root_dir, namespace_path)
+      FileUtils.mkdir_p(new_namespace_folder) unless Dir.exist?(new_namespace_folder)
+      from = File.join(root_dir, namespace_path_was, project_path)
+      to = File.join(root_dir, namespace_path, project_path)
+      move(from, to, "")
+    end
+
+    def rename_project(path_was, path, namespace_path)
+      base_dir = File.join(root_dir, namespace_path)
+      move(path_was, path, base_dir)
+    end
+
+    def rename_namespace(path_was, path)
+      move(path_was, path)
+    end
+
+    private
+
+    def move(path_was, path, base_dir = nil)
+      base_dir = root_dir unless base_dir
+      from = File.join(base_dir, path_was)
+      to = File.join(base_dir, path)
+      FileUtils.mv(from, to)
+    rescue Errno::ENOENT
+      false
+    end
+
+    def root_dir
+      File.join(Rails.root, "public", "uploads")
+    end
+  end
+end

spec/controllers/projects/uploads_controller_spec.rb

@@ -33,7 +33,7 @@ describe Projects::UploadsController do
 
       it 'returns a content with original filename, new link, and correct type.' do
         expect(response.body).to match '\"alt\":\"rails_sample\"'
-        expect(response.body).to match "\"url\":\"http://localhost/#{project.path_with_namespace}/uploads"
+        expect(response.body).to match "\"url\":\"/uploads"
         expect(response.body).to match '\"is_image\":true'
       end
     end
@@ -49,7 +49,7 @@ describe Projects::UploadsController do
 
       it 'returns a content with original filename, new link, and correct type.' do
         expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
-        expect(response.body).to match "\"url\":\"http://localhost/#{project.path_with_namespace}/uploads"
+        expect(response.body).to match "\"url\":\"/uploads"
         expect(response.body).to match '\"is_image\":false'
       end
     end

spec/lib/gitlab/email/attachment_uploader_spec.rb

@@ -13,7 +13,6 @@ describe Gitlab::Email::AttachmentUploader do
       expect(link).not_to be_nil
       expect(link[:is_image]).to be_truthy
       expect(link[:alt]).to eq("bricks")
-      expect(link[:url]).to include("/#{project.path_with_namespace}")
       expect(link[:url]).to include("bricks.png")
     end
   end

spec/lib/gitlab/markdown/upload_link_filter_spec.rb

@@ -0,0 +1,75 @@
+# encoding: UTF-8
+
+require 'spec_helper'
+
+module Gitlab::Markdown
+  describe UploadLinkFilter do
+    def filter(doc, contexts = {})
+      contexts.reverse_merge!({
+        project: project
+      })
+
+      described_class.call(doc, contexts)
+    end
+
+    def image(path)
+      %(<img src="#{path}" />)
+    end
+
+    def link(path)
+      %(<a href="#{path}">#{path}</a>)
+    end
+
+    let(:project) { create(:project) }
+
+    shared_examples :preserve_unchanged do
+      it 'does not modify any relative URL in anchor' do
+        doc = filter(link('README.md'))
+        expect(doc.at_css('a')['href']).to eq 'README.md'
+      end
+
+      it 'does not modify any relative URL in image' do
+        doc = filter(image('files/images/logo-black.png'))
+        expect(doc.at_css('img')['src']).to eq 'files/images/logo-black.png'
+      end
+    end
+
+    it 'does not raise an exception on invalid URIs' do
+      act = link("://foo")
+      expect { filter(act) }.not_to raise_error
+    end
+
+    context 'with a valid repository' do
+      it 'rebuilds relative URL for a link' do
+        doc = filter(link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
+        expect(doc.at_css('a')['href']).
+          to eq "#{Gitlab.config.gitlab.url}/#{project.path_with_namespace}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
+      end
+
+      it 'rebuilds relative URL for an image' do
+        doc = filter(link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
+        expect(doc.at_css('a')['href']).
+          to eq "#{Gitlab.config.gitlab.url}/#{project.path_with_namespace}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
+      end
+
+      it 'does not modify absolute URL' do
+        doc = filter(link('http://example.com'))
+        expect(doc.at_css('a')['href']).to eq 'http://example.com'
+      end
+
+      it 'supports Unicode filenames' do
+        path = '/uploads/한글.png'
+        escaped = Addressable::URI.escape(path)
+
+        # Stub these methods so the file doesn't actually need to be in the repo
+        allow_any_instance_of(described_class).
+          to receive(:file_exists?).and_return(true)
+        allow_any_instance_of(described_class).
+          to receive(:image?).with(path).and_return(true)
+
+        doc = filter(image(escaped))
+        expect(doc.at_css('img')['src']).to match "#{Gitlab.config.gitlab.url}/#{project.path_with_namespace}/uploads/%ED%95%9C%EA%B8%80.png"
+      end
+    end
+  end
+end

spec/lib/gitlab/uploads_transfer_spec.rb

@@ -0,0 +1,50 @@
+require 'spec_helper'
+
+describe Gitlab::UploadsTransfer do
+  before do
+    @root_dir = File.join(Rails.root, "public", "uploads")
+    @upload_transfer = Gitlab::UploadsTransfer.new
+
+    @project_path_was = "test_project_was"
+    @project_path = "test_project"
+    @namespace_path_was = "test_namespace_was"
+    @namespace_path = "test_namespace"
+  end
+
+  after do
+    FileUtils.rm_rf([
+      File.join(@root_dir, @namespace_path),
+      File.join(@root_dir, @namespace_path_was)
+    ])
+  end
+
+  describe '#move_project' do
+    it "moves project upload to another namespace" do
+      FileUtils.mkdir_p(File.join(@root_dir, @namespace_path_was, @project_path))
+      @upload_transfer.move_project(@project_path, @namespace_path_was, @namespace_path)
+
+      expected_path = File.join(@root_dir, @namespace_path, @project_path)
+      expect(Dir.exist?(expected_path)).to be_truthy
+    end
+  end
+
+  describe '#rename_project' do
+    it "renames project" do
+      FileUtils.mkdir_p(File.join(@root_dir, @namespace_path, @project_path_was))
+      @upload_transfer.rename_project(@project_path_was, @project_path, @namespace_path)
+
+      expected_path = File.join(@root_dir, @namespace_path, @project_path)
+      expect(Dir.exist?(expected_path)).to be_truthy
+    end
+  end
+
+  describe '#rename_namespace' do
+    it "renames namespace" do
+      FileUtils.mkdir_p(File.join(@root_dir, @namespace_path_was, @project_path))
+      @upload_transfer.rename_namespace(@namespace_path_was, @namespace_path)
+
+      expected_path = File.join(@root_dir, @namespace_path, @project_path)
+      expect(Dir.exist?(expected_path)).to be_truthy
+    end
+  end
+end

spec/services/projects/download_service_spec.rb

@@ -37,7 +37,6 @@ describe Projects::DownloadService do
         it { expect(@link_to_file).to have_key('url') }
         it { expect(@link_to_file).to have_key('is_image') }
         it { expect(@link_to_file['is_image']).to be true }
-        it { expect(@link_to_file['url']).to match("/#{@project.path_with_namespace}") }
         it { expect(@link_to_file['url']).to match('rails_sample.jpg') }
         it { expect(@link_to_file['alt']).to eq('rails_sample') }
       end
@@ -52,7 +51,6 @@ describe Projects::DownloadService do
         it { expect(@link_to_file).to have_key('url') }
         it { expect(@link_to_file).to have_key('is_image') }
         it { expect(@link_to_file['is_image']).to be false }
-        it { expect(@link_to_file['url']).to match("/#{@project.path_with_namespace}") }
         it { expect(@link_to_file['url']).to match('doc_sample.txt') }
         it { expect(@link_to_file['alt']).to eq('doc_sample.txt') }
       end

spec/services/projects/transfer_service_spec.rb

@@ -7,6 +7,8 @@ describe Projects::TransferService do
 
   context 'namespace -> namespace' do
     before do
+      allow_any_instance_of(Gitlab::UploadsTransfer).
+        to receive(:move_project).and_return(true)
       group.add_owner(user)
       @result = transfer_project(project, user, group)
     end

spec/services/projects/upload_service_spec.rb

@@ -18,7 +18,6 @@ describe Projects::UploadService do
       it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file).to have_value('banana_sample') }
       it { expect(@link_to_file[:is_image]).to equal(true) }
-      it { expect(@link_to_file[:url]).to match("/#{@project.path_with_namespace}") }
       it { expect(@link_to_file[:url]).to match('banana_sample.gif') }
     end
 
@@ -34,7 +33,6 @@ describe Projects::UploadService do
       it { expect(@link_to_file).to have_value('dk') }
       it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file[:is_image]).to equal(true) }
-      it { expect(@link_to_file[:url]).to match("/#{@project.path_with_namespace}") }
       it { expect(@link_to_file[:url]).to match('dk.png') }
     end
 
@@ -49,7 +47,6 @@ describe Projects::UploadService do
       it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file).to have_value('rails_sample') }
       it { expect(@link_to_file[:is_image]).to equal(true) }
-      it { expect(@link_to_file[:url]).to match("/#{@project.path_with_namespace}") }
       it { expect(@link_to_file[:url]).to match('rails_sample.jpg') }
     end
 
@@ -64,7 +61,6 @@ describe Projects::UploadService do
       it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file).to have_value('doc_sample.txt') }
       it { expect(@link_to_file[:is_image]).to equal(false) }
-      it { expect(@link_to_file[:url]).to match("/#{@project.path_with_namespace}") }
       it { expect(@link_to_file[:url]).to match('doc_sample.txt') }
     end