Merge branch 'security-163-mirror-user' into 'master'

Do not set fallback mirror user

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

ee/app/mailers/ee/emails/projects.rb

@@ -11,6 +11,17 @@ module EE
              subject: subject('Repository mirroring paused'))
       end
 
+      def mirror_was_disabled_email(project_id, user_id, deleted_user_name)
+        @project = ::Project.find(project_id)
+        user = ::User.find_by_id(user_id)
+        @deleted_user_name = deleted_user_name
+
+        return unless user
+
+        mail(to: user.notification_email_for(@project.group),
+             subject: subject('Repository mirroring disabled'))
+      end
+
       def project_mirror_user_changed_email(new_mirror_user_id, deleted_user_name, project_id)
         @project = ::Project.find(project_id)
         @deleted_user_name = deleted_user_name

ee/app/mailers/ee/preview/notify_preview.rb

@@ -24,6 +24,10 @@ module EE
           ::Notify.mirror_was_hard_failed_email(project.id, user.id).message
         end
 
+        def mirror_was_disabled_email
+          ::Notify.mirror_was_disabled_email(project.id, user.id, 'deleted_user_name').message
+        end
+
         def project_mirror_user_changed_email
           ::Notify.project_mirror_user_changed_email(user.id, 'deleted_user_name', project.id).message
         end

ee/app/services/ee/notification_service.rb

@@ -38,6 +38,14 @@ module EE
       end
     end
 
+    def mirror_was_disabled(project, deleted_user_name)
+      return if project.emails_disabled?
+
+      owners_and_maintainers_without_invites(project).each do |recipient|
+        mailer.mirror_was_disabled_email(project.id, recipient.user.id, deleted_user_name).deliver_later
+      end
+    end
+
     def new_epic(epic)
       new_resource_email(epic, :new_epic_email)
     end

ee/app/services/ee/users/destroy_service.rb

@@ -20,10 +20,14 @@ module EE
         user_mirrors = ::Project.where(mirror_user: user) # rubocop: disable CodeReuse/ActiveRecord
 
         user_mirrors.find_each do |mirror|
-          new_mirror_user = first_mirror_owner(user, mirror)
-
-          mirror.update(mirror_user: new_mirror_user)
-          ::NotificationService.new.project_mirror_user_changed(new_mirror_user, user.name, mirror)
+          mirror.update(mirror: false, mirror_user: nil)
+          ::Gitlab::ErrorTracking.track_exception(
+            RuntimeError.new('Disabled mirroring'),
+            user_id: user.id,
+            project_id: mirror.id
+          )
+
+          ::NotificationService.new.mirror_was_disabled(mirror, user.name)
         end
       end
 

ee/app/views/notify/mirror_was_disabled_email.html.haml

+%p
+  Repository mirroring on #{@project.full_path} was disabled because the mirror user #{sanitize_name(@deleted_user_name)} was deleted.
+
+%p
+  To re-enable mirroring, update your #{link_to("repository mirroring settings", project_settings_repository_url(@project))}.

ee/app/views/notify/mirror_was_disabled_email.text.erb

+Repository mirroring on <%= @project.full_path %> was disabled because the mirror user <%= sanitize_name(@deleted_user_name) %> was deleted.
+
+To re-enable mirroring, update your repository settings at <%= project_settings_repository_url(@project) %>.

ee/changelogs/unreleased/security-163-mirror-user.yml

+---
+title: Do not set fallback mirror user
+merge_request:
+author:
+type: security

ee/spec/mailers/notify_spec.rb

@@ -372,6 +372,33 @@ RSpec.describe Notify do
     end
   end
 
