Merge branch 'sa-pat-expiration-notification' into 'master'

Personal Access Token Expiration Notification

See merge request gitlab-org/gitlab!19296

app/mailers/emails/profile.rb

@@ -32,6 +32,18 @@ module Emails
       mail(to: @user.notification_email, subject: subject("GPG key was added to your account"))
     end
     # rubocop: enable CodeReuse/ActiveRecord
+
+    def access_token_about_to_expire_email(user)
+      return unless user
+
+      @user = user
+      @target_url = profile_personal_access_tokens_url
+      @days_to_expire = PersonalAccessToken::DAYS_TO_EXPIRE
+
+      Gitlab::I18n.with_locale(@user.preferred_language) do
+        mail(to: @user.notification_email, subject: subject(_("Your Personal Access Tokens will expire in %{days_to_expire} days or less") % { days_to_expire: @days_to_expire }))
+      end
+    end
   end
 end

app/models/concerns/expirable.rb

@@ -3,6 +3,8 @@
 module Expirable
   extend ActiveSupport::Concern
+  DAYS_TO_EXPIRE = 7
+
   included do
     scope :expired, -> { where('expires_at <= ?', Time.current) }
   end
@@ -16,6 +18,6 @@ module Expirable
   end
   def expires_soon?
-    expires? && expires_at < 7.days.from_now
+    expires? && expires_at < DAYS_TO_EXPIRE.days.from_now
   end
 end

app/models/personal_access_token.rb

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

app/models/user.rb

@@ -310,6 +310,13 @@ class User < ApplicationRecord
   scope :with_dashboard, -> (dashboard) { where(dashboard: dashboard) }
   scope :with_public_profile, -> { where(private_profile: false) }
+  scope :with_expiring_and_not_notified_personal_access_tokens, ->(at) do
+    where('EXISTS (?)',
+          ::PersonalAccessToken
+            .where('personal_access_tokens.user_id = users.id')
+            .expiring_and_not_notified(at).select(1))
+  end
+
   def self.with_visible_profile(user)
     return with_public_profile if user.nil?

app/services/notification_service.rb

@@ -58,6 +58,14 @@ class NotificationService
     end
   end
+  # Notify the owner of the personal access token, when it is about to expire
+  # And mark the token with about_to_expire_delivered
+  def access_token_about_to_expire(user)
+    return unless user.can?(:receive_notifications)
+
+    mailer.access_token_about_to_expire_email(user).deliver_later
+  end
+
   # When create an issue we should send an email to:
   #
   #  * issue assignee if their notification level is not Disabled

app/views/notify/access_token_about_to_expire_email.html.haml

