diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-11-29 13:47:43 +0000 |
---|---|---|
committer | Alejandro RodrÃguez <alejorro70@gmail.com> | 2016-12-08 21:42:07 -0300 |
commit | f23b1cb453deea2659c0cb9e9047c72d859bbf9d (patch) | |
tree | addf2e0a54980a21a30049b4d9aa9df7ca773508 /spec | |
parent | edf7dbfacd5a6b884ae1af72204e3718e89f3c35 (diff) | |
download | gitlab-ce-f23b1cb453deea2659c0cb9e9047c72d859bbf9d.tar.gz |
Merge branch 'jej-23867-use-mr-finder-instead-of-access-check' into 'security'
Replace MR access checks with use of MergeRequestsFinder
Split from !2024 to partially solve https://gitlab.com/gitlab-org/gitlab-ce/issues/23867
:warning: - Potentially untested
:bomb: - No test coverage
:traffic_light: - Test coverage of some sort exists (a test failed when error raised)
:vertical_traffic_light: - Test coverage of return value (a test failed when nil used)
:white_check_mark: - Permissions check tested
- [x] :bomb: app/finders/notes_finder.rb:17
- [x] :warning: app/views/layouts/nav/_project.html.haml:80 [`.count`]
- [x] :bomb: app/controllers/concerns/creates_commit.rb:84
- [x] :traffic_light: app/controllers/projects/commits_controller.rb:24
- [x] :traffic_light: app/controllers/projects/compare_controller.rb:56
- [x] :vertical_traffic_light: app/controllers/projects/discussions_controller.rb:29
- [x] :white_check_mark: app/controllers/projects/todos_controller.rb:27
- [x] :vertical_traffic_light: app/models/commit.rb:268
- [x] :white_check_mark: lib/gitlab/search_results.rb:71
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_267_266 Memoize ` merged_merge_request(current_user)`
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_248_247 Expected side effect for `merged_merge_request!`, consider `skip_authorization: true`.
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_269_269 Scary use of unchecked `merged_merge_request?`
See merge request !2033
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/projects/todo_controller_spec.rb | 15 | ||||
-rw-r--r-- | spec/lib/gitlab/search_results_spec.rb | 16 | ||||
-rw-r--r-- | spec/models/commit_range_spec.rb | 15 | ||||
-rw-r--r-- | spec/models/commit_spec.rb | 11 |
4 files changed, 43 insertions, 14 deletions
diff --git a/spec/controllers/projects/todo_controller_spec.rb b/spec/controllers/projects/todo_controller_spec.rb index 193a3f6b5a3..415c264e0dd 100644 --- a/spec/controllers/projects/todo_controller_spec.rb +++ b/spec/controllers/projects/todo_controller_spec.rb @@ -110,7 +110,7 @@ describe Projects::TodosController do end end - context 'when not authorized' do + context 'when not authorized for project' do it 'does not create todo for merge request user has no access to' do sign_in(user) expect do @@ -128,6 +128,19 @@ describe Projects::TodosController do expect(response).to have_http_status(302) end end + + context 'when not authorized for merge_request' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE) + sign_in(user) + end + + it "doesn't create todo" do + expect{ go }.not_to change { user.todos.count } + expect(response).to have_http_status(404) + end + end end end end diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb index f23e3522625..9614aad3e73 100644 --- a/spec/lib/gitlab/search_results_spec.rb +++ b/spec/lib/gitlab/search_results_spec.rb @@ -40,6 +40,15 @@ describe Gitlab::SearchResults do expect(results.milestones_count).to eq(1) end end + + it 'includes merge requests from source and target projects' do + forked_project = create(:empty_project, forked_from_project: project) + merge_request_2 = create(:merge_request, target_project: project, source_project: forked_project, title: 'foo') + + results = described_class.new(user, Project.where(id: forked_project.id), 'foo') + + expect(results.objects('merge_requests')).to include merge_request_2 + end end it 'does not list issues on private projects' do @@ -152,4 +161,11 @@ describe Gitlab::SearchResults do expect(results.issues_count).to eq 5 end end + + it 'does not list merge requests on projects with limited access' do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE) + + expect(results.objects('merge_requests')).not_to include merge_request + end end diff --git a/spec/models/commit_range_spec.rb b/spec/models/commit_range_spec.rb index d89d4342dea..30782ca75a0 100644 --- a/spec/models/commit_range_spec.rb +++ b/spec/models/commit_range_spec.rb @@ -137,26 +137,25 @@ describe CommitRange, models: true do end describe '#has_been_reverted?' do - it 'returns true if the commit has been reverted' do - issue = create(:issue) + let(:issue) { create(:issue) } + let(:user) { issue.author } + it 'returns true if the commit has been reverted' do create(:note_on_issue, noteable: issue, system: true, - note: commit1.revert_description, + note: commit1.revert_description(user), project: issue.project) expect_any_instance_of(Commit).to receive(:reverts_commit?). - with(commit1). + with(commit1, user). and_return(true) - expect(commit1.has_been_reverted?(nil, issue)).to eq(true) + expect(commit1.has_been_reverted?(user, issue)).to eq(true) end it 'returns false a commit has not been reverted' do - issue = create(:issue) - - expect(commit1.has_been_reverted?(nil, issue)).to eq(false) + expect(commit1.has_been_reverted?(user, issue)).to eq(false) end end end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index eb482c7f913..0935fc0561c 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -179,25 +179,26 @@ eos describe '#reverts_commit?' do let(:another_commit) { double(:commit, revert_description: "This reverts commit #{commit.sha}") } + let(:user) { commit.author } - it { expect(commit.reverts_commit?(another_commit)).to be_falsy } + it { expect(commit.reverts_commit?(another_commit, user)).to be_falsy } context 'commit has no description' do before { allow(commit).to receive(:description?).and_return(false) } - it { expect(commit.reverts_commit?(another_commit)).to be_falsy } + it { expect(commit.reverts_commit?(another_commit, user)).to be_falsy } end context "another_commit's description does not revert commit" do before { allow(commit).to receive(:description).and_return("Foo Bar") } - it { expect(commit.reverts_commit?(another_commit)).to be_falsy } + it { expect(commit.reverts_commit?(another_commit, user)).to be_falsy } end context "another_commit's description reverts commit" do before { allow(commit).to receive(:description).and_return("Foo #{another_commit.revert_description} Bar") } - it { expect(commit.reverts_commit?(another_commit)).to be_truthy } + it { expect(commit.reverts_commit?(another_commit, user)).to be_truthy } end context "another_commit's description reverts merged merge request" do @@ -207,7 +208,7 @@ eos allow(commit).to receive(:description).and_return("Foo #{another_commit.revert_description} Bar") end - it { expect(commit.reverts_commit?(another_commit)).to be_truthy } + it { expect(commit.reverts_commit?(another_commit, user)).to be_truthy } end end |