Commit 63d52d83 authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Persist tmp snippet uploads

It persist temporary personal snippets under
user/:id namespaces temporarily while creating
a upload record to track it. If an user gets removed
while it's still a tmp upload, it also gets removed.
If the tmp upload is sent, the upload gets moved to
personal_snippets/:id as before. The upload record
also gets updated to the new model type as well.
parent e3eeb779
...@@ -137,7 +137,7 @@ class SnippetsController < ApplicationController ...@@ -137,7 +137,7 @@ class SnippetsController < ApplicationController
def move_temporary_files def move_temporary_files
params[:files].each do |file| params[:files].each do |file|
FileMover.new(file, @snippet).execute FileMover.new(file, from_model: current_user, to_model: @snippet).execute
end end
end end
end end
...@@ -41,7 +41,11 @@ class UploadsController < ApplicationController ...@@ -41,7 +41,11 @@ class UploadsController < ApplicationController
when Note when Note
can?(current_user, :read_project, model.project) can?(current_user, :read_project, model.project)
when User when User
true # We validate the current user has enough (writing)
# access to itself when a secret is given.
# For instance, user avatars are readable by anyone,
# while temporary, user snippet uploads are not.
!secret? || can?(current_user, :update_user, model)
when Appearance when Appearance
true true
else else
...@@ -56,9 +60,13 @@ class UploadsController < ApplicationController ...@@ -56,9 +60,13 @@ class UploadsController < ApplicationController
def authorize_create_access! def authorize_create_access!
return unless model return unless model
# for now we support only personal snippets comments. Only personal_snippet authorized =
# is allowed as a model to #create through routing. case model
authorized = can?(current_user, :create_note, model) when User
can?(current_user, :update_user, model)
else
can?(current_user, :create_note, model)
end
render_unauthorized unless authorized render_unauthorized unless authorized
end end
...@@ -75,6 +83,10 @@ class UploadsController < ApplicationController ...@@ -75,6 +83,10 @@ class UploadsController < ApplicationController
User === model || Appearance === model User === model || Appearance === model
end end
def secret?
params[:secret].present?
end
def upload_model_class def upload_model_class
MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError) MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError)
end end
......
# frozen_string_literal: true # frozen_string_literal: true
module SnippetsHelper module SnippetsHelper
def snippets_upload_path(snippet, user)
return unless user
if snippet&.persisted?
upload_path('personal_snippet', id: snippet.id)
else
upload_path('user', id: user.id)
end
end
def reliable_snippet_path(snippet, opts = nil) def reliable_snippet_path(snippet, opts = nil)
if snippet.project_id? if snippet.project_id?
project_snippet_path(snippet.project, snippet, opts) project_snippet_path(snippet.project, snippet, opts)
......
# frozen_string_literal: true # frozen_string_literal: true
class FileMover class FileMover
attr_reader :secret, :file_name, :model, :update_field attr_reader :secret, :file_name, :from_model, :to_model, :update_field
def initialize(file_path, model, update_field = :description) def initialize(file_path, update_field = :description, from_model:, to_model:)
@secret = File.split(File.dirname(file_path)).last @secret = File.split(File.dirname(file_path)).last
@file_name = File.basename(file_path) @file_name = File.basename(file_path)
@model = model @from_model = from_model
@to_model = to_model
@update_field = update_field @update_field = update_field
end end
...@@ -16,7 +17,7 @@ class FileMover ...@@ -16,7 +17,7 @@ class FileMover
move move
if update_markdown if update_markdown
uploader.record_upload update_upload_model
uploader.schedule_background_upload uploader.schedule_background_upload
end end
end end
...@@ -35,14 +36,20 @@ class FileMover ...@@ -35,14 +36,20 @@ class FileMover
end end
def update_markdown def update_markdown
updated_text = model.read_attribute(update_field) updated_text = to_model.read_attribute(update_field)
.gsub(temp_file_uploader.markdown_link, uploader.markdown_link) .gsub(temp_file_uploader.markdown_link, uploader.markdown_link)
model.update_attribute(update_field, updated_text) to_model.update_attribute(update_field, updated_text)
rescue rescue
revert revert
false false
end end
def update_upload_model
return unless upload = temp_file_uploader.upload
upload.update!(model_id: to_model.id, model_type: to_model.type)
end
def temp_file_path def temp_file_path
return @temp_file_path if @temp_file_path return @temp_file_path if @temp_file_path
...@@ -60,15 +67,15 @@ class FileMover ...@@ -60,15 +67,15 @@ class FileMover
end end
def uploader def uploader
@uploader ||= PersonalFileUploader.new(model, secret: secret) @uploader ||= PersonalFileUploader.new(to_model, secret: secret)
end end
def temp_file_uploader def temp_file_uploader
@temp_file_uploader ||= PersonalFileUploader.new(nil, secret: secret) @temp_file_uploader ||= PersonalFileUploader.new(from_model, secret: secret)
end end
def revert def revert
Rails.logger.warn("Markdown not updated, file move reverted for #{model}") Rails.logger.warn("Markdown not updated, file move reverted for #{to_model}")
FileUtils.move(file_path, temp_file_path) FileUtils.move(file_path, temp_file_path)
end end
......
- header_title _("Snippets"), snippets_path - header_title _("Snippets"), snippets_path
- snippets_upload_path = snippets_upload_path(@snippet, current_user)
- content_for :page_specific_javascripts do - content_for :page_specific_javascripts do
- if @snippet && current_user - if snippets_upload_path
-# haml-lint:disable InlineJavaScript -# haml-lint:disable InlineJavaScript
:javascript :javascript
window.uploads_path = "#{upload_path('personal_snippet', id: @snippet.id)}"; window.uploads_path = "#{snippets_upload_path}";
= render template: "layouts/application" = render template: "layouts/application"
---
title: Persist tmp snippet uploads at users
merge_request:
author:
type: security
...@@ -7,7 +7,7 @@ scope path: :uploads do ...@@ -7,7 +7,7 @@ scope path: :uploads do
# show uploads for models, snippets (notes) available for now # show uploads for models, snippets (notes) available for now
get '-/system/:model/:id/:secret/:filename', get '-/system/:model/:id/:secret/:filename',
to: 'uploads#show', to: 'uploads#show',
constraints: { model: /personal_snippet/, id: /\d+/, filename: %r{[^/]+} } constraints: { model: /personal_snippet|user/, id: /\d+/, filename: %r{[^/]+} }
# show temporary uploads # show temporary uploads
get '-/system/temp/:secret/:filename', get '-/system/temp/:secret/:filename',
...@@ -28,7 +28,7 @@ scope path: :uploads do ...@@ -28,7 +28,7 @@ scope path: :uploads do
# create uploads for models, snippets (notes) available for now # create uploads for models, snippets (notes) available for now
post ':model', post ':model',
to: 'uploads#create', to: 'uploads#create',
constraints: { model: /personal_snippet/, id: /\d+/ }, constraints: { model: /personal_snippet|user/, id: /\d+/ },
as: 'upload' as: 'upload'
end end
......
...@@ -209,8 +209,8 @@ describe SnippetsController do ...@@ -209,8 +209,8 @@ describe SnippetsController do
context 'when the snippet description contains a file' do context 'when the snippet description contains a file' do
include FileMoverHelpers include FileMoverHelpers
let(:picture_file) { '/-/system/temp/secret56/picture.jpg' } let(:picture_file) { "/-/system/user/#{user.id}/secret56/picture.jpg" }
let(:text_file) { '/-/system/temp/secret78/text.txt' } let(:text_file) { "/-/system/user/#{user.id}/secret78/text.txt" }
let(:description) do let(:description) do
"Description with picture: ![picture](/uploads#{picture_file}) and "\ "Description with picture: ![picture](/uploads#{picture_file}) and "\
"text: [text.txt](/uploads#{text_file})" "text: [text.txt](/uploads#{text_file})"
......
...@@ -24,11 +24,13 @@ describe UploadsController do ...@@ -24,11 +24,13 @@ describe UploadsController do
let!(:user) { create(:user, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) } let!(:user) { create(:user, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) }
describe 'POST create' do describe 'POST create' do
let(:model) { 'personal_snippet' }
let(:snippet) { create(:personal_snippet, :public) }
let(:jpg) { fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg') } let(:jpg) { fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg') }
let(:txt) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') } let(:txt) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') }
context 'snippet uploads' do
let(:model) { 'personal_snippet' }
let(:snippet) { create(:personal_snippet, :public) }
context 'when a user does not have permissions to upload a file' do context 'when a user does not have permissions to upload a file' do
it "returns 401 when the user is not logged in" do it "returns 401 when the user is not logged in" do
post :create, params: { model: model, id: snippet.id }, format: :json post :create, params: { model: model, id: snippet.id }, format: :json
...@@ -107,38 +109,75 @@ describe UploadsController do ...@@ -107,38 +109,75 @@ describe UploadsController do
end end
end end
end end
end
end
context 'user uploads' do
let(:model) { 'user' }
it 'returns 401 when the user has no access' do
post :create, params: { model: 'user', id: user.id }, format: :json
expect(response).to have_gitlab_http_status(401)
end
context 'when user is logged in' do
before do
sign_in(user)
end
context 'temporal with valid image' do
subject do subject do
post :create, params: { model: 'personal_snippet', file: jpg }, format: :json post :create, params: { model: model, id: user.id, file: jpg }, format: :json
end end
it 'returns a content with original filename, new link, and correct type.' do it 'returns a content with original filename, new link, and correct type.' do
subject subject
expect(response.body).to match '\"alt\":\"rails_sample\"' expect(response.body).to match '\"alt\":\"rails_sample\"'
expect(response.body).to match "\"url\":\"/uploads/-/system/temp" expect(response.body).to match "\"url\":\"/uploads/-/system/user/#{user.id}/"
end end
it 'does not create an Upload record' do it 'creates a corresponding Upload record' do
expect { subject }.not_to change { Upload.count } expect { subject }.to change { Upload.count }
upload = Upload.last
aggregate_failures do
expect(upload).to exist
expect(upload.model).to eq user
end end
end end
context 'temporal with valid non-image file' do context 'with valid non-image file' do
subject do subject do
post :create, params: { model: 'personal_snippet', file: txt }, format: :json post :create, params: { model: model, id: user.id, file: txt }, format: :json
end end
it 'returns a content with original filename, new link, and correct type.' do it 'returns a content with original filename, new link, and correct type.' do
subject subject
expect(response.body).to match '\"alt\":\"doc_sample.txt\"' expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
expect(response.body).to match "\"url\":\"/uploads/-/system/temp" expect(response.body).to match "\"url\":\"/uploads/-/system/user/#{user.id}/"
end
it 'creates a corresponding Upload record' do
expect { subject }.to change { Upload.count }
upload = Upload.last
aggregate_failures do
expect(upload).to exist
expect(upload.model).to eq user
end
end end
end
it 'returns 404 when given user is not the logged in one' do
another_user = create(:user)
it 'does not create an Upload record' do post :create, params: { model: model, id: another_user.id, file: txt }, format: :json
expect { subject }.not_to change { Upload.count }
expect(response).to have_gitlab_http_status(404)
end end
end end
end end
......
...@@ -41,7 +41,7 @@ describe 'User creates snippet', :js do ...@@ -41,7 +41,7 @@ describe 'User creates snippet', :js do
expect(page).to have_content('My Snippet') expect(page).to have_content('My Snippet')
link = find('a.no-attachment-icon img[alt="banana_sample"]')['src'] link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
expect(link).to match(%r{/uploads/-/system/temp/\h{32}/banana_sample\.gif\z}) expect(link).to match(%r{/uploads/-/system/user/#{user.id}/\h{32}/banana_sample\.gif\z})
reqs = inspect_requests { visit(link) } reqs = inspect_requests { visit(link) }
expect(reqs.first.status_code).to eq(200) expect(reqs.first.status_code).to eq(200)
......
...@@ -12,10 +12,19 @@ describe 'Uploads', 'routing' do ...@@ -12,10 +12,19 @@ describe 'Uploads', 'routing' do
) )
end end
it 'allows creating uploads for users' do
expect(post('/uploads/user?id=1')).to route_to(
controller: 'uploads',
action: 'create',
model: 'user',
id: '1'
)
end
it 'does not allow creating uploads for other models' do it 'does not allow creating uploads for other models' do
UploadsController::MODEL_CLASSES.keys.compact.each do |model| unroutable_models = UploadsController::MODEL_CLASSES.keys.compact - %w(personal_snippet user)
next if model == 'personal_snippet'
unroutable_models.each do |model|
expect(post("/uploads/#{model}?id=1")).not_to be_routable expect(post("/uploads/#{model}?id=1")).not_to be_routable
end end
end end
......
...@@ -3,20 +3,29 @@ require 'spec_helper' ...@@ -3,20 +3,29 @@ require 'spec_helper'
describe FileMover do describe FileMover do
include FileMoverHelpers include FileMoverHelpers
let(:user) { create(:user) }
let(:filename) { 'banana_sample.gif' } let(:filename) { 'banana_sample.gif' }
let(:temp_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) } let(:secret) { 'secret55' }
let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", secret, filename) }
let(:temp_description) do let(:temp_description) do
"test ![banana_sample](/#{temp_file_path}) "\ "test ![banana_sample](/#{temp_file_path}) "\
"same ![banana_sample](/#{temp_file_path}) " "same ![banana_sample](/#{temp_file_path}) "
end end
let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) } let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, secret, filename) }
let(:snippet) { create(:personal_snippet, description: temp_description) } let(:snippet) { create(:personal_snippet, description: temp_description) }
subject { described_class.new(temp_file_path, snippet).execute } let(:tmp_uploader) do
PersonalFileUploader.new(user, secret: secret)
end
let(:file) { fixture_file_upload('spec/fixtures/banana_sample.gif') }
subject { described_class.new(temp_file_path, from_model: user, to_model: snippet).execute }
describe '#execute' do describe '#execute' do
before do before do
tmp_uploader.store!(file)
expect(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path))) expect(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path)))
expect(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path)) expect(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path))
allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true) allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true)
...@@ -25,6 +34,8 @@ describe FileMover do ...@@ -25,6 +34,8 @@ describe FileMover do
stub_file_mover(temp_file_path) stub_file_mover(temp_file_path)
end end
let(:tmp_upload) { tmp_uploader.upload }
context 'when move and field update successful' do context 'when move and field update successful' do
it 'updates the description correctly' do it 'updates the description correctly' do
subject subject
...@@ -36,8 +47,10 @@ describe FileMover do ...@@ -36,8 +47,10 @@ describe FileMover do
) )
end end
it 'creates a new update record' do it 'updates existing upload record' do
expect { subject }.to change { Upload.count }.by(1) expect { subject }
.to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
.from([user.id, 'User']).to([snippet.id, 'PersonalSnippet'])
end end
it 'schedules a background migration' do it 'schedules a background migration' do
...@@ -52,30 +65,31 @@ describe FileMover do ...@@ -52,30 +65,31 @@ describe FileMover do
expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path)) expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path))
end end
subject { described_class.new(file_path, snippet, :non_existing_field).execute } subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute }
it 'does not update the description' do it 'does not update the description' do
subject subject
expect(snippet.reload.description) expect(snippet.reload.description)
.to eq( .to eq(
"test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "\ "test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
"same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) " "same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "
) )
end end
it 'does not create a new update record' do it 'does not change the upload record' do
expect { subject }.not_to change { Upload.count } expect { subject }
.not_to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') }
end end
end end
end end
context 'security' do context 'security' do
context 'when relative path is involved' do context 'when relative path is involved' do
let(:temp_file_path) { File.join('uploads/-/system/temp', '..', 'another_subdir_of_temp') } let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", '..', 'another_subdir_of_temp') }
it 'does not trigger move if path is outside designated directory' do it 'does not trigger move if path is outside designated directory' do
stub_file_mover('uploads/-/system/another_subdir_of_temp') stub_file_mover("uploads/-/system/user/#{user.id}/another_subdir_of_temp")
expect(FileUtils).not_to receive(:move) expect(FileUtils).not_to receive(:move)
subject subject
......
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