Commit a1f44c1b authored by Stan Hu's avatar Stan Hu

Fix incorrect prefix used in new uploads for personal snippets

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24550 fixed the
case where the wrong path on disk was being searched, but it
inadvertently ommitted the `/uploads/-/system` prefix when rendering the
Markdown for personal snippet uploads when they were stored directly in
object storage.

A personal snippet path is stored using FileUploader#upload_path.
The format for the path:

Local storage: :random_hex/:filename.
Object storage: personal_snippet/:id/:random_hex/:filename.

upload_paths represent the possible paths for a given identifier,
which will vary depending on whether the file is stored in local or
object storage. upload_path should match an element in upload_paths.

base_dir represents the path seen by the user in Markdown, and it
should always be prefixed with uploads/-/system.

store_dirs represent the paths that are actually used on disk. For
object storage, this should omit the prefix /uploads/-/system.

For example, consider the requested path
/uploads/-/system/personal_snippet/172/ff4ad5c2/file.png.

For local storage:

base_dir: uploads/-/system/personal_snippet/172
upload_path: ff4ad5c2/file.png
upload_paths: ["ff4ad5c2/file.png", "personal_snippet/172/ff4ad5c2/file.png"].
store_dirs: {1=>"uploads/-/system/personal_snippet/172/ff4ad5c2",
             2=>"personal_snippet/172/ff4ad5c2"}

For object storage:

upload_path: personal_snippet/172/ff4ad5c2/file.png

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/61671
parent 04794fb4
......@@ -109,12 +109,20 @@ class FileUploader < GitlabUploader
def upload_path
if file_storage?
# Legacy path relative to project.full_path
File.join(dynamic_segment, identifier)
local_storage_path(identifier)
else
File.join(store_dir, identifier)
remote_storage_path(identifier)
end
end
def local_storage_path(file_identifier)
File.join(dynamic_segment, file_identifier)
end
def remote_storage_path(file_identifier)
File.join(store_dir, file_identifier)
end
def store_dirs
{
Store::LOCAL => File.join(base_dir, dynamic_segment),
......
......@@ -6,15 +6,12 @@ class PersonalFileUploader < FileUploader
options.storage_path
end
def self.base_dir(model, store = nil)
base_dirs(model)[store || Store::LOCAL]
end
def self.base_dirs(model)
{
Store::LOCAL => File.join(options.base_dir, model_path_segment(model)),
Store::REMOTE => model_path_segment(model)
}
def self.base_dir(model, _store = nil)
# base_dir is the path seen by the user when rendering Markdown, so
# it should be the same for both local and object storage. It is
# typically prefaced with uploads/-/system, but that prefix
# is omitted in the path stored on disk.
File.join(options.base_dir, model_path_segment(model))
end
def self.model_path_segment(model)
......@@ -40,8 +37,61 @@ class PersonalFileUploader < FileUploader
store_dirs[object_store]
end
# A personal snippet path is stored using FileUploader#upload_path.
#
# The format for the path:
#
# Local storage: :random_hex/:filename.
# Object storage: personal_snippet/:id/:random_hex/:filename.
#
# upload_paths represent the possible paths for a given identifier,
# which will vary depending on whether the file is stored in local or
# object storage. upload_path should match an element in upload_paths.
#
# base_dir represents the path seen by the user in Markdown, and it
# should always be prefixed with uploads/-/system.
#
# store_dirs represent the paths that are actually used on disk. For
# object storage, this should omit the prefix /uploads/-/system.
#
# For example, consider the requested path /uploads/-/system/personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20/file.png.
#
# For local storage:
#
# File on disk: /opt/gitlab/embedded/service/gitlab-rails/public/uploads/-/system/personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20/file.png.
#
# base_dir: uploads/-/system/personal_snippet/172
# upload_path: ff4ad5c2e40b39ae57cda51577317d20/file.png
# upload_paths: ["ff4ad5c2e40b39ae57cda51577317d20/file.png", "personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20/file.png"].
# store_dirs:
# => {1=>"uploads/-/system/personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20", 2=>"personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20"}
#
# For object storage:
#
# upload_path: personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20/file.png
def upload_paths(identifier)
[
local_storage_path(identifier),
File.join(remote_storage_base_path, identifier)
]
end
def store_dirs
{
Store::LOCAL => File.join(base_dir, dynamic_segment),
Store::REMOTE => remote_storage_base_path
}
end
private
# To avoid prefacing the remote storage path with `/uploads/-/system`,
# we just drop that part so that the destination path will be
# personal_snippet/:id/:random_hex/:filename.
def remote_storage_base_path
File.join(self.class.model_path_segment(model), dynamic_segment)
end
def secure_url
File.join('/', base_dir, secret, file.filename)
end
......
---
title: Fix incorrect prefix used in new uploads for personal snippets
merge_request: 28337
author:
type: fixed
......@@ -7,33 +7,19 @@ describe PersonalFileUploader do
subject { uploader }
it_behaves_like 'builds correct paths',
store_dir: %r[uploads/-/system/personal_snippet/\d+],
upload_path: %r[\h+/\S+],
absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet\/\d+\/\h+\/\S+$]
context "object_store is REMOTE" do
shared_examples '#base_dir' do
before do
stub_uploads_object_storage
end
include_context 'with storage', described_class::Store::REMOTE
it_behaves_like 'builds correct paths',
store_dir: %r[\d+/\h+],
upload_path: %r[^personal_snippet\/\d+\/\h+\/<filename>]
subject.instance_variable_set(:@secret, 'secret')
end
describe '#upload_paths' do
it 'builds correct paths for both local and remote storage' do
paths = uploader.upload_paths('test.jpg')
it 'is prefixed with uploads/-/system' do
allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name'))
expect(paths.first).to match(%r[\h+\/test.jpg])
expect(paths.second).to match(%r[^personal_snippet\/\d+\/\h+\/test.jpg])
expect(described_class.base_dir(model)).to eq("uploads/-/system/personal_snippet/#{model.id}")
end
end
describe '#to_h' do
shared_examples '#to_h' do
before do
subject.instance_variable_set(:@secret, 'secret')
end
......@@ -50,6 +36,40 @@ describe PersonalFileUploader do
end
end
describe '#upload_paths' do
it 'builds correct paths for both local and remote storage' do
paths = uploader.upload_paths('test.jpg')
expect(paths.first).to match(%r[\h+\/test.jpg])
expect(paths.second).to match(%r[^personal_snippet\/\d+\/\h+\/test.jpg])
end
end
context 'object_store is LOCAL' do
it_behaves_like 'builds correct paths',
store_dir: %r[uploads/-/system/personal_snippet/\d+/\h+],
upload_path: %r[\h+/\S+],
absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet\/\d+\/\h+\/\S+$]
it_behaves_like '#base_dir'
it_behaves_like '#to_h'
end
context "object_store is REMOTE" do
before do
stub_uploads_object_storage
end
include_context 'with storage', described_class::Store::REMOTE
it_behaves_like 'builds correct paths',
store_dir: %r[\d+/\h+],
upload_path: %r[^personal_snippet\/\d+\/\h+\/<filename>]
it_behaves_like '#base_dir'
it_behaves_like '#to_h'
end
describe "#migrate!" do
before do
uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt'))
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment