Merge branch 'ssh_key_expiration' into 'master'

Expiry email notification for SSH keys [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!56888

app/mailers/emails/profile.rb

@@ -74,6 +74,18 @@ module Emails
       end
     end
 
+    def ssh_key_expired_email(user, fingerprints)
+      return unless user && 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 has expired")))
+      end
+    end
+
     def unknown_sign_in_email(user, ip, time)
       @user = user
       @ip = ip

app/models/key.rb

@@ -43,6 +43,7 @@ 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"]) }
 
   def self.regular_keys
     where(type: ['Key', nil])

app/models/user.rb

@@ -103,6 +103,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 :deploy_keys, -> { where(type: 'DeployKey') }, dependent: :nullify # rubocop:disable Cop/ActiveRecordDependent
   has_many :group_deploy_keys
   has_many :gpg_keys
@@ -398,6 +399,14 @@ 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 :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')) }

app/services/keys/expiry_notification_service.rb

+# frozen_string_literal: true
+
+module Keys
+  class ExpiryNotificationService < ::Keys::BaseService
+    attr_accessor :keys
+
+    def initialize(user, params)
+      @keys = params[:keys]
+
+      super
+    end
+
+    def execute
+      return unless user.can?(:receive_notifications)
+
+      notification_service.ssh_key_expired(user, keys.map(&:fingerprint))
+
+      keys.update_all(expiry_notification_delivered_at: Time.current.utc)
+    end
+  end
+end

app/services/notification_service.rb

@@ -79,6 +79,13 @@ class NotificationService
     mailer.access_token_expired_email(user).deliver_later
   end
 
+  # Notify the user when at least one of their ssh key has expired today
+  def ssh_key_expired(user, fingerprints)
+    return unless user.can?(:receive_notifications)
+
+    mailer.ssh_key_expired_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)

app/views/notify/ssh_key_expired_email.html.haml

+%p
+  = _('Hi %{username}!') % { username: sanitize_name(@user.name) }
+%p
+  = _('Your SSH keys with the following fingerprints has expired:')
+%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 }

app/views/notify/ssh_key_expired_email.text.erb

+<%= _('Hi %{username}!') % { username: sanitize_name(@user.name) } %>
+
+<%= _('Your SSH keys with the following fingerprints has expired:') %>
+
+<% @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 } %>

app/workers/all_queues.yml

@@ -443,6 +443,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: cronjob:ssh_keys_expired_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:

app/workers/ssh_keys/expired_notification_worker.rb

+# frozen_string_literal: true
+
+module SshKeys
+  class ExpiredNotificationWorker
+    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_expired_today.find_each 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
+
+          Keys::ExpiryNotificationService.new(user, { keys: keys }).execute
+        end
+      end
+    end
+  end
+end

changelogs/unreleased/ssh_key_expiration.yml

+---
+title: Send email notification on SSH key expiration
+merge_request: 56888
+author:
+type: added

config/feature_flags/development/ssh_key_expiration_email_notification.yml

+---
+name: ssh_key_expiration_email_notification
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56888
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/326386
+milestone: '13.11'
+type: development
+group: group::compliance
+default_enabled: false

config/initializers/1_settings.rb

