Merge branch '31509-rate-limit-email-blast-from-admin-area' into 'master'

Resolve "Rate limit email blast from Admin area"

See merge request gitlab-org/gitlab!31308

doc/tools/email.md

@@ -26,6 +26,9 @@ at their primary email address.
 
    ![compose an email](email2.png)
 
+NOTE: **Note:**
+[Starting with GitLab 13.0](https://gitlab.com/gitlab-org/gitlab/-/issues/31509), email notifications can be sent only once every 10 minutes. This helps minimize performance issues.
+
 ## Unsubscribing from emails
 
 Users can choose to unsubscribe from receiving emails from GitLab by following

ee/app/controllers/admin/emails_controller.rb

@@ -4,17 +4,24 @@ class Admin::EmailsController < Admin::ApplicationController
   include Admin::EmailsHelper
 
   before_action :check_license_send_emails_from_admin_area_available!
+  before_action :check_rate_limit!, only: [:create]
 
   def show
   end
 
   def create
-    AdminEmailsWorker.perform_async(params[:recipients], params[:subject], params[:body]) # rubocop:disable CodeReuse/Worker
-    redirect_to admin_email_path, notice: 'Email sent'
+    Admin::EmailService.new(params[:recipients], params[:subject], params[:body]).execute
+    redirect_to admin_email_path, notice: _('Email sent')
   end
 
   private
 
+  def check_rate_limit!
+    if admin_emails_are_currently_rate_limited?
+      redirect_to admin_email_path, alert: _('Email could not be sent')
+    end
+  end
+
   def check_license_send_emails_from_admin_area_available!
     render_404 unless send_emails_from_admin_area_feature_available?
   end

ee/app/helpers/admin/emails_helper.rb

@@ -2,8 +2,30 @@
 
 module Admin
   module EmailsHelper
+    include Gitlab::Utils::StrongMemoize
+
     def send_emails_from_admin_area_feature_available?
       License.feature_available?(:send_emails_from_admin_area)
     end
+
+    def admin_emails_are_currently_rate_limited?
+      admin_emails_rate_limit_ttl.present?
+    end
+
+    def admin_emails_rate_limit_ttl
+      strong_memoize(:admin_emails_rate_limit_ttl) do
+        Gitlab::ExclusiveLease.new(
+          Admin::EmailService::LEASE_KEY,
+          timeout: Admin::EmailService::DEFAULT_LEASE_TIMEOUT
+        ).ttl
+      end
+    end
+
+    def admin_emails_rate_limited_alert
+      return '' unless admin_emails_are_currently_rate_limited?
+
+      _("An email notification was recently sent from the admin panel. Please wait %{wait_time_in_words} before attempting to send another message.") %
+        { wait_time_in_words: distance_of_time_in_words(admin_emails_rate_limit_ttl) }
+    end
   end
 end

ee/app/services/admin/email_service.rb

+# frozen_string_literal: true
+
+module Admin
+  class EmailService
+    include ExclusiveLeaseGuard
+
+    DEFAULT_LEASE_TIMEOUT = 10.minutes.to_i
+    LEASE_KEY = 'admin/email_service'.freeze
+
+    def initialize(recipients, subject, body)
+      @recipients, @subject, @body = recipients, subject, body
+    end
+
+    def execute
+      try_obtain_lease do
+        AdminEmailsWorker.perform_async(recipients, subject, body)
+      end
+    end
+
+    private
+
+    attr_reader :recipients, :subject, :body
+
+    def lease_key
+      LEASE_KEY
+    end
+
+    def lease_timeout
+      DEFAULT_LEASE_TIMEOUT
+    end
+
+    def lease_release?
+      false
+    end
+  end
+end

ee/app/views/admin/emails/show.html.haml

+- if admin_emails_are_currently_rate_limited?
+  %div{ class: 'gl-alert gl-alert-danger', role: 'alert' }
+    = sprite_icon('warning', size: 16, css_class: 'gl-icon s16 gl-alert-icon gl-alert-icon-no-title')
+    .gl-alert-body
+      = admin_emails_rate_limited_alert
+
 - page_title "Email Notification"
 %h3.page-title
-  Send email notification
+  = _('Send email notification')
 %p.light
-  You can notify the app / group or a project by sending them an email notification
+  = _('You can notify the app / group or a project by sending them an email notification')
 
 = form_tag admin_email_path, id: 'new-admin-email' do
-  .form-group.row
+  .form-group.row.ml-1
     %label.col-form-label{ for: :subject } Subject
     .col-sm-10
       = text_field_tag :subject, '', class: 'form-control', required: true
 
-  .form-group.row
+  .form-group.row.ml-1
     %label.col-form-label{ for: :body } Body
     .col-sm-10
       = text_area_tag :body, '', class: 'form-control', rows: 15, required: true
 
-  .form-group.row
+  .form-group.row.ml-1
     %label.col-form-label{ for: :recipients } Recipient group
     .col-sm-10
       = admin_email_select_tag(:recipients)
   .form-actions
-    = submit_tag 'Send message', class: 'btn btn-success'
+    = submit_tag _('Send message'), class: 'btn btn-success', disabled: admin_emails_are_currently_rate_limited?

ee/changelogs/unreleased/31509-rate-limit-email-blast-from-admin-area.yml

+---
+title: Rate limit the 'Send emails from Admin Area' feature
+merge_request: 31308
+author:
+type: added

ee/spec/controllers/admin/emails_controller_spec.rb

@@ -2,7 +2,9 @@
 
 require 'spec_helper'
 
-describe Admin::EmailsController do
+describe Admin::EmailsController, :clean_gitlab_redis_shared_state do
+  include ExclusiveLeaseHelpers
+
   let_it_be(:admin) { create(:admin) }
   let_it_be(:user) { create(:user) }
 
@@ -53,11 +55,15 @@ describe Admin::EmailsController do
   end
 
   describe 'POST #create' do
+    let(:recipients) { 'all' }
+    let(:email_subject) { 'subject' }
+    let(:body) { 'body' }
+
     subject do
       post :create, params: {
-        recipients: 'all',
-        subject: 'subject',
-        body: 'body'
+        recipients: recipients,
+        subject: email_subject,
+        body: body
       }
     end
 
@@ -71,18 +77,45 @@ describe Admin::EmailsController do
           stub_licensed_features(send_emails_from_admin_area: true)
         end
 
-        it 'trigger the background job to send emails' do
-          expect(AdminEmailsWorker).to receive(:perform_async).with('all', 'subject', 'body')
+        context 'when emails from admin area are not rate limited' do
+          it 'triggers the service to send emails' do
+            expect_next_instance_of(Admin::EmailService, recipients, email_subject, body) do |email_service|
+              expect(email_service).to receive(:execute)
+            end
 
-          subject
+            subject
+          end
+
+          it 'redirects to `admin_email_path` with success notice' do
+            subject
+
+            expect(response).to have_gitlab_http_status(:found)
+            expect(response).to redirect_to(admin_email_path)
+            expect(flash[:notice]).to eq('Email sent')
+          end
         end
 
-        it 'redirects to `admin_email_path`' do
-          subject
+        context 'when emails from admin area are rate limited' do
+          let(:lease_key) { Admin::EmailService::LEASE_KEY }
+          let(:timeout) { Admin::EmailService::DEFAULT_LEASE_TIMEOUT }
+
+          before do
+            stub_exclusive_lease(lease_key, timeout: timeout)
+          end
+
+          it 'does not trigger the service to send emails' do
+            expect(Admin::EmailService).not_to receive(:new)
+
+            subject
+          end
+
+          it 'redirects to `admin_email_path`' do
+            subject
 
-          expect(response).to have_gitlab_http_status(:found)
-          expect(response).to redirect_to(admin_email_path)
-          expect(flash[:notice]).to eq('Email sent')
+            expect(response).to have_gitlab_http_status(:found)
+            expect(response).to redirect_to(admin_email_path)
+            expect(flash[:alert]).to eq('Email could not be sent')
+          end
         end
       end
 
@@ -91,8 +124,8 @@ describe Admin::EmailsController do
           stub_licensed_features(send_emails_from_admin_area: false)
         end
 
-        it 'does not trigger the background job to send emails' do
-          expect(AdminEmailsWorker).not_to receive(:perform_async)
+        it 'does not trigger the service to send emails' do
+          expect(Admin::EmailService).not_to receive(:new)
 
           subject
         end

ee/spec/features/admin/admin_emails_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Admin::Emails', :clean_gitlab_redis_shared_state do
+  include ExclusiveLeaseHelpers
+
+  before do
+    sign_in(create(:admin))
+  end
+
+  context 'when `send_emails_from_admin_area` feature is not licensed' do
+    before do
+      stub_licensed_features(send_emails_from_admin_area: false)
+    end
+
+    it 'returns 404' do
+      visit admin_email_path
+
+      expect(page.status_code).to eq(404)
+    end
+  end
+
+  context 'when `send_emails_from_admin_area` feature is licensed' do
+    let(:rate_limited_alert) do
+      'An email notification was recently sent from the admin panel. '\
+      'Please wait 10 minutes before attempting to send another message.'
+    end
+    let(:submit_button) { find('input[type="submit"]') }
+
+    before do
+      stub_licensed_features(send_emails_from_admin_area: true)
+    end
+
+    context 'when emails from admin area are not rate limited' do
+      it 'does not show the waiting period alert'\
+        'and the submit button is in enabled state' do
+        visit admin_email_path
+
+        expect(page).not_to have_content(rate_limited_alert)
+        expect(submit_button.disabled?).to eq(false)
+      end
+    end
+
+    context 'when emails from admin area are rate limited' do
+      let(:lease_key) { Admin::EmailService::LEASE_KEY }
+      let(:timeout) { Admin::EmailService::DEFAULT_LEASE_TIMEOUT }
+
+      before do
+        allow(Gitlab::ExclusiveLease).to receive(:new).and_call_original
+        stub_exclusive_lease(lease_key, timeout: timeout)
+      end
+
+      it 'shows the waiting period alert'\
+        'and the submit button is in disabled state' do
+        visit admin_email_path
+
+        expect(page).to have_content(rate_limited_alert)
+        expect(submit_button.disabled?).to eq(true)
+      end
+    end
+  end
+end

ee/spec/helpers/admin/emails_helper_spec.rb

@@ -2,7 +2,12 @@
 
 require 'spec_helper'
 
-describe Admin::EmailsHelper do
+describe Admin::EmailsHelper, :clean_gitlab_redis_shared_state do
+  include ExclusiveLeaseHelpers
+
+  let(:lease_key) { Admin::EmailService::LEASE_KEY }
+  let(:timeout) { Admin::EmailService::DEFAULT_LEASE_TIMEOUT }
+
   describe '#send_emails_from_admin_area_feature_available?' do
     subject { helper.send_emails_from_admin_area_feature_available? }
 
@@ -22,4 +27,59 @@ describe Admin::EmailsHelper do
       it { is_expected.to be_falsey }
     end
   end
+
+  describe '#admin_emails_are_currently_rate_limited?' do
+    subject { helper.admin_emails_are_currently_rate_limited? }
+
+    context 'when the lease key exists' do
+      it 'returns true' do
+        stub_exclusive_lease(lease_key, timeout: timeout)
+
+        expect(subject).to eq(true)
+      end
+    end
+
+    context 'when the lease key does not exist' do
+      it 'returns false' do
+        expect(subject).to eq(false)
+      end
+    end
+  end
+
+  describe '#admin_emails_rate_limit_ttl' do
+    subject { helper.admin_emails_rate_limit_ttl }
+
+    context 'when the lease key exists' do
+      it 'returns the time remaining till the key expires' do
+        stub_exclusive_lease(lease_key, timeout: timeout)
+
+        expect(subject).to eq(timeout)
+      end
+    end
+
+    context 'when the lease key does not exist' do
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+  end
+
+  describe '#admin_emails_rate_limited_alert' do
+    subject { helper.admin_emails_rate_limited_alert }
+
+    context 'when the lease key exists' do
+      it 'returns the alert' do
+        stub_exclusive_lease(lease_key, timeout: timeout)
+
+        expect(subject).to \
+          eq('An email notification was recently sent from the admin panel. Please wait 10 minutes before attempting to send another message.')
+      end
+    end
+
+    context 'when the lease key does not exist' do
+      it 'returns empty string' do
+        expect(subject).to eq('')
+      end
+    end
+  end
 end

ee/spec/services/admin/email_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Admin::EmailService do
+  include ExclusiveLeaseHelpers
+
+  describe '#execute', :clean_gitlab_redis_shared_state do
+    let(:args) { %w(all email_subject email_body) }
+    let(:lease_key) { 'admin/email_service' }
+
+    subject { described_class.new(*args) }
+
+    context 'when we can obtain the lease' do
+      it 'schedules the worker' do
+        stub_exclusive_lease(lease_key, timeout: described_class::DEFAULT_LEASE_TIMEOUT)
+
+        expect(AdminEmailsWorker).to receive(:perform_async).with(*args).once
+
+        subject.execute
+      end
+    end
+
+    context "when we can't obtain the lease" do
+      it 'does not schedule the worker' do
+        stub_exclusive_lease_taken(lease_key, timeout: described_class::DEFAULT_LEASE_TIMEOUT)
+
+        expect(AdminEmailsWorker).not_to receive(:perform_async)
+
+        subject.execute
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -1975,6 +1975,9 @@ msgstr ""
 msgid "An application called %{link_to_client} is requesting access to your GitLab account."
 msgstr ""
 
+msgid "An email notification was recently sent from the admin panel. Please wait %{wait_time_in_words} before attempting to send another message."
+msgstr ""
+
 msgid "An empty GitLab User field will add the FogBugz user's full name (e.g. \"By John Smith\") in the description of all issues and comments. It will also associate and/or assign these issues and comments with the project creator."
 msgstr ""
 
@@ -7780,6 +7783,9 @@ msgstr ""
 msgid "Email address"
 msgstr ""
 
+msgid "Email could not be sent"
+msgstr ""
+
 msgid "Email display name"
 msgstr ""
 
@@ -7795,6 +7801,9 @@ msgstr ""
 msgid "Email restrictions for sign-ups"
 msgstr ""
 
+msgid "Email sent"
+msgstr ""
+
 msgid "Email the pipelines status to a list of recipients."
 msgstr ""
 
@@ -18857,6 +18866,12 @@ msgstr ""
 msgid "Send email"
 msgstr ""
 
+msgid "Send email notification"
+msgstr ""
+
+msgid "Send message"
+msgstr ""
+
 msgid "Send report"
 msgstr ""
 
@@ -24251,6 +24266,9 @@ msgstr ""
 msgid "You can move around the graph by using the arrow keys."
 msgstr ""
 
+msgid "You can notify the app / group or a project by sending them an email notification"
+msgstr ""
+
 msgid "You can now export your security dashboard to a CSV report."
 msgstr ""
 

spec/support/helpers/exclusive_lease_helpers.rb

@@ -10,7 +10,8 @@ module ExclusiveLeaseHelpers
       try_obtain: uuid,
       exists?: true,
       renew: renew,
-      cancel: nil
+      cancel: nil,
+      ttl: timeout
     )
 
     allow(Gitlab::ExclusiveLease)