Merge branch '280596-user-admin-approval-reject-user' into 'master'

Send user email when their request to join an instance is denied

See merge request gitlab-org/gitlab!48185

app/controllers/admin/users_controller.rb

@@ -72,6 +72,16 @@ class Admin::UsersController < Admin::ApplicationController
     end
   end
 
+  def reject
+    result = Users::RejectService.new(current_user).execute(user)
+
+    if result[:status] == :success
+      redirect_to admin_users_path, status: :found, notice: _("You've rejected %{user}" % { user: user.name })
+    else
+      redirect_back_or_admin_user(alert: result[:message])
+    end
+  end
+
   def activate
     return redirect_back_or_admin_user(notice: _("Error occurred. A blocked user must be unblocked to be activated")) if user.blocked?
 

app/mailers/emails/profile.rb

@@ -18,6 +18,14 @@ module Emails
         subject: subject(_("GitLab Account Request")))
     end
 
+    def user_admin_rejection_email(name, email)
+      @name = name
+
+      profile_email_with_layout(
+        to: email,
+        subject: subject(_("GitLab account request rejected")))
+    end
+
     # rubocop: disable CodeReuse/ActiveRecord
     def new_ssh_key_email(key_id)
       @key = Key.find_by(id: key_id)

app/policies/global_policy.rb

@@ -99,6 +99,7 @@ class GlobalPolicy < BasePolicy
     enable :read_custom_attribute
     enable :update_custom_attribute
     enable :approve_user
+    enable :reject_user
   end
 
   # We can't use `read_statistics` because the user may have different permissions for different projects

app/services/notification_service.rb

@@ -380,6 +380,10 @@ class NotificationService
     end
   end
 
+  def user_admin_rejection(name, email)
+    mailer.user_admin_rejection_email(name, email).deliver_later
+  end
+
   # Members
   def new_access_request(member)
     return true unless member.notifiable?(:subscription)

app/services/users/reject_service.rb

+# frozen_string_literal: true
+
+module Users
+  class RejectService < BaseService
+    def initialize(current_user)
+      @current_user = current_user
+    end
+
+    def execute(user)
+      return error(_('You are not allowed to reject a user')) unless allowed?
+      return error(_('This user does not have a pending request')) unless user.blocked_pending_approval?
+
+      user.delete_async(deleted_by: current_user, params: { hard_delete: true })
+
+      NotificationService.new.user_admin_rejection(user.name, user.email)
+
+      success
+    end
+
+    private
+
+    attr_reader :current_user
+
+    def allowed?
+      can?(current_user, :reject_user)
+    end
+  end
+end

app/views/admin/users/_reject_pending_user.html.haml

+.card.border-danger
+  .card-header.bg-danger.gl-text-white
+    = s_('AdminUsers|This user has requested access')
+  .card-body
+    = render partial: 'admin/users/user_reject_effects'
+    %br
+    = link_to s_('AdminUsers|Reject request'), reject_admin_user_path(user), method: :delete, class: "btn gl-button btn-danger", data: { confirm: s_('AdminUsers|Are you sure?') }

app/views/admin/users/_user.html.haml

@@ -37,8 +37,7 @@
                 - elsif user.blocked?
                   - if user.blocked_pending_approval?
                     = link_to s_('AdminUsers|Approve'), approve_admin_user_path(user), method: :put
-                    %button.btn.btn-default-tertiary.js-confirm-modal-button{ data: user_block_data(user, user_block_effects) }
-                      = s_('AdminUsers|Block')
+                    = link_to s_('AdminUsers|Reject'), reject_admin_user_path(user), method: :delete
                   - else
                     %button.btn.btn-default-tertiary.js-confirm-modal-button{ data: user_unblock_data(user) }
                       = s_('AdminUsers|Unblock')
@@ -56,7 +55,7 @@
               - if user.access_locked?
                 %li
                   = link_to _('Unlock'), unlock_admin_user_path(user), method: :put, data: { confirm: _('Are you sure?') }
-            - if can?(current_user, :destroy_user, user)
+            - if can?(current_user, :destroy_user, user) && !user.blocked_pending_approval?
               %li.divider
               - if user.can_be_removed?
                 %li

app/views/admin/users/_user_reject_effects.html.haml

+%p
+  = s_('AdminUsers|Rejected users:')
+%ul
+  %li
+    = s_('AdminUsers|Cannot sign in or access instance information')
+  %li
+    = s_('AdminUsers|Will be deleted')
+%p
+  - link_start = '<a href="%{url}">'.html_safe % { url: help_page_path("user/profile/account/delete_account", anchor: "associated-records") }
+  = s_('AdminUsers|For more information, please refer to the %{link_start}user account deletion documentation.%{link_end}').html_safe % { link_start: link_start, link_end: '</a>'.html_safe }

