diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index c327a0bab43abbce99299ed0937060180424b656..b45755788b857132f9da2423a8caffb767a621f4 100644 --- a/app/mailers/emails/profile.rb +++ b/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 diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 488ebd531a86f8c12c842d6d31ddad5b68ce214a..e01cb0530a53687a88471659b39e09c746df04e8 100644 --- a/app/models/personal_access_token.rb +++ b/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) } diff --git a/app/models/user.rb b/app/models/user.rb index b019449468f71a0c394303e2faa1cd232653383e..8c44494db562fbefb5c2637b5f9c52eedb611efa 100644 --- a/app/models/user.rb +++ b/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')) } diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 0e8499e074e0c483bd6c50937f90e228b8d30871..909a0033d12938f331ecd359dff6f0eeab74a9be 100644 --- a/app/services/notification_service.rb +++ b/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) diff --git a/app/views/notify/access_token_about_to_expire_email.html.haml b/app/views/notify/access_token_about_to_expire_email.html.haml index d1923e324f750d69a852f6cbb2f33354e871f7a1..240c7300c7fbdf909b9505eac94d12d85e21088f 100644 --- a/app/views/notify/access_token_about_to_expire_email.html.haml +++ b/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 } diff --git a/app/views/notify/access_token_about_to_expire_email.text.erb b/app/views/notify/access_token_about_to_expire_email.text.erb index 5e6bd68d33fd5b92ce5658793615b8a69faa8340..edcec51aeb4b941ccd0edc1492c5a5cbb0cf5c11 100644 --- a/app/views/notify/access_token_about_to_expire_email.text.erb +++ b/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 } %> diff --git a/app/views/notify/access_token_expired_email.html.haml b/app/views/notify/access_token_expired_email.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..b26431cce91d77a91d6c29ca8bbaca7f4e58b0c5 --- /dev/null +++ b/app/views/notify/access_token_expired_email.html.haml @@ -0,0 +1,7 @@ +%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 } diff --git a/app/views/notify/access_token_expired_email.text.erb b/app/views/notify/access_token_expired_email.text.erb new file mode 100644 index 0000000000000000000000000000000000000000..d44f993d094d1a372f3bfae798412c867fa55acc --- /dev/null +++ b/app/views/notify/access_token_expired_email.text.erb @@ -0,0 +1,5 @@ +<%= _('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 } %> diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index d20519ff774a46e9a1dfae518fa7b450e94e9f55..6c532a24ea29a0bc82dc784c53908ce2a72cf070 100644 --- a/app/workers/all_queues.yml +++ b/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: diff --git a/app/workers/personal_access_tokens/expired_notification_worker.rb b/app/workers/personal_access_tokens/expired_notification_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..c1b1f1a461de01e1413b909a78d0f2981edd6a50 --- /dev/null +++ b/app/workers/personal_access_tokens/expired_notification_worker.rb @@ -0,0 +1,26 @@ +# 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 diff --git a/changelogs/unreleased/additional-pat-expiry-notification.yml b/changelogs/unreleased/additional-pat-expiry-notification.yml new file mode 100644 index 0000000000000000000000000000000000000000..7ba71cd14bb3f6de39b004d13f2e01664ab8f4b2 --- /dev/null +++ b/changelogs/unreleased/additional-pat-expiry-notification.yml @@ -0,0 +1,5 @@ +--- +title: Email notification for expired personal access token +merge_request: 38086 +author: +type: added diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 803a6f0c59d946e6bf0248fb8ed7b2bf34a5864f..e8e2f7edd07c55ee192ed36bf6394bc6c1953f66 100644 --- a/config/initializers/1_settings.rb +++ b/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' diff --git a/db/migrate/20200729151021_add_after_expiry_notification_delivered_to_personal_access_tokens.rb b/db/migrate/20200729151021_add_after_expiry_notification_delivered_to_personal_access_tokens.rb new file mode 100644 index 0000000000000000000000000000000000000000..001436b72853766f78a2b998d835b925f0a3be28 --- /dev/null +++ b/db/migrate/20200729151021_add_after_expiry_notification_delivered_to_personal_access_tokens.rb @@ -0,0 +1,11 @@ +# 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 diff --git a/db/schema_migrations/20200729151021 b/db/schema_migrations/20200729151021 new file mode 100644 index 0000000000000000000000000000000000000000..2ce06409ac410b04e1fa132f9f381760be08a7e7 --- /dev/null +++ b/db/schema_migrations/20200729151021 @@ -0,0 +1 @@ +0a080250afe61007852cb65e8fd6cdccbdad1666abf12b59d46fb55ec0d455cc \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 991112e856494eefa539385f16c58670754a854b..bca8e63c2d903efbbbf0885e3199f8faf8b20fb8 100644 --- a/db/structure.sql +++ b/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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5122495e22a3314e967122232d9f815999c4f3dc..540ffe815d9f80dae2755d2dd4a1f00e0fcbee25 100644 --- a/locale/gitlab.pot +++ b/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 "" diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index ee91df360b66a079352e96a52cc0dbc7950fbebb..fbbdef5feee20de6b2fe45db4f9d1d31a900188c 100644 --- a/spec/mailers/emails/profile_spec.rb +++ b/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' } diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index a39a37b605f141f57a01699c0b8cd08320c79823..9e80d0e088648058c269a5c4bf550ed394b9b193 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/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) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 014f380dcf6140a30f3b6b549a93ecd92266cf50..1a1675a623fbcea673b2fbf324ac4fcdd141c25e 100644 --- a/spec/models/user_spec.rb +++ b/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') } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 4c4ca4db1b4acba7c1b21795b4d5c273ec3864a3..8186bc40bc00dd6201a0c414db0bc7744a60c770 100644 --- a/spec/services/notification_service_spec.rb +++ b/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 diff --git a/spec/workers/personal_access_tokens/expired_notification_worker_spec.rb b/spec/workers/personal_access_tokens/expired_notification_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..676a419553f4e8e45575a4cf29fa9f651aa0c7a4 --- /dev/null +++ b/spec/workers/personal_access_tokens/expired_notification_worker_spec.rb @@ -0,0 +1,69 @@ +# 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