Merge branch 'additional-pat-expiry-notification' into 'master'

Email notification for Expired Personal Access Token

See merge request gitlab-org/gitlab!38086

app/mailers/emails/profile.rb

@@ -45,6 +45,17 @@ module Emails
       end
     end
 
+    def access_token_expired_email(user)
+      return unless user && user.active?
+
+      @user = user
+      @target_url = profile_personal_access_tokens_url
+
+      Gitlab::I18n.with_locale(@user.preferred_language) do
+        mail(to: @user.notification_email, subject: subject(_("Your personal access token has expired")))
+      end
+    end
+
     def unknown_sign_in_email(user, ip, time)
       @user = user
       @ip = ip

app/models/personal_access_token.rb

@@ -19,6 +19,7 @@ class PersonalAccessToken < ApplicationRecord
 
   scope :active, -> { where("revoked = false AND (expires_at >= CURRENT_DATE OR expires_at IS NULL)") }
   scope :expiring_and_not_notified, ->(date) { where(["revoked = false AND expire_notification_delivered = false AND expires_at >= CURRENT_DATE AND expires_at <= ?", date]) }
+  scope :expired_today_and_not_notified, -> { where(["revoked = false AND expires_at = CURRENT_DATE AND after_expiry_notification_delivered = false"]) }
   scope :inactive, -> { where("revoked = true OR expires_at < CURRENT_DATE") }
   scope :with_impersonation, -> { where(impersonation: true) }
   scope :without_impersonation, -> { where(impersonation: false) }

app/models/user.rb

@@ -350,6 +350,14 @@ class User < ApplicationRecord
             .without_impersonation
             .expiring_and_not_notified(at).select(1))
   end
