Merge branch '28243-check-for-docker-images-before-renaming-group' into 'master'

Resolve "Check for docker images before renaming group"

See merge request gitlab-org/gitlab!17583

app/controllers/groups_controller.rb

@@ -104,7 +104,6 @@ class GroupsController < Groups::ApplicationController
       redirect_to edit_group_path(@group, anchor: params[:update_section]), notice: "Group '#{@group.name}' was successfully updated."
     else
       @group.path = @group.path_before_last_save || @group.path_was
-
       render action: "edit"
     end
   end
@@ -124,7 +123,7 @@ class GroupsController < Groups::ApplicationController
       flash[:notice] = "Group '#{@group.name}' was successfully transferred."
       redirect_to group_path(@group)
     else
-      flash[:alert] = service.error
+      flash[:alert] = service.error.html_safe
       redirect_to edit_group_path(@group)
     end
   end

app/models/group.rb

@@ -259,6 +259,10 @@ class Group < Namespace
     members_with_parents.maintainers.exists?(user_id: user)
   end
 
+  def has_container_repositories?
+    container_repositories.exists?
+  end
+
   # @deprecated
   alias_method :has_master?, :has_maintainer?
 

app/services/groups/transfer_service.rb

@@ -7,7 +7,8 @@ module Groups
       namespace_with_same_path: s_('TransferGroup|The parent group already has a subgroup with the same path.'),
       group_is_already_root: s_('TransferGroup|Group is already a root group.'),
       same_parent_as_current: s_('TransferGroup|Group is already associated to the parent group.'),
-      invalid_policies: s_("TransferGroup|You don't have enough permissions.")
+      invalid_policies: s_("TransferGroup|You don't have enough permissions."),
+      group_contains_images: s_('TransferGroup|Cannot update the path because there are projects under this group that contain Docker images in their Container Registry. Please remove the images from your projects first and try again.')
     }.freeze
 
     TransferError = Class.new(StandardError)
@@ -46,6 +47,7 @@ module Groups
       raise_transfer_error(:same_parent_as_current) if same_parent?
       raise_transfer_error(:invalid_policies) unless valid_policies?
       raise_transfer_error(:namespace_with_same_path) if namespace_with_same_path?
+      raise_transfer_error(:group_contains_images) if group_projects_contain_registry_images?
     end
 
     def group_is_already_root?
@@ -72,6 +74,10 @@ module Groups
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    def group_projects_contain_registry_images?
+      @group.has_container_repositories?
+    end
+
     def update_group_attributes
       if @new_parent_group && @new_parent_group.visibility_level < @group.visibility_level
         update_children_and_projects_visibility

app/services/groups/update_service.rb

@@ -8,6 +8,11 @@ module Groups
       reject_parent_id!
       remove_unallowed_params
 
+      if renaming_group_with_container_registry_images?
+        group.errors.add(:base, container_images_error)
+        return false
+      end
+
       return false unless valid_visibility_level_change?(group, params[:visibility_level])
 
       return false unless valid_share_with_group_lock_change?
@@ -35,6 +40,17 @@ module Groups
       # overridden in EE
     end
 
+    def renaming_group_with_container_registry_images?
+      new_path = params[:path]
+
+      new_path && new_path != group.path &&
+        group.has_container_repositories?
+    end
+
+    def container_images_error
+      s_("GroupSettings|Cannot update the path because there are projects under this group that contain Docker images in their Container Registry. Please remove the images from your projects first and try again.")
+    end
+
     def after_update
       if group.previous_changes.include?(:visibility_level) && group.private?
         # don't enqueue immediately to prevent todos removal in case of a mistake

changelogs/unreleased/28243-check-for-docker-images-before-renaming-group.yml

+---
+title: Prevents a group path change when a project inside the group has container
+  registry images
+merge_request: 17583
+author:
+type: fixed

locale/gitlab.pot

@@ -8282,6 +8282,9 @@ msgstr ""
 msgid "GroupSettings|Be careful. Changing a group's parent can have unintended %{side_effects_link_start}side effects%{side_effects_link_end}."
 msgstr ""
 
+msgid "GroupSettings|Cannot update the path because there are projects under this group that contain Docker images in their Container Registry. Please remove the images from your projects first and try again."
+msgstr ""
+
 msgid "GroupSettings|Change group path"
 msgstr ""
 
