Merge branch...

Merge branch '301116-group-push-permissions-can-enforce-project-level-insecure-settings' into 'master'

Remove push rules lock on group and project level

See merge request gitlab-org/gitlab!58195

ee/app/policies/ee/group_policy.rb

@@ -98,16 +98,6 @@ module EE
         @subject.saml_group_sync_available?
       end
 
-      with_scope :global
-      condition(:commit_committer_check_disabled_globally) do
-        !PushRule.global&.commit_committer_check
-      end
-
-      with_scope :global
-      condition(:reject_unsigned_commits_disabled_globally) do
-        !PushRule.global&.reject_unsigned_commits
-      end
-
       condition(:commit_committer_check_available) do
         @subject.feature_available?(:commit_committer_check)
       end
@@ -329,9 +319,7 @@ module EE
         prevent(:download_wiki_code)
       end
 
-      rule { admin | (commit_committer_check_disabled_globally & can?(:maintainer_access)) }.policy do
-        enable :change_commit_committer_check
-      end
+      rule { admin | maintainer }.enable :change_commit_committer_check
 
       rule { commit_committer_check_available }.policy do
         enable :read_commit_committer_check
@@ -341,7 +329,7 @@ module EE
         prevent :change_commit_committer_check
       end
 
-      rule { admin | (reject_unsigned_commits_disabled_globally & can?(:maintainer_access)) }.enable :change_reject_unsigned_commits
+      rule { admin | maintainer }.enable :change_reject_unsigned_commits
 
       rule { reject_unsigned_commits_available }.enable :read_reject_unsigned_commits
 

ee/app/policies/ee/project_policy.rb

@@ -23,16 +23,6 @@ module EE
       with_scope :global
       condition(:is_development) { Rails.env.development? }
 
-      with_scope :global
-      condition(:reject_unsigned_commits_disabled_globally) do
-        !PushRule.global&.reject_unsigned_commits
-      end
-
-      with_scope :global
-      condition(:commit_committer_check_disabled_globally) do
-        !PushRule.global&.commit_committer_check
-      end
-
       with_scope :global
       condition(:locked_approvers_rules) do
         License.feature_available?(:admin_merge_request_approvers_rules) &&
@@ -74,42 +64,11 @@ module EE
         group_push_rules_enabled? && subject.group.push_rule
       end
 
-      with_scope :subject
-      condition(:reject_unsigned_commits_disabled_by_group) do
-        if group_push_rule_present?
-          !subject.group.push_rule.reject_unsigned_commits
-        else
-          true
-        end
-      end
-
-      condition(:can_change_reject_unsigned_commits) do
-        admin? ||
-          (can?(:maintainer_access) &&
-            reject_unsigned_commits_disabled_globally? &&
-            reject_unsigned_commits_disabled_by_group?)
-      end
-
-      condition(:commit_committer_check_disabled_by_group) do
-        if group_push_rule_present?
-          !subject.group.push_rule.commit_committer_check
-        else
-          true
-        end
-      end
-
       with_scope :subject
       condition(:commit_committer_check_available) do
         @subject.feature_available?(:commit_committer_check)
       end
 
-      condition(:can_change_commit_commiter_check) do
-        admin? ||
-          (can?(:maintainer_access) &&
-            commit_committer_check_disabled_globally? &&
-            commit_committer_check_disabled_by_group?)
-      end
-
       with_scope :subject
       condition(:reject_unsigned_commits_available) do
         @subject.feature_available?(:reject_unsigned_commits)
@@ -322,13 +281,13 @@ module EE
 
       rule { ~can?(:push_code) }.prevent :push_code_to_protected_branches
 
-      rule { can_change_reject_unsigned_commits }.enable :change_reject_unsigned_commits
+      rule { admin | maintainer }.enable :change_reject_unsigned_commits
 
       rule { reject_unsigned_commits_available }.enable :read_reject_unsigned_commits
 
       rule { ~reject_unsigned_commits_available }.prevent :change_reject_unsigned_commits
 
-      rule { can_change_commit_commiter_check }.enable :change_commit_committer_check
+      rule { admin | maintainer }.enable :change_commit_committer_check
 
       rule { commit_committer_check_available }.enable :read_commit_committer_check
 

ee/changelogs/unreleased/301116-group-push-permissions-can-enforce-project-level-insecure-setti.yml

+---
+title: Remove push rules locks on group and project level
+merge_request: 58195
+author:
+type: fixed

ee/spec/policies/group_policy_spec.rb

@@ -970,6 +970,13 @@ RSpec.describe GroupPolicy do
       stub_licensed_features(commit_committer_check: true)
     end
 
+    context 'when the user is an admin', :enable_admin_mode do
+      let(:current_user) { admin }
+
+      it { is_expected.to be_allowed(:change_commit_committer_check) }
+      it { is_expected.to be_allowed(:read_commit_committer_check) }
+    end
+
     context 'the user is a maintainer' do
       let(:current_user) { maintainer }
 
@@ -983,26 +990,6 @@ RSpec.describe GroupPolicy do
       it { is_expected.not_to be_allowed(:change_commit_committer_check) }
       it { is_expected.to be_allowed(:read_commit_committer_check) }
     end
-
-    context 'it is enabled on global level' do
-      before do
-        create(:push_rule_sample, commit_committer_check: true)
-      end
-
-      context 'the user is a maintainer' do
-        let(:current_user) { maintainer }
-
-        it { is_expected.not_to be_allowed(:change_commit_committer_check) }
-        it { is_expected.to be_allowed(:read_commit_committer_check) }
-      end
-
-      context 'the user is a developer' do
-        let(:current_user) { developer }
-
-        it { is_expected.not_to be_allowed(:change_commit_committer_check) }
-        it { is_expected.to be_allowed(:read_commit_committer_check) }
-      end
-    end
   end
 
   context 'reject_unsigned_commits is not enabled by the current license' do
