Merge branch '411832-namespace-required' into 'master'

Ensure Organization is set while creating a new Group

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142155



Merged-by: Gabriel Mazetto <brodock@gmail.com>
Approved-by: Doug Stull <dstull@gitlab.com>
Approved-by: Gabriel Mazetto <brodock@gmail.com>
Reviewed-by: Gabriel Mazetto <brodock@gmail.com>
Reviewed-by: Doug Stull <dstull@gitlab.com>
Co-authored-by: Rutger Wessels <rwessels@gitlab.com>

app/services/groups/create_service.rb

@@ -2,6 +2,8 @@
 
 module Groups
   class CreateService < Groups::BaseService
+    include Organization::CurrentOrganization
+
     def initialize(user, params = {})
       @current_user = user
       @params = params.dup
@@ -15,6 +17,8 @@ def execute
 
       @group = Group.new(params.except(*::NamespaceSetting.allowed_namespace_settings_params))
 
+      set_organization unless @params[:organization_id]
+
       @group.build_namespace_settings
       handle_namespace_settings
 
@@ -96,8 +100,6 @@ def can_create_group?
         # We are unsetting this here to match behavior of invalid parent_id above and protect against possible
         # committing to the database of a value that isn't allowed.
         @group.organization = nil
-        message = s_("CreateGroup|You don't have permission to create a group in the provided organization.")
-        @group.errors.add(:organization_id, message)
 
         return false
       end
@@ -105,6 +107,25 @@ def can_create_group?
       true
     end
 
+    def can_create_group_in_organization?
+      return true if can?(current_user, :create_group, @group.organization)
+
+      message = s_("CreateGroup|You don't have permission to create a group in the provided organization.")
+      @group.errors.add(:organization_id, message)
+
+      false
+    end
+
+    def matches_parent_organization?
+      return true if @group.parent_id.blank?
+      return true if @group.parent.organization_id == @group.organization_id
+
+      message = s_("CreateGroup|You can't create a group in a different organization than the parent group.")
+      @group.errors.add(:organization_id, message)
+
+      false
+    end
+
     def organization_setting_valid?
       # we check for the params presence explicitly since:
       # 1. We have a default organization_id at db level set and organization exists and may not have the entry
@@ -115,7 +136,7 @@ def organization_setting_valid?
       return true if params[:organization_id].blank?
       return true if @group.organization.blank?
 
-      can?(current_user, :create_group, @group.organization)
+      can_create_group_in_organization? && matches_parent_organization?
     end
 
     def can_use_visibility_level?
@@ -139,6 +160,14 @@ def inherit_group_shared_runners_settings
       @group.shared_runners_enabled = @group.parent.shared_runners_enabled
       @group.allow_descendants_override_disabled_shared_runners = @group.parent.allow_descendants_override_disabled_shared_runners
     end
+
+    def set_organization
+      if @group.parent_id
+        @group.organization = @group.parent.organization
+      elsif current_organization
+        @group.organization = current_organization
+      end
+    end
   end
 end
 

locale/gitlab.pot

@@ -14703,6 +14703,9 @@ msgstr ""
 msgid "CreateGitTag|Set tag message"
 msgstr ""
+msgid "CreateGroup|You can't create a group in a different organization than the parent group."
+msgstr ""
+
 msgid "CreateGroup|You don't have permission to create a group in the provided organization."
 msgstr ""

spec/services/groups/create_service_spec.rb

@@ -127,6 +127,8 @@
   end
 
   context 'when creating a group within an organization' do
+    let_it_be(:other_organization) { create(:organization, name: 'Other Organization') }
+
     context 'when organization is provided' do
       let_it_be(:organization) { create(:organization) }
       let(:extra_params) { { organization_id: organization.id } }
@@ -154,14 +156,58 @@
           expect(created_group.organization_id).to be_nil
         end
       end
+
+      context 'when parent group is different from provided group' do
+        let_it_be(:parent_group) { create(:group, organization: other_organization) }
+        let(:extra_params) { { parent_id: parent_group.id, organization_id: organization.id } }
+
+        before_all do
+          create(:organization_user, user: user, organization: organization)
+          create(:organization_user, user: user, organization: other_organization)
+          parent_group.add_owner(user)
+        end
+
+        it_behaves_like 'does not create a group'
+
+        it 'returns an error and does not set organization_id' do
+          expect(created_group.errors[:organization_id].first)
+            .to eq(s_("CreateGroup|You can't create a group in a different organization than the parent group."))
+          expect(created_group.organization_id).to be_nil
+        end
+      end
     end
 
-    context 'when organization is the default organization and not set by params' do
-      before do
-        create(:organization, :default)
+    context 'when organization is not set by params' do
+      let_it_be(:default_organization) { create(:organization, :default) }
+      let_it_be(:current_organization) { create(:organization, name: 'Current Organization') }
+
+      context 'and the parent of the group has an organization' do
+        let_it_be(:parent_group) { create(:group, organization: other_organization) }
+
+        let(:extra_params) { { parent_id: parent_group.id } }
+
+        it 'creates group with the parent group organization' do
+          expect(created_group.organization).to eq(other_organization)
+        end
       end
 
-      it_behaves_like 'creating a group'
+      context 'and current_organization is known' do
+        before do
+          allow_next_instance_of(Groups::CreateService) do |instance|
+            allow(instance).to receive(:current_organization).and_return(current_organization)
+          end
+        end
+
+        it 'creates group with the current organization' do
+          expect(created_group.organization).to eq(current_organization)
+        end
+      end
+
+      context 'and no group can be found' do
+        it 'creates group with the default organization' do
+          expect(created_group.organization).to eq(default_organization)
+        end
+      end
     end
   end