Merge branch...

Merge branch '300714-provide-name-of-expiring-token-in-personal-access-token-expiration-mail' into 'master'

Provide name of expiring token in personal access token expiration mail

See merge request gitlab-org/gitlab!53766

app/mailers/emails/profile.rb

@@ -50,15 +50,16 @@ module Emails
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    def access_token_about_to_expire_email(user)
+    def access_token_about_to_expire_email(user, token_names)
       return unless user
 
       @user = user
+      @token_names = token_names
       @target_url = profile_personal_access_tokens_url
       @days_to_expire = PersonalAccessToken::DAYS_TO_EXPIRE
 
       Gitlab::I18n.with_locale(@user.preferred_language) do
-        mail(to: @user.notification_email, subject: subject(_("Your Personal Access Tokens will expire in %{days_to_expire} days or less") % { days_to_expire: @days_to_expire }))
+        mail(to: @user.notification_email, subject: subject(_("Your personal access tokens will expire in %{days_to_expire} days or less") % { days_to_expire: @days_to_expire }))
       end
     end
 

app/models/personal_access_token.rb

@@ -4,6 +4,7 @@ class PersonalAccessToken < ApplicationRecord
   include Expirable
   include TokenAuthenticatable
   include Sortable
+  include EachBatch
   extend ::Gitlab::Utils::Override
 
   add_authentication_token_field :token, digest: true
@@ -97,6 +98,10 @@ class PersonalAccessToken < ApplicationRecord
   end
 
   def set_default_scopes
+    # When only loading a select set of attributes, for example using `EachBatch`,
+    # the `scopes` attribute is not present, so we can't initialize it.
+    return unless has_attribute?(:scopes)
+
     self.scopes = Gitlab::Auth::DEFAULT_SCOPES if self.scopes.empty?
   end
 

app/services/notification_service.rb

@@ -66,10 +66,10 @@ class NotificationService
 
   # 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)
+  def access_token_about_to_expire(user, token_names)
     return unless user.can?(:receive_notifications)
 
-    mailer.access_token_about_to_expire_email(user).deliver_later
+    mailer.access_token_about_to_expire_email(user, token_names).deliver_later
   end
 
   # Notify the user when at least one of their personal access tokens has expired today

app/views/notify/access_token_about_to_expire_email.html.haml

 %p
   = _('Hi %{username}!') % { username: sanitize_name(@user.name) }
 %p
-  = _('One or more of your personal access tokens will expire in %{days_to_expire} days or less.') % { days_to_expire: @days_to_expire }
+  = _('One or more of your personal access tokens will expire in %{days_to_expire} days or less:') % { days_to_expire: @days_to_expire }
 %p
-  - pat_link_start =  '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: @target_url }
+  %ul
+    - @token_names.each do |token|
+      %li= token
+%p
+  - pat_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: @target_url }
   = html_escape(_('You can create a new one or check them in your %{pat_link_start}personal access tokens%{pat_link_end} settings')) % { pat_link_start: pat_link_start, pat_link_end: '</a>'.html_safe }

app/views/notify/access_token_about_to_expire_email.text.erb

 <%= _('Hi %{username}!') % { username: sanitize_name(@user.name) } %>
 
-<%= _('One or more of your personal access tokens will expire in %{days_to_expire} days or less.') % { days_to_expire: @days_to_expire} %>
+<%= _('One or more of your personal access tokens will expire in %{days_to_expire} days or less:') % { days_to_expire: @days_to_expire } %>
+
+<% @token_names.each do |token| %>
+  - <%= token %>
+<% end %>
 
 <%= _('You can create a new one or check them in your personal access tokens settings %{pat_link}') % { pat_link: @target_url } %>

app/workers/personal_access_tokens/expiring_worker.rb

@@ -7,17 +7,32 @@ module PersonalAccessTokens
 
     feature_category :authentication_and_authorization
 
+    MAX_TOKENS = 100
+
     def perform(*args)
       notification_service = NotificationService.new
       limit_date = PersonalAccessToken::DAYS_TO_EXPIRE.days.from_now.to_date
 
       User.with_expiring_and_not_notified_personal_access_tokens(limit_date).find_each do |user|
         with_context(user: user) do
-          notification_service.access_token_about_to_expire(user)
+          expiring_user_tokens = user.personal_access_tokens.without_impersonation.expiring_and_not_notified(limit_date)
+
+          # rubocop: disable CodeReuse/ActiveRecord
+          # 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
+
+          # rubocop: enable CodeReuse/ActiveRecord
+
+          notification_service.access_token_about_to_expire(user, token_names)
 
           Gitlab::AppLogger.info "#{self.class}: Notifying User #{user.id} about expiring tokens"
 
-          user.personal_access_tokens.without_impersonation.expiring_and_not_notified(limit_date).update_all(expire_notification_delivered: true)
+          expiring_user_tokens.each_batch do |expiring_tokens|
+            expiring_tokens.update_all(expire_notification_delivered: true)
+          end
         end
       end
     end

changelogs/unreleased/300714-provide-name-of-expiring-token-in-personal-access-token-expiration.yml

+---
+title: Provide name of expiring token in personal access token expiration mail
+merge_request: 53766
+author:
+type: changed

locale/gitlab.pot

@@ -21067,7 +21067,7 @@ msgstr ""
 msgid "One or more of your personal access tokens has expired."
 msgstr ""
 
