diff options
author | Oswaldo Ferreira <oswaldo@gitlab.com> | 2019-06-05 02:50:57 +0300 |
---|---|---|
committer | Oswaldo Ferreira <oswaldo@gitlab.com> | 2019-06-17 17:25:40 +0300 |
commit | 44e1915d4f0e20cf445196ccc7e1d279c75ef0ce (patch) | |
tree | 2619dcea9bf96af77023f8fb9a0b84cd5a8f69ee /spec | |
parent | e398409a74db7f3ca1c90d3b056b3a84ebb1b6cf (diff) |
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.
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/snippets_controller_spec.rb | 4 | ||||
-rw-r--r-- | spec/controllers/uploads_controller_spec.rb | 177 | ||||
-rw-r--r-- | spec/features/snippets/user_creates_snippet_spec.rb | 2 | ||||
-rw-r--r-- | spec/routing/uploads_routing_spec.rb | 13 | ||||
-rw-r--r-- | spec/uploaders/file_mover_spec.rb | 38 |
5 files changed, 148 insertions, 86 deletions
diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index f8666a1986f..3aba02bf3ff 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -209,8 +209,8 @@ describe SnippetsController do context 'when the snippet description contains a file' do include FileMoverHelpers - let(:picture_file) { '/-/system/temp/secret56/picture.jpg' } - let(:text_file) { '/-/system/temp/secret78/text.txt' } + let(:picture_file) { "/-/system/user/#{user.id}/secret56/picture.jpg" } + let(:text_file) { "/-/system/user/#{user.id}/secret78/text.txt" } let(:description) do "Description with picture: ![picture](/uploads#{picture_file}) and "\ "text: [text.txt](/uploads#{text_file})" diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index d27658e02cb..0876502a899 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -24,121 +24,160 @@ describe UploadsController do let!(:user) { create(:user, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) } 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(:txt) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') } - context 'when a user does not have permissions to upload a file' do - it "returns 401 when the user is not logged in" do - post :create, params: { model: model, id: snippet.id }, format: :json + context 'snippet uploads' do + let(:model) { 'personal_snippet' } + let(:snippet) { create(:personal_snippet, :public) } - expect(response).to have_gitlab_http_status(401) - end + context 'when a user does not have permissions to upload a file' do + it "returns 401 when the user is not logged in" do + post :create, params: { model: model, id: snippet.id }, format: :json - it "returns 404 when user can't comment on a snippet" do - private_snippet = create(:personal_snippet, :private) + expect(response).to have_gitlab_http_status(401) + end - sign_in(user) - post :create, params: { model: model, id: private_snippet.id }, format: :json + it "returns 404 when user can't comment on a snippet" do + private_snippet = create(:personal_snippet, :private) - expect(response).to have_gitlab_http_status(404) - end - end + sign_in(user) + post :create, params: { model: model, id: private_snippet.id }, format: :json - context 'when a user is logged in' do - before do - sign_in(user) + expect(response).to have_gitlab_http_status(404) + end end - it "returns an error without file" do - post :create, params: { model: model, id: snippet.id }, format: :json + context 'when a user is logged in' do + before do + sign_in(user) + end - expect(response).to have_gitlab_http_status(422) - end + it "returns an error without file" do + post :create, params: { model: model, id: snippet.id }, format: :json - it "returns an error with invalid model" do - expect { post :create, params: { model: 'invalid', id: snippet.id }, format: :json } - .to raise_error(ActionController::UrlGenerationError) - end + expect(response).to have_gitlab_http_status(422) + end - it "returns 404 status when object not found" do - post :create, params: { model: model, id: 9999 }, format: :json + it "returns an error with invalid model" do + expect { post :create, params: { model: 'invalid', id: snippet.id }, format: :json } + .to raise_error(ActionController::UrlGenerationError) + end - expect(response).to have_gitlab_http_status(404) - end + it "returns 404 status when object not found" do + post :create, params: { model: model, id: 9999 }, format: :json - context 'with valid image' do - before do - post :create, params: { model: 'personal_snippet', id: snippet.id, file: jpg }, format: :json + expect(response).to have_gitlab_http_status(404) end - it 'returns a content with original filename, new link, and correct type.' do - expect(response.body).to match '\"alt\":\"rails_sample\"' - expect(response.body).to match "\"url\":\"/uploads" + context 'with valid image' do + before do + post :create, params: { model: 'personal_snippet', id: snippet.id, file: jpg }, format: :json + end + + it 'returns a content with original filename, new link, and correct type.' do + expect(response.body).to match '\"alt\":\"rails_sample\"' + expect(response.body).to match "\"url\":\"/uploads" + end + + it 'creates a corresponding Upload record' do + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq snippet + end + end end - it 'creates a corresponding Upload record' do - upload = Upload.last + context 'with valid non-image file' do + before do + post :create, params: { model: 'personal_snippet', id: snippet.id, file: txt }, format: :json + end - aggregate_failures do - expect(upload).to exist - expect(upload.model).to eq snippet + it 'returns a content with original filename, new link, and correct type.' do + expect(response.body).to match '\"alt\":\"doc_sample.txt\"' + expect(response.body).to match "\"url\":\"/uploads" + end + + it 'creates a corresponding Upload record' do + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq snippet + 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 - context 'with valid non-image file' do + expect(response).to have_gitlab_http_status(401) + end + + context 'when user is logged in' do before do - post :create, params: { model: 'personal_snippet', id: snippet.id, file: txt }, format: :json + sign_in(user) + end + + subject do + post :create, params: { model: model, id: user.id, file: jpg }, format: :json end it 'returns a content with original filename, new link, and correct type.' do - expect(response.body).to match '\"alt\":\"doc_sample.txt\"' - expect(response.body).to match "\"url\":\"/uploads" + subject + + expect(response.body).to match '\"alt\":\"rails_sample\"' + 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 snippet + expect(upload.model).to eq user end end - end - context 'temporal with valid image' do - subject do - post :create, params: { model: 'personal_snippet', file: jpg }, format: :json - end + context 'with valid non-image file' do + subject do + post :create, params: { model: model, id: user.id, file: txt }, format: :json + end - it 'returns a content with original filename, new link, and correct type.' do - subject + it 'returns a content with original filename, new link, and correct type.' do + subject - expect(response.body).to match '\"alt\":\"rails_sample\"' - expect(response.body).to match "\"url\":\"/uploads/-/system/temp" - end + expect(response.body).to match '\"alt\":\"doc_sample.txt\"' + expect(response.body).to match "\"url\":\"/uploads/-/system/user/#{user.id}/" + end - it 'does not create an Upload record' do - expect { subject }.not_to change { Upload.count } - end - end + it 'creates a corresponding Upload record' do + expect { subject }.to change { Upload.count } - context 'temporal with valid non-image file' do - subject do - post :create, params: { model: 'personal_snippet', file: txt }, format: :json + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq user + end + end end - it 'returns a content with original filename, new link, and correct type.' do - subject + it 'returns 404 when given user is not the logged in one' do + another_user = create(:user) - expect(response.body).to match '\"alt\":\"doc_sample.txt\"' - expect(response.body).to match "\"url\":\"/uploads/-/system/temp" - end + post :create, params: { model: model, id: another_user.id, file: txt }, format: :json - it 'does not create an Upload record' do - expect { subject }.not_to change { Upload.count } + expect(response).to have_gitlab_http_status(404) end end end diff --git a/spec/features/snippets/user_creates_snippet_spec.rb b/spec/features/snippets/user_creates_snippet_spec.rb index 1c97d5ec5b4..93d77d5b5ce 100644 --- a/spec/features/snippets/user_creates_snippet_spec.rb +++ b/spec/features/snippets/user_creates_snippet_spec.rb @@ -41,7 +41,7 @@ describe 'User creates snippet', :js 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/-/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) } expect(reqs.first.status_code).to eq(200) diff --git a/spec/routing/uploads_routing_spec.rb b/spec/routing/uploads_routing_spec.rb index 6a041ffdd6c..42e84774088 100644 --- a/spec/routing/uploads_routing_spec.rb +++ b/spec/routing/uploads_routing_spec.rb @@ -12,10 +12,19 @@ describe 'Uploads', 'routing' do ) 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 - UploadsController::MODEL_CLASSES.keys.compact.each do |model| - next if model == 'personal_snippet' + unroutable_models = UploadsController::MODEL_CLASSES.keys.compact - %w(personal_snippet user) + unroutable_models.each do |model| expect(post("/uploads/#{model}?id=1")).not_to be_routable end end diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index e474a714b10..d5e8a90cecd 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -3,20 +3,29 @@ require 'spec_helper' describe FileMover do include FileMoverHelpers + let(:user) { create(:user) } 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 "test ![banana_sample](/#{temp_file_path}) "\ "same ![banana_sample](/#{temp_file_path}) " 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) } - 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 before do + tmp_uploader.store!(file) + 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)) allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true) @@ -25,6 +34,8 @@ describe FileMover do stub_file_mover(temp_file_path) end + let(:tmp_upload) { tmp_uploader.upload } + context 'when move and field update successful' do it 'updates the description correctly' do subject @@ -36,8 +47,10 @@ describe FileMover do ) end - it 'creates a new update record' do - expect { subject }.to change { Upload.count }.by(1) + it 'updates existing upload record' do + expect { subject } + .to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') } + .from([user.id, 'User']).to([snippet.id, 'PersonalSnippet']) end it 'schedules a background migration' 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)) 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 subject expect(snippet.reload.description) .to eq( - "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "\ - "same ![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/user/#{user.id}/secret55/banana_sample.gif) " ) end - it 'does not create a new update record' do - expect { subject }.not_to change { Upload.count } + it 'does not change the upload record' do + expect { subject } + .not_to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') } end end end context 'security' 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 - 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) subject |