diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-09-04 06:09:19 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-09-04 06:09:19 +0300 |
commit | 03a2dce94a42ba978e60d02aeb0ee9909a0e947e (patch) | |
tree | ff0c3166e3e3ef6d91148963b0dd274cdff95411 | |
parent | 570e71a92204fd24ef8c8176702941e2d45bcfb5 (diff) |
Add latest changes from gitlab-org/gitlab@master
-rw-r--r-- | app/mailers/emails/profile.rb | 24 | ||||
-rw-r--r-- | app/mailers/previews/notify_preview.rb | 4 | ||||
-rw-r--r-- | app/models/concerns/has_user_type.rb | 17 | ||||
-rw-r--r-- | app/models/project.rb | 7 | ||||
-rw-r--r-- | app/services/notification_service.rb | 13 | ||||
-rw-r--r-- | app/views/notify/resource_access_tokens_about_to_expire_email.html.haml | 13 | ||||
-rw-r--r-- | app/views/notify/resource_access_tokens_about_to_expire_email.text.erb | 11 | ||||
-rw-r--r-- | app/workers/personal_access_tokens/expiring_worker.rb | 16 | ||||
-rw-r--r-- | doc/security/token_overview.md | 6 | ||||
-rw-r--r-- | locale/gitlab.pot | 12 | ||||
-rw-r--r-- | spec/lib/gitlab/import_export/all_models.yml | 1 | ||||
-rw-r--r-- | spec/mailers/emails/profile_spec.rb | 73 | ||||
-rw-r--r-- | spec/models/concerns/has_user_type_spec.rb | 80 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 18 | ||||
-rw-r--r-- | spec/services/notification_service_spec.rb | 68 | ||||
-rw-r--r-- | spec/workers/personal_access_tokens/expiring_worker_spec.rb | 27 |
16 files changed, 357 insertions, 33 deletions
diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index b7ba21c6f12..a9e1efbdd5d 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -2,6 +2,8 @@ module Emails module Profile + include SafeFormatHelper + def new_user_email(user_id, token = nil) @current_user = @user = User.find(user_id) @target_url = user_url(@user) @@ -58,6 +60,28 @@ module Emails end # rubocop: enable CodeReuse/ActiveRecord + def resource_access_tokens_about_to_expire_email(recipient, resource, token_names) + @user = recipient + @token_names = token_names + @days_to_expire = PersonalAccessToken::DAYS_TO_EXPIRE + @resource = resource + @target_url = if resource.is_a?(Group) + group_settings_access_tokens_url(resource) + else + project_settings_access_tokens_url(resource) + end + + mail_with_locale( + to: recipient.notification_email_or_default, + subject: subject( + safe_format( + _("Your resource access tokens will expire in %{days_to_expire} or less"), + days_to_expire: pluralize(@days_to_expire, _('day')) + ) + ) + ) + end + def access_token_created_email(user, token_name) return unless user&.active? diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index f43f4511913..638df56b770 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -64,6 +64,10 @@ class NotifyPreview < ActionMailer::Preview end end + def resource_access_token_about_to_expire_email + Notify.resource_access_tokens_about_to_expire_email(user, group, ['token_name']) + end + def access_token_created_email Notify.access_token_created_email(user, 'token_name').message end diff --git a/app/models/concerns/has_user_type.rb b/app/models/concerns/has_user_type.rb index 2d0ff82e624..c3f702a4e69 100644 --- a/app/models/concerns/has_user_type.rb +++ b/app/models/concerns/has_user_type.rb @@ -74,4 +74,21 @@ module HasUserType # https://gitlab.com/gitlab-org/gitlab/-/issues/346058 '****' end + + def resource_bot_resource + return unless project_bot? + + projects&.first || groups&.first + end + + def resource_bot_owners + return [] unless project_bot? + + resource = resource_bot_resource + return [] unless resource + + return resource.maintainers if resource.is_a?(Project) + + resource.owners + end end diff --git a/app/models/project.rb b/app/models/project.rb index a1cef322983..cb1468a8d0c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -339,6 +339,13 @@ class Project < ApplicationRecord has_many :users, -> { allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/422405") }, through: :project_members + has_many :maintainers, + -> do + allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/422405") + .where(members: { access_level: Gitlab::Access::MAINTAINER }) + end, + through: :project_members, + source: :user has_many :project_callouts, class_name: 'Users::ProjectCallout', foreign_key: :project_id diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 2832dfc340f..f1781b3d3c5 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -75,6 +75,19 @@ class NotificationService end end + def resource_access_tokens_about_to_expire(bot_user, token_names) + 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( + recipient, + resource, + token_names + ).deliver_later + end + end + # Notify the owner of the account when a new personal access token is created def access_token_created(user, token_name) return unless user.can?(:receive_notifications) 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 new file mode 100644 index 00000000000..e4e34f6c8ee --- /dev/null +++ b/app/views/notify/resource_access_tokens_about_to_expire_email.html.haml @@ -0,0 +1,13 @@ +%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 } 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 new file mode 100644 index 00000000000..bea74f09129 --- /dev/null +++ b/app/views/notify/resource_access_tokens_about_to_expire_email.text.erb @@ -0,0 +1,11 @@ +<%= _('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 } %> diff --git a/app/workers/personal_access_tokens/expiring_worker.rb b/app/workers/personal_access_tokens/expiring_worker.rb index de0bda82573..5f8316d184d 100644 --- a/app/workers/personal_access_tokens/expiring_worker.rb +++ b/app/workers/personal_access_tokens/expiring_worker.rb @@ -29,9 +29,21 @@ module PersonalAccessTokens # rubocop: enable CodeReuse/ActiveRecord - notification_service.access_token_about_to_expire(user, token_names) + message = if user.project_bot? + notification_service.resource_access_tokens_about_to_expire(user, token_names) - Gitlab::AppLogger.info "#{self.class}: Notifying User #{user.id} about expiring tokens" + "Notifying Bot User resource owners about expiring tokens" + else + notification_service.access_token_about_to_expire(user, token_names) + + "Notifying User about expiring tokens" + end + + Gitlab::AppLogger.info( + message: message, + class: self.class, + user_id: user.id + ) expiring_user_tokens.each_batch do |expiring_tokens| expiring_tokens.update_all(expire_notification_delivered: true) diff --git a/doc/security/token_overview.md b/doc/security/token_overview.md index bceb10023a4..ecf4b8feb7c 100644 --- a/doc/security/token_overview.md +++ b/doc/security/token_overview.md @@ -24,6 +24,8 @@ You can use the [personal access tokens API](../api/personal_access_tokens.md) t programmatically take action, such as [rotating a personal access token](../api/personal_access_tokens.md#rotate-a-personal-access-token). +You will receive an email when personal access tokens are 7 days or less from expiration. + ## OAuth2 tokens GitLab can serve as an [OAuth2 provider](../api/oauth2.md) to allow other services to access the GitLab API on a user's behalf. @@ -55,6 +57,8 @@ You can use the [project access tokens API](../api/project_access_tokens.md) to programmatically take action, such as [rotating a project access token](../api/project_access_tokens.md#rotate-a-project-access-token). +All project maintainers receive an email when project access tokens are 7 days or less from expiration. + ## Group access tokens [Group access tokens](../user/group/settings/group_access_tokens.md#group-access-tokens) @@ -72,6 +76,8 @@ You can use the [group access tokens API](../api/group_access_tokens.md) to programmatically take action, such as [rotating a group access token](../api/group_access_tokens.md#rotate-a-group-access-token). +All group owners receive an email when group access tokens are 7 days or less from expiration. + ## Deploy tokens [Deploy tokens](../user/project/deploy_tokens/index.md) allow you to download (`git clone`) or push and pull packages and container registry images of a project without having a user and a password. Deploy tokens cannot be used with the GitLab API. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6e13f2d2e90..4732a31a12d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -32792,6 +32792,9 @@ 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 "" @@ -54142,6 +54145,9 @@ 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." +msgstr "" + msgid "You can create a new one or check them in your %{pat_link_start}personal access tokens%{pat_link_end} settings." msgstr "" @@ -54151,6 +54157,9 @@ msgstr "" msgid "You can create a new one or check them in your SSH keys settings %{ssh_key_link}." msgstr "" +msgid "You can create a new one or check them in your access token settings: %{target_url}" +msgstr "" + msgid "You can create a new one or check them in your personal access tokens settings %{pat_link}." msgstr "" @@ -54988,6 +54997,9 @@ msgstr "" msgid "Your requirements will be imported in the background. After it's finished, you'll get a confirmation email." msgstr "" +msgid "Your resource access tokens will expire in %{days_to_expire} or less" +msgstr "" + msgid "Your search didn't match any commits." msgstr "" diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index b1b438fa592..99e871e5255 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -621,6 +621,7 @@ project: - project_members - project_repository - users +- maintainers - requesters - namespace_members - namespace_requesters diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index 140b067f7aa..4816e88a311 100644 --- a/spec/mailers/emails/profile_spec.rb +++ b/spec/mailers/emails/profile_spec.rb @@ -157,42 +157,67 @@ RSpec.describe Emails::Profile, feature_category: :user_profile do end end - describe 'user personal access token is about to expire' do + describe 'resource 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, [expiring_token.name]) } + shared_examples 'resource about to expire email' do + it 'is sent to the owners' do + is_expected.to deliver_to user + end - 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 'has the correct subject' do + is_expected.to have_subject /^Your resource access tokens will expire in 7 days or less$/i + end - it 'is sent to the user' do - is_expected.to deliver_to user.email - end + it 'includes a link to access tokens page' do + is_expected.to have_body_text /#{resource_access_tokens_path}/ + 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 'provides the names of expiring tokens' do + is_expected.to have_body_text /#{expiring_token.name}/ + 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/ + it 'includes the email reason' do + is_expected.to have_body_text %r{You're receiving this email because of your account on <a .*>localhost</a>} + end end - it 'provides the names of expiring tokens' do - is_expected.to have_body_text /#{expiring_token.name}/ - end + context 'when access token belongs to a group' do + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:expiring_token) { create(:personal_access_token, user: project_bot, expires_at: 5.days.from_now) } + let_it_be(:resource) { create(:group) } + let_it_be(:resource_access_tokens_path) { group_settings_access_tokens_path(resource) } - it 'includes a link to personal access tokens page' do - is_expected.to have_body_text /#{profile_personal_access_tokens_path}/ - end + before_all do + resource.add_owner(user) + resource.add_developer(project_bot) + end - it 'includes the email reason' do - is_expected.to have_body_text %r{You're receiving this email because of your account on <a .*>localhost</a>} + subject { Notify.resource_access_tokens_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' + it_behaves_like 'a user cannot unsubscribe through footer link' + it_behaves_like 'resource about to expire email' end - context 'with User does not exist' do - it { expect { Notify.access_token_about_to_expire_email('foo') }.not_to raise_error } + context 'when access token belongs to a project' do + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:expiring_token) { create(:personal_access_token, user: project_bot, expires_at: 5.days.from_now) } + let_it_be(:resource) { create(:project) } + let_it_be(:resource_access_tokens_path) { project_settings_access_tokens_path(resource) } + + before_all do + resource.add_maintainer(user) + resource.add_reporter(project_bot) + end + + subject { Notify.resource_access_tokens_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' + it_behaves_like 'a user cannot unsubscribe through footer link' + it_behaves_like 'resource about to expire email' end end diff --git a/spec/models/concerns/has_user_type_spec.rb b/spec/models/concerns/has_user_type_spec.rb index 49c3d11ed6b..54614ec2b21 100644 --- a/spec/models/concerns/has_user_type_spec.rb +++ b/spec/models/concerns/has_user_type_spec.rb @@ -3,6 +3,13 @@ require 'spec_helper' RSpec.describe User, feature_category: :system_access do + User::USER_TYPES.keys.each do |type| # rubocop:disable RSpec/UselessDynamicDefinition + let_it_be(type) { create(:user, username: type, user_type: type) } + end + let(:bots) { User::BOT_USER_TYPES.map { |type| public_send(type) } } + let(:non_internal) { User::NON_INTERNAL_USER_TYPES.map { |type| public_send(type) } } + let(:everyone) { User::USER_TYPES.keys.map { |type| public_send(type) } } + specify 'types consistency checks', :aggregate_failures do expect(described_class::USER_TYPES.keys) .to match_array(%w[human ghost alert_bot project_bot support_bot service_user security_bot @@ -20,13 +27,6 @@ RSpec.describe User, feature_category: :system_access do end describe 'scopes & predicates' do - User::USER_TYPES.keys.each do |type| # rubocop:disable RSpec/UselessDynamicDefinition - let_it_be(type) { create(:user, username: type, user_type: type) } - end - let(:bots) { User::BOT_USER_TYPES.map { |type| public_send(type) } } - let(:non_internal) { User::NON_INTERNAL_USER_TYPES.map { |type| public_send(type) } } - let(:everyone) { User::USER_TYPES.keys.map { |type| public_send(type) } } - describe '.bots' do it 'includes all bots' do expect(described_class.bots).to match_array(bots) @@ -118,5 +118,71 @@ RSpec.describe User, feature_category: :system_access do end end end + + describe '#resource_bot_resource' do + let_it_be(:group) { create(:group) } + let_it_be(:group2) { create(:group) } + let_it_be(:project) { create(:project) } + let_it_be(:project2) { create(:project) } + + using RSpec::Parameterized::TableSyntax + + where(:bot_user, :member_of, :owning_resource) do + ref(:human) | [ref(:group)] | nil + ref(:project_bot) | [] | nil # orphaned project bot + ref(:project_bot) | [ref(:group)] | ref(:group) + ref(:project_bot) | [ref(:project)] | ref(:project) + + # Project bot can only be added to one group or project. + # That first group or project becomes the owning resource. + ref(:project_bot) | [ref(:group), ref(:project)] | ref(:group) + ref(:project_bot) | [ref(:group), ref(:group2)] | ref(:group) + ref(:project_bot) | [ref(:project), ref(:group)] | ref(:project) + ref(:project_bot) | [ref(:project), ref(:project2)] | ref(:project) + end + + with_them do + before do + member_of.each { |resource| resource.add_developer(bot_user) } + end + + it 'returns the owning resource' do + expect(bot_user.resource_bot_resource).to eq(owning_resource) + end + end + end + + describe 'resource_bot_owners' do + it 'returns nil when user is not a project bot' do + expect(human.resource_bot_resource).to be_nil + end + + context 'when the user is a project bot' do + let(:user1) { create(:user) } + let(:user2) { create(:user) } + + subject(:owners) { project_bot.resource_bot_owners } + + it 'returns an empty array when there is no owning resource' do + expect(owners).to match_array([]) + end + + it 'returns group owners when owned by a group' do + group = create(:group) + group.add_developer(project_bot) + group.add_owner(user1) + + expect(owners).to match_array([user1]) + end + + it 'returns project maintainers when owned by a project' do + project = create(:project) + project.add_developer(project_bot) + project.add_maintainer(user2) + + expect(owners).to match_array([user2]) + end + end + end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 091f61768d9..657c7d5dee8 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -23,6 +23,7 @@ RSpec.describe Project, factory_default: :keep, feature_category: :groups_and_pr it { is_expected.to belong_to(:creator).class_name('User') } it { is_expected.to belong_to(:pool_repository) } it { is_expected.to have_many(:users) } + it { is_expected.to have_many(:maintainers).through(:project_members).source(:user).conditions(members: { access_level: Gitlab::Access::MAINTAINER }) } it { is_expected.to have_many(:events) } it { is_expected.to have_many(:merge_requests) } it { is_expected.to have_many(:merge_request_metrics).class_name('MergeRequest::Metrics') } @@ -223,6 +224,23 @@ RSpec.describe Project, factory_default: :keep, feature_category: :groups_and_pr expect(project.lfs_objects.to_a).to eql([lfs_object]) end + describe 'maintainers association' do + let_it_be(:project) { create(:project) } + let_it_be(:maintainer1) { create(:user) } + let_it_be(:maintainer2) { create(:user) } + let_it_be(:reporter) { create(:user) } + + before do + project.add_maintainer(maintainer1) + project.add_maintainer(maintainer2) + project.add_reporter(reporter) + end + + it 'returns only maintainers' do + expect(project.maintainers).to match_array([maintainer1, maintainer2]) + end + end + context 'after initialized' do it "has a project_feature" do expect(described_class.new.project_feature).to be_present diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index a10ace9466e..40597c30c4a 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -376,6 +376,74 @@ RSpec.describe NotificationService, :mailer, feature_category: :team_planning do end end + describe '#resource_access_token_about_to_expire' do + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:expiring_token) { create(:personal_access_token, user: project_bot, expires_at: 5.days.from_now) } + + let_it_be(:owner1) { create(:user) } + let_it_be(:owner2) { create(:user) } + + subject(:notification_service) do + notification.resource_access_tokens_about_to_expire(project_bot, [expiring_token.name]) + end + + context 'when the resource is a group' do + let(:group) { create(:group) } + + before do + group.add_owner(owner1) + group.add_owner(owner2) + group.add_reporter(project_bot) + end + + it 'sends emails to the group owners' do + expect { notification_service }.to( + have_enqueued_email( + owner1, + project_bot.resource_bot_resource, + [expiring_token.name], + mail: "resource_access_tokens_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" + ) + ) + ) + end + end + + context 'when the resource is a project' do + let(:project) { create(:project) } + + before do + project.add_maintainer(owner1) + project.add_maintainer(owner2) + project.add_reporter(project_bot) + end + + it 'sends emails to the group owners' do + expect { notification_service }.to( + have_enqueued_email( + owner1, + project_bot.resource_bot_resource, + [expiring_token.name], + mail: "resource_access_tokens_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" + ) + ) + ) + end + end + end + 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) } diff --git a/spec/workers/personal_access_tokens/expiring_worker_spec.rb b/spec/workers/personal_access_tokens/expiring_worker_spec.rb index 01ce4e85fe2..0cc63fdb85e 100644 --- a/spec/workers/personal_access_tokens/expiring_worker_spec.rb +++ b/spec/workers/personal_access_tokens/expiring_worker_spec.rb @@ -58,5 +58,32 @@ RSpec.describe PersonalAccessTokens::ExpiringWorker, type: :worker, feature_cate expect { worker.perform }.not_to change { pat.reload.expire_notification_delivered } end end + + context 'when a token is owned by a project bot' do + let_it_be(:maintainer1) { create(:user) } + let_it_be(:maintainer2) { create(:user) } + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:project) { create(:project) } + let_it_be(:expiring_token) { create(:personal_access_token, user: project_bot, expires_at: 5.days.from_now) } + + before_all do + project.add_developer(project_bot) + project.add_maintainer(maintainer1) + project.add_maintainer(maintainer2) + end + + 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) + .with(project_bot, match_array([expiring_token.name])) + end + + worker.perform + end + + it 'marks the notification as delivered' do + expect { worker.perform }.to change { expiring_token.reload.expire_notification_delivered }.from(false).to(true) + end + end end end |