diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index c368e6c8ac7fd5eb66c4c543b5cb7ffaee491406..441439444d543a92f8cbfaa93edb5edb83346ce5 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -32,6 +32,18 @@ def new_gpg_key_email(gpg_key_id) 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 diff --git a/app/models/concerns/expirable.rb b/app/models/concerns/expirable.rb index 1f27448793598aac0ff69895543b2a3bdef75ef0..512822089ba65cf34f713b8152c327de74a2adbf 100644 --- a/app/models/concerns/expirable.rb +++ b/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 @@ def expires? end def expires_soon? - expires? && expires_at < 7.days.from_now + expires? && expires_at < DAYS_TO_EXPIRE.days.from_now end end diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 770fc2b5c8fd17529776df67933f1f43bc61a4e4..9ccc90fb74d2302ff70d460f69a0cd45d9b80d90 100644 --- a/app/models/personal_access_token.rb +++ b/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) } diff --git a/app/models/user.rb b/app/models/user.rb index d0e758b0055fff17675d67c5d4bc8745f088997e..fd02db865828bdaef0c31b09e1e0688dec5a4ee1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -310,6 +310,13 @@ def inactive_message 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? diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 1709474a6c7b5302316622bb7a240116bffee354..a75eaa99c23f632bff6808ed5f52c049597aec5c 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -58,6 +58,14 @@ def new_gpg_key(gpg_key) 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 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 new file mode 100644 index 0000000000000000000000000000000000000000..d1923e324f750d69a852f6cbb2f33354e871f7a1 --- /dev/null +++ b/app/views/notify/access_token_about_to_expire_email.html.haml @@ -0,0 +1,7 @@ +%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 = ''.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: ''.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 new file mode 100644 index 0000000000000000000000000000000000000000..5e6bd68d33fd5b92ce5658793615b8a69faa8340 --- /dev/null +++ b/app/views/notify/access_token_about_to_expire_email.text.erb @@ -0,0 +1,5 @@ +<%= _('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 } %> diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 710998dcd1a0915ec2da8fa3dc425f4fe016f785..8f04bd06b9665f18e7aa5bd8eee67041b6a174dc 100644 --- a/app/workers/all_queues.yml +++ b/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 diff --git a/app/workers/personal_access_tokens/expiring_worker.rb b/app/workers/personal_access_tokens/expiring_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..f28109c45834d1d9e3e2611412fd927614c6dfed --- /dev/null +++ b/app/workers/personal_access_tokens/expiring_worker.rb @@ -0,0 +1,23 @@ +# 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 diff --git a/changelogs/unreleased/sa-pat-expiration-notification.yml b/changelogs/unreleased/sa-pat-expiration-notification.yml new file mode 100644 index 0000000000000000000000000000000000000000..9f4f1cc51ffdd787f23020c802df46615b3be3a4 --- /dev/null +++ b/changelogs/unreleased/sa-pat-expiration-notification.yml @@ -0,0 +1,5 @@ +--- +title: Add Personal Access Token expiration reminder +merge_request: 19296 +author: +type: added diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 711fd4ef3c29d910d7140ffd26941c72403fde13..a7b0a0f0b62aecd23d14171b8a3c41f384274de0 100644 --- a/config/gitlab.yml.example +++ b/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: diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 9c5a07919b36ef9c3f5f6a296b229cc6757e5b5c..07c348975b55fd832fe1d7144341f7199f0ac179 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -407,6 +407,9 @@ 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' diff --git a/db/migrate/20191014123159_add_expire_notification_delivered_to_personal_access_tokens.rb b/db/migrate/20191014123159_add_expire_notification_delivered_to_personal_access_tokens.rb new file mode 100644 index 0000000000000000000000000000000000000000..f172d3bdcbdab0cb1ed48d66f78f54e44eb43cbe --- /dev/null +++ b/db/migrate/20191014123159_add_expire_notification_delivered_to_personal_access_tokens.rb @@ -0,0 +1,17 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index bdc088820d4c74395f1797358c53cddf611e2e32..36c9ea89e50cc7c41364900499e1681cda7b0b30 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2929,6 +2929,7 @@ 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" diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7081d696d88f4715c76fac89685fd12235cd6730..73e4fb0a37699f6dc7b4a7110e5b4f5de94afb8e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -12024,6 +12024,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 "" @@ -20152,6 +20155,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 "" @@ -20479,6 +20488,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 "" diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index d340f207dc78b2bad49c063d3bae2252eae83f9a..58c04fb48345ba9483de3e0b9f0f04da09a5bf0b 100644 --- a/spec/mailers/emails/profile_spec.rb +++ b/spec/mailers/emails/profile_spec.rb @@ -122,4 +122,38 @@ 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 diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index e0e1101ffc604ed4d6003883a4cf63b4e4112d5b..aaf9ecb80895e3de6357e6aa90f5f9eb889ec3d3 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/spec/models/personal_access_token_spec.rb @@ -146,4 +146,25 @@ 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ee7edb1516c018ead7290a1180f1ecd27b5c750f..4f9dfbd91033a83a521c8c2a612b1469d09411f3 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -529,6 +529,35 @@ .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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 25900043f11203dcebcffc3baf6382a0a19dc725..c088cb376b33e039fcc4f66339ddcb89ce2b72db 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -210,6 +210,18 @@ 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) } diff --git a/spec/workers/personal_access_tokens/expiring_worker_spec.rb b/spec/workers/personal_access_tokens/expiring_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..fcc09e2705c4ea36e0b8bf171dbc010844fdd112 --- /dev/null +++ b/spec/workers/personal_access_tokens/expiring_worker_spec.rb @@ -0,0 +1,41 @@ +# 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