app/views/admin/users/show.html.haml

@@ -172,7 +172,7 @@
       - if @user.blocked?
         - if @user.blocked_pending_approval?
           = render 'admin/users/approve_user', user: @user
-          = render 'admin/users/block_user', user: @user
+          = render 'admin/users/reject_pending_user', user: @user
         - else
           .card.border-info
             .card-header.gl-bg-blue-500.gl-text-white
@@ -196,52 +196,52 @@
             %p This user has been temporarily locked due to excessive number of failed logins. You may manually unlock the account.
             %br
             = link_to 'Unlock user', unlock_admin_user_path(@user), method: :put, class: "btn gl-button btn-info", data: { confirm: 'Are you sure?' }
-
-      .card.border-danger
-        .card-header.bg-danger.text-white
-          = s_('AdminUsers|Delete user')
-        .card-body
-          - if @user.can_be_removed? && can?(current_user, :destroy_user, @user)
-            %p Deleting a user has the following effects:
-            = render 'users/deletion_guidance', user: @user
-            %br
-            %button.delete-user-button.btn.gl-button.btn-danger{ data: { 'gl-modal-action': 'delete',
-              delete_user_url: admin_user_path(@user),
-              block_user_url: block_admin_user_path(@user),
-              username: sanitize_name(@user.name) } }
-              = s_('AdminUsers|Delete user')
-          - else
-            - if @user.solo_owned_groups.present?
-              %p
-                This user is currently an owner in these groups:
-                %strong= @user.solo_owned_groups.map(&:name).join(', ')
+      - if !@user.blocked_pending_approval?
+        .card.border-danger
+          .card-header.bg-danger.text-white
+            = s_('AdminUsers|Delete user')
+          .card-body
+            - if @user.can_be_removed? && can?(current_user, :destroy_user, @user)
+              %p Deleting a user has the following effects:
+              = render 'users/deletion_guidance', user: @user
+              %br
+              %button.delete-user-button.btn.gl-button.btn-danger{ data: { 'gl-modal-action': 'delete',
+                delete_user_url: admin_user_path(@user),
+                block_user_url: block_admin_user_path(@user),
+                username: sanitize_name(@user.name) } }
+                = s_('AdminUsers|Delete user')
+            - else
+              - if @user.solo_owned_groups.present?
+                %p
+                  This user is currently an owner in these groups:
+                  %strong= @user.solo_owned_groups.map(&:name).join(', ')
+                %p
+                  You must transfer ownership or delete these groups before you can delete this user.
+              - else
+                %p
+                  You don't have access to delete this user.
+
+        .card.border-danger
+          .card-header.bg-danger.text-white
+            = s_('AdminUsers|Delete user and contributions')
+          .card-body
+            - if can?(current_user, :destroy_user, @user)
               %p
-                You must transfer ownership or delete these groups before you can delete this user.
+                This option deletes the user and any contributions that
+                would usually be moved to the
+                = succeed "." do
+                  = link_to "system ghost user", help_page_path("user/profile/account/delete_account")
+                As well as the user's personal projects, groups owned solely by
+                the user, and projects in them, will also be removed. Commits
+                to other projects are unaffected.
+              %br
+                %button.delete-user-button.btn.gl-button.btn-danger{ data: { 'gl-modal-action': 'delete-with-contributions',
+                  delete_user_url: admin_user_path(@user, hard_delete: true),
+                  block_user_url: block_admin_user_path(@user),
+                  username: @user.name } }
+                  = s_('AdminUsers|Delete user and contributions')
             - else
               %p
                 You don't have access to delete this user.
 
-      .card.border-danger
-        .card-header.bg-danger.text-white
-          = s_('AdminUsers|Delete user and contributions')
-        .card-body
-          - if can?(current_user, :destroy_user, @user)
-            %p
-              This option deletes the user and any contributions that
-              would usually be moved to the
-              = succeed "." do
-                = link_to "system ghost user", help_page_path("user/profile/account/delete_account")
-              As well as the user's personal projects, groups owned solely by
-              the user, and projects in them, will also be removed. Commits
-              to other projects are unaffected.
-            %br
-              %button.delete-user-button.btn.gl-button.btn-danger{ data: { 'gl-modal-action': 'delete-with-contributions',
-                delete_user_url: admin_user_path(@user, hard_delete: true),
-                block_user_url: block_admin_user_path(@user),
-                username: @user.name } }
-                = s_('AdminUsers|Delete user and contributions')
-          - else
-            %p
-              You don't have access to delete this user.
-
 = render partial: 'admin/users/modals'

app/views/notify/user_admin_rejection_email.html.haml

