From 817fb2ec2733ee302f14d6da61ad66987012a387 Mon Sep 17 00:00:00 2001
From: Smriti Garg <sgarg@gitlab.com>
Date: Mon, 15 Apr 2024 12:56:24 +0000
Subject: [PATCH] Error in rendering of token names
A bot user can have one pat at any point of time.
ExpiringWorker is written on the same premise and we expect
the user of type bot to have only one pat. This mailer need
not be an exception and run a loop on multiple token names
Changelog: fixed
Error in rendering of token names
Wrong code comitted
---
app/mailers/emails/profile.rb | 5 +++--
app/mailers/previews/notify_preview.rb | 2 +-
app/services/notification_service.rb | 6 +++---
...e_access_token_about_to_expire_email.html.haml | 10 ++++++++++
...ce_access_token_about_to_expire_email.text.erb | 7 +++++++
..._access_tokens_about_to_expire_email.html.haml | 15 ---------------
...e_access_tokens_about_to_expire_email.text.erb | 13 -------------
.../personal_access_tokens/expiring_worker.rb | 10 +++++-----
locale/gitlab.pot | 11 +++++++----
spec/mailers/emails/profile_spec.rb | 4 ++--
spec/services/notification_service_spec.rb | 10 +++++-----
.../expiring_worker_spec.rb | 2 +-
12 files changed, 44 insertions(+), 51 deletions(-)
create mode 100644 app/views/notify/bot_resource_access_token_about_to_expire_email.html.haml
create mode 100644 app/views/notify/bot_resource_access_token_about_to_expire_email.text.erb
delete mode 100644 app/views/notify/resource_access_tokens_about_to_expire_email.html.haml
delete mode 100644 app/views/notify/resource_access_tokens_about_to_expire_email.text.erb
diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb
index dbfbb9d684cc0..56f884bcbc293 100644
--- a/app/mailers/emails/profile.rb
+++ b/app/mailers/emails/profile.rb
@@ -60,9 +60,10 @@ def new_gpg_key_email(gpg_key_id)
end
# rubocop: enable CodeReuse/ActiveRecord
- def resource_access_tokens_about_to_expire_email(recipient, resource, token_names)
+ # 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)
@user = recipient
- @token_names = token_names
+ @token_name = token_name
@days_to_expire = PersonalAccessToken::DAYS_TO_EXPIRE
@resource = resource
if resource.is_a?(Group)
diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb
index da289cdbb3b2c..d7102fb022650 100644
--- a/app/mailers/previews/notify_preview.rb
+++ b/app/mailers/previews/notify_preview.rb
@@ -65,7 +65,7 @@ def note_merge_request_email_for_diff_discussion
end
def resource_access_token_about_to_expire_email
- Notify.resource_access_tokens_about_to_expire_email(user, group, ['token_name'])
+ Notify.bot_resource_access_token_about_to_expire_email(user, group, 'token_name')
end
def access_token_created_email
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 3c40707d0c642..d33db5c7b34ca 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -75,15 +75,15 @@ def new_gpg_key(gpg_key)
end
end
- def resource_access_tokens_about_to_expire(bot_user, token_names)
+ def bot_resource_access_token_about_to_expire(bot_user, token_name)
recipients = bot_user.resource_bot_owners.select { |owner| owner.can?(:receive_notifications) }
resource = bot_user.resource_bot_resource
recipients.each do |recipient|
- mailer.resource_access_tokens_about_to_expire_email(
+ mailer.bot_resource_access_token_about_to_expire_email(
recipient,
resource,
- token_names
+ token_name
).deliver_later
end
end
diff --git a/app/views/notify/bot_resource_access_token_about_to_expire_email.html.haml b/app/views/notify/bot_resource_access_token_about_to_expire_email.html.haml
new file mode 100644
index 0000000000000..f61082bbf761c
--- /dev/null
+++ b/app/views/notify/bot_resource_access_token_about_to_expire_email.html.haml
@@ -0,0 +1,10 @@
+%p
+ = _('Hi %{username}!') % { username: sanitize_name(@user.name) }
+%p
+ - code_tag_pair = tag_pair(tag.code, :codeOpen, :codeClose)
+ = safe_format(_('Your %{resource_type} access token %{codeOpen}%{token_name}%{codeClose} for %{codeOpen}%{resource_path}%{codeClose} will expire in %{days_to_expire} or less.'), code_tag_pair, days_to_expire: pluralize(@days_to_expire, _('day')), token_name: @token_name, resource_path: @resource.full_path, resource_type: @resource.class.name)
+%p
+ - 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 %{link_start}Access Tokens%{link_end} settings.')) % { link_start: link_start, link_end: '</a>'.html_safe }
+%p
+ = @reason_text
diff --git a/app/views/notify/bot_resource_access_token_about_to_expire_email.text.erb b/app/views/notify/bot_resource_access_token_about_to_expire_email.text.erb
new file mode 100644
index 0000000000000..00b553f02a6fb
--- /dev/null
+++ b/app/views/notify/bot_resource_access_token_about_to_expire_email.text.erb
@@ -0,0 +1,7 @@
+<%= _('Hi %{username}!') % { username: sanitize_name(@user.name) } %>
+
+<%= _('Your %{resource_type} access token %{token_name} for %{resource_path} will expire in %{days_to_expire} or less.') % { days_to_expire: pluralize(@days_to_expire, _('day')), token_name: @token_name, resource_path: "#{@resource.class.name.titleize}: #{@resource.full_path}", resource_type: "#{@resource.class.name.titleize}" }%>
+
+<%= _('You can create a new one or check them in your access token settings: %{target_url}') % { target_url: @target_url } %>
+
+<%= @reason_text %>
diff --git a/app/views/notify/resource_access_tokens_about_to_expire_email.html.haml b/app/views/notify/resource_access_tokens_about_to_expire_email.html.haml
deleted file mode 100644
index 35c2260f24fd1..0000000000000
--- a/app/views/notify/resource_access_tokens_about_to_expire_email.html.haml
+++ /dev/null
@@ -1,15 +0,0 @@
-%p
- = _('Hi %{username}!') % { username: sanitize_name(@user.name) }
-%p
- = _('One or more of your resource access tokens will expire in %{days_to_expire} or less:') % { days_to_expire: pluralize(@days_to_expire, _('day')) }
-%p
- #{@resource.class.name.titleize}: #{@resource.full_path}
-%p
- %ul
- - @token_names.each do |token|
- %li= token
-%p
- - 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 %{link_start}access tokens%{link_end} settings.')) % { link_start: link_start, link_end: '</a>'.html_safe }
-%p
- = @reason_text
diff --git a/app/views/notify/resource_access_tokens_about_to_expire_email.text.erb b/app/views/notify/resource_access_tokens_about_to_expire_email.text.erb
deleted file mode 100644
index f57c3e7b0d01a..0000000000000
--- a/app/views/notify/resource_access_tokens_about_to_expire_email.text.erb
+++ /dev/null
@@ -1,13 +0,0 @@
-<%= _('Hi %{username}!') % { username: sanitize_name(@user.name) } %>
-
-<%= _('One or more of your resource access tokens will expire in %{days_to_expire} or less:') % { days_to_expire: pluralize(@days_to_expire, _('day')) } %>
-
-<%= "#{@resource.class.name.titleize}: #{@resource.full_path}" %>
-
-<% @token_names.each do |token| %>
- - <%= token %>
-<% end %>
-
-<%= _('You can create a new one or check them in your access token settings: %{target_url}') % { target_url: @target_url } %>
-
-<%= @reason_text %>
diff --git a/app/workers/personal_access_tokens/expiring_worker.rb b/app/workers/personal_access_tokens/expiring_worker.rb
index e4c39b2ae9967..f2a11e440202f 100644
--- a/app/workers/personal_access_tokens/expiring_worker.rb
+++ b/app/workers/personal_access_tokens/expiring_worker.rb
@@ -50,7 +50,7 @@ def process_user_tokens
# 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
- deliver_user_notifications(token_names, user)
+ deliver_user_notifications(user, token_names)
expiring_user_tokens.update_all(expire_notification_delivered: true)
end
@@ -77,7 +77,7 @@ def process_project_access_tokens
bot_users.each do |user|
with_context(user: user) do
- expiring_user_token = user.personal_access_tokens.first
+ expiring_user_token = user.personal_access_tokens.first # bot user should not have more than 1 token
execute_web_hooks(expiring_user_token, user)
deliver_bot_notifications(expiring_user_token.name, user)
@@ -91,8 +91,8 @@ def process_project_access_tokens
# rubocop: enable CodeReuse/ActiveRecord
end
- def deliver_bot_notifications(token_names, user)
- notification_service.resource_access_tokens_about_to_expire(user, token_names)
+ def deliver_bot_notifications(token_name, user)
+ notification_service.bot_resource_access_token_about_to_expire(user, token_name)
Gitlab::AppLogger.info(
message: "Notifying Bot User resource owners about expiring tokens",
@@ -101,7 +101,7 @@ def deliver_bot_notifications(token_names, user)
)
end
- def deliver_user_notifications(token_names, user)
+ def deliver_user_notifications(user, token_names)
notification_service.access_token_about_to_expire(user, token_names)
Gitlab::AppLogger.info(
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 02d75676e8243..46ac702259048 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -35347,9 +35347,6 @@ msgstr ""
msgid "One or more of your personal access tokens will expire in %{days_to_expire} days or less:"
msgstr ""
-msgid "One or more of your resource access tokens will expire in %{days_to_expire} or less:"
-msgstr ""
-
msgid "Only %{workspaceType} members with %{permissions} can view or be notified about this %{issuableType}."
msgstr ""
@@ -58791,7 +58788,7 @@ msgstr ""
msgid "You can create a new SSH key by visiting %{link}"
msgstr ""
-msgid "You can create a new one or check them in your %{link_start}access tokens%{link_end} settings."
+msgid "You can create a new one or check them in your %{link_start}Access Tokens%{link_end} settings."
msgstr ""
msgid "You can create a new one or check them in your %{pat_link_start}personal access tokens%{pat_link_end} settings."
@@ -59378,6 +59375,12 @@ msgstr ""
msgid "Your %{plan} plan will be applied to your group."
msgstr ""
+msgid "Your %{resource_type} access token %{codeOpen}%{token_name}%{codeClose} for %{codeOpen}%{resource_path}%{codeClose} will expire in %{days_to_expire} or less."
+msgstr ""
+
+msgid "Your %{resource_type} access token %{token_name} for %{resource_path} will expire in %{days_to_expire} or less."
+msgstr ""
+
msgid "Your %{spammable_entity_type} has been recognized as spam. Please, change the content or solve the reCAPTCHA to proceed."
msgstr ""
diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb
index 37118a18c9473..547fe878cc34d 100644
--- a/spec/mailers/emails/profile_spec.rb
+++ b/spec/mailers/emails/profile_spec.rb
@@ -193,7 +193,7 @@
resource.add_developer(project_bot)
end
- subject { Notify.resource_access_tokens_about_to_expire_email(user, resource, [expiring_token.name]) }
+ subject { Notify.bot_resource_access_token_about_to_expire_email(user, resource, expiring_token.name) }
it_behaves_like 'an email sent from GitLab'
it_behaves_like 'it should not have Gmail Actions links'
@@ -216,7 +216,7 @@
resource.add_reporter(project_bot)
end
- subject { Notify.resource_access_tokens_about_to_expire_email(user, resource, [expiring_token.name]) }
+ subject { Notify.bot_resource_access_token_about_to_expire_email(user, resource, expiring_token.name) }
it_behaves_like 'an email sent from GitLab'
it_behaves_like 'it should not have Gmail Actions links'
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 35e84e9cdb60a..ad740c1b7dcd4 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -384,7 +384,7 @@
let_it_be(:owner2) { create(:user) }
subject(:notification_service) do
- notification.resource_access_tokens_about_to_expire(project_bot, [expiring_token.name])
+ notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token.name])
end
context 'when the resource is a group' do
@@ -402,13 +402,13 @@
owner1,
project_bot.resource_bot_resource,
[expiring_token.name],
- mail: "resource_access_tokens_about_to_expire_email"
+ mail: "bot_resource_access_token_about_to_expire_email"
).and(
have_enqueued_email(
owner2,
project_bot.resource_bot_resource,
[expiring_token.name],
- mail: "resource_access_tokens_about_to_expire_email"
+ mail: "bot_resource_access_token_about_to_expire_email"
)
)
)
@@ -430,13 +430,13 @@
owner1,
project_bot.resource_bot_resource,
[expiring_token.name],
- mail: "resource_access_tokens_about_to_expire_email"
+ mail: "bot_resource_access_token_about_to_expire_email"
).and(
have_enqueued_email(
owner2,
project_bot.resource_bot_resource,
[expiring_token.name],
- mail: "resource_access_tokens_about_to_expire_email"
+ mail: "bot_resource_access_token_about_to_expire_email"
)
)
)
diff --git a/spec/workers/personal_access_tokens/expiring_worker_spec.rb b/spec/workers/personal_access_tokens/expiring_worker_spec.rb
index 930e6ef8ff79f..6b021da623d52 100644
--- a/spec/workers/personal_access_tokens/expiring_worker_spec.rb
+++ b/spec/workers/personal_access_tokens/expiring_worker_spec.rb
@@ -8,7 +8,7 @@
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(:resource_access_tokens_about_to_expire)
+ expect(notification_service).to receive(:bot_resource_access_token_about_to_expire)
.with(project_bot, expiring_token.name)
end
--
GitLab