Add validation for new_user_signups_cap enabling

Enabling the User Cap setting for a root group should only be possible
if the group or any of it's subgroups have not been shared outside
of the root group's hierarchy

Changelog: changed
EE: true

app/models/namespace_setting.rb

@@ -12,9 +12,6 @@ class NamespaceSetting < ApplicationRecord
   validate :allow_mfa_for_group
   validate :allow_resource_access_token_creation_for_group
 
-  before_save :set_prevent_sharing_groups_outside_hierarchy, if: -> { user_cap_enabled? }
-  after_save :disable_project_sharing!, if: -> { user_cap_enabled? }
-
   before_validation :normalize_default_branch_name
 
   enum jobs_to_be_done: { basics: 0, move_repository: 1, code_storage: 2, exploring: 3, ci: 4, other: 5 }, _suffix: true
@@ -59,18 +56,6 @@ class NamespaceSetting < ApplicationRecord
       errors.add(:resource_access_token_creation_allowed, _('is not allowed since the group is not top-level group.'))
     end
   end
-
-  def set_prevent_sharing_groups_outside_hierarchy
-    self.prevent_sharing_groups_outside_hierarchy = true
-  end
-
-  def disable_project_sharing!
-    namespace.update_attribute(:share_with_group_lock, true)
-  end
-
-  def user_cap_enabled?
-    new_user_signups_cap.present? && namespace.root?
-  end
 end
 
 NamespaceSetting.prepend_mod_with('NamespaceSetting')

ee/app/models/ee/group.rb

@@ -523,6 +523,14 @@ module EE
       user_cap <= members_count
     end
 
+    def shared_externally?
+      strong_memoize(:shared_externally) do
+        invited_group_in_groups
+          .where.not(group_group_links: { shared_with_group_id: ::Group.groups_including_descendants_by([self]) })
+          .limit(1).any?
+      end
+    end
+
     private
 
     override :post_create_hook

ee/app/models/ee/namespace_setting.rb

@@ -4,14 +4,47 @@ module EE
   module NamespaceSetting
     extend ActiveSupport::Concern
 
-    delegate :root_ancestor, to: :namespace
+    prepended do
+      validate :user_cap_allowed, if: -> { enabling_user_cap? }
 
-    def prevent_forking_outside_group?
-      saml_setting = root_ancestor.saml_provider&.prohibited_outer_forks?
+      before_save :set_prevent_sharing_groups_outside_hierarchy, if: -> { user_cap_enabled? }
+      after_save :disable_project_sharing!, if: -> { user_cap_enabled? }
 
-      return saml_setting unless namespace.feature_available?(:group_forking_protection)
+      delegate :root_ancestor, to: :namespace
 
-      saml_setting || root_ancestor.namespace_settings&.prevent_forking_outside_group
+      def prevent_forking_outside_group?
+        saml_setting = root_ancestor.saml_provider&.prohibited_outer_forks?
+
+        return saml_setting unless namespace.feature_available?(:group_forking_protection)
+
+        saml_setting || root_ancestor.namespace_settings&.prevent_forking_outside_group
+      end
+
+      private
+
+      def enabling_user_cap?
+        return false unless persisted? && new_user_signups_cap_changed?
+
+        new_user_signups_cap_was.nil?
+      end
+
+      def user_cap_allowed
+        return if namespace.user_cap_available? && namespace.root? && !namespace.shared_externally?
+
+        errors.add(:new_user_signups_cap, _("cannot be enabled"))
+      end
+
+      def set_prevent_sharing_groups_outside_hierarchy
+        self.prevent_sharing_groups_outside_hierarchy = true
+      end
+
+      def disable_project_sharing!
+        namespace.update_attribute(:share_with_group_lock, true)
+      end
+
+      def user_cap_enabled?
+        new_user_signups_cap.present? && namespace.root?
+      end
     end
   end
 end

ee/spec/controllers/ee/groups_controller_spec.rb

@@ -629,6 +629,15 @@ RSpec.describe GroupsController do
       end
 
       context 'authenticated as group owner' do
+        before do
+          allow_next_found_instance_of(Group) do |group|
+            allow(group).to receive(:user_cap_available?).and_return(true)
+          end
+
+          group.add_owner(user)
+          sign_in(user)
+        end
+
         where(:new_user_signups_cap, :result, :status) do
           nil | nil   | :found
           10  | 10    | :found
@@ -639,11 +648,6 @@ RSpec.describe GroupsController do
             { id: group.to_param, group: { new_user_signups_cap: new_user_signups_cap } }
           end
 
