Merge branch '257879-user-admin-approval-admin-email-notification' into 'master'

User admin approval - Email admin the access request

See merge request gitlab-org/gitlab!46895

app/controllers/registrations_controller.rb

@@ -28,6 +28,11 @@ class RegistrationsController < Devise::RegistrationsController
     super do |new_user|
       persist_accepted_terms_if_required(new_user)
       set_role_required(new_user)
+
+      if pending_approval?
+        NotificationService.new.new_instance_access_request(new_user)
+      end
+
       yield new_user if block_given?
     end
 
@@ -131,6 +136,12 @@ class RegistrationsController < Devise::RegistrationsController
     render action: 'new'
   end
 
+  def pending_approval?
+    return false unless Gitlab::CurrentSettings.require_admin_approval_after_user_signup
+
+    resource.persisted? && resource.blocked_pending_approval?
+  end
+
   def sign_up_params
     params.require(:user).permit(:username, :email, :name, :first_name, :last_name, :password)
   end

app/helpers/emails_helper.rb

@@ -214,6 +214,24 @@ module EmailsHelper
     end
   end
 
+  def instance_access_request_text(user, format: nil)
+    gitlab_host = Gitlab.config.gitlab.host
+
+    _('%{username} has asked for a GitLab account on your instance %{host}:') % { username: sanitize_name(user.name), host: gitlab_host }
+  end
+
+  def instance_access_request_link(user, format: nil)
+    url = admin_user_url(user)
+
+    case format
+    when :html
+      user_page = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: url }
+      _("Click %{link_start}here%{link_end} to view the request.").html_safe % { link_start: user_page, link_end: '</a>'.html_safe }
+    else
+      _('Click %{link_to} to view the request.') % { link_to: url }
+    end
+  end
+
   def contact_your_administrator_text
     _('Please contact your administrator with any questions.')
   end

app/mailers/emails/profile.rb

@@ -9,6 +9,15 @@ module Emails
       mail(to: @user.notification_email, subject: subject("Account was created for you"))
     end
 
+    def instance_access_request_email(user, recipient)
+      @user = user
+      @recipient = recipient
+
+      profile_email_with_layout(
+        to: recipient.notification_email,
+        subject: subject(_("GitLab Account Request")))
+    end
+
     # rubocop: disable CodeReuse/ActiveRecord
     def new_ssh_key_email(key_id)
       @key = Key.find_by(id: key_id)
@@ -63,13 +72,9 @@ module Emails
       @target_url = edit_profile_password_url
 
       Gitlab::I18n.with_locale(@user.preferred_language) do
-        mail(
+        profile_email_with_layout(
           to: @user.notification_email,
-          subject: subject(_("%{host} sign-in from new location") % { host: Gitlab.config.gitlab.host })
-        ) do |format|
-          format.html { render layout: 'mailer' }
-          format.text { render layout: 'mailer' }
-        end
+          subject: subject(_("%{host} sign-in from new location") % { host: Gitlab.config.gitlab.host }))
       end
     end
 
@@ -82,6 +87,15 @@ module Emails
         mail(to: @user.notification_email, subject: subject(_("Two-factor authentication disabled")))
       end
     end
+
+    private
+
+    def profile_email_with_layout(to:, subject:, layout: 'mailer')
+      mail(to: to, subject: subject) do |format|
+        format.html { render layout: layout }
+        format.text { render layout: layout }
+      end
+    end
   end
 end
 

app/models/user.rb

@@ -28,6 +28,8 @@ class User < ApplicationRecord
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
+  INSTANCE_ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10
+
   add_authentication_token_field :incoming_email_token, token_generator: -> { SecureRandom.hex.to_i(16).to_s(36) }
   add_authentication_token_field :feed_token
   add_authentication_token_field :static_object_token
@@ -341,6 +343,7 @@ class User < ApplicationRecord
 
   # Scopes
   scope :admins, -> { where(admin: true) }
+  scope :instance_access_request_approvers_to_be_notified, -> { admins.active.order_recent_sign_in.limit(INSTANCE_ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT) }
   scope :blocked, -> { with_states(:blocked, :ldap_blocked) }
   scope :blocked_pending_approval, -> { with_states(:blocked_pending_approval) }
   scope :external, -> { where(external: true) }

app/services/notification_service.rb

@@ -370,6 +370,16 @@ class NotificationService
     end
   end
 
+  def new_instance_access_request(user)
+    recipients = User.instance_access_request_approvers_to_be_notified # https://gitlab.com/gitlab-org/gitlab/-/issues/277016 will change this
+
+    return true if recipients.empty?
+
+    recipients.each do |recipient|
+      mailer.instance_access_request_email(user, recipient).deliver_later
+    end
+  end
+
   # Members
   def new_access_request(member)
     return true unless member.notifiable?(:subscription)

app/views/notify/instance_access_request_email.html.haml

+#content
+  = email_default_heading(say_hello(@recipient))
+  %p
+    = instance_access_request_text(@user, format: :html)
+  %p
+    = _("Username: %{username}") % { username: @user.username }
+  %p
+    = _("Email: %{email}") % { email: @user.email }
+  %p
+    = instance_access_request_link(@user, format: :html)

