Merge branch '33359-pers-snippet-files-location' into 'security-9-3'

app/uploaders/personal_file_uploader.rb

@@ -3,6 +3,10 @@ class PersonalFileUploader < FileUploader
     File.join(CarrierWave.root, model_path(model))
   end
 
+  def self.base_dir
+    File.join(root_dir, 'system')
+  end
+
   private
 
   def secure_url

changelogs/unreleased/33359-pers-snippet-files-location.yml

+---
+title: Use uploads/system directory for personal snippets
+merge_request:
+author:

config/routes/uploads.rb

@@ -5,12 +5,12 @@ scope path: :uploads do
       constraints:  { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /[^\/]+/ }
 
   # show uploads for models, snippets (notes) available for now
-  get ':model/:id/:secret/:filename',
+  get 'system/:model/:id/:secret/:filename',
     to: 'uploads#show',
     constraints: { model: /personal_snippet/, id: /\d+/, filename: /[^\/]+/ }
 
   # show temporary uploads
-  get 'temp/:secret/:filename',
+  get 'system/temp/:secret/:filename',
     to: 'uploads#show',
     constraints: { filename: /[^\/]+/ }
 

db/post_migrate/20170612071012_move_personal_snippets_files.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+class MovePersonalSnippetsFiles < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+
+  def up
+    return unless file_storage?
+
+    @source_relative_location = File.join('/uploads', 'personal_snippet')
+    @destination_relative_location = File.join('/uploads', 'system', 'personal_snippet')
+
+    move_personal_snippet_files
+  end
+
+  def down
+    return unless file_storage?
+
+    @source_relative_location = File.join('/uploads', 'system', 'personal_snippet')
+    @destination_relative_location = File.join('/uploads', 'personal_snippet')
+
+    move_personal_snippet_files
+  end
+
+  def move_personal_snippet_files
+    query = "SELECT uploads.path, uploads.model_id, snippets.description FROM uploads "\
+            "INNER JOIN snippets ON snippets.id = uploads.model_id WHERE uploader = 'PersonalFileUploader'"
+    select_all(query).each do |upload|
+      secret = upload['path'].split('/')[0]
+      file_name = upload['path'].split('/')[1]
+
+      next unless move_file(upload['model_id'], secret, file_name)
+      update_markdown(upload['model_id'], secret, file_name, upload['description'])
+    end
+  end
+
+  def move_file(snippet_id, secret, file_name)
+    source_dir = File.join(base_directory, @source_relative_location, snippet_id.to_s, secret)
+    destination_dir = File.join(base_directory, @destination_relative_location, snippet_id.to_s, secret)
+
+    source_file_path = File.join(source_dir, file_name)
+    destination_file_path = File.join(destination_dir, file_name)
+
+    unless File.exist?(source_file_path)
+      say "Source file `#{source_file_path}` doesn't exist. Skipping."
+      return
+    end
+
+    say "Moving file #{source_file_path} -> #{destination_file_path}"
+
+    FileUtils.mkdir_p(destination_dir)
+    FileUtils.move(source_file_path, destination_file_path)
+
+    true
+  end
+
+  def update_markdown(snippet_id, secret, file_name, description)
+    source_markdown_path = File.join(@source_relative_location, snippet_id.to_s, secret, file_name)
+    destination_markdown_path = File.join(@destination_relative_location, snippet_id.to_s, secret, file_name)
+
+    source_markdown = "](#{source_markdown_path})"
+    destination_markdown = "](#{destination_markdown_path})"
+
+    if description.present?
+      description = description.gsub(source_markdown, destination_markdown)
+      quoted_description = quote_string(description)
+
+      execute("UPDATE snippets SET description = '#{quoted_description}', description_html = NULL "\
+              "WHERE id = #{snippet_id}")
+    end
+
+    query = "SELECT id, note FROM notes WHERE noteable_id = #{snippet_id} "\
+            "AND noteable_type = 'Snippet' AND note IS NOT NULL"
+    select_all(query).each do |note|
+      text = note['note'].gsub(source_markdown, destination_markdown)
+      quoted_text = quote_string(text)
+
+      execute("UPDATE notes SET note = '#{quoted_text}', note_html = NULL WHERE id = #{note['id']}")
+    end
+  end
+
+  def base_directory
+    File.join(Rails.root, 'public')
+  end
+
+  def file_storage?
+    CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File
+  end
+end

