Commit dbeb0733 authored by sfang97's avatar sfang97

Email admin when user requests access

When admin approval required on sign up, send email to admin with user
info when user requests an account

Add link to pending approval

Change url to host

Remove extra new line

Run get text regenerate

Add changelog entry

Add negative spec case

Add newline at end of file

Add params to Devise mailer

Devise mailer is weird and you can only pass it one param, so do the
admin loop in the devise method

Add spec to devise mailer spec

Use NotificationService instead of DeviseMailer

NotificationService lets you pass multiple params to the email view and
it's much easier to send emails to the correct address with the correct
information

Remove DeviseMailer changes

There were changes to Devise Mailer spec and preview, so removing those
in favor of NotificationService changes

Change email helper func names

Add shared example for 10 admins

Remove extra new line

Fast return if no recipients

Change email to notification email

Committing for save

Move instance access request mailer to profiles

Mailer method made more sense in profiles than in members file

Add rubocop disable

Change click here copy and link

Change spec names

Change spec name for clarity

Remove extra space

MR review comments

Address review comments

Run gettext regenerate

Render different email formats

Redundant return, formatting error

Cleanup return conditions

Don't need to pass resource param

Scope which admins to email

Private profile email layout method

Remove extra new line

Address MR review comments

Localize strings, early return, change variable name

Don't need shared example

Address MR review comments

Remove extra new lines

Address static analysis errors

Change sign in factory

