From c79faeffc650a204bba8a560b941b92326c87575 Mon Sep 17 00:00:00 2001 From: agius Date: Fri, 23 Aug 2024 12:12:39 -0700 Subject: [PATCH 1/5] Add db columns for 30d and 60d PAT expiry notifications Part 1 of a multi-commit MR. This adds db columns to the personal_access_tokens table to enable sending 30 and 60 day notifications. This also migrates the existing boolean expiry notification column for to match the new format, and backfills data as necessary. The ExpiringWorker is also set to dual-write to both fields for forward compatibility, and backwards compatibility in the case of a rollback. Changelog: added --- app/models/personal_access_token.rb | 6 ++- .../personal_access_tokens/expiring_worker.rb | 10 +++- ...fication_cols_to_personal_access_tokens.rb | 43 +++++++++++++++ ...0619_add_indices_for_pat_expiry_columns.rb | 53 +++++++++++++++++++ ...ess_tokens_seven_days_notification_sent.rb | 43 +++++++++++++++ db/schema_migrations/20240822195511 | 1 + db/schema_migrations/20240822200619 | 1 + db/schema_migrations/20240822201906 | 1 + db/structure.sql | 9 ++++ ...okens_seven_days_notification_sent_spec.rb | 51 ++++++++++++++++++ spec/models/personal_access_token_spec.rb | 2 + .../expiring_worker_spec.rb | 9 +++- 12 files changed, 223 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb create mode 100644 db/migrate/20240822200619_add_indices_for_pat_expiry_columns.rb create mode 100644 db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent.rb create mode 100644 db/schema_migrations/20240822195511 create mode 100644 db/schema_migrations/20240822200619 create mode 100644 db/schema_migrations/20240822201906 create mode 100644 spec/migrations/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent_spec.rb diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 0b4336a6113798..1da85489ea40f8 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -27,7 +27,7 @@ class PersonalAccessToken < ApplicationRecord before_save :ensure_token scope :active, -> { not_revoked.not_expired } - scope :expiring_and_not_notified, ->(date) { where(["revoked = false AND expire_notification_delivered = false AND expires_at >= CURRENT_DATE AND expires_at <= ?", date]) } + scope :expiring_and_not_notified, ->(date) { where(revoked: false, expire_notification_delivered: false, seven_days_notification_sent_at: nil).where('expires_at >= CURRENT_DATE').where('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 :last_used_before_or_unused, ->(date) { where("personal_access_tokens.created_at < :date AND (last_used_at < :date OR last_used_at IS NULL)", date: date) } @@ -44,7 +44,9 @@ class PersonalAccessToken < ApplicationRecord scope :owner_is_human, -> { includes(:user).references(:user).merge(User.human) } scope :last_used_before, ->(date) { where("last_used_at <= ?", date) } scope :last_used_after, ->(date) { where("last_used_at >= ?", date) } - scope :expiring_and_not_notified_without_impersonation, -> { where(["(revoked = false AND expire_notification_delivered = false AND expires_at >= CURRENT_DATE AND expires_at <= :date) and impersonation = false", { date: DAYS_TO_EXPIRE.days.from_now.to_date }]) } + scope :expiring_and_not_notified_without_impersonation, -> { + expiring_and_not_notified(DAYS_TO_EXPIRE.days.from_now.to_date).without_impersonation + } validates :scopes, presence: true validates :expires_at, presence: true, on: :create, unless: :allow_expires_at_to_be_empty? diff --git a/app/workers/personal_access_tokens/expiring_worker.rb b/app/workers/personal_access_tokens/expiring_worker.rb index 492fdf9776cdd3..0bbe85bf6875d1 100644 --- a/app/workers/personal_access_tokens/expiring_worker.rb +++ b/app/workers/personal_access_tokens/expiring_worker.rb @@ -52,7 +52,10 @@ def process_user_tokens deliver_user_notifications(user, token_names) - expiring_user_tokens.update_all(expire_notification_delivered: true) + expiring_user_tokens.update_all( + expire_notification_delivered: true, + seven_days_notification_sent_at: Time.current + ) end end end @@ -102,7 +105,10 @@ def process_project_bot_tokens tokens_with_delivered_notifications = tokens .where.not(user_id: project_bot_ids_without_resource | project_bot_ids_with_failed_delivery) - tokens_with_delivered_notifications.update_all(expire_notification_delivered: true) + tokens_with_delivered_notifications.update_all( + expire_notification_delivered: true, + seven_days_notification_sent_at: Time.current + ) notifications_delivered += tokens_with_delivered_notifications.count end diff --git a/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb b/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb new file mode 100644 index 00000000000000..6946f182bd4107 --- /dev/null +++ b/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddNotificationColsToPersonalAccessTokens < Gitlab::Database::Migration[2.2] + # When using the methods "add_concurrent_index" or "remove_concurrent_index" + # you must disable the use of transactions + # as these methods can not run in an existing transaction. + # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure + # that either of them is the _only_ method called in the migration, + # any other changes should go in a separate migration. + # This ensures that upon failure _only_ the index creation or removing fails + # and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + disable_ddl_transaction! + # + # Configure the `gitlab_schema` to perform data manipulation (DML). + # Visit: https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html + # restrict_gitlab_migration gitlab_schema: :gitlab_main + + # Add dependent 'batched_background_migrations.queued_migration_version' values. + # DEPENDENT_BATCHED_BACKGROUND_MIGRATIONS = [] + milestone '17.4' + + def up + with_lock_retries do + add_column :personal_access_tokens, :seven_days_notification_sent_at, :datetime_with_timezone + add_column :personal_access_tokens, :thirty_days_notification_sent_at, :datetime_with_timezone + add_column :personal_access_tokens, :sixty_days_notification_sent_at, :datetime_with_timezone + end + end + + def down + with_lock_retries do + remove_column :personal_access_tokens, :seven_days_notification_sent_at + remove_column :personal_access_tokens, :thirty_days_notification_sent_at + remove_column :personal_access_tokens, :sixty_days_notification_sent_at + end + end +end diff --git a/db/migrate/20240822200619_add_indices_for_pat_expiry_columns.rb b/db/migrate/20240822200619_add_indices_for_pat_expiry_columns.rb new file mode 100644 index 00000000000000..8d3d607d875fd3 --- /dev/null +++ b/db/migrate/20240822200619_add_indices_for_pat_expiry_columns.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIndicesForPatExpiryColumns < Gitlab::Database::Migration[2.2] + # When using the methods "add_concurrent_index" or "remove_concurrent_index" + # you must disable the use of transactions + # as these methods can not run in an existing transaction. + # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure + # that either of them is the _only_ method called in the migration, + # any other changes should go in a separate migration. + # This ensures that upon failure _only_ the index creation or removing fails + # and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + disable_ddl_transaction! + # + # Configure the `gitlab_schema` to perform data manipulation (DML). + # Visit: https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html + # restrict_gitlab_migration gitlab_schema: :gitlab_main + + # Add dependent 'batched_background_migrations.queued_migration_version' values. + # DEPENDENT_BATCHED_BACKGROUND_MIGRATIONS = [] + milestone '17.4' + + def up + add_concurrent_index :personal_access_tokens, + [:id, :expires_at], + where: 'impersonation = false AND revoked = false AND seven_days_notification_sent_at IS NULL', + name: 'index_pats_on_expiring_at_seven_days_notification_sent_at' + + add_concurrent_index :personal_access_tokens, + [:id, :expires_at], + where: 'impersonation = false AND revoked = false AND thirty_days_notification_sent_at IS NULL', + name: 'index_pats_on_expiring_at_thirty_days_notification_sent_at' + + add_concurrent_index :personal_access_tokens, + [:id, :expires_at], + where: 'impersonation = false AND revoked = false AND sixty_days_notification_sent_at IS NULL', + name: 'index_pats_on_expiring_at_sixty_days_notification_sent_at' + end + + def down + remove_concurrent_index_by_name :personal_access_tokens, + name: 'index_pats_on_expiring_at_seven_days_notification_sent_at' + remove_concurrent_index_by_name :personal_access_tokens, + name: 'index_pats_on_expiring_at_thirty_days_notification_sent_at' + remove_concurrent_index_by_name :personal_access_tokens, + name: 'index_pats_on_expiring_at_sixty_days_notification_sent_at' + end +end diff --git a/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent.rb b/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent.rb new file mode 100644 index 00000000000000..9f1f86469bd315 --- /dev/null +++ b/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class BackfillPersonalAccessTokensSevenDaysNotificationSent < Gitlab::Database::Migration[2.2] + # When using the methods "add_concurrent_index" or "remove_concurrent_index" + # you must disable the use of transactions + # as these methods can not run in an existing transaction. + # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure + # that either of them is the _only_ method called in the migration, + # any other changes should go in a separate migration. + # This ensures that upon failure _only_ the index creation or removing fails + # and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + disable_ddl_transaction! + # + # Configure the `gitlab_schema` to perform data manipulation (DML). + # Visit: https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html + restrict_gitlab_migration gitlab_schema: :gitlab_main + + # Add dependent 'batched_background_migrations.queued_migration_version' values. + # DEPENDENT_BATCHED_BACKGROUND_MIGRATIONS = [] + milestone '17.4' + + include ::Gitlab::Database::DynamicModelHelpers + + def up + scope = ->(table) { table.where(expire_notification_delivered: true).where.not(expires_at: nil) } + each_batch_range('personal_access_tokens', scope: scope, of: BATCH_SIZE) do |min, max| + execute <<~SQL + UPDATE personal_access_tokens + SET seven_days_notification_sent_at = (expires_at - interval '7 days') + WHERE expire_notification_delivered = true + AND seven_days_notification_sent_at IS NULL + AND expires_at IS NOT NULL + AND id BETWEEN #{min} AND #{max} + SQL + end + end +end diff --git a/db/schema_migrations/20240822195511 b/db/schema_migrations/20240822195511 new file mode 100644 index 00000000000000..45c3c17bdf0ac2 --- /dev/null +++ b/db/schema_migrations/20240822195511 @@ -0,0 +1 @@ +234b63a3ca62c94f3d2a7b56d466528151d82c36dbdb11614fc035c7ac23d434 \ No newline at end of file diff --git a/db/schema_migrations/20240822200619 b/db/schema_migrations/20240822200619 new file mode 100644 index 00000000000000..9495d9228b5ded --- /dev/null +++ b/db/schema_migrations/20240822200619 @@ -0,0 +1 @@ +d7ef657ff096c75aa7113e0e2562d9432fddfa4e1bb9a3a148fb29aa2926f006 \ No newline at end of file diff --git a/db/schema_migrations/20240822201906 b/db/schema_migrations/20240822201906 new file mode 100644 index 00000000000000..ed93006976b7bd --- /dev/null +++ b/db/schema_migrations/20240822201906 @@ -0,0 +1 @@ +f70c475c2db47712af045b59f36d9f0971f18a18c9e0f8060b86b35969dc248c \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index a4a019b08d7b64..b4372ab7012756 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -15383,6 +15383,9 @@ CREATE TABLE personal_access_tokens ( previous_personal_access_token_id bigint, advanced_scopes text, organization_id bigint DEFAULT 1 NOT NULL, + seven_days_notification_sent_at timestamp with time zone, + thirty_days_notification_sent_at timestamp with time zone, + sixty_days_notification_sent_at timestamp with time zone, CONSTRAINT check_aa95773861 CHECK ((char_length(advanced_scopes) <= 4096)) ); @@ -29511,6 +29514,12 @@ CREATE INDEX index_path_locks_on_project_id ON path_locks USING btree (project_i CREATE INDEX index_path_locks_on_user_id ON path_locks USING btree (user_id); +CREATE INDEX index_pats_on_expiring_at_seven_days_notification_sent_at ON personal_access_tokens USING btree (id, expires_at) WHERE ((impersonation = false) AND (revoked = false) AND (seven_days_notification_sent_at IS NULL)); + +CREATE INDEX index_pats_on_expiring_at_sixty_days_notification_sent_at ON personal_access_tokens USING btree (id, expires_at) WHERE ((impersonation = false) AND (revoked = false) AND (sixty_days_notification_sent_at IS NULL)); + +CREATE INDEX index_pats_on_expiring_at_thirty_days_notification_sent_at ON personal_access_tokens USING btree (id, expires_at) WHERE ((impersonation = false) AND (revoked = false) AND (thirty_days_notification_sent_at IS NULL)); + CREATE INDEX index_pe_approval_rules_on_required_approvals_and_created_at ON protected_environment_approval_rules USING btree (required_approvals, created_at); CREATE INDEX index_personal_access_tokens_on_id_and_created_at ON personal_access_tokens USING btree (id, created_at); diff --git a/spec/migrations/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent_spec.rb b/spec/migrations/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent_spec.rb new file mode 100644 index 00000000000000..1fa23b8e64a8a9 --- /dev/null +++ b/spec/migrations/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe BackfillPersonalAccessTokensSevenDaysNotificationSent, migration: :gitlab_main, feature_category: :system_access do + let(:pats_table) { table(:personal_access_tokens) } + let(:users) { table(:users) } + let(:organizations) { table(:organizations) } + + let!(:organization) { organizations.create!(name: 'default org', path: 'dflt') } + let!(:user) { users.create!(email: 'capybara@example.com', encrypted_password: 'abc123', projects_limit: 2) } + let!(:pat_to_update) do + pats_table.create!(name: 'notified token', expire_notification_delivered: true, expires_at: Date.current, + user_id: user.id, organization_id: organization.id) + end + + let!(:seven_days_notified) do + pats_table.create!(name: 'seven days token', expire_notification_delivered: true, expires_at: Date.current, + seven_days_notification_sent_at: Time.current - 1.day, user_id: user.id, organization_id: organization.id) + end + + let!(:not_notified_pat) do + pats_table.create!(name: 'not notified token', expires_at: Date.current + 1, user_id: user.id, + organization_id: organization.id) + end + + let!(:no_expiry_pat) do + pats_table.create!(name: 'no expiry token', expire_notification_delivered: true, user_id: user.id, + organization_id: organization.id) + end + + describe '#up' do + it 'backfills seven_days_notification_sent_at field', :freeze_time do + expect(pat_to_update.reload.seven_days_notification_sent_at).to be_nil + expect(seven_days_notified.reload.seven_days_notification_sent_at).to eq(Time.current - 1.day) + expect(not_notified_pat.reload.seven_days_notification_sent_at).to be_nil + expect(no_expiry_pat.reload.seven_days_notification_sent_at).to be_nil + + migrate! + + # db updates do not use the same timezone as Rails; default to UTC + db_updated_time = Time.utc(Time.current.year, Time.current.month, Time.current.day) - 7.days + expect(pat_to_update.reload.seven_days_notification_sent_at).to eq(db_updated_time) + + expect(seven_days_notified.reload.seven_days_notification_sent_at).to eq(Time.current - 1.day) + expect(not_notified_pat.reload.seven_days_notification_sent_at).to be_nil + expect(no_expiry_pat.reload.seven_days_notification_sent_at).to be_nil + end + end +end diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index 2f8e82117cd191..d81ba9a0ed5b2e 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/spec/models/personal_access_token_spec.rb @@ -334,6 +334,7 @@ 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_and_7d_notified) { create(:personal_access_token, expires_at: 2.days.from_now, seven_days_notification_sent_at: Time.current) } let_it_be(:valid_token) { create(:personal_access_token, expires_at: 2.days.from_now) } let_it_be(:long_expiry_token) { create(:personal_access_token, expires_at: described_class::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now) } @@ -354,6 +355,7 @@ 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_and_7d_notified) { create(:personal_access_token, expires_at: 2.days.from_now, seven_days_notification_sent_at: Time.current) } let_it_be(:valid_token) { create(:personal_access_token, expires_at: 2.days.from_now, impersonation: false) } let_it_be(:long_expiry_token) { create(:personal_access_token, expires_at: described_class::MAX_PERSONAL_ACCESS_TOKEN_LIFETIME_IN_DAYS.days.from_now) } diff --git a/spec/workers/personal_access_tokens/expiring_worker_spec.rb b/spec/workers/personal_access_tokens/expiring_worker_spec.rb index 3065d7c12f4cc8..95026e53bd3a7f 100644 --- a/spec/workers/personal_access_tokens/expiring_worker_spec.rb +++ b/spec/workers/personal_access_tokens/expiring_worker_spec.rb @@ -15,8 +15,9 @@ worker.perform end - it 'marks the notification as delivered' do + it 'marks the notification as delivered', :freeze_time do expect { worker.perform }.to change { expiring_token.reload.expire_notification_delivered }.from(false).to(true) + .and change { expiring_token.reload.seven_days_notification_sent_at }.from(nil).to(Time.current) end end @@ -37,8 +38,9 @@ worker.perform end - it 'marks the notification as delivered' do + it 'marks the notification as delivered', :freeze_time do expect { worker.perform }.to change { expiring_token.reload.expire_notification_delivered }.from(false).to(true) + .and change { expiring_token.reload.seven_days_notification_sent_at }.from(nil).to(Time.current) end it 'avoids N+1 queries', :use_sql_query_cache do @@ -64,6 +66,9 @@ context 'when no tokens need to be notified' do let_it_be(:pat) { create(:personal_access_token, expires_at: 5.days.from_now, expire_notification_delivered: true) } + let_it_be(:seven_days_pat) do + create(:personal_access_token, expires_at: 5.days.from_now, seven_days_notification_sent_at: Time.current - 2.days) + end it "doesn't call notification services" do expect(worker).not_to receive(:notification_service) -- GitLab From efd5334b63fe453a5912a917d55505625e1f3874 Mon Sep 17 00:00:00 2001 From: agius Date: Fri, 23 Aug 2024 16:10:54 -0700 Subject: [PATCH 2/5] Add workers , FF for 30d and 60d notifications Refactors the existing ExpiringWorker into ExpiringPersonalTokenWorker and ExpiringBotTokenWorker. Retains the old ExpiringWorker with minimal changes so that we can deploy or roll back with a feature flag. The new workers send notifications at 60 days and 30 days in addition to the current 7. Webhooks for project bot tokens are only run at 7 days, following the current behavior. Adds feature flag for switching between 7+30+60d behavior and previous 7-day-only behavior. --- app/mailers/emails/profile.rb | 11 +- app/models/personal_access_token.rb | 23 ++ app/services/notification_service.rb | 9 +- .../expiring_bot_token_worker.rb | 130 ++++++++ .../expiring_personal_token_worker.rb | 92 ++++++ .../personal_access_tokens/expiring_worker.rb | 3 + .../expiring_pats_30d_60d_notifications.yml | 9 + spec/mailers/emails/profile_spec.rb | 47 +++ spec/models/personal_access_token_spec.rb | 16 + .../expiring_bot_token_worker_spec.rb | 279 ++++++++++++++++++ .../expiring_personal_token_worker_spec.rb | 180 +++++++++++ .../expiring_worker_spec.rb | 16 + 12 files changed, 807 insertions(+), 8 deletions(-) create mode 100644 app/workers/personal_access_tokens/expiring_bot_token_worker.rb create mode 100644 app/workers/personal_access_tokens/expiring_personal_token_worker.rb create mode 100644 config/feature_flags/gitlab_com_derisk/expiring_pats_30d_60d_notifications.yml create mode 100644 spec/workers/personal_access_tokens/expiring_bot_token_worker_spec.rb create mode 100644 spec/workers/personal_access_tokens/expiring_personal_token_worker_spec.rb diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index 8a0d1ea2638b6f..18c68e73786846 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -61,10 +61,11 @@ def new_gpg_key_email(gpg_key_id) # rubocop: enable CodeReuse/ActiveRecord # resource owners are sent mail about expiring access tokens which belong to a bot user - def bot_resource_access_token_about_to_expire_email(recipient, resource, token_name) + def bot_resource_access_token_about_to_expire_email(recipient, resource, token_name, params = {}) + params = params.with_indifferent_access @user = recipient @token_name = token_name - @days_to_expire = PersonalAccessToken::DAYS_TO_EXPIRE + @days_to_expire = params[:days_to_expire] || PersonalAccessToken::DAYS_TO_EXPIRE @resource = resource if resource.is_a?(Group) @target_url = group_settings_access_tokens_url(resource) @@ -95,13 +96,15 @@ def access_token_created_email(user, token_name) email_with_layout(to: @user.notification_email_or_default, subject: subject(_("A new personal access token has been created"))) end - def access_token_about_to_expire_email(user, token_names) + def access_token_about_to_expire_email(user, token_names, params = {}) return unless user + params = params.with_indifferent_access + @user = user @token_names = token_names @target_url = user_settings_personal_access_tokens_url - @days_to_expire = PersonalAccessToken::DAYS_TO_EXPIRE + @days_to_expire = params[:days_to_expire] || PersonalAccessToken::DAYS_TO_EXPIRE email_with_layout(to: @user.notification_email_or_default, subject: subject(_("Your personal access tokens will expire in %{days_to_expire} days or less") % { days_to_expire: @days_to_expire })) end diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 1da85489ea40f8..a6f7200a42e983 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -9,6 +9,12 @@ class PersonalAccessToken < ApplicationRecord include Gitlab::SQL::Pattern extend ::Gitlab::Utils::Override + NOTIFICATION_INTERVALS = { + seven_days: 0..7, + thirty_days: 8..30, + sixty_days: 31..60 + }.freeze + add_authentication_token_field :token, digest: true, format_with_prefix: :prefix_from_application_current_settings @@ -79,6 +85,23 @@ def self.search(query) fuzzy_search(query, [:name]) end + def self.notification_interval(interval) + NOTIFICATION_INTERVALS.fetch(interval).max + end + + def self.scope_for_notification_interval(interval) + date_range = NOTIFICATION_INTERVALS.fetch(interval) + min_expiry_date = Date.current + date_range.min + max_expiry_date = Date.current + date_range.max + interval_attr = "#{interval}_notification_sent_at" + + without_impersonation + .not_revoked + .where(interval_attr => nil) + .where(expire_notification_delivered: false) # legacy version of seven_days_notification_sent_at + .where('expires_at BETWEEN ? AND ?', min_expiry_date, max_expiry_date) + end + def hook_attrs Gitlab::HookData::ResourceAccessTokenBuilder.new(self).build end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 4d3bf1efedca6f..1ca6ef1361fc32 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -75,7 +75,7 @@ def new_gpg_key(gpg_key) end end - def bot_resource_access_token_about_to_expire(bot_user, token_name) + def bot_resource_access_token_about_to_expire(bot_user, token_name, params = {}) recipients = bot_user.resource_bot_owners_and_maintainers.select { |user| user.can?(:receive_notifications) } resource = bot_user.resource_bot_resource @@ -85,7 +85,8 @@ def bot_resource_access_token_about_to_expire(bot_user, token_name) mailer.bot_resource_access_token_about_to_expire_email( recipient, resource, - token_name + token_name, + params ).deliver_later end end @@ -99,12 +100,12 @@ def access_token_created(user, token_name) # 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, token_names) + def access_token_about_to_expire(user, token_names, params = {}) return unless user.can?(:receive_notifications) log_info("Notifying User about expiring tokens", user) - mailer.access_token_about_to_expire_email(user, token_names).deliver_later + mailer.access_token_about_to_expire_email(user, token_names, params).deliver_later end # Notify the user when at least one of their personal access tokens has expired today diff --git a/app/workers/personal_access_tokens/expiring_bot_token_worker.rb b/app/workers/personal_access_tokens/expiring_bot_token_worker.rb new file mode 100644 index 00000000000000..75b62cdcece368 --- /dev/null +++ b/app/workers/personal_access_tokens/expiring_bot_token_worker.rb @@ -0,0 +1,130 @@ +# frozen_string_literal: true + +module PersonalAccessTokens # rubocop:disable Gitlab/BoundedContexts -- continuing pattern from legacy ExpiringWorker + class ExpiringBotTokenWorker # rubocop:disable Scalability/IdempotentWorker -- continuing pattern from legacy ExpiringWorker + include ApplicationWorker + include Gitlab::Utils::StrongMemoize + + data_consistency :always # rubocop:disable SidekiqLoadBalancing/WorkerDataConsistency -- continuing pattern from legacy ExpiringWorker + + include CronjobQueue + + feature_category :system_access + + MAX_TOKENS = 100 + + # For the worker is timing out with a bigger batch size + # https://gitlab.com/gitlab-org/gitlab/-/issues/432518 + BATCH_SIZE = 100 + + def perform + # if feature is not enabled, use classic ExpiringWorker instead of this worker + return unless Feature.enabled?(:expiring_pats_30d_60d_notifications, :instance) + + PersonalAccessToken::NOTIFICATION_INTERVALS.each_key do |interval| + process_project_bot_tokens(interval) + end + end + + private + + # rubocop: disable CodeReuse/ActiveRecord -- We need to specify batch size to avoid timing out of worker + def process_project_bot_tokens(interval = :seven_days) + notifications_delivered = 0 + project_bot_ids_without_resource = [] + project_bot_ids_with_failed_delivery = [] + + loop do + tokens = base_scope(interval) + .where.not(user_id: project_bot_ids_without_resource | project_bot_ids_with_failed_delivery) + .limit(BATCH_SIZE) + + break if tokens.empty? + + bot_users = User.id_in(tokens.distinct(:user_id).pluck(:user_id)).with_personal_access_tokens_and_resources + + bot_users.each do |project_bot| + case process_bot_user(project_bot, interval) + when :no_resource + project_bot_ids_without_resource << project_bot.id + when :failed_delivery + project_bot_ids_with_failed_delivery << project_bot.id + end + end + + tokens_with_delivered_notifications = + tokens + .where.not(user_id: project_bot_ids_without_resource | project_bot_ids_with_failed_delivery) + + updated_attrs = { "#{interval}_notification_sent_at" => Time.current } + updated_attrs[:expire_notification_delivered] = true if interval == :seven_days + tokens_with_delivered_notifications.update_all(updated_attrs) + + notifications_delivered += tokens_with_delivered_notifications.count + end + + log_extra_metadata_on_done( + :total_notification_delivered_for_resource_access_tokens, notifications_delivered) + log_extra_metadata_on_done( + :total_resource_bot_without_membership, project_bot_ids_without_resource.count) + log_extra_metadata_on_done( + :total_failed_notifications_for_resource_bots, project_bot_ids_with_failed_delivery.count) + end + + def process_bot_user(project_bot, interval) + return :no_resource if project_bot.resource_bot_resource.nil? + + with_context(user: project_bot) do + # project bot does not have more than 1 token + expiring_user_token = project_bot.personal_access_tokens.first + + execute_web_hooks(project_bot, expiring_user_token) if interval == :seven_days + + interval_days = PersonalAccessToken.notification_interval(interval) + deliver_bot_notifications(project_bot, expiring_user_token.name, days_to_expire: interval_days) + end + + :success + rescue StandardError => e + log_exception(e, project_bot) + + :failed_delivery + end + + def deliver_bot_notifications(bot_user, token_name, params = {}) + notification_service.bot_resource_access_token_about_to_expire(bot_user, token_name, params) + end + + def log_exception(ex, user) + Gitlab::AppLogger.error( + message: 'Failed to send notification about expiring resource access tokens', + 'exception.message': ex.message, + 'exception.class': ex.class.name, + class: self.class, + user_id: user.id + ) + end + + def execute_web_hooks(bot_user, token) + resource = bot_user.resource_bot_resource + + return unless resource + return if resource.is_a?(Project) && !resource.has_active_hooks?(:resource_access_token_hooks) + + hook_data = Gitlab::DataBuilder::ResourceAccessToken.build(token, :expiring, resource) + resource.execute_hooks(hook_data, :resource_access_token_hooks) + end + + def notification_service + NotificationService.new + end + strong_memoize_attr :notification_service + + def base_scope(interval) + PersonalAccessToken + .scope_for_notification_interval(interval) + .project_access_token + end + # rubocop: enable CodeReuse/ActiveRecord + end +end diff --git a/app/workers/personal_access_tokens/expiring_personal_token_worker.rb b/app/workers/personal_access_tokens/expiring_personal_token_worker.rb new file mode 100644 index 00000000000000..8b1429a15e70bc --- /dev/null +++ b/app/workers/personal_access_tokens/expiring_personal_token_worker.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +module PersonalAccessTokens # rubocop:disable Gitlab/BoundedContexts -- continuing pattern from legacy ExpiringWorker + class ExpiringPersonalTokenWorker # rubocop:disable Scalability/IdempotentWorker -- copied from legacy ExpiringWorker + include ApplicationWorker + include Gitlab::Utils::StrongMemoize + + data_consistency :always # rubocop:disable SidekiqLoadBalancing/WorkerDataConsistency -- continuing pattern from legacy ExpiringWorker + + include CronjobQueue + + feature_category :system_access + + MAX_TOKENS = 100 + + # For the worker is timing out with a bigger batch size + # https://gitlab.com/gitlab-org/gitlab/-/issues/432518 + BATCH_SIZE = 100 + + def perform + # if feature is not enabled, use classic ExpiringWorker instead of this worker + return unless Feature.enabled?(:expiring_pats_30d_60d_notifications, :instance) + + PersonalAccessToken::NOTIFICATION_INTERVALS.each_key do |interval| + process_user_tokens(interval) + end + end + + private + + # rubocop: disable CodeReuse/ActiveRecord -- We need to specify batch size to avoid timing out of worker + def process_user_tokens(interval = :seven_days) + loop do + user_ids = base_scope(interval).distinct(:user_id).limit(BATCH_SIZE).pluck(:user_id) + + break if user_ids.empty? + + users = User.id_in(user_ids).with_personal_access_tokens_expiring_soon + + users.each do |user| + with_context(user: user) do + process_user(user, interval) + end + end + end + end + + def process_user(user, interval) + expiring_user_tokens = base_scope(interval).where(user_id: user.id) + + # We never materialise the token instances. We need the names to mention them in the + # email. Later we trigger an update query on the entire relation, not on individual instances. + token_names = expiring_user_tokens.limit(MAX_TOKENS).pluck(:name) + # We're limiting to 100 tokens so we avoid loading too many tokens into memory. + # At the time of writing this would only affect 69 users on GitLab.com + + interval_days = PersonalAccessToken.notification_interval(interval) + deliver_user_notifications(user, token_names, days_to_expire: interval_days) + + notification_attribute = "#{interval}_notification_sent_at" + updated_attrs = { notification_attribute => Time.current } + updated_attrs[:expire_notification_delivered] = true if interval == :seven_days + expiring_user_tokens.update_all(updated_attrs) + end + + def deliver_user_notifications(user, token_names, params = {}) + notification_service.access_token_about_to_expire(user, token_names, params) + end + + def log_exception(ex, user) + Gitlab::AppLogger.error( + message: 'Failed to send notification about expiring resource access tokens', + 'exception.message': ex.message, + 'exception.class': ex.class.name, + class: self.class, + user_id: user.id + ) + end + + def notification_service + NotificationService.new + end + strong_memoize_attr :notification_service + + def base_scope(expiring_date) + PersonalAccessToken + .scope_for_notification_interval(expiring_date) + .owner_is_human + end + # rubocop: enable CodeReuse/ActiveRecord + end +end diff --git a/app/workers/personal_access_tokens/expiring_worker.rb b/app/workers/personal_access_tokens/expiring_worker.rb index 0bbe85bf6875d1..2a94eb15819252 100644 --- a/app/workers/personal_access_tokens/expiring_worker.rb +++ b/app/workers/personal_access_tokens/expiring_worker.rb @@ -18,6 +18,9 @@ class ExpiringWorker # rubocop:disable Scalability/IdempotentWorker BATCH_SIZE = 100 def perform(*args) + # if feature enabled, two other cron jobs will be enqueued instead + return if Feature.enabled?(:expiring_pats_30d_60d_notifications, :instance) + process_user_tokens process_project_bot_tokens end diff --git a/config/feature_flags/gitlab_com_derisk/expiring_pats_30d_60d_notifications.yml b/config/feature_flags/gitlab_com_derisk/expiring_pats_30d_60d_notifications.yml new file mode 100644 index 00000000000000..f06d9a5fa4d73e --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/expiring_pats_30d_60d_notifications.yml @@ -0,0 +1,9 @@ +--- +name: expiring_pats_30d_60d_notifications +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/464040 +introduced_by_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/480556 +milestone: '17.4' +group: group::authentication +type: gitlab_com_derisk +default_enabled: false diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index 0b70a7693ae2be..f2ddeacdf2d50d 100644 --- a/spec/mailers/emails/profile_spec.rb +++ b/spec/mailers/emails/profile_spec.rb @@ -157,6 +157,37 @@ end end + describe 'personal access token is about to expire' do + let_it_be(:user) { create(:user) } + + subject { Notify.access_token_about_to_expire_email(user, ['example token']) } + + it 'is sent to the user' do + is_expected.to deliver_to user + 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 'includes a link to access tokens page' do + is_expected.to have_body_text(/#{user_settings_personal_access_tokens_path}/) + end + + it 'provides the names of expiring tokens' do + is_expected.to have_body_text(/example token/) + end + + context 'when passed days_to_expire parameter' do + subject { Notify.access_token_about_to_expire_email(user, ['example token'], days_to_expire: 42) } + + it 'includes the days_to_expire' do + is_expected.to have_subject(/^Your personal access tokens will expire in 42 days or less$/i) + is_expected.to have_body_text('42') + end + end + end + describe 'resource access token is about to expire' do let_it_be(:user) { create(:user) } @@ -203,6 +234,14 @@ it 'includes the email reason' do is_expected.to have_body_text _('You are receiving this email because you are an Owner of the Group.') end + + context 'when passed days_to_expire parameter' do + subject { Notify.bot_resource_access_token_about_to_expire_email(user, resource, expiring_token.name, days_to_expire: 42) } + + it 'includes the days_to_expire' do + is_expected.to have_body_text('42') + end + end end context 'when access token belongs to a project' do @@ -226,6 +265,14 @@ it 'includes the email reason' do is_expected.to have_body_text _('You are receiving this email because you are either an Owner or Maintainer of the project.') end + + context 'when passed days_to_expire parameter' do + subject { Notify.bot_resource_access_token_about_to_expire_email(user, resource, expiring_token.name, days_to_expire: 42) } + + it 'includes the days_to_expire' do + is_expected.to have_body_text('42') + end + end end end diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index d81ba9a0ed5b2e..a7533ce6dbf612 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/spec/models/personal_access_token_spec.rb @@ -416,6 +416,22 @@ end end + describe '.notification_interval' do + let(:interval) { :seven_days } + + subject(:notification_interval) { described_class.notification_interval(interval) } + + it { is_expected.to eq(7) } + + context 'with invalid interval' do + let(:interval) { :not_real_interval } + + it 'raises error' do + expect { notification_interval }.to raise_error(KeyError) + end + end + end + describe 'ordering by expires_at' do let_it_be(:earlier_token) { create(:personal_access_token, expires_at: 2.days.ago) } let_it_be(:later_token) { create(:personal_access_token, expires_at: 1.day.ago) } diff --git a/spec/workers/personal_access_tokens/expiring_bot_token_worker_spec.rb b/spec/workers/personal_access_tokens/expiring_bot_token_worker_spec.rb new file mode 100644 index 00000000000000..ebd182e45b97af --- /dev/null +++ b/spec/workers/personal_access_tokens/expiring_bot_token_worker_spec.rb @@ -0,0 +1,279 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe PersonalAccessTokens::ExpiringBotTokenWorker, type: :worker, feature_category: :system_access do + subject(:worker) { described_class.new } + + shared_examples 'sends notification about expiry of bot user tokens' do + it 'uses notification service to send the email' do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:bot_resource_access_token_about_to_expire) + .with(project_bot, + expiring_token.name, + a_hash_including(days_to_expire: 7)) + + expect(notification_service).to receive(:bot_resource_access_token_about_to_expire) + .with(project_bot, + thirty_days_expiring_token.name, + a_hash_including(days_to_expire: 30)) + + expect(notification_service).to receive(:bot_resource_access_token_about_to_expire) + .with(project_bot, + sixty_days_expiring_token.name, + a_hash_including(days_to_expire: 60)) + end + + worker.perform + end + + it 'marks expiring_token as delivered', :freeze_time do + expiring_token.reload + expect(expiring_token.expire_notification_delivered).to eq(false) + expect(expiring_token.seven_days_notification_sent_at).to be_nil + expect(expiring_token.thirty_days_notification_sent_at).to be_nil + expect(expiring_token.sixty_days_notification_sent_at).to be_nil + + worker.perform + + expiring_token.reload + expect(expiring_token.expire_notification_delivered).to eq(true) + expect(expiring_token.seven_days_notification_sent_at).to eq(Time.current) + expect(expiring_token.thirty_days_notification_sent_at).to be_nil + expect(expiring_token.sixty_days_notification_sent_at).to be_nil + end + + it 'marks thirty_days_expiring_token as delivered', :freeze_time do + expect(thirty_days_expiring_token.expire_notification_delivered).to eq(false) + expect(thirty_days_expiring_token.seven_days_notification_sent_at).to be_nil + expect(thirty_days_expiring_token.thirty_days_notification_sent_at).to be_nil + expect(thirty_days_expiring_token.sixty_days_notification_sent_at).to be_nil + + worker.perform + + thirty_days_expiring_token.reload + expect(thirty_days_expiring_token.expire_notification_delivered).to eq(false) + expect(thirty_days_expiring_token.seven_days_notification_sent_at).to be_nil + expect(thirty_days_expiring_token.thirty_days_notification_sent_at).to eq(Time.current) + expect(thirty_days_expiring_token.sixty_days_notification_sent_at).to be_nil + end + + it 'marks sixty_days_expiring_token as delivered', :freeze_time do + expect(sixty_days_expiring_token.expire_notification_delivered).to eq(false) + expect(sixty_days_expiring_token.seven_days_notification_sent_at).to be_nil + expect(sixty_days_expiring_token.thirty_days_notification_sent_at).to be_nil + expect(sixty_days_expiring_token.sixty_days_notification_sent_at).to be_nil + + worker.perform + + sixty_days_expiring_token.reload + expect(sixty_days_expiring_token.expire_notification_delivered).to eq(false) + expect(sixty_days_expiring_token.seven_days_notification_sent_at).to be_nil + expect(sixty_days_expiring_token.thirty_days_notification_sent_at).to be_nil + expect(sixty_days_expiring_token.sixty_days_notification_sent_at).to eq(Time.current) + end + + it 'does not mark not_expiring_token as delivered', :freeze_time do + expect(not_expiring_token.expire_notification_delivered).to eq(false) + expect(not_expiring_token.seven_days_notification_sent_at).to be_nil + expect(not_expiring_token.thirty_days_notification_sent_at).to be_nil + expect(not_expiring_token.sixty_days_notification_sent_at).to be_nil + + worker.perform + + not_expiring_token.reload + expect(not_expiring_token.expire_notification_delivered).to eq(false) + expect(not_expiring_token.seven_days_notification_sent_at).to be_nil + expect(not_expiring_token.thirty_days_notification_sent_at).to be_nil + expect(not_expiring_token.sixty_days_notification_sent_at).to be_nil + end + + it 'marks the notification as delivered', :freeze_time do + expect { worker.perform }.to change { expiring_token.reload.expire_notification_delivered }.from(false).to(true) + .and change { expiring_token.reload.seven_days_notification_sent_at }.from(nil).to(Time.current) + end + + context 'when expiring_pats_30d_60d_notifications feature is disabled' do + before do + stub_feature_flags(expiring_pats_30d_60d_notifications: false) + end + + it 'does not mark the notification as delivered' do + expect(NotificationService).not_to receive(:new) + expect { worker.perform }.not_to change { expiring_token.reload.seven_days_notification_sent_at } + end + end + end + + describe '#perform' do + context 'when a token is owned by a project bot' do + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:project) { create(:project, developers: project_bot) } + let_it_be(:expiring_token) do + create(:personal_access_token, name: 'expiring_token', user: project_bot, expires_at: 5.days.from_now) + end + + let_it_be(:thirty_days_expiring_token) do + create(:personal_access_token, name: 'thirty_days_expiring_token', user: project_bot, + expires_at: 28.days.from_now) + end + + let_it_be(:sixty_days_expiring_token) do + create(:personal_access_token, name: 'sixty_days_expiring_token', user: project_bot, + expires_at: 58.days.from_now) + end + + let_it_be(:not_expiring_token) do + create(:personal_access_token, name: 'not_expiring_token', user: project_bot, expires_at: 11.months.from_now) + end + + let(:fake_wh_service) { double } + + it_behaves_like 'sends notification about expiry of bot user tokens' + + context 'with a project webhook' do + let_it_be(:project_hook) { create(:project_hook, project: project, resource_access_token_events: true) } + + let(:hook_data) { {} } + + it 'executes access token webhook' do + expect(Gitlab::DataBuilder::ResourceAccessToken).to receive(:build).with(expiring_token, :expiring, + project).and_return(hook_data) + expect(fake_wh_service).to receive(:async_execute).once + + expect(WebHookService) + .to receive(:new) + .with( + project_hook, + {}, + 'resource_access_token_hooks', + idempotency_key: anything + ) { fake_wh_service } + + worker.perform + end + end + + it 'avoids N+1 queries', :use_sql_query_cache do + control = ActiveRecord::QueryRecorder.new(skip_cached: false) { worker.perform } + + user1 = create(:user, :project_bot, developer_of: project) + create(:personal_access_token, user: user1, expires_at: 5.days.from_now) + + user2 = create(:user, :project_bot, developer_of: project) + create(:personal_access_token, user: user2, expires_at: 5.days.from_now) + + expect { worker.perform }.not_to exceed_all_query_limit(control) + end + end + + context 'when a token is owned by a group bot' do + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:expiring_token) do + create(:personal_access_token, name: 'expiring_token', user: project_bot, expires_at: 5.days.from_now) + end + + let_it_be(:thirty_days_expiring_token) do + create(:personal_access_token, name: 'thirty_days_expiring_token', user: project_bot, + expires_at: 28.days.from_now) + end + + let_it_be(:sixty_days_expiring_token) do + create(:personal_access_token, name: 'sixty_days_expiring_token', user: project_bot, + expires_at: 58.days.from_now) + end + + let_it_be(:not_expiring_token) do + create(:personal_access_token, name: 'not_expiring_token', user: project_bot, expires_at: 11.months.from_now) + end + + context 'when the group of the resource bot exists' do + let_it_be(:group) { create(:group) } + + before_all do + group.add_maintainer(project_bot) + end + + it_behaves_like 'sends notification about expiry of bot user tokens' + end + + context 'when the group of the resource bot has been deleted' do + it 'does not update token with no delivery' do + expect(Group).to be_none + expect(Project).to be_none + + expect { worker.perform }.not_to change { expiring_token.reload.seven_days_notification_sent_at } + end + end + end + + context 'when exception is raised during processing' do + let_it_be(:group) { create(:group) } + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:expiring_token) do + create(:personal_access_token, name: 'expiring_token', user: project_bot, expires_at: 5.days.from_now) + end + + before_all do + group.add_maintainer(project_bot) + end + + context 'with a single resource access token' do + before do + allow_next_instance_of(NotificationService) do |service| + allow(service).to( + receive(:bot_resource_access_token_about_to_expire) + .with(project_bot, expiring_token.name, a_hash_including(days_to_expire: 7)) + .and_raise('boom!') + ) + end + end + + it 'logs error' do + expect(Gitlab::AppLogger).to( + receive(:error) + .with({ message: 'Failed to send notification about expiring resource access tokens', + class: described_class, + "exception.class": "RuntimeError", + "exception.message": "boom!", + user_id: project_bot.id }) + ) + + worker.perform + end + + it 'does not update token with failed delivery' do + expect { worker.perform }.not_to change { expiring_token.reload.seven_days_notification_sent_at } + end + end + + context 'with multiple resource access tokens' do + let_it_be(:another_project_bot) { create(:user, :project_bot) } + let_it_be(:another_expiring_token) do + create(:personal_access_token, user: another_project_bot, expires_at: 5.days.from_now) + end + + before_all do + group.add_maintainer(another_project_bot) + end + + it 'continues sending email' do + expect_next_instance_of(NotificationService) do |service| + expect(service).to( + receive(:bot_resource_access_token_about_to_expire) + .with(project_bot, expiring_token.name, a_hash_including(days_to_expire: 7)) + .and_raise('boom!') + ) + expect(service).to( + receive(:bot_resource_access_token_about_to_expire) + .with(another_project_bot, another_expiring_token.name, a_hash_including(days_to_expire: 7)) + .and_call_original + ) + end + + worker.perform + end + end + end + end +end diff --git a/spec/workers/personal_access_tokens/expiring_personal_token_worker_spec.rb b/spec/workers/personal_access_tokens/expiring_personal_token_worker_spec.rb new file mode 100644 index 00000000000000..2bfaea713637d2 --- /dev/null +++ b/spec/workers/personal_access_tokens/expiring_personal_token_worker_spec.rb @@ -0,0 +1,180 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe PersonalAccessTokens::ExpiringPersonalTokenWorker, type: :worker, feature_category: :system_access do + subject(:worker) { described_class.new } + + describe '#perform' do + context 'when a token needs to be notified' do + let_it_be(:user) { create(:user) } + let_it_be(:expiring_token) do + create(:personal_access_token, name: 'expiring_token', user: user, expires_at: 5.days.from_now) + end + + let_it_be(:expiring_token2) do + create(:personal_access_token, name: 'expiring_token2', user: user, expires_at: 3.days.from_now) + end + + let_it_be(:thirty_days_expiring_token) do + create(:personal_access_token, name: 'thirty_days_expiring_token', user: user, expires_at: 28.days.from_now) + end + + let_it_be(:sixty_days_expiring_token) do + create(:personal_access_token, name: 'sixty_days_expiring_token', user: user, expires_at: 58.days.from_now) + end + + let_it_be(:notified_token) do + create(:personal_access_token, name: 'notified_token', user: user, expires_at: 5.days.from_now, + expire_notification_delivered: true) + end + + let_it_be(:seven_days_notified_token) do + create(:personal_access_token, name: 'seven_days_notified_token', user: user, expires_at: 5.days.from_now, + seven_days_notification_sent_at: Time.current) + end + + let_it_be(:not_expiring_token) do + create(:personal_access_token, name: 'not_expiring_token', user: user, expires_at: 10.months.from_now) + end + + let_it_be(:impersonation_token) do + create(:personal_access_token, name: 'impersonation_token', user: user, expires_at: 5.days.from_now, + impersonation: true) + end + + it 'uses notification service to send emails for each interval' do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:access_token_about_to_expire).with(user, + match_array([expiring_token.name, expiring_token2.name]), a_hash_including(days_to_expire: 7)) + expect(notification_service).to receive(:access_token_about_to_expire).with(user, + match_array([thirty_days_expiring_token.name]), a_hash_including(days_to_expire: 30)) + expect(notification_service).to receive(:access_token_about_to_expire).with(user, + match_array([sixty_days_expiring_token.name]), a_hash_including(days_to_expire: 60)) + end + + worker.perform + end + + it 'marks expiring_token as delivered', :freeze_time do + expect(expiring_token.expire_notification_delivered).to eq(false) + expect(expiring_token.seven_days_notification_sent_at).to be_nil + expect(expiring_token.thirty_days_notification_sent_at).to be_nil + expect(expiring_token.sixty_days_notification_sent_at).to be_nil + + worker.perform + + expiring_token.reload + expect(expiring_token.expire_notification_delivered).to eq(true) + expect(expiring_token.seven_days_notification_sent_at).to eq(Time.current) + expect(expiring_token.thirty_days_notification_sent_at).to be_nil + expect(expiring_token.sixty_days_notification_sent_at).to be_nil + end + + it 'marks thirty_days_expiring_token as delivered', :freeze_time do + expect(thirty_days_expiring_token.expire_notification_delivered).to eq(false) + expect(thirty_days_expiring_token.seven_days_notification_sent_at).to be_nil + expect(thirty_days_expiring_token.thirty_days_notification_sent_at).to be_nil + expect(thirty_days_expiring_token.sixty_days_notification_sent_at).to be_nil + + worker.perform + + thirty_days_expiring_token.reload + expect(thirty_days_expiring_token.expire_notification_delivered).to eq(false) + expect(thirty_days_expiring_token.seven_days_notification_sent_at).to be_nil + expect(thirty_days_expiring_token.thirty_days_notification_sent_at).to eq(Time.current) + expect(thirty_days_expiring_token.sixty_days_notification_sent_at).to be_nil + end + + it 'marks sixty_days_expiring_token as delivered', :freeze_time do + expect(sixty_days_expiring_token.expire_notification_delivered).to eq(false) + expect(sixty_days_expiring_token.seven_days_notification_sent_at).to be_nil + expect(sixty_days_expiring_token.thirty_days_notification_sent_at).to be_nil + expect(sixty_days_expiring_token.sixty_days_notification_sent_at).to be_nil + + worker.perform + + sixty_days_expiring_token.reload + expect(sixty_days_expiring_token.expire_notification_delivered).to eq(false) + expect(sixty_days_expiring_token.seven_days_notification_sent_at).to be_nil + expect(sixty_days_expiring_token.thirty_days_notification_sent_at).to be_nil + expect(sixty_days_expiring_token.sixty_days_notification_sent_at).to eq(Time.current) + end + + it 'does not mark not_expiring_token as delivered' do + expect(not_expiring_token.expire_notification_delivered).to eq(false) + expect(not_expiring_token.seven_days_notification_sent_at).to be_nil + expect(not_expiring_token.thirty_days_notification_sent_at).to be_nil + expect(not_expiring_token.sixty_days_notification_sent_at).to be_nil + + worker.perform + + not_expiring_token.reload + expect(not_expiring_token.expire_notification_delivered).to eq(false) + expect(not_expiring_token.seven_days_notification_sent_at).to be_nil + expect(not_expiring_token.thirty_days_notification_sent_at).to be_nil + expect(not_expiring_token.sixty_days_notification_sent_at).to be_nil + end + + context 'when expiring_pats_30d_60d_notifications feature is disabled' do + before do + stub_feature_flags(expiring_pats_30d_60d_notifications: false) + end + + it 'does not mark the notification as delivered' do + expect(NotificationService).not_to receive(:new) + expect { worker.perform }.not_to change { expiring_token.reload.seven_days_notification_sent_at } + end + end + + it 'avoids N+1 queries', :use_sql_query_cache do + control = ActiveRecord::QueryRecorder.new(skip_cached: false) { worker.perform } + + user1 = create(:user) + create(:personal_access_token, user: user1, expires_at: 5.days.from_now) + + user2 = create(:user) + create(:personal_access_token, user: user2, expires_at: 5.days.from_now) + + # Query count increased for the user look up + # there are still two N+1 queries one for token name look up and another for token update. + expect { worker.perform }.not_to exceed_all_query_limit(control).with_threshold(2) + end + end + + context 'when no tokens need to be notified' do + let_it_be(:pat) do + create(:personal_access_token, expires_at: 5.days.from_now, expire_notification_delivered: true) + end + + let_it_be(:seven_days_pat) do + create(:personal_access_token, expires_at: 5.days.from_now, + seven_days_notification_sent_at: Time.current - 2.days) + end + + it "doesn't call notification services" do + expect(worker).not_to receive(:notification_service) + + worker.perform + end + + it "doesn't change the notification delivered of the token" do + expect { worker.perform }.not_to change { pat.reload.expire_notification_delivered } + end + end + + context 'when a token is an impersonation token' do + let_it_be(:pat) { create(:personal_access_token, :impersonation, expires_at: 5.days.from_now) } + + it "doesn't use notification service to send the email" do + expect(worker).not_to receive(:notification_service) + + worker.perform + end + + it "doesn't change the notification delivered of the token" do + expect { worker.perform }.not_to change { pat.reload.expire_notification_delivered } + end + end + end +end diff --git a/spec/workers/personal_access_tokens/expiring_worker_spec.rb b/spec/workers/personal_access_tokens/expiring_worker_spec.rb index 95026e53bd3a7f..0ca044d0578f8d 100644 --- a/spec/workers/personal_access_tokens/expiring_worker_spec.rb +++ b/spec/workers/personal_access_tokens/expiring_worker_spec.rb @@ -21,6 +21,12 @@ end end + # this worker is being replaced with ExpiringPersonalTokenWorker and ExpiringBotTokenWorker + # Once the feature flag is enabled, this worker should be removed along with all specs + before do + stub_feature_flags(expiring_pats_30d_60d_notifications: false) + end + describe '#perform' do context 'when a token needs to be notified' do let_it_be(:user) { create(:user) } @@ -43,6 +49,16 @@ .and change { expiring_token.reload.seven_days_notification_sent_at }.from(nil).to(Time.current) end + context 'when expiring_pats_30d_60d_notifications is enabled' do + before do + stub_feature_flags(expiring_pats_30d_60d_notifications: true) + end + + it 'does not send the notification' do + expect { worker.perform }.not_to change { expiring_token.reload.expire_notification_delivered } + end + end + it 'avoids N+1 queries', :use_sql_query_cache do control = ActiveRecord::QueryRecorder.new(skip_cached: false) { worker.perform } -- GitLab From d87aedc8530068f195ce9a3a11d9cee86a473fa6 Mon Sep 17 00:00:00 2001 From: agius Date: Fri, 23 Aug 2024 16:17:23 -0700 Subject: [PATCH 3/5] Update sidekiq queues list --- app/workers/all_queues.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index ddbf0f485eb55c..75cf16904fcf0b 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -768,6 +768,24 @@ :weight: 1 :idempotent: false :tags: [] +- :name: cronjob:personal_access_tokens_expiring_bot_token + :worker_name: PersonalAccessTokens::ExpiringBotTokenWorker + :feature_category: :system_access + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: false + :tags: [] +- :name: cronjob:personal_access_tokens_expiring_personal_token + :worker_name: PersonalAccessTokens::ExpiringPersonalTokenWorker + :feature_category: :system_access + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: false + :tags: [] - :name: cronjob:pipeline_schedule :worker_name: PipelineScheduleWorker :feature_category: :continuous_integration -- GitLab From 5547264b1f754d3f525eb88c76afc8ddc3c1b413 Mon Sep 17 00:00:00 2001 From: agius Date: Tue, 3 Sep 2024 17:42:20 -0700 Subject: [PATCH 4/5] Fix spec expectations --- .../personal_access_tokens/expiring_worker_spec.rb | 1 + spec/services/notification_service_spec.rb | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/ee/spec/workers/personal_access_tokens/expiring_worker_spec.rb b/ee/spec/workers/personal_access_tokens/expiring_worker_spec.rb index 5d851685e9595a..664f3ea3c7199a 100644 --- a/ee/spec/workers/personal_access_tokens/expiring_worker_spec.rb +++ b/ee/spec/workers/personal_access_tokens/expiring_worker_spec.rb @@ -16,6 +16,7 @@ before_all do group.add_developer(project_bot) + stub_feature_flags(expiring_pats_30d_60d_notifications: false) end it 'executes access token webhook' do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index e1a77582aba65c..610a964ae4450a 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -404,12 +404,14 @@ owner1, project_bot.resource_bot_resource, expiring_token, + {}, mail: "bot_resource_access_token_about_to_expire_email" ).and( have_enqueued_email( owner2, project_bot.resource_bot_resource, expiring_token, + {}, mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -466,6 +468,7 @@ owner1, project_bot.resource_bot_resource, [expiring_token_1, expiring_token_2], + {}, mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -475,6 +478,7 @@ owner2, project_bot.resource_bot_resource, [expiring_token_1, expiring_token_2], + {}, mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -496,12 +500,14 @@ maintainer, project_bot.resource_bot_resource, expiring_token, + {}, mail: "bot_resource_access_token_about_to_expire_email" ).and( have_enqueued_email( project.owner, project_bot.resource_bot_resource, expiring_token, + {}, mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -520,6 +526,7 @@ maintainer, project_bot.resource_bot_resource, expiring_token, + {}, mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -529,6 +536,7 @@ project.owner, project_bot.resource_bot_resource, expiring_token, + {}, mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -544,7 +552,7 @@ subject(:notification_service) { notification.access_token_about_to_expire(user, [pat.name]) } it 'sends email to the token owner' do - expect { notification_service }.to have_enqueued_email(user, [pat.name], mail: "access_token_about_to_expire_email") + expect { notification_service }.to have_enqueued_email(user, [pat.name], {}, mail: "access_token_about_to_expire_email") end it "logs notication sent message" do -- GitLab From ef02f26668a6a27a918ef6a54e8be26ca090f9dd Mon Sep 17 00:00:00 2001 From: agius Date: Wed, 4 Sep 2024 12:50:42 -0700 Subject: [PATCH 5/5] Move long migrations to post_migrate and batched migration --- ...ess_token_seven_days_notification_sent.yml | 10 +++++ ...0619_add_indices_for_pat_expiry_columns.rb | 0 ...ess_tokens_seven_days_notification_sent.rb | 43 ------------------- ...cess_token_seven_days_notification_sent.rb | 37 ++++++++++++++++ db/schema_migrations/20240822201906 | 1 - db/schema_migrations/20240904184422 | 1 + ...cess_token_seven_days_notification_sent.rb | 30 +++++++++++++ ...oken_seven_days_notification_sent_spec.rb} | 19 ++++++-- ...token_seven_days_notification_sent_spec.rb | 26 +++++++++++ 9 files changed, 119 insertions(+), 48 deletions(-) create mode 100644 db/docs/batched_background_migrations/backfill_personal_access_token_seven_days_notification_sent.yml rename db/{migrate => post_migrate}/20240822200619_add_indices_for_pat_expiry_columns.rb (100%) delete mode 100644 db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent.rb create mode 100644 db/post_migrate/20240904184422_queue_backfill_personal_access_token_seven_days_notification_sent.rb delete mode 100644 db/schema_migrations/20240822201906 create mode 100644 db/schema_migrations/20240904184422 create mode 100644 lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent.rb rename spec/{migrations/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent_spec.rb => lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent_spec.rb} (80%) create mode 100644 spec/migrations/20240904184422_queue_backfill_personal_access_token_seven_days_notification_sent_spec.rb diff --git a/db/docs/batched_background_migrations/backfill_personal_access_token_seven_days_notification_sent.yml b/db/docs/batched_background_migrations/backfill_personal_access_token_seven_days_notification_sent.yml new file mode 100644 index 00000000000000..c9ea77a5715f35 --- /dev/null +++ b/db/docs/batched_background_migrations/backfill_personal_access_token_seven_days_notification_sent.yml @@ -0,0 +1,10 @@ +--- +migration_job_name: BackfillPersonalAccessTokenSevenDaysNotificationSent +description: Backfill seven_days_notification_sent_at column using data from expires_at column in personal_access_tokens table. +feature_category: system_access +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/163818 +milestone: '17.4' +queued_migration_version: 20240904184422 +# Replace with the approximate date you think it's best to ensure the completion of this BBM. +finalize_after: '2024-10-24' +finalized_by: # version of the migration that finalized this BBM diff --git a/db/migrate/20240822200619_add_indices_for_pat_expiry_columns.rb b/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb similarity index 100% rename from db/migrate/20240822200619_add_indices_for_pat_expiry_columns.rb rename to db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb diff --git a/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent.rb b/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent.rb deleted file mode 100644 index 9f1f86469bd315..00000000000000 --- a/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent.rb +++ /dev/null @@ -1,43 +0,0 @@ -# frozen_string_literal: true - -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class BackfillPersonalAccessTokensSevenDaysNotificationSent < Gitlab::Database::Migration[2.2] - # When using the methods "add_concurrent_index" or "remove_concurrent_index" - # you must disable the use of transactions - # as these methods can not run in an existing transaction. - # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure - # that either of them is the _only_ method called in the migration, - # any other changes should go in a separate migration. - # This ensures that upon failure _only_ the index creation or removing fails - # and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - disable_ddl_transaction! - # - # Configure the `gitlab_schema` to perform data manipulation (DML). - # Visit: https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html - restrict_gitlab_migration gitlab_schema: :gitlab_main - - # Add dependent 'batched_background_migrations.queued_migration_version' values. - # DEPENDENT_BATCHED_BACKGROUND_MIGRATIONS = [] - milestone '17.4' - - include ::Gitlab::Database::DynamicModelHelpers - - def up - scope = ->(table) { table.where(expire_notification_delivered: true).where.not(expires_at: nil) } - each_batch_range('personal_access_tokens', scope: scope, of: BATCH_SIZE) do |min, max| - execute <<~SQL - UPDATE personal_access_tokens - SET seven_days_notification_sent_at = (expires_at - interval '7 days') - WHERE expire_notification_delivered = true - AND seven_days_notification_sent_at IS NULL - AND expires_at IS NOT NULL - AND id BETWEEN #{min} AND #{max} - SQL - end - end -end diff --git a/db/post_migrate/20240904184422_queue_backfill_personal_access_token_seven_days_notification_sent.rb b/db/post_migrate/20240904184422_queue_backfill_personal_access_token_seven_days_notification_sent.rb new file mode 100644 index 00000000000000..6b5163727cadd0 --- /dev/null +++ b/db/post_migrate/20240904184422_queue_backfill_personal_access_token_seven_days_notification_sent.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/database/batched_background_migrations.html +# for more information on when/how to queue batched background migrations + +# Update below commented lines with appropriate values. + +class QueueBackfillPersonalAccessTokenSevenDaysNotificationSent < Gitlab::Database::Migration[2.2] + milestone '17.4' + + # Select the applicable gitlab schema for your batched background migration + restrict_gitlab_migration gitlab_schema: :gitlab_main # / :gitlab_ci / :gitlab_main_clusterwide / ... + + MIGRATION = "BackfillPersonalAccessTokenSevenDaysNotificationSent" + DELAY_INTERVAL = 2.minutes + BATCH_SIZE = 1000 + SUB_BATCH_SIZE = 100 + + def up + # If you are requeueing an already executed migration, you need to delete the prior batched migration record + # for the new enqueue to be executed, else, you can delete this line. + # delete_batched_background_migration(MIGRATION, :personal_access_tokens, :id, []) + + queue_batched_background_migration( + MIGRATION, + :personal_access_tokens, + :id, + job_interval: DELAY_INTERVAL, + batch_size: BATCH_SIZE, + sub_batch_size: SUB_BATCH_SIZE + ) + end + + def down + delete_batched_background_migration(MIGRATION, :personal_access_tokens, :id, []) + end +end diff --git a/db/schema_migrations/20240822201906 b/db/schema_migrations/20240822201906 deleted file mode 100644 index ed93006976b7bd..00000000000000 --- a/db/schema_migrations/20240822201906 +++ /dev/null @@ -1 +0,0 @@ -f70c475c2db47712af045b59f36d9f0971f18a18c9e0f8060b86b35969dc248c \ No newline at end of file diff --git a/db/schema_migrations/20240904184422 b/db/schema_migrations/20240904184422 new file mode 100644 index 00000000000000..30b483042994e2 --- /dev/null +++ b/db/schema_migrations/20240904184422 @@ -0,0 +1 @@ +6c5a9b4590942f52812d09deeca5ce06b1d05931630c3dbc48b5deb20bbab8fc \ No newline at end of file diff --git a/lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent.rb b/lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent.rb new file mode 100644 index 00000000000000..4a414eda197e7b --- /dev/null +++ b/lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/database/batched_background_migrations.html +# for more information on how to use batched background migrations + +# Update below commented lines with appropriate values. + +module Gitlab + module BackgroundMigration + class BackfillPersonalAccessTokenSevenDaysNotificationSent < BatchedMigrationJob + # This is used as the key on collecting metrics + operation_name :backfill_personal_access_token_seven_days_notification_sent + + # rubocop:disable CodeReuse/ActiveRecord -- guidelines says query methods are okay to use here + scope_to ->(relation) do + relation.where(expire_notification_delivered: true, seven_days_notification_sent_at: nil) + .where.not(expires_at: nil) + end + # rubocop:enable CodeReuse/ActiveRecord + + feature_category :system_access + + def perform + each_sub_batch do |sub_batch| + sub_batch.update_all("seven_days_notification_sent_at = (expires_at - interval '7 days')") + end + end + end + end +end diff --git a/spec/migrations/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent_spec.rb b/spec/lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent_spec.rb similarity index 80% rename from spec/migrations/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent_spec.rb rename to spec/lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent_spec.rb index 1fa23b8e64a8a9..73ba0144f796b3 100644 --- a/spec/migrations/db/post_migrate/20240822201906_backfill_personal_access_tokens_seven_days_notification_sent_spec.rb +++ b/spec/lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent_spec.rb @@ -1,9 +1,8 @@ # frozen_string_literal: true require 'spec_helper' -require_migration! -RSpec.describe BackfillPersonalAccessTokensSevenDaysNotificationSent, migration: :gitlab_main, feature_category: :system_access do +RSpec.describe Gitlab::BackgroundMigration::BackfillPersonalAccessTokenSevenDaysNotificationSent, feature_category: :system_access do let(:pats_table) { table(:personal_access_tokens) } let(:users) { table(:users) } let(:organizations) { table(:organizations) } @@ -30,14 +29,26 @@ organization_id: organization.id) end - describe '#up' do + describe '#perform' do + subject(:perform_migration) do + described_class.new( + start_id: pats_table.first.id, + end_id: pats_table.last.id, + batch_table: :personal_access_tokens, + batch_column: :id, + sub_batch_size: pats_table.count, + pause_ms: 0, + connection: ActiveRecord::Base.connection + ).perform + end + it 'backfills seven_days_notification_sent_at field', :freeze_time do expect(pat_to_update.reload.seven_days_notification_sent_at).to be_nil expect(seven_days_notified.reload.seven_days_notification_sent_at).to eq(Time.current - 1.day) expect(not_notified_pat.reload.seven_days_notification_sent_at).to be_nil expect(no_expiry_pat.reload.seven_days_notification_sent_at).to be_nil - migrate! + perform_migration # db updates do not use the same timezone as Rails; default to UTC db_updated_time = Time.utc(Time.current.year, Time.current.month, Time.current.day) - 7.days diff --git a/spec/migrations/20240904184422_queue_backfill_personal_access_token_seven_days_notification_sent_spec.rb b/spec/migrations/20240904184422_queue_backfill_personal_access_token_seven_days_notification_sent_spec.rb new file mode 100644 index 00000000000000..1a53799b7236d4 --- /dev/null +++ b/spec/migrations/20240904184422_queue_backfill_personal_access_token_seven_days_notification_sent_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe QueueBackfillPersonalAccessTokenSevenDaysNotificationSent, feature_category: :system_access do + let!(:batched_migration) { described_class::MIGRATION } + + it 'schedules a new batched migration' do + reversible_migration do |migration| + migration.before -> { + expect(batched_migration).not_to have_scheduled_batched_migration + } + + migration.after -> { + expect(batched_migration).to have_scheduled_batched_migration( + table_name: :personal_access_tokens, + column_name: :id, + interval: described_class::DELAY_INTERVAL, + batch_size: described_class::BATCH_SIZE, + sub_batch_size: described_class::SUB_BATCH_SIZE + ) + } + end + end +end -- GitLab