@@ -560,6 +560,9 @@ Settings.cron_jobs['manage_evidence_worker']['job_class'] = 'Releases::ManageEvi
 Settings.cron_jobs['user_status_cleanup_batch_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['user_status_cleanup_batch_worker']['cron'] ||= '* * * * *'
 Settings.cron_jobs['user_status_cleanup_batch_worker']['job_class'] = 'UserStatusCleanup::BatchWorker'
+Settings.cron_jobs['ssh_keys_expired_notification_worker'] ||= Settingslogic.new({})
+Settings.cron_jobs['ssh_keys_expired_notification_worker']['cron'] ||= '0 2 * * *'
+Settings.cron_jobs['ssh_keys_expired_notification_worker']['job_class'] = 'SshKeys::ExpiredNotificationWorker'
 
 Gitlab.com do
   Settings.cron_jobs['namespaces_in_product_marketing_emails_worker'] ||= Settingslogic.new({})

db/migrate/20210317192943_add_expiry_notification_delivered_to_keys.rb

+# frozen_string_literal: true
+
+class AddExpiryNotificationDeliveredToKeys < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :keys, :expiry_notification_delivered_at, :datetime_with_timezone
+  end
+end

db/migrate/20210322182751_add_index_to_keys_on_expires_at_and_expiry_notification_undelivered.rb

+# frozen_string_literal: true
+
+class AddIndexToKeysOnExpiresAtAndExpiryNotificationUndelivered < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_keys_on_expires_at_and_expiry_notification_undelivered'
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :keys,
+                         "date(timezone('UTC', expires_at)), expiry_notification_delivered_at",
+                         where: 'expiry_notification_delivered_at IS NULL', name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name(:keys, INDEX_NAME)
+  end
+end

db/schema_migrations/20210317192943

+dfb88ea7a213da1e56bef532255f01a284d7b9be9ec8a6b9dd0e95ec04d0f524
\ No newline at end of file

db/schema_migrations/20210322182751

+79ad2de15faef8edb8752c2a9c89f1739a805af999c86db6e73482a613c4f9d1
\ No newline at end of file

db/structure.sql

@@ -14031,7 +14031,8 @@ CREATE TABLE keys (
     public boolean DEFAULT false NOT NULL,
     last_used_at timestamp without time zone,
     fingerprint_sha256 bytea,
-    expires_at timestamp with time zone
+    expires_at timestamp with time zone,
+    expiry_notification_delivered_at timestamp with time zone
 );
 
 CREATE SEQUENCE keys_id_seq
@@ -22991,6 +22992,8 @@ CREATE INDEX index_jira_imports_on_user_id ON jira_imports USING btree (user_id)
 
 CREATE INDEX index_jira_tracker_data_on_service_id ON jira_tracker_data USING btree (service_id);
 
+CREATE INDEX index_keys_on_expires_at_and_expiry_notification_undelivered ON keys USING btree (date(timezone('UTC'::text, expires_at)), expiry_notification_delivered_at) WHERE (expiry_notification_delivered_at IS NULL);
+
 CREATE UNIQUE INDEX index_keys_on_fingerprint ON keys USING btree (fingerprint);
 
 CREATE INDEX index_keys_on_fingerprint_sha256 ON keys USING btree (fingerprint_sha256);

doc/ssh/README.md

@@ -218,6 +218,7 @@ 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.)
 1. Select **Add key**.
 
 ## Verify that you can connect

locale/gitlab.pot

@@ -34818,6 +34818,12 @@ msgstr ""
 msgid "You can create a new one or check them in your %{pat_link_start}personal access tokens%{pat_link_end} settings"
 msgstr ""
 
+msgid "You can create a new one or check them in your %{ssh_key_link_start}SSH keys%{ssh_key_link_end} settings."
+msgstr ""
+
+msgid "You can create a new one or check them in your SSH keys settings %{ssh_key_link}."
+msgstr ""
+
 msgid "You can create a new one or check them in your personal access tokens settings %{pat_link}"
 msgstr ""
 
@@ -35313,12 +35319,18 @@ msgstr ""
 msgid "Your Public Email will be displayed on your public profile."
 msgstr ""
 
+msgid "Your SSH key has expired"
+msgstr ""
+
 msgid "Your SSH key was deleted"
 msgstr ""
 
 msgid "Your SSH keys (%{count})"
 msgstr ""
 
+msgid "Your SSH keys with the following fingerprints has expired:"
+msgstr ""
+
 msgid "Your To-Do List"
 msgstr ""
 

spec/mailers/emails/profile_spec.rb

@@ -212,6 +212,57 @@ RSpec.describe Emails::Profile do
     end
   end
 
+  describe 'notification email for expired ssh key' do
+    let_it_be(: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) }
+
+      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 'is sent to the user' do
+        is_expected.to deliver_to user.email
+      end
+
+      it 'has the correct subject' do
+        is_expected.to have_subject /Your SSH key has expired/
+      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
+
+      it 'includes a link to ssh key page' do
+        is_expected.to have_body_text /#{profile_keys_url}/
+      end
+
+      it 'includes the email reason' do
+        is_expected.to have_body_text /You're receiving this email because of your account on localhost/
+      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
+      end
+
+      context 'when user is not active' do
+        before do
+          user.block!
+        end
+
+        it do
+          expect { Notify.ssh_key_expired_email(user) }.not_to change { ActionMailer::Base.deliveries.count }
+        end
+      end
+    end
+  end
+
   describe 'user unknown sign in email' do
     let_it_be(:user) { create(:user) }
     let_it_be(:ip) { '169.0.0.1' }

spec/models/key_spec.rb

@@ -75,6 +75,18 @@ RSpec.describe Key, :mailer do
           .to eq([key_3, key_1, key_2])
       end
     end
+
+    describe '.expired_today_and_not_notified' 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) }
+
+      it 'returns tokens that have expired today' do
+        expect(described_class.expired_today_and_not_notified).to contain_exactly(expired_today_not_notified)
+      end
+    end
   end
 
   context "validation of uniqueness (based on fingerprint uniqueness)" do

spec/models/user_spec.rb

@@ -85,6 +85,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(:deploy_keys).dependent(:nullify) }
     it { is_expected.to have_many(:group_deploy_keys) }
     it { is_expected.to have_many(:events).dependent(:delete_all) }
@@ -1000,6 +1001,18 @@ RSpec.describe User do
       end
     end
 
+    describe '.with_ssh_key_expired_today' 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_already_notified) { create(:key, expires_at: Time.current, user: user2, 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)
+      end
+    end
+
     describe '.active_without_ghosts' do
       let_it_be(:user1) { create(:user, :external) }
       let_it_be(:user2) { create(:user, state: 'blocked') }

spec/services/keys/expiry_notification_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+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 } }
+
+  subject { described_class.new(user, params) }
+
+  context 'with expired key', :mailer do
+    let(:keys) { user.keys }
+
+    it 'sends a notification' do
+      perform_enqueued_jobs do
+        subject.execute
+      end
+      should_email(user)
+    end
+
+    it 'uses notification service to send email to the user' do
+      expect_next_instance_of(NotificationService) do |notification_service|
+        expect(notification_service).to receive(:ssh_key_expired).with(expired_key.user, [expired_key.fingerprint])
+      end
+
+      subject.execute
+    end
+
+    it 'updates notified column' do
+      expect { subject.execute }.to change { expired_key.reload.expiry_notification_delivered_at }
+    end
+
+    context 'when user does not have permission to receive notification' do
+      before do
+        user.block!
+      end
+
+      it 'does not send notification' do
+        perform_enqueued_jobs do
+          subject.execute
+        end
+        should_not_email(user)
+      end
+
+      it 'does not update notified column' do
+        expect { subject.execute }.not_to change { expired_key.reload.expiry_notification_delivered_at }
+      end
+    end
+  end
+end

spec/services/notification_service_spec.rb

@@ -288,6 +288,27 @@ RSpec.describe NotificationService, :mailer do
         end
       end
     end
+
+    describe '#ssh_key_expired' do
+      let_it_be(:user) { create(:user) }
+      let_it_be(:fingerprints) { ["aa:bb:cc:dd:ee:zz"] }
+
+      subject { notification.ssh_key_expired(user, fingerprints) }
+
+      it 'sends email to the token owner' do
+        expect { subject }.to have_enqueued_email(user, fingerprints, mail: "ssh_key_expired_email")
+      end
+
+      context 'when user is not allowed to receive notifications' do
+        before do
+          user.block!
+        end
+
+        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
   end
 
   describe '#unknown_sign_in' do

spec/workers/ssh_keys/expired_notification_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe SshKeys::ExpiredNotificationWorker, 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_expired_notification',
+      'queue_namespace' => :cronjob
+    )
+  end
+
+  describe '#perform' do
+    let_it_be(:user) { create(:user) }
+
+    context 'with expiring key today' do
+      let_it_be_with_reload(:expired_today) { create(:key, expires_at: Time.current, 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 { expired_today.reload.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 { expired_today.reload.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.expiry_notification_delivered_at }
+      end
+    end
+  end
+end