diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2018-02-02 13:59:43 +0000 |
---|---|---|
committer | Kamil Trzciński <ayufan@ayufan.eu> | 2018-02-28 20:58:15 +0100 |
commit | a7dae52e9d27adde427ef8aa066c0761071a3cd9 (patch) | |
tree | 8b6229e4e0afe7e71f9754089758cee8acd56cde /app | |
parent | 45d2c31643017807cb3fc66c0be6e9cad9964faf (diff) | |
download | gitlab-ce-a7dae52e9d27adde427ef8aa066c0761071a3cd9.tar.gz |
Merge branch '4163-move-uploads-to-object-storage' into 'master'
Move uploads to object storage
Closes #4163
See merge request gitlab-org/gitlab-ee!3867
Diffstat (limited to 'app')
31 files changed, 460 insertions, 550 deletions
diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index a6fb1f40001..61554029d09 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -1,6 +1,8 @@ module UploadsActions include Gitlab::Utils::StrongMemoize + UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo).freeze + def create link_to_file = UploadService.new(model, params[:file], uploader_class).execute @@ -17,34 +19,71 @@ module UploadsActions end end + # This should either + # - send the file directly + # - or redirect to its URL + # def show return render_404 unless uploader.exists? - disposition = uploader.image_or_video? ? 'inline' : 'attachment' - - expires_in 0.seconds, must_revalidate: true, private: true + if uploader.file_storage? + disposition = uploader.image_or_video? ? 'inline' : 'attachment' + expires_in 0.seconds, must_revalidate: true, private: true - send_file uploader.file.path, disposition: disposition + send_file uploader.file.path, disposition: disposition + else + redirect_to uploader.url + end end private + def uploader_class + raise NotImplementedError + end + + def upload_mount + mounted_as = params[:mounted_as] + mounted_as if UPLOAD_MOUNTS.include?(mounted_as) + end + + def uploader_mounted? + upload_model_class < CarrierWave::Mount::Extension && !upload_mount.nil? + end + def uploader strong_memoize(:uploader) do - return if show_model.nil? + if uploader_mounted? + model.public_send(upload_mount) # rubocop:disable GitlabSecurity/PublicSend + else + build_uploader_from_upload || build_uploader_from_params + end + end + end - file_uploader = FileUploader.new(show_model, params[:secret]) - file_uploader.retrieve_from_store!(params[:filename]) + def build_uploader_from_upload + return nil unless params[:secret] && params[:filename] - file_uploader - end + upload_path = uploader_class.upload_path(params[:secret], params[:filename]) + upload = Upload.find_by(uploader: uploader_class.to_s, path: upload_path) + upload&.build_uploader + end + + def build_uploader_from_params + uploader = uploader_class.new(model, params[:secret]) + uploader.retrieve_from_store!(params[:filename]) + uploader end def image_or_video? uploader && uploader.exists? && uploader.image_or_video? end - def uploader_class - FileUploader + def find_model + nil + end + + def model + strong_memoize(:model) { find_model } end end diff --git a/app/controllers/groups/uploads_controller.rb b/app/controllers/groups/uploads_controller.rb index e6bd9806401..f1578f75e88 100644 --- a/app/controllers/groups/uploads_controller.rb +++ b/app/controllers/groups/uploads_controller.rb @@ -7,29 +7,23 @@ class Groups::UploadsController < Groups::ApplicationController private - def show_model - strong_memoize(:show_model) do - group_id = params[:group_id] - - Group.find_by_full_path(group_id) - end + def upload_model_class + Group end - def authorize_upload_file! - render_404 unless can?(current_user, :upload_file, group) + def uploader_class + NamespaceFileUploader end - def uploader - strong_memoize(:uploader) do - file_uploader = uploader_class.new(show_model, params[:secret]) - file_uploader.retrieve_from_store!(params[:filename]) - file_uploader - end - end + def find_model + return @group if @group - def uploader_class - NamespaceFileUploader + group_id = params[:group_id] + + Group.find_by_full_path(group_id) end - alias_method :model, :group + def authorize_upload_file! + render_404 unless can?(current_user, :upload_file, group) + end end diff --git a/app/controllers/projects/lfs_storage_controller.rb b/app/controllers/projects/lfs_storage_controller.rb index 5b0f3d11d9e..88fc373945a 100644 --- a/app/controllers/projects/lfs_storage_controller.rb +++ b/app/controllers/projects/lfs_storage_controller.rb @@ -61,7 +61,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController def store_file(oid, size, tmp_file) # Define tmp_file_path early because we use it in "ensure" - tmp_file_path = File.join("#{Gitlab.config.lfs.storage_path}/tmp/upload", tmp_file) + tmp_file_path = File.join(LfsObjectUploader.workhorse_upload_path, tmp_file) object = LfsObject.find_or_create_by(oid: oid, size: size) file_exists = object.file.exists? || move_tmp_file_to_storage(object, tmp_file_path) diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index 4685bbe80b4..f5cf089ad98 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -1,6 +1,7 @@ class Projects::UploadsController < Projects::ApplicationController include UploadsActions + # These will kick you out if you don't have access. skip_before_action :project, :repository, if: -> { action_name == 'show' && image_or_video? } @@ -8,14 +9,20 @@ class Projects::UploadsController < Projects::ApplicationController private - def show_model - strong_memoize(:show_model) do - namespace = params[:namespace_id] - id = params[:project_id] + def upload_model_class + Project + end - Project.find_by_full_path("#{namespace}/#{id}") - end + def uploader_class + FileUploader end - alias_method :model, :project + def find_model + return @project if @project + + namespace = params[:namespace_id] + id = params[:project_id] + + Project.find_by_full_path("#{namespace}/#{id}") + end end diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 16a74f82d3f..3d227b0a955 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -1,19 +1,34 @@ class UploadsController < ApplicationController include UploadsActions + UnknownUploadModelError = Class.new(StandardError) + + MODEL_CLASSES = { + "user" => User, + "project" => Project, + "note" => Note, + "group" => Group, + "appearance" => Appearance, + "personal_snippet" => PersonalSnippet, + nil => PersonalSnippet + }.freeze + + rescue_from UnknownUploadModelError, with: :render_404 + skip_before_action :authenticate_user! + before_action :upload_mount_satisfied? before_action :find_model before_action :authorize_access!, only: [:show] before_action :authorize_create_access!, only: [:create] - private + def uploader_class + PersonalFileUploader + end def find_model return nil unless params[:id] - return render_404 unless upload_model && upload_mount - - @model = upload_model.find(params[:id]) + upload_model_class.find(params[:id]) end def authorize_access! @@ -53,55 +68,17 @@ class UploadsController < ApplicationController end end - def upload_model - upload_models = { - "user" => User, - "project" => Project, - "note" => Note, - "group" => Group, - "appearance" => Appearance, - "personal_snippet" => PersonalSnippet - } - - upload_models[params[:model]] - end - - def upload_mount - return true unless params[:mounted_as] - - upload_mounts = %w(avatar attachment file logo header_logo) - - if upload_mounts.include?(params[:mounted_as]) - params[:mounted_as] - end + def upload_model_class + MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError) end - def uploader - return @uploader if defined?(@uploader) - - case model - when nil - @uploader = PersonalFileUploader.new(nil, params[:secret]) - - @uploader.retrieve_from_store!(params[:filename]) - when PersonalSnippet - @uploader = PersonalFileUploader.new(model, params[:secret]) - - @uploader.retrieve_from_store!(params[:filename]) - else - @uploader = @model.public_send(upload_mount) # rubocop:disable GitlabSecurity/PublicSend - - redirect_to @uploader.url unless @uploader.file_storage? - end - - @uploader + def upload_model_class_has_mounts? + upload_model_class < CarrierWave::Mount::Extension end - def uploader_class - PersonalFileUploader - end + def upload_mount_satisfied? + return true unless upload_model_class_has_mounts? - def model - @model ||= find_model + upload_model_class.uploader_options.has_key?(upload_mount) end end diff --git a/app/models/appearance.rb b/app/models/appearance.rb index 76cfe28742a..dcd14c08f3c 100644 --- a/app/models/appearance.rb +++ b/app/models/appearance.rb @@ -11,6 +11,7 @@ class Appearance < ActiveRecord::Base mount_uploader :logo, AttachmentUploader mount_uploader :header_logo, AttachmentUploader + has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent CACHE_KEY = 'current_appearance'.freeze diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index b65daa376d2..4eeccd4d934 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -45,7 +45,7 @@ module Ci end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } - scope :with_artifacts_stored_locally, ->() { with_artifacts.where(artifacts_file_store: [nil, LegacyArtifactUploader::LOCAL_STORE]) } + scope :with_artifacts_stored_locally, ->() { with_artifacts.where(artifacts_file_store: [nil, LegacyArtifactUploader::Store::LOCAL]) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } scope :ref_protected, -> { where(protected: true) } diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb index 10659030910..d35e37935fb 100644 --- a/app/models/concerns/avatarable.rb +++ b/app/models/concerns/avatarable.rb @@ -1,6 +1,30 @@ module Avatarable extend ActiveSupport::Concern + included do + prepend ShadowMethods + + validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } + validates :avatar, file_size: { maximum: 200.kilobytes.to_i } + + mount_uploader :avatar, AvatarUploader + end + + module ShadowMethods + def avatar_url(**args) + # We use avatar_path instead of overriding avatar_url because of carrierwave. + # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864 + + avatar_path(only_path: args.fetch(:only_path, true)) || super + end + end + + def avatar_type + unless self.avatar.image? + self.errors.add :avatar, "only images allowed" + end + end + def avatar_path(only_path: true) return unless self[:avatar].present? diff --git a/app/models/group.rb b/app/models/group.rb index fddace03387..5d1e2f62982 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -29,18 +29,14 @@ class Group < Namespace has_many :variables, class_name: 'Ci::GroupVariable' has_many :custom_attributes, class_name: 'GroupCustomAttribute' - validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } + has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + validate :visibility_level_allowed_by_projects validate :visibility_level_allowed_by_sub_groups validate :visibility_level_allowed_by_parent - validates :avatar, file_size: { maximum: 200.kilobytes.to_i } - validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } - mount_uploader :avatar, AvatarUploader - has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - after_create :post_create_hook after_destroy :post_destroy_hook after_save :update_two_factor_requirement @@ -116,12 +112,6 @@ class Group < Namespace visibility_level_allowed_by_sub_groups?(level) end - def avatar_url(**args) - # We use avatar_path instead of overriding avatar_url because of carrierwave. - # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864 - avatar_path(args) - end - def lfs_enabled? return false unless Gitlab.config.lfs.enabled return Gitlab.config.lfs.enabled if self[:lfs_enabled].nil? @@ -193,12 +183,6 @@ class Group < Namespace owners.include?(user) && owners.size == 1 end - def avatar_type - unless self.avatar.image? - self.errors.add :avatar, "only images allowed" - end - end - def post_create_hook Gitlab::AppLogger.info("Group \"#{name}\" was created") diff --git a/app/models/lfs_object.rb b/app/models/lfs_object.rb index 6ad792aab30..65c157d61ca 100644 --- a/app/models/lfs_object.rb +++ b/app/models/lfs_object.rb @@ -7,7 +7,7 @@ class LfsObject < ActiveRecord::Base validates :oid, presence: true, uniqueness: true - scope :with_files_stored_locally, ->() { where(file_store: [nil, LfsObjectUploader::LOCAL_STORE]) } + scope :with_files_stored_locally, -> { where(file_store: [nil, LfsObjectUploader::Store::LOCAL]) } mount_uploader :file, LfsObjectUploader diff --git a/app/models/note.rb b/app/models/note.rb index 184fbd5f5ae..a84db8982e5 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -88,6 +88,7 @@ class Note < ActiveRecord::Base end end + # @deprecated attachments are handler by the MarkdownUploader mount_uploader :attachment, AttachmentUploader # Scopes diff --git a/app/models/project.rb b/app/models/project.rb index fbe65e700a4..b3c2b599129 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -255,9 +255,6 @@ class Project < ActiveRecord::Base validates :star_count, numericality: { greater_than_or_equal_to: 0 } validate :check_limit, on: :create validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? } - validate :avatar_type, - if: ->(project) { project.avatar.present? && project.avatar_changed? } - validates :avatar, file_size: { maximum: 200.kilobytes.to_i } validate :visibility_level_allowed_by_group validate :visibility_level_allowed_as_fork validate :check_wiki_path_conflict @@ -265,7 +262,6 @@ class Project < ActiveRecord::Base presence: true, inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } } - mount_uploader :avatar, AvatarUploader has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent # Scopes @@ -917,20 +913,12 @@ class Project < ActiveRecord::Base issues_tracker.to_param == 'jira' end - def avatar_type - unless self.avatar.image? - self.errors.add :avatar, 'only images allowed' - end - end - def avatar_in_git repository.avatar end def avatar_url(**args) - # We use avatar_path instead of overriding avatar_url because of carrierwave. - # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864 - avatar_path(args) || (Gitlab::Routing.url_helpers.project_avatar_url(self) if avatar_in_git) + Gitlab::Routing.url_helpers.project_avatar_url(self) if avatar_in_git end # For compatibility with old code diff --git a/app/models/upload.rb b/app/models/upload.rb index f194d7bdb80..e227baea994 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -9,44 +9,52 @@ class Upload < ActiveRecord::Base validates :model, presence: true validates :uploader, presence: true - before_save :calculate_checksum, if: :foreground_checksum? - after_commit :schedule_checksum, unless: :foreground_checksum? + before_save :calculate_checksum!, if: :foreground_checksummable? + after_commit :schedule_checksum, if: :checksummable? - def self.remove_path(path) - where(path: path).destroy_all - end - - def self.record(uploader) - remove_path(uploader.relative_path) - - create( - size: uploader.file.size, - path: uploader.relative_path, - model: uploader.model, - uploader: uploader.class.to_s - ) + def self.hexdigest(path) + Digest::SHA256.file(path).hexdigest end def absolute_path + raise ObjectStorage::RemoteStoreError, "Remote object has no absolute path." unless local? return path unless relative_path? uploader_class.absolute_path(self) end - def calculate_checksum - return unless exist? + def calculate_checksum! + self.checksum = nil + return unless checksummable? self.checksum = Digest::SHA256.file(absolute_path).hexdigest end + def build_uploader + uploader_class.new(model).tap do |uploader| + uploader.upload = self + uploader.retrieve_from_store!(identifier) + end + end + def exist? File.exist?(absolute_path) end private - def foreground_checksum? - size <= CHECKSUM_THRESHOLD + def checksummable? + checksum.nil? && local? && exist? + end + + def local? + return true if store.nil? + + store == ObjectStorage::Store::LOCAL + end + + def foreground_checksummable? + checksummable? && size <= CHECKSUM_THRESHOLD end def schedule_checksum @@ -57,6 +65,10 @@ class Upload < ActiveRecord::Base !path.start_with?('/') end + def identifier + File.basename(path) + end + def uploader_class Object.const_get(uploader) end diff --git a/app/models/user.rb b/app/models/user.rb index 4484ee9ff4c..eb6d12b5ec5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -134,6 +134,7 @@ class User < ActiveRecord::Base has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent has_many :custom_attributes, class_name: 'UserCustomAttribute' + has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent # # Validations @@ -156,12 +157,10 @@ class User < ActiveRecord::Base validate :namespace_uniq, if: :username_changed? validate :namespace_move_dir_allowed, if: :username_changed? - validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } validate :unique_email, if: :email_changed? validate :owns_notification_email, if: :notification_email_changed? validate :owns_public_email, if: :public_email_changed? validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id } - validates :avatar, file_size: { maximum: 200.kilobytes.to_i } before_validation :sanitize_attrs before_validation :set_notification_email, if: :email_changed? @@ -223,9 +222,6 @@ class User < ActiveRecord::Base end end - mount_uploader :avatar, AvatarUploader - has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - # Scopes scope :admins, -> { where(admin: true) } scope :blocked, -> { with_states(:blocked, :ldap_blocked) } @@ -521,12 +517,6 @@ class User < ActiveRecord::Base end end - def avatar_type - unless avatar.image? - errors.add :avatar, "only images allowed" - end - end - def unique_email if !emails.exists?(email: email) && Email.exists?(email: email) errors.add(:email, 'has already been taken') @@ -854,9 +844,7 @@ class User < ActiveRecord::Base end def avatar_url(size: nil, scale: 2, **args) - # We use avatar_path instead of overriding avatar_url because of carrierwave. - # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864 - avatar_path(args) || GravatarService.new.execute(email, size, scale, username: username) + GravatarService.new.execute(email, size, scale, username: username) end def primary_email_verified? diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index f8aaec8a9c0..bc897d891d5 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -14,9 +14,9 @@ module Projects @old_path = project.full_path @new_path = project.disk_path - origin = FileUploader.dynamic_path_segment(project) + origin = FileUploader.absolute_base_dir(project) project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments] - target = FileUploader.dynamic_path_segment(project) + target = FileUploader.absolute_base_dir(project) result = move_folder!(origin, target) project.save! diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb index 109eb2fea0b..cd819dc9bff 100644 --- a/app/uploaders/attachment_uploader.rb +++ b/app/uploaders/attachment_uploader.rb @@ -1,10 +1,12 @@ class AttachmentUploader < GitlabUploader - include RecordsUploads + include RecordsUploads::Concern + include ObjectStorage::Concern + prepend ObjectStorage::Extension::RecordsUploads include UploaderHelper - storage :file + private - def store_dir - "#{base_dir}/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" + def dynamic_segment + File.join(model.class.to_s.underscore, mounted_as.to_s, model.id.to_s) end end diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb index cbb79376d5f..5848e6c6994 100644 --- a/app/uploaders/avatar_uploader.rb +++ b/app/uploaders/avatar_uploader.rb @@ -1,20 +1,13 @@ class AvatarUploader < GitlabUploader - include RecordsUploads include UploaderHelper - - storage :file - - def store_dir - "#{base_dir}/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" - end + include RecordsUploads::Concern + include ObjectStorage::Concern + prepend ObjectStorage::Extension::RecordsUploads def exists? model.avatar.file && model.avatar.file.present? end - # We set move_to_store and move_to_cache to 'false' to prevent stealing - # the avatar file from a project when forking it. - # https://gitlab.com/gitlab-org/gitlab-ce/issues/26158 def move_to_store false end @@ -22,4 +15,10 @@ class AvatarUploader < GitlabUploader def move_to_cache false end + + private + + def dynamic_segment + File.join(model.class.to_s.underscore, mounted_as.to_s, model.id.to_s) + end end diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb index 00c2888d224..f37567d6141 100644 --- a/app/uploaders/file_mover.rb +++ b/app/uploaders/file_mover.rb @@ -21,13 +21,11 @@ class FileMover end def update_markdown - updated_text = model.read_attribute(update_field).gsub(temp_file_uploader.to_markdown, uploader.to_markdown) + updated_text = model.read_attribute(update_field) + .gsub(temp_file_uploader.markdown_link, uploader.markdown_link) model.update_attribute(update_field, updated_text) - - true rescue revert - false end diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 0b591e3bbbb..81952dacce4 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -1,23 +1,40 @@ +# This class breaks the actual CarrierWave concept. +# Every uploader should use a base_dir that is model agnostic so we can build +# back URLs from base_dir-relative paths saved in the `Upload` model. +# +# As the `.base_dir` is model dependent and **not** saved in the upload model (see #upload_path) +# there is no way to build back the correct file path without the model, which defies +# CarrierWave way of storing files. +# class FileUploader < GitlabUploader - include RecordsUploads include UploaderHelper + include RecordsUploads::Concern + include ObjectStorage::Concern + prepend ObjectStorage::Extension::RecordsUploads MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)} + DYNAMIC_PATH_PATTERN = %r{(?<secret>\h{32})/(?<identifier>.*)} - storage :file + attr_accessor :model + + def self.root + File.join(options.storage_path, 'uploads') + end - def self.absolute_path(upload_record) + def self.absolute_path(upload) File.join( - self.dynamic_path_segment(upload_record.model), - upload_record.path + absolute_base_dir(upload.model), + upload.path # already contain the dynamic_segment, see #upload_path ) end - # Not using `GitlabUploader.base_dir` because all project namespaces are in - # the `public/uploads` dir. - # - def self.base_dir - root_dir + def self.base_dir(model) + model_path_segment(model) + end + + # used in migrations and import/exports + def self.absolute_base_dir(model) + File.join(root, base_dir(model)) end # Returns the part of `store_dir` that can change based on the model's current @@ -29,63 +46,94 @@ class FileUploader < GitlabUploader # model - Object that responds to `full_path` and `disk_path` # # Returns a String without a trailing slash - def self.dynamic_path_segment(model) + def self.model_path_segment(model) if model.hashed_storage?(:attachments) - dynamic_path_builder(model.disk_path) + model.disk_path else - dynamic_path_builder(model.full_path) + model.full_path end end - # Auxiliary method to build dynamic path segment when not using a project model - # - # Prefer to use the `.dynamic_path_segment` as it includes Hashed Storage specific logic - def self.dynamic_path_builder(path) - File.join(CarrierWave.root, base_dir, path) + def self.upload_path(secret, identifier) + File.join(secret, identifier) end - attr_accessor :model - attr_reader :secret + def self.generate_secret + SecureRandom.hex + end def initialize(model, secret = nil) @model = model - @secret = secret || generate_secret + @secret = secret end - def store_dir - File.join(dynamic_path_segment, @secret) + def base_dir + self.class.base_dir(@model) end - def relative_path - self.file.path.sub("#{dynamic_path_segment}/", '') + # we don't need to know the actual path, an uploader instance should be + # able to yield the file content on demand, so we should build the digest + def absolute_path + self.class.absolute_path(@upload) end - def to_markdown - to_h[:markdown] + def upload_path + self.class.upload_path(dynamic_segment, identifier) end - def to_h - filename = image_or_video? ? self.file.basename : self.file.filename - escaped_filename = filename.gsub("]", "\\]") + def model_path_segment + self.class.model_path_segment(@model) + end - markdown = "[#{escaped_filename}](#{secure_url})" + def store_dir + File.join(base_dir, dynamic_segment) + end + + def markdown_link + markdown = "[#{markdown_name}](#{secure_url})" markdown.prepend("!") if image_or_video? || dangerous? + markdown + end + def to_h { - alt: filename, + alt: markdown_name, url: secure_url, - markdown: markdown + markdown: markdown_link } end + def filename + 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] + end + + super + end + + def secret + @secret ||= self.class.generate_secret + end + private - def dynamic_path_segment - self.class.dynamic_path_segment(model) + def markdown_name + (image_or_video? ? File.basename(filename, File.extname(filename)) : filename).gsub("]", "\\]") end - def generate_secret - SecureRandom.hex + def identifier + @identifier ||= filename + end + + def dynamic_segment + secret end def secure_url diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index 7f72b3ce471..ba2ceb0c8cf 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -1,64 +1,56 @@ class GitlabUploader < CarrierWave::Uploader::Base - def self.absolute_path(upload_record) - File.join(CarrierWave.root, upload_record.path) - end + class_attribute :options - def self.root_dir - 'uploads' - end + class << self + # DSL setter + def storage_options(options) + self.options = options + end - # When object storage is used, keep the `root_dir` as `base_dir`. - # The files aren't really in folders there, they just have a name. - # The files that contain user input in their name, also contain a hash, so - # the names are still unique - # - # This method is overridden in the `FileUploader` - def self.base_dir - return root_dir unless file_storage? + def root + options.storage_path + end - File.join(root_dir, '-', 'system') - end + # represent the directory namespacing at the class level + def base_dir + options.fetch('base_dir', '') + end - def self.file_storage? - self.storage == CarrierWave::Storage::File + def file_storage? + storage == CarrierWave::Storage::File + end + + def absolute_path(upload_record) + File.join(root, upload_record.path) + end end + storage_options Gitlab.config.uploads + delegate :base_dir, :file_storage?, to: :class def file_cache_storage? cache_storage.is_a?(CarrierWave::Storage::File) end - # Reduce disk IO def move_to_cache - true + file_storage? end - # Reduce disk IO def move_to_store - true - end - - # Designed to be overridden by child uploaders that have a dynamic path - # segment -- that is, a path that changes based on mutable attributes of its - # associated model - # - # For example, `FileUploader` builds the storage path based on the associated - # project model's `path_with_namespace` value, which can change when the - # project or its containing namespace is moved or renamed. - def relative_path - self.file.path.sub("#{root}/", '') + file_storage? end def exists? file.present? end - # Override this if you don't want to save files by default to the Rails.root directory + def cache_dir + File.join(root, base_dir, 'tmp/cache') + end + def work_dir - # Default path set by CarrierWave: - # https://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L182 - CarrierWave.tmp_path + File.join(root, base_dir, 'tmp/work') end def filename @@ -67,6 +59,17 @@ class GitlabUploader < CarrierWave::Uploader::Base private + # Designed to be overridden by child uploaders that have a dynamic path + # segment -- that is, a path that changes based on mutable attributes of its + # associated model + # + # For example, `FileUploader` builds the storage path based on the associated + # project model's `path_with_namespace` value, which can change when the + # project or its containing namespace is moved or renamed. + def dynamic_segment + raise(NotImplementedError) + end + # To prevent files from moving across filesystems, override the default # implementation: # http://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L181-L183 @@ -74,6 +77,6 @@ class GitlabUploader < CarrierWave::Uploader::Base # To be safe, keep this directory outside of the the cache directory # because calling CarrierWave.clean_cache_files! will remove any files in # the cache directory. - File.join(work_dir, @cache_id, version_name.to_s, for_file) + File.join(work_dir, cache_id, version_name.to_s, for_file) end end diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index a0757dbe6b2..3ad3e6ea32b 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -1,13 +1,8 @@ -class JobArtifactUploader < ObjectStoreUploader - storage_options Gitlab.config.artifacts - - def self.local_store_path - Gitlab.config.artifacts.path - end +class JobArtifactUploader < GitlabUploader + extend Workhorse::UploadPath + include ObjectStorage::Concern - def self.artifacts_upload_path - File.join(self.local_store_path, 'tmp/uploads/') - end + storage_options Gitlab.config.artifacts def size return super if model.size.nil? @@ -15,9 +10,13 @@ class JobArtifactUploader < ObjectStoreUploader model.size end + def store_dir + dynamic_segment + end + private - def default_path + def dynamic_segment creation_date = model.created_at.utc.strftime('%Y_%m_%d') File.join(disk_hash[0..1], disk_hash[2..3], disk_hash, diff --git a/app/uploaders/legacy_artifact_uploader.rb b/app/uploaders/legacy_artifact_uploader.rb index 476a46c1754..b726b053493 100644 --- a/app/uploaders/legacy_artifact_uploader.rb +++ b/app/uploaders/legacy_artifact_uploader.rb @@ -1,17 +1,16 @@ -class LegacyArtifactUploader < ObjectStoreUploader - storage_options Gitlab.config.artifacts +class LegacyArtifactUploader < GitlabUploader + extend Workhorse::UploadPath + include ObjectStorage::Concern - def self.local_store_path - Gitlab.config.artifacts.path - end + storage_options Gitlab.config.artifacts - def self.artifacts_upload_path - File.join(self.local_store_path, 'tmp/uploads/') + def store_dir + dynamic_segment end private - def default_path + def dynamic_segment File.join(model.created_at.utc.strftime('%Y_%m'), model.project_id.to_s, model.id.to_s) end end diff --git a/app/uploaders/lfs_object_uploader.rb b/app/uploaders/lfs_object_uploader.rb index fa42e4710b7..e7cce1bbb0a 100644 --- a/app/uploaders/lfs_object_uploader.rb +++ b/app/uploaders/lfs_object_uploader.rb @@ -1,17 +1,25 @@ -class LfsObjectUploader < ObjectStoreUploader - storage_options Gitlab.config.lfs +class LfsObjectUploader < GitlabUploader + extend Workhorse::UploadPath + include ObjectStorage::Concern - def self.local_store_path - Gitlab.config.lfs.storage_path + # LfsObject are in `tmp/upload` instead of `tmp/uploads` + def self.workhorse_upload_path + File.join(root, 'tmp/upload') end + storage_options Gitlab.config.lfs + def filename model.oid[4..-1] end + def store_dir + dynamic_segment + end + private - def default_path - "#{model.oid[0, 2]}/#{model.oid[2, 2]}" + def dynamic_segment + File.join(model.oid[0, 2], model.oid[2, 2]) end end diff --git a/app/uploaders/namespace_file_uploader.rb b/app/uploaders/namespace_file_uploader.rb index 672126e9ec2..269415b1926 100644 --- a/app/uploaders/namespace_file_uploader.rb +++ b/app/uploaders/namespace_file_uploader.rb @@ -1,15 +1,26 @@ class NamespaceFileUploader < FileUploader - def self.base_dir - File.join(root_dir, '-', 'system', 'namespace') + # Re-Override + def self.root + options.storage_path end - def self.dynamic_path_segment(model) - dynamic_path_builder(model.id.to_s) + def self.base_dir(model) + File.join(options.base_dir, 'namespace', model_path_segment(model)) end - private + def self.model_path_segment(model) + File.join(model.id.to_s) + end + + # Re-Override + def store_dir + store_dirs[object_store] + end - def secure_url - File.join('/uploads', @secret, file.filename) + def store_dirs + { + Store::LOCAL => File.join(base_dir, dynamic_segment), + Store::REMOTE => File.join('namespace', model_path_segment, dynamic_segment) + } end end diff --git a/app/uploaders/object_store_uploader.rb b/app/uploaders/object_store_uploader.rb deleted file mode 100644 index bb25dc4219f..00000000000 --- a/app/uploaders/object_store_uploader.rb +++ /dev/null @@ -1,215 +0,0 @@ -require 'fog/aws' -require 'carrierwave/storage/fog' - -class ObjectStoreUploader < GitlabUploader - before :store, :set_default_local_store - before :store, :verify_license! - - LOCAL_STORE = 1 - REMOTE_STORE = 2 - - class << self - def storage_options(options) - @storage_options = options - end - - def object_store_options - @storage_options&.object_store - end - - def object_store_enabled? - object_store_options&.enabled - end - - def background_upload_enabled? - object_store_options&.background_upload - end - - def object_store_credentials - @object_store_credentials ||= object_store_options&.connection&.to_hash&.deep_symbolize_keys - end - - def object_store_directory - object_store_options&.remote_directory - end - - def local_store_path - raise NotImplementedError - end - end - - def file_storage? - storage.is_a?(CarrierWave::Storage::File) - end - - def file_cache_storage? - cache_storage.is_a?(CarrierWave::Storage::File) - end - - def real_object_store - model.public_send(store_serialization_column) # rubocop:disable GitlabSecurity/PublicSend - end - - def object_store - subject.public_send(:"#{field}_store") - end - - def object_store=(value) - @storage = nil - model.public_send(:"#{store_serialization_column}=", value) # rubocop:disable GitlabSecurity/PublicSend - end - - def store_dir - if file_storage? - default_local_path - else - default_path - end - end - - def use_file - if file_storage? - return yield path - end - - begin - cache_stored_file! - yield cache_path - ensure - cache_storage.delete_dir!(cache_path(nil)) - end - end - - def filename - super || file&.filename - end - - def migrate!(new_store) - raise 'Undefined new store' unless new_store - - return unless object_store != new_store - return unless file - - old_file = file - old_store = object_store - - # for moving remote file we need to first store it locally - cache_stored_file! unless file_storage? - - # change storage - self.object_store = new_store - - with_callbacks(:store, file) do - storage.store!(file).tap do |new_file| - # since we change storage store the new storage - # in case of failure delete new file - begin - model.save! - rescue => e - new_file.delete - self.object_store = old_store - raise e - end - - old_file.delete - end - end - end - - def schedule_migration_to_object_storage(*args) - return unless self.class.object_store_enabled? - return unless self.class.background_upload_enabled? - return unless self.licensed? - return unless self.file_storage? - - ObjectStorageUploadWorker.perform_async(self.class.name, model.class.name, mounted_as, model.id) - end - - def fog_directory - self.class.object_store_options.remote_directory - end - - def fog_credentials - self.class.object_store_options.connection - end - - def fog_public - false - end - - def move_to_store - file.try(:storage) == storage - end - - def move_to_cache - file.try(:storage) == cache_storage - end - - # We block storing artifacts on Object Storage, not receiving - def verify_license!(new_file) - return if file_storage? - - raise 'Object Storage feature is missing' unless licensed? - end - - def exists? - file.try(:exists?) - end - - def cache_dir - File.join(self.class.local_store_path, 'tmp/cache') - end - - # Override this if you don't want to save local files by default to the Rails.root directory - def work_dir - # Default path set by CarrierWave: - # https://github.com/carrierwaveuploader/carrierwave/blob/v1.1.0/lib/carrierwave/uploader/cache.rb#L182 - # CarrierWave.tmp_path - File.join(self.class.local_store_path, 'tmp/work') - end - - def licensed? - License.feature_available?(:object_storage) - end - - private - - def set_default_local_store(new_file) - self.object_store = LOCAL_STORE unless self.object_store - end - - def default_local_path - File.join(self.class.local_store_path, default_path) - end - - def default_path - raise NotImplementedError - end - - def serialization_column - model.class.uploader_option(mounted_as, :mount_on) || mounted_as - end - - def store_serialization_column - :"#{serialization_column}_store" - end - - def storage - @storage ||= - if object_store == REMOTE_STORE - remote_storage - else - local_storage - end - end - - def remote_storage - raise 'Object Storage is not enabled' unless self.class.object_store_enabled? - - CarrierWave::Storage::Fog.new(self) - end - - def local_storage - CarrierWave::Storage::File.new(self) - end -end diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb index 3298ad104ec..440972affec 100644 --- a/app/uploaders/personal_file_uploader.rb +++ b/app/uploaders/personal_file_uploader.rb @@ -1,23 +1,40 @@ class PersonalFileUploader < FileUploader - def self.dynamic_path_segment(model) - File.join(CarrierWave.root, model_path(model)) + # Re-Override + def self.root + options.storage_path end - def self.base_dir - File.join(root_dir, '-', 'system') + def self.base_dir(model) + File.join(options.base_dir, model_path_segment(model)) end - private + def self.model_path_segment(model) + return 'temp/' unless model - def secure_url - File.join(self.class.model_path(model), secret, file.filename) + File.join(model.class.to_s.underscore, model.id.to_s) + end + + def object_store + return Store::LOCAL unless model + + super + end + + # Revert-Override + def store_dir + store_dirs[object_store] + end + + def store_dirs + { + Store::LOCAL => File.join(base_dir, dynamic_segment), + Store::REMOTE => File.join(model_path_segment, dynamic_segment) + } end - def self.model_path(model) - if model - File.join("/#{base_dir}", model.class.to_s.underscore, model.id.to_s) - else - File.join("/#{base_dir}", 'temp') - end + private + + def secure_url + File.join('/', base_dir, secret, file.filename) end end diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index feb4f04d7b7..dfb8dccec57 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -1,35 +1,61 @@ module RecordsUploads - extend ActiveSupport::Concern + module Concern + extend ActiveSupport::Concern - included do - after :store, :record_upload - before :remove, :destroy_upload - end + attr_accessor :upload - # After storing an attachment, create a corresponding Upload record - # - # NOTE: We're ignoring the argument passed to this callback because we want - # the `SanitizedFile` object from `CarrierWave::Uploader::Base#file`, not the - # `Tempfile` object the callback gets. - # - # Called `after :store` - def record_upload(_tempfile = nil) - return unless model - return unless file_storage? - return unless file.exists? - - Upload.record(self) - end + included do + after :store, :record_upload + before :remove, :destroy_upload + end + + # After storing an attachment, create a corresponding Upload record + # + # NOTE: We're ignoring the argument passed to this callback because we want + # the `SanitizedFile` object from `CarrierWave::Uploader::Base#file`, not the + # `Tempfile` object the callback gets. + # + # Called `after :store` + def record_upload(_tempfile = nil) + return unless model + return unless file && file.exists? + + Upload.transaction do + uploads.where(path: upload_path).delete_all + upload.destroy! if upload + + self.upload = build_upload_from_uploader(self) + upload.save! + end + end + + def upload_path + File.join(store_dir, filename.to_s) + end + + private + + def uploads + Upload.order(id: :desc).where(uploader: self.class.to_s) + end - private + def build_upload_from_uploader(uploader) + Upload.new( + size: uploader.file.size, + path: uploader.upload_path, + model: uploader.model, + uploader: uploader.class.to_s + ) + end - # Before removing an attachment, destroy any Upload records at the same path - # - # Called `before :remove` - def destroy_upload(*args) - return unless file_storage? - return unless file + # Before removing an attachment, destroy any Upload records at the same path + # + # Called `before :remove` + def destroy_upload(*args) + return unless file && file.exists? - Upload.remove_path(relative_path) + self.upload = nil + uploads.where(path: upload_path).delete_all + end end end diff --git a/app/uploaders/uploader_helper.rb b/app/uploaders/uploader_helper.rb index 7635c20ab3a..fd446d31092 100644 --- a/app/uploaders/uploader_helper.rb +++ b/app/uploaders/uploader_helper.rb @@ -32,14 +32,7 @@ module UploaderHelper def extension_match?(extensions) return false unless file - extension = - if file.respond_to?(:extension) - file.extension - else - # Not all CarrierWave storages respond to :extension - File.extname(file.path).delete('.') - end - + extension = file.try(:extension) || File.extname(file.path).delete('.') extensions.include?(extension.downcase) end end diff --git a/app/uploaders/workhorse.rb b/app/uploaders/workhorse.rb new file mode 100644 index 00000000000..782032cf516 --- /dev/null +++ b/app/uploaders/workhorse.rb @@ -0,0 +1,7 @@ +module Workhorse + module UploadPath + def workhorse_upload_path + File.join(root, base_dir, 'tmp/uploads') + end + end +end diff --git a/app/workers/object_storage_upload_worker.rb b/app/workers/object_storage_upload_worker.rb index 0b9411ff2df..e087261770f 100644 --- a/app/workers/object_storage_upload_worker.rb +++ b/app/workers/object_storage_upload_worker.rb @@ -8,16 +8,16 @@ class ObjectStorageUploadWorker uploader_class = uploader_class_name.constantize subject_class = subject_class_name.constantize + return unless uploader_class < ObjectStorage::Concern return unless uploader_class.object_store_enabled? + return unless uploader_class.licensed? return unless uploader_class.background_upload_enabled? - subject = subject_class.find_by(id: subject_id) - return unless subject - - file = subject.public_send(file_field) # rubocop:disable GitlabSecurity/PublicSend - - return unless file.licensed? - - file.migrate!(uploader_class::REMOTE_STORE) + subject = subject_class.find(subject_id) + uploader = subject.public_send(file_field) # rubocop:disable GitlabSecurity/PublicSend + uploader.migrate!(ObjectStorage::Store::REMOTE) + rescue RecordNotFound + # does not retry when the record do not exists + Rails.logger.warn("Cannot find subject #{subject_class} with id=#{subject_id}.") end end diff --git a/app/workers/upload_checksum_worker.rb b/app/workers/upload_checksum_worker.rb index 9222760c031..65d40336f18 100644 --- a/app/workers/upload_checksum_worker.rb +++ b/app/workers/upload_checksum_worker.rb @@ -3,7 +3,7 @@ class UploadChecksumWorker def perform(upload_id) upload = Upload.find(upload_id) - upload.calculate_checksum + upload.calculate_checksum! upload.save! rescue ActiveRecord::RecordNotFound Rails.logger.error("UploadChecksumWorker: couldn't find upload #{upload_id}, skipping") |