+  describe 'mirror was disabled' do
+    let(:project) { create(:project) }
+
+    subject { described_class.mirror_was_disabled_email(project.id, user.id, 'deleted_user_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 'has the correct subject and body' do
+      is_expected.to have_subject("#{project.name} | Repository mirroring disabled")
+      is_expected.to have_body_text(project.full_path)
+      is_expected.to have_body_text(project_settings_repository_url(project))
+      is_expected.to have_body_text('deleted_user_name')
+    end
+
+    context 'user was deleted' do
+      before do
+        user.destroy!
+      end
+
+      it 'does not send email' do
+        expect(subject.message).to be_a_kind_of ActionMailer::Base::NullMail
+      end
+    end
+  end
+
   describe 'mirror user changed' do
     let(:mirror_user) { create(:user) }
     let(:project) { create(:project, :mirror, mirror_user_id: mirror_user.id) }

ee/spec/services/ee/notification_service_spec.rb

@@ -307,6 +307,125 @@ RSpec.describe EE::NotificationService, :mailer do
     end
   end
 
+  describe 'mirror was disabled' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let(:deleted_username) { 'deleted_user_name' }
+
+    context 'when the project has invited members' do
+      let!(:project_member) { create(:project_member, :invited, project: project) }
+
+      it 'sends email' do
+        expect(Notify).to receive(:mirror_was_disabled_email).with(project.id, project.owner.id, deleted_username).and_call_original
+
+        subject.mirror_was_disabled(project, deleted_username)
+      end
+
+      it_behaves_like 'project emails are disabled' do
+        let(:notification_target)  { project }
+        let(:notification_trigger) { subject.mirror_was_disabled(project, deleted_username) }
+
+        around do |example|
+          perform_enqueued_jobs { example.run }
+        end
+      end
+    end
+
+    context 'when user is owner' do
+      it 'sends email' do
+        expect(Notify).to receive(:mirror_was_disabled_email).with(project.id, project.owner.id, deleted_username).and_call_original
+
+        subject.mirror_was_disabled(project, deleted_username)
+      end
+
+      it_behaves_like 'project emails are disabled' do
+        let(:notification_target)  { project }
+        let(:notification_trigger) { subject.mirror_was_disabled(project, deleted_username) }
+
+        around do |example|
+          perform_enqueued_jobs { example.run }
+        end
+      end
+
+      context 'when owner is blocked' do
+        it 'does not send email' do
+          project.owner.block!
+
+          expect(Notify).not_to receive(:mirror_was_disabled_email)
+
+          subject.mirror_was_disabled(project, deleted_username)
+        end
+
+        context 'when project belongs to group' do
+          it 'does not send email to the blocked owner' do
+            blocked_user = create(:user, :blocked)
+
+            group = create(:group, :public)
+            group.add_owner(blocked_user)
+            group.add_owner(user)
+
+            project = create(:project, namespace: group)
+
+            expect(Notify).not_to receive(:mirror_was_disabled_email).with(project.id, blocked_user.id, deleted_username).and_call_original
+            expect(Notify).to receive(:mirror_was_disabled_email).with(project.id, user.id, deleted_username).and_call_original
+
+            subject.mirror_was_disabled(project, deleted_username)
+          end
+        end
+      end
+    end
+
+    context 'when user is maintainer' do
+      it 'sends email' do
+        project.add_maintainer(user)
+
+        expect(Notify).to receive(:mirror_was_disabled_email).with(project.id, project.owner.id, deleted_username).and_call_original
+        expect(Notify).to receive(:mirror_was_disabled_email).with(project.id, user.id, deleted_username).and_call_original
+
+        subject.mirror_was_disabled(project, deleted_username)
+      end
+    end
+
+    context 'when user is not owner nor maintainer' do
+      it 'does not send email' do
+        project.add_developer(user)
+
+        expect(Notify).not_to receive(:mirror_was_disabled_email).with(project.id, user.id, deleted_username).and_call_original
+        expect(Notify).to receive(:mirror_was_disabled_email).with(project.id, project.creator.id, deleted_username).and_call_original
+
+        subject.mirror_was_disabled(project, deleted_username)
+      end
+
+      context 'when user is group owner' do
+        it 'sends email' do
+          group = create(:group, :public) do |group|
+            group.add_owner(user)
+          end
+
+          project = create(:project, namespace: group)
+
+          expect(Notify).to receive(:mirror_was_disabled_email).with(project.id, user.id, deleted_username).and_call_original
+
+          subject.mirror_was_disabled(project, deleted_username)
+        end
+      end
+
+      context 'when user is group maintainer' do
+        it 'sends email' do
+          group = create(:group, :public) do |group|
+            group.add_maintainer(user)
+          end
+
+          project = create(:project, namespace: group)
+
+          expect(Notify).to receive(:mirror_was_disabled_email).with(project.id, user.id, deleted_username).and_call_original
+
+          subject.mirror_was_disabled(project, deleted_username)
+        end
+      end
+    end
+  end
+
   context 'mirror user changed' do
     let(:mirror_user) { create(:user) }
     let(:project) { create(:project, :mirror, mirror_user_id: mirror_user.id) }

ee/spec/services/ee/users/destroy_service_spec.rb

@@ -21,15 +21,17 @@ RSpec.describe Users::DestroyService do
     context 'when project is a mirror' do
       let(:project) { create(:project, :mirror, mirror_user_id: user.id) }
 
-      it 'assigns mirror_user to a project owner' do
-        new_mirror_user = project.team.owners.first
+      it 'disables mirror and does not assign a new mirror_user' do
+        expect(::Gitlab::ErrorTracking).to receive(:track_exception)
 
-        expect_any_instance_of(EE::NotificationService)
-          .to receive(:project_mirror_user_changed)
-          .with(new_mirror_user, user.name, project)
+        allow_next_instance_of(::NotificationService) do |notification|
+          expect(notification).to receive(:mirror_was_disabled)
+            .with(project, user.name)
+            .and_call_original
+        end
 
-        expect { operation }.to change { project.reload.mirror_user }
-          .from(user).to(new_mirror_user)
+        expect { operation }.to change { project.reload.mirror_user }.from(user).to(nil)
+          .and change { project.reload.mirror }.from(true).to(false)
       end
     end