+%p
+  = _('Hi %{username}!') % { username: sanitize_name(@user.name) }
+%p
+  = _('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 }

app/views/notify/access_token_about_to_expire_email.text.erb

+<%= _('Hi %{username}!') % { username: sanitize_name(@user.name) } %>
+
+<%= _('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 } %>

app/workers/all_queues.yml

@@ -16,6 +16,7 @@
 - cronjob:pages_domain_verification_cron
 - cronjob:pages_domain_removal_cron
 - cronjob:pages_domain_ssl_renewal_cron
+- cronjob:personal_access_tokens_expiring
 - cronjob:pipeline_schedule
 - cronjob:prune_old_events
 - cronjob:remove_expired_group_links

app/workers/personal_access_tokens/expiring_worker.rb

+# frozen_string_literal: true
+
+module PersonalAccessTokens
+  class ExpiringWorker
+    include ApplicationWorker
+    include CronjobQueue
+
+    feature_category :authentication_and_authorization
+
+    def perform(*args)
+      notification_service = NotificationService.new
+      limit_date = PersonalAccessToken::DAYS_TO_EXPIRE.days.from_now.to_date
+
+      User.with_expiring_and_not_notified_personal_access_tokens(limit_date).find_each do |user|
+        notification_service.access_token_about_to_expire(user)
+
+        Rails.logger.info "#{self.class}: Notifying User #{user.id} about expiring tokens" # rubocop:disable Gitlab/RailsLogger
+
+        user.personal_access_tokens.expiring_and_not_notified(limit_date).update_all(expire_notification_delivered: true)
+      end
+    end
+  end
+end

changelogs/unreleased/sa-pat-expiration-notification.yml

+---
+title: Add Personal Access Token expiration reminder
+merge_request: 19296
+author:
+type: added

config/gitlab.yml.example

@@ -366,6 +366,9 @@ production: &base
     # Send admin emails once a week
     admin_email_worker:
       cron: "0 0 * * 0"
+    # Send emails for personal tokens which are about to expire
+    personal_access_tokens_expiring_worker:
+      cron: "0 1 * * *"
     # Remove outdated repository archives
     repository_archive_cache_worker:

config/initializers/1_settings.rb

@@ -407,6 +407,9 @@ Settings.cron_jobs['repository_check_worker']['job_class'] = 'RepositoryCheck::D
 Settings.cron_jobs['admin_email_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['admin_email_worker']['cron'] ||= '0 0 * * 0'
 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['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/20191014123159_add_expire_notification_delivered_to_personal_access_tokens.rb

+# frozen_string_literal: true
+
+class AddExpireNotificationDeliveredToPersonalAccessTokens < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :personal_access_tokens, :expire_notification_delivered, :boolean, default: false
+  end
+
+  def down
+    remove_column :personal_access_tokens, :expire_notification_delivered
+  end
+end

db/schema.rb

@@ -2931,6 +2931,7 @@ ActiveRecord::Schema.define(version: 2019_12_04_093410) do
     t.string "scopes", default: "--- []\n", null: false
     t.boolean "impersonation", default: false, null: false
     t.string "token_digest"
+    t.boolean "expire_notification_delivered", default: false, null: false
     t.index ["token_digest"], name: "index_personal_access_tokens_on_token_digest", unique: true
     t.index ["user_id", "expires_at"], name: "index_pat_on_user_id_and_expires_at"
     t.index ["user_id"], name: "index_personal_access_tokens_on_user_id"

locale/gitlab.pot

@@ -12036,6 +12036,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 will expire in %{days_to_expire} days or less."
+msgstr ""
+
 msgid "Only 'Reporter' roles and above on tiers Premium / Silver and above can see Cycle Analytics."
 msgstr ""
@@ -20164,6 +20167,12 @@ 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"
+msgstr ""
+
+msgid "You can create a new one or check them in your Personal Access Tokens settings %{pat_link}"
+msgstr ""
+
 msgid "You can create files directly in GitLab using one of the following options."
 msgstr ""
@@ -20491,6 +20500,9 @@ msgstr ""
 msgid "Your New Personal Access Token"
 msgstr ""
+msgid "Your Personal Access Tokens will expire in %{days_to_expire} days or less"
+msgstr ""
+
 msgid "Your Primary Email will be used for avatar detection."
 msgstr ""

spec/mailers/emails/profile_spec.rb

@@ -122,4 +122,38 @@ describe Emails::Profile do
       it { expect { Notify.new_gpg_key_email('foo') }.not_to raise_error }
     end
   end
+
+  describe 'user personal access token is about to expire' do
+    let_it_be(:user) { create(:user) }
+
+    subject { Notify.access_token_about_to_expire_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 Tokens will expire in 7 days or less$/i
+    end
+
+    it 'mentions the access tokens will expire' do
+      is_expected.to have_body_text /One or more of your personal access tokens will expire in 7 days or less/
+    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
+
+    context 'with User does not exist' do
+      it { expect { Notify.access_token_about_to_expire_email('foo') }.not_to raise_error }
+    end
+  end
 end

spec/models/personal_access_token_spec.rb

@@ -146,4 +146,25 @@ describe PersonalAccessToken do
       expect(personal_access_token.errors[:scopes].first).to eq "can only contain available scopes"
     end
   end
+
+  describe 'scopes' do
+    describe '.expiring_and_not_notified' do
+      let_it_be(:expired_token) { create(:personal_access_token, expires_at: 2.days.ago) }
+      let_it_be(:revoked_token) { create(:personal_access_token, revoked: true) }
+      let_it_be(:valid_token_and_notified) { create(:personal_access_token, expires_at: 2.days.from_now, expire_notification_delivered: true) }
+      let_it_be(:valid_token) { create(:personal_access_token, expires_at: 2.days.from_now) }
+
+      context 'in one day' do
+        it "doesn't have any tokens" do
+          expect(described_class.expiring_and_not_notified(1.day.from_now)).to be_empty
+        end
+      end
+
+      context 'in three days' do
+        it 'only includes a valid token' do
+          expect(described_class.expiring_and_not_notified(3.days.from_now)).to contain_exactly(valid_token)
+        end
+      end
+    end
+  end
 end

spec/models/user_spec.rb

@@ -529,6 +529,35 @@ describe User, :do_not_mock_admin_mode do
           .to contain_exactly(user)
       end
     end
+
+    describe '.with_expiring_and_not_notified_personal_access_tokens' do
+      let_it_be(:user1) { create(:user) }
+      let_it_be(:user2) { create(:user) }
+      let_it_be(:user3) { create(:user) }
+
+      let_it_be(:expired_token) { create(:personal_access_token, user: user1, expires_at: 2.days.ago) }
+      let_it_be(:revoked_token) { create(:personal_access_token, user: user1, revoked: true) }
+      let_it_be(:valid_token_and_notified) { create(:personal_access_token, user: user2, expires_at: 2.days.from_now, expire_notification_delivered: true) }
+      let_it_be(:valid_token1) { create(:personal_access_token, user: user2, expires_at: 2.days.from_now) }
+      let_it_be(:valid_token2) { create(:personal_access_token, user: user2, expires_at: 2.days.from_now) }
+      let(:users) { described_class.with_expiring_and_not_notified_personal_access_tokens(from) }
+
+      context 'in one day' do
+        let(:from) { 1.day.from_now }
+
+        it "doesn't include an user" do
+          expect(users).to be_empty
+        end
+      end
+
+      context 'in three days' do
+        let(:from) { 3.days.from_now }
+
+        it 'only includes user2' do
+          expect(users).to contain_exactly(user2)
+        end
+      end
+    end
   end
   describe "Respond to" do

spec/services/notification_service_spec.rb

@@ -210,6 +210,18 @@ describe NotificationService, :mailer do
     end
   end
+  describe 'AccessToken' do
+    describe '#access_token_about_to_expire' do
+      let_it_be(:user) { create(:user) }
+
+      it 'sends email to the token owner' do
+        expect(notification.access_token_about_to_expire(user)).to be_truthy
+
+        should_email user
+      end
+    end
+  end
+
   describe 'Notes' do
     context 'issue note' do
       let(:project) { create(:project, :private) }

spec/workers/personal_access_tokens/expiring_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe PersonalAccessTokens::ExpiringWorker, type: :worker do
+  subject(:worker) { described_class.new }
+
+  describe '#perform' do
+    context 'when a token needs to be notified' do
+      let!(:pat) { create(:personal_access_token, expires_at: 5.days.from_now) }
+
+      it 'uses notification service to send the email' do
+        expect_next_instance_of(NotificationService) do |notification_service|
+          expect(notification_service).to receive(:access_token_about_to_expire).with(pat.user)
+        end
+
+        worker.perform
+      end
+
+      it 'marks the notification as delivered' do
+        expect { worker.perform }.to change { pat.reload.expire_notification_delivered }.from(false).to(true)
+      end
+    end
+
+    context 'when no tokens need to be notified' do
+      let!(:pat) { create(:personal_access_token, expires_at: 5.days.from_now, expire_notification_delivered: true) }
+
+      it "doesn't use notification service to send the email" do
+        expect_next_instance_of(NotificationService) do |notification_service|
+          expect(notification_service).not_to receive(:access_token_about_to_expire).with(pat.user)
+        end
+
+        worker.perform
+      end
+
+      it "doesn't change the notificationd delivered of the token" do
+        expect { worker.perform }.not_to change { pat.reload.expire_notification_delivered }
+      end
+    end
+  end
+end