spec/controllers/snippets_controller_spec.rb

@@ -186,8 +186,8 @@ describe SnippetsController do
     end
 
     context 'when the snippet description contains a file' do
-      let(:picture_file) { '/temp/secret56/picture.jpg' }
-      let(:text_file) { '/temp/secret78/text.txt' }
+      let(:picture_file) { '/system/temp/secret56/picture.jpg' }
+      let(:text_file) { '/system/temp/secret78/text.txt' }
       let(:description) do
         "Description with picture: ![picture](/uploads#{picture_file}) and "\
         "text: [text.txt](/uploads#{text_file})"
@@ -208,8 +208,8 @@ describe SnippetsController do
         snippet = subject
 
         expected_description = "Description with picture: "\
-          "![picture](/uploads/personal_snippet/#{snippet.id}/secret56/picture.jpg) and "\
-          "text: [text.txt](/uploads/personal_snippet/#{snippet.id}/secret78/text.txt)"
+          "![picture](/uploads/system/personal_snippet/#{snippet.id}/secret56/picture.jpg) and "\
+          "text: [text.txt](/uploads/system/personal_snippet/#{snippet.id}/secret78/text.txt)"
 
         expect(snippet.description).to eq(expected_description)
       end

spec/controllers/uploads_controller_spec.rb

@@ -102,7 +102,7 @@ describe UploadsController do
           subject
 
           expect(response.body).to match '\"alt\":\"rails_sample\"'
-          expect(response.body).to match "\"url\":\"/uploads/temp"
+          expect(response.body).to match "\"url\":\"/uploads/system/temp"
         end
 
         it 'does not create an Upload record' do
@@ -119,7 +119,7 @@ describe UploadsController do
           subject
 
           expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
-          expect(response.body).to match "\"url\":\"/uploads/temp"
+          expect(response.body).to match "\"url\":\"/uploads/system/temp"
         end
 
         it 'does not create an Upload record' do

spec/features/snippets/user_creates_snippet_spec.rb

@@ -41,7 +41,7 @@ feature 'User creates snippet', :js, feature: true do
       expect(page).to have_content('My Snippet')
 
       link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
-      expect(link).to match(%r{/uploads/temp/\h{32}/banana_sample\.gif\z})
+      expect(link).to match(%r{/uploads/system/temp/\h{32}/banana_sample\.gif\z})
 
       visit(link)
       expect(page.status_code).to eq(200)
@@ -59,7 +59,7 @@ feature 'User creates snippet', :js, feature: true do
     wait_for_requests
 
     link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
