diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index 56f884bcbc293824e5fded6ac87e1673084ba0fb..8a0d1ea2638b6ffa1b9a306b39cb10ea1972850d 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -71,7 +71,7 @@ def bot_resource_access_token_about_to_expire_email(recipient, resource, token_n @reason_text = _('You are receiving this email because you are an Owner of the Group.') else @target_url = project_settings_access_tokens_url(resource) - @reason_text = _('You are receiving this email because you are a Maintainer of the Project.') + @reason_text = _('You are receiving this email because you are either an Owner or Maintainer of the project.') end mail_with_locale( diff --git a/app/models/concerns/has_user_type.rb b/app/models/concerns/has_user_type.rb index 6c272921351cba9bd52e5c86e8796e89345527b3..9082078182989337a821c6286bc58e332125c868 100644 --- a/app/models/concerns/has_user_type.rb +++ b/app/models/concerns/has_user_type.rb @@ -82,13 +82,13 @@ def resource_bot_resource projects&.first || groups&.first end - def resource_bot_owners + def resource_bot_owners_and_maintainers return [] unless project_bot? resource = resource_bot_resource return [] unless resource - return resource.maintainers if resource.is_a?(Project) + return resource.owners_and_maintainers if resource.is_a?(Project) resource .owners diff --git a/app/models/project.rb b/app/models/project.rb index 4065039d09a71c381603fc57ab5f502c2985b0ae..0e1c0d1c58937e4a015da7ff688b341410ab6dd4 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -370,6 +370,7 @@ def self.integration_association_name(name) 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") @@ -378,6 +379,13 @@ def self.integration_association_name(name) through: :project_members, source: :user + has_many :owners_and_maintainers, + -> do + where(members: { access_level: [Gitlab::Access::OWNER, Gitlab::Access::MAINTAINER] }) + end, + through: :project_members, + source: :user + has_many :project_callouts, class_name: 'Users::ProjectCallout', foreign_key: :project_id has_many :deploy_keys_projects, inverse_of: :project diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index d33db5c7b34ca8c80b81f9b9c0dce4c7e38d2227..e1154452f5325e195c7b3d64c5cec46a5a9ea3d3 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -76,7 +76,7 @@ def new_gpg_key(gpg_key) end def bot_resource_access_token_about_to_expire(bot_user, token_name) - recipients = bot_user.resource_bot_owners.select { |owner| owner.can?(:receive_notifications) } + recipients = bot_user.resource_bot_owners_and_maintainers.select { |user| user.can?(:receive_notifications) } resource = bot_user.resource_bot_resource recipients.each do |recipient| diff --git a/doc/security/token_overview.md b/doc/security/token_overview.md index b987cf9fe806f0f4004d707250c22ccdf30680c8..3691ffa2a1dad01b92840e2da9bba57f9432b9e8 100644 --- a/doc/security/token_overview.md +++ b/doc/security/token_overview.md @@ -59,7 +59,7 @@ 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). -Project maintainers with a direct membership receive an email when project access tokens are 7 days or less from expiration. Inherited members do not receive an email. +Project Owners and Maintainers with a direct membership receive an email when project access tokens are seven days or less from expiration. Inherited members do not receive an email. ## Group access tokens diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7853466836552dc9722d0177ff3cf9e39d41e715..b81f959069b12494f30a0540488e2e747e665512 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -60283,10 +60283,10 @@ msgstr "" msgid "You are on a read-only GitLab instance." msgstr "" -msgid "You are receiving this email because you are a Maintainer of the Project." +msgid "You are receiving this email because you are an Owner of the Group." msgstr "" -msgid "You are receiving this email because you are an Owner of the Group." +msgid "You are receiving this email because you are either an Owner or Maintainer of the project." msgstr "" msgid "You are receiving this message because you are a GitLab administrator for %{url}." diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 6380424da88d2b9ae884c9c7cb44c953ec358d15..2a7d7ca59e1d34f8396b7a0d13a977c5203c395f 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -648,6 +648,7 @@ project: - project_repository - users - maintainers +- owners_and_maintainers - requesters - namespace_members - namespace_requesters diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index 92fd507c4934721ea9b2c8a8e88929b930080e04..0b70a7693ae2be8d4856911e3874287d1065ecee 100644 --- a/spec/mailers/emails/profile_spec.rb +++ b/spec/mailers/emails/profile_spec.rb @@ -224,7 +224,7 @@ it_behaves_like 'resource about to expire email' it 'includes the email reason' do - is_expected.to have_body_text _('You are receiving this email because you are a Maintainer of the Project.') + is_expected.to have_body_text _('You are receiving this email because you are either an Owner or Maintainer of the project.') end end end diff --git a/spec/models/concerns/has_user_type_spec.rb b/spec/models/concerns/has_user_type_spec.rb index e9f8e0a487cbbd59a2c1b09960a24b582711d4a5..630212aa8580d3c42cbf0c672fa915f4297a0d57 100644 --- a/spec/models/concerns/has_user_type_spec.rb +++ b/spec/models/concerns/has_user_type_spec.rb @@ -152,7 +152,7 @@ end end - describe 'resource_bot_owners' do + describe 'resource_bot_owners_and_maintainers' do it 'returns nil when user is not a project bot' do expect(human.resource_bot_resource).to be_nil end @@ -161,10 +161,10 @@ let(:user1) { create(:user) } let(:user2) { create(:user) } - subject(:owners) { project_bot.resource_bot_owners } + subject(:owners_and_maintainers) { project_bot.resource_bot_owners_and_maintainers } it 'returns an empty array when there is no owning resource' do - expect(owners).to match_array([]) + expect(owners_and_maintainers).to match_array([]) end it 'returns group owners when owned by a group' do @@ -172,15 +172,23 @@ group.add_developer(project_bot) group.add_owner(user1) - expect(owners).to match_array([user1]) + expect(owners_and_maintainers).to match_array([user1]) end - it 'returns project maintainers when owned by a project' do + it 'returns project owners and 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]) + expect(owners_and_maintainers).to match_array([project.owner, user2]) + end + + it 'does not returns any other role than owner or maintainer' do + project = create(:project) + project.add_developer(project_bot) + project.add_maintainer(user2) + + expect(owners_and_maintainers).not_to include(project_bot) end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 37783346a27891899c6bff9394c6ea79d40b8932..8e0093b97ad8a4515762ec354144e68567ace171 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -32,6 +32,7 @@ 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(:owners_and_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') } @@ -261,6 +262,23 @@ end end + describe 'owners_and_maintainers association' do + let_it_be(:project) { create(:project) } + let_it_be(:maintainer) { create(:user) } + let_it_be(:reporter) { create(:user) } + let_it_be(:developer) { create(:user) } + + before do + project.add_maintainer(maintainer) + project.add_developer(developer) + project.add_reporter(reporter) + end + + it 'returns only maintainers and owners' do + expect(project.owners_and_maintainers).to match_array([maintainer, project.owner]) + end + end + context 'when deleting project' do # using delete rather than destroy due to `delete` skipping AR hooks/callbacks # so it's ensured to work at the DB level. Uses AFTER DELETE trigger. diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index ad740c1b7dcd4815210da9d2e32afcaf709c13b6..2dabd2c9bbf66e671174c39394903af23994e499 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -378,22 +378,24 @@ 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(:expiring_token) { "Expiring Token" } let_it_be(:owner1) { create(:user) } let_it_be(:owner2) { create(:user) } + let_it_be(:maintainer) { create(:user) } + let_it_be(:parent_group) { create(:group) } + let_it_be(:group) { create(:group, parent: parent_group) } subject(:notification_service) do - notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token.name]) + notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token]) end context 'when the resource is a group' do - let(:group) { create(:group) } - - before do + before_all do group.add_owner(owner1) group.add_owner(owner2) group.add_reporter(project_bot) + group.add_maintainer(maintainer) end it 'sends emails to the group owners' do @@ -401,46 +403,119 @@ have_enqueued_email( owner1, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ).and( have_enqueued_email( owner2, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ) ) ) end + + it 'does not send an email to group maintainer' do + expect { notification_service }.not_to( + have_enqueued_email( + maintainer, + project_bot.resource_bot_resource, + [expiring_token], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) + end + + context 'when group has inherited members' do + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:expiring_token_1) { "Expiring Token 1" } + let_it_be(:expiring_token_2) { "Expirigin Token 2" } + + subject(:notification_service) do + notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token_1, expiring_token_2]) + end + + before_all do + parent_group.add_owner(owner2) + group.add_owner(owner1) + group.add_reporter(project_bot) + end + + it 'does not send email to inherited members' do + expect { notification_service }.to( + have_enqueued_email( + owner1, + project_bot.resource_bot_resource, + [expiring_token_1, expiring_token_2], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) + + expect { notification_service }.not_to( + have_enqueued_email( + owner2, + project_bot.resource_bot_resource, + [expiring_token_1, expiring_token_2], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) + end + end end context 'when the resource is a project' do - let(:project) { create(:project) } + let_it_be(:project) { create(:project) } - before do - project.add_maintainer(owner1) - project.add_maintainer(owner2) + before_all do + project.add_maintainer(maintainer) project.add_reporter(project_bot) end - it 'sends emails to the group owners' do + it 'sends emails to the project maintainers and owners' do expect { notification_service }.to( have_enqueued_email( - owner1, + maintainer, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ).and( have_enqueued_email( - owner2, + project.owner, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ) ) ) end + + context 'when project has inherited members' do + before_all do + project.namespace = group + project.save! + end + + it 'does not send email to inherited members' do + expect { notification_service }.to( + have_enqueued_email( + maintainer, + project_bot.resource_bot_resource, + [expiring_token], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) + + expect { notification_service }.not_to( + have_enqueued_email( + project.owner, + project_bot.resource_bot_resource, + [expiring_token], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) + end + end end end