diff options
Diffstat (limited to 'spec/lib')
24 files changed, 995 insertions, 407 deletions
diff --git a/spec/lib/banzai/commit_renderer_spec.rb b/spec/lib/banzai/commit_renderer_spec.rb index 84adaebdcbe..e7ebb2a332f 100644 --- a/spec/lib/banzai/commit_renderer_spec.rb +++ b/spec/lib/banzai/commit_renderer_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Banzai::CommitRenderer do describe '.render' do it 'renders a commit description and title' do - user = double(:user) + user = build(:user) project = create(:project, :repository) expect(Banzai::ObjectRenderer).to receive(:new).with(project, user).and_call_original diff --git a/spec/lib/banzai/filter/issuable_state_filter_spec.rb b/spec/lib/banzai/filter/issuable_state_filter_spec.rb index cacb33d3372..17347768a49 100644 --- a/spec/lib/banzai/filter/issuable_state_filter_spec.rb +++ b/spec/lib/banzai/filter/issuable_state_filter_spec.rb @@ -77,6 +77,14 @@ describe Banzai::Filter::IssuableStateFilter do expect(doc.css('a').last.text).to eq("#{closed_issue.to_reference(other_project)} (closed)") end + it 'skips cross project references if the user cannot read cross project' do + expect(Ability).to receive(:allowed?).with(user, :read_cross_project) { false } + link = create_link(closed_issue.to_reference(other_project), issue: closed_issue.id, reference_type: 'issue') + doc = filter(link, context.merge(project: other_project)) + + expect(doc.css('a').last.text).to eq("#{closed_issue.to_reference(other_project)}") + end + it 'does not append state when filter is not enabled' do link = create_link('text', issue: closed_issue.id, reference_type: 'issue') context = { current_user: user } diff --git a/spec/lib/banzai/filter/redactor_filter_spec.rb b/spec/lib/banzai/filter/redactor_filter_spec.rb index 5a7858e77f3..9a2e521fdcf 100644 --- a/spec/lib/banzai/filter/redactor_filter_spec.rb +++ b/spec/lib/banzai/filter/redactor_filter_spec.rb @@ -6,7 +6,7 @@ describe Banzai::Filter::RedactorFilter do it 'ignores non-GFM links' do html = %(See <a href="https://google.com/">Google</a>) - doc = filter(html, current_user: double) + doc = filter(html, current_user: build(:user)) expect(doc.css('a').length).to eq 1 end diff --git a/spec/lib/banzai/redactor_spec.rb b/spec/lib/banzai/redactor_spec.rb index 2424c3fdc66..1fa89137972 100644 --- a/spec/lib/banzai/redactor_spec.rb +++ b/spec/lib/banzai/redactor_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Banzai::Redactor do - let(:user) { build(:user) } + let(:user) { create(:user) } let(:project) { build(:project) } let(:redactor) { described_class.new(project, user) } @@ -88,6 +88,55 @@ describe Banzai::Redactor do end end + context 'when the user cannot read cross project' do + include ActionView::Helpers::UrlHelper + let(:project) { create(:project) } + let(:other_project) { create(:project, :public) } + + def create_link(issuable) + type = issuable.class.name.underscore.downcase + link_to(issuable.to_reference, '', + class: 'gfm has-tooltip', + title: issuable.title, + data: { + reference_type: type, + "#{type}": issuable.id + }) + end + + before do + project.add_developer(user) + + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :read_cross_project, :global) { false } + allow(Ability).to receive(:allowed?).with(user, :read_cross_project) { false } + end + + it 'skips links to issues within the same project' do + issue = create(:issue, project: project) + link = create_link(issue) + doc = Nokogiri::HTML.fragment(link) + + redactor.redact([doc]) + result = doc.css('a').last + + expect(result['class']).to include('has-tooltip') + expect(result['title']).to eq(issue.title) + end + + it 'removes info from a cross project reference' do + issue = create(:issue, project: other_project) + link = create_link(issue) + doc = Nokogiri::HTML.fragment(link) + + redactor.redact([doc]) + result = doc.css('a').last + + expect(result['class']).not_to include('has-tooltip') + expect(result['title']).to be_empty + end + end + describe '#redact_nodes' do it 'redacts an Array of nodes' do doc = Nokogiri::HTML.fragment('<a href="foo">foo</a>') diff --git a/spec/lib/banzai/reference_parser/issue_parser_spec.rb b/spec/lib/banzai/reference_parser/issue_parser_spec.rb index 4cef3bdb24b..0a63567ee40 100644 --- a/spec/lib/banzai/reference_parser/issue_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/issue_parser_spec.rb @@ -19,19 +19,58 @@ describe Banzai::ReferenceParser::IssueParser do it 'returns the nodes when the user can read the issue' do expect(Ability).to receive(:issues_readable_by_user) - .with([issue], user) - .and_return([issue]) + .with([issue], user) + .and_return([issue]) expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) end it 'returns an empty Array when the user can not read the issue' do expect(Ability).to receive(:issues_readable_by_user) - .with([issue], user) - .and_return([]) + .with([issue], user) + .and_return([]) expect(subject.nodes_visible_to_user(user, [link])).to eq([]) end + + context 'when the user cannot read cross project' do + let(:issue) { create(:issue) } + + before do + allow(Ability).to receive(:allowed?).with(user, :read_cross_project) { false } + allow(Ability).to receive(:allowed?).with(user, :read_cross_project, :global) { false } + end + + it 'returns the nodes when the user can read the issue' do + expect(Ability).to receive(:allowed?) + .with(user, :read_issue_iid, issue) + .and_return(true) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) + end + + it 'returns an empty Array when the user can not read the issue' do + expect(Ability).to receive(:allowed?) + .with(user, :read_issue_iid, issue) + .and_return(false) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([]) + end + + context 'when the issue is not cross project' do + let(:issue) { create(:issue, project: project) } + + it 'does not check `can_read_reference` if the issue is not cross project' do + expect(Ability).to receive(:issues_readable_by_user) + .with([issue], user) + .and_return([]) + + expect(subject).not_to receive(:can_read_reference?).with(user, issue) + + expect(subject.nodes_visible_to_user(user, [link])).to eq([]) + end + end + end end context 'when the link does not have a data-issue attribute' do diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_dependencies/untracked_file_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_dependencies/untracked_file_spec.rb new file mode 100644 index 00000000000..c76adcbe2f5 --- /dev/null +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_dependencies/untracked_file_spec.rb @@ -0,0 +1,262 @@ +require 'spec_helper' + +# Rollback DB to 10.5 (later than this was originally written for) because it still needs to work. +describe Gitlab::BackgroundMigration::PopulateUntrackedUploadsDependencies::UntrackedFile, :migration, schema: 20180208183958 do + include MigrationsHelpers::TrackUntrackedUploadsHelpers + + let!(:appearances) { table(:appearances) } + let!(:namespaces) { table(:namespaces) } + let!(:projects) { table(:projects) } + let!(:routes) { table(:routes) } + let!(:uploads) { table(:uploads) } + + before(:all) do + ensure_temporary_tracking_table_exists + end + + describe '#upload_path' do + def assert_upload_path(file_path, expected_upload_path) + untracked_file = create_untracked_file(file_path) + + expect(untracked_file.upload_path).to eq(expected_upload_path) + end + + context 'for an appearance logo file path' do + it 'returns the file path relative to the CarrierWave root' do + assert_upload_path('/-/system/appearance/logo/1/some_logo.jpg', 'uploads/-/system/appearance/logo/1/some_logo.jpg') + end + end + + context 'for an appearance header_logo file path' do + it 'returns the file path relative to the CarrierWave root' do + assert_upload_path('/-/system/appearance/header_logo/1/some_logo.jpg', 'uploads/-/system/appearance/header_logo/1/some_logo.jpg') + end + end + + context 'for a pre-Markdown Note attachment file path' do + it 'returns the file path relative to the CarrierWave root' do + assert_upload_path('/-/system/note/attachment/1234/some_attachment.pdf', 'uploads/-/system/note/attachment/1234/some_attachment.pdf') + end + end + + context 'for a user avatar file path' do + it 'returns the file path relative to the CarrierWave root' do + assert_upload_path('/-/system/user/avatar/1234/avatar.jpg', 'uploads/-/system/user/avatar/1234/avatar.jpg') + end + end + + context 'for a group avatar file path' do + it 'returns the file path relative to the CarrierWave root' do + assert_upload_path('/-/system/group/avatar/1234/avatar.jpg', 'uploads/-/system/group/avatar/1234/avatar.jpg') + end + end + + context 'for a project avatar file path' do + it 'returns the file path relative to the CarrierWave root' do + assert_upload_path('/-/system/project/avatar/1234/avatar.jpg', 'uploads/-/system/project/avatar/1234/avatar.jpg') + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + it 'returns the file path relative to the project directory in uploads' do + project = create_project + random_hex = SecureRandom.hex + + assert_upload_path("/#{get_full_path(project)}/#{random_hex}/Some file.jpg", "#{random_hex}/Some file.jpg") + end + end + end + + describe '#uploader' do + def assert_uploader(file_path, expected_uploader) + untracked_file = create_untracked_file(file_path) + + expect(untracked_file.uploader).to eq(expected_uploader) + end + + context 'for an appearance logo file path' do + it 'returns AttachmentUploader as a string' do + assert_uploader('/-/system/appearance/logo/1/some_logo.jpg', 'AttachmentUploader') + end + end + + context 'for an appearance header_logo file path' do + it 'returns AttachmentUploader as a string' do + assert_uploader('/-/system/appearance/header_logo/1/some_logo.jpg', 'AttachmentUploader') + end + end + + context 'for a pre-Markdown Note attachment file path' do + it 'returns AttachmentUploader as a string' do + assert_uploader('/-/system/note/attachment/1234/some_attachment.pdf', 'AttachmentUploader') + end + end + + context 'for a user avatar file path' do + it 'returns AvatarUploader as a string' do + assert_uploader('/-/system/user/avatar/1234/avatar.jpg', 'AvatarUploader') + end + end + + context 'for a group avatar file path' do + it 'returns AvatarUploader as a string' do + assert_uploader('/-/system/group/avatar/1234/avatar.jpg', 'AvatarUploader') + end + end + + context 'for a project avatar file path' do + it 'returns AvatarUploader as a string' do + assert_uploader('/-/system/project/avatar/1234/avatar.jpg', 'AvatarUploader') + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + it 'returns FileUploader as a string' do + project = create_project + + assert_uploader("/#{get_full_path(project)}/#{SecureRandom.hex}/Some file.jpg", 'FileUploader') + end + end + end + + describe '#model_type' do + def assert_model_type(file_path, expected_model_type) + untracked_file = create_untracked_file(file_path) + + expect(untracked_file.model_type).to eq(expected_model_type) + end + + context 'for an appearance logo file path' do + it 'returns Appearance as a string' do + assert_model_type('/-/system/appearance/logo/1/some_logo.jpg', 'Appearance') + end + end + + context 'for an appearance header_logo file path' do + it 'returns Appearance as a string' do + assert_model_type('/-/system/appearance/header_logo/1/some_logo.jpg', 'Appearance') + end + end + + context 'for a pre-Markdown Note attachment file path' do + it 'returns Note as a string' do + assert_model_type('/-/system/note/attachment/1234/some_attachment.pdf', 'Note') + end + end + + context 'for a user avatar file path' do + it 'returns User as a string' do + assert_model_type('/-/system/user/avatar/1234/avatar.jpg', 'User') + end + end + + context 'for a group avatar file path' do + it 'returns Namespace as a string' do + assert_model_type('/-/system/group/avatar/1234/avatar.jpg', 'Namespace') + end + end + + context 'for a project avatar file path' do + it 'returns Project as a string' do + assert_model_type('/-/system/project/avatar/1234/avatar.jpg', 'Project') + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + it 'returns Project as a string' do + project = create_project + + assert_model_type("/#{get_full_path(project)}/#{SecureRandom.hex}/Some file.jpg", 'Project') + end + end + end + + describe '#model_id' do + def assert_model_id(file_path, expected_model_id) + untracked_file = create_untracked_file(file_path) + + expect(untracked_file.model_id).to eq(expected_model_id) + end + + context 'for an appearance logo file path' do + it 'returns the ID as a string' do + assert_model_id('/-/system/appearance/logo/1/some_logo.jpg', 1) + end + end + + context 'for an appearance header_logo file path' do + it 'returns the ID as a string' do + assert_model_id('/-/system/appearance/header_logo/1/some_logo.jpg', 1) + end + end + + context 'for a pre-Markdown Note attachment file path' do + it 'returns the ID as a string' do + assert_model_id('/-/system/note/attachment/1234/some_attachment.pdf', 1234) + end + end + + context 'for a user avatar file path' do + it 'returns the ID as a string' do + assert_model_id('/-/system/user/avatar/1234/avatar.jpg', 1234) + end + end + + context 'for a group avatar file path' do + it 'returns the ID as a string' do + assert_model_id('/-/system/group/avatar/1234/avatar.jpg', 1234) + end + end + + context 'for a project avatar file path' do + it 'returns the ID as a string' do + assert_model_id('/-/system/project/avatar/1234/avatar.jpg', 1234) + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + it 'returns the ID as a string' do + project = create_project + + assert_model_id("/#{get_full_path(project)}/#{SecureRandom.hex}/Some file.jpg", project.id) + end + end + end + + describe '#file_size' do + context 'for an appearance logo file path' do + let(:appearance) { create_or_update_appearance(logo: true) } + let(:untracked_file) { described_class.create!(path: get_uploads(appearance, 'Appearance').first.path) } + + it 'returns the file size' do + expect(untracked_file.file_size).to eq(1062) + end + end + + context 'for a project avatar file path' do + let(:project) { create_project(avatar: true) } + let(:untracked_file) { described_class.create!(path: get_uploads(project, 'Project').first.path) } + + it 'returns the file size' do + expect(untracked_file.file_size).to eq(1062) + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + let(:project) { create_project } + let(:untracked_file) { create_untracked_file("/#{get_full_path(project)}/#{get_uploads(project, 'Project').first.path}") } + + before do + add_markdown_attachment(project) + end + + it 'returns the file size' do + expect(untracked_file.file_size).to eq(1062) + end + end + end + + def create_untracked_file(path_relative_to_upload_dir) + described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}#{path_relative_to_upload_dir}") + end +end diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index c8fa252439a..0d2074eed22 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -1,17 +1,19 @@ require 'spec_helper' -# This migration is using UploadService, which sets uploads.secret that is only -# added to the DB schema in 20180129193323. Since the test isn't isolated, we -# just use the latest schema when testing this migration. -# Ideally, the test should not use factories nor UploadService, and rely on the -# `table` helper instead. -describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migration, schema: 20180129193323 do - include TrackUntrackedUploadsHelpers +# Rollback DB to 10.5 (later than this was originally written for) because it still needs to work. +describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migration, schema: 20180208183958 do + include MigrationsHelpers::TrackUntrackedUploadsHelpers subject { described_class.new } - let!(:untracked_files_for_uploads) { described_class::UntrackedFile } - let!(:uploads) { described_class::Upload } + let!(:appearances) { table(:appearances) } + let!(:namespaces) { table(:namespaces) } + let!(:notes) { table(:notes) } + let!(:projects) { table(:projects) } + let!(:routes) { table(:routes) } + let!(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } + let!(:uploads) { table(:uploads) } + let!(:users) { table(:users) } before do ensure_temporary_tracking_table_exists @@ -19,30 +21,30 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra end context 'with untracked files and tracked files in untracked_files_for_uploads' do - let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) } - let!(:user1) { create(:user, :with_avatar) } - let!(:user2) { create(:user, :with_avatar) } - let!(:project1) { create(:project, :legacy_storage, :with_avatar) } - let!(:project2) { create(:project, :legacy_storage, :with_avatar) } + let!(:appearance) { create_or_update_appearance(logo: true, header_logo: true) } + let!(:user1) { create_user(avatar: true) } + let!(:user2) { create_user(avatar: true) } + let!(:project1) { create_project(avatar: true) } + let!(:project2) { create_project(avatar: true) } before do - UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload - UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload + add_markdown_attachment(project1) + add_markdown_attachment(project2) # File records created by PrepareUntrackedUploads - untracked_files_for_uploads.create!(path: appearance.uploads.first.path) - untracked_files_for_uploads.create!(path: appearance.uploads.last.path) - untracked_files_for_uploads.create!(path: user1.uploads.first.path) - untracked_files_for_uploads.create!(path: user2.uploads.first.path) - untracked_files_for_uploads.create!(path: project1.uploads.first.path) - untracked_files_for_uploads.create!(path: project2.uploads.first.path) - untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project1.full_path}/#{project1.uploads.last.path}") - untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project2.full_path}/#{project2.uploads.last.path}") + untracked_files_for_uploads.create!(path: get_uploads(appearance, 'Appearance').first.path) + untracked_files_for_uploads.create!(path: get_uploads(appearance, 'Appearance').last.path) + untracked_files_for_uploads.create!(path: get_uploads(user1, 'User').first.path) + untracked_files_for_uploads.create!(path: get_uploads(user2, 'User').first.path) + untracked_files_for_uploads.create!(path: get_uploads(project1, 'Project').first.path) + untracked_files_for_uploads.create!(path: get_uploads(project2, 'Project').first.path) + untracked_files_for_uploads.create!(path: "#{legacy_project_uploads_dir(project1).sub("#{MigrationsHelpers::TrackUntrackedUploadsHelpers::PUBLIC_DIR}/", '')}/#{get_uploads(project1, 'Project').last.path}") + untracked_files_for_uploads.create!(path: "#{legacy_project_uploads_dir(project2).sub("#{MigrationsHelpers::TrackUntrackedUploadsHelpers::PUBLIC_DIR}/", '')}/#{get_uploads(project2, 'Project').last.path}") # Untrack 4 files - user2.uploads.delete_all - project2.uploads.delete_all # 2 files: avatar and a Markdown upload - appearance.uploads.where("path like '%header_logo%'").delete_all + get_uploads(user2, 'User').delete_all + get_uploads(project2, 'Project').delete_all # 2 files: avatar and a Markdown upload + get_uploads(appearance, 'Appearance').where("path like '%header_logo%'").delete_all end it 'adds untracked files to the uploads table' do @@ -50,9 +52,9 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra subject.perform(1, untracked_files_for_uploads.reorder(:id).last.id) end.to change { uploads.count }.from(4).to(8) - expect(user2.uploads.count).to eq(1) - expect(project2.uploads.count).to eq(2) - expect(appearance.uploads.count).to eq(2) + expect(get_uploads(user2, 'User').count).to eq(1) + expect(get_uploads(project2, 'Project').count).to eq(2) + expect(get_uploads(appearance, 'Appearance').count).to eq(2) end it 'deletes rows after processing them' do @@ -66,9 +68,9 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra it 'does not create duplicate uploads of already tracked files' do subject.perform(1, untracked_files_for_uploads.last.id) - expect(user1.uploads.count).to eq(1) - expect(project1.uploads.count).to eq(2) - expect(appearance.uploads.count).to eq(2) + expect(get_uploads(user1, 'User').count).to eq(1) + expect(get_uploads(project1, 'Project').count).to eq(2) + expect(get_uploads(appearance, 'Appearance').count).to eq(2) end it 'uses the start and end batch ids [only 1st half]' do @@ -80,11 +82,11 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra subject.perform(start_id, end_id) end.to change { uploads.count }.from(4).to(6) - expect(user1.uploads.count).to eq(1) - expect(user2.uploads.count).to eq(1) - expect(appearance.uploads.count).to eq(2) - expect(project1.uploads.count).to eq(2) - expect(project2.uploads.count).to eq(0) + expect(get_uploads(user1, 'User').count).to eq(1) + expect(get_uploads(user2, 'User').count).to eq(1) + expect(get_uploads(appearance, 'Appearance').count).to eq(2) + expect(get_uploads(project1, 'Project').count).to eq(2) + expect(get_uploads(project2, 'Project').count).to eq(0) # Only 4 have been either confirmed or added to uploads expect(untracked_files_for_uploads.count).to eq(4) @@ -99,11 +101,11 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra subject.perform(start_id, end_id) end.to change { uploads.count }.from(4).to(6) - expect(user1.uploads.count).to eq(1) - expect(user2.uploads.count).to eq(0) - expect(appearance.uploads.count).to eq(1) - expect(project1.uploads.count).to eq(2) - expect(project2.uploads.count).to eq(2) + expect(get_uploads(user1, 'User').count).to eq(1) + expect(get_uploads(user2, 'User').count).to eq(0) + expect(get_uploads(appearance, 'Appearance').count).to eq(1) + expect(get_uploads(project1, 'Project').count).to eq(2) + expect(get_uploads(project2, 'Project').count).to eq(2) # Only 4 have been either confirmed or added to uploads expect(untracked_files_for_uploads.count).to eq(4) @@ -122,7 +124,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra end it 'does not block a whole batch because of one bad path' do - untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project2.full_path}/._7d37bf4c747916390e596744117d5d1a") + untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{get_full_path(project2)}/._7d37bf4c747916390e596744117d5d1a") expect(untracked_files_for_uploads.count).to eq(9) expect(uploads.count).to eq(4) @@ -133,7 +135,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra end it 'an unparseable path is shown in error output' do - bad_path = "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project2.full_path}/._7d37bf4c747916390e596744117d5d1a" + bad_path = "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{get_full_path(project2)}/._7d37bf4c747916390e596744117d5d1a" untracked_files_for_uploads.create!(path: bad_path) expect(Rails.logger).to receive(:error).with(/Error parsing path "#{bad_path}":/) @@ -152,363 +154,100 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra describe 'upload outcomes for each path pattern' do shared_examples_for 'non_markdown_file' do - let!(:expected_upload_attrs) { model.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') } + let!(:expected_upload_attrs) { model_uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') } let!(:untracked_file) { untracked_files_for_uploads.create!(path: expected_upload_attrs['path']) } before do - model.uploads.delete_all + model_uploads.delete_all end it 'creates an Upload record' do expect do subject.perform(1, untracked_files_for_uploads.last.id) - end.to change { model.reload.uploads.count }.from(0).to(1) + end.to change { model_uploads.count }.from(0).to(1) - expect(model.uploads.first.attributes).to include(expected_upload_attrs) + expect(model_uploads.first.attributes).to include(expected_upload_attrs) end end context 'for an appearance logo file path' do - let(:model) { create_or_update_appearance(logo: uploaded_file) } + let(:model) { create_or_update_appearance(logo: true) } + let(:model_uploads) { get_uploads(model, 'Appearance') } it_behaves_like 'non_markdown_file' end context 'for an appearance header_logo file path' do - let(:model) { create_or_update_appearance(header_logo: uploaded_file) } + let(:model) { create_or_update_appearance(header_logo: true) } + let(:model_uploads) { get_uploads(model, 'Appearance') } it_behaves_like 'non_markdown_file' end context 'for a pre-Markdown Note attachment file path' do - let(:model) { create(:note, :with_attachment) } - let!(:expected_upload_attrs) { Upload.where(model_type: 'Note', model_id: model.id).first.attributes.slice('path', 'uploader', 'size', 'checksum') } + let(:model) { create_note(attachment: true) } + let!(:expected_upload_attrs) { get_uploads(model, 'Note').first.attributes.slice('path', 'uploader', 'size', 'checksum') } let!(:untracked_file) { untracked_files_for_uploads.create!(path: expected_upload_attrs['path']) } before do - Upload.where(model_type: 'Note', model_id: model.id).delete_all + get_uploads(model, 'Note').delete_all end # Can't use the shared example because Note doesn't have an `uploads` association it 'creates an Upload record' do expect do subject.perform(1, untracked_files_for_uploads.last.id) - end.to change { Upload.where(model_type: 'Note', model_id: model.id).count }.from(0).to(1) + end.to change { get_uploads(model, 'Note').count }.from(0).to(1) - expect(Upload.where(model_type: 'Note', model_id: model.id).first.attributes).to include(expected_upload_attrs) + expect(get_uploads(model, 'Note').first.attributes).to include(expected_upload_attrs) end end context 'for a user avatar file path' do - let(:model) { create(:user, :with_avatar) } + let(:model) { create_user(avatar: true) } + let(:model_uploads) { get_uploads(model, 'User') } it_behaves_like 'non_markdown_file' end context 'for a group avatar file path' do - let(:model) { create(:group, :with_avatar) } + let(:model) { create_group(avatar: true) } + let(:model_uploads) { get_uploads(model, 'Namespace') } it_behaves_like 'non_markdown_file' end context 'for a project avatar file path' do - let(:model) { create(:project, :legacy_storage, :with_avatar) } + let(:model) { create_project(avatar: true) } + let(:model_uploads) { get_uploads(model, 'Project') } it_behaves_like 'non_markdown_file' end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - let(:model) { create(:project, :legacy_storage) } + let(:model) { create_project } before do # Upload the file - UploadService.new(model, uploaded_file, FileUploader).execute + add_markdown_attachment(model) # Create the untracked_files_for_uploads record - untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{model.full_path}/#{model.uploads.first.path}") + untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{get_full_path(model)}/#{get_uploads(model, 'Project').first.path}") # Save the expected upload attributes - @expected_upload_attrs = model.reload.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') + @expected_upload_attrs = get_uploads(model, 'Project').first.attributes.slice('path', 'uploader', 'size', 'checksum') # Untrack the file - model.reload.uploads.delete_all + get_uploads(model, 'Project').delete_all end it 'creates an Upload record' do expect do subject.perform(1, untracked_files_for_uploads.last.id) - end.to change { model.reload.uploads.count }.from(0).to(1) + end.to change { get_uploads(model, 'Project').count }.from(0).to(1) - expect(model.uploads.first.attributes).to include(@expected_upload_attrs) + expect(get_uploads(model, 'Project').first.attributes).to include(@expected_upload_attrs) end end end end - -describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do - include TrackUntrackedUploadsHelpers - - let(:upload_class) { Gitlab::BackgroundMigration::PopulateUntrackedUploads::Upload } - - before(:all) do - ensure_temporary_tracking_table_exists - end - - describe '#upload_path' do - def assert_upload_path(file_path, expected_upload_path) - untracked_file = create_untracked_file(file_path) - - expect(untracked_file.upload_path).to eq(expected_upload_path) - end - - context 'for an appearance logo file path' do - it 'returns the file path relative to the CarrierWave root' do - assert_upload_path('/-/system/appearance/logo/1/some_logo.jpg', 'uploads/-/system/appearance/logo/1/some_logo.jpg') - end - end - - context 'for an appearance header_logo file path' do - it 'returns the file path relative to the CarrierWave root' do - assert_upload_path('/-/system/appearance/header_logo/1/some_logo.jpg', 'uploads/-/system/appearance/header_logo/1/some_logo.jpg') - end - end - - context 'for a pre-Markdown Note attachment file path' do - it 'returns the file path relative to the CarrierWave root' do - assert_upload_path('/-/system/note/attachment/1234/some_attachment.pdf', 'uploads/-/system/note/attachment/1234/some_attachment.pdf') - end - end - - context 'for a user avatar file path' do - it 'returns the file path relative to the CarrierWave root' do - assert_upload_path('/-/system/user/avatar/1234/avatar.jpg', 'uploads/-/system/user/avatar/1234/avatar.jpg') - end - end - - context 'for a group avatar file path' do - it 'returns the file path relative to the CarrierWave root' do - assert_upload_path('/-/system/group/avatar/1234/avatar.jpg', 'uploads/-/system/group/avatar/1234/avatar.jpg') - end - end - - context 'for a project avatar file path' do - it 'returns the file path relative to the CarrierWave root' do - assert_upload_path('/-/system/project/avatar/1234/avatar.jpg', 'uploads/-/system/project/avatar/1234/avatar.jpg') - end - end - - context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - it 'returns the file path relative to the project directory in uploads' do - project = create(:project, :legacy_storage) - random_hex = SecureRandom.hex - - assert_upload_path("/#{project.full_path}/#{random_hex}/Some file.jpg", "#{random_hex}/Some file.jpg") - end - end - end - - describe '#uploader' do - def assert_uploader(file_path, expected_uploader) - untracked_file = create_untracked_file(file_path) - - expect(untracked_file.uploader).to eq(expected_uploader) - end - - context 'for an appearance logo file path' do - it 'returns AttachmentUploader as a string' do - assert_uploader('/-/system/appearance/logo/1/some_logo.jpg', 'AttachmentUploader') - end - end - - context 'for an appearance header_logo file path' do - it 'returns AttachmentUploader as a string' do - assert_uploader('/-/system/appearance/header_logo/1/some_logo.jpg', 'AttachmentUploader') - end - end - - context 'for a pre-Markdown Note attachment file path' do - it 'returns AttachmentUploader as a string' do - assert_uploader('/-/system/note/attachment/1234/some_attachment.pdf', 'AttachmentUploader') - end - end - - context 'for a user avatar file path' do - it 'returns AvatarUploader as a string' do - assert_uploader('/-/system/user/avatar/1234/avatar.jpg', 'AvatarUploader') - end - end - - context 'for a group avatar file path' do - it 'returns AvatarUploader as a string' do - assert_uploader('/-/system/group/avatar/1234/avatar.jpg', 'AvatarUploader') - end - end - - context 'for a project avatar file path' do - it 'returns AvatarUploader as a string' do - assert_uploader('/-/system/project/avatar/1234/avatar.jpg', 'AvatarUploader') - end - end - - context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - it 'returns FileUploader as a string' do - project = create(:project, :legacy_storage) - - assert_uploader("/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg", 'FileUploader') - end - end - end - - describe '#model_type' do - def assert_model_type(file_path, expected_model_type) - untracked_file = create_untracked_file(file_path) - - expect(untracked_file.model_type).to eq(expected_model_type) - end - - context 'for an appearance logo file path' do - it 'returns Appearance as a string' do - assert_model_type('/-/system/appearance/logo/1/some_logo.jpg', 'Appearance') - end - end - - context 'for an appearance header_logo file path' do - it 'returns Appearance as a string' do - assert_model_type('/-/system/appearance/header_logo/1/some_logo.jpg', 'Appearance') - end - end - - context 'for a pre-Markdown Note attachment file path' do - it 'returns Note as a string' do - assert_model_type('/-/system/note/attachment/1234/some_attachment.pdf', 'Note') - end - end - - context 'for a user avatar file path' do - it 'returns User as a string' do - assert_model_type('/-/system/user/avatar/1234/avatar.jpg', 'User') - end - end - - context 'for a group avatar file path' do - it 'returns Namespace as a string' do - assert_model_type('/-/system/group/avatar/1234/avatar.jpg', 'Namespace') - end - end - - context 'for a project avatar file path' do - it 'returns Project as a string' do - assert_model_type('/-/system/project/avatar/1234/avatar.jpg', 'Project') - end - end - - context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - it 'returns Project as a string' do - project = create(:project, :legacy_storage) - - assert_model_type("/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg", 'Project') - end - end - end - - describe '#model_id' do - def assert_model_id(file_path, expected_model_id) - untracked_file = create_untracked_file(file_path) - - expect(untracked_file.model_id).to eq(expected_model_id) - end - - context 'for an appearance logo file path' do - it 'returns the ID as a string' do - assert_model_id('/-/system/appearance/logo/1/some_logo.jpg', 1) - end - end - - context 'for an appearance header_logo file path' do - it 'returns the ID as a string' do - assert_model_id('/-/system/appearance/header_logo/1/some_logo.jpg', 1) - end - end - - context 'for a pre-Markdown Note attachment file path' do - it 'returns the ID as a string' do - assert_model_id('/-/system/note/attachment/1234/some_attachment.pdf', 1234) - end - end - - context 'for a user avatar file path' do - it 'returns the ID as a string' do - assert_model_id('/-/system/user/avatar/1234/avatar.jpg', 1234) - end - end - - context 'for a group avatar file path' do - it 'returns the ID as a string' do - assert_model_id('/-/system/group/avatar/1234/avatar.jpg', 1234) - end - end - - context 'for a project avatar file path' do - it 'returns the ID as a string' do - assert_model_id('/-/system/project/avatar/1234/avatar.jpg', 1234) - end - end - - context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - it 'returns the ID as a string' do - project = create(:project, :legacy_storage) - - assert_model_id("/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg", project.id) - end - end - end - - describe '#file_size' do - context 'for an appearance logo file path' do - let(:appearance) { create_or_update_appearance(logo: uploaded_file) } - let(:untracked_file) { described_class.create!(path: appearance.uploads.first.path) } - - it 'returns the file size' do - expect(untracked_file.file_size).to eq(35255) - end - - it 'returns the same thing that CarrierWave would return' do - expect(untracked_file.file_size).to eq(appearance.logo.size) - end - end - - context 'for a project avatar file path' do - let(:project) { create(:project, :legacy_storage, avatar: uploaded_file) } - let(:untracked_file) { described_class.create!(path: project.uploads.first.path) } - - it 'returns the file size' do - expect(untracked_file.file_size).to eq(35255) - end - - it 'returns the same thing that CarrierWave would return' do - expect(untracked_file.file_size).to eq(project.avatar.size) - end - end - - context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - let(:project) { create(:project, :legacy_storage) } - let(:untracked_file) { create_untracked_file("/#{project.full_path}/#{project.uploads.first.path}") } - - before do - UploadService.new(project, uploaded_file, FileUploader).execute - end - - it 'returns the file size' do - expect(untracked_file.file_size).to eq(35255) - end - - it 'returns the same thing that CarrierWave would return' do - expect(untracked_file.file_size).to eq(project.uploads.first.size) - end - end - end - - def create_untracked_file(path_relative_to_upload_dir) - described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}#{path_relative_to_upload_dir}") - end -end diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index ca77e64ae40..35750d89c35 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -1,10 +1,16 @@ require 'spec_helper' -describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq, :migration, schema: 20180129193323 do - include TrackUntrackedUploadsHelpers - include MigrationsHelpers - - let!(:untracked_files_for_uploads) { described_class::UntrackedFile } +# Rollback DB to 10.5 (later than this was originally written for) because it still needs to work. +describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq, :migration, schema: 20180208183958 do + include MigrationsHelpers::TrackUntrackedUploadsHelpers + + let!(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } + let!(:appearances) { table(:appearances) } + let!(:namespaces) { table(:namespaces) } + let!(:projects) { table(:projects) } + let!(:routes) { table(:routes) } + let!(:uploads) { table(:uploads) } + let!(:users) { table(:users) } around do |example| # Especially important so the follow-up migration does not get run @@ -15,19 +21,17 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq, :migrat shared_examples 'prepares the untracked_files_for_uploads table' do context 'when files were uploaded before and after hashed storage was enabled' do - let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) } - let!(:user) { create(:user, :with_avatar) } - let!(:project1) { create(:project, :with_avatar, :legacy_storage) } - let(:project2) { create(:project) } # instantiate after enabling hashed_storage + let!(:appearance) { create_or_update_appearance(logo: true, header_logo: true) } + let!(:user) { create_user(avatar: true) } + let!(:project1) { create_project(avatar: true) } + let(:project2) { create_project } # instantiate after enabling hashed_storage before do # Markdown upload before enabling hashed_storage - UploadService.new(project1, uploaded_file, FileUploader).execute - - stub_application_setting(hashed_storage_enabled: true) + add_markdown_attachment(project1) # Markdown upload after enabling hashed_storage - UploadService.new(project2, uploaded_file, FileUploader).execute + add_markdown_attachment(project2, hashed_storage: true) end it 'has a path field long enough for really long paths' do @@ -61,7 +65,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq, :migrat it 'does not add hashed files to the untracked_files_for_uploads table' do described_class.new.perform - hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path + hashed_file_path = get_uploads(project2, 'Project').where(uploader: 'FileUploader').first.path expect(untracked_files_for_uploads.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey end diff --git a/spec/lib/gitlab/contributions_calendar_spec.rb b/spec/lib/gitlab/contributions_calendar_spec.rb index f1655854486..49a179ba875 100644 --- a/spec/lib/gitlab/contributions_calendar_spec.rb +++ b/spec/lib/gitlab/contributions_calendar_spec.rb @@ -118,6 +118,19 @@ describe Gitlab::ContributionsCalendar do expect(calendar.events_by_date(today)).to contain_exactly(e1) expect(calendar(contributor).events_by_date(today)).to contain_exactly(e1, e2, e3) end + + context 'when the user cannot read read cross project' do + before do + allow(Ability).to receive(:allowed?).and_call_original + expect(Ability).to receive(:allowed?).with(user, :read_cross_project) { false } + end + + it 'does not return any events' do + create_event(public_project, today) + + expect(calendar(user).events_by_date(today)).to be_empty + end + end end describe '#starting_year' do diff --git a/spec/lib/gitlab/cross_project_access/check_collection_spec.rb b/spec/lib/gitlab/cross_project_access/check_collection_spec.rb new file mode 100644 index 00000000000..a9e7575240e --- /dev/null +++ b/spec/lib/gitlab/cross_project_access/check_collection_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +describe Gitlab::CrossProjectAccess::CheckCollection do + subject(:collection) { described_class.new } + + describe '#add_collection' do + it 'merges the checks of 2 collections' do + initial_check = double('check') + collection.add_check(initial_check) + + other_collection = described_class.new + other_check = double('other_check') + other_collection.add_check(other_check) + + shared_check = double('shared check') + other_collection.add_check(shared_check) + collection.add_check(shared_check) + + collection.add_collection(other_collection) + + expect(collection.checks).to contain_exactly(initial_check, shared_check, other_check) + end + end + + describe '#should_run?' do + def fake_check(run, skip) + check = double("Check: run=#{run} - skip={skip}") + allow(check).to receive(:should_run?).and_return(run) + allow(check).to receive(:should_skip?).and_return(skip) + allow(check).to receive(:skip).and_return(skip) + + check + end + + it 'returns true if one of the check says it should run' do + check = fake_check(true, false) + other_check = fake_check(false, false) + + collection.add_check(check) + collection.add_check(other_check) + + expect(collection.should_run?(double)).to be_truthy + end + + it 'returns false if one of the check says it should be skipped' do + check = fake_check(true, false) + other_check = fake_check(false, true) + + collection.add_check(check) + collection.add_check(other_check) + + expect(collection.should_run?(double)).to be_falsey + end + end +end diff --git a/spec/lib/gitlab/cross_project_access/check_info_spec.rb b/spec/lib/gitlab/cross_project_access/check_info_spec.rb new file mode 100644 index 00000000000..bc9dbf2bece --- /dev/null +++ b/spec/lib/gitlab/cross_project_access/check_info_spec.rb @@ -0,0 +1,111 @@ +require 'spec_helper' + +describe Gitlab::CrossProjectAccess::CheckInfo do + let(:dummy_controller) { double } + + before do + allow(dummy_controller).to receive(:action_name).and_return('index') + end + + describe '#should_run?' do + it 'runs when an action is defined' do + info = described_class.new({ index: true }, nil, nil, false) + + expect(info.should_run?(dummy_controller)).to be_truthy + end + + it 'runs when the action is missing' do + info = described_class.new({}, nil, nil, false) + + expect(info.should_run?(dummy_controller)).to be_truthy + end + + it 'does not run when the action is excluded' do + info = described_class.new({ index: false }, nil, nil, false) + + expect(info.should_run?(dummy_controller)).to be_falsy + end + + it 'runs when the `if` conditional is true' do + info = described_class.new({}, -> { true }, nil, false) + + expect(info.should_run?(dummy_controller)).to be_truthy + end + + it 'does not run when the if condition is false' do + info = described_class.new({}, -> { false }, nil, false) + + expect(info.should_run?(dummy_controller)).to be_falsy + end + + it 'does not run when the `unless` check is true' do + info = described_class.new({}, nil, -> { true }, false) + + expect(info.should_run?(dummy_controller)).to be_falsy + end + + it 'runs when the `unless` check is false' do + info = described_class.new({}, nil, -> { false }, false) + + expect(info.should_run?(dummy_controller)).to be_truthy + end + + it 'returns the the oposite of #should_skip? when the check is a skip' do + info = described_class.new({}, nil, nil, true) + + expect(info).to receive(:should_skip?).with(dummy_controller).and_return(false) + expect(info.should_run?(dummy_controller)).to be_truthy + end + end + + describe '#should_skip?' do + it 'skips when an action is defined' do + info = described_class.new({ index: true }, nil, nil, true) + + expect(info.should_skip?(dummy_controller)).to be_truthy + end + + it 'does not skip when the action is not defined' do + info = described_class.new({}, nil, nil, true) + + expect(info.should_skip?(dummy_controller)).to be_falsy + end + + it 'does not skip when the action is excluded' do + info = described_class.new({ index: false }, nil, nil, true) + + expect(info.should_skip?(dummy_controller)).to be_falsy + end + + it 'skips when the `if` conditional is true' do + info = described_class.new({ index: true }, -> { true }, nil, true) + + expect(info.should_skip?(dummy_controller)).to be_truthy + end + + it 'does not skip the `if` conditional is false' do + info = described_class.new({ index: true }, -> { false }, nil, true) + + expect(info.should_skip?(dummy_controller)).to be_falsy + end + + it 'does not skip when the `unless` check is true' do + info = described_class.new({ index: true }, nil, -> { true }, true) + + expect(info.should_skip?(dummy_controller)).to be_falsy + end + + it 'skips when `unless` check is false' do + info = described_class.new({ index: true }, nil, -> { false }, true) + + expect(info.should_skip?(dummy_controller)).to be_truthy + end + + it 'returns the the oposite of #should_run? when the check is not a skip' do + info = described_class.new({}, nil, nil, false) + + expect(info).to receive(:should_run?).with(dummy_controller).and_return(false) + expect(info.should_skip?(dummy_controller)).to be_truthy + end + end +end diff --git a/spec/lib/gitlab/cross_project_access/class_methods_spec.rb b/spec/lib/gitlab/cross_project_access/class_methods_spec.rb new file mode 100644 index 00000000000..5349685e633 --- /dev/null +++ b/spec/lib/gitlab/cross_project_access/class_methods_spec.rb @@ -0,0 +1,46 @@ +require 'spec_helper' + +describe Gitlab::CrossProjectAccess::ClassMethods do + let(:dummy_class) do + Class.new do + extend Gitlab::CrossProjectAccess::ClassMethods + end + end + let(:dummy_proc) { lambda { false } } + + describe '#requires_cross_project_access' do + it 'creates a correct check when a hash is passed' do + expect(Gitlab::CrossProjectAccess) + .to receive(:add_check).with(dummy_class, + actions: { hello: true, world: false }, + positive_condition: dummy_proc, + negative_condition: dummy_proc) + + dummy_class.requires_cross_project_access( + hello: true, world: false, if: dummy_proc, unless: dummy_proc + ) + end + + it 'creates a correct check when an array is passed' do + expect(Gitlab::CrossProjectAccess) + .to receive(:add_check).with(dummy_class, + actions: { hello: true, world: true }, + positive_condition: nil, + negative_condition: nil) + + dummy_class.requires_cross_project_access(:hello, :world) + end + + it 'creates a correct check when an array and a hash is passed' do + expect(Gitlab::CrossProjectAccess) + .to receive(:add_check).with(dummy_class, + actions: { hello: true, world: true }, + positive_condition: dummy_proc, + negative_condition: dummy_proc) + + dummy_class.requires_cross_project_access( + :hello, :world, if: dummy_proc, unless: dummy_proc + ) + end + end +end diff --git a/spec/lib/gitlab/cross_project_access_spec.rb b/spec/lib/gitlab/cross_project_access_spec.rb new file mode 100644 index 00000000000..614b0473c7e --- /dev/null +++ b/spec/lib/gitlab/cross_project_access_spec.rb @@ -0,0 +1,84 @@ +require 'spec_helper' + +describe Gitlab::CrossProjectAccess do + let(:super_class) { Class.new } + let(:descendant_class) { Class.new(super_class) } + let(:current_instance) { described_class.new } + + before do + allow(described_class).to receive(:instance).and_return(current_instance) + end + + describe '#add_check' do + it 'keeps track of the properties to check' do + expect do + described_class.add_check(super_class, + actions: { index: true }, + positive_condition: -> { true }, + negative_condition: -> { false }) + end.to change { described_class.checks.size }.by(1) + end + + it 'builds the check correctly' do + check_collection = described_class.add_check(super_class, + actions: { index: true }, + positive_condition: -> { 'positive' }, + negative_condition: -> { 'negative' }) + + check = check_collection.checks.first + + expect(check.actions).to eq(index: true) + expect(check.positive_condition.call).to eq('positive') + expect(check.negative_condition.call).to eq('negative') + end + + it 'merges the checks of a parent class into existing checks of a subclass' do + subclass_collection = described_class.add_check(descendant_class) + + expect(subclass_collection).to receive(:add_collection).and_call_original + + described_class.add_check(super_class) + end + + it 'merges the existing checks of a superclass into the checks of a subclass' do + super_collection = described_class.add_check(super_class) + descendant_collection = described_class.add_check(descendant_class) + + expect(descendant_collection.checks).to include(*super_collection.checks) + end + end + + describe '#find_check' do + it 'returns a check when it was defined for a superclass' do + expected_check = described_class.add_check(super_class, + actions: { index: true }, + positive_condition: -> { 'positive' }, + negative_condition: -> { 'negative' }) + + expect(described_class.find_check(descendant_class.new)) + .to eq(expected_check) + end + + it 'caches the result for a subclass' do + described_class.add_check(super_class, + actions: { index: true }, + positive_condition: -> { 'positive' }, + negative_condition: -> { 'negative' }) + + expect(described_class.instance).to receive(:closest_parent).once.and_call_original + + 2.times { described_class.find_check(descendant_class.new) } + end + + it 'returns the checks for the closest class if there are more checks available' do + described_class.add_check(super_class, + actions: { index: true }) + expected_check = described_class.add_check(descendant_class, + actions: { index: true, show: false }) + + check = described_class.find_check(descendant_class.new) + + expect(check).to eq(expected_check) + end + end +end diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb index cd602ccab8e..73d60c021c8 100644 --- a/spec/lib/gitlab/diff/highlight_spec.rb +++ b/spec/lib/gitlab/diff/highlight_spec.rb @@ -72,6 +72,28 @@ describe Gitlab::Diff::Highlight do expect(subject[5].text).to eq(code) expect(subject[5].text).to be_html_safe end + + context 'when the inline diff marker has an invalid range' do + before do + allow_any_instance_of(Gitlab::Diff::InlineDiffMarker).to receive(:mark).and_raise(RangeError) + end + + it 'keeps the original rich line' do + code = %q{+ raise RuntimeError, "System commands must be given as an array of strings"} + + expect(subject[5].text).to eq(code) + expect(subject[5].text).not_to be_html_safe + end + + it 'reports to Sentry if configured' do + allow(Gitlab::Sentry).to receive(:enabled?).and_return(true) + + expect(Gitlab::Sentry).to receive(:context) + expect(Raven).to receive(:capture_exception) + + subject + end + end end end end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 67271d769a0..d601a383a98 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -600,6 +600,33 @@ describe Gitlab::Git::Repository, seed_helper: true do end end + describe '#branch_names_contains_sha' do + shared_examples 'returning the right branches' do + let(:head_id) { repository.rugged.head.target.oid } + let(:new_branch) { head_id } + + before do + repository.create_branch(new_branch, 'master') + end + + after do + repository.delete_branch(new_branch) + end + + it 'displays that branch' do + expect(repository.branch_names_contains_sha(head_id)).to include('master', new_branch) + end + end + + context 'when Gitaly is enabled' do + it_behaves_like 'returning the right branches' + end + + context 'when Gitaly is disabled', :disable_gitaly do + it_behaves_like 'returning the right branches' + end + end + describe "#refs_hash" do subject { repository.refs_hash } @@ -2204,7 +2231,7 @@ describe Gitlab::Git::Repository, seed_helper: true do context 'sparse checkout', :skip_gitaly_mock do let(:expected_files) { %w(files files/js files/js/application.js) } - before do + it 'checks out only the files in the diff' do allow(repository).to receive(:with_worktree).and_wrap_original do |m, *args| m.call(*args) do worktree_path = args[0] @@ -2216,11 +2243,34 @@ describe Gitlab::Git::Repository, seed_helper: true do expect(Dir[files_pattern]).to eq(expected) end end - end - it 'checkouts only the files in the diff' do subject end + + context 'when the diff contains a rename' do + let(:repo) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged } + let(:end_sha) { new_commit_move_file(repo).oid } + + after do + # Erase our commits so other tests get the original repo + repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged + repo.references.update('refs/heads/master', SeedRepo::LastCommit::ID) + end + + it 'does not include the renamed file in the sparse checkout' do + allow(repository).to receive(:with_worktree).and_wrap_original do |m, *args| + m.call(*args) do + worktree_path = args[0] + files_pattern = File.join(worktree_path, '**', '*') + + expect(Dir[files_pattern]).not_to include('CHANGELOG') + expect(Dir[files_pattern]).not_to include('encoding/CHANGELOG') + end + end + + subject + end + end end context 'with an ASCII-8BIT diff', :skip_gitaly_mock do @@ -2230,7 +2280,7 @@ describe Gitlab::Git::Repository, seed_helper: true do allow(repository).to receive(:run_git!).and_call_original allow(repository).to receive(:run_git!).with(%W(diff --binary #{start_sha}...#{end_sha})).and_return(diff.force_encoding('ASCII-8BIT')) - expect(subject.length).to eq(40) + expect(subject).to match(/\h{40}/) end end end diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb index 186b2d9279d..215f1ecc9c5 100644 --- a/spec/lib/gitlab/git_access_wiki_spec.rb +++ b/spec/lib/gitlab/git_access_wiki_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::GitAccessWiki do let(:access) { described_class.new(user, project, 'web', authentication_abilities: authentication_abilities, redirected_path: redirected_path) } - let(:project) { create(:project, :repository) } + let(:project) { create(:project, :wiki_repo) } let(:user) { create(:user) } let(:changes) { ['6f6d7e7ed 570e7b2ab refs/heads/master'] } let(:redirected_path) { nil } @@ -48,6 +48,18 @@ describe Gitlab::GitAccessWiki do it 'give access to download wiki code' do expect { subject }.not_to raise_error end + + context 'when the wiki repository does not exist' do + it 'returns not found' do + wiki_repo = project.wiki.repository + FileUtils.rm_rf(wiki_repo.path) + + # Sanity check for rm_rf + expect(wiki_repo.exists?).to eq(false) + + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'A repository for this project does not exist yet.') + end + end end context 'when wiki feature is disabled' do diff --git a/spec/lib/gitlab/gitaly_client/repository_service_spec.rb b/spec/lib/gitlab/gitaly_client/repository_service_spec.rb index cbc7ce1c1b0..c50e73cecfc 100644 --- a/spec/lib/gitlab/gitaly_client/repository_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/repository_service_spec.rb @@ -84,4 +84,30 @@ describe Gitlab::GitalyClient::RepositoryService do expect(client.has_local_branches?).to be(true) end end + + describe '#rebase_in_progress?' do + let(:rebase_id) { 1 } + + it 'sends a repository_rebase_in_progress message' do + expect_any_instance_of(Gitaly::RepositoryService::Stub) + .to receive(:is_rebase_in_progress) + .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash)) + .and_return(double(in_progress: true)) + + client.rebase_in_progress?(rebase_id) + end + end + + describe '#squash_in_progress?' do + let(:squash_id) { 1 } + + it 'sends a repository_squash_in_progress message' do + expect_any_instance_of(Gitaly::RepositoryService::Stub) + .to receive(:is_squash_in_progress) + .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash)) + .and_return(double(in_progress: true)) + + client.squash_in_progress?(squash_id) + end + end end diff --git a/spec/lib/gitlab/middleware/go_spec.rb b/spec/lib/gitlab/middleware/go_spec.rb index 60a134be939..b24c9882c0c 100644 --- a/spec/lib/gitlab/middleware/go_spec.rb +++ b/spec/lib/gitlab/middleware/go_spec.rb @@ -3,19 +3,30 @@ require 'spec_helper' describe Gitlab::Middleware::Go do let(:app) { double(:app) } let(:middleware) { described_class.new(app) } + let(:env) do + { + 'rack.input' => '', + 'REQUEST_METHOD' => 'GET' + } + end describe '#call' do describe 'when go-get=0' do + before do + env['QUERY_STRING'] = 'go-get=0' + end + it 'skips go-import generation' do - env = { 'rack.input' => '', - 'QUERY_STRING' => 'go-get=0' } expect(app).to receive(:call).with(env).and_return('no-go') middleware.call(env) end end describe 'when go-get=1' do - let(:current_user) { nil } + before do + env['QUERY_STRING'] = 'go-get=1' + env['PATH_INFO'] = "/#{path}" + end shared_examples 'go-get=1' do |enabled_protocol:| context 'with simple 2-segment project path' do @@ -54,21 +65,75 @@ describe Gitlab::Middleware::Go do project.update_attribute(:visibility_level, Project::PRIVATE) end - context 'with access to the project' do + shared_examples 'unauthorized' do + it 'returns the 2-segment group path' do + expect_response_with_path(go, enabled_protocol, group.full_path) + end + end + + context 'when not authenticated' do + it_behaves_like 'unauthorized' + end + + context 'when authenticated' do let(:current_user) { project.creator } before do project.team.add_master(current_user) end - it 'returns the full project path' do - expect_response_with_path(go, enabled_protocol, project.full_path) + shared_examples 'authenticated' do + context 'with access to the project' do + it 'returns the full project path' do + expect_response_with_path(go, enabled_protocol, project.full_path) + end + end + + context 'without access to the project' do + before do + project.team.find_member(current_user).destroy + end + + it_behaves_like 'unauthorized' + end end - end - context 'without access to the project' do - it 'returns the 2-segment group path' do - expect_response_with_path(go, enabled_protocol, group.full_path) + context 'using warden' do + before do + env['warden'] = double(authenticate: current_user) + end + + context 'when active' do + it_behaves_like 'authenticated' + end + + context 'when blocked' do + before do + current_user.block! + end + + it_behaves_like 'unauthorized' + end + end + + context 'using a personal access token' do + let(:personal_access_token) { create(:personal_access_token, user: current_user) } + + before do + env['HTTP_PRIVATE_TOKEN'] = personal_access_token.token + end + + context 'with api scope' do + it_behaves_like 'authenticated' + end + + context 'with read_user scope' do + before do + personal_access_token.update_attribute(:scopes, [:read_user]) + end + + it_behaves_like 'unauthorized' + end end end end @@ -138,12 +203,6 @@ describe Gitlab::Middleware::Go do end def go - env = { - 'rack.input' => '', - 'QUERY_STRING' => 'go-get=1', - 'PATH_INFO' => "/#{path}", - 'warden' => double(authenticate: current_user) - } middleware.call(env) end diff --git a/spec/lib/gitlab/prometheus/queries/matched_metrics_query_spec.rb b/spec/lib/gitlab/prometheus/queries/matched_metrics_query_spec.rb index 2b488101496..c95719eff1d 100644 --- a/spec/lib/gitlab/prometheus/queries/matched_metrics_query_spec.rb +++ b/spec/lib/gitlab/prometheus/queries/matched_metrics_query_spec.rb @@ -24,7 +24,7 @@ describe Gitlab::Prometheus::Queries::MatchedMetricsQuery do context 'with one group where two metrics is found' do before do - allow(metric_group_class).to receive(:all).and_return([simple_metric_group]) + allow(metric_group_class).to receive(:common_metrics).and_return([simple_metric_group]) allow(client).to receive(:label_values).and_return(metric_names) end @@ -70,7 +70,7 @@ describe Gitlab::Prometheus::Queries::MatchedMetricsQuery do context 'with one group where only one metric is found' do before do - allow(metric_group_class).to receive(:all).and_return([simple_metric_group]) + allow(metric_group_class).to receive(:common_metrics).and_return([simple_metric_group]) allow(client).to receive(:label_values).and_return('metric_a') end @@ -99,7 +99,7 @@ describe Gitlab::Prometheus::Queries::MatchedMetricsQuery do let(:second_metric_group) { simple_metric_group(name: 'nameb', metrics: simple_metrics(added_metric_name: 'metric_c')) } before do - allow(metric_group_class).to receive(:all).and_return([simple_metric_group, second_metric_group]) + allow(metric_group_class).to receive(:common_metrics).and_return([simple_metric_group, second_metric_group]) allow(client).to receive(:label_values).and_return('metric_c') end diff --git a/spec/lib/gitlab/prometheus_client_spec.rb b/spec/lib/gitlab/prometheus_client_spec.rb index 5d86007f71f..4c3b8deefb9 100644 --- a/spec/lib/gitlab/prometheus_client_spec.rb +++ b/spec/lib/gitlab/prometheus_client_spec.rb @@ -19,41 +19,41 @@ describe Gitlab::PrometheusClient do # - execute_query: A query call shared_examples 'failure response' do context 'when request returns 400 with an error message' do - it 'raises a Gitlab::PrometheusError error' do + it 'raises a Gitlab::PrometheusClient::Error error' do req_stub = stub_prometheus_request(query_url, status: 400, body: { error: 'bar!' }) expect { execute_query } - .to raise_error(Gitlab::PrometheusError, 'bar!') + .to raise_error(Gitlab::PrometheusClient::Error, 'bar!') expect(req_stub).to have_been_requested end end context 'when request returns 400 without an error message' do - it 'raises a Gitlab::PrometheusError error' do + it 'raises a Gitlab::PrometheusClient::Error error' do req_stub = stub_prometheus_request(query_url, status: 400) expect { execute_query } - .to raise_error(Gitlab::PrometheusError, 'Bad data received') + .to raise_error(Gitlab::PrometheusClient::Error, 'Bad data received') expect(req_stub).to have_been_requested end end context 'when request returns 500' do - it 'raises a Gitlab::PrometheusError error' do + it 'raises a Gitlab::PrometheusClient::Error error' do req_stub = stub_prometheus_request(query_url, status: 500, body: { message: 'FAIL!' }) expect { execute_query } - .to raise_error(Gitlab::PrometheusError, '500 - {"message":"FAIL!"}') + .to raise_error(Gitlab::PrometheusClient::Error, '500 - {"message":"FAIL!"}') expect(req_stub).to have_been_requested end end context 'when request returns non json data' do - it 'raises a Gitlab::PrometheusError error' do + it 'raises a Gitlab::PrometheusClient::Error error' do req_stub = stub_prometheus_request(query_url, status: 200, body: 'not json') expect { execute_query } - .to raise_error(Gitlab::PrometheusError, 'Parsing response failed') + .to raise_error(Gitlab::PrometheusClient::Error, 'Parsing response failed') expect(req_stub).to have_been_requested end end @@ -65,27 +65,27 @@ describe Gitlab::PrometheusClient do subject { described_class.new(RestClient::Resource.new(prometheus_url)) } context 'exceptions are raised' do - it 'raises a Gitlab::PrometheusError error when a SocketError is rescued' do + it 'raises a Gitlab::PrometheusClient::Error error when a SocketError is rescued' do req_stub = stub_prometheus_request_with_exception(prometheus_url, SocketError) expect { subject.send(:get, '/', {}) } - .to raise_error(Gitlab::PrometheusError, "Can't connect to #{prometheus_url}") + .to raise_error(Gitlab::PrometheusClient::Error, "Can't connect to #{prometheus_url}") expect(req_stub).to have_been_requested end - it 'raises a Gitlab::PrometheusError error when a SSLError is rescued' do + it 'raises a Gitlab::PrometheusClient::Error error when a SSLError is rescued' do req_stub = stub_prometheus_request_with_exception(prometheus_url, OpenSSL::SSL::SSLError) expect { subject.send(:get, '/', {}) } - .to raise_error(Gitlab::PrometheusError, "#{prometheus_url} contains invalid SSL data") + .to raise_error(Gitlab::PrometheusClient::Error, "#{prometheus_url} contains invalid SSL data") expect(req_stub).to have_been_requested end - it 'raises a Gitlab::PrometheusError error when a RestClient::Exception is rescued' do + it 'raises a Gitlab::PrometheusClient::Error error when a RestClient::Exception is rescued' do req_stub = stub_prometheus_request_with_exception(prometheus_url, RestClient::Exception) expect { subject.send(:get, '/', {}) } - .to raise_error(Gitlab::PrometheusError, "Network connection error") + .to raise_error(Gitlab::PrometheusClient::Error, "Network connection error") expect(req_stub).to have_been_requested end end diff --git a/spec/lib/gitlab/quick_actions/command_definition_spec.rb b/spec/lib/gitlab/quick_actions/command_definition_spec.rb index f44a562dc63..b03c1e23ca3 100644 --- a/spec/lib/gitlab/quick_actions/command_definition_spec.rb +++ b/spec/lib/gitlab/quick_actions/command_definition_spec.rb @@ -40,7 +40,7 @@ describe Gitlab::QuickActions::CommandDefinition do end describe "#available?" do - let(:opts) { { go: false } } + let(:opts) { OpenStruct.new(go: false) } context "when the command has a condition block" do before do @@ -78,7 +78,7 @@ describe Gitlab::QuickActions::CommandDefinition do it "doesn't execute the command" do expect(context).not_to receive(:instance_exec) - subject.execute(context, {}, nil) + subject.execute(context, nil) expect(context.run).to be false end @@ -95,7 +95,7 @@ describe Gitlab::QuickActions::CommandDefinition do end it "doesn't execute the command" do - subject.execute(context, {}, nil) + subject.execute(context, nil) expect(context.run).to be false end @@ -109,7 +109,7 @@ describe Gitlab::QuickActions::CommandDefinition do context "when the command is provided an argument" do it "executes the command" do - subject.execute(context, {}, true) + subject.execute(context, true) expect(context.run).to be true end @@ -117,7 +117,7 @@ describe Gitlab::QuickActions::CommandDefinition do context "when the command is not provided an argument" do it "executes the command" do - subject.execute(context, {}, nil) + subject.execute(context, nil) expect(context.run).to be true end @@ -131,7 +131,7 @@ describe Gitlab::QuickActions::CommandDefinition do context "when the command is provided an argument" do it "executes the command" do - subject.execute(context, {}, true) + subject.execute(context, true) expect(context.run).to be true end @@ -139,7 +139,7 @@ describe Gitlab::QuickActions::CommandDefinition do context "when the command is not provided an argument" do it "doesn't execute the command" do - subject.execute(context, {}, nil) + subject.execute(context, nil) expect(context.run).to be false end @@ -153,7 +153,7 @@ describe Gitlab::QuickActions::CommandDefinition do context "when the command is provided an argument" do it "executes the command" do - subject.execute(context, {}, true) + subject.execute(context, true) expect(context.run).to be true end @@ -161,7 +161,7 @@ describe Gitlab::QuickActions::CommandDefinition do context "when the command is not provided an argument" do it "executes the command" do - subject.execute(context, {}, nil) + subject.execute(context, nil) expect(context.run).to be true end @@ -175,7 +175,7 @@ describe Gitlab::QuickActions::CommandDefinition do end it 'executes the command passing the parsed param' do - subject.execute(context, {}, 'something ') + subject.execute(context, 'something ') expect(context.received_arg).to eq('something') end @@ -192,7 +192,7 @@ describe Gitlab::QuickActions::CommandDefinition do end it 'returns nil' do - result = subject.explain({}, {}, nil) + result = subject.explain({}, nil) expect(result).to be_nil end @@ -204,7 +204,7 @@ describe Gitlab::QuickActions::CommandDefinition do end it 'returns this static string' do - result = subject.explain({}, {}, nil) + result = subject.explain({}, nil) expect(result).to eq 'Explanation' end @@ -216,7 +216,7 @@ describe Gitlab::QuickActions::CommandDefinition do end it 'invokes the proc' do - result = subject.explain({}, {}, 'explanation') + result = subject.explain({}, 'explanation') expect(result).to eq 'Dynamic explanation' end diff --git a/spec/lib/gitlab/quick_actions/dsl_spec.rb b/spec/lib/gitlab/quick_actions/dsl_spec.rb index ff59dc48bcb..067a30fd7e2 100644 --- a/spec/lib/gitlab/quick_actions/dsl_spec.rb +++ b/spec/lib/gitlab/quick_actions/dsl_spec.rb @@ -76,7 +76,7 @@ describe Gitlab::QuickActions::Dsl do expect(dynamic_description_def.name).to eq(:dynamic_description) expect(dynamic_description_def.aliases).to eq([]) - expect(dynamic_description_def.to_h(noteable: 'issue')[:description]).to eq('A dynamic description for ISSUE') + expect(dynamic_description_def.to_h(OpenStruct.new(noteable: 'issue'))[:description]).to eq('A dynamic description for ISSUE') expect(dynamic_description_def.explanation).to eq('') expect(dynamic_description_def.params).to eq(['The first argument', 'The second argument']) expect(dynamic_description_def.condition_block).to be_nil diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb index ef51e3cc8df..5b5052de372 100644 --- a/spec/lib/gitlab/sql/pattern_spec.rb +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -154,6 +154,12 @@ describe Gitlab::SQL::Pattern do it 'returns a single equality condition' do expect(fuzzy_arel_match.to_sql).to match(/title.*I?LIKE 'fo'/) end + + it 'uses LOWER instead of ILIKE when LOWER is enabled' do + rel = Issue.fuzzy_arel_match(:title, query, lower_exact_match: true) + + expect(rel.to_sql).to match(/LOWER\(.*title.*\).*=.*'fo'/) + end end context 'with two words both equal to 3 chars' do diff --git a/spec/lib/google_api/cloud_platform/client_spec.rb b/spec/lib/google_api/cloud_platform/client_spec.rb index f65e41dfea3..db9d9158b29 100644 --- a/spec/lib/google_api/cloud_platform/client_spec.rb +++ b/spec/lib/google_api/cloud_platform/client_spec.rb @@ -115,6 +115,9 @@ describe GoogleApi::CloudPlatform::Client do "initial_node_count": cluster_size, "node_config": { "machine_type": machine_type + }, + "legacy_abac": { + "enabled": true } } } ) |