@@ -1021,6 +1008,13 @@ RSpec.describe GroupPolicy do
       stub_licensed_features(reject_unsigned_commits: true)
     end
 
+    context 'when the user is an admin', :enable_admin_mode do
+      let(:current_user) { admin }
+
+      it { is_expected.to be_allowed(:change_reject_unsigned_commits) }
+      it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
+    end
+
     context 'the user is a maintainer' do
       let(:current_user) { maintainer }
 
@@ -1034,33 +1028,6 @@ RSpec.describe GroupPolicy do
       it { is_expected.not_to be_allowed(:change_reject_unsigned_commits) }
       it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
     end
-
-    context 'it is enabled on global level' do
-      before do
-        create(:push_rule_sample, reject_unsigned_commits: true)
-      end
-
-      context 'the user is a maintainer' do
-        let(:current_user) { maintainer }
-
-        it { is_expected.not_to be_allowed(:change_reject_unsigned_commits) }
-        it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
-      end
-
-      context 'the user is a developer' do
-        let(:current_user) { developer }
-
-        it { is_expected.not_to be_allowed(:change_reject_unsigned_commits) }
-        it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
-      end
-
-      context 'the user is an admin', :enable_admin_mode do
-        let(:current_user) { admin }
-
-        it { is_expected.to be_allowed(:change_reject_unsigned_commits) }
-        it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
-      end
-    end
   end
 
   shared_examples 'analytics policy' do |action|

ee/spec/policies/project_policy_spec.rb

@@ -1126,6 +1126,13 @@ RSpec.describe ProjectPolicy do
       stub_licensed_features(commit_committer_check: true)
     end
 
+    context 'when the user is an admin', :enable_admin_mode do
+      let(:current_user) { admin }
+
+      it { is_expected.to be_allowed(:change_commit_committer_check) }
+      it { is_expected.to be_allowed(:read_commit_committer_check) }
+    end
+
     context 'the user is a maintainer' do
       let(:current_user) { maintainer }
 
@@ -1139,46 +1146,6 @@ RSpec.describe ProjectPolicy do
       it { is_expected.not_to be_allowed(:change_commit_committer_check) }
       it { is_expected.to be_allowed(:read_commit_committer_check) }
     end
-
-    context 'it is enabled on global level' do
-      before do
-        create(:push_rule_sample, commit_committer_check: true)
-      end
-
-      context 'when the user is a maintainer' do
-        let(:current_user) { maintainer }
-
-        it { is_expected.not_to be_allowed(:change_commit_committer_check) }
-        it { is_expected.to be_allowed(:read_commit_committer_check) }
-      end
-
-      context 'when the user is a developer' do
-        let(:current_user) { developer }
-
-        it { is_expected.not_to be_allowed(:change_commit_committer_check) }
-        it { is_expected.to be_allowed(:read_commit_committer_check) }
-      end
-    end
-
-    context 'it is enabled on group level' do
-      let(:push_rule) { create(:push_rule, commit_committer_check: true) }
-      let(:group) { create(:group, push_rule: push_rule) }
-      let(:project) { create(:project, namespace_id: group.id) }
-
-      context 'when the user is a maintainer' do
-        let(:current_user) { maintainer }
-
-        it { is_expected.not_to be_allowed(:change_commit_committer_check) }
-        it { is_expected.to be_allowed(:read_commit_committer_check) }
-      end
-
-      context 'when the user is a developer' do
-        let(:current_user) { developer }
-
-        it { is_expected.not_to be_allowed(:change_commit_committer_check) }
-        it { is_expected.to be_allowed(:read_commit_committer_check) }
-      end
-    end
   end
 
   context 'reject_unsigned_commits is not enabled by the current license' do
@@ -1197,6 +1164,13 @@ RSpec.describe ProjectPolicy do
       stub_licensed_features(reject_unsigned_commits: true)
     end
 
+    context 'when the user is an admin', :enable_admin_mode do
+      let(:current_user) { admin }
+
+      it { is_expected.to be_allowed(:change_reject_unsigned_commits) }
+      it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
+    end
+
     context 'when the user is a maintainer' do
       let(:current_user) { maintainer }
 
@@ -1210,46 +1184,6 @@ RSpec.describe ProjectPolicy do
       it { is_expected.not_to be_allowed(:change_reject_unsigned_commits) }
       it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
     end
-
-    context 'it is enabled on global level' do
-      before do
-        create(:push_rule_sample, reject_unsigned_commits: true)
-      end
-
-      context 'when the user is a maintainer' do
-        let(:current_user) { maintainer }
-
-        it { is_expected.not_to be_allowed(:change_reject_unsigned_commits) }
-        it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
-      end
-
-      context 'when the user is a developer' do
-        let(:current_user) { developer }
-
-        it { is_expected.not_to be_allowed(:change_reject_unsigned_commits) }
-        it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
-      end
-    end
-
-    context 'it is enabled on group level' do
-      let(:push_rule) { create(:push_rule_without_project, reject_unsigned_commits: true) }
-      let(:group) { create(:group, push_rule: push_rule) }
-      let(:project) { create(:project, namespace_id: group.id) }
-
-      context 'when the user is a maintainer' do
-        let(:current_user) { maintainer }
-
-        it { is_expected.not_to be_allowed(:change_reject_unsigned_commits) }
-        it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
-      end
-
-      context 'when the user is a developer' do
-        let(:current_user) { developer }
-
-        it { is_expected.not_to be_allowed(:change_reject_unsigned_commits) }
-        it { is_expected.to be_allowed(:read_reject_unsigned_commits) }
-      end
-    end
   end
 
   context 'when dora4 analytics is available' do