Commit 7105937e authored by manojmj's avatar manojmj

Rate limit the 'Send emails from Admin Area' feature

This change rate limits the ‘Send emails from Admin area’
feature so that admins can trigger only 1 set of emails
every 10 minutes.
parent 2a9c6d04
...@@ -26,6 +26,9 @@ at their primary email address. ...@@ -26,6 +26,9 @@ at their primary email address.
![compose an email](email2.png) ![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 ## Unsubscribing from emails
Users can choose to unsubscribe from receiving emails from GitLab by following Users can choose to unsubscribe from receiving emails from GitLab by following
......
...@@ -4,17 +4,24 @@ class Admin::EmailsController < Admin::ApplicationController ...@@ -4,17 +4,24 @@ class Admin::EmailsController < Admin::ApplicationController
include Admin::EmailsHelper include Admin::EmailsHelper
before_action :check_license_send_emails_from_admin_area_available! before_action :check_license_send_emails_from_admin_area_available!
before_action :check_rate_limit!, only: [:create]
def show def show
end end
def create def create
AdminEmailsWorker.perform_async(params[:recipients], params[:subject], params[:body]) # rubocop:disable CodeReuse/Worker Admin::EmailService.new(params[:recipients], params[:subject], params[:body]).execute
redirect_to admin_email_path, notice: 'Email sent' redirect_to admin_email_path, notice: _('Email sent')
end end
private 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! def check_license_send_emails_from_admin_area_available!
render_404 unless send_emails_from_admin_area_feature_available? render_404 unless send_emails_from_admin_area_feature_available?
end end
......
...@@ -2,8 +2,30 @@ ...@@ -2,8 +2,30 @@
module Admin module Admin
module EmailsHelper module EmailsHelper
include Gitlab::Utils::StrongMemoize
def send_emails_from_admin_area_feature_available? def send_emails_from_admin_area_feature_available?
License.feature_available?(:send_emails_from_admin_area) License.feature_available?(:send_emails_from_admin_area)
end 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
end end
# 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
- 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" - page_title "Email Notification"
%h3.page-title %h3.page-title
Send email notification = _('Send email notification')
%p.light %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_tag admin_email_path, id: 'new-admin-email' do
.form-group.row .form-group.row.ml-1
%label.col-form-label{ for: :subject } Subject %label.col-form-label{ for: :subject } Subject
.col-sm-10 .col-sm-10
= text_field_tag :subject, '', class: 'form-control', required: true = text_field_tag :subject, '', class: 'form-control', required: true
.form-group.row .form-group.row.ml-1
%label.col-form-label{ for: :body } Body %label.col-form-label{ for: :body } Body
.col-sm-10 .col-sm-10
= text_area_tag :body, '', class: 'form-control', rows: 15, required: true = 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 %label.col-form-label{ for: :recipients } Recipient group
.col-sm-10 .col-sm-10
= admin_email_select_tag(:recipients) = admin_email_select_tag(:recipients)
.form-actions .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?
---
title: Rate limit the 'Send emails from Admin Area' feature
merge_request: 31308
author:
type: added
...@@ -2,7 +2,9 @@ ...@@ -2,7 +2,9 @@
require 'spec_helper' 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(:admin) { create(:admin) }
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
...@@ -53,11 +55,15 @@ describe Admin::EmailsController do ...@@ -53,11 +55,15 @@ describe Admin::EmailsController do
end end
describe 'POST #create' do describe 'POST #create' do
let(:recipients) { 'all' }
let(:email_subject) { 'subject' }
let(:body) { 'body' }
subject do subject do
post :create, params: { post :create, params: {
recipients: 'all', recipients: recipients,
subject: 'subject', subject: email_subject,
body: 'body' body: body
} }
end end
...@@ -71,18 +77,45 @@ describe Admin::EmailsController do ...@@ -71,18 +77,45 @@ describe Admin::EmailsController do
stub_licensed_features(send_emails_from_admin_area: true) stub_licensed_features(send_emails_from_admin_area: true)
end end
it 'trigger the background job to send emails' do context 'when emails from admin area are not rate limited' do
expect(AdminEmailsWorker).to receive(:perform_async).with('all', 'subject', 'body') 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 end
it 'redirects to `admin_email_path`' do context 'when emails from admin area are rate limited' do
subject 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 have_gitlab_http_status(:found)
expect(response).to redirect_to(admin_email_path) expect(response).to redirect_to(admin_email_path)
expect(flash[:notice]).to eq('Email sent') expect(flash[:alert]).to eq('Email could not be sent')
end
end end
end end
...@@ -91,8 +124,8 @@ describe Admin::EmailsController do ...@@ -91,8 +124,8 @@ describe Admin::EmailsController do
stub_licensed_features(send_emails_from_admin_area: false) stub_licensed_features(send_emails_from_admin_area: false)
end end
it 'does not trigger the background job to send emails' do it 'does not trigger the service to send emails' do
expect(AdminEmailsWorker).not_to receive(:perform_async) expect(Admin::EmailService).not_to receive(:new)
subject subject
end end
......
# 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
...@@ -2,7 +2,12 @@ ...@@ -2,7 +2,12 @@
require 'spec_helper' 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 describe '#send_emails_from_admin_area_feature_available?' do
subject { helper.send_emails_from_admin_area_feature_available? } subject { helper.send_emails_from_admin_area_feature_available? }
...@@ -22,4 +27,59 @@ describe Admin::EmailsHelper do ...@@ -22,4 +27,59 @@ describe Admin::EmailsHelper do
it { is_expected.to be_falsey } it { is_expected.to be_falsey }
end end
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 end
# 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
...@@ -1961,6 +1961,9 @@ msgstr "" ...@@ -1961,6 +1961,9 @@ msgstr ""
msgid "An application called %{link_to_client} is requesting access to your GitLab account." msgid "An application called %{link_to_client} is requesting access to your GitLab account."
msgstr "" 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." 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 "" msgstr ""
...@@ -7757,6 +7760,9 @@ msgstr "" ...@@ -7757,6 +7760,9 @@ msgstr ""
msgid "Email address" msgid "Email address"
msgstr "" msgstr ""
msgid "Email could not be sent"
msgstr ""
msgid "Email display name" msgid "Email display name"
msgstr "" msgstr ""
...@@ -7772,6 +7778,9 @@ msgstr "" ...@@ -7772,6 +7778,9 @@ msgstr ""
msgid "Email restrictions for sign-ups" msgid "Email restrictions for sign-ups"
msgstr "" msgstr ""
msgid "Email sent"
msgstr ""
msgid "Email the pipelines status to a list of recipients." msgid "Email the pipelines status to a list of recipients."
msgstr "" msgstr ""
...@@ -18807,6 +18816,12 @@ msgstr "" ...@@ -18807,6 +18816,12 @@ msgstr ""
msgid "Send email" msgid "Send email"
msgstr "" msgstr ""
msgid "Send email notification"
msgstr ""
msgid "Send message"
msgstr ""
msgid "Send report" msgid "Send report"
msgstr "" msgstr ""
...@@ -24207,6 +24222,9 @@ msgstr "" ...@@ -24207,6 +24222,9 @@ msgstr ""
msgid "You can move around the graph by using the arrow keys." msgid "You can move around the graph by using the arrow keys."
msgstr "" 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." msgid "You can now export your security dashboard to a CSV report."
msgstr "" msgstr ""
......
...@@ -10,7 +10,8 @@ module ExclusiveLeaseHelpers ...@@ -10,7 +10,8 @@ module ExclusiveLeaseHelpers
try_obtain: uuid, try_obtain: uuid,
exists?: true, exists?: true,
renew: renew, renew: renew,
cancel: nil cancel: nil,
ttl: timeout
) )
allow(Gitlab::ExclusiveLease) allow(Gitlab::ExclusiveLease)
......
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