@@ -17290,6 +17293,9 @@ msgstr ""
 msgid "Transfer project"
 msgstr ""
 
+msgid "TransferGroup|Cannot update the path because there are projects under this group that contain Docker images in their Container Registry. Please remove the images from your projects first and try again."
+msgstr ""
+
 msgid "TransferGroup|Database is not supported."
 msgstr ""
 

spec/controllers/groups_controller_spec.rb

@@ -385,6 +385,29 @@ describe GroupsController do
       expect(response).to have_gitlab_http_status(302)
       expect(group.reload.project_creation_level).to eq(::Gitlab::Access::MAINTAINER_PROJECT_ACCESS)
     end
+
+    context 'when a project inside the group has container repositories' do
+      before do
+        stub_container_registry_config(enabled: true)
+        stub_container_registry_tags(repository: /image/, tags: %w[rc1])
+        create(:container_repository, project: project, name: :image)
+      end
+
+      it 'does allow the group to be renamed' do
+        post :update, params: { id: group.to_param, group: { name: 'new_name' } }
+
+        expect(controller).to set_flash[:notice]
+        expect(response).to have_gitlab_http_status(302)
+        expect(group.reload.name).to eq('new_name')
+      end
+
+      it 'does not allow to path of the group to be changed' do
+        post :update, params: { id: group.to_param, group: { path: 'new_path' } }
+
+        expect(assigns(:group).errors[:base].first).to match(/Docker images in their Container Registry/)
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
   end
 
   describe '#ensure_canonical_path' do
@@ -673,6 +696,28 @@ describe GroupsController do
         expect(response).to have_gitlab_http_status(404)
       end
     end
+
+    context 'transferring when a project has container images' do
+      let(:group) { create(:group, :public, :nested) }
+      let!(:group_member) { create(:group_member, :owner, group: group, user: user) }
+
+      before do
+        stub_container_registry_config(enabled: true)
+        stub_container_registry_tags(repository: /image/, tags: %w[rc1])
+        create(:container_repository, project: project, name: :image)
+
+        put :transfer,
+          params: {
+            id: group.to_param,
+            new_parent_group_id: ''
+          }
+      end
+
+      it 'does not allow the group to be transferred' do
+        expect(controller).to set_flash[:alert].to match(/Docker images in their Container Registry/)
+        expect(response).to redirect_to(edit_group_path(group))
+      end
+    end
   end
 
   context 'token authentication' do

spec/services/groups/transfer_service_spec.rb

@@ -426,5 +426,22 @@ describe Groups::TransferService do
         end
       end
     end
+
+    context 'when a project in group has container images' do
+      let(:group) { create(:group, :public, :nested) }
+      let!(:project) { create(:project, :repository, :public, namespace: group) }
+
+      before do
+        stub_container_registry_tags(repository: /image/, tags: %w[rc1])
+        create(:container_repository, project: project, name: :image)
+        create(:group_member, :owner, group: new_parent_group, user: user)
+      end
+
+      it 'does not allow group to be transferred' do
+        transfer_service.execute(new_parent_group)
+
+        expect(transfer_service.error).to match(/Docker images in their Container Registry/)
+      end
+    end
   end
 end

spec/services/groups/update_service_spec.rb

@@ -148,6 +148,30 @@ describe Groups::UpdateService do
     end
   end
 
+  context 'projects in group have container images' do
+    let(:service) { described_class.new(public_group, user, path: SecureRandom.hex) }
+    let(:project) { create(:project, :internal, group: public_group) }
+
+    before do
+      stub_container_registry_tags(repository: /image/, tags: %w[rc1])
+      create(:container_repository, project: project, name: :image)
+    end
+
+    it 'does not allow path to be changed' do
+      result = described_class.new(public_group, user, path: 'new-path').execute
+
+      expect(result).to eq false
+      expect(public_group.errors[:base].first).to match(/Docker images in their Container Registry/)
+    end
+
+    it 'allows other settings to be changed' do
+      result = described_class.new(public_group, user, name: 'new-name').execute
+
+      expect(result).to eq true
+      expect(public_group.reload.name).to eq('new-name')
+    end
+  end
+
   context 'for a subgroup' do
     let(:subgroup) { create(:group, :private, parent: private_group) }