From d4289fcb5e12358c9f3f5969031d6dd655f70fec Mon Sep 17 00:00:00 2001
From: Gavin Hinfey <ghinfey@gitlab.com>
Date: Fri, 16 Feb 2024 22:52:29 +0000
Subject: [PATCH] Implement BranchRules::UpdateService
This service handles changes to BranchRule objects.
The EE version returns errors if a custom branch rule is passed.
Changelog: changed
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/144630
---
app/services/branch_rules/base_service.rb | 11 ++-
app/services/branch_rules/update_service.rb | 45 +++++++++++
.../protected_branches/update_service.rb | 8 +-
.../ee/branch_rules/update_service.rb | 24 ++++++
.../renaming_blocked_by_policy.rb | 2 +-
.../branch_rules/update_service_spec.rb | 49 ++++++++++++
.../branch_rules/update_service_spec.rb | 77 +++++++++++++++++++
.../protected_branches/update_service_spec.rb | 9 +++
8 files changed, 220 insertions(+), 5 deletions(-)
create mode 100644 app/services/branch_rules/update_service.rb
create mode 100644 ee/app/services/ee/branch_rules/update_service.rb
create mode 100644 ee/spec/services/branch_rules/update_service_spec.rb
create mode 100644 spec/services/branch_rules/update_service_spec.rb
diff --git a/app/services/branch_rules/base_service.rb b/app/services/branch_rules/base_service.rb
index 17b3cbd472b61..5ed8ea3a23bec 100644
--- a/app/services/branch_rules/base_service.rb
+++ b/app/services/branch_rules/base_service.rb
@@ -4,12 +4,19 @@ module BranchRules
class BaseService
include Gitlab::Allowable
- attr_accessor :project, :branch_rule, :current_user
+ attr_reader :project, :branch_rule, :current_user, :params
- def initialize(branch_rule, user = nil)
+ def initialize(branch_rule, user = nil, params = {})
@branch_rule = branch_rule
@project = branch_rule.project
@current_user = user
+ @params = params.slice(*permitted_params)
+ end
+
+ private
+
+ def permitted_params
+ []
end
end
end
diff --git a/app/services/branch_rules/update_service.rb b/app/services/branch_rules/update_service.rb
new file mode 100644
index 0000000000000..6b79376663e91
--- /dev/null
+++ b/app/services/branch_rules/update_service.rb
@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+module BranchRules
+ class UpdateService < BaseService
+ PERMITTED_PARAMS = %i[name].freeze
+
+ attr_reader :skip_authorization
+
+ def execute(skip_authorization: false)
+ @skip_authorization = skip_authorization
+
+ raise Gitlab::Access::AccessDeniedError unless can_update_branch_rule?
+
+ return update_protected_branch if branch_rule.instance_of?(Projects::BranchRule)
+
+ yield if block_given?
+
+ ServiceResponse.error(message: 'Unknown branch rule type.')
+ end
+
+ private
+
+ def permitted_params
+ PERMITTED_PARAMS
+ end
+
+ def can_update_branch_rule?
+ return true if skip_authorization
+
+ can?(current_user, :update_protected_branch, branch_rule)
+ end
+
+ def update_protected_branch
+ service = ProtectedBranches::UpdateService.new(project, current_user, params)
+
+ service_response = service.execute(branch_rule.protected_branch, skip_authorization: skip_authorization)
+
+ return ServiceResponse.success unless service_response.errors.any?
+
+ ServiceResponse.error(message: service_response.errors.full_messages)
+ end
+ end
+end
+
+BranchRules::UpdateService.prepend_mod
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
index 4b54bf929893f..0c44871f6dc92 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -2,8 +2,8 @@
module ProtectedBranches
class UpdateService < ProtectedBranches::BaseService
- def execute(protected_branch)
- raise Gitlab::Access::AccessDeniedError unless can?(current_user, :update_protected_branch, protected_branch)
+ def execute(protected_branch, skip_authorization: false)
+ raise Gitlab::Access::AccessDeniedError unless skip_authorization || authorized?(protected_branch)
old_merge_access_levels = protected_branch.merge_access_levels.map(&:clone)
old_push_access_levels = protected_branch.push_access_levels.map(&:clone)
@@ -16,6 +16,10 @@ def execute(protected_branch)
protected_branch
end
+
+ def authorized?(protected_branch)
+ can?(current_user, :update_protected_branch, protected_branch)
+ end
end
end
diff --git a/ee/app/services/ee/branch_rules/update_service.rb b/ee/app/services/ee/branch_rules/update_service.rb
new file mode 100644
index 0000000000000..61fdf9733e04a
--- /dev/null
+++ b/ee/app/services/ee/branch_rules/update_service.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module EE
+ module BranchRules
+ module UpdateService
+ extend ::Gitlab::Utils::Override
+
+ # rubocop:disable Cop/AvoidReturnFromBlocks -- see similar
+ # implementation and explanation in BranchRules::DestroyService
+ override :execute
+ def execute(skip_authorization: false)
+ super do
+ case branch_rule
+ when ::Projects::AllBranchesRule
+ return ServiceResponse.error(message: 'All branch rules cannot be updated')
+ when ::Projects::AllProtectedBranchesRule
+ return ServiceResponse.error(message: 'All protected branch rules cannot be updated')
+ end
+ end
+ end
+ # rubocop:enable Cop/AvoidReturnFromBlocks
+ end
+ end
+end
diff --git a/ee/app/services/ee/protected_branches/renaming_blocked_by_policy.rb b/ee/app/services/ee/protected_branches/renaming_blocked_by_policy.rb
index ad34fa7d10725..1debecfb01916 100644
--- a/ee/app/services/ee/protected_branches/renaming_blocked_by_policy.rb
+++ b/ee/app/services/ee/protected_branches/renaming_blocked_by_policy.rb
@@ -3,7 +3,7 @@
module EE
module ProtectedBranches
module RenamingBlockedByPolicy
- def execute(protected_branch)
+ def execute(protected_branch, skip_authorization: false)
raise ::Gitlab::Access::AccessDeniedError if renaming? && blocked?(protected_branch)
super
diff --git a/ee/spec/services/branch_rules/update_service_spec.rb b/ee/spec/services/branch_rules/update_service_spec.rb
new file mode 100644
index 0000000000000..395182b29e52e
--- /dev/null
+++ b/ee/spec/services/branch_rules/update_service_spec.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe BranchRules::UpdateService, feature_category: :source_code_management do
+ let_it_be(:project) { create(:project, :repository) }
+ let_it_be(:user) { create(:user) }
+ let_it_be(:name) { 'new_name' }
+ let_it_be(:params) { { name: name } }
+
+ describe '#execute' do
+ subject(:execute) { described_class.new(branch_rule, user, params).execute }
+
+ before do
+ allow(Ability).to receive(:allowed?).and_return(true)
+ end
+
+ context 'when branch_rule is a Projects::AllBranchesRule' do
+ let(:branch_rule) { Projects::AllBranchesRule.new(project) }
+
+ it 'returns an error response' do
+ response = execute
+ expect(response).to be_error
+ expect(response[:message]).to eq('All branch rules cannot be updated')
+ end
+ end
+
+ context 'when branch_rule is a Projects::AllProtectedBranchesRule' do
+ let(:branch_rule) { Projects::AllProtectedBranchesRule.new(project) }
+
+ it 'returns an error response' do
+ response = execute
+ expect(response).to be_error
+ expect(response[:message]).to eq('All protected branch rules cannot be updated')
+ end
+ end
+
+ context 'when branch_rule is a ProtectedBranch' do
+ let(:protected_branch) { create(:protected_branch) }
+ let(:branch_rule) { Projects::BranchRule.new(project, protected_branch) }
+
+ it 'returns a success response' do
+ response = execute
+ expect(response).to be_success
+ expect(protected_branch.reload.name).to eq(name)
+ end
+ end
+ end
+end
diff --git a/spec/services/branch_rules/update_service_spec.rb b/spec/services/branch_rules/update_service_spec.rb
new file mode 100644
index 0000000000000..a1cd12f2cb3c6
--- /dev/null
+++ b/spec/services/branch_rules/update_service_spec.rb
@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe BranchRules::UpdateService, feature_category: :source_code_management do
+ let_it_be(:project) { create(:project, :repository) }
+ let_it_be(:user) { create(:user) }
+ let_it_be(:protected_branch) { create(:protected_branch) }
+
+ describe '#execute' do
+ let(:branch_rule) { Projects::BranchRule.new(project, protected_branch) }
+ let(:action_allowed) { true }
+ let(:update_service) { ProtectedBranches::UpdateService }
+ let(:update_service_instance) { instance_double(update_service) }
+ let(:new_name) { 'new_name' }
+ let(:errors) { ["Error 1", "Error 2"] }
+ let(:params) { { name: new_name } }
+
+ subject(:execute) { described_class.new(branch_rule, user, params).execute }
+
+ before do
+ allow(Ability).to receive(:allowed?).and_return(true)
+ allow(Ability)
+ .to receive(:allowed?).with(user, :update_protected_branch, branch_rule)
+ .and_return(action_allowed)
+ end
+
+ context 'when the current_user cannot update the branch rule' do
+ let(:action_allowed) { false }
+
+ it 'raises an access denied error' do
+ expect { execute }.to raise_error(Gitlab::Access::AccessDeniedError)
+ end
+ end
+
+ context 'when branch_rule is a Projects::BranchRule' do
+ it 'updates the ProtectedBranch and returns a success execute' do
+ expect(execute[:status]).to eq(:success)
+ expect(protected_branch.reload.name).to eq(new_name)
+ end
+
+ context 'if the update fails' do
+ before do
+ allow(update_service).to receive(:new).and_return(update_service_instance)
+ allow(update_service_instance).to receive(:execute).and_return(protected_branch)
+ allow(protected_branch).to receive_message_chain(:errors, :any?).and_return(errors)
+ allow(protected_branch).to receive_message_chain(:errors, :full_messages).and_return(errors)
+ end
+
+ it 'returns an error' do
+ response = execute
+ expect(response[:message]).to eq(errors)
+ expect(response[:status]).to eq(:error)
+ end
+ end
+ end
+
+ context 'when branch_rule is a ProtectedBranch' do
+ let(:branch_rule) { protected_branch }
+
+ it 'returns an error' do
+ response = execute
+ expect(response[:message]).to eq('Unknown branch rule type.')
+ expect(response[:status]).to eq(:error)
+ end
+ end
+
+ context 'when unpermitted params are provided' do
+ let(:params) { { name: new_name, not_permitted: 'not_permitted' } }
+
+ it 'removes them' do
+ expect(update_service).to receive(:new).with(project, user, { name: new_name }).and_call_original
+ execute
+ end
+ end
+ end
+end
diff --git a/spec/services/protected_branches/update_service_spec.rb b/spec/services/protected_branches/update_service_spec.rb
index 8b11604aa15d7..73e721e015f81 100644
--- a/spec/services/protected_branches/update_service_spec.rb
+++ b/spec/services/protected_branches/update_service_spec.rb
@@ -40,6 +40,15 @@
expect { service.execute(protected_branch) }.to raise_error(Gitlab::Access::AccessDeniedError)
end
end
+
+ context 'with skip authorization and unauthorized user' do
+ let(:user) { create(:user) }
+ let(:result) { service.execute(protected_branch, skip_authorization: true) }
+
+ it 'updates a protected branch' do
+ expect(result.reload.name).to eq(params[:name])
+ end
+ end
end
end
--
GitLab