-    expect(link).to match(%r{/uploads/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
+    expect(link).to match(%r{/uploads/system/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
 
     visit(link)
     expect(page.status_code).to eq(200)
@@ -84,7 +84,7 @@ feature 'User creates snippet', :js, feature: true do
     end
     expect(page).to have_content('Hello World!')
     link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
-    expect(link).to match(%r{/uploads/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
+    expect(link).to match(%r{/uploads/system/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
 
     visit(link)
     expect(page.status_code).to eq(200)

spec/features/snippets/user_edits_snippet_spec.rb

@@ -33,7 +33,7 @@ feature 'User edits snippet', :js, feature: true do
     wait_for_requests
 
     link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
-    expect(link).to match(%r{/uploads/personal_snippet/#{snippet.id}/\h{32}/banana_sample\.gif\z})
+    expect(link).to match(%r{/uploads/system/personal_snippet/#{snippet.id}/\h{32}/banana_sample\.gif\z})
   end
 
   it 'updates the snippet to make it internal' do

spec/migrations/move_personal_snippets_files_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20170612071012_move_personal_snippets_files.rb')
+
+describe MovePersonalSnippetsFiles do
+  let(:migration) { described_class.new }
+  let(:test_dir) { File.join(Rails.root, "tmp", "tests", "move_snippet_files_test") }
+  let(:uploads_dir) { File.join(test_dir, 'uploads') }
+  let(:new_uploads_dir) { File.join(uploads_dir, 'system') }
+
+  before do
+    allow(CarrierWave).to receive(:root).and_return(test_dir)
+    allow(migration).to receive(:base_directory).and_return(test_dir)
+    FileUtils.remove_dir(test_dir) if File.directory?(test_dir)
+    allow(migration).to receive(:say)
+  end
+
+  describe "#up" do
+    let(:snippet) do
+      snippet = create(:personal_snippet)
+      create_upload('picture.jpg', snippet)
+      snippet.update(description: markdown_linking_file('picture.jpg', snippet))
+      snippet
+    end
+
+    let(:snippet_with_missing_file) do
+      snippet = create(:snippet)
+      create_upload('picture.jpg', snippet, create_file: false)
+      snippet.update(description: markdown_linking_file('picture.jpg', snippet))
+      snippet
+    end
+
+    it 'moves the files' do
+      source_path = File.join(uploads_dir, model_file_path('picture.jpg', snippet))
+      destination_path = File.join(new_uploads_dir, model_file_path('picture.jpg', snippet))
+
+      migration.up
+
+      expect(File.exist?(source_path)).to be_falsy
+      expect(File.exist?(destination_path)).to be_truthy
+    end
+
+    describe 'updating the markdown' do
+      it 'includes the new path when the file exists' do
+        secret = "secret#{snippet.id}"
+        file_location = "/uploads/system/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
+
+        migration.up
+
+        expect(snippet.reload.description).to include(file_location)
+      end
+
+      it 'does not update the markdown when the file is missing' do
+        secret = "secret#{snippet_with_missing_file.id}"
+        file_location = "/uploads/personal_snippet/#{snippet_with_missing_file.id}/#{secret}/picture.jpg"
+
+        migration.up
+
+        expect(snippet_with_missing_file.reload.description).to include(file_location)
+      end
+
+      it 'updates the note markdown' do
+        secret = "secret#{snippet.id}"
+        file_location = "/uploads/system/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
+        markdown = markdown_linking_file('picture.jpg', snippet)
+        note = create(:note_on_personal_snippet, noteable: snippet, note: "with #{markdown}")
+
+        migration.up
+
+        expect(note.reload.note).to include(file_location)
+      end
+    end
+  end
+
+  describe "#down" do
+    let(:snippet) do
+      snippet = create(:personal_snippet)
+      create_upload('picture.jpg', snippet, in_new_path: true)
+      snippet.update(description: markdown_linking_file('picture.jpg', snippet, in_new_path: true))
+      snippet
+    end
+
+    let(:snippet_with_missing_file) do
+      snippet = create(:personal_snippet)
+      create_upload('picture.jpg', snippet, create_file: false, in_new_path: true)
+      snippet.update(description: markdown_linking_file('picture.jpg', snippet, in_new_path: true))
+      snippet
+    end
+
+    it 'moves the files' do
+      source_path = File.join(new_uploads_dir, model_file_path('picture.jpg', snippet))
+      destination_path = File.join(uploads_dir, model_file_path('picture.jpg', snippet))
+
+      migration.down
+
+      expect(File.exist?(source_path)).to be_falsey
+      expect(File.exist?(destination_path)).to be_truthy
+    end
+
+    describe 'updating the markdown' do
+      it 'includes the new path when the file exists' do
+        secret = "secret#{snippet.id}"
+        file_location = "/uploads/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
+
+        migration.down
+
+        expect(snippet.reload.description).to include(file_location)
+      end
+
+      it 'keeps the markdown as is when the file is missing' do
+        secret = "secret#{snippet_with_missing_file.id}"
+        file_location = "/uploads/system/personal_snippet/#{snippet_with_missing_file.id}/#{secret}/picture.jpg"
+
+        migration.down
+
+        expect(snippet_with_missing_file.reload.description).to include(file_location)
+      end
+
+      it 'updates the note markdown' do
+        markdown = markdown_linking_file('picture.jpg', snippet, in_new_path: true)
+        secret = "secret#{snippet.id}"
+        file_location = "/uploads/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
+        note = create(:note_on_personal_snippet, noteable: snippet, note: "with #{markdown}")
+
+        migration.down
+
+        expect(note.reload.note).to include(file_location)
+      end
+    end
+  end
+
+  describe '#update_markdown' do
+    it 'escapes sql in the snippet description' do
+      migration.instance_variable_set('@source_relative_location', '/uploads/personal_snippet')
+      migration.instance_variable_set('@destination_relative_location', '/uploads/system/personal_snippet')
+
+      secret = '123456789'
+      filename = 'hello.jpg'
+      snippet = create(:personal_snippet)
+
+      path_before = "/uploads/personal_snippet/#{snippet.id}/#{secret}/#{filename}"
+      path_after = "/uploads/system/personal_snippet/#{snippet.id}/#{secret}/#{filename}"
+      description_before = "Hello world; ![image](#{path_before})'; select * from users;"
+      description_after = "Hello world; ![image](#{path_after})'; select * from users;"
+
+      migration.update_markdown(snippet.id, secret, filename, description_before)
+
+      expect(snippet.reload.description).to eq(description_after)
+    end
+  end
+
+  def create_upload(filename, snippet, create_file: true, in_new_path: false)
+    secret = "secret#{snippet.id}"
+    absolute_path = if in_new_path
+                      File.join(new_uploads_dir, model_file_path(filename, snippet))
+                    else
+                      File.join(uploads_dir, model_file_path(filename, snippet))
+                    end
+
+    if create_file
+      FileUtils.mkdir_p(File.dirname(absolute_path))
+      FileUtils.touch(absolute_path)
+    end
+
+    create(:upload, model: snippet, path: "#{secret}/#{filename}", uploader: PersonalFileUploader)
+  end
+
+  def markdown_linking_file(filename, snippet, in_new_path: false)
+    markdown =  "![#{filename.split('.')[0]}]"
+    markdown += '(/uploads'
+    markdown += '/system' if in_new_path
+    markdown += "/#{model_file_path(filename, snippet)})"
+    markdown
+  end
+
+  def model_file_path(filename, snippet)
+    secret = "secret#{snippet.id}"
+
+    File.join('personal_snippet', snippet.id.to_s, secret, filename)
+  end
+end

spec/uploaders/file_mover_spec.rb

@@ -4,11 +4,11 @@ describe FileMover do
   let(:filename) { 'banana_sample.gif' }
   let(:file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', filename)) }
   let(:temp_description) do
-    'test ![banana_sample](/uploads/temp/secret55/banana_sample.gif) same ![banana_sample]'\
-    '(/uploads/temp/secret55/banana_sample.gif)'
+    'test ![banana_sample](/uploads/system/temp/secret55/banana_sample.gif) same ![banana_sample]'\
+    '(/uploads/system/temp/secret55/banana_sample.gif)'
   end
   let(:temp_file_path) { File.join('secret55', filename).to_s }
-  let(:file_path) { File.join('uploads', 'personal_snippet', snippet.id.to_s, 'secret55', filename).to_s }
+  let(:file_path) { File.join('uploads', 'system', 'personal_snippet', snippet.id.to_s, 'secret55', filename).to_s }
 
   let(:snippet) { create(:personal_snippet, description: temp_description) }
 
@@ -28,8 +28,8 @@ describe FileMover do
 
         expect(snippet.reload.description)
           .to eq(
-            "test ![banana_sample](/uploads/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"\
-            " same ![banana_sample](/uploads/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"
+            "test ![banana_sample](/uploads/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"\
+            " same ![banana_sample](/uploads/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"
           )
       end
 
@@ -50,8 +50,8 @@ describe FileMover do
 
         expect(snippet.reload.description)
           .to eq(
-            "test ![banana_sample](/uploads/temp/secret55/banana_sample.gif)"\
-            " same ![banana_sample](/uploads/temp/secret55/banana_sample.gif)"
+            "test ![banana_sample](/uploads/system/temp/secret55/banana_sample.gif)"\
+            " same ![banana_sample](/uploads/system/temp/secret55/banana_sample.gif)"
           )
       end
 

spec/uploaders/personal_file_uploader_spec.rb

@@ -10,7 +10,7 @@ describe PersonalFileUploader do
 
       dynamic_segment = "personal_snippet/#{snippet.id}"
 
-      expect(described_class.absolute_path(upload)).to end_with("#{dynamic_segment}/secret/foo.jpg")
+      expect(described_class.absolute_path(upload)).to end_with("/system/#{dynamic_segment}/secret/foo.jpg")
     end
   end
 
@@ -19,7 +19,7 @@ describe PersonalFileUploader do
       uploader = described_class.new(snippet, 'secret')
 
       allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name'))
-      expected_url = "/uploads/personal_snippet/#{snippet.id}/secret/file_name"
+      expected_url = "/uploads/system/personal_snippet/#{snippet.id}/secret/file_name"
 
       expect(uploader.to_h).to eq(
         alt: 'file_name',