Merge branch 'security-check-group-exists-before-email' into 'master'

Stop excess logs from invite email when group no longer exists

Closes #184

See merge request gitlab-org/security/gitlab!703

app/mailers/emails/members.rb

@@ -13,6 +13,8 @@ module Emails
       @member_source_type = member_source_type
       @member_id = member_id
 
+      return unless member_exists?
+
       user = User.find(recipient_id)
 
       member_email_with_layout(
@@ -24,6 +26,8 @@ module Emails
       @member_source_type = member_source_type
       @member_id = member_id
 
+      return unless member_exists?
+
       member_email_with_layout(
         to: member.user.notification_email_for(notification_group),
         subject: subject("Access to the #{member_source.human_name} #{member_source.model_name.singular} was granted"))
@@ -45,6 +49,8 @@ module Emails
       @member_id = member_id
       @token = token
 
+      return unless member_exists?
+
       member_email_with_layout(
         to: member.invite_email,
         subject: subject("Invitation to join the #{member_source.human_name} #{member_source.model_name.singular}"))
@@ -53,6 +59,8 @@ module Emails
     def member_invite_accepted_email(member_source_type, member_id)
       @member_source_type = member_source_type
       @member_id = member_id
+
+      return unless member_exists?
       return unless member.created_by
 
       member_email_with_layout(
@@ -74,9 +82,11 @@ module Emails
         subject: subject('Invitation declined'))
     end
 
+    # rubocop: disable CodeReuse/ActiveRecord
     def member
-      @member ||= Member.find(@member_id)
+      @member ||= Member.find_by(id: @member_id)
     end
+    # rubocop: enable CodeReuse/ActiveRecord
 
     def member_source
       @member_source ||= member.source
@@ -88,6 +98,11 @@ module Emails
 
     private
 
+    def member_exists?
+      Gitlab::AppLogger.info("Tried to send an email invitation for a deleted group. Member id: #{@member_id}") if member.blank?
+      member.present?
+    end
+
     def member_source_class
       @member_source_type.classify.constantize
     end

changelogs/unreleased/security-check-group-exists-before-email.yml

+---
+title: Stop excess logs from failure to send invite email when group no longer exists
+merge_request:
+author:
+type: security

spec/mailers/notify_spec.rb

@@ -45,6 +45,21 @@ RSpec.describe Notify do
     end
   end
 
+  shared_examples 'it requires a group' do
+    context 'when given an deleted group' do
+      before do
+        # destroy group and group member
+        group_member.destroy!
+        group.destroy!
+      end
+
+      it 'returns NullMail type message' do
+        expect(Gitlab::AppLogger).to receive(:info)
+        expect(subject.message).to be_a(ActionMailer::Base::NullMail)
+      end
+    end
+  end
+
   context 'for a project' do
     shared_examples 'an assignee email' do
       let(:recipient) { assignee }
@@ -1388,6 +1403,7 @@ RSpec.describe Notify do
       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_behaves_like 'it requires a group'
 
       it 'contains all the useful information' do
         is_expected.to have_subject "Access to the #{group.name} group was granted"
@@ -1422,6 +1438,7 @@ RSpec.describe Notify do
       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_behaves_like 'it requires a group'
 
       it 'contains all the useful information' do
         is_expected.to have_subject "Invitation to join the #{group.name} group"
@@ -1448,6 +1465,7 @@ RSpec.describe Notify do
       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_behaves_like 'it requires a group'
 
       it 'contains all the useful information' do
         is_expected.to have_subject 'Invitation accepted'