Remove early return
parent 7472a375
...@@ -28,6 +28,11 @@ class RegistrationsController < Devise::RegistrationsController ...@@ -28,6 +28,11 @@ class RegistrationsController < Devise::RegistrationsController
super do |new_user| super do |new_user|
persist_accepted_terms_if_required(new_user) persist_accepted_terms_if_required(new_user)
set_role_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? yield new_user if block_given?
end end
...@@ -131,6 +136,12 @@ class RegistrationsController < Devise::RegistrationsController ...@@ -131,6 +136,12 @@ class RegistrationsController < Devise::RegistrationsController
render action: 'new' render action: 'new'
end 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 def sign_up_params
params.require(:user).permit(:username, :email, :name, :first_name, :last_name, :password) params.require(:user).permit(:username, :email, :name, :first_name, :last_name, :password)
end end
......
...@@ -214,6 +214,24 @@ module EmailsHelper ...@@ -214,6 +214,24 @@ module EmailsHelper
end end
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 def contact_your_administrator_text
_('Please contact your administrator with any questions.') _('Please contact your administrator with any questions.')
end end
......
...@@ -9,6 +9,15 @@ module Emails ...@@ -9,6 +9,15 @@ module Emails
mail(to: @user.notification_email, subject: subject("Account was created for you")) mail(to: @user.notification_email, subject: subject("Account was created for you"))
end 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 # rubocop: disable CodeReuse/ActiveRecord
def new_ssh_key_email(key_id) def new_ssh_key_email(key_id)
@key = Key.find_by(id: key_id) @key = Key.find_by(id: key_id)
...@@ -63,13 +72,9 @@ module Emails ...@@ -63,13 +72,9 @@ module Emails
@target_url = edit_profile_password_url @target_url = edit_profile_password_url
Gitlab::I18n.with_locale(@user.preferred_language) do Gitlab::I18n.with_locale(@user.preferred_language) do
mail( profile_email_with_layout(
to: @user.notification_email, to: @user.notification_email,
subject: subject(_("%{host} sign-in from new location") % { host: Gitlab.config.gitlab.host }) 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
end end
end end
...@@ -82,6 +87,15 @@ module Emails ...@@ -82,6 +87,15 @@ module Emails
mail(to: @user.notification_email, subject: subject(_("Two-factor authentication disabled"))) mail(to: @user.notification_email, subject: subject(_("Two-factor authentication disabled")))
end end
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
end end
......
...@@ -28,6 +28,8 @@ class User < ApplicationRecord ...@@ -28,6 +28,8 @@ class User < ApplicationRecord
DEFAULT_NOTIFICATION_LEVEL = :participating 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 :incoming_email_token, token_generator: -> { SecureRandom.hex.to_i(16).to_s(36) }
add_authentication_token_field :feed_token add_authentication_token_field :feed_token
add_authentication_token_field :static_object_token add_authentication_token_field :static_object_token
...@@ -341,6 +343,7 @@ class User < ApplicationRecord ...@@ -341,6 +343,7 @@ class User < ApplicationRecord
# Scopes # Scopes
scope :admins, -> { where(admin: true) } 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, -> { with_states(:blocked, :ldap_blocked) }
scope :blocked_pending_approval, -> { with_states(:blocked_pending_approval) } scope :blocked_pending_approval, -> { with_states(:blocked_pending_approval) }
scope :external, -> { where(external: true) } scope :external, -> { where(external: true) }
......
...@@ -370,6 +370,16 @@ class NotificationService ...@@ -370,6 +370,16 @@ class NotificationService
end end
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 # Members
def new_access_request(member) def new_access_request(member)
return true unless member.notifiable?(:subscription) return true unless member.notifiable?(:subscription)
......
#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)
<%= say_hello(@recipient) %>
<%= instance_access_request_text(@user) %>
<%= _("Username: %{username}") % { username: @user.username } %>
<%= _("Email: %{email}") % { email: @user.email } %>
<%= instance_access_request_link(@user) %>
---
title: Send email notifications to admins about users pending approval
merge_request: 46895
author:
type: added
...@@ -879,6 +879,9 @@ msgstr "" ...@@ -879,6 +879,9 @@ msgstr ""
msgid "%{user_name} profile page" msgid "%{user_name} profile page"
msgstr "" msgstr ""
msgid "%{username} has asked for a GitLab account on your instance %{host}:"
msgstr ""
msgid "%{username}'s avatar" msgid "%{username}'s avatar"
msgstr "" msgstr ""
...@@ -5530,6 +5533,12 @@ msgstr "" ...@@ -5530,6 +5533,12 @@ msgstr ""
msgid "Clears weight." msgid "Clears weight."
msgstr "" 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." msgid "Click the %{strong_open}Download%{strong_close} button and wait for downloading to complete."
msgstr "" msgstr ""
...@@ -9972,6 +9981,9 @@ msgstr "" ...@@ -9972,6 +9981,9 @@ msgstr ""
msgid "Email updates (optional)" msgid "Email updates (optional)"
msgstr "" 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." 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 "" msgstr ""
...@@ -12554,6 +12566,9 @@ msgstr "" ...@@ -12554,6 +12566,9 @@ msgstr ""
msgid "GitLab API" msgid "GitLab API"
msgstr "" msgstr ""
msgid "GitLab Account Request"
msgstr ""
msgid "GitLab Billing Team." msgid "GitLab Billing Team."
msgstr "" msgstr ""
...@@ -29532,6 +29547,9 @@ msgstr "" ...@@ -29532,6 +29547,9 @@ msgstr ""
msgid "Username or email" msgid "Username or email"
msgstr "" msgstr ""
msgid "Username: %{username}"
msgstr ""
msgid "Users" msgid "Users"
msgstr "" msgstr ""
......
...@@ -53,6 +53,14 @@ RSpec.describe RegistrationsController do ...@@ -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.') .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 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 'email confirmation' do
context 'when `send_user_confirmation_email` is true' do context 'when `send_user_confirmation_email` is true' do
before do before do
...@@ -86,6 +94,12 @@ RSpec.describe RegistrationsController do ...@@ -86,6 +94,12 @@ RSpec.describe RegistrationsController do
expect(flash[:notice]).to be_nil expect(flash[:notice]).to be_nil
end end
it 'does not email the approvers' do
expect(NotificationService).not_to receive(:new)
subject
end
context 'email confirmation' do context 'email confirmation' do
context 'when `send_user_confirmation_email` is true' do context 'when `send_user_confirmation_email` is true' do
before do before do
......
...@@ -66,7 +66,7 @@ FactoryBot.define do ...@@ -66,7 +66,7 @@ FactoryBot.define do
trait :with_sign_ins do trait :with_sign_ins do
sign_in_count { 3 } 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) } last_sign_in_at { FFaker::Time.between(10.days.ago, 1.day.ago) }
current_sign_in_ip { '127.0.0.1' } current_sign_in_ip { '127.0.0.1' }
last_sign_in_ip { '127.0.0.1' } last_sign_in_ip { '127.0.0.1' }
......
...@@ -1740,6 +1740,16 @@ RSpec.describe User do ...@@ -1740,6 +1740,16 @@ RSpec.describe User do
end end
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 describe '.filter_items' do
let(:user) { double } let(:user) { double }
......
...@@ -2322,6 +2322,26 @@ RSpec.describe NotificationService, :mailer do ...@@ -2322,6 +2322,26 @@ RSpec.describe NotificationService, :mailer do
end end
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 describe 'GroupMember', :deliver_mails_inline do
let(:added_user) { create(:user) } let(:added_user) { create(:user) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment