Commit 20ccad24 authored by Heinrich Lee Yu's avatar Heinrich Lee Yu

Merge branch 'sk/329422-protected-branch-check' into 'master'

Check if default branch is already protected before creating

See merge request gitlab-org/gitlab!65133
parents 65afcd84 56141147
...@@ -22,7 +22,7 @@ module Projects ...@@ -22,7 +22,7 @@ module Projects
# Ensure HEAD points to the default branch in case it is not master # Ensure HEAD points to the default branch in case it is not master
project.change_head(default_branch) project.change_head(default_branch)
create_protected_branch if protect_branch? create_protected_branch if protect_branch? && !protected_branch_exists?
end end
def create_protected_branch def create_protected_branch
...@@ -44,6 +44,10 @@ module Projects ...@@ -44,6 +44,10 @@ module Projects
!ProtectedBranch.protected?(project, default_branch) !ProtectedBranch.protected?(project, default_branch)
end end
def protected_branch_exists?
project.protected_branches.find_by_name(default_branch).present?
end
def default_branch def default_branch
project.default_branch project.default_branch
end end
......
...@@ -13,7 +13,7 @@ module Security ...@@ -13,7 +13,7 @@ module Security
project.create_security_orchestration_policy_configuration! do |p| project.create_security_orchestration_policy_configuration! do |p|
p.security_policy_management_project_id = policy_project.id p.security_policy_management_project_id = policy_project.id
end end
create_protected_branch(policy_project) create_or_update_protected_branch(policy_project)
members = add_members(policy_project) members = add_members(policy_project)
errors = members.flat_map { |member| member.errors.full_messages } errors = members.flat_map { |member| member.errors.full_messages }
...@@ -25,13 +25,21 @@ module Security ...@@ -25,13 +25,21 @@ module Security
private private
def create_protected_branch(policy_project) def create_or_update_protected_branch(policy_project)
protected_branch = policy_project.protected_branches.find_by_name(policy_project.default_branch_or_main)
params = { params = {
name: policy_project.default_branch_or_main, name: policy_project.default_branch_or_main,
push_access_levels_attributes: [{ access_level: Gitlab::Access::NO_ACCESS }], push_access_levels_attributes: [{ access_level: Gitlab::Access::NO_ACCESS }],
merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }] merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }]
} }
if protected_branch.present?
ProtectedBranch::UpdateService
.new(policy_project, current_user, params)
.execute(protected_branch)
return
end
ProtectedBranches::CreateService ProtectedBranches::CreateService
.new(policy_project, current_user, params) .new(policy_project, current_user, params)
.execute(skip_authorization: true) .execute(skip_authorization: true)
......
...@@ -23,6 +23,8 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProjectCreateService do ...@@ -23,6 +23,8 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProjectCreateService do
expect(project.security_orchestration_policy_configuration.security_policy_management_project).to eq(policy_project) expect(project.security_orchestration_policy_configuration.security_policy_management_project).to eq(policy_project)
expect(policy_project.namespace).to eq(project.namespace) expect(policy_project.namespace).to eq(project.namespace)
expect(policy_project.protected_branches.map(&:name)).to contain_exactly(project.default_branch_or_main) expect(policy_project.protected_branches.map(&:name)).to contain_exactly(project.default_branch_or_main)
expect(policy_project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER])
expect(policy_project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::NO_ACCESS])
expect(policy_project.team.developers).to contain_exactly(maintainer) expect(policy_project.team.developers).to contain_exactly(maintainer)
end end
end end
......
...@@ -99,6 +99,53 @@ RSpec.describe Projects::ProtectDefaultBranchService do ...@@ -99,6 +99,53 @@ RSpec.describe Projects::ProtectDefaultBranchService do
.not_to have_received(:create_protected_branch) .not_to have_received(:create_protected_branch)
end end
end end
context 'when protected branch does not exist' do
before do
allow(service)
.to receive(:protected_branch_exists?)
.and_return(false)
allow(service)
.to receive(:protect_branch?)
.and_return(true)
end
it 'changes the HEAD of the project' do
service.protect_default_branch
expect(project)
.to have_received(:change_head)
end
it 'protects the default branch' do
service.protect_default_branch
expect(service)
.to have_received(:create_protected_branch)
end
end
context 'when protected branch already exists' do
before do
allow(service)
.to receive(:protected_branch_exists?)
.and_return(true)
end
it 'changes the HEAD of the project' do
service.protect_default_branch
expect(project)
.to have_received(:change_head)
end
it 'does not protect the default branch' do
service.protect_default_branch
expect(service)
.not_to have_received(:create_protected_branch)
end
end
end end
describe '#create_protected_branch' do describe '#create_protected_branch' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment