From a283ca464b8291e4ba3bad55020f21fb007d92ec Mon Sep 17 00:00:00 2001
From: Aishwarya Subramanian <asubramanian@gitlab.com>
Date: Wed, 7 Apr 2021 23:14:48 +0000
Subject: [PATCH] SSH key expiring soon email notification
https://gitlab.com/gitlab-org/gitlab/-/issues/322637
---
app/mailers/emails/profile.rb | 14 ++-
app/models/key.rb | 1 +
app/models/user.rb | 9 ++
.../keys/expiry_notification_service.rb | 25 ++++-
app/services/notification_service.rb | 7 ++
.../notify/ssh_key_expiring_soon.text.erb | 9 ++
.../ssh_key_expiring_soon_email.html.haml | 13 +++
app/workers/all_queues.yml | 8 ++
.../ssh_keys/expired_notification_worker.rb | 2 +-
.../expiring_soon_notification_worker.rb | 25 +++++
.../expiring-ssh-key-notification.yml | 5 +
config/initializers/1_settings.rb | 3 +
...e_expiry_notification_delivered_to_keys.rb | 9 ++
..._before_expiry_notification_undelivered.rb | 19 ++++
db/schema_migrations/20210401175134 | 1 +
db/schema_migrations/20210401192808 | 1 +
db/structure.sql | 5 +-
doc/ssh/README.md | 3 +-
locale/gitlab.pot | 6 ++
spec/mailers/emails/profile_spec.rb | 99 ++++++++++++++-----
spec/models/key_spec.rb | 18 +++-
spec/models/user_spec.rb | 19 +++-
.../keys/expiry_notification_service_spec.rb | 83 ++++++++++++----
spec/services/notification_service_spec.rb | 34 +++++--
.../expiring_soon_notification_worker_spec.rb | 66 +++++++++++++
25 files changed, 419 insertions(+), 65 deletions(-)
create mode 100644 app/views/notify/ssh_key_expiring_soon.text.erb
create mode 100644 app/views/notify/ssh_key_expiring_soon_email.html.haml
create mode 100644 app/workers/ssh_keys/expiring_soon_notification_worker.rb
create mode 100644 changelogs/unreleased/expiring-ssh-key-notification.yml
create mode 100644 db/migrate/20210401175134_add_before_expiry_notification_delivered_to_keys.rb
create mode 100644 db/migrate/20210401192808_add_index_to_keys_on_expires_at_and_before_expiry_notification_undelivered.rb
create mode 100644 db/schema_migrations/20210401175134
create mode 100644 db/schema_migrations/20210401192808
create mode 100644 spec/workers/ssh_keys/expiring_soon_notification_worker_spec.rb
diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb
index 0734f1786400a..f967323f849cd 100644
--- a/app/mailers/emails/profile.rb
+++ b/app/mailers/emails/profile.rb
@@ -75,7 +75,7 @@ def access_token_expired_email(user)
end
def ssh_key_expired_email(user, fingerprints)
- return unless user && user.active?
+ return unless user&.active?
@user = user
@fingerprints = fingerprints
@@ -86,6 +86,18 @@ def ssh_key_expired_email(user, fingerprints)
end
end
+ def ssh_key_expiring_soon_email(user, fingerprints)
+ return unless user&.active?
+
+ @user = user
+ @fingerprints = fingerprints
+ @target_url = profile_keys_url
+
+ Gitlab::I18n.with_locale(@user.preferred_language) do
+ mail(to: @user.notification_email, subject: subject(_("Your SSH key is expiring soon.")))
+ end
+ end
+
def unknown_sign_in_email(user, ip, time)
@user = user
@ip = ip
diff --git a/app/models/key.rb b/app/models/key.rb
index d9da700376aef..131416d1bee38 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -44,6 +44,7 @@ class Key < ApplicationRecord
scope :for_user, -> (user) { where(user: user) }
scope :order_last_used_at_desc, -> { reorder(::Gitlab::Database.nulls_last_order('last_used_at', 'DESC')) }
scope :expired_today_and_not_notified, -> { where(["date(expires_at AT TIME ZONE 'UTC') = CURRENT_DATE AND expiry_notification_delivered_at IS NULL"]) }
+ scope :expiring_soon_and_not_notified, -> { where(["date(expires_at AT TIME ZONE 'UTC') > CURRENT_DATE AND date(expires_at AT TIME ZONE 'UTC') < ? AND before_expiry_notification_delivered_at IS NULL", DAYS_TO_EXPIRE.days.from_now.to_date]) }
def self.regular_keys
where(type: ['Key', nil])
diff --git a/app/models/user.rb b/app/models/user.rb
index de7bb1ccf49b6..a0b23429f9fb0 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -104,6 +104,7 @@ def update_tracked_fields!(request)
# Profile
has_many :keys, -> { regular_keys }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_many :expired_today_and_unnotified_keys, -> { expired_today_and_not_notified }, class_name: 'Key'
+ has_many :expiring_soon_and_unnotified_keys, -> { expiring_soon_and_not_notified }, class_name: 'Key'
has_many :deploy_keys, -> { where(type: 'DeployKey') }, dependent: :nullify # rubocop:disable Cop/ActiveRecordDependent
has_many :group_deploy_keys
has_many :gpg_keys
@@ -402,6 +403,14 @@ def blocked?
.where('keys.user_id = users.id')
.expired_today_and_not_notified)
end
+ scope :with_ssh_key_expiring_soon, -> do
+ includes(:expiring_soon_and_unnotified_keys)
+ .where('EXISTS (?)',
+ ::Key
+ .select(1)
+ .where('keys.user_id = users.id')
+ .expiring_soon_and_not_notified)
+ end
scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) }
scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) }
scope :order_recent_last_activity, -> { reorder(Gitlab::Database.nulls_last_order('last_activity_on', 'DESC')) }
diff --git a/app/services/keys/expiry_notification_service.rb b/app/services/keys/expiry_notification_service.rb
index 4ba896477d290..b486f77ced233 100644
--- a/app/services/keys/expiry_notification_service.rb
+++ b/app/services/keys/expiry_notification_service.rb
@@ -2,17 +2,38 @@
module Keys
class ExpiryNotificationService < ::Keys::BaseService
- attr_accessor :keys
+ attr_accessor :keys, :expiring_soon
def initialize(user, params)
@keys = params[:keys]
+ @expiring_soon = params[:expiring_soon]
super
end
def execute
- return unless user.can?(:receive_notifications)
+ return unless allowed?
+ if expiring_soon
+ trigger_expiring_soon_notification
+ else
+ trigger_expired_notification
+ end
+ end
+
+ private
+
+ def allowed?
+ user.can?(:receive_notifications)
+ end
+
+ def trigger_expiring_soon_notification
+ notification_service.ssh_key_expiring_soon(user, keys.map(&:fingerprint))
+
+ keys.update_all(before_expiry_notification_delivered_at: Time.current.utc)
+ end
+
+ def trigger_expired_notification
notification_service.ssh_key_expired(user, keys.map(&:fingerprint))
keys.update_all(expiry_notification_delivered_at: Time.current.utc)
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 6e014c0a6709e..6f1f3309ad99e 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -86,6 +86,13 @@ def ssh_key_expired(user, fingerprints)
mailer.ssh_key_expired_email(user, fingerprints).deliver_later
end
+ # Notify the user when at least one of their ssh key is expiring soon
+ def ssh_key_expiring_soon(user, fingerprints)
+ return unless user.can?(:receive_notifications)
+
+ mailer.ssh_key_expiring_soon_email(user, fingerprints).deliver_later
+ end
+
# Notify a user when a previously unknown IP or device is used to
# sign in to their account
def unknown_sign_in(user, ip, time)
diff --git a/app/views/notify/ssh_key_expiring_soon.text.erb b/app/views/notify/ssh_key_expiring_soon.text.erb
new file mode 100644
index 0000000000000..2a7c0cafe83f8
--- /dev/null
+++ b/app/views/notify/ssh_key_expiring_soon.text.erb
@@ -0,0 +1,9 @@
+<%= _('Hi %{username}!') % { username: sanitize_name(@user.name) } %>
+
+<%= _('Your SSH keys with the following fingerprints are scheduled to expire soon:') %>
+
+<% @fingerprints.each do |fingerprint| %>
+ - <%= fingerprint %>
+<% end %>
+
+<%= _('You can create a new one or check them in your SSH keys settings %{ssh_key_link}.') % { ssh_key_link: @target_url } %>
diff --git a/app/views/notify/ssh_key_expiring_soon_email.html.haml b/app/views/notify/ssh_key_expiring_soon_email.html.haml
new file mode 100644
index 0000000000000..f4aee9c5fded1
--- /dev/null
+++ b/app/views/notify/ssh_key_expiring_soon_email.html.haml
@@ -0,0 +1,13 @@
+%p
+ = _('Hi %{username}!') % { username: sanitize_name(@user.name) }
+%p
+ = _('Your SSH keys with the following fingerprints are scheduled to expire soon:')
+%table
+ %tbody
+ - @fingerprints.each do |fingerprint|
+ %tr
+ %td= fingerprint
+
+%p
+ - ssh_key_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: @target_url }
+ = html_escape(_('You can create a new one or check them in your %{ssh_key_link_start}SSH keys%{ssh_key_link_end} settings.')) % { ssh_key_link_start: ssh_key_link_start, ssh_key_link_end: '</a>'.html_safe }
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 359eb5499c760..24c96095ccc16 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -451,6 +451,14 @@
:weight: 1
:idempotent: true
:tags: []
+- :name: cronjob:ssh_keys_expiring_soon_notification
+ :feature_category: :compliance_management
+ :has_external_dependencies:
+ :urgency: :low
+ :resource_boundary: :unknown
+ :weight: 1
+ :idempotent: true
+ :tags: []
- :name: cronjob:stuck_ci_jobs
:feature_category: :continuous_integration
:has_external_dependencies:
diff --git a/app/workers/ssh_keys/expired_notification_worker.rb b/app/workers/ssh_keys/expired_notification_worker.rb
index 57a9060b57349..ab6d199877353 100644
--- a/app/workers/ssh_keys/expired_notification_worker.rb
+++ b/app/workers/ssh_keys/expired_notification_worker.rb
@@ -17,7 +17,7 @@ def perform
keys = user.expired_today_and_unnotified_keys
- Keys::ExpiryNotificationService.new(user, { keys: keys }).execute
+ Keys::ExpiryNotificationService.new(user, { keys: keys, expiring_soon: false }).execute
end
end
end
diff --git a/app/workers/ssh_keys/expiring_soon_notification_worker.rb b/app/workers/ssh_keys/expiring_soon_notification_worker.rb
new file mode 100644
index 0000000000000..3214cd7a24253
--- /dev/null
+++ b/app/workers/ssh_keys/expiring_soon_notification_worker.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+module SshKeys
+ class ExpiringSoonNotificationWorker
+ include ApplicationWorker
+ include CronjobQueue
+
+ feature_category :compliance_management
+ idempotent!
+
+ def perform
+ return unless ::Feature.enabled?(:ssh_key_expiration_email_notification, default_enabled: :yaml)
+
+ User.with_ssh_key_expiring_soon.find_each do |user|
+ with_context(user: user) do
+ Gitlab::AppLogger.info "#{self.class}: Notifying User #{user.id} about expiring soon ssh key(s)"
+
+ keys = user.expiring_soon_and_unnotified_keys
+
+ Keys::ExpiryNotificationService.new(user, { keys: keys, expiring_soon: true }).execute
+ end
+ end
+ end
+ end
+end
diff --git a/changelogs/unreleased/expiring-ssh-key-notification.yml b/changelogs/unreleased/expiring-ssh-key-notification.yml
new file mode 100644
index 0000000000000..d1e14249df3d9
--- /dev/null
+++ b/changelogs/unreleased/expiring-ssh-key-notification.yml
@@ -0,0 +1,5 @@
+---
+title: User notification when SSH key is set to expire soon
+merge_request: 58171
+author:
+type: added
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 7e0c2778e8543..c14df942ed886 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -569,6 +569,9 @@
Settings.cron_jobs['namespaces_in_product_marketing_emails_worker'] ||= Settingslogic.new({})
Settings.cron_jobs['namespaces_in_product_marketing_emails_worker']['cron'] ||= '0 9 * * *'
Settings.cron_jobs['namespaces_in_product_marketing_emails_worker']['job_class'] = 'Namespaces::InProductMarketingEmailsWorker'
+Settings.cron_jobs['ssh_keys_expiring_soon_notification_worker'] ||= Settingslogic.new({})
+Settings.cron_jobs['ssh_keys_expiring_soon_notification_worker']['cron'] ||= '0 1 * * *'
+Settings.cron_jobs['ssh_keys_expiring_soon_notification_worker']['job_class'] = 'SshKeys::ExpiringSoonNotificationWorker'
Gitlab.com do
Settings.cron_jobs['batched_background_migrations_worker'] ||= Settingslogic.new({})
diff --git a/db/migrate/20210401175134_add_before_expiry_notification_delivered_to_keys.rb b/db/migrate/20210401175134_add_before_expiry_notification_delivered_to_keys.rb
new file mode 100644
index 0000000000000..6a2ea0e738c5b
--- /dev/null
+++ b/db/migrate/20210401175134_add_before_expiry_notification_delivered_to_keys.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class AddBeforeExpiryNotificationDeliveredToKeys < ActiveRecord::Migration[6.0]
+ DOWNTIME = false
+
+ def change
+ add_column :keys, :before_expiry_notification_delivered_at, :datetime_with_timezone
+ end
+end
diff --git a/db/migrate/20210401192808_add_index_to_keys_on_expires_at_and_before_expiry_notification_undelivered.rb b/db/migrate/20210401192808_add_index_to_keys_on_expires_at_and_before_expiry_notification_undelivered.rb
new file mode 100644
index 0000000000000..ff792d2e6e6c4
--- /dev/null
+++ b/db/migrate/20210401192808_add_index_to_keys_on_expires_at_and_before_expiry_notification_undelivered.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class AddIndexToKeysOnExpiresAtAndBeforeExpiryNotificationUndelivered < ActiveRecord::Migration[6.0]
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+ INDEX_NAME = 'idx_keys_expires_at_and_before_expiry_notification_undelivered'
+ disable_ddl_transaction!
+
+ def up
+ add_concurrent_index :keys,
+ "date(timezone('UTC', expires_at)), before_expiry_notification_delivered_at",
+ where: 'before_expiry_notification_delivered_at IS NULL', name: INDEX_NAME
+ end
+
+ def down
+ remove_concurrent_index_by_name(:keys, INDEX_NAME)
+ end
+end
diff --git a/db/schema_migrations/20210401175134 b/db/schema_migrations/20210401175134
new file mode 100644
index 0000000000000..8b93c13bfdc47
--- /dev/null
+++ b/db/schema_migrations/20210401175134
@@ -0,0 +1 @@
+07d527134f776dbed2199f1717c34b3a6c41caadcaa3c50e6e5866f2cfad31b0
\ No newline at end of file
diff --git a/db/schema_migrations/20210401192808 b/db/schema_migrations/20210401192808
new file mode 100644
index 0000000000000..fd005c98732a8
--- /dev/null
+++ b/db/schema_migrations/20210401192808
@@ -0,0 +1 @@
+1cd4799ed7df41bfb9d96a7d18faaa9cbb2dc03f2a804c2bc3c1a6bba15d6d3d
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 1dd216b52da91..7bd7a5bfb26f5 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -13969,7 +13969,8 @@ CREATE TABLE keys (
last_used_at timestamp without time zone,
fingerprint_sha256 bytea,
expires_at timestamp with time zone,
- expiry_notification_delivered_at timestamp with time zone
+ expiry_notification_delivered_at timestamp with time zone,
+ before_expiry_notification_delivered_at timestamp with time zone
);
CREATE SEQUENCE keys_id_seq
@@ -21729,6 +21730,8 @@ CREATE INDEX idx_jira_connect_subscriptions_on_installation_id ON jira_connect_s
CREATE UNIQUE INDEX idx_jira_connect_subscriptions_on_installation_id_namespace_id ON jira_connect_subscriptions USING btree (jira_connect_installation_id, namespace_id);
+CREATE INDEX idx_keys_expires_at_and_before_expiry_notification_undelivered ON keys USING btree (date(timezone('UTC'::text, expires_at)), before_expiry_notification_delivered_at) WHERE (before_expiry_notification_delivered_at IS NULL);
+
CREATE INDEX idx_members_created_at_user_id_invite_token ON members USING btree (created_at) WHERE ((invite_token IS NOT NULL) AND (user_id IS NULL));
CREATE INDEX idx_merge_requests_on_id_and_merge_jid ON merge_requests USING btree (id, merge_jid) WHERE ((merge_jid IS NOT NULL) AND (state_id = 4));
diff --git a/doc/ssh/README.md b/doc/ssh/README.md
index 38356878822ba..87213f72534d0 100644
--- a/doc/ssh/README.md
+++ b/doc/ssh/README.md
@@ -218,7 +218,8 @@ To use SSH with GitLab, copy your public key to your GitLab account.
The expiration date is informational only, and does not prevent you from using
the key. However, administrators can view expiration dates and
use them for guidance when [deleting keys](../user/admin_area/credentials_inventory.md#delete-a-users-ssh-key).
- GitLab checks all SSH keys at 02:00 AM UTC every day. It emails an expiration notice for all SSH keys that expire on the current date. ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/322637) in GitLab 13.11.)
+ - GitLab checks all SSH keys at 02:00 AM UTC every day. It emails an expiration notice for all SSH keys that expire on the current date. ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/322637) in GitLab 13.11.)
+ - GitLab checks all SSH keys at 01:00 AM UTC every day. It emails an expiration notice for all SSH keys that are scheduled to expire seven days from now. ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/322637) in GitLab 13.11.)
1. Select **Add key**.
## Verify that you can connect
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 8feaf10d5d887..ee7c82a3d4f7b 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -35592,12 +35592,18 @@ msgstr ""
msgid "Your SSH key has expired"
msgstr ""
+msgid "Your SSH key is expiring soon."
+msgstr ""
+
msgid "Your SSH key was deleted"
msgstr ""
msgid "Your SSH keys (%{count})"
msgstr ""
+msgid "Your SSH keys with the following fingerprints are scheduled to expire soon:"
+msgstr ""
+
msgid "Your SSH keys with the following fingerprints has expired:"
msgstr ""
diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb
index 764186dc22643..8ac1f15d67ebb 100644
--- a/spec/mailers/emails/profile_spec.rb
+++ b/spec/mailers/emails/profile_spec.rb
@@ -212,52 +212,101 @@
end
end
- describe 'notification email for expired ssh key' do
- let_it_be(:user) { create(:user) }
+ describe 'SSH key notification' do
+ let_it_be_with_reload(:user) { create(:user) }
let_it_be(:fingerprints) { ["aa:bb:cc:dd:ee:zz"] }
- context 'when valid' do
- subject { Notify.ssh_key_expired_email(user, fingerprints) }
+ shared_examples 'is sent to the user' do
+ it { is_expected.to deliver_to user.email }
+ end
+
+ shared_examples 'has the correct subject' do |subject_text|
+ it { is_expected.to have_subject subject_text }
+ end
+
+ shared_examples 'has the correct body text' do |body_text|
+ it { is_expected.to have_body_text body_text }
+ end
+
+ shared_examples 'includes a link to ssh key page' do
+ it { is_expected.to have_body_text /#{profile_keys_url}/ }
+ end
+
+ shared_examples 'includes the email reason' do
+ it { is_expected.to have_body_text /You're receiving this email because of your account on localhost/ }
+ end
+ shared_examples 'valid use case' do
it_behaves_like 'an email sent from GitLab'
it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like 'a user cannot unsubscribe through footer link'
+ it_behaves_like 'is sent to the user'
+ it_behaves_like 'includes a link to ssh key page'
+ it_behaves_like 'includes the email reason'
+ end
- it 'is sent to the user' do
- is_expected.to deliver_to user.email
+ shared_examples 'does not send email' do
+ it do
+ expect { subject }.not_to change { ActionMailer::Base.deliveries.count }
end
+ end
- it 'has the correct subject' do
- is_expected.to have_subject /Your SSH key has expired/
+ shared_context 'block user' do
+ before do
+ user.block!
end
+ end
- it 'mentions the ssh keu has expired' do
- is_expected.to have_body_text /Your SSH keys with the following fingerprints has expired/
- end
+ context 'notification email for expired ssh key' do
+ context 'when valid' do
+ subject { Notify.ssh_key_expired_email(user, fingerprints) }
- it 'includes a link to ssh key page' do
- is_expected.to have_body_text /#{profile_keys_url}/
+ include_examples 'valid use case'
+
+ it_behaves_like 'has the correct subject', /Your SSH key has expired/
+ it_behaves_like 'has the correct body text', /Your SSH keys with the following fingerprints has expired/
end
- it 'includes the email reason' do
- is_expected.to have_body_text /You're receiving this email because of your account on localhost/
+ context 'when invalid' do
+ context 'when user does not exist' do
+ subject { Notify.ssh_key_expired_email(nil, fingerprints) }
+
+ it_behaves_like 'does not send email'
+ end
+
+ context 'when user is not active' do
+ subject { Notify.ssh_key_expired_email(user, fingerprints) }
+
+ include_context 'block user'
+
+ it_behaves_like 'does not send email'
+ end
end
end
- context 'when invalid' do
- context 'when user does not exist' do
- it do
- expect { Notify.ssh_key_expired_email(nil) }.not_to change { ActionMailer::Base.deliveries.count }
- end
+ context 'notification email for expiring ssh key' do
+ context 'when valid' do
+ subject { Notify.ssh_key_expiring_soon_email(user, fingerprints) }
+
+ include_examples 'valid use case'
+
+ it_behaves_like 'has the correct subject', /Your SSH key is expiring soon/
+ it_behaves_like 'has the correct body text', /Your SSH keys with the following fingerprints are scheduled to expire soon/
end
- context 'when user is not active' do
- before do
- user.block!
+ context 'when invalid' do
+ context 'when user does not exist' do
+ subject { Notify.ssh_key_expiring_soon_email(nil, fingerprints) }
+
+ it_behaves_like 'does not send email'
end
- it do
- expect { Notify.ssh_key_expired_email(user) }.not_to change { ActionMailer::Base.deliveries.count }
+ context 'when user is not active' do
+ subject { Notify.ssh_key_expiring_soon_email(user, fingerprints) }
+
+ include_context 'block user'
+
+ it_behaves_like 'does not send email'
end
end
end
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index 195ec1fa83b9e..0cb20efcb0a7c 100644
--- a/spec/models/key_spec.rb
+++ b/spec/models/key_spec.rb
@@ -76,15 +76,25 @@
end
end
- describe '.expired_today_and_not_notified' do
+ context 'expiration scopes' do
let_it_be(:user) { create(:user) }
let_it_be(:expired_today_not_notified) { create(:key, expires_at: Time.current, user: user) }
let_it_be(:expired_today_already_notified) { create(:key, expires_at: Time.current, user: user, expiry_notification_delivered_at: Time.current) }
let_it_be(:expired_yesterday) { create(:key, expires_at: 1.day.ago, user: user) }
- let_it_be(:future_expiry) { create(:key, expires_at: 1.day.from_now, user: user) }
+ let_it_be(:expiring_soon_unotified) { create(:key, expires_at: 3.days.from_now, user: user) }
+ let_it_be(:expiring_soon_notified) { create(:key, expires_at: 4.days.from_now, user: user, before_expiry_notification_delivered_at: Time.current) }
+ let_it_be(:future_expiry) { create(:key, expires_at: 1.month.from_now, user: user) }
+
+ describe '.expired_today_and_not_notified' do
+ it 'returns keys that expire today' do
+ expect(described_class.expired_today_and_not_notified).to contain_exactly(expired_today_not_notified)
+ end
+ end
- it 'returns tokens that have expired today' do
- expect(described_class.expired_today_and_not_notified).to contain_exactly(expired_today_not_notified)
+ describe '.expiring_soon_and_not_notified' do
+ it 'returns keys that will expire soon' do
+ expect(described_class.expiring_soon_and_not_notified).to contain_exactly(expiring_soon_unotified)
+ end
end
end
end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index ee67afcd50b33..520f616bfa049 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1001,15 +1001,24 @@
end
end
- describe '.with_ssh_key_expired_today' do
+ context 'SSH key expiration scopes' do
let_it_be(:user1) { create(:user) }
- let_it_be(:expired_today_not_notified) { create(:key, expires_at: Time.current, user: user1) }
-
let_it_be(:user2) { create(:user) }
+ let_it_be(:expired_today_not_notified) { create(:key, expires_at: Time.current, user: user1) }
let_it_be(:expired_today_already_notified) { create(:key, expires_at: Time.current, user: user2, expiry_notification_delivered_at: Time.current) }
+ let_it_be(:expiring_soon_not_notified) { create(:key, expires_at: 2.days.from_now, user: user2) }
+ let_it_be(:expiring_soon_notified) { create(:key, expires_at: 2.days.from_now, user: user1, before_expiry_notification_delivered_at: Time.current) }
- it 'returns users whose token has expired today' do
- expect(described_class.with_ssh_key_expired_today).to contain_exactly(user1)
+ describe '.with_ssh_key_expired_today' do
+ it 'returns users whose key has expired today' do
+ expect(described_class.with_ssh_key_expired_today).to contain_exactly(user1)
+ end
+ end
+
+ describe '.with_ssh_key_expiring_soon' do
+ it 'returns users whose keys will expire soon' do
+ expect(described_class.with_ssh_key_expiring_soon).to contain_exactly(user2)
+ end
end
end
diff --git a/spec/services/keys/expiry_notification_service_spec.rb b/spec/services/keys/expiry_notification_service_spec.rb
index 4cfd576a15f21..1d1da179cf78c 100644
--- a/spec/services/keys/expiry_notification_service_spec.rb
+++ b/spec/services/keys/expiry_notification_service_spec.rb
@@ -4,48 +4,93 @@
RSpec.describe Keys::ExpiryNotificationService do
let_it_be_with_reload(:user) { create(:user) }
- let_it_be_with_reload(:expired_key) { create(:key, expires_at: Time.current, user: user) }
- let(:params) { { keys: keys } }
+ let(:params) { { keys: user.keys, expiring_soon: expiring_soon } }
subject { described_class.new(user, params) }
- context 'with expired key', :mailer do
- let(:keys) { user.keys }
-
- it 'sends a notification' do
+ shared_examples 'sends a notification' do
+ it do
perform_enqueued_jobs do
subject.execute
end
should_email(user)
end
+ end
- it 'uses notification service to send email to the user' do
+ shared_examples 'uses notification service to send email to the user' do |notification_method|
+ it do
expect_next_instance_of(NotificationService) do |notification_service|
- expect(notification_service).to receive(:ssh_key_expired).with(expired_key.user, [expired_key.fingerprint])
+ expect(notification_service).to receive(notification_method).with(key.user, [key.fingerprint])
end
subject.execute
end
+ end
- it 'updates notified column' do
- expect { subject.execute }.to change { expired_key.reload.expiry_notification_delivered_at }
+ shared_examples 'does not send notification' do
+ it do
+ perform_enqueued_jobs do
+ subject.execute
+ end
+ should_not_email(user)
end
+ end
- context 'when user does not have permission to receive notification' do
- before do
- user.block!
+ shared_context 'block user' do
+ before do
+ user.block!
+ end
+ end
+
+ context 'with key expiring today', :mailer do
+ let_it_be_with_reload(:key) { create(:key, expires_at: Time.current, user: user) }
+
+ let(:expiring_soon) { false }
+
+ context 'when user has permission to receive notification' do
+ it_behaves_like 'sends a notification'
+
+ it_behaves_like 'uses notification service to send email to the user', :ssh_key_expired
+
+ it 'updates notified column' do
+ expect { subject.execute }.to change { key.reload.expiry_notification_delivered_at }
end
+ end
- it 'does not send notification' do
- perform_enqueued_jobs do
- subject.execute
- end
- should_not_email(user)
+ context 'when user does NOT have permission to receive notification' do
+ include_context 'block user'
+
+ it_behaves_like 'does not send notification'
+
+ it 'does not update notified column' do
+ expect { subject.execute }.not_to change { key.reload.expiry_notification_delivered_at }
+ end
+ end
+ end
+
+ context 'with key expiring soon', :mailer do
+ let_it_be_with_reload(:key) { create(:key, expires_at: 3.days.from_now, user: user) }
+
+ let(:expiring_soon) { true }
+
+ context 'when user has permission to receive notification' do
+ it_behaves_like 'sends a notification'
+
+ it_behaves_like 'uses notification service to send email to the user', :ssh_key_expiring_soon
+
+ it 'updates notified column' do
+ expect { subject.execute }.to change { key.reload.before_expiry_notification_delivered_at }
end
+ end
+
+ context 'when user does NOT have permission to receive notification' do
+ include_context 'block user'
+
+ it_behaves_like 'does not send notification'
it 'does not update notified column' do
- expect { subject.execute }.not_to change { expired_key.reload.expiry_notification_delivered_at }
+ expect { subject.execute }.not_to change { key.reload.before_expiry_notification_delivered_at }
end
end
end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 63bfc090a0dff..6eff768eac29c 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -288,11 +288,19 @@
end
end
end
+ end
- describe '#ssh_key_expired' do
- let_it_be(:user) { create(:user) }
- let_it_be(:fingerprints) { ["aa:bb:cc:dd:ee:zz"] }
+ describe 'SSH Keys' do
+ let_it_be_with_reload(:user) { create(:user) }
+ let_it_be(:fingerprints) { ["aa:bb:cc:dd:ee:zz"] }
+ shared_context 'block user' do
+ before do
+ user.block!
+ end
+ end
+
+ describe '#ssh_key_expired' do
subject { notification.ssh_key_expired(user, fingerprints) }
it 'sends email to the token owner' do
@@ -300,15 +308,29 @@
end
context 'when user is not allowed to receive notifications' do
- before do
- user.block!
- end
+ include_context 'block user'
it 'does not send email to the token owner' do
expect { subject }.not_to have_enqueued_email(user, fingerprints, mail: "ssh_key_expired_email")
end
end
end
+
+ describe '#ssh_key_expiring_soon' do
+ subject { notification.ssh_key_expiring_soon(user, fingerprints) }
+
+ it 'sends email to the token owner' do
+ expect { subject }.to have_enqueued_email(user, fingerprints, mail: "ssh_key_expiring_soon_email")
+ end
+
+ context 'when user is not allowed to receive notifications' do
+ include_context 'block user'
+
+ it 'does not send email to the token owner' do
+ expect { subject }.not_to have_enqueued_email(user, fingerprints, mail: "ssh_key_expiring_soon_email")
+ end
+ end
+ end
end
describe '#unknown_sign_in' do
diff --git a/spec/workers/ssh_keys/expiring_soon_notification_worker_spec.rb b/spec/workers/ssh_keys/expiring_soon_notification_worker_spec.rb
new file mode 100644
index 0000000000000..f9276c86cdf28
--- /dev/null
+++ b/spec/workers/ssh_keys/expiring_soon_notification_worker_spec.rb
@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe SshKeys::ExpiringSoonNotificationWorker, type: :worker do
+ subject(:worker) { described_class.new }
+
+ it 'uses a cronjob queue' do
+ expect(worker.sidekiq_options_hash).to include(
+ 'queue' => 'cronjob:ssh_keys_expiring_soon_notification',
+ 'queue_namespace' => :cronjob
+ )
+ end
+
+ describe '#perform' do
+ let_it_be(:user) { create(:user) }
+
+ context 'with key expiring soon' do
+ let_it_be_with_reload(:expiring_soon) { create(:key, expires_at: 6.days.from_now, user: user) }
+
+ it 'invoke the notification service' do
+ expect_next_instance_of(Keys::ExpiryNotificationService) do |expiry_service|
+ expect(expiry_service).to receive(:execute)
+ end
+
+ worker.perform
+ end
+
+ it 'updates notified column' do
+ expect { worker.perform }.to change { expiring_soon.reload.before_expiry_notification_delivered_at }
+ end
+
+ include_examples 'an idempotent worker' do
+ subject do
+ perform_multiple(worker: worker)
+ end
+ end
+
+ context 'when feature is not enabled' do
+ before do
+ stub_feature_flags(ssh_key_expiration_email_notification: false)
+ end
+
+ it 'does not update notified column' do
+ expect { worker.perform }.not_to change { expiring_soon.reload.before_expiry_notification_delivered_at }
+ end
+ end
+ end
+
+ context 'when key has expired in the past' do
+ let_it_be(:expired_past) { create(:key, expires_at: 1.day.ago, user: user) }
+
+ it 'does not update notified column' do
+ expect { worker.perform }.not_to change { expired_past.reload.before_expiry_notification_delivered_at }
+ end
+ end
+
+ context 'when key is not expiring soon' do
+ let_it_be(:expires_future) { create(:key, expires_at: 8.days.from_now, user: user) }
+
+ it 'does not update notified column' do
+ expect { worker.perform }.not_to change { expires_future.reload.before_expiry_notification_delivered_at }
+ end
+ end
+ end
+end
--
GitLab