+  scope :with_personal_access_tokens_expired_today, -> do
+    where('EXISTS (?)',
+          ::PersonalAccessToken
+            .select(1)
+            .where('personal_access_tokens.user_id = users.id')
+            .without_impersonation
+            .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/notification_service.rb

@@ -66,6 +66,13 @@ class NotificationService
     mailer.access_token_about_to_expire_email(user).deliver_later
   end
 
+  # Notify the user when at least one of their personal access tokens has expired today
+  def access_token_expired(user)
+    return unless user.can?(:receive_notifications)
+
+    mailer.access_token_expired_email(user).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/access_token_about_to_expire_email.html.haml

@@ -4,4 +4,4 @@
   = _('One or more of your personal access tokens will expire in %{days_to_expire} days or less.') % { days_to_expire: @days_to_expire }
 %p
   - pat_link_start =  '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: @target_url }
-  = _('You can create a new one or check them in your %{pat_link_start}Personal Access Tokens%{pat_link_end} settings').html_safe % { pat_link_start: pat_link_start, pat_link_end: '</a>'.html_safe }
+  = html_escape(_('You can create a new one or check them in your %{pat_link_start}personal access tokens%{pat_link_end} settings')) % { pat_link_start: pat_link_start, pat_link_end: '</a>'.html_safe }

app/views/notify/access_token_about_to_expire_email.text.erb

@@ -2,4 +2,4 @@
 
 <%= _('One or more of your personal access tokens will expire in %{days_to_expire} days or less.') % { days_to_expire: @days_to_expire} %>
 
-<%= _('You can create a new one or check them in your Personal Access Tokens settings %{pat_link}') % { pat_link: @target_url } %>
+<%= _('You can create a new one or check them in your personal access tokens settings %{pat_link}') % { pat_link: @target_url } %>

app/views/notify/access_token_expired_email.html.haml

+%p
+  = _('Hi %{username}!') % { username: sanitize_name(@user.name) }
+%p
+  = _('One or more of your personal access tokens has expired.')
+%p
+  - pat_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 %{pat_link_start}personal access tokens%{pat_link_end} settings')) % { pat_link_start: pat_link_start, pat_link_end: '</a>'.html_safe }

app/views/notify/access_token_expired_email.text.erb

+<%= _('Hi %{username}!') % { username: sanitize_name(@user.name) } %>
+
+<%= _('One or more of your personal access tokens has expired.') %>
+
+<%= _('You can create a new one or check them in your personal access tokens settings %{pat_link}') % { pat_link: @target_url } %>

app/workers/all_queues.yml

@@ -243,6 +243,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: cronjob:personal_access_tokens_expired_notification
+  :feature_category: :authentication_and_authorization
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: cronjob:personal_access_tokens_expiring
   :feature_category: :authentication_and_authorization
   :has_external_dependencies:

app/workers/personal_access_tokens/expired_notification_worker.rb

+# frozen_string_literal: true
+
+module PersonalAccessTokens
+  class ExpiredNotificationWorker # rubocop:disable Scalability/IdempotentWorker
+    include ApplicationWorker
+    include CronjobQueue
+
+    feature_category :authentication_and_authorization
+
+    def perform(*args)
+      return unless Feature.enabled?(:expired_pat_email_notification)
+
+      notification_service = NotificationService.new
+
+      User.with_personal_access_tokens_expired_today.find_each do |user|
+        with_context(user: user) do
+          Gitlab::AppLogger.info "#{self.class}: Notifying User #{user.id} about an expired token"
+
+          notification_service.access_token_expired(user)
+
+          user.personal_access_tokens.without_impersonation.expired_today_and_not_notified.update_all(after_expiry_notification_delivered: true)
+        end
+      end
+    end
+  end
+end

changelogs/unreleased/additional-pat-expiry-notification.yml

+---
+title: Email notification for expired personal access token
+merge_request: 38086
+author:
+type: added

config/initializers/1_settings.rb

@@ -423,6 +423,9 @@ Settings.cron_jobs['admin_email_worker']['job_class'] = 'AdminEmailWorker'
 Settings.cron_jobs['personal_access_tokens_expiring_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['personal_access_tokens_expiring_worker']['cron'] ||= '0 1 * * *'
 Settings.cron_jobs['personal_access_tokens_expiring_worker']['job_class'] = 'PersonalAccessTokens::ExpiringWorker'
+Settings.cron_jobs['personal_access_tokens_expired_notification_worker'] ||= Settingslogic.new({})
+Settings.cron_jobs['personal_access_tokens_expired_notification_worker']['cron'] ||= '0 2 * * *'
+Settings.cron_jobs['personal_access_tokens_expired_notification_worker']['job_class'] = 'PersonalAccessTokens::ExpiredNotificationWorker'
 Settings.cron_jobs['repository_archive_cache_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['repository_archive_cache_worker']['cron'] ||= '0 * * * *'
 Settings.cron_jobs['repository_archive_cache_worker']['job_class'] = 'RepositoryArchiveCacheWorker'

db/migrate/20200729151021_add_after_expiry_notification_delivered_to_personal_access_tokens.rb

+# frozen_string_literal: true
+
+class AddAfterExpiryNotificationDeliveredToPersonalAccessTokens < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def change
+    add_column :personal_access_tokens, :after_expiry_notification_delivered, :boolean, null: false, default: false
+  end
+end

db/schema_migrations/20200729151021

+0a080250afe61007852cb65e8fd6cdccbdad1666abf12b59d46fb55ec0d455cc
\ No newline at end of file

db/structure.sql

@@ -13979,7 +13979,8 @@ CREATE TABLE public.personal_access_tokens (
     impersonation boolean DEFAULT false NOT NULL,
     token_digest character varying,
     expire_notification_delivered boolean DEFAULT false NOT NULL,
-    last_used_at timestamp with time zone
+    last_used_at timestamp with time zone,
+    after_expiry_notification_delivered boolean DEFAULT false NOT NULL
 );
 
 CREATE SEQUENCE public.personal_access_tokens_id_seq

locale/gitlab.pot

@@ -16692,6 +16692,9 @@ msgstr ""
 msgid "One or more of your dependency files are not supported, and the dependency list may be incomplete. Below is a list of supported file types."
 msgstr ""
 
+msgid "One or more of your personal access tokens has expired."
+msgstr ""
+
 msgid "One or more of your personal access tokens will expire in %{days_to_expire} days or less."
 msgstr ""
 
@@ -27566,10 +27569,10 @@ msgstr ""
 msgid "You can apply your Trial to your Personal account or create a New Group."
 msgstr ""
 
-msgid "You can create a new one or check them in your %{pat_link_start}Personal Access Tokens%{pat_link_end} settings"
+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 Personal Access Tokens settings %{pat_link}"
+msgid "You can create a new one or check them in your personal access tokens settings %{pat_link}"
 msgstr ""
 
 msgid "You can create new ones at your %{pat_link_start}Personal Access Tokens%{pat_link_end} settings"
@@ -28130,6 +28133,9 @@ msgstr ""
 msgid "Your password reset token has expired."
 msgstr ""
 
+msgid "Your personal access token has expired"
+msgstr ""
+
 msgid "Your profile"
 msgstr ""
 

spec/mailers/emails/profile_spec.rb

@@ -157,6 +157,56 @@ RSpec.describe Emails::Profile do
     end
   end
 
+  describe 'user personal access token has expired' do
+    let_it_be(:user) { create(:user) }
+
+    context 'when valid' do
+      subject { Notify.access_token_expired_email(user) }
+
+      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 personal access token has expired/
+      end
+
+      it 'mentions the access token has expired' do
+        is_expected.to have_body_text /One or more of your personal access tokens has expired/
+      end
+
+      it 'includes a link to personal access tokens page' do
+        is_expected.to have_body_text /#{profile_personal_access_tokens_path}/
+      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.access_token_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.access_token_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/personal_access_token_spec.rb

@@ -180,6 +180,18 @@ RSpec.describe PersonalAccessToken do
       end
     end
 
+    describe '.expired_today_and_not_notified' do
+      let_it_be(:active) { create(:personal_access_token) }
+      let_it_be(:expired_yesterday) { create(:personal_access_token, expires_at: Date.yesterday) }
+      let_it_be(:revoked_token) { create(:personal_access_token, expires_at: Date.current, revoked: true) }
+      let_it_be(:expired_today) { create(:personal_access_token, expires_at: Date.current) }
+      let_it_be(:expired_today_and_notified) { create(:personal_access_token, expires_at: Date.current, after_expiry_notification_delivered: true) }
+
+      it 'returns tokens that have expired today' do
+        expect(described_class.expired_today_and_not_notified).to contain_exactly(expired_today)
+      end
+    end
+
     describe '.without_impersonation' do
       let_it_be(:impersonation_token) { create(:personal_access_token, :impersonation) }
       let_it_be(:personal_access_token) { create(:personal_access_token) }

spec/models/user_spec.rb

@@ -855,6 +855,24 @@ RSpec.describe User do
       end
     end
 
+    describe '.with_personal_access_tokens_expired_today' do
+      let_it_be(:user1) { create(:user) }
+      let_it_be(:expired_today) { create(:personal_access_token, user: user1, expires_at: Date.current) }
+
+      let_it_be(:user2) { create(:user) }
+      let_it_be(:revoked_token) { create(:personal_access_token, user: user2, expires_at: Date.current, revoked: true) }
+
+      let_it_be(:user3) { create(:user) }
+      let_it_be(:impersonated_token) { create(:personal_access_token, user: user3, expires_at: Date.current, impersonation: true) }
+
+      let_it_be(:user4) { create(:user) }
+      let_it_be(:already_notified) { create(:personal_access_token, user: user4, expires_at: Date.current, after_expiry_notification_delivered: true) }
+
+      it 'returns users whose token has expired today' do
+        expect(described_class.with_personal_access_tokens_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/notification_service_spec.rb

@@ -238,6 +238,26 @@ RSpec.describe NotificationService, :mailer do
         expect { subject }.to have_enqueued_email(user, mail: "access_token_about_to_expire_email")
       end
     end
+
+    describe '#access_token_expired' do
+      let_it_be(:user) { create(:user) }
+
+      subject { notification.access_token_expired(user) }
+
+      it 'sends email to the token owner' do
+        expect { subject }.to have_enqueued_email(user, mail: "access_token_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, mail: "access_token_expired_email")
+        end
+      end
+    end
   end
 
   describe '#unknown_sign_in' do

spec/workers/personal_access_tokens/expired_notification_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe PersonalAccessTokens::ExpiredNotificationWorker, type: :worker do
+  subject(:worker) { described_class.new }
+
+  describe '#perform' do
+    context 'when a token has expired' do
+      let(:expired_today) { create(:personal_access_token, expires_at: Date.current) }
+
+      context 'when feature is enabled' do
+        it 'uses notification service to send email to the user' do
+          expect_next_instance_of(NotificationService) do |notification_service|
+            expect(notification_service).to receive(:access_token_expired).with(expired_today.user)
+          end
+
+          worker.perform
+        end
+
+        it 'updates notified column' do
+          expect { worker.perform }.to change { expired_today.reload.after_expiry_notification_delivered }.from(false).to(true)
+        end
+      end
+
+      context 'when feature is disabled' do
+        before do
+          stub_feature_flags(expired_pat_email_notification: false)
+        end
+
+        it 'does not update notified column' do
+          expect { worker.perform }.not_to change { expired_today.reload.after_expiry_notification_delivered }
+        end
+
+        it 'does not trigger email' do
+          expect { worker.perform }.not_to change { ActionMailer::Base.deliveries.count }
+        end
+      end
+    end
+
+    shared_examples 'expiry notification is not required to be sent for the token' do
+      it do
+        expect_next_instance_of(NotificationService) do |notification_service|
+          expect(notification_service).not_to receive(:access_token_expired).with(token.user)
+        end
+
+        worker.perform
+      end
+    end
+
+    context 'when token has expired in the past' do
+      let(:token) { create(:personal_access_token, expires_at: Date.yesterday) }
+
+      it_behaves_like 'expiry notification is not required to be sent for the token'
+    end
+
+    context 'when token is impersonated' do
+      let(:token) { create(:personal_access_token, expires_at: Date.current, impersonation: true) }
+
+      it_behaves_like 'expiry notification is not required to be sent for the token'
+    end
+
+    context 'when token is revoked' do
+      let(:token) { create(:personal_access_token, expires_at: Date.current, revoked: true) }
+
+      it_behaves_like 'expiry notification is not required to be sent for the token'
+    end
+  end
+end