-          before do
-            group.add_owner(user)
-            sign_in(user)
-          end
-
           it_behaves_like 'updates the attribute'
         end
       end

ee/spec/models/ee/group_spec.rb

@@ -1521,6 +1521,7 @@ RSpec.describe Group do
 
         shared_examples 'returning the right value for user_cap_reached?' do
           before do
+            allow(root_group).to receive(:user_cap_available?).and_return(true)
             root_group.namespace_settings.update!(new_user_signups_cap: new_user_signups_cap)
           end
 
@@ -1571,6 +1572,43 @@ RSpec.describe Group do
     end
   end
 
+  describe '#shared_externally?' do
+    let_it_be(:group, refind: true) { create(:group) }
+    let_it_be(:subgroup_1) { create(:group, parent: group) }
+    let_it_be(:subgroup_2) { create(:group, parent: group) }
+    let_it_be(:external_group) { create(:group) }
+
+    subject(:shared_externally?) { group.shared_externally? }
+
+    it 'returns false when the group is not shared outside of the namespace hierarchy' do
+      expect(shared_externally?).to be false
+    end
+
+    it 'returns true when the group is shared outside of the namespace hierarchy' do
+      create(:group_group_link, shared_group: group, shared_with_group: external_group)
+
+      expect(shared_externally?).to be true
+    end
+
+    it 'returns false when the group is shared internally within the namespace hierarchy' do
+      create(:group_group_link, shared_group: subgroup_1, shared_with_group: subgroup_2)
+
+      expect(shared_externally?).to be false
+    end
+
+    it 'returns true when a subgroup is shared outside of the namespace hierarchy' do
+      create(:group_group_link, shared_group: subgroup_1, shared_with_group: external_group)
+
+      expect(shared_externally?).to be true
+    end
+
+    it 'returns false when the only shared groups are outside of the namespace hierarchy' do
+      create(:group_group_link)
+
+      expect(shared_externally?).to be false
+    end
+  end
+
   it_behaves_like 'can move repository storage' do
     let_it_be(:container) { create(:group, :wiki_repo) }
 

ee/spec/models/namespace_setting_spec.rb

@@ -96,4 +96,128 @@ RSpec.describe NamespaceSetting do
       end
     end
   end
+
+  context 'validating new_user_signup_cap' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:feature_available, :old_value, :new_value, :expectation) do
+      true  | nil | 10 | true
+      true  | 0   | 10 | true
+      true  | 0   | 0  | true
+      false | nil | 10 | false
+      false | 10  | 10 | true
+    end
+
+    with_them do
+      let(:setting) { build(:namespace_settings, new_user_signups_cap: old_value) }
+      let(:group) { create(:group, namespace_settings: setting) }
+
+      before do
+        allow(group).to receive(:user_cap_available?).and_return feature_available
+
+        setting.new_user_signups_cap = new_value
+      end
+
+      it 'returns the expected response' do
+        expect(setting.valid?).to be expectation
+        expect(setting.errors.messages[:new_user_signups_cap]).to include("cannot be enabled") unless expectation
+      end
+    end
+
+    context 'when enabling the setting' do
+      let(:feature_available) { true }
+
+      before do
+        allow(group).to receive(:user_cap_available?).and_return feature_available
+
+        setting.new_user_signups_cap = 10
+      end
+
+      shared_examples 'user cap is not available' do
+        it 'is invalid' do
+          expect(setting.valid?).to be false
+          expect(setting.errors.messages[:new_user_signups_cap]).to include("cannot be enabled")
+        end
+      end
+
+      context 'when the group is a subgroup' do
+        before do
+          group.parent = build(:group)
+        end
+
+        it_behaves_like 'user cap is not available'
+      end
+
+      context 'when the group is shared externally' do
+        before do
+          create(:group_group_link, shared_group: group)
+        end
+
+        it_behaves_like 'user cap is not available'
+      end
+
+      context 'when the namespace is a user' do
+        let(:user) { create(:user) }
+        let(:setting) { user.namespace.namespace_settings }
+
+        it_behaves_like 'user cap is not available'
+      end
+    end
+  end
+
+  context 'hooks related to group user cap update' do
+    let(:group) { create(:group) }
+    let(:settings) { group.namespace_settings }
+
+    before do
+      allow(group).to receive(:root?).and_return(true)
+      allow(group).to receive(:user_cap_available?).and_return(true)
+
+      group.namespace_settings.update!(new_user_signups_cap: user_cap)
+    end
+
+    context 'when updating a group with a user cap' do
+      let(:user_cap) { nil }
+
+      it 'also sets share_with_group_lock and prevent_sharing_groups_outside_hierarchy to true' do
+        expect(group.new_user_signups_cap).to be_nil
+        expect(group.share_with_group_lock).to be_falsey
+        expect(settings.prevent_sharing_groups_outside_hierarchy).to be_falsey
+
+        settings.update!(new_user_signups_cap: 10)
+        group.reload
+
+        expect(group.new_user_signups_cap).to eq(10)
+        expect(group.share_with_group_lock).to be_truthy
+        expect(settings.reload.prevent_sharing_groups_outside_hierarchy).to be_truthy
+      end
+
+      it 'has share_with_group_lock and prevent_sharing_groups_outside_hierarchy returning true for descendent groups' do
+        descendent = create(:group, parent: group)
+        desc_settings = descendent.namespace_settings
+
+        expect(descendent.share_with_group_lock).to be_falsey
+        expect(desc_settings.prevent_sharing_groups_outside_hierarchy).to be_falsey
+
+        settings.update!(new_user_signups_cap: 10)
+
+        expect(descendent.reload.share_with_group_lock).to be_truthy
+        expect(desc_settings.reload.prevent_sharing_groups_outside_hierarchy).to be_truthy
+      end
+    end
+
+    context 'when removing a user cap from namespace settings' do
+      let(:user_cap) { 10 }
+
+      it 'leaves share_with_group_lock and prevent_sharing_groups_outside_hierarchy set to true to the related group' do
+        expect(group.share_with_group_lock).to be_truthy
+        expect(settings.prevent_sharing_groups_outside_hierarchy).to be_truthy
+
+        settings.update!(new_user_signups_cap: nil)
+
+        expect(group.reload.share_with_group_lock).to be_truthy
+        expect(settings.reload.prevent_sharing_groups_outside_hierarchy).to be_truthy
+      end
+    end
+  end
 end

