Merge branch '331423-ssh-expiration-past-expiry' into 'master'

Send SSH key expiration notification emails for all expired keys

See merge request gitlab-org/gitlab!62114

app/models/key.rb

@@ -7,6 +7,7 @@ class Key < ApplicationRecord
   include Sortable
   include Sha256Attribute
   include Expirable
+  include EachBatch
 
   sha256_attribute :fingerprint_sha256
 
@@ -43,7 +44,9 @@ class Key < ApplicationRecord
   scope :preload_users, -> { preload(:user) }
   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"]) }
+
+  # Date is set specifically in this scope to improve query time.
+  scope :expired_and_not_notified, -> { where(["date(expires_at AT TIME ZONE 'UTC') BETWEEN '2000-01-01' AND 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

app/models/user.rb

@@ -108,7 +108,7 @@ class User < ApplicationRecord
 
   # 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 :expired_and_unnotified_keys, -> { expired_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
@@ -411,14 +411,7 @@ class User < ApplicationRecord
             .without_impersonation
             .expired_today_and_not_notified)
   end
-  scope :with_ssh_key_expired_today, -> do
-    includes(:expired_today_and_unnotified_keys)
-      .where('EXISTS (?)',
-        ::Key
-        .select(1)
-        .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 (?)',

app/workers/ssh_keys/expired_notification_worker.rb

@@ -15,16 +15,16 @@ module SshKeys
       return unless ::Feature.enabled?(:ssh_key_expiration_email_notification, default_enabled: :yaml)
 
       # rubocop:disable CodeReuse/ActiveRecord
-      User.with_ssh_key_expired_today.find_each(batch_size: 10_000) do |user|
-        with_context(user: user) do
-          Gitlab::AppLogger.info "#{self.class}: Notifying User #{user.id} about expired ssh key(s)"
-
-          keys = user.expired_today_and_unnotified_keys
+      Key.expired_and_not_notified.each_batch(of: 1000) do |relation| # rubocop:disable Cop/InBatches
+        users = User.where(id: relation.select(:user_id))
 
-          Keys::ExpiryNotificationService.new(user, { keys: keys, expiring_soon: false }).execute
+        users.each do |user|
+          with_context(user: user) do
+            Keys::ExpiryNotificationService.new(user, { keys: user.expired_and_unnotified_keys, expiring_soon: false }).execute
+          end
         end
-        # rubocop:enable CodeReuse/ActiveRecord
       end
+      # rubocop:enable CodeReuse/ActiveRecord
     end
   end
 end

spec/models/key_spec.rb

@@ -85,9 +85,9 @@ RSpec.describe Key, :mailer do
       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)
+      describe '.expired_and_not_notified' do
+        it 'returns keys that expire today and in the past' do
+          expect(described_class.expired_and_not_notified).to contain_exactly(expired_today_not_notified, expired_yesterday)
         end
       end
 

spec/models/user_spec.rb

@@ -92,7 +92,7 @@ RSpec.describe User do
     it { is_expected.to have_many(:group_members) }
     it { is_expected.to have_many(:groups) }
     it { is_expected.to have_many(:keys).dependent(:destroy) }
-    it { is_expected.to have_many(:expired_today_and_unnotified_keys) }
+    it { is_expected.to have_many(:expired_and_unnotified_keys) }
     it { is_expected.to have_many(:deploy_keys).dependent(:nullify) }
     it { is_expected.to have_many(:group_deploy_keys) }
     it { is_expected.to have_many(:events).dependent(:delete_all) }
@@ -1027,12 +1027,6 @@ RSpec.describe User do
       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) }
 
-      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)

spec/workers/ssh_keys/expired_notification_worker_spec.rb

@@ -50,9 +50,19 @@ RSpec.describe SshKeys::ExpiredNotificationWorker, type: :worker do
     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 update notified column' do
+        expect { worker.perform }.to change { expired_past.reload.expiry_notification_delivered_at }
+      end
+
+      context 'when key has already been notified of expiration' do
+        before do
+          expired_past.update!(expiry_notification_delivered_at: 1.day.ago)
+        end
+
         it 'does not update notified column' do
           expect { worker.perform }.not_to change { expired_past.reload.expiry_notification_delivered_at }
         end
       end
     end
+  end
 end