add the uploader context to the upload model

12 files changed, 85 insertions, 30 deletions

diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb
index 61554029d09..7ad79a1e56c 100644
--- a/app/controllers/concerns/uploads_actions.rb
+++ b/app/controllers/concerns/uploads_actions.rb
@@ -70,7 +70,7 @@ module UploadsActions
   end
 
   def build_uploader_from_params
-    uploader = uploader_class.new(model, params[:secret])
+    uploader = uploader_class.new(model, secret: params[:secret])
     uploader.retrieve_from_store!(params[:filename])
     uploader
   end
diff --git a/app/models/upload.rb b/app/models/upload.rb
index fb55fd8007b..28baee95091 100644
--- a/app/models/upload.rb
+++ b/app/models/upload.rb
@@ -30,7 +30,8 @@ class Upload < ActiveRecord::Base
   end
 
   def build_uploader
-    uploader_class.new(model).tap do |uploader|
+    uploader_class.new(model, mount_point, **uploader_context)
+      .tap do |uploader|
       uploader.upload = self
       uploader.retrieve_from_store!(identifier)
     end
@@ -40,6 +41,13 @@ class Upload < ActiveRecord::Base
     File.exist?(absolute_path)
   end
 
+  def uploader_context
+    {
+      identifier: identifier,
+      secret: secret
+    }.compact
+  end
+
   private
 
   def checksummable?
@@ -62,11 +70,15 @@ class Upload < ActiveRecord::Base
     !path.start_with?('/')
   end
 
+  def uploader_class
+    Object.const_get(uploader)
+  end
+
   def identifier
     File.basename(path)
   end
 
-  def uploader_class
-    Object.const_get(uploader)
+  def mount_point
+    super&.to_sym
   end
 end
diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb
index e7af1483d23..8f56f09c9f7 100644
--- a/app/uploaders/file_mover.rb
+++ b/app/uploaders/file_mover.rb
@@ -49,11 +49,11 @@ class FileMover
   end
 
   def uploader
-    @uploader ||= PersonalFileUploader.new(model, secret)
+    @uploader ||= PersonalFileUploader.new(model, secret: secret)
   end
 
   def temp_file_uploader
-    @temp_file_uploader ||= PersonalFileUploader.new(nil, secret)
+    @temp_file_uploader ||= PersonalFileUploader.new(nil, secret: secret)
   end
 
   def revert
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index 85ae9863b13..cc4e7a38165 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -62,9 +62,11 @@ class FileUploader < GitlabUploader
 
   attr_accessor :model
 
-  def initialize(model, secret = nil)
+  def initialize(model, mounted_as = nil, **uploader_context)
+    super(model, nil, **uploader_context)
+
     @model = model
-    @secret = secret
+    apply_context!(uploader_context)
   end
 
   def base_dir
@@ -107,12 +109,12 @@ class FileUploader < GitlabUploader
     self.file.filename
   end
 
-  # the upload does not hold the secret, but holds the path
-  # which contains the secret: extract it
   def upload=(value)
-    if matches = DYNAMIC_PATH_PATTERN.match(value.path)
-      @secret = matches[:secret]
-      @identifier = matches[:identifier]
+    unless apply_context!(value.uploader_context)
+      if matches = DYNAMIC_PATH_PATTERN.match(value.path)
+        @secret = matches[:secret]
+        @identifier = matches[:identifier]
+      end
     end
 
     super
@@ -124,6 +126,18 @@ class FileUploader < GitlabUploader
 
   private
 
+  def apply_context!(uploader_context)
+    @secret, @identifier = uploader_context.values_at(:secret, :identifier)
+
+    !!(@secret && @identifier)
+  end
+
+  def build_upload
+    super.tap do |upload|
+      upload.secret = secret
+    end
+  end
+
   def markdown_name
     (image_or_video? ? File.basename(filename, File.extname(filename)) : filename).gsub("]", "\\]")
   end
diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb
index b12829efe73..a9e5c028b03 100644
--- a/app/uploaders/gitlab_uploader.rb
+++ b/app/uploaders/gitlab_uploader.rb
@@ -29,6 +29,10 @@ class GitlabUploader < CarrierWave::Uploader::Base
 
   delegate :base_dir, :file_storage?, to: :class
 
+  def initialize(model, mounted_as = nil, **uploader_context)
+    super(model, mounted_as)
+  end
+
   def file_cache_storage?
     cache_storage.is_a?(CarrierWave::Storage::File)
   end
diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb
index dfb8dccec57..458928bc067 100644
--- a/app/uploaders/records_uploads.rb
+++ b/app/uploaders/records_uploads.rb
@@ -24,7 +24,7 @@ module RecordsUploads
         uploads.where(path: upload_path).delete_all
         upload.destroy! if upload
 
-        self.upload = build_upload_from_uploader(self)
+        self.upload = build_upload
         upload.save!
       end
     end
