diff --git a/app/mailers/emails/members.rb b/app/mailers/emails/members.rb index 7743a3dd7f8a2bc30250a0b22846c097589b40ab..5541781fa5954b041f296f81489815ff75686808 100644 --- a/app/mailers/emails/members.rb +++ b/app/mailers/emails/members.rb @@ -35,21 +35,6 @@ def member_access_granted_email(member_source_type, member_id) subject: subject("Access to the #{member_source.human_name} #{member_source.model_name.singular} was granted")) end - def member_access_denied_email(member_source_type, source_id, user_id) - @member_source_type = member_source_type - @member_source = member_source_class.find(source_id) - - user = User.find(user_id) - - @source_hidden = !member_source.readable_by?(user) - - human_name = @source_hidden ? 'Hidden' : member_source.human_name - - email_with_layout( - to: user.notification_email_for(notification_group), - subject: subject("Access to the #{human_name} #{member_source.model_name.singular} was denied")) - end - def member_invite_accepted_email(member_source_type, member_id) @member_source_type = member_source_type @member_id = member_id diff --git a/app/mailers/members/access_denied_mailer.rb b/app/mailers/members/access_denied_mailer.rb new file mode 100644 index 0000000000000000000000000000000000000000..a08ddb9b31d91121189df118bafe25292d2f25b1 --- /dev/null +++ b/app/mailers/members/access_denied_mailer.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Members + class AccessDeniedMailer < ApplicationMailer + helper EmailsHelper + + helper_method :member_source, :source_hidden? + + layout 'mailer' + + def email + return unless member.notifiable?(:subscription) + + mail_with_locale( + to: user.notification_email_for(member_source.notification_group), + subject: EmailsHelper.subject_with_suffix([email_subject_text]) + ) + end + + private + + delegate :source, to: :member, prefix: true + delegate :user, to: :member + + def source_hidden? + !member_source.readable_by?(user) + end + + def member + params[:member] + end + + def email_subject_text + human_name = source_hidden? ? 'Hidden' : member_source.human_name + + "Access to the #{human_name} #{member_source.model_name.singular} was denied" + end + end +end diff --git a/app/mailers/previews/members/access_denied_mailer_preview.rb b/app/mailers/previews/members/access_denied_mailer_preview.rb new file mode 100644 index 0000000000000000000000000000000000000000..eb69a2304bd8f99bf118b80771b7bfd7135316a8 --- /dev/null +++ b/app/mailers/previews/members/access_denied_mailer_preview.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Members + class AccessDeniedMailerPreview < ActionMailer::Preview + def public_source_email + Members::AccessDeniedMailer.with(member: public_member).email.message # rubocop:disable CodeReuse/ActiveRecord -- false positive + end + + def private_source_email + Members::AccessDeniedMailer.with(member: private_member).email.message # rubocop:disable CodeReuse/ActiveRecord -- false positive + end + + private + + def public_member + # may need to create one if this doesn't find any + member(Group.public_only) + end + + def private_member + # may need to create one if this doesn't find any + member(Group.private_only) + end + + def member(scope) + scope.with_request_group_members.last.request_group_members.last + end + end +end diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index 091daaff1a73e2d02d88635932b08c8fac6c9af8..43b357a75d5d9af1e35e34116fde32f371d5f3ec 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -160,10 +160,6 @@ def changed_milestone_merge_request_email Notify.changed_milestone_merge_request_email(user.id, merge_request.id, milestone, user.id) end - def member_access_denied_email - Notify.member_access_denied_email('project', project.id, user.id).message - end - def member_access_granted_email Notify.member_access_granted_email(member.source_type, member.id).message end @@ -469,7 +465,7 @@ def group end def member - @member ||= Member.non_invite.last + @member ||= Member.non_invite.non_request.last end def key diff --git a/app/models/group.rb b/app/models/group.rb index dca2f0020ee10b42a1bb90a39886bcd43d6fd9bb..7358dafe96c824120c6868caa8cc8840e7c134ba 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -39,6 +39,10 @@ def self.supported_keyset_orderings has_many :all_owner_members, -> { non_request.all_owners }, as: :source, class_name: 'GroupMember' has_many :group_members, -> { non_request.non_minimal_access }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent has_many :non_invite_group_members, -> { non_request.non_minimal_access.non_invite }, class_name: 'GroupMember', as: :source + has_many :request_group_members, -> do + request.non_minimal_access + end, inverse_of: :group, class_name: 'GroupMember', as: :source + has_many :namespace_members, -> { non_request.non_minimal_access.unscope(where: %i[source_id source_type]) }, foreign_key: :member_namespace_id, inverse_of: :group, class_name: 'GroupMember' alias_method :members, :group_members @@ -184,6 +188,7 @@ def of_ancestors_and_self scope :with_non_archived_projects, -> { includes(:non_archived_projects) } scope :with_non_invite_group_members, -> { includes(:non_invite_group_members) } + scope :with_request_group_members, -> { includes(:request_group_members) } scope :by_id, ->(groups) { where(id: groups) } @@ -1007,6 +1012,10 @@ def readme_project end strong_memoize_attr :readme_project + def notification_group + self + end + def group_readme readme_project&.repository&.readme end diff --git a/app/models/project.rb b/app/models/project.rb index e9559392182256e2ca5d31258ca6ff8b73e2e2f1..54658c06c2d19a8c115391cca7eac346f609a54a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -189,6 +189,7 @@ class Project < ApplicationRecord belongs_to :creator, class_name: 'User' belongs_to :organization, class_name: 'Organizations::Organization' belongs_to :group, -> { where(type: Group.sti_name) }, foreign_key: 'namespace_id' + alias_method :notification_group, :group belongs_to :namespace # Sync deletion via DB Trigger to ensure we do not have # a project without a project_namespace (or vice-versa) diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index b1e77048603e0a67430745de4c6b3c2d81fa5a8a..47f059754f9571c5542c09b2eae8540c6e850a19 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -91,7 +91,7 @@ def a_group_owner?(member) def delete_member_associations(member, skip_subresources, skip_saml_identity) if member.request? && member.user != current_user - notification_service.decline_access_request(member) + Members::AccessDeniedMailer.with(member: member).email.deliver_later # rubocop:disable CodeReuse/ActiveRecord -- false positive end delete_subresources(member) unless skip_subresources diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 0155d009722e9be0977c56e3cd21f3728da78aab..eeba7274d25deaaa5abdeba35c96d1fd745594db 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -512,12 +512,6 @@ def new_access_request(member) recipients.each { |recipient| deliver_access_request_email(recipient, member) } end - def decline_access_request(member) - return true unless member.notifiable?(:subscription) - - mailer.member_access_denied_email(member.real_source_type, member.source_id, member.user_id).deliver_later - end - def decline_invite(member) # Must always send, regardless of project/namespace configuration since it's a # response to the user's action. diff --git a/app/views/notify/member_access_denied_email.html.haml b/app/views/members/access_denied_mailer/email.html.haml similarity index 87% rename from app/views/notify/member_access_denied_email.html.haml rename to app/views/members/access_denied_mailer/email.html.haml index 98d3daf210739d38afbee8f7b3e009f33804fdef..387ae03c37ce33e8c5d24bf1684b9429ffca0f42 100644 --- a/app/views/notify/member_access_denied_email.html.haml +++ b/app/views/members/access_denied_mailer/email.html.haml @@ -1,7 +1,7 @@ %tr %td.text-content %p - - target_to_join = @source_hidden ? content_tag(:span, _('Hidden'), class: :highlight) : link_to(member_source.human_name, member_source.web_url, class: :highlight) + - target_to_join = source_hidden? ? content_tag(:span, _('Hidden'), class: :highlight) : link_to(member_source.human_name, member_source.web_url, class: :highlight) - denied_tag = content_tag :span, _('denied'), class: :highlight = s_('Notify|Your request to join the %{target_to_join} %{target_type} has been %{denied_tag}.').html_safe % { target_to_join: target_to_join, target_type: member_source.model_name.singular, denied_tag: denied_tag } diff --git a/app/views/members/access_denied_mailer/email.text.erb b/app/views/members/access_denied_mailer/email.text.erb new file mode 100644 index 0000000000000000000000000000000000000000..29110afcd5f227016444b66561a94343fad96858 --- /dev/null +++ b/app/views/members/access_denied_mailer/email.text.erb @@ -0,0 +1,3 @@ +Your request to join the <%= source_hidden? ? 'Hidden' : member_source.human_name %> <%= member_source.model_name.singular %> has been denied. + +<%= member_source.web_url unless source_hidden? %> diff --git a/app/views/notify/member_access_denied_email.text.erb b/app/views/notify/member_access_denied_email.text.erb deleted file mode 100644 index 87f2ef817eee25ba18e61fbba454ff2c11a062ec..0000000000000000000000000000000000000000 --- a/app/views/notify/member_access_denied_email.text.erb +++ /dev/null @@ -1,3 +0,0 @@ -Your request to join the <%= member_source.human_name %> <%= member_source.model_name.singular %> has been denied. - -<%= member_source.web_url %> diff --git a/spec/mailers/members/access_denied_mailer_spec.rb b/spec/mailers/members/access_denied_mailer_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..61562fe8503b4fa0b26b2e00a443aadccf9c6c91 --- /dev/null +++ b/spec/mailers/members/access_denied_mailer_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::AccessDeniedMailer, feature_category: :groups_and_projects do + include EmailSpec::Matchers + using RSpec::Parameterized::TableSyntax + + describe '#email' do + let(:recipient) { build(:recipient) } + let(:group) { build(:group) } + let(:group_member) { build(:group_member, source: group, user: recipient) } + let(:project) { build(:project, :public, group: group) } + let(:project_member) { build(:project_member, source: project, user: recipient) } + let(:notifiable) { true } + + before do + allow(member).to receive(:notifiable?).with(:subscription).and_return(notifiable) + end + + subject(:email) { described_class.with(member: member).email } + + where(:member, :source, :type) do + ref(:group_member) | ref(:group) | 'group' + ref(:project_member) | ref(:project) | 'project' + end + + with_them do + it_behaves_like 'an email sent from GitLab' do + let(:gitlab_sender_display_name) { Gitlab.config.gitlab.email_display_name } + let(:gitlab_sender) { Gitlab.config.gitlab.email_from } + let(:gitlab_sender_reply_to) { Gitlab.config.gitlab.email_reply_to } + end + + it_behaves_like 'an email sent to a user' + it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user cannot unsubscribe through footer link' + it_behaves_like 'appearance header and footer enabled' + it_behaves_like 'appearance header and footer not enabled' + + it 'contains all the useful information', :aggregate_failures do + is_expected.to have_subject "Access to the #{source.full_name} #{type} was denied" + is_expected.to have_body_text source.full_name + is_expected.to have_body_text source.web_url + end + + context 'when user can not read source' do + before do + source.visibility_level = Gitlab::VisibilityLevel::PRIVATE + end + + it 'hides source name from subject and body', :aggregate_failures do + is_expected.to have_subject "Access to the Hidden #{type} was denied" + is_expected.to have_body_text "Hidden #{type}" + is_expected.not_to have_body_text source.full_name + is_expected.not_to have_body_text source.web_url + end + end + + context 'when the recipient is not notifiable' do + let(:notifiable) { false } + + it_behaves_like 'no email is sent' + end + end + end +end diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index ead5a7495ae6f2ccb48c652d78a1e741f9c88714..411ce2c4be2221e5ee898b856dc32cce5358730c 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -585,36 +585,6 @@ def id end end - describe 'project access denied' do - let_it_be(:project) { create(:project, :public) } - let_it_be(:project_member) { create(:project_member, :developer, :access_request, user: user, source: project) } - - subject { described_class.member_access_denied_email('project', project.id, user.id) } - - 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 'appearance header and footer enabled' - it_behaves_like 'appearance header and footer not enabled' - - it 'contains all the useful information' do - is_expected.to have_subject "Access to the #{project.full_name} project was denied" - is_expected.to have_body_text project.full_name - is_expected.to have_body_text project.web_url - end - - context 'when user can not read project' do - let_it_be(:project) { create(:project, :private) } - - it 'hides project name from subject and body' do - is_expected.to have_subject "Access to the Hidden project was denied" - is_expected.to have_body_text "Hidden project" - is_expected.not_to have_body_text project.full_name - is_expected.not_to have_body_text project.web_url - end - end - end - describe 'project access changed' do let(:owner) { create(:user, name: "Chang O'Keefe") } let(:project) { create(:project, :public, namespace: owner.namespace) } @@ -1644,39 +1614,6 @@ def create_note end end - describe 'group access denied' do - let_it_be(:group) { create(:group, :public) } - let_it_be(:group_member) { create(:group_member, :developer, :access_request, user: user, source: group) } - - let(:recipient) { user } - - subject { described_class.member_access_denied_email('group', group.id, user.id) } - - it_behaves_like 'an email sent from GitLab' - it_behaves_like 'an email sent to a user' - it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user cannot unsubscribe through footer link" - it_behaves_like 'appearance header and footer enabled' - it_behaves_like 'appearance header and footer not enabled' - - it 'contains all the useful information' do - is_expected.to have_subject "Access to the #{group.name} group was denied" - is_expected.to have_body_text group.name - is_expected.to have_body_text group.web_url - end - - context 'when user can not read group' do - let_it_be(:group) { create(:group, :private) } - - it 'hides group name from subject and body' do - is_expected.to have_subject "Access to the Hidden group was denied" - is_expected.to have_body_text "Hidden group" - is_expected.not_to have_body_text group.name - is_expected.not_to have_body_text group.web_url - end - end - end - describe 'group access changed' do let(:group_member) { create(:group_member, group: group, user: user) } let(:recipient) { user } diff --git a/spec/mailers/previews_spec.rb b/spec/mailers/previews_spec.rb index 7eb11d50bcfbdb9af5189210b4db5ebb224a5b05..df94b99496353ea5f0177383c9036da3f498aaec 100644 --- a/spec/mailers/previews_spec.rb +++ b/spec/mailers/previews_spec.rb @@ -35,6 +35,8 @@ before_all do create(:project_member, :maintainer, source: project, created_by: user) create(:project_member, :invited, source: project, created_by: user) + create(:group_member, :access_request, source: group) + create(:group_member, :access_request, source: create(:group, :private)) end subject { preview.call(email) } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 219b8ade9b8671d9b467f74f0fa1370bd17f1259..385d343bde2d3bb62197717748418afc5311adaa 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -15,6 +15,7 @@ it { is_expected.to have_many(:all_owner_members) } it { is_expected.to have_many(:group_members).dependent(:destroy) } it { is_expected.to have_many(:non_invite_group_members).class_name('GroupMember') } + it { is_expected.to have_many(:request_group_members).class_name('GroupMember').inverse_of(:group) } it { is_expected.to have_many(:namespace_members) } it { is_expected.to have_many(:users).through(:group_members) } it { is_expected.to have_many(:owners).through(:all_owner_members) } @@ -77,6 +78,21 @@ end end + describe '#request_group_members' do + let_it_be(:group) { create(:group) } + let_it_be(:requested_member) { create(:group_member, :access_request, group: group) } + + before do + create(:group_member, group: group) # regular member + create(:group_member, :invited, group: group) + create(:group_member, :minimal_access, group: group) + end + + it 'includes the correct members' do + expect(group.request_group_members).to contain_exactly(requested_member) + end + end + describe '#namespace_members' do let(:requester) { create(:user) } let(:developer) { create(:user) } @@ -892,6 +908,14 @@ end end + describe '#notification_group' do + it 'is expected to reference itself' do + group = build(:group) + + expect(group.notification_group).to eq(group) + end + end + describe '.public_or_visible_to_user' do let!(:private_group) { create(:group, :private) } let!(:private_subgroup) { create(:group, :private, parent: private_group) } @@ -1143,6 +1167,17 @@ end end + describe '.with_request_group_members' do + let_it_be(:group_member) { create(:group_member, :access_request, member_namespace: private_group) } + + subject(:with_request_group_members) { described_class.with_request_group_members } + + it 'loads the records of non invite group members' do + associations = with_request_group_members.map { |group| group.association(:request_group_members) } + expect(associations).to all(be_loaded) + end + end + describe 'for_authorized_group_members' do let_it_be(:group_member1) { create(:group_member, source: private_group, user_id: user1.id, access_level: Gitlab::Access::OWNER) } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2ca12b72dbc8e692db9f4390415fa2ddee08ef47..cfd4e36b7419db59684f4a6e033a9675eb65b7d7 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1023,6 +1023,12 @@ it_behaves_like 'a BulkUsersByEmailLoad model' + describe '#notification_group' do + it 'is expected to be an alias' do + expect(build(:project).method(:notification_group).original_name).to eq(:group) + end + end + describe '#all_pipelines' do let_it_be(:project) { create(:project) } diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index b23577ba8d43cefe03e12f145f3721b9f0022ded..5cc28dee11fd6c03fe9473c51a21be9d8781f39b 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -88,10 +88,12 @@ shared_examples 'a service destroying an access request of another user' do it_behaves_like 'a service destroying a member' - it 'calls Member#after_decline_request' do - expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member) + it 'calls the access denied mailer' do + allow(Members::AccessDeniedMailer).to receive(:email).with(member: member).and_call_original - described_class.new(current_user).execute(member, **opts) + expect do + described_class.new(current_user).execute(member, **opts) + end.to have_enqueued_mail(Members::AccessDeniedMailer, :email) end end @@ -99,10 +101,10 @@ it_behaves_like 'a service destroying a member' context 'when current user is the member' do - it 'does not call Member#after_decline_request' do - expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) - - described_class.new(current_user).execute(member, **opts) + it 'does not call the access denied mailer' do + expect do + described_class.new(current_user).execute(member, **opts) + end.not_to have_enqueued_mail(Members::AccessDeniedMailer, :email) end end end diff --git a/spec/views/members/access_denied_mailer/email.html.haml_spec.rb b/spec/views/members/access_denied_mailer/email.html.haml_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..8953bb69be87ba2754b23f7248b58bf757075135 --- /dev/null +++ b/spec/views/members/access_denied_mailer/email.html.haml_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'members/access_denied_mailer/email.html.haml', feature_category: :groups_and_projects do + let(:source_hidden?) { false } + let(:member_source) { build(:group) } + + before do + allow(view).to receive(:source_hidden?).and_return(source_hidden?) + allow(view).to receive(:member_source).and_return(member_source) + end + + subject { render && rendered } + + context 'when source is not hidden' do + it { is_expected.not_to have_text('Hidden') } + it { is_expected.to have_link(member_source.human_name, href: member_source.web_url) } + end + + context 'when source is hidden' do + let(:source_hidden?) { true } + + it { is_expected.to have_text('Hidden') } + it { is_expected.not_to have_link(member_source.human_name) } + end +end diff --git a/spec/views/members/access_denied_mailer/email.text.erb_spec.rb b/spec/views/members/access_denied_mailer/email.text.erb_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..ef64e8374210e6d6c4485aae3e20c35bfc1c2696 --- /dev/null +++ b/spec/views/members/access_denied_mailer/email.text.erb_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'members/access_denied_mailer/email.text.erb', feature_category: :groups_and_projects do + let(:source_hidden?) { false } + let(:member_source) { build(:group) } + + before do + allow(view).to receive(:source_hidden?).and_return(source_hidden?) + allow(view).to receive(:member_source).and_return(member_source) + end + + subject { render && rendered } + + context 'when source is not hidden' do + it { is_expected.not_to have_text('Hidden') } + it { is_expected.to have_text(member_source.human_name) } + end + + context 'when source is hidden' do + let(:source_hidden?) { true } + + it { is_expected.to have_text('Hidden') } + it { is_expected.not_to have_text(member_source.human_name) } + end +end