From 23ffc95c794126ae4039ab6a209f90c6df03d14a Mon Sep 17 00:00:00 2001
From: Valerie Burton <vburton@gitlab.com>
Date: Tue, 9 Aug 2022 12:27:04 -0500
Subject: [PATCH] Inherited permissions tests for merge requests
Refactors and adds new specs to test inherited permissions for MRs
---
.../user_creates_merge_request_spec.rb | 183 ++++++---
.../mutations/merge_requests/create_spec.rb | 119 ++++--
spec/policies/merge_request_policy_spec.rb | 374 ++++++++++++------
spec/requests/api/merge_requests_spec.rb | 91 +++++
4 files changed, 537 insertions(+), 230 deletions(-)
diff --git a/spec/features/merge_request/user_creates_merge_request_spec.rb b/spec/features/merge_request/user_creates_merge_request_spec.rb
index c8b22bb31253f..0ae4ef186491d 100644
--- a/spec/features/merge_request/user_creates_merge_request_spec.rb
+++ b/spec/features/merge_request/user_creates_merge_request_spec.rb
@@ -1,111 +1,164 @@
# frozen_string_literal: true
-require "spec_helper"
+require 'spec_helper'
-RSpec.describe "User creates a merge request", :js do
+RSpec.describe 'User creates a merge request', :js do
include ProjectForksHelper
- let_it_be(:project) { create(:project, :repository) }
- let_it_be(:user) { create(:user) }
-
- let(:title) { "Some feature" }
-
- before do
- project.add_maintainer(user)
- sign_in(user)
- end
-
- it "creates a merge request" do
- visit(project_new_merge_request_path(project))
+ shared_examples 'creates a merge request' do
+ specify do
+ visit(project_new_merge_request_path(project))
- find(".js-source-branch").click
- click_link("fix")
+ compare_source_and_target('fix', 'feature')
- find(".js-target-branch").click
- click_link("feature")
+ page.within('.merge-request-form') do
+ expect(page.find('#merge_request_description')['placeholder']).to eq 'Describe the goal of the changes and what reviewers should be aware of.'
+ end
- click_button("Compare branches")
+ fill_in('Title', with: title)
+ click_button('Create merge request')
- page.within('.merge-request-form') do
- expect(page.find('#merge_request_description')['placeholder']).to eq 'Describe the goal of the changes and what reviewers should be aware of.'
+ page.within('.merge-request') do
+ expect(page).to have_content(title)
+ end
end
+ end
- fill_in("Title", with: title)
- click_button("Create merge request")
+ shared_examples 'renders not found' do
+ specify do
+ visit project_new_merge_request_path(project)
- page.within(".merge-request") do
- expect(page).to have_content(title)
+ expect(page).to have_title('Not Found')
+ expect(page).to have_content('Page Not Found')
end
end
- context "XSS branch name exists" do
+ context 'when user is a direct project member' do
+ let_it_be(:project) { create(:project, :repository) }
+ let_it_be(:user) { create(:user) }
+
+ let(:title) { 'Some feature' }
+
before do
- project.repository.create_branch("<img/src='x'/onerror=alert('oops')>", "master")
+ project.add_maintainer(user)
+ sign_in(user)
end
- it "doesn't execute the dodgy branch name" do
- visit(project_new_merge_request_path(project))
+ it_behaves_like 'creates a merge request'
- find(".js-source-branch").click
- click_link("<img/src='x'/onerror=alert('oops')>")
+ context 'with XSS branch name' do
+ before do
+ project.repository.create_branch("<img/src='x'/onerror=alert('oops')>", 'master')
+ end
- find(".js-target-branch").click
- click_link("feature")
+ it 'does not execute the suspicious branch name' do
+ visit(project_new_merge_request_path(project))
- click_button("Compare branches")
+ compare_source_and_target("<img/src='x'/onerror=alert('oops')>", 'feature')
- expect { page.driver.browser.switch_to.alert }.to raise_error(Selenium::WebDriver::Error::NoSuchAlertError)
+ expect { page.driver.browser.switch_to.alert }.to raise_error(Selenium::WebDriver::Error::NoSuchAlertError)
+ end
end
- end
- context "to a forked project" do
- let(:forked_project) { fork_project(project, user, namespace: user.namespace, repository: true) }
+ context 'to a forked project' do
+ let(:forked_project) { fork_project(project, user, namespace: user.namespace, repository: true) }
+
+ it 'creates a merge request', :sidekiq_might_not_need_inline do
+ visit(project_new_merge_request_path(forked_project))
+
+ expect(page).to have_content('Source branch').and have_content('Target branch')
+ expect(find('#merge_request_target_project_id', visible: false).value).to eq(project.id.to_s)
- it "creates a merge request", :sidekiq_might_not_need_inline do
- visit(project_new_merge_request_path(forked_project))
+ click_button('Compare branches and continue')
- expect(page).to have_content("Source branch").and have_content("Target branch")
- expect(find("#merge_request_target_project_id", visible: false).value).to eq(project.id.to_s)
+ expect(page).to have_content('You must select source and target branch')
- click_button("Compare branches and continue")
+ first('.js-source-project').click
+ first('.dropdown-source-project a', text: forked_project.full_path)
- expect(page).to have_content("You must select source and target branch")
+ first('.js-target-project').click
+ first('.dropdown-target-project a', text: project.full_path)
- first(".js-source-project").click
- first(".dropdown-source-project a", text: forked_project.full_path)
+ first('.js-source-branch').click
- first(".js-target-project").click
- first(".dropdown-target-project a", text: project.full_path)
+ wait_for_requests
- first(".js-source-branch").click
+ source_branch = 'fix'
- wait_for_requests
+ first('.js-source-branch-dropdown .dropdown-content a', text: source_branch).click
- source_branch = "fix"
+ click_button('Compare branches and continue')
- first(".js-source-branch-dropdown .dropdown-content a", text: source_branch).click
+ expect(page).to have_text _('New merge request')
- click_button("Compare branches and continue")
+ page.within('form#new_merge_request') do
+ fill_in('Title', with: title)
+ end
- expect(page).to have_text _('New merge request')
+ expect(find('.js-assignee-search')['data-project-id']).to eq(project.id.to_s)
+ find('.js-assignee-search').click
- page.within("form#new_merge_request") do
- fill_in("Title", with: title)
+ page.within('.dropdown-menu-user') do
+ expect(page).to have_content('Unassigned')
+ .and have_content(user.name)
+ .and have_content(project.users.first.name)
+ end
+ find('.js-assignee-search').click
+
+ click_button('Create merge request')
+
+ expect(page).to have_content(title).and have_content("requested to merge #{forked_project.full_path}:#{source_branch} into master")
+ end
+ end
+ end
+
+ context 'when user is an inherited member from the group' do
+ let_it_be(:group) { create(:group, :public) }
+
+ let(:user) { create(:user) }
+
+ context 'when project is public and merge requests are private' do
+ let_it_be(:project) do
+ create(:project,
+ :public,
+ :repository,
+ group: group,
+ merge_requests_access_level: ProjectFeature::DISABLED)
end
- expect(find(".js-assignee-search")["data-project-id"]).to eq(project.id.to_s)
- find('.js-assignee-search').click
+ context 'and user is a guest' do
+ before do
+ group.add_guest(user)
+ sign_in(user)
+ end
- page.within(".dropdown-menu-user") do
- expect(page).to have_content("Unassigned")
- .and have_content(user.name)
- .and have_content(project.users.first.name)
+ it_behaves_like 'renders not found'
end
- find('.js-assignee-search').click
+ end
+
+ context 'when project is private' do
+ let_it_be(:project) { create(:project, :private, :repository, group: group) }
- click_button("Create merge request")
+ context 'and user is a guest' do
+ before do
+ group.add_guest(user)
+ sign_in(user)
+ end
- expect(page).to have_content(title).and have_content("requested to merge #{forked_project.full_path}:#{source_branch} into master")
+ it_behaves_like 'renders not found'
+ end
end
end
+
+ private
+
+ def compare_source_and_target(source_branch, target_branch)
+ find('.js-source-branch').click
+ click_link(source_branch)
+
+ find('.js-target-branch').click
+ click_link(target_branch)
+
+ click_button('Compare branches')
+ end
end
diff --git a/spec/graphql/mutations/merge_requests/create_spec.rb b/spec/graphql/mutations/merge_requests/create_spec.rb
index e1edb60e4ff96..6e593a5f4bef0 100644
--- a/spec/graphql/mutations/merge_requests/create_spec.rb
+++ b/spec/graphql/mutations/merge_requests/create_spec.rb
@@ -7,8 +7,7 @@
subject(:mutation) { described_class.new(object: nil, context: context, field: nil) }
- let_it_be(:project) { create(:project, :public, :repository) }
- let_it_be(:user) { create(:user) }
+ let(:user) { create(:user) }
let(:context) do
GraphQL::Query::Context.new(
@@ -38,62 +37,106 @@
let(:mutated_merge_request) { subject[:merge_request] }
- it 'raises an error if the resource is not accessible to the user' do
- expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+ shared_examples 'resource not available' do
+ it 'raises an error' do
+ expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+ end
end
- context 'when user does not have enough permissions to create a merge request' do
- before do
- project.add_guest(user)
- end
+ context 'when user is not a project member' do
+ let_it_be(:project) { create(:project, :public, :repository) }
- it 'raises an error if the resource is not accessible to the user' do
- expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
- end
+ it_behaves_like 'resource not available'
end
- context 'when the user can create a merge request' do
- before_all do
- project.add_developer(user)
- end
+ context 'when user is a direct project member' do
+ let_it_be(:project) { create(:project, :public, :repository) }
- it 'creates a new merge request' do
- expect { mutated_merge_request }.to change(MergeRequest, :count).by(1)
- end
+ context 'and user is a guest' do
+ before do
+ project.add_guest(user)
+ end
- it 'returns a new merge request' do
- expect(mutated_merge_request.title).to eq(title)
- expect(subject[:errors]).to be_empty
+ it_behaves_like 'resource not available'
end
- context 'when optional description field is set' do
- let(:description) { 'content' }
+ context 'and user is a developer' do
+ before do
+ project.add_developer(user)
+ end
+
+ it 'creates a new merge request' do
+ expect { mutated_merge_request }.to change(MergeRequest, :count).by(1)
+ end
- it 'returns a new merge request with a description' do
- expect(mutated_merge_request.description).to eq(description)
+ it 'returns a new merge request' do
+ expect(mutated_merge_request.title).to eq(title)
expect(subject[:errors]).to be_empty
end
- end
- context 'when optional labels field is set' do
- let(:labels) { %w[label-1 label-2] }
+ context 'when optional description field is set' do
+ let(:description) { 'content' }
- it 'returns a new merge request with labels' do
- expect(mutated_merge_request.labels.map(&:title)).to eq(labels)
- expect(subject[:errors]).to be_empty
+ it 'returns a new merge request with a description' do
+ expect(mutated_merge_request.description).to eq(description)
+ expect(subject[:errors]).to be_empty
+ end
+ end
+
+ context 'when optional labels field is set' do
+ let(:labels) { %w[label-1 label-2] }
+
+ it 'returns a new merge request with labels' do
+ expect(mutated_merge_request.labels.map(&:title)).to eq(labels)
+ expect(subject[:errors]).to be_empty
+ end
+ end
+
+ context 'when service cannot create a merge request' do
+ let(:title) { nil }
+
+ it 'does not create a new merge request' do
+ expect { mutated_merge_request }.not_to change(MergeRequest, :count)
+ end
+
+ it 'returns errors' do
+ expect(mutated_merge_request).to be_nil
+ expect(subject[:errors]).to match_array(['Title can\'t be blank'])
+ end
end
end
+ end
- context 'when service cannot create a merge request' do
- let(:title) { nil }
+ context 'when user is an inherited member from the group' do
+ let_it_be(:group) { create(:group, :public) }
- it 'does not create a new merge request' do
- expect { mutated_merge_request }.not_to change(MergeRequest, :count)
+ context 'when project is public with private merge requests' do
+ let_it_be(:project) do
+ create(:project,
+ :public,
+ :repository,
+ group: group,
+ merge_requests_access_level: ProjectFeature::DISABLED)
end
- it 'returns errors' do
- expect(mutated_merge_request).to be_nil
- expect(subject[:errors]).to eq(['Title can\'t be blank'])
+ context 'and user is a guest' do
+ before do
+ group.add_guest(user)
+ end
+
+ it_behaves_like 'resource not available'
+ end
+ end
+
+ context 'when project is private' do
+ let_it_be(:project) { create(:project, :private, :repository, group: group) }
+
+ context 'and user is a guest' do
+ before do
+ group.add_guest(user)
+ end
+
+ it_behaves_like 'resource not available'
end
end
end
diff --git a/spec/policies/merge_request_policy_spec.rb b/spec/policies/merge_request_policy_spec.rb
index dd42e1b931343..7e1af132b1d14 100644
--- a/spec/policies/merge_request_policy_spec.rb
+++ b/spec/policies/merge_request_policy_spec.rb
@@ -7,24 +7,18 @@
let_it_be(:guest) { create(:user) }
let_it_be(:author) { create(:user) }
+ let_it_be(:reporter) { create(:user) }
let_it_be(:developer) { create(:user) }
let_it_be(:non_team_member) { create(:user) }
- let(:project) { create(:project, :public) }
-
def permissions(user, merge_request)
described_class.new(user, merge_request)
end
- before do
- project.add_guest(guest)
- project.add_guest(author)
- project.add_developer(developer)
- end
-
mr_perms = %i[create_merge_request_in
create_merge_request_from
read_merge_request
+ update_merge_request
create_todo
approve_merge_request
create_note
@@ -40,7 +34,28 @@ def permissions(user, merge_request)
end
end
- shared_examples_for 'a user with access' do
+ shared_examples_for 'a user with reporter access' do
+ using RSpec::Parameterized::TableSyntax
+
+ where(:policy, :is_allowed) do
+ :create_merge_request_in | true
+ :read_merge_request | true
+ :create_todo | true
+ :create_note | true
+ :update_subscription | true
+ :create_merge_request_from | false
+ :approve_merge_request | false
+ :update_merge_request | false
+ end
+
+ with_them do
+ specify do
+ is_allowed ? (is_expected.to be_allowed(policy)) : (is_expected.to be_disallowed(policy))
+ end
+ end
+ end
+
+ shared_examples_for 'a user with full access' do
let(:perms) { permissions(subject, merge_request) }
mr_perms.each do |thing|
@@ -50,199 +65,304 @@ def permissions(user, merge_request)
end
end
- context 'when merge request is public' do
- let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: user) }
- let(:user) { author }
-
- context 'and user is anonymous' do
- subject { permissions(nil, merge_request) }
+ context 'when user is a direct project member' do
+ let(:project) { create(:project, :public) }
- it do
- is_expected.to be_disallowed(:create_todo, :update_subscription)
- end
+ before do
+ project.add_guest(guest)
+ project.add_guest(author)
+ project.add_developer(developer)
end
- context 'and user is author' do
- subject { permissions(user, merge_request) }
+ context 'when merge request is public' do
+ let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: user) }
+ let(:user) { author }
- context 'and the user is a guest' do
- let(:user) { guest }
+ context 'and user is author' do
+ subject { permissions(user, merge_request) }
- it do
- is_expected.to be_allowed(:update_merge_request)
- end
+ context 'and the user is a guest' do
+ let(:user) { guest }
- it do
- is_expected.to be_allowed(:reopen_merge_request)
- end
+ it do
+ is_expected.to be_allowed(:update_merge_request)
+ end
- it do
- is_expected.to be_allowed(:approve_merge_request)
+ it do
+ is_expected.to be_allowed(:reopen_merge_request)
+ end
+
+ it do
+ is_expected.to be_allowed(:approve_merge_request)
+ end
end
end
+ end
- context 'and the user is a group member' do
- let(:project) { create(:project, :public, group: group) }
- let(:group) { create(:group) }
- let(:user) { non_team_member }
-
- before do
- group.add_guest(non_team_member)
- end
+ context 'when merge requests have been disabled' do
+ let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) }
- it do
- is_expected.to be_allowed(:approve_merge_request)
- end
+ before do
+ project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED)
end
- context 'and the user is a member of a shared group' do
- let(:user) { non_team_member }
+ describe 'the author' do
+ subject { author }
- before do
- group = create(:group)
- project.project_group_links.create!(
- group: group,
- group_access: Gitlab::Access::DEVELOPER)
+ it_behaves_like 'a denied user'
+ end
- group.add_guest(non_team_member)
- end
+ describe 'a guest' do
+ subject { guest }
- it do
- is_expected.to be_allowed(:approve_merge_request)
- end
+ it_behaves_like 'a denied user'
end
- context 'and the user is not a project member' do
- let(:user) { non_team_member }
+ describe 'a developer' do
+ subject { developer }
- it do
- is_expected.not_to be_allowed(:approve_merge_request)
- end
+ it_behaves_like 'a denied user'
end
end
- end
- context 'when merge requests have been disabled' do
- let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) }
+ context 'when merge requests are private' do
+ let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) }
- before do
- project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED)
- end
+ before do
+ project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+ project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+ end
- describe 'the author' do
- subject { author }
+ describe 'the author' do
+ subject { author }
- it_behaves_like 'a denied user'
+ it_behaves_like 'a denied user'
+ end
+
+ describe 'a developer' do
+ subject { developer }
+
+ it_behaves_like 'a user with full access'
+ end
end
- describe 'a guest' do
- subject { guest }
+ context 'when merge request is unlocked' do
+ let(:merge_request) { create(:merge_request, :closed, source_project: project, target_project: project, author: author) }
- it_behaves_like 'a denied user'
+ it 'allows author to reopen merge request' do
+ expect(permissions(author, merge_request)).to be_allowed(:reopen_merge_request)
+ end
+
+ it 'allows developer to reopen merge request' do
+ expect(permissions(developer, merge_request)).to be_allowed(:reopen_merge_request)
+ end
+
+ it 'prevents guest from reopening merge request' do
+ expect(permissions(guest, merge_request)).to be_disallowed(:reopen_merge_request)
+ end
end
- describe 'a developer' do
- subject { developer }
+ context 'when merge request is locked' do
+ let(:merge_request_locked) { create(:merge_request, :closed, discussion_locked: true, source_project: project, target_project: project, author: author) }
- it_behaves_like 'a denied user'
+ it 'prevents author from reopening merge request' do
+ expect(permissions(author, merge_request_locked)).to be_disallowed(:reopen_merge_request)
+ end
+
+ it 'prevents developer from reopening merge request' do
+ expect(permissions(developer, merge_request_locked)).to be_disallowed(:reopen_merge_request)
+ end
+
+ it 'prevents guests from reopening merge request' do
+ expect(permissions(guest, merge_request_locked)).to be_disallowed(:reopen_merge_request)
+ end
+
+ context 'when the user is project member, with at least guest access' do
+ let(:user) { guest }
+
+ it 'can create a note' do
+ expect(permissions(user, merge_request_locked)).to be_allowed(:create_note)
+ end
+ end
end
- describe 'any other user' do
- subject { non_team_member }
+ context 'with external authorization enabled' do
+ let(:user) { create(:user) }
+ let(:project) { create(:project, :public) }
+ let(:merge_request) { create(:merge_request, source_project: project) }
+ let(:policies) { described_class.new(user, merge_request) }
- it_behaves_like 'a denied user'
+ before do
+ enable_external_authorization_service_check
+ end
+
+ it 'can read the issue iid without accessing the external service' do
+ expect(::Gitlab::ExternalAuthorization).not_to receive(:access_allowed?)
+
+ expect(policies).to be_allowed(:read_merge_request_iid)
+ end
end
end
- context 'when merge requests are private' do
- let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) }
+ context 'when user is an inherited member from the parent group' do
+ let_it_be(:group) { create(:group, :public) }
- before do
- project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
- project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+ let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) }
+
+ before_all do
+ group.add_guest(guest)
+ group.add_guest(author)
+ group.add_reporter(reporter)
+ group.add_developer(developer)
end
- describe 'a non-team-member' do
- subject { non_team_member }
+ context 'when project is public' do
+ let(:project) { create(:project, :public, group: group) }
- it_behaves_like 'a denied user'
- end
+ describe 'the merge request author' do
+ subject { permissions(author, merge_request) }
- describe 'the author' do
- subject { author }
+ specify do
+ is_expected.to be_allowed(:approve_merge_request)
+ end
+ end
- it_behaves_like 'a denied user'
+ context 'and merge requests are private' do
+ before do
+ project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+ project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+ end
+
+ describe 'a guest' do
+ subject { guest }
+
+ it_behaves_like 'a denied user'
+ end
+
+ describe 'a reporter' do
+ subject { permissions(reporter, merge_request) }
+
+ it_behaves_like 'a user with reporter access'
+ end
+
+ describe 'a developer' do
+ subject { developer }
+
+ it_behaves_like 'a user with full access'
+ end
+ end
end
- describe 'a developer' do
- subject { developer }
+ context 'when project is private' do
+ let(:project) { create(:project, :private, group: group) }
+
+ describe 'a guest' do
+ subject { guest }
- it_behaves_like 'a user with access'
+ it_behaves_like 'a denied user'
+ end
+
+ describe 'a reporter' do
+ subject { permissions(reporter, merge_request) }
+
+ it_behaves_like 'a user with reporter access'
+ end
+
+ describe 'a developer' do
+ subject { developer }
+
+ it_behaves_like 'a user with full access'
+ end
end
end
- context 'when merge request is unlocked' do
- let(:merge_request) { create(:merge_request, :closed, source_project: project, target_project: project, author: author) }
+ context 'when user is an inherited member from a shared group' do
+ let(:project) { create(:project, :public) }
+ let(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: user) }
+ let(:user) { author }
- it 'allows author to reopen merge request' do
- expect(permissions(author, merge_request)).to be_allowed(:reopen_merge_request)
+ before do
+ project.add_guest(author)
end
- it 'allows developer to reopen merge request' do
- expect(permissions(developer, merge_request)).to be_allowed(:reopen_merge_request)
- end
+ context 'and group is given developer access' do
+ let(:user) { non_team_member }
+
+ subject { permissions(user, merge_request) }
+
+ before do
+ group = create(:group)
+ project.project_group_links.create!(
+ group: group,
+ group_access: Gitlab::Access::DEVELOPER)
+
+ group.add_guest(non_team_member)
+ end
- it 'prevents guest from reopening merge request' do
- expect(permissions(guest, merge_request)).to be_disallowed(:reopen_merge_request)
+ specify do
+ is_expected.to be_allowed(:approve_merge_request)
+ end
end
end
- context 'when merge request is locked' do
- let(:merge_request_locked) { create(:merge_request, :closed, discussion_locked: true, source_project: project, target_project: project, author: author) }
+ context 'when user is not a project member' do
+ let(:project) { create(:project, :public) }
- it 'prevents author from reopening merge request' do
- expect(permissions(author, merge_request_locked)).to be_disallowed(:reopen_merge_request)
- end
+ context 'when merge request is public' do
+ let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
- it 'prevents developer from reopening merge request' do
- expect(permissions(developer, merge_request_locked)).to be_disallowed(:reopen_merge_request)
+ subject { permissions(non_team_member, merge_request) }
+
+ specify do
+ is_expected.not_to be_allowed(:approve_merge_request)
+ end
end
- it 'prevents guests from reopening merge request' do
- expect(permissions(guest, merge_request_locked)).to be_disallowed(:reopen_merge_request)
+ context 'when merge requests are disabled' do
+ let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+ before do
+ project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED)
+ end
+
+ subject { non_team_member }
+
+ it_behaves_like 'a denied user'
end
- context 'when the user is not a project member' do
- let(:user) { create(:user) }
+ context 'when merge requests are private' do
+ let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
- it 'cannot create a note' do
- expect(permissions(user, merge_request_locked)).to be_disallowed(:create_note)
+ before do
+ project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+ project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
end
+
+ subject { non_team_member }
+
+ it_behaves_like 'a denied user'
end
- context 'when the user is project member, with at least guest access' do
- let(:user) { guest }
+ context 'when merge request is locked' do
+ let(:merge_request) { create(:merge_request, :closed, discussion_locked: true, source_project: project, target_project: project) }
- it 'can create a note' do
- expect(permissions(user, merge_request_locked)).to be_allowed(:create_note)
+ it 'cannot create a note' do
+ expect(permissions(non_team_member, merge_request)).to be_disallowed(:create_note)
end
end
end
- context 'with external authorization enabled' do
- let(:user) { create(:user) }
+ context 'when user is anonymous' do
let(:project) { create(:project, :public) }
- let(:merge_request) { create(:merge_request, source_project: project) }
- let(:policies) { described_class.new(user, merge_request) }
- before do
- enable_external_authorization_service_check
- end
+ context 'when merge request is public' do
+ let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
- it 'can read the issue iid without accessing the external service' do
- expect(::Gitlab::ExternalAuthorization).not_to receive(:access_allowed?)
+ subject { permissions(nil, merge_request) }
- expect(policies).to be_allowed(:read_merge_request_iid)
+ specify do
+ is_expected.to be_disallowed(:create_todo, :update_subscription)
+ end
end
end
end
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 2a03ae8938998..320e9e0cd6665 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -1120,6 +1120,44 @@
end.not_to exceed_query_limit(control)
end
end
+
+ context 'when user is an inherited member from the group' do
+ let_it_be(:group) { create(:group) }
+
+ shared_examples 'user cannot view merge requests' do
+ it 'returns 403 forbidden' do
+ get api("/projects/#{group_project.id}/merge_requests", inherited_user)
+
+ expect(response).to have_gitlab_http_status(:forbidden)
+ end
+ end
+
+ context 'and user is a guest' do
+ let_it_be(:inherited_user) { create(:user) }
+
+ before_all do
+ group.add_guest(inherited_user)
+ end
+
+ context 'when project is public with private merge requests' do
+ let(:group_project) do
+ create(:project,
+ :public,
+ :repository,
+ group: group,
+ merge_requests_access_level: ProjectFeature::DISABLED)
+ end
+
+ it_behaves_like 'user cannot view merge requests'
+ end
+
+ context 'when project is private' do
+ let(:group_project) { create(:project, :private, :repository, group: group) }
+
+ it_behaves_like 'user cannot view merge requests'
+ end
+ end
+ end
end
describe "GET /groups/:id/merge_requests" do
@@ -2219,6 +2257,59 @@
expect(response).to have_gitlab_http_status(:created)
end
end
+
+ context 'when user is an inherited member from the group' do
+ let_it_be(:group) { create(:group) }
+
+ shared_examples 'user cannot create merge requests' do
+ it 'returns 403 forbidden' do
+ post api("/projects/#{group_project.id}/merge_requests", inherited_user), params: params
+
+ expect(response).to have_gitlab_http_status(:forbidden)
+ end
+ end
+
+ context 'and user is a guest' do
+ let_it_be(:inherited_user) { create(:user) }
+ let_it_be(:params) do
+ {
+ title: 'Test merge request',
+ source_branch: 'feature_conflict',
+ target_branch: 'master',
+ author_id: inherited_user.id
+ }
+ end
+
+ before_all do
+ group.add_guest(inherited_user)
+ end
+
+ context 'when project is public with private merge requests' do
+ let(:group_project) do
+ create(:project,
+ :public,
+ :repository,
+ group: group,
+ merge_requests_access_level: ProjectFeature::DISABLED,
+ only_allow_merge_if_pipeline_succeeds: false)
+ end
+
+ it_behaves_like 'user cannot create merge requests'
+ end
+
+ context 'when project is private' do
+ let(:group_project) do
+ create(:project,
+ :private,
+ :repository,
+ group: group,
+ only_allow_merge_if_pipeline_succeeds: false)
+ end
+
+ it_behaves_like 'user cannot create merge requests'
+ end
+ end
+ end
end
describe 'PUT /projects/:id/merge_requests/:merge_request_iid' do
--
GitLab