Remove push rule locks on group and project level

When group level push rules are checked, the children project level ones
are locked but the values are not checked. This behavior leaves the
permission in inconsistent states. The same issue is also observed
between instance level and group level push rules.

This commit remove locks imposed by instance level and group level on
the lower lever push rules.

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