diff --git a/doc/api/merge_request_approvals.md b/doc/api/merge_request_approvals.md
index 6463c428a2f35f39d9c14ad57c775c986ddc78ff..af472a1166d818c83af6bfc797c19031a98786db 100644
--- a/doc/api/merge_request_approvals.md
+++ b/doc/api/merge_request_approvals.md
@@ -33,7 +33,7 @@ Group approval rules apply to all protected branches of projects belonging to th
### Create group-level approval rules
-Users with at least the Maintainer role can create group level approval rules using the following endpoint:
+Group admins can create group level approval rules using the following endpoint:
```plaintext
POST /groups/:id/approval_rules
diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb
index 748798f9164b1dba2b46d829e76ea7d514222625..9b9c3283e37d688cda98259476740cf03573f3f8 100644
--- a/ee/app/policies/ee/group_policy.rb
+++ b/ee/app/policies/ee/group_policy.rb
@@ -316,7 +316,6 @@ module GroupPolicy
enable :maintainer_access
enable :admin_wiki
enable :modify_product_analytics_settings
- enable :update_approval_rule
end
rule { (admin | maintainer) & group_analytics_dashboards_available & ~has_parent }.policy do
@@ -579,6 +578,7 @@ module GroupPolicy
rule { (admin | owner) & group_merge_request_approval_settings_enabled }.policy do
enable :admin_merge_request_approval_settings
+ enable :update_approval_rule
end
rule { needs_new_sso_session }.policy do
diff --git a/ee/lib/api/helpers/group_approval_rules_helpers.rb b/ee/lib/api/helpers/group_approval_rules_helpers.rb
index 8d04d29dcba3722764cb52b14207d681d3998306..7d017cc9435ed613efe44609cce2845778b71cd3 100644
--- a/ee/lib/api/helpers/group_approval_rules_helpers.rb
+++ b/ee/lib/api/helpers/group_approval_rules_helpers.rb
@@ -25,8 +25,14 @@ def check_feature_flag
not_found! unless ::Feature.enabled?(:approval_group_rules, user_group)
end
+ def authorize_update_group_approval_rule!
+ return if can?(current_user, :admin_group, user_group)
+
+ authorize! :admin_merge_request_approval_settings, user_group
+ end
+
def create_group_approval_rule(present_with:)
- authorize! :update_approval_rule, user_group
+ authorize_update_group_approval_rule!
result = ::ApprovalRules::CreateService.new(user_group, current_user,
declared_params(include_missing: false)).execute
diff --git a/ee/spec/policies/approval_rules/approval_group_rule_policy_spec.rb b/ee/spec/policies/approval_rules/approval_group_rule_policy_spec.rb
index ba1952155c4281b71639f5a0c7f67ffb4344933a..aabe7d8f437b8257d64e786633eaf88645e2a2e3 100644
--- a/ee/spec/policies/approval_rules/approval_group_rule_policy_spec.rb
+++ b/ee/spec/policies/approval_rules/approval_group_rule_policy_spec.rb
@@ -12,7 +12,7 @@ def permissions(user, approval_rule)
end
before_all do
- group.add_maintainer(user)
+ group.add_owner(user)
end
context 'when user can update group' do
diff --git a/ee/spec/requests/api/group_approval_rules_spec.rb b/ee/spec/requests/api/group_approval_rules_spec.rb
index 01ad662628121e9e32f7f17c467cb60b9947cd13..ecc03c03581fb41a0d7a7118526923186994b710 100644
--- a/ee/spec/requests/api/group_approval_rules_spec.rb
+++ b/ee/spec/requests/api/group_approval_rules_spec.rb
@@ -5,7 +5,8 @@
RSpec.describe API::GroupApprovalRules, :aggregate_failures, feature_category: :source_code_management do
let_it_be(:group) { create(:group_with_members) }
let_it_be(:group2) { create(:group_with_members) }
- let_it_be(:user) { create(:user) }
+ let_it_be(:admin) { create(:admin) }
+ let_it_be(:user) { create(:admin) }
let_it_be(:user2) { create(:user) }
let_it_be(:project) do
create(:project, :public, :repository, creator: user, group: group,
@@ -16,6 +17,10 @@
let_it_be(:approver) { create(:user) }
let_it_be(:other_approver) { create(:user) }
+ before_all do
+ group.add_maintainer(user2)
+ end
+
before do
stub_licensed_features(merge_request_approvers: true)
end
@@ -32,24 +37,20 @@
}
end
- before_all do
- group.add_maintainer(user)
- end
-
context 'when approval_group_rules flag is disabled' do
before do
stub_feature_flags(approval_group_rules: false)
end
it 'returns 404' do
- post api(url, current_user), params: params
+ post api(url, current_user, admin_mode: current_user.admin?), params: params
expect(response).to have_gitlab_http_status(:not_found)
end
end
it 'returns 201 status' do
- post api(url, current_user), params: params
+ post api(url, current_user, admin_mode: current_user.admin?), params: params
expect(response).to have_gitlab_http_status(:created)
expect(response).to match_response_schema(schema, dir: 'ee')
@@ -61,7 +62,7 @@
end
it 'returns protected branches' do
- post api(url, current_user), params: params
+ post api(url, current_user, admin_mode: current_user.admin?), params: params
expect(response).to have_gitlab_http_status(:created)
expect(json_response['protected_branches'].size).to be 2
@@ -70,7 +71,7 @@
context 'when multiple_approval_rules feature is not available' do
it 'does not return protected branches' do
- post api(url, current_user), params: params
+ post api(url, current_user, admin_mode: current_user.admin?), params: params
expect(response).to have_gitlab_http_status(:created)
expect(json_response).not_to include('protected_branches')
@@ -78,8 +79,10 @@
end
context 'when a user is without access' do
+ let(:current_user) { user2 }
+
it 'returns 403' do
- post api(url, user2), params: params
+ post api(url, current_user, admin_mode: current_user.admin?), params: params
expect(response).to have_gitlab_http_status(:forbidden)
end
@@ -87,7 +90,7 @@
context 'when missing parameters' do
it 'returns 400 status' do
- post api(url, current_user)
+ post api(url, current_user, admin_mode: current_user.admin?)
expect(response).to have_gitlab_http_status(:bad_request)
end
@@ -97,7 +100,7 @@
let(:name) { '' }
it 'returns 400 status' do
- post api(url, current_user), params: params
+ post api(url, current_user, admin_mode: current_user.admin?), params: params
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['message']).to eq({ "name" => ["can't be blank"] })
@@ -106,16 +109,16 @@
context 'with user_id or group_id params' do
before do
- post api(url, current_user), params: params.merge!(extra_params)
+ post api(url, current_user, admin_mode: current_user.admin?), params: params.merge!(extra_params)
end
context 'with user_ids' do
- let(:extra_params) { { user_ids: [user.id] } }
+ let(:extra_params) { { user_ids: [user2.id] } }
it 'returns a user' do
expect(response).to have_gitlab_http_status(:created)
expect(json_response['users'].size).to be 1
- expect(json_response.dig('users', 0, 'id')).to eq(user.id)
+ expect(json_response.dig('users', 0, 'id')).to eq(user2.id)
end
end
diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb
index 913d97397442856bb7e97bd5d7faf85fb7378706..39ab2658d18e4d04d3dfb574e1d120900e7d5789 100644
--- a/ee/spec/services/approval_rules/create_service_spec.rb
+++ b/ee/spec/services/approval_rules/create_service_spec.rb
@@ -243,10 +243,6 @@
}
end
- before do
- target.add_owner(user)
- end
-
subject do
described_class.new(
target,
@@ -255,55 +251,71 @@
).execute
end
- it_behaves_like "creatable"
-
- context 'when approval_group_rules is disabled for group' do
+ context 'when user is a maintainer' do
before do
- stub_feature_flags(approval_group_rules: false)
+ target.add_maintainer(user)
end
- it 'returns an error' do
- expect(subject[:status]).to eq(:error)
- expect(subject[:message]).to eq('The feature approval_group_rules is not enabled.')
+ it 'returns a forbidden status' do
+ expect(subject[:http_status]).to eq 403
end
end
- context 'and multiple approval rules are enabled' do
+ context 'when user is an owner' do
before do
- stub_licensed_features(multiple_approval_rules: true)
+ target.add_owner(user)
end
- context 'when protected_branch_ids param is present' do
- let(:protected_branch) { create(:protected_branch, project: nil, group: group) }
- let(:protected_branch_ids) { [protected_branch.id] }
- let(:params) do
- {
- name: name,
- approvals_required: 1,
- skip_authorization: true
- }
+ it_behaves_like "creatable"
+
+ context 'when approval_group_rules is disabled for group' do
+ before do
+ stub_feature_flags(approval_group_rules: false)
end
- it 'does not associate the group approval rule to the protected branches' do
- expect(subject[:status]).to eq(:success)
- expect(subject[:rule].protected_branches).to eq([])
+ it 'returns an error' do
+ expect(subject[:status]).to eq(:error)
+ expect(subject[:message]).to eq('The feature approval_group_rules is not enabled.')
end
end
- context 'when applies_to_all_protected_branches is set to false' do
- let(:protected_branch) { create(:protected_branch, project: nil, group: group) }
- let(:skip_authorization) { true }
- let(:params) do
- {
- name: name,
- approvals_required: 1,
- skip_authorization: true,
- applies_to_all_protected_branches: false
- }
+ context 'and multiple approval rules are enabled' do
+ before do
+ stub_licensed_features(multiple_approval_rules: true)
end
- it 'throws a validation error' do
- expect(subject[:status]).to eq(:error)
+ context 'when protected_branch_ids param is present' do
+ let(:protected_branch) { create(:protected_branch, project: nil, group: group) }
+ let(:protected_branch_ids) { [protected_branch.id] }
+ let(:params) do
+ {
+ name: name,
+ approvals_required: 1,
+ skip_authorization: true
+ }
+ end
+
+ it 'does not associate the group approval rule to the protected branches' do
+ expect(subject[:status]).to eq(:success)
+ expect(subject[:rule].protected_branches).to eq([])
+ end
+ end
+
+ context 'when applies_to_all_protected_branches is set to false' do
+ let(:protected_branch) { create(:protected_branch, project: nil, group: group) }
+ let(:skip_authorization) { true }
+ let(:params) do
+ {
+ name: name,
+ approvals_required: 1,
+ skip_authorization: true,
+ applies_to_all_protected_branches: false
+ }
+ end
+
+ it 'throws a validation error' do
+ expect(subject[:status]).to eq(:error)
+ end
end
end
end