Commit 56141147 authored by Sashi's avatar Sashi

Check if default branch is already protected while creating

This change checks if the default branch is already
protected before creating a protected branch. This happens when a
security policy project is created with branch protection enabled
for the default branch. To avoid the race condition from
BranchHooksService we update the branch protection if it
already exists while creating security policy project.
parent af2320c7
...@@ -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