Enforce share_with_group_lock rules

…in Groups::UpdateService instead of only in the browser.

4 files changed, 104 insertions, 1 deletions

diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index 8ada661e571..dab27bbf6da 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -15,6 +15,9 @@ class GroupPolicy < BasePolicy
 
   condition(:nested_groups_supported, scope: :global) { Group.supports_nested_groups? }
 
+  condition(:parent_share_locked) { @subject.has_parent? && @subject.parent.share_with_group_lock? }
+  condition(:parent_owner) { @subject.has_parent? && @subject.parent.has_owner?(@user) }
+
   condition(:has_projects) do
     GroupProjectsFinder.new(group: @subject, current_user: @user).execute.any?
   end
@@ -54,6 +57,8 @@ class GroupPolicy < BasePolicy
   rule { ~can?(:view_globally) }.prevent   :request_access
   rule { has_access }.prevent              :request_access
 
+  rule { owner & (~parent_share_locked | parent_owner) }.enable :change_share_with_group_lock
+
   def access_level
     return GroupMember::NO_ACCESS if @user.nil?
 
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index 1d65c76d282..45716c63cfc 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -10,10 +10,12 @@ module Groups
             Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
 
           deny_visibility_level(group, new_visibility)
-          return group
+          return false
         end
       end
 
+      return false unless valid_share_with_group_lock_change?
+
       group.assign_attributes(params)
 
       begin
@@ -30,5 +32,19 @@ module Groups
     def reject_parent_id!
       params.except!(:parent_id)
     end
+
+    def valid_share_with_group_lock_change?
+      return true unless changing_share_with_group_lock?
+      return true if can?(current_user, :change_share_with_group_lock, group)
+
+      group.errors.add(:share_with_group_lock, 'cannot be disabled when the parent group Share lock is enabled, except by the owner of the parent group')
+      false
+    end
+
+    def changing_share_with_group_lock?
+      return false if params[:share_with_group_lock].nil?
+
+      params[:share_with_group_lock] != group.share_with_group_lock
+    end
   end
 end
diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index 7f832bfa563..9fa63982790 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/spec/policies/group_policy_spec.rb
@@ -242,4 +242,52 @@ describe GroupPolicy do
       end
     end
   end
+
+  describe 'change_share_with_group_lock' do
+    context 'when the group has a parent' do
+      let(:group) { create(:group, parent: parent) }
+
+      context 'when the parent share_with_group_lock is enabled' do
+        let(:parent) { create(:group, share_with_group_lock: true) }
+        let(:current_user) { owner }
+
+        context 'when current_user owns the parent' do
+          before do
+            parent.add_owner(owner)
+          end
+
+          it { expect_allowed(:change_share_with_group_lock) }
+        end
+
+        context 'when current_user owns the group but not the parent' do
+          it { expect_disallowed(:change_share_with_group_lock) }
+        end
+      end
+
+      context 'when the parent share_with_group_lock is disabled' do
+        let(:parent) { create(:group) }
+        let(:current_user) { owner }
+
+        context 'when current_user owns the parent' do
+          before do
+            parent.add_owner(owner)
+          end
+
+          it { expect_allowed(:change_share_with_group_lock) }
+        end
+
+        context 'when current_user owns the group but not the parent' do
+          it { expect_allowed(:change_share_with_group_lock) }
+        end
+      end
+    end
+
+    context 'when the group does not have a parent' do
+      context 'when current_user owns the group' do
+        let(:current_user) { owner }
+
+        it { expect_allowed(:change_share_with_group_lock) }
+      end
+    end
+  end
 end
diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb
index 44f22a3b37b..aa8e058903b 100644
--- a/spec/services/groups/update_service_spec.rb
+++ b/spec/services/groups/update_service_spec.rb
@@ -100,4 +100,38 @@ describe Groups::UpdateService do
       end
     end
   end
+
+  context 'for a subgroup' do
+    let(:subgroup) { create(:group, :private, parent: private_group) }
+
+    context 'when the parent group share_with_group_lock is enabled' do
+      before do
+        private_group.update_column(:share_with_group_lock, true)
+      end
+
+      context 'for the parent group owner' do
+        it 'allows disabling share_with_group_lock' do
+          private_group.add_owner(user)
+
+          result = described_class.new(subgroup, user, share_with_group_lock: false).execute
+
+          expect(result).to be_truthy
+          expect(subgroup.reload.share_with_group_lock).to be_falsey
+        end
+      end
+
+      context 'for a subgroup owner (who does not own the parent)' do
+        it 'does not allow disabling share_with_group_lock' do
+          subgroup_owner = create(:user)
+          subgroup.add_owner(subgroup_owner)
+
+          result = described_class.new(subgroup, subgroup_owner, share_with_group_lock: false).execute
+
+          expect(result).to be_falsey
+          expect(subgroup.errors.full_messages.first).to match(/cannot be disabled when the parent group Share lock is enabled, except by the owner of the parent group/)
+          expect(subgroup.reload.share_with_group_lock).to be_truthy
+        end
+      end
+    end
+  end
 end