Merge branch 'security-ensure-prerequisites-are-met-before-account-deletion' into 'master'

Ensure user has no solo owned groups before triggering account deletion

See merge request gitlab-org/security/gitlab!913

app/controllers/admin/users_controller.rb

@@ -5,6 +5,7 @@ class Admin::UsersController < Admin::ApplicationController
 
   before_action :user, except: [:index, :new, :create]
   before_action :check_impersonation_availability, only: :impersonate
+  before_action :ensure_destroy_prerequisites_met, only: [:destroy]
 
   def index
     @users = User.filter_items(params[:filter]).order_name_asc
@@ -174,7 +175,7 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def destroy
-    user.delete_async(deleted_by: current_user, params: params.permit(:hard_delete))
+    user.delete_async(deleted_by: current_user, params: destroy_params)
 
     respond_to do |format|
       format.html { redirect_to admin_users_path, status: :found, notice: _("The user is being deleted.") }
@@ -203,6 +204,24 @@ class Admin::UsersController < Admin::ApplicationController
     user != current_user
   end
 
+  def destroy_params
+    params.permit(:hard_delete)
+  end
+
+  def ensure_destroy_prerequisites_met
+    return if hard_delete?
+
+    if user.solo_owned_groups.present?
+      message = s_('AdminUsers|You must transfer ownership or delete the groups owned by this user before you can delete their account')
+
+      redirect_to admin_user_path(user), status: :see_other, alert: message
+    end
+  end
+
+  def hard_delete?
+    destroy_params[:hard_delete]
+  end
+
   def user
     @user ||= find_routable!(User, params[:id])
   end

app/controllers/registrations_controller.rb

@@ -10,7 +10,7 @@ class RegistrationsController < Devise::RegistrationsController
 
   skip_before_action :required_signup_info, :check_two_factor_requirement, only: [:welcome, :update_registration]
   prepend_before_action :check_captcha, only: :create
-  before_action :whitelist_query_limiting, only: [:destroy]
+  before_action :whitelist_query_limiting, :ensure_destroy_prerequisites_met, only: [:destroy]
   before_action :ensure_terms_accepted,
     if: -> { action_name == 'create' && Gitlab::CurrentSettings.current_application_settings.enforce_terms? }
   before_action :load_recaptcha, only: :new
@@ -124,6 +124,14 @@ class RegistrationsController < Devise::RegistrationsController
 
   private
 
+  def ensure_destroy_prerequisites_met
+    if current_user.solo_owned_groups.present?
+      redirect_to profile_account_path,
+        status: :see_other,
+        alert: s_('Profiles|You must transfer ownership or delete groups you are an owner of before you can delete your account')
+    end
+  end
+
   def user_created_message(confirmed: false)
     "User Created: username=#{resource.username} email=#{resource.email} ip=#{request.remote_ip} confirmed:#{confirmed}"
   end

changelogs/unreleased/security-ensure-prerequisites-are-met-before-account-deletion.yml

+---
+title: Ensure user has no solo owned groups before triggering account deletion
+merge_request:
+author:
+type: security

locale/gitlab.pot

@@ -2151,6 +2151,9 @@ msgstr ""
 msgid "AdminUsers|You cannot remove your own admin rights."
 msgstr ""
 
+msgid "AdminUsers|You must transfer ownership or delete the groups owned by this user before you can delete their account"
+msgstr ""
+
 msgid "Administration"
 msgstr ""
 
@@ -19554,6 +19557,9 @@ msgstr ""
 msgid "Profiles|You don't have access to delete this user."
 msgstr ""
 
+msgid "Profiles|You must transfer ownership or delete groups you are an owner of before you can delete your account"
+msgstr ""
+
 msgid "Profiles|You must transfer ownership or delete these groups before you can delete your account."
 msgstr ""
 

spec/controllers/admin/users_controller_spec.rb

@@ -42,7 +42,7 @@ RSpec.describe Admin::UsersController do
     end
   end
 
-  describe 'DELETE #user with projects', :sidekiq_might_not_need_inline do
+  describe 'DELETE #destroy', :sidekiq_might_not_need_inline do
     let(:project) { create(:project, namespace: user.namespace) }
     let!(:issue) { create(:issue, author: user) }
 
@@ -65,6 +65,41 @@ RSpec.describe Admin::UsersController do
       expect(User.exists?(user.id)).to be_falsy
       expect(Issue.exists?(issue.id)).to be_falsy
     end
+
+    context 'prerequisites for account deletion' do
+      context 'solo-owned groups' do
+        let(:group) { create(:group) }
+
+        context 'if the user is the sole owner of at least one group' do
+          before do
+            create(:group_member, :owner, group: group, user: user)
+          end
+
+          context 'soft-delete' do
+            it 'fails' do
+              delete :destroy, params: { id: user.username }
+
+              message = s_('AdminUsers|You must transfer ownership or delete the groups owned by this user before you can delete their account')
+
+              expect(flash[:alert]).to eq(message)
+              expect(response).to have_gitlab_http_status(:see_other)
+              expect(response).to redirect_to admin_user_path(user)
+              expect(User.exists?(user.id)).to be_truthy
+            end
+          end
+
+          context 'hard-delete' do
+            it 'succeeds' do
+              delete :destroy, params: { id: user.username, hard_delete: true }
+
+              expect(response).to redirect_to(admin_users_path)
+              expect(flash[:notice]).to eq(_('The user is being deleted.'))
+              expect(User.exists?(user.id)).to be_falsy
+            end
+          end
+        end
+      end
+    end
   end
 
   describe 'PUT #activate' do

spec/controllers/registrations_controller_spec.rb

@@ -459,6 +459,24 @@ RSpec.describe RegistrationsController do
         expect_success
       end
     end
+
+    context 'prerequisites for account deletion' do
+      context 'solo-owned groups' do
+        let(:group) { create(:group) }
+
+        context 'if the user is the sole owner of at least one group' do
+          before do
+            create(:group_member, :owner, group: group, user: user)
+          end
+
+          it 'fails' do
+            delete :destroy, params: { password: '12345678' }
+
+            expect_failure(s_('Profiles|You must transfer ownership or delete groups you are an owner of before you can delete your account'))
+          end
+        end
+      end
+    end
   end
 
   describe '#welcome' do