locale/gitlab.pot

@@ -41526,6 +41526,9 @@ msgstr ""
 msgid "cannot be changed if shared runners are enabled"
 msgstr ""
 
+msgid "cannot be enabled"
+msgstr ""
+
 msgid "cannot be enabled because parent group does not allow it"
 msgstr ""
 

spec/models/namespace_setting_spec.rb

@@ -126,57 +126,4 @@ RSpec.describe NamespaceSetting, type: :model do
       end
     end
   end
-
-  describe 'hooks related to group user cap update' do
-    let(:settings) { create(:namespace_settings, new_user_signups_cap: user_cap) }
-    let(:group) { create(:group, namespace_settings: settings) }
-
-    before do
-      allow(group).to receive(:root?).and_return(true)
-    end
-
-    context 'when updating a group with a user cap' do
-      let(:user_cap) { nil }
-
-      it 'also sets share_with_group_lock and prevent_sharing_groups_outside_hierarchy to true' do
-        expect(group.new_user_signups_cap).to be_nil
-        expect(group.share_with_group_lock).to be_falsey
-        expect(settings.prevent_sharing_groups_outside_hierarchy).to be_falsey
-
-        settings.update!(new_user_signups_cap: 10)
-        group.reload
-
-        expect(group.new_user_signups_cap).to eq(10)
-        expect(group.share_with_group_lock).to be_truthy
-        expect(settings.reload.prevent_sharing_groups_outside_hierarchy).to be_truthy
-      end
-
-      it 'has share_with_group_lock and prevent_sharing_groups_outside_hierarchy returning true for descendent groups' do
-        descendent = create(:group, parent: group)
-        desc_settings = descendent.namespace_settings
-
-        expect(descendent.share_with_group_lock).to be_falsey
-        expect(desc_settings.prevent_sharing_groups_outside_hierarchy).to be_falsey
-
-        settings.update!(new_user_signups_cap: 10)
-
-        expect(descendent.reload.share_with_group_lock).to be_truthy
-        expect(desc_settings.reload.prevent_sharing_groups_outside_hierarchy).to be_truthy
-      end
-    end
-
-    context 'when removing a user cap from namespace settings' do
-      let(:user_cap) { 10 }
-
-      it 'leaves share_with_group_lock and prevent_sharing_groups_outside_hierarchy set to true to the related group' do
-        expect(group.share_with_group_lock).to be_truthy
-        expect(settings.prevent_sharing_groups_outside_hierarchy).to be_truthy
-
-        settings.update!(new_user_signups_cap: nil)
-
-        expect(group.reload.share_with_group_lock).to be_truthy
-        expect(settings.reload.prevent_sharing_groups_outside_hierarchy).to be_truthy
-      end
-    end
-  end
 end