+= email_default_heading(_('Hello %{name},') % { name: @name })
+%p
+  = _('Your request to join %{host} has been rejected.').html_safe % { host: link_to(root_url, root_url) }
+%p
+  = _('Please contact your GitLab administrator if you think this is an error.')

app/views/notify/user_admin_rejection_email.text.erb

+<%= _('Hello %{name},') % { name: @name } %>
+
+<%= _('Your request to join %{host} has been rejected.') % { host: root_url } %>
+
+<%= _('Please contact your GitLab administrator if you think this is an error.') %>
+

changelogs/unreleased/280596-user-admin-approval-reject-user.yml

+---
+title: Email user when registration request is rejected
+merge_request: 48185
+author:
+type: added

config/routes/admin.rb

@@ -18,6 +18,7 @@ namespace :admin do
       put :unlock
       put :confirm
       put :approve
+      delete :reject
       post :impersonate
       patch :disable_two_factor
       delete 'remove/:email_id', action: 'remove_email', as: 'remove_email'

locale/gitlab.pot

@@ -2119,6 +2119,9 @@ msgstr ""
 msgid "AdminUsers|Blocking user has the following effects:"
 msgstr ""
 
+msgid "AdminUsers|Cannot sign in or access instance information"
+msgstr ""
+
 msgid "AdminUsers|Cannot unblock LDAP blocked users"
 msgstr ""
 
@@ -2155,6 +2158,9 @@ msgstr ""
 msgid "AdminUsers|External users cannot see internal or private projects unless access is explicitly granted. Also, external users cannot create projects, groups, or personal snippets."
 msgstr ""
 
+msgid "AdminUsers|For more information, please refer to the %{link_start}user account deletion documentation.%{link_end}"
+msgstr ""
+
 msgid "AdminUsers|Is using seat"
 msgstr ""
 
@@ -2191,6 +2197,15 @@ msgstr ""
 msgid "AdminUsers|Regular users have access to their groups and projects"
 msgstr ""
 
+msgid "AdminUsers|Reject"
+msgstr ""
+
+msgid "AdminUsers|Reject request"
+msgstr ""
+
+msgid "AdminUsers|Rejected users:"
+msgstr ""
+
 msgid "AdminUsers|Restore user access to the account, including web, Git and API."
 msgstr ""
 
@@ -2248,6 +2263,9 @@ msgstr ""
 msgid "AdminUsers|When the user logs back in, their account will reactivate as a fully active account"
 msgstr ""
 
+msgid "AdminUsers|Will be deleted"
+msgstr ""
+
 msgid "AdminUsers|Without projects"
 msgstr ""
 
@@ -12773,6 +12791,9 @@ msgstr ""
 msgid "GitLab Workhorse"
 msgstr ""
 
+msgid "GitLab account request rejected"
+msgstr ""
+
 msgid "GitLab commit"
 msgstr ""
 
@@ -13811,6 +13832,9 @@ msgstr ""
 msgid "HealthCheck|Unhealthy"
 msgstr ""
 
+msgid "Hello %{name},"
+msgstr ""
+
 msgid "Hello there"
 msgstr ""
 
@@ -20320,6 +20344,9 @@ msgstr ""
 msgid "Please complete your profile with email address"
 msgstr ""
 
+msgid "Please contact your GitLab administrator if you think this is an error."
+msgstr ""
+
 msgid "Please contact your administrator with any questions."
 msgstr ""
 
@@ -28113,6 +28140,9 @@ msgstr ""
 msgid "This user cannot be unlocked manually from GitLab"
 msgstr ""
 
+msgid "This user does not have a pending request"
+msgstr ""
+
 msgid "This user has no active %{type}."
 msgstr ""
 
@@ -31048,6 +31078,9 @@ msgstr ""
 msgid "You are not allowed to push into this branch. Create another branch or open a merge request."
 msgstr ""
 
+msgid "You are not allowed to reject a user"
+msgstr ""
+
 msgid "You are not allowed to unlink your primary login account"
 msgstr ""
 
@@ -31516,6 +31549,9 @@ msgstr ""
 msgid "You've already enabled two-factor authentication using one time password authenticators. In order to register a different device, you must first disable two-factor authentication."
 msgstr ""
 
+msgid "You've rejected %{user}"
+msgstr ""
+
 msgid "YouTube"
 msgstr ""
 
@@ -31750,6 +31786,9 @@ msgstr ""
 msgid "Your request for access has been queued for review."
 msgstr ""
 
+msgid "Your request to join %{host} has been rejected."
+msgstr ""
+
 msgid "Your requirements are being imported. Once finished, you'll receive a confirmation email."
 msgstr ""
 

spec/controllers/admin/users_controller_spec.rb

@@ -102,6 +102,57 @@ RSpec.describe Admin::UsersController do
     end
   end
 
