diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index 8a0d1ea2638b6ffa1b9a306b39cb10ea1972850d..18c68e737868467e09a81c31eb8744b989d329df 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 0b4336a6113798eaf6dd4af95781b40180d836c3..a6f7200a42e9837ad90b52526b499e5b3a4e1b79 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 @@ -27,7 +33,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 +50,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? @@ -77,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 4d3bf1efedca6f0adde6e5a06e5a2079ad0040f3..1ca6ef1361fc32b32ca37f0b09989aab4cec80cd 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/all_queues.yml b/app/workers/all_queues.yml index ddbf0f485eb55c370c4fdaaf5c9e84a5b98251d9..75cf16904fcf0b149f79e37fb39670e737dfed0b 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 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 0000000000000000000000000000000000000000..75b62cdcece368d22142bedc2f9ac246b8959548 --- /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 0000000000000000000000000000000000000000..8b1429a15e70bc212750f785a6629e4de18e7b31 --- /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 492fdf9776cdd37cde868d9758e44dc3fe4d9a95..2a94eb158192521ebfb190bbb39a9bf763641155 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 @@ -52,7 +55,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 +108,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/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 0000000000000000000000000000000000000000..f06d9a5fa4d73e03ec5ba6a086991c18307b7343 --- /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/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 0000000000000000000000000000000000000000..c9ea77a5715f3555252fe9077879dea938a7017c --- /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/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 0000000000000000000000000000000000000000..6946f182bd4107b214462b5ff93a199bcda4e596 --- /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/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb b/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb new file mode 100644 index 0000000000000000000000000000000000000000..8d3d607d875fd3b0157d03eaf8b71f7e16417690 --- /dev/null +++ b/db/post_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/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 0000000000000000000000000000000000000000..6b5163727cadd08af3260ab9bc4a7551bfbb51a4 --- /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/20240822195511 b/db/schema_migrations/20240822195511 new file mode 100644 index 0000000000000000000000000000000000000000..45c3c17bdf0ac2f3037938b47ca659b0bf1eb8c1 --- /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 0000000000000000000000000000000000000000..9495d9228b5ded44cd81de05729c3062b43f770d --- /dev/null +++ b/db/schema_migrations/20240822200619 @@ -0,0 +1 @@ +d7ef657ff096c75aa7113e0e2562d9432fddfa4e1bb9a3a148fb29aa2926f006 \ No newline at end of file diff --git a/db/schema_migrations/20240904184422 b/db/schema_migrations/20240904184422 new file mode 100644 index 0000000000000000000000000000000000000000..30b483042994e24a7445e27f1617fbdee3848c98 --- /dev/null +++ b/db/schema_migrations/20240904184422 @@ -0,0 +1 @@ +6c5a9b4590942f52812d09deeca5ce06b1d05931630c3dbc48b5deb20bbab8fc \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index a4a019b08d7b6443da68cab8da07f81058fce3f4..b4372ab7012756f19256de641c5519a98bb250b0 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/ee/spec/workers/personal_access_tokens/expiring_worker_spec.rb b/ee/spec/workers/personal_access_tokens/expiring_worker_spec.rb index 5d851685e9595a1e7765ee3469651a90021d2fb4..664f3ea3c7199ac42f6e31715a6bfa443ab61ecf 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/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 0000000000000000000000000000000000000000..4a414eda197e7b42bd75632bb47b2ca9df620d91 --- /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/lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent_spec.rb b/spec/lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..73ba0144f796b3bec535436530ed500fdf1e23ea --- /dev/null +++ b/spec/lib/gitlab/background_migration/backfill_personal_access_token_seven_days_notification_sent_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +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) } + + 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 '#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 + + 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 + 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/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index 0b70a7693ae2be8d4856911e3874287d1065ecee..f2ddeacdf2d50d59f8e65fa583a0d4e41a349731 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/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 0000000000000000000000000000000000000000..1a53799b7236d4cd1a71844a6fd06dbea7a7583c --- /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 diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index 2f8e82117cd191bcd1a383dbfcf1ff8d781b87fa..a7533ce6dbf612b047601aa96591be263bad3939 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) } @@ -414,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/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index e1a77582aba65c3be57d97b772a78cef46d83200..610a964ae4450a375baee881143897eb68cc325c 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 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 0000000000000000000000000000000000000000..ebd182e45b97af82d661bd805911176eac850a82 --- /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 0000000000000000000000000000000000000000..2bfaea713637d21e398d0f38c58525d05c7e9436 --- /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 3065d7c12f4cc8336310b7f8e5d644134a85c6e1..0ca044d0578f8d446f50b71b69d631c3c86e1e06 100644 --- a/spec/workers/personal_access_tokens/expiring_worker_spec.rb +++ b/spec/workers/personal_access_tokens/expiring_worker_spec.rb @@ -15,11 +15,18 @@ 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 + # 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) } @@ -37,8 +44,19 @@ 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 + + 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 @@ -64,6 +82,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)