diff options
author | Regis Boudinot <boudinot.regis@yahoo.com> | 2017-06-08 20:06:09 +0000 |
---|---|---|
committer | Regis Boudinot <boudinot.regis@yahoo.com> | 2017-06-08 20:06:09 +0000 |
commit | b1bf6d88fceb24663bfe4be2d9cc111710d9126b (patch) | |
tree | 9275e78a1822df870183a85a2db83f18607595c0 /spec | |
parent | af16177707418ff9a4f96c0fee95b3788d153474 (diff) | |
parent | e0e5d097327c52e54a6e7433bbf0e350f15bf1f3 (diff) | |
download | gitlab-ce-b1bf6d88fceb24663bfe4be2d9cc111710d9126b.tar.gz |
Merge branch 'master-security-update' into 'master'
Master security update
See merge request !12025
Diffstat (limited to 'spec')
28 files changed, 748 insertions, 41 deletions
diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb index 2c9d1ffc9c2..4c3a5ec49ef 100644 --- a/spec/controllers/autocomplete_controller_spec.rb +++ b/spec/controllers/autocomplete_controller_spec.rb @@ -170,22 +170,32 @@ describe AutocompleteController do end context 'author of issuable included' do - before do - sign_in(user) - end - let(:body) { JSON.parse(response.body) } - it 'includes the author' do - get(:users, author_id: non_member.id) + context 'authenticated' do + before do + sign_in(user) + end + + it 'includes the author' do + get(:users, author_id: non_member.id) + + expect(body.first["username"]).to eq non_member.username + end + + it 'rejects non existent user ids' do + get(:users, author_id: 99999) - expect(body.first["username"]).to eq non_member.username + expect(body.collect { |u| u['id'] }).not_to include(99999) + end end - it 'rejects non existent user ids' do - get(:users, author_id: 99999) + context 'without authenticating' do + it 'returns empty result' do + get(:users, author_id: non_member.id) - expect(body.collect { |u| u['id'] }).not_to include(99999) + expect(body).to be_empty + end end end diff --git a/spec/factories/uploads.rb b/spec/factories/uploads.rb new file mode 100644 index 00000000000..1383420fb44 --- /dev/null +++ b/spec/factories/uploads.rb @@ -0,0 +1,8 @@ +FactoryGirl.define do + factory :upload do + model { build(:project) } + path { "uploads/system/project/avatar/avatar.jpg" } + size 100.kilobytes + uploader "AvatarUploader" + end +end diff --git a/spec/features/admin/admin_appearance_spec.rb b/spec/features/admin/admin_appearance_spec.rb index 96d715ef383..595366ce352 100644 --- a/spec/features/admin/admin_appearance_spec.rb +++ b/spec/features/admin/admin_appearance_spec.rb @@ -63,11 +63,11 @@ feature 'Admin Appearance', feature: true do end def logo_selector - '//img[@src^="/uploads/appearance/logo"]' + '//img[@src^="/uploads/system/appearance/logo"]' end def header_logo_selector - '//img[@src^="/uploads/appearance/header_logo"]' + '//img[@src^="/uploads/system/appearance/header_logo"]' end def logo_fixture diff --git a/spec/features/uploads/user_uploads_avatar_to_group_spec.rb b/spec/features/uploads/user_uploads_avatar_to_group_spec.rb index f88a515f7fc..d9d6f2e2382 100644 --- a/spec/features/uploads/user_uploads_avatar_to_group_spec.rb +++ b/spec/features/uploads/user_uploads_avatar_to_group_spec.rb @@ -18,7 +18,7 @@ feature 'User uploads avatar to group', feature: true do visit group_path(group) - expect(page).to have_selector(%Q(img[src$="/uploads/group/avatar/#{group.id}/dk.png"])) + expect(page).to have_selector(%Q(img[src$="/uploads/system/group/avatar/#{group.id}/dk.png"])) # Cheating here to verify something that isn't user-facing, but is important expect(group.reload.avatar.file).to exist diff --git a/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb b/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb index 0dfd29045e5..eb8dbd76aab 100644 --- a/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb +++ b/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb @@ -16,7 +16,7 @@ feature 'User uploads avatar to profile', feature: true do visit user_path(user) - expect(page).to have_selector(%Q(img[src$="/uploads/user/avatar/#{user.id}/dk.png"])) + expect(page).to have_selector(%Q(img[src$="/uploads/system/user/avatar/#{user.id}/dk.png"])) # Cheating here to verify something that isn't user-facing, but is important expect(user.reload.avatar.file).to exist diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 785fb724132..49df91b236f 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -1,3 +1,4 @@ +# coding: utf-8 require 'spec_helper' describe ApplicationHelper do @@ -58,13 +59,13 @@ describe ApplicationHelper do describe 'project_icon' do it 'returns an url for the avatar' do project = create(:empty_project, avatar: File.open(uploaded_image_temp_path)) - avatar_url = "/uploads/project/avatar/#{project.id}/banana_sample.gif" + avatar_url = "/uploads/system/project/avatar/#{project.id}/banana_sample.gif" expect(helper.project_icon(project.full_path).to_s). to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />" allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host) - avatar_url = "#{gitlab_host}/uploads/project/avatar/#{project.id}/banana_sample.gif" + avatar_url = "#{gitlab_host}/uploads/system/project/avatar/#{project.id}/banana_sample.gif" expect(helper.project_icon(project.full_path).to_s). to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />" @@ -84,12 +85,12 @@ describe ApplicationHelper do it 'returns an url for the avatar' do user = create(:user, avatar: File.open(uploaded_image_temp_path)) - avatar_url = "/uploads/user/avatar/#{user.id}/banana_sample.gif" + avatar_url = "/uploads/system/user/avatar/#{user.id}/banana_sample.gif" expect(helper.avatar_icon(user.email).to_s).to match(avatar_url) allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host) - avatar_url = "#{gitlab_host}/uploads/user/avatar/#{user.id}/banana_sample.gif" + avatar_url = "#{gitlab_host}/uploads/system/user/avatar/#{user.id}/banana_sample.gif" expect(helper.avatar_icon(user.email).to_s).to match(avatar_url) end @@ -102,7 +103,7 @@ describe ApplicationHelper do user = create(:user, avatar: File.open(uploaded_image_temp_path)) expect(helper.avatar_icon(user.email).to_s). - to match("/gitlab/uploads/user/avatar/#{user.id}/banana_sample.gif") + to match("/gitlab/uploads/system/user/avatar/#{user.id}/banana_sample.gif") end it 'calls gravatar_icon when no User exists with the given email' do @@ -116,7 +117,7 @@ describe ApplicationHelper do user = create(:user, avatar: File.open(uploaded_image_temp_path)) expect(helper.avatar_icon(user).to_s). - to match("/uploads/user/avatar/#{user.id}/banana_sample.gif") + to match("/uploads/system/user/avatar/#{user.id}/banana_sample.gif") end end end diff --git a/spec/helpers/emails_helper_spec.rb b/spec/helpers/emails_helper_spec.rb index cd112dbb2fb..c68e4f56b05 100644 --- a/spec/helpers/emails_helper_spec.rb +++ b/spec/helpers/emails_helper_spec.rb @@ -52,7 +52,7 @@ describe EmailsHelper do ) expect(header_logo).to eq( - %{<img style="height: 50px" src="/uploads/appearance/header_logo/#{appearance.id}/dk.png" alt="Dk" />} + %{<img style="height: 50px" src="/uploads/system/appearance/header_logo/#{appearance.id}/dk.png" alt="Dk" />} ) end end diff --git a/spec/helpers/groups_helper_spec.rb b/spec/helpers/groups_helper_spec.rb index c8b0d86425f..0337afa4452 100644 --- a/spec/helpers/groups_helper_spec.rb +++ b/spec/helpers/groups_helper_spec.rb @@ -9,7 +9,7 @@ describe GroupsHelper do group.avatar = fixture_file_upload(avatar_file_path) group.save! expect(group_icon(group.path).to_s). - to match("/uploads/group/avatar/#{group.id}/banana_sample.gif") + to match("/uploads/system/group/avatar/#{group.id}/banana_sample.gif") end it 'gives default avatar_icon when no avatar is present' do diff --git a/spec/helpers/page_layout_helper_spec.rb b/spec/helpers/page_layout_helper_spec.rb index 2cc0b40b2d0..dff2784f21f 100644 --- a/spec/helpers/page_layout_helper_spec.rb +++ b/spec/helpers/page_layout_helper_spec.rb @@ -60,7 +60,7 @@ describe PageLayoutHelper do %w(project user group).each do |type| context "with @#{type} assigned" do it "uses #{type.titlecase} avatar if available" do - object = double(avatar_url: 'http://example.com/uploads/avatar.png') + object = double(avatar_url: 'http://example.com/uploads/system/avatar.png') assign(type, object) expect(helper.page_image).to eq object.avatar_url diff --git a/spec/javascripts/notes_spec.js b/spec/javascripts/notes_spec.js index 24335614e09..bfd8b8648a6 100644 --- a/spec/javascripts/notes_spec.js +++ b/spec/javascripts/notes_spec.js @@ -461,6 +461,45 @@ import '~/notes'; }); }); + describe('update comment with script tags', () => { + const sampleComment = '<script></script>'; + const updatedComment = '<script></script>'; + const note = { + id: 1234, + html: `<li class="note note-row-1234 timeline-entry" id="note_1234"> + <div class="note-text">${sampleComment}</div> + </li>`, + note: sampleComment, + valid: true + }; + let $form; + let $notesContainer; + + beforeEach(() => { + this.notes = new Notes('', []); + window.gon.current_username = 'root'; + window.gon.current_user_fullname = 'Administrator'; + $form = $('form.js-main-target-form'); + $notesContainer = $('ul.main-notes-list'); + $form.find('textarea.js-note-text').html(sampleComment); + }); + + it('should not render a script tag', () => { + const deferred = $.Deferred(); + spyOn($, 'ajax').and.returnValue(deferred.promise()); + $('.js-comment-button').click(); + + deferred.resolve(note); + const $noteEl = $notesContainer.find(`#note_${note.id}`); + $noteEl.find('.js-note-edit').click(); + $noteEl.find('textarea.js-note-text').html(updatedComment); + $noteEl.find('.js-comment-save-button').click(); + + const $updatedNoteEl = $notesContainer.find(`#note_${note.id}`).find('.js-task-list-container'); + expect($updatedNoteEl.find('.note-text').text().trim()).toEqual(''); + }); + }); + describe('getFormData', () => { let $form; let sampleComment; diff --git a/spec/javascripts/vue_shared/components/commit_spec.js b/spec/javascripts/vue_shared/components/commit_spec.js index 050170a54e9..540245fe71e 100644 --- a/spec/javascripts/vue_shared/components/commit_spec.js +++ b/spec/javascripts/vue_shared/components/commit_spec.js @@ -22,7 +22,7 @@ describe('Commit component', () => { shortSha: 'b7836edd', title: 'Commit message', author: { - avatar_url: 'https://gitlab.com/uploads/user/avatar/300478/avatar.png', + avatar_url: 'https://gitlab.com/uploads/system/user/avatar/300478/avatar.png', web_url: 'https://gitlab.com/jschatz1', path: '/jschatz1', username: 'jschatz1', @@ -45,7 +45,7 @@ describe('Commit component', () => { shortSha: 'b7836edd', title: 'Commit message', author: { - avatar_url: 'https://gitlab.com/uploads/user/avatar/300478/avatar.png', + avatar_url: 'https://gitlab.com/uploads/system/user/avatar/300478/avatar.png', web_url: 'https://gitlab.com/jschatz1', path: '/jschatz1', username: 'jschatz1', diff --git a/spec/lib/banzai/reference_parser/base_parser_spec.rb b/spec/lib/banzai/reference_parser/base_parser_spec.rb index d5746107ee1..f4f42bfc3ed 100644 --- a/spec/lib/banzai/reference_parser/base_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/base_parser_spec.rb @@ -30,7 +30,7 @@ describe Banzai::ReferenceParser::BaseParser, lib: true do it 'checks if user can read the resource' do link['data-project'] = project.id.to_s - expect(subject).to receive(:can_read_reference?).with(user, project) + expect(subject).to receive(:can_read_reference?).with(user, project, link) subject.nodes_visible_to_user(user, [link]) end diff --git a/spec/lib/banzai/reference_parser/snippet_parser_spec.rb b/spec/lib/banzai/reference_parser/snippet_parser_spec.rb index d217a775802..620875ece20 100644 --- a/spec/lib/banzai/reference_parser/snippet_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/snippet_parser_spec.rb @@ -4,20 +4,199 @@ describe Banzai::ReferenceParser::SnippetParser, lib: true do include ReferenceParserHelpers let(:project) { create(:empty_project, :public) } + let(:user) { create(:user) } - let(:snippet) { create(:snippet, project: project) } + let(:external_user) { create(:user, :external) } + let(:project_member) { create(:user) } + subject { described_class.new(project, user) } let(:link) { empty_html_link } + def visible_references(snippet_visibility, user = nil) + snippet = create(:project_snippet, snippet_visibility, project: project) + link['data-project'] = project.id.to_s + link['data-snippet'] = snippet.id.to_s + + subject.nodes_visible_to_user(user, [link]) + end + + before do + project.add_user(project_member, :developer) + end + describe '#nodes_visible_to_user' do - context 'when the link has a data-issue attribute' do - before { link['data-snippet'] = snippet.id.to_s } + context 'when a project is public and the snippets feature is enabled for everyone' do + before do + project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::ENABLED) + end + + it 'creates a reference for guest for a public snippet' do + expect(visible_references(:public)).to eq([link]) + end + + it 'creates a reference for a regular user for a public snippet' do + expect(visible_references(:public, user)).to eq([link]) + end + + it 'creates a reference for a regular user for an internal snippet' do + expect(visible_references(:internal, user)).to eq([link]) + end + + it 'does not create a reference for an external user for an internal snippet' do + expect(visible_references(:internal, external_user)).to be_empty + end + + it 'creates a reference for a project member for a private snippet' do + expect(visible_references(:private, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for a private snippet' do + expect(visible_references(:private, user)).to be_empty + end + end + + context 'when a project is public and the snippets feature is enabled for project team members' do + before do + project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::PRIVATE) + end + + it 'creates a reference for a project member for a public snippet' do + expect(visible_references(:public, project_member)).to eq([link]) + end + + it 'does not create a reference for guest for a public snippet' do + expect(visible_references(:public, nil)).to be_empty + end + + it 'does not create a reference for a regular user for a public snippet' do + expect(visible_references(:public, user)).to be_empty + end + + it 'creates a reference for a project member for an internal snippet' do + expect(visible_references(:internal, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for an internal snippet' do + expect(visible_references(:internal, user)).to be_empty + end + + it 'creates a reference for a project member for a private snippet' do + expect(visible_references(:private, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for a private snippet' do + expect(visible_references(:private, user)).to be_empty + end + end + + context 'when a project is internal and the snippets feature is enabled for everyone' do + before do + project.update_attribute(:visibility, Gitlab::VisibilityLevel::INTERNAL) + project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::ENABLED) + end + + it 'does not create a reference for guest for a public snippet' do + expect(visible_references(:public)).to be_empty + end + + it 'does not create a reference for an external user for a public snippet' do + expect(visible_references(:public, external_user)).to be_empty + end - it_behaves_like "referenced feature visibility", "snippets" + it 'creates a reference for a regular user for a public snippet' do + expect(visible_references(:public, user)).to eq([link]) + end + + it 'creates a reference for a regular user for an internal snippet' do + expect(visible_references(:internal, user)).to eq([link]) + end + + it 'does not create a reference for an external user for an internal snippet' do + expect(visible_references(:internal, external_user)).to be_empty + end + + it 'creates a reference for a project member for a private snippet' do + expect(visible_references(:private, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for a private snippet' do + expect(visible_references(:private, user)).to be_empty + end + end + + context 'when a project is internal and the snippets feature is enabled for project team members' do + before do + project.update_attribute(:visibility, Gitlab::VisibilityLevel::INTERNAL) + project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::PRIVATE) + end + + it 'creates a reference for a project member for a public snippet' do + expect(visible_references(:public, project_member)).to eq([link]) + end + + it 'does not create a reference for guest for a public snippet' do + expect(visible_references(:public, nil)).to be_empty + end + + it 'does not create reference for a regular user for a public snippet' do + expect(visible_references(:public, user)).to be_empty + end + + it 'creates a reference for a project member for an internal snippet' do + expect(visible_references(:internal, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for an internal snippet' do + expect(visible_references(:internal, user)).to be_empty + end + + it 'creates a reference for a project member for a private snippet' do + expect(visible_references(:private, project_member)).to eq([link]) + end + + it 'does not create reference for a regular user for a private snippet' do + expect(visible_references(:private, user)).to be_empty + end + end + + context 'when a project is private and the snippets feature is enabled for project team members' do + before do + project.update_attribute(:visibility, Gitlab::VisibilityLevel::PRIVATE) + project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::PRIVATE) + end + + it 'creates a reference for a project member for a public snippet' do + expect(visible_references(:public, project_member)).to eq([link]) + end + + it 'does not create a reference for guest for a public snippet' do + expect(visible_references(:public, nil)).to be_empty + end + + it 'does not create a reference for a regular user for a public snippet' do + expect(visible_references(:public, user)).to be_empty + end + + it 'creates a reference for a project member for an internal snippet' do + expect(visible_references(:internal, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for an internal snippet' do + expect(visible_references(:internal, user)).to be_empty + end + + it 'creates a reference for a project member for a private snippet' do + expect(visible_references(:private, project_member)).to eq([link]) + end + + it 'does not create a reference for a regular user for a private snippet' do + expect(visible_references(:private, user)).to be_empty + end end end describe '#referenced_by' do + let(:snippet) { create(:snippet, project: project) } describe 'when the link has a data-snippet attribute' do context 'using an existing snippet ID' do it 'returns an Array of snippets' do @@ -31,7 +210,7 @@ describe Banzai::ReferenceParser::SnippetParser, lib: true do it 'returns an empty Array' do link['data-snippet'] = '' - expect(subject.referenced_by([link])).to eq([]) + expect(subject.referenced_by([link])).to be_empty end end end diff --git a/spec/lib/gitlab/uploads_transfer_spec.rb b/spec/lib/gitlab/uploads_transfer_spec.rb new file mode 100644 index 00000000000..109559bb01c --- /dev/null +++ b/spec/lib/gitlab/uploads_transfer_spec.rb @@ -0,0 +1,11 @@ +require 'spec_helper' + +describe Gitlab::UploadsTransfer do + it 'leaves avatar uploads where they are' do + project_with_avatar = create(:empty_project, :with_avatar) + + described_class.new.rename_namespace('project', 'project-renamed') + + expect(File.exist?(project_with_avatar.avatar.path)).to be_truthy + end +end diff --git a/spec/migrations/clean_upload_symlinks_spec.rb b/spec/migrations/clean_upload_symlinks_spec.rb new file mode 100644 index 00000000000..cecb3ddac53 --- /dev/null +++ b/spec/migrations/clean_upload_symlinks_spec.rb @@ -0,0 +1,46 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20170406111121_clean_upload_symlinks.rb') + +describe CleanUploadSymlinks do + let(:migration) { described_class.new } + let(:test_dir) { File.join(Rails.root, "tmp", "tests", "move_uploads_test") } + let(:uploads_dir) { File.join(test_dir, "public", "uploads") } + let(:new_uploads_dir) { File.join(uploads_dir, "system") } + let(:original_path) { File.join(new_uploads_dir, 'user') } + let(:symlink_path) { File.join(uploads_dir, 'user') } + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + allow(migration).to receive(:base_directory).and_return(test_dir) + allow(migration).to receive(:say) + end + + describe "#up" do + before do + FileUtils.mkdir_p(original_path) + FileUtils.ln_s(original_path, symlink_path) + end + + it 'removes the symlink' do + migration.up + + expect(File.symlink?(symlink_path)).to be(false) + end + end + + describe '#down' do + before do + FileUtils.mkdir_p(File.join(original_path)) + FileUtils.touch(File.join(original_path, 'dummy.file')) + end + + it 'creates a symlink' do + expected_path = File.join(symlink_path, "dummy.file") + migration.down + + expect(File.exist?(expected_path)).to be(true) + expect(File.symlink?(symlink_path)).to be(true) + end + end +end diff --git a/spec/migrations/move_uploads_to_system_dir_spec.rb b/spec/migrations/move_uploads_to_system_dir_spec.rb new file mode 100644 index 00000000000..37d66452447 --- /dev/null +++ b/spec/migrations/move_uploads_to_system_dir_spec.rb @@ -0,0 +1,68 @@ +require "spec_helper" +require Rails.root.join("db", "migrate", "20170316163845_move_uploads_to_system_dir.rb") + +describe MoveUploadsToSystemDir do + let(:migration) { described_class.new } + let(:test_dir) { File.join(Rails.root, "tmp", "move_uploads_test") } + let(:uploads_dir) { File.join(test_dir, "public", "uploads") } + let(:new_uploads_dir) { File.join(uploads_dir, "system") } + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + allow(migration).to receive(:base_directory).and_return(test_dir) + allow(migration).to receive(:say) + end + + describe "#up" do + before do + FileUtils.mkdir_p(File.join(uploads_dir, 'user')) + FileUtils.touch(File.join(uploads_dir, 'user', 'dummy.file')) + end + + it 'moves the directory to the new path' do + expected_path = File.join(new_uploads_dir, 'user', 'dummy.file') + + migration.up + + expect(File.exist?(expected_path)).to be(true) + end + + it 'creates a symlink in the old location' do + symlink_path = File.join(uploads_dir, 'user') + expected_path = File.join(symlink_path, 'dummy.file') + + migration.up + + expect(File.exist?(expected_path)).to be(true) + expect(File.symlink?(symlink_path)).to be(true) + end + end + + describe "#down" do + before do + FileUtils.mkdir_p(File.join(new_uploads_dir, 'user')) + FileUtils.touch(File.join(new_uploads_dir, 'user', 'dummy.file')) + end + + it 'moves the directory to the old path' do + expected_path = File.join(uploads_dir, 'user', 'dummy.file') + + migration.down + + expect(File.exist?(expected_path)).to be(true) + end + + it 'removes the symlink if it existed' do + FileUtils.ln_s(File.join(new_uploads_dir, 'user'), File.join(uploads_dir, 'user')) + + directory = File.join(uploads_dir, 'user') + expected_path = File.join(directory, 'dummy.file') + + migration.down + + expect(File.exist?(expected_path)).to be(true) + expect(File.symlink?(directory)).to be(false) + end + end +end diff --git a/spec/migrations/rename_system_namespaces_spec.rb b/spec/migrations/rename_system_namespaces_spec.rb new file mode 100644 index 00000000000..626a6005838 --- /dev/null +++ b/spec/migrations/rename_system_namespaces_spec.rb @@ -0,0 +1,254 @@ +require "spec_helper" +require Rails.root.join("db", "migrate", "20170316163800_rename_system_namespaces.rb") + +describe RenameSystemNamespaces, truncate: true do + let(:migration) { described_class.new } + let(:test_dir) { File.join(Rails.root, "tmp", "tests", "rename_namespaces_test") } + let(:uploads_dir) { File.join(test_dir, "public", "uploads") } + let(:system_namespace) do + namespace = build(:namespace, path: "system") + namespace.save(validate: false) + namespace + end + + def save_invalid_routable(routable) + routable.__send__(:prepare_route) + routable.save(validate: false) + end + + before do + FileUtils.remove_dir(test_dir) if File.directory?(test_dir) + FileUtils.mkdir_p(uploads_dir) + FileUtils.remove_dir(TestEnv.repos_path) if File.directory?(TestEnv.repos_path) + allow(migration).to receive(:say) + allow(migration).to receive(:uploads_dir).and_return(uploads_dir) + end + + describe "#system_namespace" do + it "only root namespaces called with path `system`" do + system_namespace + system_namespace_with_parent = build(:namespace, path: 'system', parent: create(:namespace)) + system_namespace_with_parent.save(validate: false) + + expect(migration.system_namespace.id).to eq(system_namespace.id) + end + end + + describe "#up" do + before do + system_namespace + end + + it "doesn't break if there are no namespaces called system" do + Namespace.delete_all + + migration.up + end + + it "renames namespaces called system" do + migration.up + + expect(system_namespace.reload.path).to eq("system0") + end + + it "renames the route to the namespace" do + migration.up + + expect(system_namespace.reload.full_path).to eq("system0") + end + + it "renames the route for projects of the namespace" do + project = build(:project, path: "project-path", namespace: system_namespace) + save_invalid_routable(project) + + migration.up + + expect(project.route.reload.path).to eq("system0/project-path") + end + + it "doesn't touch routes of namespaces that look like system" do + namespace = create(:group, path: 'systemlookalike') + project = create(:project, namespace: namespace, path: 'the-project') + + migration.up + + expect(project.route.reload.path).to eq('systemlookalike/the-project') + expect(namespace.route.reload.path).to eq('systemlookalike') + end + + it "moves the the repository for a project in the namespace" do + project = build(:project, namespace: system_namespace, path: "system-project") + save_invalid_routable(project) + TestEnv.copy_repo(project, + bare_repo: TestEnv.factory_repo_path_bare, + refs: TestEnv::BRANCH_SHA) + expected_repo = File.join(TestEnv.repos_path, "system0", "system-project.git") + + migration.up + + expect(File.directory?(expected_repo)).to be(true) + end + + it "moves the uploads for the namespace" do + allow(migration).to receive(:move_namespace_folders).with(Settings.pages.path, "system", "system0") + expect(migration).to receive(:move_namespace_folders).with(uploads_dir, "system", "system0") + + migration.up + end + + it "moves the pages for the namespace" do + allow(migration).to receive(:move_namespace_folders).with(uploads_dir, "system", "system0") + expect(migration).to receive(:move_namespace_folders).with(Settings.pages.path, "system", "system0") + + migration.up + end + + describe "clears the markdown cache for projects in the system namespace" do + let!(:project) do + project = build(:project, namespace: system_namespace) + save_invalid_routable(project) + project + end + + it 'removes description_html from projects' do + migration.up + + expect(project.reload.description_html).to be_nil + end + + it 'removes issue descriptions' do + issue = create(:issue, project: project, description_html: 'Issue description') + + migration.up + + expect(issue.reload.description_html).to be_nil + end + + it 'removes merge request descriptions' do + merge_request = create(:merge_request, + source_project: project, + target_project: project, + description_html: 'MergeRequest description') + + migration.up + + expect(merge_request.reload.description_html).to be_nil + end + + it 'removes note html' do + note = create(:note, + project: project, + noteable: create(:issue, project: project), + note_html: 'note description') + + migration.up + + expect(note.reload.note_html).to be_nil + end + + it 'removes milestone description' do + milestone = create(:milestone, + project: project, + description_html: 'milestone description') + + migration.up + + expect(milestone.reload.description_html).to be_nil + end + end + + context "system namespace -> subgroup -> system0 project" do + it "updates the route of the project correctly" do + subgroup = build(:group, path: "subgroup", parent: system_namespace) + save_invalid_routable(subgroup) + project = build(:project, path: "system0", namespace: subgroup) + save_invalid_routable(project) + + migration.up + + expect(project.route.reload.path).to eq("system0/subgroup/system0") + end + end + end + + describe "#move_repositories" do + let(:namespace) { create(:group, name: "hello-group") } + it "moves a project for a namespace" do + create(:project, namespace: namespace, path: "hello-project") + expected_path = File.join(TestEnv.repos_path, "bye-group", "hello-project.git") + + migration.move_repositories(namespace, "hello-group", "bye-group") + + expect(File.directory?(expected_path)).to be(true) + end + + it "moves a namespace in a subdirectory correctly" do + child_namespace = create(:group, name: "sub-group", parent: namespace) + create(:project, namespace: child_namespace, path: "hello-project") + + expected_path = File.join(TestEnv.repos_path, "hello-group", "renamed-sub-group", "hello-project.git") + + migration.move_repositories(child_namespace, "hello-group/sub-group", "hello-group/renamed-sub-group") + + expect(File.directory?(expected_path)).to be(true) + end + + it "moves a parent namespace with subdirectories" do + child_namespace = create(:group, name: "sub-group", parent: namespace) + create(:project, namespace: child_namespace, path: "hello-project") + expected_path = File.join(TestEnv.repos_path, "renamed-group", "sub-group", "hello-project.git") + + migration.move_repositories(child_namespace, "hello-group", "renamed-group") + + expect(File.directory?(expected_path)).to be(true) + end + end + + describe "#move_namespace_folders" do + it "moves a namespace with files" do + source = File.join(uploads_dir, "parent-group", "sub-group") + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, "parent-group", "moved-group") + FileUtils.touch(File.join(source, "test.txt")) + expected_file = File.join(destination, "test.txt") + + migration.move_namespace_folders(uploads_dir, File.join("parent-group", "sub-group"), File.join("parent-group", "moved-group")) + + expect(File.exist?(expected_file)).to be(true) + end + + it "moves a parent namespace uploads" do + source = File.join(uploads_dir, "parent-group", "sub-group") + FileUtils.mkdir_p(source) + destination = File.join(uploads_dir, "moved-parent", "sub-group") + FileUtils.touch(File.join(source, "test.txt")) + expected_file = File.join(destination, "test.txt") + + migration.move_namespace_folders(uploads_dir, "parent-group", "moved-parent") + + expect(File.exist?(expected_file)).to be(true) + end + end + + describe "#child_ids_for_parent" do + it "collects child ids for all levels" do + parent = create(:group) + first_child = create(:group, parent: parent) + second_child = create(:group, parent: parent) + third_child = create(:group, parent: second_child) + all_ids = [parent.id, first_child.id, second_child.id, third_child.id] + + collected_ids = migration.child_ids_for_parent(parent, ids: [parent.id]) + + expect(collected_ids).to contain_exactly(*all_ids) + end + end + + describe "#remove_last_ocurrence" do + it "removes only the last occurance of a string" do + input = "this/is/system/namespace/with/system" + + expect(migration.remove_last_occurrence(input, "system")).to eq("this/is/system/namespace/with/") + end + end +end diff --git a/spec/migrations/update_upload_paths_to_system_spec.rb b/spec/migrations/update_upload_paths_to_system_spec.rb new file mode 100644 index 00000000000..7df44515424 --- /dev/null +++ b/spec/migrations/update_upload_paths_to_system_spec.rb @@ -0,0 +1,53 @@ +require "spec_helper" +require Rails.root.join("db", "post_migrate", "20170317162059_update_upload_paths_to_system.rb") + +describe UpdateUploadPathsToSystem do + let(:migration) { described_class.new } + + before do + allow(migration).to receive(:say) + end + + describe "#uploads_to_switch_to_new_path" do + it "contains only uploads with the old path for the correct models" do + _upload_for_other_type = create(:upload, model: create(:ci_pipeline), path: "uploads/ci_pipeline/avatar.jpg") + _upload_with_system_path = create(:upload, model: create(:empty_project), path: "uploads/system/project/avatar.jpg") + _upload_with_other_path = create(:upload, model: create(:empty_project), path: "thelongsecretforafileupload/avatar.jpg") + old_upload = create(:upload, model: create(:empty_project), path: "uploads/project/avatar.jpg") + group_upload = create(:upload, model: create(:group), path: "uploads/group/avatar.jpg") + + expect(Upload.where(migration.uploads_to_switch_to_new_path)).to contain_exactly(old_upload, group_upload) + end + end + + describe "#uploads_to_switch_to_old_path" do + it "contains only uploads with the new path for the correct models" do + _upload_for_other_type = create(:upload, model: create(:ci_pipeline), path: "uploads/ci_pipeline/avatar.jpg") + upload_with_system_path = create(:upload, model: create(:empty_project), path: "uploads/system/project/avatar.jpg") + _upload_with_other_path = create(:upload, model: create(:empty_project), path: "thelongsecretforafileupload/avatar.jpg") + _old_upload = create(:upload, model: create(:empty_project), path: "uploads/project/avatar.jpg") + + expect(Upload.where(migration.uploads_to_switch_to_old_path)).to contain_exactly(upload_with_system_path) + end + end + + describe "#up", truncate: true do + it "updates old upload records to the new path" do + old_upload = create(:upload, model: create(:empty_project), path: "uploads/project/avatar.jpg") + + migration.up + + expect(old_upload.reload.path).to eq("uploads/system/project/avatar.jpg") + end + end + + describe "#down", truncate: true do + it "updates the new system patsh to the old paths" do + new_upload = create(:upload, model: create(:empty_project), path: "uploads/system/project/avatar.jpg") + + migration.down + + expect(new_upload.reload.path).to eq("uploads/project/avatar.jpg") + end + end +end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 316bf153660..3d437ca0fcc 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -179,7 +179,7 @@ describe Group, models: true do let!(:group) { create(:group, :access_requestable, :with_avatar) } let(:user) { create(:user) } let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" } - let(:avatar_path) { "/uploads/group/avatar/#{group.id}/dk.png" } + let(:avatar_path) { "/uploads/system/group/avatar/#{group.id}/dk.png" } context 'when avatar file is uploaded' do before { group.add_master(user) } diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 0e74f1ab1bd..145c7ad5770 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -43,6 +43,12 @@ describe Namespace, models: true do end end + context "is case insensitive" do + let(:group) { build(:group, path: "System") } + + it { expect(group).not_to be_valid } + end + context 'top-level group' do let(:group) { build(:group, path: 'tree') } @@ -178,8 +184,8 @@ describe Namespace, models: true do let(:parent) { create(:group, name: 'parent', path: 'parent') } let(:child) { create(:group, name: 'child', path: 'child', parent: parent) } let!(:project) { create(:project_empty_repo, path: 'the-project', namespace: child) } - let(:uploads_dir) { File.join(CarrierWave.root, 'uploads') } - let(:pages_dir) { TestEnv.pages_path } + let(:uploads_dir) { File.join(CarrierWave.root, FileUploader.base_dir) } + let(:pages_dir) { File.join(TestEnv.pages_path) } before do FileUtils.mkdir_p(File.join(uploads_dir, 'parent', 'child', 'the-project')) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 3ed52d42f86..454eeb58ecd 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -812,7 +812,7 @@ describe Project, models: true do context 'when avatar file is uploaded' do let(:project) { create(:empty_project, :with_avatar) } - let(:avatar_path) { "/uploads/project/avatar/#{project.id}/dk.png" } + let(:avatar_path) { "/uploads/system/project/avatar/#{project.id}/dk.png" } let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" } it 'shows correct url' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a83726b48a0..d5bd9946ab6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -987,7 +987,7 @@ describe User, models: true do context 'when avatar file is uploaded' do let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" } - let(:avatar_path) { "/uploads/user/avatar/#{user.id}/dk.png" } + let(:avatar_path) { "/uploads/system/user/avatar/#{user.id}/dk.png" } it 'shows correct avatar url' do expect(user.avatar_url).to eq(avatar_path) diff --git a/spec/policies/project_snippet_policy_spec.rb b/spec/policies/project_snippet_policy_spec.rb index e1771b636b8..ddbed5f781e 100644 --- a/spec/policies/project_snippet_policy_spec.rb +++ b/spec/policies/project_snippet_policy_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe ProjectSnippetPolicy, models: true do let(:regular_user) { create(:user) } let(:external_user) { create(:user, :external) } - let(:project) { create(:empty_project) } + let(:project) { create(:empty_project, :public) } let(:author_permissions) do [ @@ -107,7 +107,7 @@ describe ProjectSnippetPolicy, models: true do end context 'snippet author' do - let(:snippet) { create(:project_snippet, :private, author: regular_user) } + let(:snippet) { create(:project_snippet, :private, author: regular_user, project: project) } subject { described_class.abilities(regular_user, snippet).to_set } diff --git a/spec/requests/openid_connect_spec.rb b/spec/requests/openid_connect_spec.rb index 05176c3beaa..6d1f0b24196 100644 --- a/spec/requests/openid_connect_spec.rb +++ b/spec/requests/openid_connect_spec.rb @@ -79,7 +79,7 @@ describe 'OpenID Connect requests' do 'email_verified' => true, 'website' => 'https://example.com', 'profile' => 'http://localhost/alice', - 'picture' => "http://localhost/uploads/user/avatar/#{user.id}/dk.png" + 'picture' => "http://localhost/uploads/system/user/avatar/#{user.id}/dk.png" }) end end diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb index 0657b7e93fe..d75851134ee 100644 --- a/spec/services/projects/participants_service_spec.rb +++ b/spec/services/projects/participants_service_spec.rb @@ -13,7 +13,7 @@ describe Projects::ParticipantsService, services: true do groups = participants.groups expect(groups.size).to eq 1 - expect(groups.first[:avatar_url]).to eq("/uploads/group/avatar/#{group.id}/dk.png") + expect(groups.first[:avatar_url]).to eq("/uploads/system/group/avatar/#{group.id}/dk.png") end it 'should return an url for the avatar with relative url' do @@ -24,7 +24,7 @@ describe Projects::ParticipantsService, services: true do groups = participants.groups expect(groups.size).to eq 1 - expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/group/avatar/#{group.id}/dk.png") + expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/system/group/avatar/#{group.id}/dk.png") end end end diff --git a/spec/uploaders/attachment_uploader_spec.rb b/spec/uploaders/attachment_uploader_spec.rb index ea714fb08f0..d82dbe871d5 100644 --- a/spec/uploaders/attachment_uploader_spec.rb +++ b/spec/uploaders/attachment_uploader_spec.rb @@ -3,6 +3,17 @@ require 'spec_helper' describe AttachmentUploader do let(:uploader) { described_class.new(build_stubbed(:user)) } + describe "#store_dir" do + it "stores in the system dir" do + expect(uploader.store_dir).to start_with("uploads/system/user") + end + + it "uses the old path when using object storage" do + expect(described_class).to receive(:file_storage?).and_return(false) + expect(uploader.store_dir).to start_with("uploads/user") + end + end + describe '#move_to_cache' do it 'is true' do expect(uploader.move_to_cache).to eq(true) diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb index c4d558805ab..201fe6949aa 100644 --- a/spec/uploaders/avatar_uploader_spec.rb +++ b/spec/uploaders/avatar_uploader_spec.rb @@ -3,6 +3,17 @@ require 'spec_helper' describe AvatarUploader do let(:uploader) { described_class.new(build_stubbed(:user)) } + describe "#store_dir" do + it "stores in the system dir" do + expect(uploader.store_dir).to start_with("uploads/system/user") + end + + it "uses the old path when using object storage" do + expect(described_class).to receive(:file_storage?).and_return(false) + expect(uploader.store_dir).to start_with("uploads/user") + end + end + describe '#move_to_cache' do it 'is false' do expect(uploader.move_to_cache).to eq(false) diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index d9113ef4095..47e9365e13d 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -15,6 +15,16 @@ describe FileUploader do end end + describe "#store_dir" do + it "stores in the namespace path" do + project = build_stubbed(:empty_project) + uploader = described_class.new(project) + + expect(uploader.store_dir).to include(project.path_with_namespace) + expect(uploader.store_dir).not_to include("system") + end + end + describe 'initialize' do it 'generates a secret if none is provided' do expect(SecureRandom).to receive(:hex).and_return('secret') |