diff options
Diffstat (limited to 'spec')
22 files changed, 659 insertions, 326 deletions
diff --git a/spec/features/dashboard/groups_list_spec.rb b/spec/features/dashboard/groups_list_spec.rb index a71020002dc..ed47f7ed390 100644 --- a/spec/features/dashboard/groups_list_spec.rb +++ b/spec/features/dashboard/groups_list_spec.rb @@ -40,7 +40,7 @@ feature 'Dashboard Groups page', :js do expect(page).to have_content(nested_group.name) end - describe 'when filtering groups', :nested_groups do + context 'when filtering groups', :nested_groups do before do group.add_owner(user) nested_group.add_owner(user) @@ -75,7 +75,7 @@ feature 'Dashboard Groups page', :js do end end - describe 'group with subgroups', :nested_groups do + context 'with subgroups', :nested_groups do let!(:subgroup) { create(:group, :public, parent: group) } before do @@ -106,7 +106,7 @@ feature 'Dashboard Groups page', :js do end end - describe 'when using pagination' do + context 'when using pagination' do let(:group) { create(:group, created_at: 5.days.ago) } let(:group2) { create(:group, created_at: 2.days.ago) } @@ -141,4 +141,20 @@ feature 'Dashboard Groups page', :js do expect(page).not_to have_selector("#group-#{group2.id}") end end + + context 'when signed in as admin' do + let(:admin) { create(:admin) } + + it 'shows only groups admin is member of' do + group.add_owner(admin) + expect(another_group).to be_persisted + + sign_in(admin) + visit dashboard_groups_path + wait_for_requests + + expect(page).to have_content(group.name) + expect(page).not_to have_content(another_group.name) + end + end end diff --git a/spec/features/projects/issues/user_toggles_subscription_spec.rb b/spec/features/projects/issues/user_toggles_subscription_spec.rb index 117a614b980..c2b2a193682 100644 --- a/spec/features/projects/issues/user_toggles_subscription_spec.rb +++ b/spec/features/projects/issues/user_toggles_subscription_spec.rb @@ -1,9 +1,9 @@ require "spec_helper" describe "User toggles subscription", :js do - set(:project) { create(:project_empty_repo, :public) } - set(:user) { create(:user) } - set(:issue) { create(:issue, project: project, author: user) } + let(:project) { create(:project_empty_repo, :public) } + let(:user) { create(:user) } + let(:issue) { create(:issue, project: project, author: user) } before do project.add_developer(user) @@ -12,7 +12,7 @@ describe "User toggles subscription", :js do visit(project_issue_path(project, issue)) end - it "unsibscribes from issue" do + it "unsubscribes from issue" do subscription_button = find(".js-issuable-subscribe-button") # Check we're subscribed. diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb index abc470788e1..16c0d418d98 100644 --- a/spec/finders/groups_finder_spec.rb +++ b/spec/finders/groups_finder_spec.rb @@ -2,43 +2,71 @@ require 'spec_helper' describe GroupsFinder do describe '#execute' do - let(:user) { create(:user) } - - context 'root level groups' do - let!(:private_group) { create(:group, :private) } - let!(:internal_group) { create(:group, :internal) } - let!(:public_group) { create(:group, :public) } - - context 'without a user' do - subject { described_class.new.execute } - - it { is_expected.to eq([public_group]) } + let(:user) { create(:user) } + + describe 'root level groups' do + using RSpec::Parameterized::TableSyntax + + where(:user_type, :params, :results) do + nil | { all_available: true } | %i(public_group user_public_group) + nil | { all_available: false } | %i(public_group user_public_group) + nil | {} | %i(public_group user_public_group) + + :regular | { all_available: true } | %i(public_group internal_group user_public_group user_internal_group + user_private_group) + :regular | { all_available: false } | %i(user_public_group user_internal_group user_private_group) + :regular | {} | %i(public_group internal_group user_public_group user_internal_group user_private_group) + + :external | { all_available: true } | %i(public_group user_public_group user_internal_group user_private_group) + :external | { all_available: false } | %i(user_public_group user_internal_group user_private_group) + :external | {} | %i(public_group user_public_group user_internal_group user_private_group) + + :admin | { all_available: true } | %i(public_group internal_group private_group user_public_group + user_internal_group user_private_group) + :admin | { all_available: false } | %i(user_public_group user_internal_group user_private_group) + :admin | {} | %i(public_group internal_group private_group user_public_group user_internal_group + user_private_group) end - context 'with a user' do - subject { described_class.new(user).execute } - - context 'normal user' do - it { is_expected.to contain_exactly(public_group, internal_group) } - end - - context 'external user' do - let(:user) { create(:user, external: true) } - - it { is_expected.to contain_exactly(public_group) } + with_them do + before do + # Fixme: Because of an issue: https://github.com/tomykaira/rspec-parameterized/issues/8#issuecomment-381888428 + # The groups need to be created here, not with let syntax, and also compared by name and not ids + + @groups = { + private_group: create(:group, :private, name: 'private_group'), + internal_group: create(:group, :internal, name: 'internal_group'), + public_group: create(:group, :public, name: 'public_group'), + + user_private_group: create(:group, :private, name: 'user_private_group'), + user_internal_group: create(:group, :internal, name: 'user_internal_group'), + user_public_group: create(:group, :public, name: 'user_public_group') + } + + if user_type + user = + case user_type + when :regular + create(:user) + when :external + create(:user, external: true) + when :admin + create(:user, :admin) + end + @groups.values_at(:user_private_group, :user_internal_group, :user_public_group).each do |group| + group.add_developer(user) + end + end end - context 'user is member of the private group' do - before do - private_group.add_guest(user) - end + subject { described_class.new(User.last, params).execute.to_a } - it { is_expected.to contain_exactly(public_group, internal_group, private_group) } - end + it { is_expected.to match_array(@groups.values_at(*results)) } end end context 'subgroups', :nested_groups do + let(:user) { create(:user) } let!(:parent_group) { create(:group, :public) } let!(:public_subgroup) { create(:group, :public, parent: parent_group) } let!(:internal_subgroup) { create(:group, :internal, parent: parent_group) } diff --git a/spec/fixtures/api/schemas/public_api/v4/notes.json b/spec/fixtures/api/schemas/public_api/v4/notes.json index 4c4ca3b582f..0f9c221fd6d 100644 --- a/spec/fixtures/api/schemas/public_api/v4/notes.json +++ b/spec/fixtures/api/schemas/public_api/v4/notes.json @@ -24,7 +24,10 @@ "system": { "type": "boolean" }, "noteable_id": { "type": "integer" }, "noteable_iid": { "type": "integer" }, - "noteable_type": { "type": "string" } + "noteable_type": { "type": "string" }, + "resolved": { "type": "boolean" }, + "resolvable": { "type": "boolean" }, + "resolved_by": { "type": ["string", "null"] } }, "required": [ "id", "body", "attachment", "author", "created_at", "updated_at", diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 43cb0dfe163..5e454f8b310 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -2,8 +2,6 @@ require 'spec_helper' describe ApplicationHelper do - include UploadHelpers - describe 'current_controller?' do it 'returns true when controller matches argument' do stub_controller_name('foo') @@ -54,143 +52,6 @@ describe ApplicationHelper do end end - describe 'project_icon' do - it 'returns an url for the avatar' do - project = create(:project, :public, avatar: File.open(uploaded_image_temp_path)) - - expect(helper.project_icon(project.full_path).to_s) - .to eq "<img data-src=\"#{project.avatar.url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />" - end - end - - describe 'avatar_icon_for' do - let!(:user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: 'bar@example.com') } - let(:email) { 'foo@example.com' } - let!(:another_user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: email) } - - it 'prefers the user to retrieve the avatar_url' do - expect(helper.avatar_icon_for(user, email).to_s) - .to eq(user.avatar.url) - end - - it 'falls back to email lookup if no user given' do - expect(helper.avatar_icon_for(nil, email).to_s) - .to eq(another_user.avatar.url) - end - end - - describe 'avatar_icon_for_email' do - let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) } - - context 'using an email' do - context 'when there is a matching user' do - it 'returns a relative URL for the avatar' do - expect(helper.avatar_icon_for_email(user.email).to_s) - .to eq(user.avatar.url) - end - end - - context 'when no user exists for the email' do - it 'calls gravatar_icon' do - expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2) - - helper.avatar_icon_for_email('foo@example.com', 20, 2) - end - end - - context 'without an email passed' do - it 'calls gravatar_icon' do - expect(helper).to receive(:gravatar_icon).with(nil, 20, 2) - - helper.avatar_icon_for_email(nil, 20, 2) - end - end - end - end - - describe 'avatar_icon_for_user' do - let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) } - - context 'with a user object passed' do - it 'returns a relative URL for the avatar' do - expect(helper.avatar_icon_for_user(user).to_s) - .to eq(user.avatar.url) - end - end - - context 'without a user object passed' do - it 'calls gravatar_icon' do - expect(helper).to receive(:gravatar_icon).with(nil, 20, 2) - - helper.avatar_icon_for_user(nil, 20, 2) - end - end - end - - describe 'gravatar_icon' do - let(:user_email) { 'user@email.com' } - - context 'with Gravatar disabled' do - before do - stub_application_setting(gravatar_enabled?: false) - end - - it 'returns a generic avatar' do - expect(helper.gravatar_icon(user_email)).to match_asset_path('no_avatar.png') - end - end - - context 'with Gravatar enabled' do - before do - stub_application_setting(gravatar_enabled?: true) - end - - it 'returns a generic avatar when email is blank' do - expect(helper.gravatar_icon('')).to match_asset_path('no_avatar.png') - end - - it 'returns a valid Gravatar URL' do - stub_config_setting(https: false) - - expect(helper.gravatar_icon(user_email)) - .to match('https://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118') - end - - it 'uses HTTPs when configured' do - stub_config_setting(https: true) - - expect(helper.gravatar_icon(user_email)) - .to match('https://secure.gravatar.com') - end - - it 'returns custom gravatar path when gravatar_url is set' do - stub_gravatar_setting(plain_url: 'http://example.local/?s=%{size}&hash=%{hash}') - - expect(gravatar_icon(user_email, 20)) - .to eq('http://example.local/?s=40&hash=b58c6f14d292556214bd64909bcdb118') - end - - it 'accepts a custom size argument' do - expect(helper.gravatar_icon(user_email, 64)).to include '?s=128' - end - - it 'defaults size to 40@2x when given an invalid size' do - expect(helper.gravatar_icon(user_email, nil)).to include '?s=80' - end - - it 'accepts a scaling factor' do - expect(helper.gravatar_icon(user_email, 40, 3)).to include '?s=120' - end - - it 'ignores case and surrounding whitespace' do - normal = helper.gravatar_icon('foo@example.com') - upcase = helper.gravatar_icon(' FOO@EXAMPLE.COM ') - - expect(normal).to eq upcase - end - end - end - describe 'simple_sanitize' do let(:a_tag) { '<a href="#">Foo</a>' } diff --git a/spec/helpers/auth_helper_spec.rb b/spec/helpers/auth_helper_spec.rb index c94fedd615b..120b23e66ac 100644 --- a/spec/helpers/auth_helper_spec.rb +++ b/spec/helpers/auth_helper_spec.rb @@ -18,6 +18,30 @@ describe AuthHelper do end end + describe "providers_for_base_controller" do + it 'returns all enabled providers from devise' do + allow(helper).to receive(:auth_providers) { [:twitter, :github] } + expect(helper.providers_for_base_controller).to include(*[:twitter, :github]) + end + + it 'excludes ldap providers' do + allow(helper).to receive(:auth_providers) { [:twitter, :ldapmain] } + expect(helper.providers_for_base_controller).not_to include(:ldapmain) + end + end + + describe "form_based_providers" do + it 'includes LDAP providers' do + allow(helper).to receive(:auth_providers) { [:twitter, :ldapmain] } + expect(helper.form_based_providers).to eq %i(ldapmain) + end + + it 'includes crowd provider' do + allow(helper).to receive(:auth_providers) { [:twitter, :crowd] } + expect(helper.form_based_providers).to eq %i(crowd) + end + end + describe 'enabled_button_based_providers' do before do allow(helper).to receive(:auth_providers) { [:twitter, :github] } diff --git a/spec/helpers/avatars_helper_spec.rb b/spec/helpers/avatars_helper_spec.rb index 04c6d259135..5856bccb5b8 100644 --- a/spec/helpers/avatars_helper_spec.rb +++ b/spec/helpers/avatars_helper_spec.rb @@ -1,10 +1,147 @@ require 'rails_helper' describe AvatarsHelper do - include ApplicationHelper + include UploadHelpers let(:user) { create(:user) } + describe '#project_icon' do + it 'returns an url for the avatar' do + project = create(:project, :public, avatar: File.open(uploaded_image_temp_path)) + + expect(helper.project_icon(project.full_path).to_s) + .to eq "<img data-src=\"#{project.avatar.url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />" + end + end + + describe '#avatar_icon_for' do + let!(:user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: 'bar@example.com') } + let(:email) { 'foo@example.com' } + let!(:another_user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: email) } + + it 'prefers the user to retrieve the avatar_url' do + expect(helper.avatar_icon_for(user, email).to_s) + .to eq(user.avatar.url) + end + + it 'falls back to email lookup if no user given' do + expect(helper.avatar_icon_for(nil, email).to_s) + .to eq(another_user.avatar.url) + end + end + + describe '#avatar_icon_for_email' do + let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) } + + context 'using an email' do + context 'when there is a matching user' do + it 'returns a relative URL for the avatar' do + expect(helper.avatar_icon_for_email(user.email).to_s) + .to eq(user.avatar.url) + end + end + + context 'when no user exists for the email' do + it 'calls gravatar_icon' do + expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2) + + helper.avatar_icon_for_email('foo@example.com', 20, 2) + end + end + + context 'without an email passed' do + it 'calls gravatar_icon' do + expect(helper).to receive(:gravatar_icon).with(nil, 20, 2) + + helper.avatar_icon_for_email(nil, 20, 2) + end + end + end + end + + describe '#avatar_icon_for_user' do + let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) } + + context 'with a user object passed' do + it 'returns a relative URL for the avatar' do + expect(helper.avatar_icon_for_user(user).to_s) + .to eq(user.avatar.url) + end + end + + context 'without a user object passed' do + it 'calls gravatar_icon' do + expect(helper).to receive(:gravatar_icon).with(nil, 20, 2) + + helper.avatar_icon_for_user(nil, 20, 2) + end + end + end + + describe '#gravatar_icon' do + let(:user_email) { 'user@email.com' } + + context 'with Gravatar disabled' do + before do + stub_application_setting(gravatar_enabled?: false) + end + + it 'returns a generic avatar' do + expect(helper.gravatar_icon(user_email)).to match_asset_path('no_avatar.png') + end + end + + context 'with Gravatar enabled' do + before do + stub_application_setting(gravatar_enabled?: true) + end + + it 'returns a generic avatar when email is blank' do + expect(helper.gravatar_icon('')).to match_asset_path('no_avatar.png') + end + + it 'returns a valid Gravatar URL' do + stub_config_setting(https: false) + + expect(helper.gravatar_icon(user_email)) + .to match('https://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118') + end + + it 'uses HTTPs when configured' do + stub_config_setting(https: true) + + expect(helper.gravatar_icon(user_email)) + .to match('https://secure.gravatar.com') + end + + it 'returns custom gravatar path when gravatar_url is set' do + stub_gravatar_setting(plain_url: 'http://example.local/?s=%{size}&hash=%{hash}') + + expect(gravatar_icon(user_email, 20)) + .to eq('http://example.local/?s=40&hash=b58c6f14d292556214bd64909bcdb118') + end + + it 'accepts a custom size argument' do + expect(helper.gravatar_icon(user_email, 64)).to include '?s=128' + end + + it 'defaults size to 40@2x when given an invalid size' do + expect(helper.gravatar_icon(user_email, nil)).to include '?s=80' + end + + it 'accepts a scaling factor' do + expect(helper.gravatar_icon(user_email, 40, 3)).to include '?s=120' + end + + it 'ignores case and surrounding whitespace' do + normal = helper.gravatar_icon('foo@example.com') + upcase = helper.gravatar_icon(' FOO@EXAMPLE.COM ') + + expect(normal).to eq upcase + end + end + end + describe '#user_avatar' do subject { helper.user_avatar(user: user) } diff --git a/spec/javascripts/sidebar/mock_data.js b/spec/javascripts/sidebar/mock_data.js index 8b6e8b24f00..fcd7bea3f6d 100644 --- a/spec/javascripts/sidebar/mock_data.js +++ b/spec/javascripts/sidebar/mock_data.js @@ -138,7 +138,7 @@ const RESPONSE_MAP = { }, { id: 20, - name_with_namespace: 'foo / bar', + name_with_namespace: '<img src=x onerror=alert(document.domain)> foo / bar', }, ], }, diff --git a/spec/javascripts/sidebar/sidebar_move_issue_spec.js b/spec/javascripts/sidebar/sidebar_move_issue_spec.js index 1777053d370..8f35b9ca437 100644 --- a/spec/javascripts/sidebar/sidebar_move_issue_spec.js +++ b/spec/javascripts/sidebar/sidebar_move_issue_spec.js @@ -71,6 +71,15 @@ describe('SidebarMoveIssue', function () { expect($.fn.glDropdown).toHaveBeenCalled(); }); + + it('escapes html from project name', (done) => { + this.$toggleButton.dropdown('toggle'); + + setTimeout(() => { + expect(this.$content.find('.js-move-issue-dropdown-item')[1].innerHTML.trim()).toEqual('<img src=x onerror=alert(document.domain)> foo / bar'); + done(); + }); + }); }); describe('onConfirmClicked', () => { diff --git a/spec/lib/banzai/filter/commit_trailers_filter_spec.rb b/spec/lib/banzai/filter/commit_trailers_filter_spec.rb index 1fd145116df..068cdc85a07 100644 --- a/spec/lib/banzai/filter/commit_trailers_filter_spec.rb +++ b/spec/lib/banzai/filter/commit_trailers_filter_spec.rb @@ -47,16 +47,36 @@ describe Banzai::Filter::CommitTrailersFilter do ) end - it 'non GitLab users and replaces them with mailto links' do - _, message_html = build_commit_message( - trailer: trailer, - name: FFaker::Name.name, - email: email - ) + context 'non GitLab users' do + shared_examples 'mailto links' do + it 'replaces them with mailto links' do + _, message_html = build_commit_message( + trailer: trailer, + name: FFaker::Name.name, + email: email + ) - doc = filter(message_html) + doc = filter(message_html) - expect_to_have_mailto_link(doc, email: email, trailer: trailer) + expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: trailer) + end + end + + context 'when Gravatar is disabled' do + before do + stub_application_setting(gravatar_enabled: false) + end + + it_behaves_like 'mailto links' + end + + context 'when Gravatar is enabled' do + before do + stub_application_setting(gravatar_enabled: true) + end + + it_behaves_like 'mailto links' + end end it 'multiple trailers in the same message' do @@ -69,7 +89,7 @@ describe Banzai::Filter::CommitTrailersFilter do doc = filter(message) expect_to_have_user_link_with_avatar(doc, user: user, trailer: trailer) - expect_to_have_mailto_link(doc, email: email, trailer: different_trailer) + expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: different_trailer) end context 'special names' do @@ -90,7 +110,7 @@ describe Banzai::Filter::CommitTrailersFilter do doc = filter(message_html) - expect_to_have_mailto_link(doc, email: email, trailer: trailer) + expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: trailer) expect(doc.text).to match Regexp.escape(message) end end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index da1a6229ccf..9924641f829 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -234,59 +234,72 @@ describe Gitlab::Git::Repository, seed_helper: true do it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :tag_names end - shared_examples 'archive check' do |extenstion| - it { expect(metadata['ArchivePath']).to match(%r{tmp/gitlab-git-test.git/gitlab-git-test-master-#{SeedRepo::LastCommit::ID}}) } - it { expect(metadata['ArchivePath']).to end_with extenstion } - end + describe '#archive_metadata' do + let(:storage_path) { '/tmp' } + let(:cache_key) { File.join(repository.gl_repository, SeedRepo::LastCommit::ID) } - describe '#archive_prefix' do - let(:project_name) { 'project-name'} + let(:append_sha) { true } + let(:ref) { 'master' } + let(:format) { nil } - before do - expect(repository).to receive(:name).once.and_return(project_name) - end + let(:expected_extension) { 'tar.gz' } + let(:expected_filename) { "#{expected_prefix}.#{expected_extension}" } + let(:expected_path) { File.join(storage_path, cache_key, expected_filename) } + let(:expected_prefix) { "gitlab-git-test-#{ref}-#{SeedRepo::LastCommit::ID}" } - it 'returns parameterised string for a ref containing slashes' do - prefix = repository.archive_prefix('test/branch', 'SHA', append_sha: nil) + subject(:metadata) { repository.archive_metadata(ref, storage_path, format, append_sha: append_sha) } - expect(prefix).to eq("#{project_name}-test-branch-SHA") + it 'sets RepoPath to the repository path' do + expect(metadata['RepoPath']).to eq(repository.path) end - it 'returns correct string for a ref containing dots' do - prefix = repository.archive_prefix('test.branch', 'SHA', append_sha: nil) - - expect(prefix).to eq("#{project_name}-test.branch-SHA") + it 'sets CommitId to the commit SHA' do + expect(metadata['CommitId']).to eq(SeedRepo::LastCommit::ID) end - it 'returns string with sha when append_sha is false' do - prefix = repository.archive_prefix('test.branch', 'SHA', append_sha: false) - - expect(prefix).to eq("#{project_name}-test.branch") + it 'sets ArchivePrefix to the expected prefix' do + expect(metadata['ArchivePrefix']).to eq(expected_prefix) end - end - describe '#archive' do - let(:metadata) { repository.archive_metadata('master', '/tmp', append_sha: true) } + it 'sets ArchivePath to the expected globally-unique path' do + # This is really important from a security perspective. Think carefully + # before changing it: https://gitlab.com/gitlab-org/gitlab-ce/issues/45689 + expect(expected_path).to include(File.join(repository.gl_repository, SeedRepo::LastCommit::ID)) - it_should_behave_like 'archive check', '.tar.gz' - end - - describe '#archive_zip' do - let(:metadata) { repository.archive_metadata('master', '/tmp', 'zip', append_sha: true) } + expect(metadata['ArchivePath']).to eq(expected_path) + end - it_should_behave_like 'archive check', '.zip' - end + context 'append_sha varies archive path and filename' do + where(:append_sha, :ref, :expected_prefix) do + sha = SeedRepo::LastCommit::ID - describe '#archive_bz2' do - let(:metadata) { repository.archive_metadata('master', '/tmp', 'tbz2', append_sha: true) } + true | 'master' | "gitlab-git-test-master-#{sha}" + true | sha | "gitlab-git-test-#{sha}-#{sha}" + false | 'master' | "gitlab-git-test-master" + false | sha | "gitlab-git-test-#{sha}" + nil | 'master' | "gitlab-git-test-master-#{sha}" + nil | sha | "gitlab-git-test-#{sha}" + end - it_should_behave_like 'archive check', '.tar.bz2' - end + with_them do + it { expect(metadata['ArchivePrefix']).to eq(expected_prefix) } + it { expect(metadata['ArchivePath']).to eq(expected_path) } + end + end - describe '#archive_fallback' do - let(:metadata) { repository.archive_metadata('master', '/tmp', 'madeup', append_sha: true) } + context 'format varies archive path and filename' do + where(:format, :expected_extension) do + nil | 'tar.gz' + 'madeup' | 'tar.gz' + 'tbz2' | 'tar.bz2' + 'zip' | 'zip' + end - it_should_behave_like 'archive check', '.tar.gz' + with_them do + it { expect(metadata['ArchivePrefix']).to eq(expected_prefix) } + it { expect(metadata['ArchivePath']).to eq(expected_path) } + end + end end describe '#size' do diff --git a/spec/lib/gitlab/kubernetes/helm/base_command_spec.rb b/spec/lib/gitlab/kubernetes/helm/base_command_spec.rb index 3cfdae794f6..7be8be54d5e 100644 --- a/spec/lib/gitlab/kubernetes/helm/base_command_spec.rb +++ b/spec/lib/gitlab/kubernetes/helm/base_command_spec.rb @@ -4,22 +4,10 @@ describe Gitlab::Kubernetes::Helm::BaseCommand do let(:application) { create(:clusters_applications_helm) } let(:base_command) { described_class.new(application.name) } - describe '#generate_script' do - let(:helm_version) { Gitlab::Kubernetes::Helm::HELM_VERSION } - let(:command) do - <<~HEREDOC - set -eo pipefail - apk add -U ca-certificates openssl >/dev/null - wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v#{helm_version}-linux-amd64.tar.gz | tar zxC /tmp >/dev/null - mv /tmp/linux-amd64/helm /usr/bin/ - HEREDOC - end - - subject { base_command.generate_script } + subject { base_command } - it 'should return a command that prepares the environment for helm-cli' do - expect(subject).to eq(command) - end + it_behaves_like 'helm commands' do + let(:commands) { '' } end describe '#pod_resource' do diff --git a/spec/lib/gitlab/kubernetes/helm/init_command_spec.rb b/spec/lib/gitlab/kubernetes/helm/init_command_spec.rb index e6920b0a76f..89e36a298f8 100644 --- a/spec/lib/gitlab/kubernetes/helm/init_command_spec.rb +++ b/spec/lib/gitlab/kubernetes/helm/init_command_spec.rb @@ -2,23 +2,9 @@ require 'spec_helper' describe Gitlab::Kubernetes::Helm::InitCommand do let(:application) { create(:clusters_applications_helm) } - let(:init_command) { described_class.new(application.name) } + let(:commands) { 'helm init >/dev/null' } - describe '#generate_script' do - let(:command) do - <<~MSG.chomp - set -eo pipefail - apk add -U ca-certificates openssl >/dev/null - wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null - mv /tmp/linux-amd64/helm /usr/bin/ - helm init >/dev/null - MSG - end + subject { described_class.new(application.name) } - subject { init_command.generate_script } - - it 'should return the appropriate command' do - is_expected.to eq(command) - end - end + it_behaves_like 'helm commands' end diff --git a/spec/lib/gitlab/kubernetes/helm/install_command_spec.rb b/spec/lib/gitlab/kubernetes/helm/install_command_spec.rb index 137b8f718de..547f3f1752c 100644 --- a/spec/lib/gitlab/kubernetes/helm/install_command_spec.rb +++ b/spec/lib/gitlab/kubernetes/helm/install_command_spec.rb @@ -12,50 +12,36 @@ describe Gitlab::Kubernetes::Helm::InstallCommand do ) end - describe '#generate_script' do - let(:command) do - <<~MSG - set -eo pipefail - apk add -U ca-certificates openssl >/dev/null - wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null - mv /tmp/linux-amd64/helm /usr/bin/ - helm init --client-only >/dev/null - helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null - MSG - end - - subject { install_command.generate_script } + subject { install_command } - it 'should return appropriate command' do - is_expected.to eq(command) + it_behaves_like 'helm commands' do + let(:commands) do + <<~EOS + helm init --client-only >/dev/null + helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null + EOS end + end - context 'with an application with a repository' do - let(:ci_runner) { create(:ci_runner) } - let(:application) { create(:clusters_applications_runner, runner: ci_runner) } - let(:install_command) do - described_class.new( - application.name, - chart: application.chart, - values: application.values, - repository: application.repository - ) - end - - let(:command) do - <<~MSG - set -eo pipefail - apk add -U ca-certificates openssl >/dev/null - wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null - mv /tmp/linux-amd64/helm /usr/bin/ - helm init --client-only >/dev/null - helm repo add #{application.name} #{application.repository} - helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null - MSG - end + context 'with an application with a repository' do + let(:ci_runner) { create(:ci_runner) } + let(:application) { create(:clusters_applications_runner, runner: ci_runner) } + let(:install_command) do + described_class.new( + application.name, + chart: application.chart, + values: application.values, + repository: application.repository + ) + end - it 'should return appropriate command' do - is_expected.to eq(command) + it_behaves_like 'helm commands' do + let(:commands) do + <<~EOS + helm init --client-only >/dev/null + helm repo add #{application.name} #{application.repository} + helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null + EOS end end end diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index 2705421e540..fb51c0172ab 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -85,12 +85,35 @@ describe DiffNote do end describe "#diff_file" do - it "returns the correct diff file" do - diff_file = subject.diff_file + context 'when the discussion was created in the diff' do + it 'returns correct diff file' do + diff_file = subject.diff_file - expect(diff_file.old_path).to eq(position.old_path) - expect(diff_file.new_path).to eq(position.new_path) - expect(diff_file.diff_refs).to eq(position.diff_refs) + expect(diff_file.old_path).to eq(position.old_path) + expect(diff_file.new_path).to eq(position.new_path) + expect(diff_file.diff_refs).to eq(position.diff_refs) + end + end + + context 'when discussion is outdated or not created in the diff' do + let(:diff_refs) { project.commit(sample_commit.id).diff_refs } + let(:position) do + Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: diff_refs + ) + end + + it 'returns the correct diff file' do + diff_file = subject.diff_file + + expect(diff_file.old_path).to eq(position.old_path) + expect(diff_file.new_path).to eq(position.new_path) + expect(diff_file.diff_refs).to eq(position.diff_refs) + end end end diff --git a/spec/requests/api/discussions_spec.rb b/spec/requests/api/discussions_spec.rb index 4a44b219a67..ef34192f888 100644 --- a/spec/requests/api/discussions_spec.rb +++ b/spec/requests/api/discussions_spec.rb @@ -2,32 +2,53 @@ require 'spec_helper' describe API::Discussions do let(:user) { create(:user) } - let!(:project) { create(:project, :public, namespace: user.namespace) } + let!(:project) { create(:project, :public, :repository, namespace: user.namespace) } let(:private_user) { create(:user) } before do - project.add_reporter(user) + project.add_developer(user) end - context "when noteable is an Issue" do + context 'when noteable is an Issue' do let!(:issue) { create(:issue, project: project, author: user) } let!(:issue_note) { create(:discussion_note_on_issue, noteable: issue, project: project, author: user) } - it_behaves_like "discussions API", 'projects', 'issues', 'iid' do + it_behaves_like 'discussions API', 'projects', 'issues', 'iid' do let(:parent) { project } let(:noteable) { issue } let(:note) { issue_note } end end - context "when noteable is a Snippet" do + context 'when noteable is a Snippet' do let!(:snippet) { create(:project_snippet, project: project, author: user) } let!(:snippet_note) { create(:discussion_note_on_snippet, noteable: snippet, project: project, author: user) } - it_behaves_like "discussions API", 'projects', 'snippets', 'id' do + it_behaves_like 'discussions API', 'projects', 'snippets', 'id' do let(:parent) { project } let(:noteable) { snippet } let(:note) { snippet_note } end end + + context 'when noteable is a Merge Request' do + let!(:noteable) { create(:merge_request_with_diffs, source_project: project, target_project: project, author: user) } + let!(:note) { create(:discussion_note_on_merge_request, noteable: noteable, project: project, author: user) } + let!(:diff_note) { create(:diff_note_on_merge_request, noteable: noteable, project: project, author: user) } + let(:parent) { project } + + it_behaves_like 'discussions API', 'projects', 'merge_requests', 'iid' + it_behaves_like 'diff discussions API', 'projects', 'merge_requests', 'iid' + it_behaves_like 'resolvable discussions API', 'projects', 'merge_requests', 'iid' + end + + context 'when noteable is a Commit' do + let!(:noteable) { create(:commit, project: project, author: user) } + let!(:note) { create(:discussion_note_on_commit, commit_id: noteable.id, project: project, author: user) } + let!(:diff_note) { create(:diff_note_on_commit, commit_id: noteable.id, project: project, author: user) } + let(:parent) { project } + + it_behaves_like 'discussions API', 'projects', 'repository/commits', 'id' + it_behaves_like 'diff discussions API', 'projects', 'repository/commits', 'id' + end end diff --git a/spec/services/notes/resolve_service_spec.rb b/spec/services/notes/resolve_service_spec.rb new file mode 100644 index 00000000000..b54d40a7a5c --- /dev/null +++ b/spec/services/notes/resolve_service_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe Notes::ResolveService do + let(:merge_request) { create(:merge_request) } + let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.project) } + let(:user) { merge_request.author } + + describe '#execute' do + it "resolves the note" do + described_class.new(merge_request.project, user).execute(note) + note.reload + + expect(note.resolved?).to be true + expect(note.resolved_by).to eq(user) + end + + it "sends notifications if all discussions are resolved" do + expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request) + + described_class.new(merge_request.project, user).execute(note) + end + end +end diff --git a/spec/services/repository_archive_clean_up_service_spec.rb b/spec/services/repository_archive_clean_up_service_spec.rb index 2d7fa3f80f7..ab1c638fc39 100644 --- a/spec/services/repository_archive_clean_up_service_spec.rb +++ b/spec/services/repository_archive_clean_up_service_spec.rb @@ -1,15 +1,47 @@ require 'spec_helper' describe RepositoryArchiveCleanUpService do - describe '#execute' do - subject(:service) { described_class.new } + subject(:service) { described_class.new } + describe '#execute (new archive locations)' do + let(:sha) { "0" * 40 } + + it 'removes outdated archives and directories in a new-style path' do + in_directory_with_files("project-999/#{sha}", %w[tar tar.bz2 tar.gz zip], 3.hours) do |dirname, files| + service.execute + + files.each { |filename| expect(File.exist?(filename)).to be_falsy } + expect(File.directory?(dirname)).to be_falsy + expect(File.directory?(File.dirname(dirname))).to be_falsy + end + end + + it 'does not remove directories when they contain outdated non-archives' do + in_directory_with_files("project-999/#{sha}", %w[tar conf rb], 2.hours) do |dirname, files| + service.execute + + expect(File.directory?(dirname)).to be_truthy + end + end + + it 'does not remove in-date archives in a new-style path' do + in_directory_with_files("project-999/#{sha}", %w[tar tar.bz2 tar.gz zip], 1.hour) do |dirname, files| + service.execute + + files.each { |filename| expect(File.exist?(filename)).to be_truthy } + end + end + end + + describe '#execute (legacy archive locations)' do context 'when the downloads directory does not exist' do it 'does not remove any archives' do path = '/invalid/path/' stub_repository_downloads_path(path) + allow(File).to receive(:directory?).and_call_original expect(File).to receive(:directory?).with(path).and_return(false) + expect(service).not_to receive(:clean_up_old_archives) expect(service).not_to receive(:clean_up_empty_directories) @@ -19,7 +51,7 @@ describe RepositoryArchiveCleanUpService do context 'when the downloads directory exists' do shared_examples 'invalid archive files' do |dirname, extensions, mtime| - it 'does not remove files and directoy' do + it 'does not remove files and directory' do in_directory_with_files(dirname, extensions, mtime) do |dir, files| service.execute @@ -43,7 +75,7 @@ describe RepositoryArchiveCleanUpService do end context 'with files older than 2 hours inside invalid directories' do - it_behaves_like 'invalid archive files', 'john_doe/sample.git', %w[conf rb tar tar.gz], 2.hours + it_behaves_like 'invalid archive files', 'john/doe/sample.git', %w[conf rb tar tar.gz], 2.hours end context 'with files newer than 2 hours that matches valid archive extensions' do @@ -58,24 +90,24 @@ describe RepositoryArchiveCleanUpService do it_behaves_like 'invalid archive files', 'sample.git', %w[conf rb tar tar.gz], 1.hour end end + end - def in_directory_with_files(dirname, extensions, mtime) - Dir.mktmpdir do |tmpdir| - stub_repository_downloads_path(tmpdir) - dir = File.join(tmpdir, dirname) - files = create_temporary_files(dir, extensions, mtime) + def in_directory_with_files(dirname, extensions, mtime) + Dir.mktmpdir do |tmpdir| + stub_repository_downloads_path(tmpdir) + dir = File.join(tmpdir, dirname) + files = create_temporary_files(dir, extensions, mtime) - yield(dir, files) - end + yield(dir, files) end + end - def stub_repository_downloads_path(path) - allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path) - end + def stub_repository_downloads_path(path) + allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path) + end - def create_temporary_files(dir, extensions, mtime) - FileUtils.mkdir_p(dir) - FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now - mtime) - end + def create_temporary_files(dir, extensions, mtime) + FileUtils.mkdir_p(dir) + FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now - mtime) end end diff --git a/spec/support/commit_trailers_spec_helper.rb b/spec/support/commit_trailers_spec_helper.rb index add359946db..efa317fd2f9 100644 --- a/spec/support/commit_trailers_spec_helper.rb +++ b/spec/support/commit_trailers_spec_helper.rb @@ -8,7 +8,7 @@ module CommitTrailersSpecHelper expect(wrapper.attribute('data-user').value).to eq user.id.to_s end - def expect_to_have_mailto_link(doc, email:, trailer:) + def expect_to_have_mailto_link_with_avatar(doc, email:, trailer:) wrapper = find_user_wrapper(doc, trailer) expect_to_have_links_with_url_and_avatar(wrapper, "mailto:#{CGI.escape_html(email)}", email) diff --git a/spec/support/shared_examples/helm_generated_script.rb b/spec/support/shared_examples/helm_generated_script.rb new file mode 100644 index 00000000000..56e86a87ab9 --- /dev/null +++ b/spec/support/shared_examples/helm_generated_script.rb @@ -0,0 +1,19 @@ +shared_examples 'helm commands' do + describe '#generate_script' do + let(:helm_setup) do + <<~EOS + set -eo pipefail + ALPINE_VERSION=$(cat /etc/alpine-release | cut -d '.' -f 1,2) + echo http://mirror.clarkson.edu/alpine/v$ALPINE_VERSION/main >> /etc/apk/repositories + echo http://mirror1.hs-esslingen.de/pub/Mirrors/alpine/v$ALPINE_VERSION/main >> /etc/apk/repositories + apk add -U ca-certificates openssl >/dev/null + wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null + mv /tmp/linux-amd64/helm /usr/bin/ + EOS + end + + it 'should return appropriate command' do + expect(subject.generate_script).to eq(helm_setup + commands) + end + end +end diff --git a/spec/support/shared_examples/requests/api/diff_discussions.rb b/spec/support/shared_examples/requests/api/diff_discussions.rb new file mode 100644 index 00000000000..85a4bd8ca27 --- /dev/null +++ b/spec/support/shared_examples/requests/api/diff_discussions.rb @@ -0,0 +1,57 @@ +shared_examples 'diff discussions API' do |parent_type, noteable_type, id_name| + describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do + it "includes diff discussions" do + get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user) + + discussion = json_response.find { |record| record['id'] == diff_note.discussion_id } + + expect(response).to have_gitlab_http_status(200) + expect(discussion).not_to be_nil + expect(discussion['individual_note']).to eq(false) + expect(discussion['notes'].first['body']).to eq(diff_note.note) + end + end + + describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do + it "returns a discussion by id" do + get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions/#{diff_note.discussion_id}", user) + + expect(response).to have_gitlab_http_status(200) + expect(json_response['id']).to eq(diff_note.discussion_id) + expect(json_response['notes'].first['body']).to eq(diff_note.note) + expect(json_response['notes'].first['position']).to eq(diff_note.position.to_h.stringify_keys) + end + end + + describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do + it "creates a new diff note" do + position = diff_note.position.to_h + + post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), body: 'hi!', position: position + + expect(response).to have_gitlab_http_status(201) + expect(json_response['notes'].first['body']).to eq('hi!') + expect(json_response['notes'].first['type']).to eq('DiffNote') + expect(json_response['notes'].first['position']).to eq(position.stringify_keys) + end + + it "returns a 400 bad request error when position is invalid" do + position = diff_note.position.to_h.merge(new_line: '100000') + + post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), body: 'hi!', position: position + + expect(response).to have_gitlab_http_status(400) + end + end + + describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes" do + it 'adds a new note to the diff discussion' do + post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{diff_note.discussion_id}/notes", user), body: 'hi!' + + expect(response).to have_gitlab_http_status(201) + expect(json_response['body']).to eq('hi!') + expect(json_response['type']).to eq('DiffNote') + end + end +end diff --git a/spec/support/shared_examples/requests/api/resolvable_discussions.rb b/spec/support/shared_examples/requests/api/resolvable_discussions.rb new file mode 100644 index 00000000000..408ad08cc48 --- /dev/null +++ b/spec/support/shared_examples/requests/api/resolvable_discussions.rb @@ -0,0 +1,87 @@ +shared_examples 'resolvable discussions API' do |parent_type, noteable_type, id_name| + describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do + it "resolves discussion if resolved is true" do + put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{note.discussion_id}", user), resolved: true + + expect(response).to have_gitlab_http_status(200) + expect(json_response['notes'].size).to eq(1) + expect(json_response['notes'][0]['resolved']).to eq(true) + end + + it "unresolves discussion if resolved is false" do + put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{note.discussion_id}", user), resolved: false + + expect(response).to have_gitlab_http_status(200) + expect(json_response['notes'].size).to eq(1) + expect(json_response['notes'][0]['resolved']).to eq(false) + end + + it "returns a 400 bad request error if resolved parameter is not passed" do + put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{note.discussion_id}", user) + + expect(response).to have_gitlab_http_status(400) + end + + it "returns a 401 unauthorized error if user is not authenticated" do + put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{note.discussion_id}"), resolved: true + + expect(response).to have_gitlab_http_status(401) + end + + it "returns a 403 error if user resolves discussion of someone else" do + put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{note.discussion_id}", private_user), resolved: true + + expect(response).to have_gitlab_http_status(403) + end + + context 'when user does not have access to read the discussion' do + before do + parent.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + it 'responds with 404' do + put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{note.discussion_id}", private_user), resolved: true + + expect(response).to have_gitlab_http_status(404) + end + end + end + + describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes/:note_id" do + it 'returns resolved note when resolved parameter is true' do + put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{note.discussion_id}/notes/#{note.id}", user), resolved: true + + expect(response).to have_gitlab_http_status(200) + expect(json_response['resolved']).to eq(true) + end + + it 'returns a 404 error when note id not found' do + put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{note.discussion_id}/notes/12345", user), + body: 'Hello!' + + expect(response).to have_gitlab_http_status(404) + end + + it 'returns a 400 bad request error if neither body nor resolved parameter is given' do + put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{note.discussion_id}/notes/#{note.id}", user) + + expect(response).to have_gitlab_http_status(400) + end + + it "returns a 403 error if user resolves note of someone else" do + put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\ + "discussions/#{note.discussion_id}/notes/#{note.id}", private_user), resolved: true + + expect(response).to have_gitlab_http_status(403) + end + end +end |