@@ -39,12 +39,13 @@ module RecordsUploads
       Upload.order(id: :desc).where(uploader: self.class.to_s)
     end
 
-    def build_upload_from_uploader(uploader)
+    def build_upload
       Upload.new(
-        size: uploader.file.size,
-        path: uploader.upload_path,
-        model: uploader.model,
-        uploader: uploader.class.to_s
+        uploader: self.class.to_s,
+        size: file.size,
+        path: upload_path,
+        model: model,
+        mount_point: mounted_as
       )
     end
 
diff --git a/db/migrate/20180129193323_add_uploads_builder_context.rb b/db/migrate/20180129193323_add_uploads_builder_context.rb
new file mode 100644
index 00000000000..b3909a770ca
--- /dev/null
+++ b/db/migrate/20180129193323_add_uploads_builder_context.rb
@@ -0,0 +1,14 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddUploadsBuilderContext < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def change
+    add_column :uploads, :mount_point, :string
+    add_column :uploads, :secret, :string
+  end
+end
diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb
index 3fdc3c27f73..1b74f735679 100644
--- a/lib/gitlab/gfm/uploads_rewriter.rb
+++ b/lib/gitlab/gfm/uploads_rewriter.rb
@@ -46,7 +46,7 @@ module Gitlab
       private
 
       def find_file(project, secret, file)
-        uploader = FileUploader.new(project, secret)
+        uploader = FileUploader.new(project, secret: secret)
         uploader.retrieve_from_store!(file)
         uploader.file
       end
diff --git a/spec/factories/uploads.rb b/spec/factories/uploads.rb
index c8cfe251d27..ff3a2a76acc 100644
--- a/spec/factories/uploads.rb
+++ b/spec/factories/uploads.rb
@@ -3,36 +3,40 @@ FactoryBot.define do
     model { build(:project) }
     size 100.kilobytes
     uploader "AvatarUploader"
+    mount_point :avatar
+    secret nil
 
     # we should build a mount agnostic upload by default
     transient do
-      mounted_as :avatar
-      secret SecureRandom.hex
+      filename 'myfile.jpg'
     end
 
     # this needs to comply with RecordsUpload::Concern#upload_path
-    path { File.join("uploads/-/system", model.class.to_s.underscore, mounted_as.to_s, 'avatar.jpg') }
+    path { File.join("uploads/-/system", model.class.to_s.underscore, mount_point.to_s, 'avatar.jpg') }
 
     trait :personal_snippet_upload do
-      model { build(:personal_snippet) }
-      path { File.join(secret, 'myfile.jpg') }
       uploader "PersonalFileUploader"
+      path { File.join(secret, filename) }
+      model { build(:personal_snippet) }
+      secret SecureRandom.hex
     end
 
     trait :issuable_upload do
-      path { File.join(secret, 'myfile.jpg') }
       uploader "FileUploader"
+      path { File.join(secret, filename) }
+      secret SecureRandom.hex
     end
 
     trait :namespace_upload do
       model { build(:group) }
-      path { File.join(secret, 'myfile.jpg') }
+      path { File.join(secret, filename) }
       uploader "NamespaceFileUploader"
+      secret SecureRandom.hex
     end
 
     trait :attachment_upload do
       transient do
-        mounted_as :attachment
+        mount_point :attachment
       end
 
       model { build(:note) }
diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb
index 42f3d609770..0dcaa026332 100644
--- a/spec/models/upload_spec.rb
+++ b/spec/models/upload_spec.rb
@@ -103,4 +103,10 @@ describe Upload do
       expect(upload).not_to exist
     end
   end
+
+  describe "#uploader_context" do
+    subject { create(:upload, :issuable_upload, secret: 'secret', filename: 'file.txt') }
+
+    it { expect(subject.uploader_context).to match(a_hash_including(secret: 'secret', identifier: 'file.txt')) }
+  end
 end
diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb
index a72f853df75..92f7467d247 100644
--- a/spec/uploaders/file_uploader_spec.rb
+++ b/spec/uploaders/file_uploader_spec.rb
@@ -40,7 +40,7 @@ describe FileUploader do
   end
 
   describe 'initialize' do
-    let(:uploader) { described_class.new(double, 'secret') }
+    let(:uploader) { described_class.new(double, secret: 'secret') }
 
     it 'accepts a secret parameter' do
       expect(described_class).not_to receive(:generate_secret)
diff --git a/spec/uploaders/gitlab_uploader_spec.rb b/spec/uploaders/gitlab_uploader_spec.rb
index a144b39f74f..60e35dcf235 100644
--- a/spec/uploaders/gitlab_uploader_spec.rb
+++ b/spec/uploaders/gitlab_uploader_spec.rb
@@ -4,7 +4,7 @@ require 'carrierwave/storage/fog'
 describe GitlabUploader do
   let(:uploader_class) { Class.new(described_class) }
 
-  subject { uploader_class.new }
+  subject { uploader_class.new(double) }
 
   describe '#file_storage?' do
     context 'when file storage is used' do