+  describe 'DELETE #reject' do
+    subject { put :reject, params: { id: user.username } }
+
+    context 'when rejecting a pending user' do
+      let(:user) { create(:user, :blocked_pending_approval) }
+
+      it 'hard deletes the user', :sidekiq_inline do
+        subject
+
+        expect(User.exists?(user.id)).to be_falsy
+      end
+
+      it 'displays the rejection message' do
+        subject
+
+        expect(response).to redirect_to(admin_users_path)
+        expect(flash[:notice]).to eq("You've rejected #{user.name}")
+      end
+
+      it 'sends the user a rejection email' do
+        expect_next_instance_of(NotificationService) do |notification|
+          allow(notification).to receive(:user_admin_rejection).with(user.name, user.notification_email)
+        end
+
+        subject
+      end
+    end
+
+    context 'when user is not pending' do
+      let(:user) { create(:user, state: 'active') }
+
+      it 'does not reject and delete the user' do
+        subject
+
+        expect(User.exists?(user.id)).to be_truthy
+      end
+
+      it 'displays the error' do
+        subject
+
+        expect(flash[:alert]).to eq('This user does not have a pending request')
+      end
+
+      it 'does not email the user' do
+        expect(NotificationService).not_to receive(:new)
+
+        subject
+      end
+    end
+  end
+
   describe 'PUT #approve' do
     let(:user) { create(:user, :blocked_pending_approval) }
 

spec/features/admin/users/user_spec.rb

@@ -37,9 +37,7 @@ RSpec.describe 'Admin::Users::User' do
         expect(page).to have_content(user.name)
         expect(page).to have_content('Pending approval')
         expect(page).to have_link('Approve user')
-        expect(page).to have_button('Block user')
-        expect(page).to have_button('Delete user')
-        expect(page).to have_button('Delete user and contributions')
+        expect(page).to have_link('Reject request')
       end
     end
 

spec/policies/global_policy_spec.rb

@@ -150,6 +150,24 @@ RSpec.describe GlobalPolicy do
     end
   end
 
+  describe 'rejecting users' do
+    context 'regular user' do
+      it { is_expected.not_to be_allowed(:reject_user) }
+    end
+
+    context 'admin' do
+      let(:current_user) { create(:admin) }
+
+      context 'when admin mode is enabled', :enable_admin_mode do
+        it { is_expected.to be_allowed(:reject_user) }
+      end
+
+      context 'when admin mode is disabled' do
+        it { is_expected.to be_disallowed(:reject_user) }
+      end
+    end
+  end
+
   describe 'using project statistics filters' do
     context 'regular user' do
       it { is_expected.not_to be_allowed(:use_project_statistics_filters) }

spec/services/notification_service_spec.rb

@@ -2326,6 +2326,20 @@ RSpec.describe NotificationService, :mailer do
     end
   end
 
+  describe '#user_admin_rejection', :deliver_mails_inline do
+    let_it_be(:user) { create(:user, :blocked_pending_approval) }
+
+    before do
+      reset_delivered_emails!
+    end
+
+    it 'sends the user a rejection email' do
+      notification.user_admin_rejection(user.name, user.email)
+
+      should_only_email(user)
+    end
+  end
+
   describe 'GroupMember', :deliver_mails_inline do
     let(:added_user) { create(:user) }
 

spec/services/users/reject_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::RejectService do
+  let_it_be(:current_user) { create(:admin) }
+  let(:user) { create(:user, :blocked_pending_approval) }
+
+  subject(:execute) { described_class.new(current_user).execute(user) }
+
+  describe '#execute' do
+    context 'failures' do
+      context 'when the executor user is not allowed to reject users' do
+        let(:current_user) { create(:user) }
+
+        it 'returns error result' do
+          expect(subject[:status]).to eq(:error)
+          expect(subject[:message]).to match(/You are not allowed to reject a user/)
+        end
+      end
+
+      context 'when the executor user is an admin in admin mode', :enable_admin_mode do
+        context 'when user is not in pending approval state' do
+          let(:user) { create(:user, state: 'active') }
+
+          it 'returns error result' do
+            expect(subject[:status]).to eq(:error)
+            expect(subject[:message])
+              .to match(/This user does not have a pending request/)
+          end
+        end
+      end
+    end
+
+    context 'success' do
+      context 'when the executor user is an admin in admin mode', :enable_admin_mode do
+        it 'deletes the user', :sidekiq_inline do
+          subject
+
+          expect(subject[:status]).to eq(:success)
+          expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
+        end
+
+        it 'emails the user on rejection' do
+          expect_next_instance_of(NotificationService) do |notification|
+            allow(notification).to receive(:user_admin_rejection).with(user.name, user.notification_email)
+          end
+
+          subject
+        end
+      end
+    end
+  end
+end