From fa5bf9e9d9e8da3a72532fda6069c0c94432967e Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 11:10:23 +0100 Subject: [PATCH 01/12] Add expire_notification_delivered_at to PAT --- ...otification_delivered_at_to_personal_access_tokens.rb | 9 +++++++++ db/schema.rb | 1 + 2 files changed, 10 insertions(+) create mode 100644 db/migrate/20191014123159_add_expire_notification_delivered_at_to_personal_access_tokens.rb diff --git a/db/migrate/20191014123159_add_expire_notification_delivered_at_to_personal_access_tokens.rb b/db/migrate/20191014123159_add_expire_notification_delivered_at_to_personal_access_tokens.rb new file mode 100644 index 00000000000000..0cdd42f40d125e --- /dev/null +++ b/db/migrate/20191014123159_add_expire_notification_delivered_at_to_personal_access_tokens.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddExpireNotificationDeliveredAtToPersonalAccessTokens < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + add_column :personal_access_tokens, :expire_notification_delivered_at, :datetime_with_timezone + end +end diff --git a/db/schema.rb b/db/schema.rb index bdc088820d4c74..d7716d08a830a0 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.datetime_with_timezone "expire_notification_delivered_at" 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" -- GitLab From b30ee7278e139ab84f8e3811147fdc9b83629c0f Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 11:11:58 +0100 Subject: [PATCH 02/12] Add access_token_about_to_expire_email templates --- .../notify/access_token_about_to_expire_email.html.haml | 7 +++++++ .../notify/access_token_about_to_expire_email.text.erb | 5 +++++ 2 files changed, 12 insertions(+) create mode 100644 app/views/notify/access_token_about_to_expire_email.html.haml create mode 100644 app/views/notify/access_token_about_to_expire_email.text.erb 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 00000000000000..d1923e324f750d --- /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 00000000000000..5e6bd68d33fd5b --- /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 } %> -- GitLab From caa23ceb2375da3adba47659b25fa71bc6568a8b Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 11:13:56 +0100 Subject: [PATCH 03/12] Add mailer for access_token_about_to_expire_email --- app/mailers/emails/profile.rb | 12 ++++++++++ spec/mailers/emails/profile_spec.rb | 34 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index c368e6c8ac7fd5..441439444d543a 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/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index d340f207dc78b2..58c04fb48345ba 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 -- GitLab From 6c9fd8dcb2cbce4bcb83564f522476bb1faf1ed6 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 11:14:50 +0100 Subject: [PATCH 04/12] Add pat about to expire to notification service --- app/services/notification_service.rb | 8 ++++++++ spec/services/notification_service_spec.rb | 12 ++++++++++++ 2 files changed, 20 insertions(+) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 1709474a6c7b53..a75eaa99c23f63 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/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 25900043f11203..c088cb376b33e0 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) } -- GitLab From 38eae6e6508738ddef41ec83872f140f3d19f416 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 11:16:46 +0100 Subject: [PATCH 05/12] Add expiring_and_not_notified scopes to PAT This new scope required a new partial index on personal_access_tokens as well --- app/models/personal_access_token.rb | 1 + spec/models/personal_access_token_spec.rb | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 770fc2b5c8fd17..3a73373ed70f9b 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_at IS NULL 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/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index e0e1101ffc604e..b6619109080da1 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_at: 1.day.ago) } + 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 -- GitLab From f4e95ce6d8bc5cad672a6df67da83679ad2b5078 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 11:21:13 +0100 Subject: [PATCH 06/12] Add worker pat expiring worker This worker should run daily to check which users have personal access tokens that will expire in about a week, it additionally marks the related tokens as delivered --- app/models/concerns/expirable.rb | 4 +- .../personal_access_token_expiring_worker.rb | 18 ++++++++ ...sonal_access_token_expiring_worker_spec.rb | 45 +++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 app/workers/personal_access_token_expiring_worker.rb create mode 100644 spec/workers/personal_access_token_expiring_worker_spec.rb diff --git a/app/models/concerns/expirable.rb b/app/models/concerns/expirable.rb index 1f27448793598a..512822089ba65c 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/workers/personal_access_token_expiring_worker.rb b/app/workers/personal_access_token_expiring_worker.rb new file mode 100644 index 00000000000000..1aee965195bebe --- /dev/null +++ b/app/workers/personal_access_token_expiring_worker.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class PersonalAccessTokenExpiringWorker + 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.joins(:personal_access_tokens).merge(PersonalAccessToken.expiring_and_not_notified(limit_date)).distinct.find_each do |user| + notification_service.access_token_about_to_expire(user) + user.personal_access_tokens.expiring_and_not_notified(limit_date).update_all(expire_notification_delivered_at: Time.zone.now) + end + end +end diff --git a/spec/workers/personal_access_token_expiring_worker_spec.rb b/spec/workers/personal_access_token_expiring_worker_spec.rb new file mode 100644 index 00000000000000..2994715638b04d --- /dev/null +++ b/spec/workers/personal_access_token_expiring_worker_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe PersonalAccessTokenExpiringWorker, 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 'updates the expires_at of the token' do + delivered_at = Time.zone.now.change(usec: 0) + + Timecop.freeze(delivered_at) do + expect { worker.perform }.to change { pat.reload.expire_notification_delivered_at }.from(nil).to(delivered_at) + end + 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_at: 2.days.ago) } + + 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 update the expires_at of the token" do + expect { worker.perform }.not_to change { pat.reload.expire_notification_delivered_at } + end + end + end +end -- GitLab From 0d145f46ca3ac4604e8e59775f4cac5430043469 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 11:28:53 +0100 Subject: [PATCH 07/12] Cronjob configuration for PersonalAccessTokenExpiringWorker --- app/workers/all_queues.yml | 1 + config/gitlab.yml.example | 3 +++ config/initializers/1_settings.rb | 3 +++ 3 files changed, 7 insertions(+) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 710998dcd1a091..99e7359aaf03b2 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_token_expiring - cronjob:pipeline_schedule - cronjob:prune_old_events - cronjob:remove_expired_group_links diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 711fd4ef3c29d9..393a67dc1e112c 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_token_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 9c5a07919b36ef..1a8e8a949f2e41 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_token_expiring_worker'] ||= Settingslogic.new({}) +Settings.cron_jobs['personal_access_token_expiring_worker']['cron'] ||= '0 1 * * *' +Settings.cron_jobs['personal_access_token_expiring_worker']['job_class'] = 'PersonalAccessTokenExpiringWorker' 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' -- GitLab From af46a853a5ff540f7aff4df140a2ea4306571a6f Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 11:49:51 +0100 Subject: [PATCH 08/12] Add locale/gitlab.pot changes --- locale/gitlab.pot | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7081d696d88f47..73e4fb0a37699f 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 "" -- GitLab From 80ea9edcb5e3f576d30aee7753606ac8a58572d0 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 11:52:36 +0100 Subject: [PATCH 09/12] Add changelog entry --- changelogs/unreleased/sa-pat-expiration-notification.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/sa-pat-expiration-notification.yml diff --git a/changelogs/unreleased/sa-pat-expiration-notification.yml b/changelogs/unreleased/sa-pat-expiration-notification.yml new file mode 100644 index 00000000000000..9f4f1cc51ffdd7 --- /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 -- GitLab From d78e48c70ab2dbda306efa0990b38a7ec75fa58f Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Tue, 29 Oct 2019 14:40:28 +0100 Subject: [PATCH 10/12] Move query logic outside PAT expiring worker to user scopes --- app/models/user.rb | 7 +++++ .../personal_access_token_expiring_worker.rb | 2 +- spec/models/user_spec.rb | 29 +++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index d0e758b0055fff..fd02db865828bd 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/workers/personal_access_token_expiring_worker.rb b/app/workers/personal_access_token_expiring_worker.rb index 1aee965195bebe..f2d01338e557ac 100644 --- a/app/workers/personal_access_token_expiring_worker.rb +++ b/app/workers/personal_access_token_expiring_worker.rb @@ -10,7 +10,7 @@ def perform(*args) notification_service = NotificationService.new limit_date = PersonalAccessToken::DAYS_TO_EXPIRE.days.from_now.to_date - User.joins(:personal_access_tokens).merge(PersonalAccessToken.expiring_and_not_notified(limit_date)).distinct.find_each do |user| + User.with_expiring_and_not_notified_personal_access_tokens(limit_date).find_each do |user| notification_service.access_token_about_to_expire(user) user.personal_access_tokens.expiring_and_not_notified(limit_date).update_all(expire_notification_delivered_at: Time.zone.now) end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ee7edb1516c018..eb39ae12189fc8 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_at: 1.day.ago) } + 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 -- GitLab From d0212c366b1325028cc8c918738ae716f2798eae Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Thu, 14 Nov 2019 22:40:29 +0100 Subject: [PATCH 11/12] Use expire_notification_delivered boolean instead of timestamp Instead of using the database for debugging the notification we can just log the action --- app/models/personal_access_token.rb | 2 +- .../personal_access_token_expiring_worker.rb | 5 ++++- ...on_delivered_at_to_personal_access_tokens.rb | 9 --------- ...ation_delivered_to_personal_access_tokens.rb | 17 +++++++++++++++++ db/schema.rb | 2 +- spec/models/personal_access_token_spec.rb | 2 +- spec/models/user_spec.rb | 2 +- ...ersonal_access_token_expiring_worker_spec.rb | 14 +++++--------- 8 files changed, 30 insertions(+), 23 deletions(-) delete mode 100644 db/migrate/20191014123159_add_expire_notification_delivered_at_to_personal_access_tokens.rb create mode 100644 db/migrate/20191014123159_add_expire_notification_delivered_to_personal_access_tokens.rb diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 3a73373ed70f9b..9ccc90fb74d230 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -16,7 +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_at IS NULL AND expires_at >= NOW() AND expires_at <= ?", date]) } + 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/workers/personal_access_token_expiring_worker.rb b/app/workers/personal_access_token_expiring_worker.rb index f2d01338e557ac..dc7d206218d6e7 100644 --- a/app/workers/personal_access_token_expiring_worker.rb +++ b/app/workers/personal_access_token_expiring_worker.rb @@ -12,7 +12,10 @@ def perform(*args) User.with_expiring_and_not_notified_personal_access_tokens(limit_date).find_each do |user| notification_service.access_token_about_to_expire(user) - user.personal_access_tokens.expiring_and_not_notified(limit_date).update_all(expire_notification_delivered_at: Time.zone.now) + + 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 diff --git a/db/migrate/20191014123159_add_expire_notification_delivered_at_to_personal_access_tokens.rb b/db/migrate/20191014123159_add_expire_notification_delivered_at_to_personal_access_tokens.rb deleted file mode 100644 index 0cdd42f40d125e..00000000000000 --- a/db/migrate/20191014123159_add_expire_notification_delivered_at_to_personal_access_tokens.rb +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -class AddExpireNotificationDeliveredAtToPersonalAccessTokens < ActiveRecord::Migration[5.2] - DOWNTIME = false - - def change - add_column :personal_access_tokens, :expire_notification_delivered_at, :datetime_with_timezone - end -end 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 00000000000000..f172d3bdcbdab0 --- /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 d7716d08a830a0..36c9ea89e50cc7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2929,7 +2929,7 @@ t.string "scopes", default: "--- []\n", null: false t.boolean "impersonation", default: false, null: false t.string "token_digest" - t.datetime_with_timezone "expire_notification_delivered_at" + 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/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index b6619109080da1..aaf9ecb80895e3 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/spec/models/personal_access_token_spec.rb @@ -151,7 +151,7 @@ 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_at: 1.day.ago) } + 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index eb39ae12189fc8..4f9dfbd91033a8 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -537,7 +537,7 @@ 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_at: 1.day.ago) } + 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) } diff --git a/spec/workers/personal_access_token_expiring_worker_spec.rb b/spec/workers/personal_access_token_expiring_worker_spec.rb index 2994715638b04d..72329f6177566f 100644 --- a/spec/workers/personal_access_token_expiring_worker_spec.rb +++ b/spec/workers/personal_access_token_expiring_worker_spec.rb @@ -17,17 +17,13 @@ worker.perform end - it 'updates the expires_at of the token' do - delivered_at = Time.zone.now.change(usec: 0) - - Timecop.freeze(delivered_at) do - expect { worker.perform }.to change { pat.reload.expire_notification_delivered_at }.from(nil).to(delivered_at) - 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_at: 2.days.ago) } + 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| @@ -37,8 +33,8 @@ worker.perform end - it "doesn't update the expires_at of the token" do - expect { worker.perform }.not_to change { pat.reload.expire_notification_delivered_at } + 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 -- GitLab From 2eac9ea6607aecedca877a9dea1b3e408ed3bbd4 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Thu, 5 Dec 2019 14:33:29 +0100 Subject: [PATCH 12/12] Move PAT worker to its own namespace This is to follow the same structure that we use for EE --- app/workers/all_queues.yml | 2 +- .../personal_access_token_expiring_worker.rb | 21 ----------------- .../personal_access_tokens/expiring_worker.rb | 23 +++++++++++++++++++ config/gitlab.yml.example | 2 +- config/initializers/1_settings.rb | 6 ++--- .../expiring_worker_spec.rb} | 2 +- 6 files changed, 29 insertions(+), 27 deletions(-) delete mode 100644 app/workers/personal_access_token_expiring_worker.rb create mode 100644 app/workers/personal_access_tokens/expiring_worker.rb rename spec/workers/{personal_access_token_expiring_worker_spec.rb => personal_access_tokens/expiring_worker_spec.rb} (95%) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 99e7359aaf03b2..8f04bd06b9665f 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -16,7 +16,7 @@ - cronjob:pages_domain_verification_cron - cronjob:pages_domain_removal_cron - cronjob:pages_domain_ssl_renewal_cron -- cronjob:personal_access_token_expiring +- cronjob:personal_access_tokens_expiring - cronjob:pipeline_schedule - cronjob:prune_old_events - cronjob:remove_expired_group_links diff --git a/app/workers/personal_access_token_expiring_worker.rb b/app/workers/personal_access_token_expiring_worker.rb deleted file mode 100644 index dc7d206218d6e7..00000000000000 --- a/app/workers/personal_access_token_expiring_worker.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -class PersonalAccessTokenExpiringWorker - 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 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 00000000000000..f28109c45834d1 --- /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/config/gitlab.yml.example b/config/gitlab.yml.example index 393a67dc1e112c..a7b0a0f0b62aec 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -367,7 +367,7 @@ production: &base admin_email_worker: cron: "0 0 * * 0" # Send emails for personal tokens which are about to expire - personal_access_token_expiring_worker: + personal_access_tokens_expiring_worker: cron: "0 1 * * *" # Remove outdated repository archives diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 1a8e8a949f2e41..07c348975b55fd 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -407,9 +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_token_expiring_worker'] ||= Settingslogic.new({}) -Settings.cron_jobs['personal_access_token_expiring_worker']['cron'] ||= '0 1 * * *' -Settings.cron_jobs['personal_access_token_expiring_worker']['job_class'] = 'PersonalAccessTokenExpiringWorker' +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/spec/workers/personal_access_token_expiring_worker_spec.rb b/spec/workers/personal_access_tokens/expiring_worker_spec.rb similarity index 95% rename from spec/workers/personal_access_token_expiring_worker_spec.rb rename to spec/workers/personal_access_tokens/expiring_worker_spec.rb index 72329f6177566f..fcc09e2705c4ea 100644 --- a/spec/workers/personal_access_token_expiring_worker_spec.rb +++ b/spec/workers/personal_access_tokens/expiring_worker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe PersonalAccessTokenExpiringWorker, type: :worker do +RSpec.describe PersonalAccessTokens::ExpiringWorker, type: :worker do subject(:worker) { described_class.new } describe '#perform' do -- GitLab