-msgid "One or more of your personal access tokens will expire in %{days_to_expire} days or less."
+msgid "One or more of your personal access tokens will expire in %{days_to_expire} days or less:"
 msgstr ""
 
 msgid "Only 'Reporter' roles and above on tiers Premium and above can see Value Stream Analytics."
@@ -34389,9 +34389,6 @@ msgstr ""
 msgid "Your Personal Access Token was revoked"
 msgstr ""
 
-msgid "Your Personal Access Tokens will expire in %{days_to_expire} days or less"
-msgstr ""
-
 msgid "Your Primary Email will be used for avatar detection."
 msgstr ""
 
@@ -34563,6 +34560,9 @@ msgstr ""
 msgid "Your personal access token has expired"
 msgstr ""
 
+msgid "Your personal access tokens will expire in %{days_to_expire} days or less"
+msgstr ""
+
 msgid "Your profile"
 msgstr ""
 

spec/mailers/emails/profile_spec.rb

@@ -125,8 +125,9 @@ RSpec.describe Emails::Profile do
 
   describe 'user personal access token is about to expire' do
     let_it_be(:user) { create(:user) }
+    let_it_be(:expiring_token) { create(:personal_access_token, user: user, expires_at: 5.days.from_now) }
 
-    subject { Notify.access_token_about_to_expire_email(user) }
+    subject { Notify.access_token_about_to_expire_email(user, [expiring_token.name]) }
 
     it_behaves_like 'an email sent from GitLab'
     it_behaves_like 'it should not have Gmail Actions links'
@@ -137,13 +138,17 @@ RSpec.describe Emails::Profile do
     end
 
     it 'has the correct subject' do
-      is_expected.to have_subject /^Your Personal Access Tokens will expire in 7 days or less$/i
+      is_expected.to have_subject /^Your personal access tokens will expire in 7 days or less$/i
     end
 
     it 'mentions the access tokens will expire' do
       is_expected.to have_body_text /One or more of your personal access tokens will expire in 7 days or less/
     end
 
+    it 'provides the names of expiring tokens' do
+      is_expected.to have_body_text /#{expiring_token.name}/
+    end
+
     it 'includes a link to personal access tokens page' do
       is_expected.to have_body_text /#{profile_personal_access_tokens_path}/
     end

spec/services/notification_service_spec.rb

@@ -243,11 +243,12 @@ RSpec.describe NotificationService, :mailer do
   describe 'AccessToken' do
     describe '#access_token_about_to_expire' do
       let_it_be(:user) { create(:user) }
+      let_it_be(:pat) { create(:personal_access_token, user: user, expires_at: 5.days.from_now) }
 
-      subject { notification.access_token_about_to_expire(user) }
+      subject { notification.access_token_about_to_expire(user, [pat.name]) }
 
       it 'sends email to the token owner' do
-        expect { subject }.to have_enqueued_email(user, mail: "access_token_about_to_expire_email")
+        expect { subject }.to have_enqueued_email(user, [pat.name], mail: "access_token_about_to_expire_email")
       end
     end
 

spec/workers/personal_access_tokens/expiring_worker_spec.rb

@@ -7,18 +7,23 @@ RSpec.describe PersonalAccessTokens::ExpiringWorker, type: :worker do
 
   describe '#perform' do
     context 'when a token needs to be notified' do
-      let_it_be(:pat) { create(:personal_access_token, expires_at: 5.days.from_now) }
+      let_it_be(:user) { create(:user) }
+      let_it_be(:expiring_token) { create(:personal_access_token, user: user, expires_at: 5.days.from_now) }
+      let_it_be(:expiring_token2) { create(:personal_access_token, user: user, expires_at: 3.days.from_now) }
+      let_it_be(:notified_token) { create(:personal_access_token, user: user, expires_at: 5.days.from_now, expire_notification_delivered: true) }
+      let_it_be(:not_expiring_token) { create(:personal_access_token, user: user, expires_at: 1.month.from_now) }
+      let_it_be(:impersonation_token) { create(:personal_access_token, user: user, expires_at: 5.days.from_now, impersonation: true) }
 
       it 'uses notification service to send the email' do
         expect_next_instance_of(NotificationService) do |notification_service|
-          expect(notification_service).to receive(:access_token_about_to_expire).with(pat.user)
+          expect(notification_service).to receive(:access_token_about_to_expire).with(user, match_array([expiring_token.name, expiring_token2.name]))
         end
 
         worker.perform
       end
 
       it 'marks the notification as delivered' do
-        expect { worker.perform }.to change { pat.reload.expire_notification_delivered }.from(false).to(true)
+        expect { worker.perform }.to change { expiring_token.reload.expire_notification_delivered }.from(false).to(true)
       end
     end
 
@@ -27,7 +32,7 @@ RSpec.describe PersonalAccessTokens::ExpiringWorker, type: :worker do
 
       it "doesn't use notification service to send the email" do
         expect_next_instance_of(NotificationService) do |notification_service|
-          expect(notification_service).not_to receive(:access_token_about_to_expire).with(pat.user)
+          expect(notification_service).not_to receive(:access_token_about_to_expire).with(pat.user, [pat.name])
         end
 
         worker.perform
@@ -43,7 +48,7 @@ RSpec.describe PersonalAccessTokens::ExpiringWorker, type: :worker do
 
       it "doesn't use notification service to send the email" do
         expect_next_instance_of(NotificationService) do |notification_service|
-          expect(notification_service).not_to receive(:access_token_about_to_expire).with(pat.user)
+          expect(notification_service).not_to receive(:access_token_about_to_expire).with(pat.user, [pat.name])
         end
 
         worker.perform