From fbe9afe952c8624e1960d11f69d93ce466f0c318 Mon Sep 17 00:00:00 2001 From: Arpit Gogia <12347103-arpitgogia@users.noreply.gitlab.com> Date: Mon, 26 Sep 2022 18:03:14 +0530 Subject: [PATCH 1/6] Send email notification when a personal access token is revoked Currently PAT revocation happens silently, this patch adds a notification email to be sent when revocation happens Changelog: added --- app/mailers/emails/profile.rb | 12 ++++++++ app/services/notification_service.rb | 6 ++++ .../personal_access_tokens/revoke_service.rb | 3 +- .../access_token_revoked_email.html.haml | 7 +++++ .../access_token_revoked_email.text.erb | 5 ++++ spec/mailers/emails/profile_spec.rb | 29 +++++++++++++++++++ spec/services/notification_service_spec.rb | 21 ++++++++++++++ 7 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 app/views/notify/access_token_revoked_email.html.haml create mode 100644 app/views/notify/access_token_revoked_email.text.erb diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index 8fe471a48f2a70..2450dcde842824 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -94,6 +94,18 @@ def access_token_expired_email(user) end end + def access_token_revoked_email(user, token_name) + return unless user&.active? + + @user = user + @token_name = token_name + @target_url = profile_personal_access_tokens_url + + Gitlab::I18n.with_locale(@user.preferred_language) do + mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("A personal access token has been revoked"))) + end + end + def ssh_key_expired_email(user, fingerprints) return unless user&.active? diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 5a92adfd25a023..33f79e5581f31c 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -87,6 +87,12 @@ def access_token_expired(user) mailer.access_token_expired_email(user).deliver_later end + def access_token_revoked(user, token_name) + return unless user.can?(:receive_notifications) + + mailer.access_token_revoked_email(user, token_name).deliver_later + end + # Notify the user when at least one of their ssh key has expired today def ssh_key_expired(user, fingerprints) return unless user.can?(:receive_notifications) diff --git a/app/services/personal_access_tokens/revoke_service.rb b/app/services/personal_access_tokens/revoke_service.rb index 0275d03bcc9275..732da75da3a1a0 100644 --- a/app/services/personal_access_tokens/revoke_service.rb +++ b/app/services/personal_access_tokens/revoke_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module PersonalAccessTokens - class RevokeService + class RevokeService < BaseService attr_reader :token, :current_user, :group def initialize(current_user = nil, token: nil, group: nil ) @@ -15,6 +15,7 @@ def execute if token.revoke! log_event + notification_service.access_token_revoked(token.user, token.name) ServiceResponse.success(message: success_message) else ServiceResponse.error(message: error_message) diff --git a/app/views/notify/access_token_revoked_email.html.haml b/app/views/notify/access_token_revoked_email.html.haml new file mode 100644 index 00000000000000..604df33b2445b8 --- /dev/null +++ b/app/views/notify/access_token_revoked_email.html.haml @@ -0,0 +1,7 @@ +%p + = _('Hi %{username}!') % { username: sanitize_name(@user.name) } +%p + = html_escape(_('A personal access token, named %{token_name}, has been revoked.')) % { token_name: @token_name } +%p + - pat_link_start = ''.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: ''.html_safe } diff --git a/app/views/notify/access_token_revoked_email.text.erb b/app/views/notify/access_token_revoked_email.text.erb new file mode 100644 index 00000000000000..7a314128a105e2 --- /dev/null +++ b/app/views/notify/access_token_revoked_email.text.erb @@ -0,0 +1,5 @@ +<%= _('Hi %{username}!') % { username: sanitize_name(@user.name) } %> + +<%= _('A personal access token, named %{token_name}, has been revoked.') % { token_name: @token_name } %> + +<%= _('You can create a new one or check them in your personal access tokens settings %{pat_link}.') % { pat_link: @target_url } %> diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index fce552569223d4..8ff47e7feb5c30 100644 --- a/spec/mailers/emails/profile_spec.rb +++ b/spec/mailers/emails/profile_spec.rb @@ -246,6 +246,35 @@ end end + describe 'user personal access token has been revoked' do + let_it_be(:user) { create(:user) } + let_it_be(:token) { create(:personal_access_token, user: user) } + + context 'when valid' do + subject { Notify.access_token_revoked_email(user, token.name) } + + it_behaves_like 'an email sent from GitLab' + it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user cannot unsubscribe through footer link' + + it 'is sent to the user' do + is_expected.to deliver_to user.email + end + + it 'has the correct subject' do + is_expected.to have_subject /^A personal access token has been revoked$/i + end + + it 'provides the names of the token' do + is_expected.to have_body_text /#{token.name}/ + end + + it 'includes the email reason' do + is_expected.to have_body_text %r{You're receiving this email because of your account on localhost<\/a>} + end + end + end + describe 'SSH key notification' do let_it_be_with_reload(:user) { create(:user) } let_it_be(:fingerprints) { ["aa:bb:cc:dd:ee:zz"] } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 935dcef1011c5a..8fbf023cda072c 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -337,6 +337,27 @@ end end end + + describe '#access_token_revoked' do + let_it_be(:user) { create(:user) } + let_it_be(:pat) { create(:personal_access_token, user: user) } + + subject(:notification_service) { notification.access_token_revoked(user, pat.name) } + + it 'sends email to the token owner' do + expect { notification_service }.to have_enqueued_email(user, pat.name, mail: "access_token_revoked_email") + end + + context 'when user is not allowed to receive notifications' do + before do + user.block! + end + + it 'does not send email to the token owner' do + expect { notification_service }.not_to have_enqueued_email(user, pat.name, mail: "access_token_revoked_email") + end + end + end end describe 'SSH Keys' do -- GitLab From a67e646badaccfe48c68061d98364813c360673a Mon Sep 17 00:00:00 2001 From: Arpit Gogia <12347103-arpitgogia@users.noreply.gitlab.com> Date: Mon, 26 Sep 2022 18:38:57 +0530 Subject: [PATCH 2/6] Update gitlab.pot file --- locale/gitlab.pot | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 65564459552e71..e27d3f221d2f9f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1695,6 +1695,12 @@ msgstr "" msgid "A page with that title already exists" msgstr "" +msgid "A personal access token has been revoked" +msgstr "" + +msgid "A personal access token, named %{token_name}, has been revoked." +msgstr "" + msgid "A plain HTML site that uses Netlify for CI/CD instead of GitLab, but still with all the other great GitLab features" msgstr "" -- GitLab From 819b3c71ba99021ffb503e3d37139838a08f000f Mon Sep 17 00:00:00 2001 From: Arpit Gogia <12347103-arpitgogia@users.noreply.gitlab.com> Date: Wed, 28 Sep 2022 13:56:20 +0530 Subject: [PATCH 3/6] Wrap token name in code formatting --- app/views/notify/access_token_revoked_email.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/notify/access_token_revoked_email.html.haml b/app/views/notify/access_token_revoked_email.html.haml index 604df33b2445b8..5c485eadaccee3 100644 --- a/app/views/notify/access_token_revoked_email.html.haml +++ b/app/views/notify/access_token_revoked_email.html.haml @@ -1,7 +1,7 @@ %p = _('Hi %{username}!') % { username: sanitize_name(@user.name) } %p - = html_escape(_('A personal access token, named %{token_name}, has been revoked.')) % { token_name: @token_name } + = html_escape(_('A personal access token, named %{code_start}%{token_name}%{code_end}, has been revoked.')) % { code_start: ''.html_safe, token_name: @token_name, code_end: ''.html_safe } %p - pat_link_start = ''.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: ''.html_safe } -- GitLab From 396558a74ac6f03c789052c4fd2c1631180006ca Mon Sep 17 00:00:00 2001 From: Arpit Gogia <12347103-arpitgogia@users.noreply.gitlab.com> Date: Wed, 28 Sep 2022 14:07:49 +0530 Subject: [PATCH 4/6] Fix grammatical ambiguity --- app/views/notify/access_token_revoked_email.html.haml | 2 +- app/views/notify/access_token_revoked_email.text.erb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/notify/access_token_revoked_email.html.haml b/app/views/notify/access_token_revoked_email.html.haml index 5c485eadaccee3..4d9b9e14d145b2 100644 --- a/app/views/notify/access_token_revoked_email.html.haml +++ b/app/views/notify/access_token_revoked_email.html.haml @@ -4,4 +4,4 @@ = html_escape(_('A personal access token, named %{code_start}%{token_name}%{code_end}, has been revoked.')) % { code_start: ''.html_safe, token_name: @token_name, code_end: ''.html_safe } %p - pat_link_start = ''.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: ''.html_safe } + = html_escape(_('You can check your tokens or create a new one in your %{pat_link_start}personal access tokens settings%{pat_link_end}.')) % { pat_link_start: pat_link_start, pat_link_end: ''.html_safe } diff --git a/app/views/notify/access_token_revoked_email.text.erb b/app/views/notify/access_token_revoked_email.text.erb index 7a314128a105e2..17dd628d76c54c 100644 --- a/app/views/notify/access_token_revoked_email.text.erb +++ b/app/views/notify/access_token_revoked_email.text.erb @@ -2,4 +2,4 @@ <%= _('A personal access token, named %{token_name}, has been revoked.') % { token_name: @token_name } %> -<%= _('You can create a new one or check them in your personal access tokens settings %{pat_link}.') % { pat_link: @target_url } %> +<%= _('You can check your tokens or create a new one in your personal access tokens settings %{pat_link}.') % { pat_link: @target_url } %> -- GitLab From a289b5ba2d870865b4c59ecc754e56a651ba6724 Mon Sep 17 00:00:00 2001 From: Arpit Gogia <12347103-arpitgogia@users.noreply.gitlab.com> Date: Wed, 28 Sep 2022 17:10:26 +0530 Subject: [PATCH 5/6] Add method intent as a comment --- app/services/notification_service.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 72deb6750e3598..1224cf80b76c4a 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -87,6 +87,7 @@ def access_token_expired(user) mailer.access_token_expired_email(user).deliver_later end + # Notify the user when one of their personal access tokens is revoked def access_token_revoked(user, token_name) return unless user.can?(:receive_notifications) -- GitLab From cda1508fd7d5c0d77a6916e22616700d4fcb73b2 Mon Sep 17 00:00:00 2001 From: Arpit Gogia <12347103-arpitgogia@users.noreply.gitlab.com> Date: Wed, 28 Sep 2022 18:21:55 +0530 Subject: [PATCH 6/6] Forgot to update the pot file --- locale/gitlab.pot | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 639d114dcd71d5..37c97f64ce9148 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1698,6 +1698,9 @@ msgstr "" msgid "A personal access token has been revoked" msgstr "" +msgid "A personal access token, named %{code_start}%{token_name}%{code_end}, has been revoked." +msgstr "" + msgid "A personal access token, named %{token_name}, has been revoked." msgstr "" @@ -45647,6 +45650,12 @@ msgstr "" msgid "You can check it in your in your personal access tokens settings %{pat_link}." msgstr "" +msgid "You can check your tokens or create a new one in your %{pat_link_start}personal access tokens settings%{pat_link_end}." +msgstr "" + +msgid "You can check your tokens or create a new one in your personal access tokens settings %{pat_link}." +msgstr "" + msgid "You can create a new %{link}." msgstr "" -- GitLab