diff options
author | James Lopez <james@gitlab.com> | 2018-12-27 16:41:49 +0300 |
---|---|---|
committer | James Lopez <james@gitlab.com> | 2018-12-27 16:41:49 +0300 |
commit | 21dd6f080c5d3671b41f7707cbe60f6fa12f2768 (patch) | |
tree | ce197aa70b021b47f1189086631c1ec08daaeb9a /spec | |
parent | 5f49eed9d971430b9472c94583b5518a5962fe79 (diff) | |
parent | b0e6341ad176abe903c5117c1c0a10ffd25de55b (diff) |
Merge branch 'security-11-6' into 'security-fix/security-group-user-removal-11-6'
# Conflicts:
# app/services/members/destroy_service.rb
Diffstat (limited to 'spec')
33 files changed, 783 insertions, 87 deletions
diff --git a/spec/controllers/groups/settings/ci_cd_controller_spec.rb b/spec/controllers/groups/settings/ci_cd_controller_spec.rb index 06ccace8242..1bcc30915a1 100644 --- a/spec/controllers/groups/settings/ci_cd_controller_spec.rb +++ b/spec/controllers/groups/settings/ci_cd_controller_spec.rb @@ -5,30 +5,65 @@ describe Groups::Settings::CiCdController do let(:user) { create(:user) } before do - group.add_maintainer(user) sign_in(user) end describe 'GET #show' do - it 'renders show with 200 status code' do - get :show, group_id: group + context 'when user is owner' do + before do + group.add_owner(user) + end - expect(response).to have_gitlab_http_status(200) - expect(response).to render_template(:show) + it 'renders show with 200 status code' do + get :show, group_id: group + + expect(response).to have_gitlab_http_status(200) + expect(response).to render_template(:show) + end + end + + context 'when user is not owner' do + before do + group.add_maintainer(user) + end + + it 'renders a 404' do + get :show, group_id: group + + expect(response).to have_gitlab_http_status(404) + end end end describe 'PUT #reset_registration_token' do subject { put :reset_registration_token, group_id: group } - it 'resets runner registration token' do - expect { subject }.to change { group.reload.runners_token } + context 'when user is owner' do + before do + group.add_owner(user) + end + + it 'resets runner registration token' do + expect { subject }.to change { group.reload.runners_token } + end + + it 'redirects the user to admin runners page' do + subject + + expect(response).to redirect_to(group_settings_ci_cd_path) + end end - it 'redirects the user to admin runners page' do - subject + context 'when user is not owner' do + before do + group.add_maintainer(user) + end + + it 'renders a 404' do + subject - expect(response).to redirect_to(group_settings_ci_cd_path) + expect(response).to have_gitlab_http_status(404) + end end end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 7849bec4762..febbced6054 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -590,10 +590,10 @@ describe ProjectsController do end describe "GET refs" do - let(:public_project) { create(:project, :public, :repository) } + let(:project) { create(:project, :public, :repository) } it 'gets a list of branches and tags' do - get :refs, namespace_id: public_project.namespace, id: public_project, sort: 'updated_desc' + get :refs, namespace_id: project.namespace, id: project, sort: 'updated_desc' parsed_body = JSON.parse(response.body) expect(parsed_body['Branches']).to include('master') @@ -603,7 +603,7 @@ describe ProjectsController do end it "gets a list of branches, tags and commits" do - get :refs, namespace_id: public_project.namespace, id: public_project, ref: "123456" + get :refs, namespace_id: project.namespace, id: project, ref: "123456" parsed_body = JSON.parse(response.body) expect(parsed_body["Branches"]).to include("master") @@ -618,7 +618,7 @@ describe ProjectsController do end it "gets a list of branches, tags and commits" do - get :refs, namespace_id: public_project.namespace, id: public_project, ref: "123456" + get :refs, namespace_id: project.namespace, id: project, ref: "123456" parsed_body = JSON.parse(response.body) expect(parsed_body["Branches"]).to include("master") @@ -626,6 +626,22 @@ describe ProjectsController do expect(parsed_body["Commits"]).to include("123456") end end + + context 'when private project' do + let(:project) { create(:project, :repository) } + + context 'as a guest' do + it 'renders forbidden' do + user = create(:user) + project.add_guest(user) + + sign_in(user) + get :refs, namespace_id: project.namespace, id: project + + expect(response).to have_gitlab_http_status(404) + end + end + end end describe 'POST #preview_markdown' do diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb index 89e0cdd8ed7..57e3ddfb39c 100644 --- a/spec/features/group_variables_spec.rb +++ b/spec/features/group_variables_spec.rb @@ -7,7 +7,7 @@ describe 'Group variables', :js do let(:page_path) { group_settings_ci_cd_path(group) } before do - group.add_maintainer(user) + group.add_owner(user) gitlab_sign_in(user) visit page_path diff --git a/spec/features/issues/gfm_autocomplete_spec.rb b/spec/features/issues/gfm_autocomplete_spec.rb index d7531d5fcd9..3b7a17ef355 100644 --- a/spec/features/issues/gfm_autocomplete_spec.rb +++ b/spec/features/issues/gfm_autocomplete_spec.rb @@ -3,6 +3,8 @@ require 'rails_helper' describe 'GFM autocomplete', :js do let(:issue_xss_title) { 'This will execute alert<img src=x onerror=alert(2)<img src=x onerror=alert(1)>' } let(:user_xss_title) { 'eve <img src=x onerror=alert(2)<img src=x onerror=alert(1)>' } + let(:label_xss_title) { 'alert label <img src=x onerror="alert(\'Hello xss\');" a'} + let(:milestone_xss_title) { 'alert milestone <img src=x onerror="alert(\'Hello xss\');" a' } let(:user_xss) { create(:user, name: user_xss_title, username: 'xss.user') } let(:user) { create(:user, name: '💃speciąl someone💃', username: 'someone.special') } @@ -25,10 +27,14 @@ describe 'GFM autocomplete', :js do simulate_input('#issue-description', "@#{user.name[0...3]}") + wait_for_requests + find('.atwho-view .cur').click click_button 'Save changes' + wait_for_requests + expect(find('.description')).to have_content(user.to_reference) end @@ -47,6 +53,8 @@ describe 'GFM autocomplete', :js do find('#note-body').native.send_keys('#') end + wait_for_requests + expect(page).to have_selector('.atwho-container') page.within '.atwho-container #at-view-issues' do @@ -59,6 +67,8 @@ describe 'GFM autocomplete', :js do find('#note-body').native.send_keys('@ev') end + wait_for_requests + expect(page).to have_selector('.atwho-container') page.within '.atwho-container #at-view-users' do @@ -66,6 +76,22 @@ describe 'GFM autocomplete', :js do end end + it 'opens autocomplete menu for Milestone when field starts with text with item escaping HTML characters' do + create(:milestone, project: project, title: milestone_xss_title) + + page.within '.timeline-content-form' do + find('#note-body').native.send_keys('%') + end + + wait_for_requests + + expect(page).to have_selector('.atwho-container') + + page.within '.atwho-container #at-view-milestones' do + expect(find('li').text).to have_content('alert milestone') + end + end + it 'doesnt open autocomplete menu character is prefixed with text' do page.within '.timeline-content-form' do find('#note-body').native.send_keys('testing') @@ -258,12 +284,28 @@ describe 'GFM autocomplete', :js do let!(:bug) { create(:label, project: project, title: 'bug') } let!(:feature_proposal) { create(:label, project: project, title: 'feature proposal') } + it 'opens autocomplete menu for Labels when field starts with text with item escaping HTML characters' do + create(:label, project: project, title: label_xss_title) + + note = find('#note-body') + + # It should show all the labels on "~". + type(note, '~') + + wait_for_requests + + page.within '.atwho-container #at-view-labels' do + expect(find('.atwho-view-ul').text).to have_content('alert label') + end + end + context 'when no labels are assigned' do it 'shows labels' do note = find('#note-body') # It should show all the labels on "~". type(note, '~') + wait_for_requests expect_labels(shown: [backend, bug, feature_proposal]) # It should show all the labels on "/label ~". @@ -290,6 +332,7 @@ describe 'GFM autocomplete', :js do # It should show all the labels on "~". type(note, '~') + wait_for_requests expect_labels(shown: [backend, bug, feature_proposal]) # It should show only unset labels on "/label ~". @@ -316,6 +359,7 @@ describe 'GFM autocomplete', :js do # It should show all the labels on "~". type(note, '~') + wait_for_requests expect_labels(shown: [backend, bug, feature_proposal]) # It should show no labels on "/label ~". diff --git a/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb b/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb new file mode 100644 index 00000000000..9318b5f1ebb --- /dev/null +++ b/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +describe 'Merge Request > Tries to access private repo of public project' do + let(:current_user) { create(:user) } + let(:private_project) do + create(:project, :public, :repository, + path: 'nothing-to-see-here', + name: 'nothing to see here', + repository_access_level: ProjectFeature::PRIVATE) + end + let(:owned_project) do + create(:project, :public, :repository, + namespace: current_user.namespace, + creator: current_user) + end + + context 'when the user enters the querystring info for the other project' do + let(:mr_path) do + project_new_merge_request_diffs_path( + owned_project, + merge_request: { + source_project_id: private_project.id, + source_branch: 'feature' + } + ) + end + + before do + sign_in current_user + visit mr_path + end + + it "does not mention the project the user can't see the repo of" do + expect(page).not_to have_content('nothing-to-see-here') + end + end +end diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb index cb7a912946c..09de983f669 100644 --- a/spec/features/runners_spec.rb +++ b/spec/features/runners_spec.rb @@ -259,8 +259,9 @@ describe 'Runners' do context 'group runners in group settings' do let(:group) { create(:group) } + before do - group.add_maintainer(user) + group.add_owner(user) end context 'group with no runners' do diff --git a/spec/lib/banzai/filter/external_link_filter_spec.rb b/spec/lib/banzai/filter/external_link_filter_spec.rb index 2a3c0cd78b8..e6dae8d5382 100644 --- a/spec/lib/banzai/filter/external_link_filter_spec.rb +++ b/spec/lib/banzai/filter/external_link_filter_spec.rb @@ -49,16 +49,16 @@ describe Banzai::Filter::ExternalLinkFilter do end context 'for invalid urls' do - it 'skips broken hrefs' do + it 'adds rel and target attributes to broken hrefs' do doc = filter %q(<p><a href="don't crash on broken urls">Google</a></p>) - expected = %q(<p><a href="don't%20crash%20on%20broken%20urls">Google</a></p>) + expected = %q(<p><a href="don't%20crash%20on%20broken%20urls" rel="nofollow noreferrer noopener" target="_blank">Google</a></p>) expect(doc.to_html).to eq(expected) end - it 'skips improperly formatted mailtos' do + it 'adds rel and target to improperly formatted mailtos' do doc = filter %q(<p><a href="mailto://jblogs@example.com">Email</a></p>) - expected = %q(<p><a href="mailto://jblogs@example.com">Email</a></p>) + expected = %q(<p><a href="mailto://jblogs@example.com" rel="nofollow noreferrer noopener" target="_blank">Email</a></p>) expect(doc.to_html).to eq(expected) end diff --git a/spec/lib/banzai/filter/label_reference_filter_spec.rb b/spec/lib/banzai/filter/label_reference_filter_spec.rb index 00257ed7904..9cfdb9e53a2 100644 --- a/spec/lib/banzai/filter/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/label_reference_filter_spec.rb @@ -236,6 +236,24 @@ describe Banzai::Filter::LabelReferenceFilter do end end + context 'References with html entities' do + let!(:label) { create(:label, name: '<html>', project: project) } + + it 'links to a valid reference' do + doc = reference_filter('See ~"<html>"') + + expect(doc.css('a').first.attr('href')).to eq urls + .project_issues_url(project, label_name: label.name) + expect(doc.text).to eq 'See <html>' + end + + it 'ignores invalid label names and escapes entities' do + act = %(Label #{Label.reference_prefix}"<non valid>") + + expect(reference_filter(act).to_html).to eq act + end + end + describe 'consecutive references' do let(:bug) { create(:label, name: 'bug', project: project) } let(:feature_proposal) { create(:label, name: 'feature proposal', project: project) } diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index 75a177d2d1f..6aa802ce6fd 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -182,4 +182,24 @@ describe Gitlab::Ci::Pipeline::Chain::Command do it { is_expected.to eq(false) } end end + + describe '#ambiguous_ref' do + let(:project) { create(:project, :repository) } + let(:command) { described_class.new(project: project, origin_ref: 'ref') } + + subject { command.ambiguous_ref? } + + context 'when ref is not ambiguous' do + it { is_expected. to eq(false) } + end + + context 'when ref is ambiguous' do + before do + project.repository.add_tag(project.creator, 'ref', 'master') + project.repository.add_branch(project.creator, 'ref', 'master') + end + + it { is_expected. to eq(true) } + end + end end diff --git a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb index 284aed91e29..1b014ecfaa4 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb @@ -14,6 +14,7 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do Gitlab::Ci::Pipeline::Chain::Command.new( project: project, current_user: user, + origin_ref: 'master', seeds_block: nil) end @@ -106,6 +107,7 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do Gitlab::Ci::Pipeline::Chain::Command.new( project: project, current_user: user, + origin_ref: 'master', seeds_block: seeds_block) end diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb index fb1b53fc55c..a7cad423d09 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb @@ -42,6 +42,27 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do end end + context 'when ref is ambiguous' do + let(:project) do + create(:project, :repository).tap do |proj| + proj.repository.add_tag(user, 'master', 'master') + end + end + let(:command) do + Gitlab::Ci::Pipeline::Chain::Command.new( + project: project, current_user: user, origin_ref: 'master') + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + + it 'adds an error about missing ref' do + expect(pipeline.errors.to_a) + .to include 'Ref is ambiguous' + end + end + context 'when does not have existing SHA set' do let(:command) do Gitlab::Ci::Pipeline::Chain::Command.new( diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index fffa727c2ed..2cf812b26dc 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Gitlab::Ci::Pipeline::Seed::Build do - let(:pipeline) { create(:ci_empty_pipeline) } + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } let(:attributes) do { name: 'rspec', diff --git a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb index 05ce3412fd8..82f741845db 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Gitlab::Ci::Pipeline::Seed::Stage do - let(:pipeline) { create(:ci_empty_pipeline) } + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } let(:attributes) do { name: 'test', diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 4cdcae5f670..7b7329c2461 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2385,6 +2385,8 @@ describe Ci::Build do end context 'when protected variable is defined' do + let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } + let(:protected_variable) do { key: 'PROTECTED_KEY', value: 'protected_value', public: false } end @@ -2397,7 +2399,7 @@ describe Ci::Build do context 'when the branch is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2405,7 +2407,7 @@ describe Ci::Build do context 'when the tag is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2430,6 +2432,8 @@ describe Ci::Build do end context 'when group protected variable is defined' do + let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } + let(:protected_variable) do { key: 'PROTECTED_KEY', value: 'protected_value', public: false } end @@ -2442,7 +2446,7 @@ describe Ci::Build do context 'when the branch is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2450,7 +2454,7 @@ describe Ci::Build do context 'when the tag is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index b67c6a4cffa..17f33785fda 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -397,6 +397,10 @@ describe Ci::Pipeline, :mailer do end describe '#protected_ref?' do + before do + pipeline.project = create(:project, :repository) + end + it 'delegates method to project' do expect(pipeline).not_to be_protected_ref end diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb new file mode 100644 index 00000000000..8aed72d77a4 --- /dev/null +++ b/spec/models/concerns/has_ref_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe HasRef do + describe '#branch?' do + let(:build) { create(:ci_build) } + + subject { build.branch? } + + context 'is not a tag' do + before do + build.tag = false + end + + it 'return true when tag is set to false' do + is_expected.to be_truthy + end + end + + context 'is not a tag' do + before do + build.tag = true + end + + it 'return false when tag is set to true' do + is_expected.to be_falsey + end + end + end + + describe '#git_ref' do + subject { build.git_ref } + + context 'when tag is true' do + let(:build) { create(:ci_build, tag: true) } + + it 'returns a tag ref' do + is_expected.to start_with(Gitlab::Git::TAG_REF_PREFIX) + end + end + + context 'when tag is false' do + let(:build) { create(:ci_build, tag: false) } + + it 'returns a branch ref' do + is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) + end + end + + context 'when tag is nil' do + let(:build) { create(:ci_build, tag: nil) } + + it 'returns a branch ref' do + is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) + end + end + end +end diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index 81748681528..a64720f1876 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -243,6 +243,20 @@ describe Event do expect(event.visible_to_user?(admin)).to eq true end end + + context 'private project' do + let(:project) { create(:project, :private) } + let(:target) { note_on_issue } + + it do + expect(event.visible_to_user?(non_member)).to eq false + expect(event.visible_to_user?(author)).to eq false + expect(event.visible_to_user?(assignee)).to eq false + expect(event.visible_to_user?(member)).to eq true + expect(event.visible_to_user?(guest)).to eq true + expect(event.visible_to_user?(admin)).to eq true + end + end end context 'merge request diff note event' do @@ -265,8 +279,8 @@ describe Event do it do expect(event.visible_to_user?(non_member)).to eq false - expect(event.visible_to_user?(author)).to eq true - expect(event.visible_to_user?(assignee)).to eq true + expect(event.visible_to_user?(author)).to eq false + expect(event.visible_to_user?(assignee)).to eq false expect(event.visible_to_user?(member)).to eq true expect(event.visible_to_user?(guest)).to eq false expect(event.visible_to_user?(admin)).to eq true diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 50920d9d1fc..1cccc95a436 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -314,6 +314,13 @@ describe Project do expect(project.errors[:import_url].first).to include('Requests to localhost are not allowed') end + it 'does not allow import_url pointing to the local network' do + project = build(:project, import_url: 'https://192.168.1.1') + + expect(project).to be_invalid + expect(project.errors[:import_url].first).to include('Requests to the local network are not allowed') + end + it "does not allow import_url with invalid ports for new projects" do project = build(:project, import_url: 'http://github.com:25/t.git') @@ -2520,6 +2527,10 @@ describe Project do end context 'when the ref is not protected' do + before do + allow(project).to receive(:protected_for?).with('ref').and_return(false) + end + it 'contains only the CI variables' do is_expected.to contain_exactly(ci_variable) end @@ -2559,42 +2570,139 @@ describe Project do end describe '#protected_for?' do - let(:project) { create(:project) } + let(:project) { create(:project, :repository) } - subject { project.protected_for?('ref') } + subject { project.protected_for?(ref) } - context 'when the ref is not protected' do + shared_examples 'ref is not protected' do before do stub_application_setting( default_branch_protection: Gitlab::Access::PROTECTION_NONE) end it 'returns false' do - is_expected.to be_falsey + is_expected.to be false end end - context 'when the ref is a protected branch' do + shared_examples 'ref is protected branch' do before do - allow(project).to receive(:repository).and_call_original - allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(true) - create(:protected_branch, name: 'ref', project: project) + create(:protected_branch, name: 'master', project: project) end it 'returns true' do - is_expected.to be_truthy + is_expected.to be true end end - context 'when the ref is a protected tag' do + shared_examples 'ref is protected tag' do before do - allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(false) - allow(project).to receive_message_chain(:repository, :tag_exists?).and_return(true) - create(:protected_tag, name: 'ref', project: project) + create(:protected_tag, name: 'v1.0.0', project: project) end it 'returns true' do - is_expected.to be_truthy + is_expected.to be true + end + end + + context 'when ref is nil' do + let(:ref) { nil } + + it 'returns false' do + is_expected.to be false + end + end + + context 'when ref is ref name' do + context 'when ref is ambiguous' do + let(:ref) { 'ref' } + + before do + project.repository.add_branch(project.creator, 'ref', 'master') + project.repository.add_tag(project.creator, 'ref', 'master') + end + + it 'raises an error' do + expect { subject }.to raise_error(Repository::AmbiguousRefError) + end + end + + context 'when the ref is not protected' do + let(:ref) { 'master' } + + it_behaves_like 'ref is not protected' + end + + context 'when the ref is a protected branch' do + let(:ref) { 'master' } + + it_behaves_like 'ref is protected branch' + end + + context 'when the ref is a protected tag' do + let(:ref) { 'v1.0.0' } + + it_behaves_like 'ref is protected tag' + end + + context 'when ref does not exist' do + let(:ref) { 'something' } + + it 'returns false' do + is_expected.to be false + end + end + end + + context 'when ref is full ref' do + context 'when the ref is not protected' do + let(:ref) { 'refs/heads/master' } + + it_behaves_like 'ref is not protected' + end + + context 'when the ref is a protected branch' do + let(:ref) { 'refs/heads/master' } + + it_behaves_like 'ref is protected branch' + end + + context 'when the ref is a protected tag' do + let(:ref) { 'refs/tags/v1.0.0' } + + it_behaves_like 'ref is protected tag' + end + + context 'when branch ref name is a full tag ref' do + let(:ref) { 'refs/tags/something' } + + before do + project.repository.add_branch(project.creator, ref, 'master') + end + + context 'when ref is not protected' do + it 'returns false' do + is_expected.to be false + end + end + + context 'when ref is a protected branch' do + before do + create(:protected_branch, name: 'refs/tags/something', project: project) + end + + it 'returns true' do + is_expected.to be true + end + end + end + + context 'when ref does not exist' do + let(:ref) { 'refs/heads/something' } + + it 'returns false' do + is_expected.to be false + end end end end @@ -2814,7 +2922,7 @@ describe Project do it 'shows full error updating an invalid MR' do error_message = 'Failed to replace merge_requests because one or more of the new records could not be saved.'\ - ' Validate fork Source project is not a fork of the target project' + ' Validate fork Source project is not a fork of the target project' expect { project.append_or_update_attribute(:merge_requests, [create(:merge_request)]) } .to raise_error(ActiveRecord::RecordNotSaved, error_message) diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb index b12ca79847c..66a25ccb410 100644 --- a/spec/models/remote_mirror_spec.rb +++ b/spec/models/remote_mirror_spec.rb @@ -24,6 +24,20 @@ describe RemoteMirror do expect(remote_mirror).to be_invalid expect(remote_mirror.errors[:url].first).to include('Username needs to start with an alphanumeric character') end + + it 'does not allow url pointing to localhost' do + remote_mirror = build(:remote_mirror, url: 'http://127.0.0.2/t.git') + + expect(remote_mirror).to be_invalid + expect(remote_mirror.errors[:url].first).to include('Requests to loopback addresses are not allowed') + end + + it 'does not allow url pointing to the local network' do + remote_mirror = build(:remote_mirror, url: 'https://192.168.1.1') + + expect(remote_mirror).to be_invalid + expect(remote_mirror.errors[:url].first).to include('Requests to the local network are not allowed') + end end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index d6c5b9228c0..ac5874fd0f7 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1005,6 +1005,67 @@ describe Repository do end end + describe '#ambiguous_ref?' do + let(:ref) { 'ref' } + + subject { repository.ambiguous_ref?(ref) } + + context 'when ref is ambiguous' do + before do + repository.add_tag(project.creator, ref, 'master') + repository.add_branch(project.creator, ref, 'master') + end + + it 'should be true' do + is_expected.to eq(true) + end + end + + context 'when ref is not ambiguous' do + before do + repository.add_tag(project.creator, ref, 'master') + end + + it 'should be false' do + is_expected.to eq(false) + end + end + end + + describe '#expand_ref' do + let(:ref) { 'ref' } + + subject { repository.expand_ref(ref) } + + context 'when ref is not tag or branch name' do + let(:ref) { 'refs/heads/master' } + + it 'returns nil' do + is_expected.to eq(nil) + end + end + + context 'when ref is tag name' do + before do + repository.add_tag(project.creator, ref, 'master') + end + + it 'returns the tag ref' do + is_expected.to eq("refs/tags/#{ref}") + end + end + + context 'when ref is branch name' do + before do + repository.add_branch(project.creator, ref, 'master') + end + + it 'returns the branch ref' do + is_expected.to eq("refs/heads/#{ref}") + end + end + end + describe '#add_branch' do let(:branch_name) { 'new_feature' } let(:target) { 'master' } diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb index d1bf98995e7..db3df760472 100644 --- a/spec/policies/issuable_policy_spec.rb +++ b/spec/policies/issuable_policy_spec.rb @@ -7,6 +7,33 @@ describe IssuablePolicy, models: true do let(:policies) { described_class.new(user, issue) } describe '#rules' do + context 'when user is author of issuable' do + let(:merge_request) { create(:merge_request, source_project: project, author: user) } + let(:policies) { described_class.new(user, merge_request) } + + context 'when user is able to read project' do + it 'enables user to read and update issuables' do + expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request) + end + end + + context 'when project is private' do + let(:project) { create(:project, :private) } + + context 'when user belongs to the projects team' do + it 'enables user to read and update issuables' do + project.add_maintainer(user) + + expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request) + end + end + + it 'disallows user from reading and updating issuables from that project' do + expect(policies).to be_disallowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request) + end + end + end + context 'when discussion is locked for the issuable' do let(:issue) { create(:issue, project: project, discussion_locked: true) } diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index 8770365c893..402031075e7 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -142,10 +142,20 @@ describe API::Jobs do end context 'unauthorized user' do - let(:api_user) { nil } + context 'when user is not logged in' do + let(:api_user) { nil } - it 'does not return project jobs' do - expect(response).to have_gitlab_http_status(401) + it 'does not return project jobs' do + expect(response).to have_gitlab_http_status(401) + end + end + + context 'when user is guest' do + let(:api_user) { guest } + + it 'does not return project jobs' do + expect(response).to have_gitlab_http_status(403) + end end end @@ -241,10 +251,20 @@ describe API::Jobs do end context 'unauthorized user' do - let(:api_user) { nil } + context 'when user is not logged in' do + let(:api_user) { nil } - it 'does not return jobs' do - expect(response).to have_gitlab_http_status(401) + it 'does not return jobs' do + expect(response).to have_gitlab_http_status(401) + end + end + + context 'when user is guest' do + let(:api_user) { guest } + + it 'does not return jobs' do + expect(response).to have_gitlab_http_status(403) + end end end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index b36087b86a7..fb460466a6a 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -441,9 +441,11 @@ describe API::Runner, :clean_gitlab_redis_shared_state do it 'picks a job' do request_job info: { platform: :darwin } + runner.reload + expect(response).to have_gitlab_http_status(201) expect(response.headers).not_to have_key('X-GitLab-Last-Update') - expect(runner.reload.platform).to eq('darwin') + expect(runner.platform).to eq('darwin') expect(json_response['id']).to eq(job.id) expect(json_response['token']).to eq(job.token) expect(json_response['job_info']).to eq(expected_job_info) @@ -537,8 +539,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(json_response['id']).to eq(test_job.id) expect(json_response['dependencies'].count).to eq(2) expect(json_response['dependencies']).to include( - { 'id' => job.id, 'name' => job.name, 'token' => job.token }, - { 'id' => job2.id, 'name' => job2.name, 'token' => job2.token }) + { 'id' => job.id, 'name' => job.name, 'token' => test_job.token }, + { 'id' => job2.id, 'name' => job2.name, 'token' => test_job.token }) end end @@ -557,7 +559,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(json_response['id']).to eq(test_job.id) expect(json_response['dependencies'].count).to eq(1) expect(json_response['dependencies']).to include( - { 'id' => job.id, 'name' => job.name, 'token' => job.token, + { 'id' => job.id, 'name' => job.name, 'token' => test_job.token, 'artifacts_file' => { 'filename' => 'ci_build_artifacts.zip', 'size' => 106365 } }) end end @@ -582,7 +584,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(response).to have_gitlab_http_status(201) expect(json_response['id']).to eq(test_job.id) expect(json_response['dependencies'].count).to eq(1) - expect(json_response['dependencies'][0]).to include('id' => job2.id, 'name' => job2.name, 'token' => job2.token) + expect(json_response['dependencies'][0]).to include( + 'id' => job2.id, 'name' => job2.name, 'token' => test_job.token) end end @@ -995,7 +998,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do patch_the_trace end - it 'returns Forbidden ' do + it 'returns Forbidden' do expect(response.status).to eq(403) end end @@ -1048,11 +1051,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do context 'when the job is canceled' do before do - job.cancel + job.cancel! patch_the_trace end - it 'receives status in header' do + it 'responds with forbidden and status in header' do + expect(response).to have_gitlab_http_status(403) expect(response.header['Job-Status']).to eq 'canceled' end end @@ -1223,7 +1227,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do it 'fails to authorize artifacts posting' do authorize_artifacts(token: job.project.runners_token) - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(404) end end @@ -1236,10 +1240,10 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end context 'authorization token is invalid' do - it 'responds with forbidden' do + it 'responds with not found' do authorize_artifacts(token: 'invalid', filesize: 100 ) - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(404) end end @@ -1272,9 +1276,21 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end it 'responds with forbidden' do + expect(response).to have_gitlab_http_status(403) + end + end + + context 'when job has been canceled' do + let(:job) { create(:ci_build) } + + before do + job.cancel! upload_artifacts(file_upload, headers_with_token) + end + it 'responds with forbidden' do expect(response).to have_gitlab_http_status(403) + expect(response.header['Job-Status']).to eq('canceled') end end @@ -1327,10 +1343,10 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end context 'when using runners token' do - it 'responds with forbidden' do + it 'responds with not found' do upload_artifacts(file_upload, headers.merge(API::Helpers::Runner::JOB_TOKEN_HEADER => job.project.runners_token)) - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(404) end end end @@ -1550,10 +1566,13 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end describe 'GET /api/v4/jobs/:id/artifacts' do - let(:token) { job.token } + let(:project) { create(:project) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } + let(:running_job) { create(:ci_build, :running, pipeline: pipeline) } + let(:token) { running_job.token } context 'when job has artifacts' do - let(:job) { create(:ci_build) } + let(:job) { create(:ci_build, pipeline: pipeline) } let(:store) { JobArtifactUploader::Store::LOCAL } before do @@ -1579,7 +1598,6 @@ describe API::Runner, :clean_gitlab_redis_shared_state do context 'when artifacts are stored remotely' do let(:store) { JobArtifactUploader::Store::REMOTE } - let!(:job) { create(:ci_build) } context 'when proxy download is being used' do before do @@ -1606,6 +1624,30 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end end + context 'when using running token from another pipeline' do + let(:running_job) { create(:ci_build, :running, project: project) } + + before do + download_artifact + end + + it 'responds with not found' do + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when using running token from another project' do + let(:running_job) { create(:ci_build, :running) } + + before do + download_artifact + end + + it 'responds with not found' do + expect(response).to have_gitlab_http_status(404) + end + end + context 'when using runnners token' do let(:token) { job.project.runners_token } @@ -1613,8 +1655,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do download_artifact end - it 'responds with forbidden' do - expect(response).to have_gitlab_http_status(403) + it 'responds with not found' do + expect(response).to have_gitlab_http_status(404) end end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index ccc6b0ef1c7..02fde579f6f 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -667,7 +667,7 @@ describe Ci::CreatePipelineService do stub_ci_pipeline_yaml_file(YAML.dump(config)) end - let(:ref_name) { 'feature' } + let(:ref_name) { 'refs/heads/feature' } context 'when source is merge request' do let(:source) { :merge_request } @@ -696,7 +696,7 @@ describe Ci::CreatePipelineService do let(:merge_request) do create(:merge_request, source_project: project, - source_branch: ref_name, + source_branch: Gitlab::Git.ref_name(ref_name), target_project: project, target_branch: 'master') end @@ -709,7 +709,7 @@ describe Ci::CreatePipelineService do end context 'when ref is tag' do - let(:ref_name) { 'v1.1.0' } + let(:ref_name) { 'refs/tags/v1.1.0' } it 'does not create a merge request pipeline' do expect(pipeline).not_to be_persisted @@ -721,7 +721,7 @@ describe Ci::CreatePipelineService do let(:merge_request) do create(:merge_request, source_project: project, - source_branch: ref_name, + source_branch: Gitlab::Git.ref_name(ref_name), target_project: target_project, target_branch: 'master') end @@ -786,7 +786,7 @@ describe Ci::CreatePipelineService do let(:merge_request) do create(:merge_request, source_project: project, - source_branch: ref_name, + source_branch: Gitlab::Git.ref_name(ref_name), target_project: project, target_branch: 'master') end @@ -839,7 +839,7 @@ describe Ci::CreatePipelineService do let(:merge_request) do create(:merge_request, source_project: project, - source_branch: ref_name, + source_branch: Gitlab::Git.ref_name(ref_name), target_project: project, target_branch: 'master') end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 84cfa53ea05..d87a7dd234d 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -56,7 +56,7 @@ describe Groups::UpdateService do create(:project, :private, group: internal_group) expect(TodosDestroyer::GroupPrivateWorker).to receive(:perform_in) - .with(1.hour, internal_group.id) + .with(Todo::WAIT_FOR_DELETE, internal_group.id) end it "changes permission level to private" do diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index f0b0f7956ce..ca366cdf1df 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -28,6 +28,33 @@ describe Issuable::BulkUpdateService do expect(project.issues.opened).to be_empty expect(project.issues.closed).not_to be_empty end + + context 'when issue for a different project is created' do + let(:private_project) { create(:project, :private) } + let(:issue) { create(:issue, project: private_project, author: user) } + + context 'when user has access to the project' do + it 'closes all issues passed' do + private_project.add_maintainer(user) + + bulk_update(issues + [issue], state_event: 'close') + + expect(project.issues.opened).to be_empty + expect(project.issues.closed).not_to be_empty + expect(private_project.issues.closed).not_to be_empty + end + end + + context 'when user does not have access to project' do + it 'only closes all issues that the user has access to' do + bulk_update(issues + [issue], state_event: 'close') + + expect(project.issues.opened).to be_empty + expect(project.issues.closed).not_to be_empty + expect(private_project.issues.closed).to be_empty + end + end + end end describe 'reopen issues' do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index bd519e7f077..ce20bf2bef6 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -77,7 +77,7 @@ describe Issues::UpdateService, :mailer do end it 'enqueues ConfidentialIssueWorker when an issue is made confidential' do - expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(1.hour, issue.id) + expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, issue.id) update_issue(confidential: true) end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 36cf82859e2..e872a537761 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -22,7 +22,7 @@ describe Members::DestroyService do shared_examples 'a service destroying a member' do before do type = member.is_a?(GroupMember) ? 'Group' : 'Project' - expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(1.hour, member.user_id, member.source_id, type) + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type) end it 'destroys the member' do diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index 6d19a95ffeb..599ed39ca37 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -20,11 +20,28 @@ describe Members::UpdateService do shared_examples 'a service updating a member' do it 'updates the member' do + expect(TodosDestroyer::EntityLeaveWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name) + updated_member = described_class.new(current_user, params).execute(member, permission: permission) expect(updated_member).to be_valid expect(updated_member.access_level).to eq(Gitlab::Access::MAINTAINER) end + + context 'when member is downgraded to guest' do + let(:params) do + { access_level: Gitlab::Access::GUEST } + end + + it 'schedules to delete confidential todos' do + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + + updated_member = described_class.new(current_user, params).execute(member, permission: permission) + + expect(updated_member).to be_valid + expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) + end + end end before do diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 1894d8c8d0e..536d0d345a4 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe MergeRequests::BuildService do using RSpec::Parameterized::TableSyntax include RepoHelpers + include ProjectForksHelper let(:project) { create(:project, :repository) } let(:source_project) { nil } @@ -49,7 +50,7 @@ describe MergeRequests::BuildService do describe '#execute' do it 'calls the compare service with the correct arguments' do - allow_any_instance_of(described_class).to receive(:branches_valid?).and_return(true) + allow_any_instance_of(described_class).to receive(:projects_and_branches_valid?).and_return(true) expect(CompareService).to receive(:new) .with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch) .and_call_original @@ -393,11 +394,27 @@ describe MergeRequests::BuildService do end end + context 'target_project is set but repo is not accessible by current_user' do + let(:target_project) do + create(:project, :public, :repository, repository_access_level: ProjectFeature::PRIVATE) + end + + it 'sets target project correctly' do + expect(merge_request.target_project).to eq(project) + end + end + context 'source_project is set and accessible by current_user' do let(:source_project) { create(:project, :public, :repository)} let(:commits) { Commit.decorate([commit_1], project) } - it 'sets target project correctly' do + before do + # To create merge requests _from_ a project the user needs at least + # developer access + source_project.add_developer(user) + end + + it 'sets source project correctly' do expect(merge_request.source_project).to eq(source_project) end end @@ -406,11 +423,43 @@ describe MergeRequests::BuildService do let(:source_project) { create(:project, :private, :repository)} let(:commits) { Commit.decorate([commit_1], project) } - it 'sets target project correctly' do + it 'sets source project correctly' do + expect(merge_request.source_project).to eq(project) + end + end + + context 'source_project is set but the user cannot create merge requests from the project' do + let(:source_project) do + create(:project, :public, :repository, merge_requests_access_level: ProjectFeature::PRIVATE) + end + + it 'sets the source_project correctly' do expect(merge_request.source_project).to eq(project) end end + context 'target_project is not in the fork network of source_project' do + let(:target_project) { create(:project, :public, :repository) } + + it 'adds an error to the merge request' do + expect(merge_request.errors[:validate_fork]).to contain_exactly('Source project is not a fork of the target project') + end + end + + context 'target_project is in the fork network of source project but no longer accessible' do + let!(:project) { fork_project(target_project, user, namespace: user.namespace, repository: true) } + let(:source_project) { project } + let(:target_project) { create(:project, :public, :repository) } + + before do + target_project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + it 'sets the target_project correctly' do + expect(merge_request.target_project).to eq(project) + end + end + context 'when specifying target branch in the description' do let(:description) { "A merge request targeting another branch\n\n/target_branch with-codeowners" } diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 6af5bfc7689..95c9b6e63b8 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -4,17 +4,15 @@ describe Projects::LfsPointers::LfsDownloadService do let(:project) { create(:project) } let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' } let(:download_link) { "http://gitlab.com/#{oid}" } - let(:lfs_content) do - <<~HEREDOC - whatever - HEREDOC - end + let(:lfs_content) { SecureRandom.random_bytes(10) } subject { described_class.new(project) } before do allow(project).to receive(:lfs_enabled?).and_return(true) WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false) end describe '#execute' do @@ -32,7 +30,7 @@ describe Projects::LfsPointers::LfsDownloadService do it 'stores the content' do subject.execute(oid, download_link) - expect(File.read(LfsObject.first.file.file.file)).to eq lfs_content + expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content end end @@ -54,6 +52,61 @@ describe Projects::LfsPointers::LfsDownloadService do end end + context 'when localhost requests are allowed' do + let(:download_link) { 'http://192.168.2.120' } + + before do + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true) + end + + it 'downloads the file' do + expect(subject).to receive(:download_and_save_file).and_call_original + + expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.by(1) + end + end + + context 'when a bad URL is used' do + where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120']) + + with_them do + it 'does not download the file' do + expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } + end + end + end + + context 'when the URL points to a redirected URL' do + context 'that is blocked' do + where(redirect_link: ['ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120']) + + with_them do + before do + WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) + end + + it 'does not follow the redirection' do + expect(Rails.logger).to receive(:error).with(/LFS file with oid #{oid} couldn't be downloaded/) + + expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } + end + end + end + + context 'that is valid' do + let(:redirect_link) { "http://example.com/"} + + before do + WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) + WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content) + end + + it 'follows the redirection' do + expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1) + end + end + end + context 'when an lfs object with the same oid already exists' do before do create(:lfs_object, oid: 'oid') diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index d58ff2cedc0..8adfc63222e 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -41,7 +41,7 @@ describe Projects::UpdateService do end it 'updates the project to private' do - expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(1.hour, project.id) + expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) @@ -191,7 +191,7 @@ describe Projects::UpdateService do context 'when changing feature visibility to private' do it 'updates the visibility correctly' do expect(TodosDestroyer::PrivateFeaturesWorker) - .to receive(:perform_in).with(1.hour, project.id) + .to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) result = update_project(project, user, project_feature_attributes: { issues_access_level: ProjectFeature::PRIVATE } diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index c52515aefd8..253f2e44d10 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -19,6 +19,7 @@ describe TodoService do before do project.add_guest(guest) project.add_developer(author) + project.add_developer(assignee) project.add_developer(member) project.add_developer(john_doe) project.add_developer(skipped) |