app/views/notify/instance_access_request_email.text.erb

+<%= say_hello(@recipient) %>
+
+<%= instance_access_request_text(@user) %>
+
+<%= _("Username: %{username}") % { username: @user.username } %>
+<%= _("Email: %{email}") % { email: @user.email } %>
+
+<%= instance_access_request_link(@user) %>

changelogs/unreleased/257879-user-admin-approval-admin-email-notification.yml

+---
+title: Send email notifications to admins about users pending approval
+merge_request: 46895
+author:
+type: added

locale/gitlab.pot

@@ -879,6 +879,9 @@ msgstr ""
 msgid "%{user_name} profile page"
 msgstr ""
 
+msgid "%{username} has asked for a GitLab account on your instance %{host}:"
+msgstr ""
+
 msgid "%{username}'s avatar"
 msgstr ""
 
@@ -5551,6 +5554,12 @@ msgstr ""
 msgid "Clears weight."
 msgstr ""
 
+msgid "Click %{link_start}here%{link_end} to view the request."
+msgstr ""
+
+msgid "Click %{link_to} to view the request."
+msgstr ""
+
 msgid "Click the %{strong_open}Download%{strong_close} button and wait for downloading to complete."
 msgstr ""
 
@@ -9993,6 +10002,9 @@ msgstr ""
 msgid "Email updates (optional)"
 msgstr ""
 
+msgid "Email: %{email}"
+msgstr ""
+
 msgid "EmailError|It appears that the email is blank. Make sure your reply is at the top of the email, we can't process inline replies."
 msgstr ""
 
@@ -12580,6 +12592,9 @@ msgstr ""
 msgid "GitLab API"
 msgstr ""
 
+msgid "GitLab Account Request"
+msgstr ""
+
 msgid "GitLab Billing Team."
 msgstr ""
 
@@ -29564,6 +29579,9 @@ msgstr ""
 msgid "Username or email"
 msgstr ""
 
+msgid "Username: %{username}"
+msgstr ""
+
 msgid "Users"
 msgstr ""
 

spec/controllers/registrations_controller_spec.rb

@@ -53,6 +53,14 @@ RSpec.describe RegistrationsController do
             .to eq('You have signed up successfully. However, we could not sign you in because your account is awaiting approval from your GitLab administrator.')
         end
 
+        it 'emails the access request to approvers' do
+          expect_next_instance_of(NotificationService) do |notification|
+            allow(notification).to receive(:new_instance_access_request).with(User.find_by(email: 'new@user.com'))
+          end
+
+          subject
+        end
+
         context 'email confirmation' do
           context 'when `send_user_confirmation_email` is true' do
             before do
@@ -86,6 +94,12 @@ RSpec.describe RegistrationsController do
           expect(flash[:notice]).to be_nil
         end
 
+        it 'does not email the approvers' do
+          expect(NotificationService).not_to receive(:new)
+
+          subject
+        end
+
         context 'email confirmation' do
           context 'when `send_user_confirmation_email` is true' do
             before do

spec/factories/users.rb

@@ -66,7 +66,7 @@ FactoryBot.define do
 
     trait :with_sign_ins do
       sign_in_count { 3 }
-      current_sign_in_at { Time.now }
+      current_sign_in_at { FFaker::Time.between(10.days.ago, 1.day.ago) }
       last_sign_in_at { FFaker::Time.between(10.days.ago, 1.day.ago) }
       current_sign_in_ip { '127.0.0.1' }
       last_sign_in_ip { '127.0.0.1' }

spec/models/user_spec.rb

@@ -1740,6 +1740,16 @@ RSpec.describe User do
     end
   end
 
+  describe '.instance_access_request_approvers_to_be_notified' do
+    let_it_be(:admin_list) { create_list(:user, 12, :admin, :with_sign_ins) }
+
+    it 'returns up to the ten most recently active instance admins' do
+      active_admins_in_recent_sign_in_desc_order = User.admins.active.order_recent_sign_in.limit(10)
+
+      expect(User.instance_access_request_approvers_to_be_notified).to eq(active_admins_in_recent_sign_in_desc_order)
+    end
+  end
+
   describe '.filter_items' do
     let(:user) { double }
 

spec/services/notification_service_spec.rb

@@ -2305,6 +2305,26 @@ RSpec.describe NotificationService, :mailer do
     end
   end
 
+  describe '#new_instance_access_request', :deliver_mails_inline do
+    let_it_be(:user) { create(:user, :blocked_pending_approval) }
+    let_it_be(:admins) { create_list(:admin, 12, :with_sign_ins) }
+
+    subject { notification.new_instance_access_request(user) }
+
+    before do
+      reset_delivered_emails!
+      stub_application_setting(require_admin_approval_after_user_signup: true)
+    end
+
+    it 'sends notification only to a maximum of ten most recently active instance admins' do
+      ten_most_recently_active_instance_admins = User.admins.active.sort_by(&:current_sign_in_at).last(10)
+
+      subject
+
+      should_only_email(*ten_most_recently_active_instance_admins)
+    end
+  end
+
   describe 'GroupMember', :deliver_mails_inline do
     let(:added_user) { create(:user) }