Sent notification only to authorized users

When moving a project, it's possible that some users who had
access to the project in old path can not access the project
in the new path.

Because `project_authorizations` records are updated asynchronously,
when we send the notification about moved project the list of project
team members contains old project members, we want to notify all these
members except the old users who can not access the new location.

app/models/member.rb

@@ -84,6 +84,8 @@ class Member < ActiveRecord::Base
   scope :order_recent_sign_in, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.last_sign_in_at', 'DESC')) }
   scope :order_oldest_sign_in, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.last_sign_in_at', 'ASC')) }
 
+  scope :on_project_and_ancestors, ->(project) { where(source: [project] + project.ancestors) }
+
   before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? }
 
   after_create :send_invite, if: :invite?, unless: :importing?

app/models/project_team.rb

@@ -74,6 +74,14 @@ class ProjectTeam
   end
   alias_method :users, :members
 
+  # `members` method uses project_authorizations table which
+  # is updated asynchronously, on project move it still contains
+  # old members who may not have access to the new location,
+  # so we filter out only members of project or project's group
+  def members_in_project_and_ancestors
+    members.where(id: member_user_ids)
+  end
+
   def guests
     @guests ||= fetch_members(Gitlab::Access::GUEST)
   end
@@ -191,4 +199,8 @@ class ProjectTeam
   def group
     project.group
   end
+
+  def member_user_ids
+    Member.on_project_and_ancestors(project).select(:user_id)
+  end
 end

app/services/notification_service.rb

@@ -373,7 +373,8 @@ class NotificationService
   end
 
   def project_was_moved(project, old_path_with_namespace)
-    recipients = notifiable_users(project.team.members, :mention, project: project)
+    recipients = project.private? ? project.team.members_in_project_and_ancestors : project.team.members
+    recipients = notifiable_users(recipients, :mention, project: project)
 
     recipients.each do |recipient|
       mailer.project_was_moved_email(

changelogs/unreleased/security-project-move-users.yml

+---
+title: Notify only users who can access the project on project move.
+merge_request:
+author:
+type: security

spec/models/project_team_spec.rb

@@ -178,6 +178,21 @@ describe ProjectTeam do
     end
   end
 
+  describe '#members_in_project_and_ancestors' do
+    context 'group project' do
+      it 'filters out users who are not members of the project' do
+        group = create(:group)
+        project = create(:project, group: group)
+        group_member = create(:group_member, group: group)
+        old_user = create(:user)
+
+        ProjectAuthorization.create!(project: project, user: old_user, access_level: Gitlab::Access::GUEST)
+
+        expect(project.team.members_in_project_and_ancestors).to contain_exactly(group_member.user)
+      end
+    end
+  end
+
   describe "#human_max_access" do
     it 'returns Maintainer role' do
       user = create(:user)

spec/services/notification_service_spec.rb

@@ -1646,6 +1646,23 @@ describe NotificationService, :mailer do
         should_not_email(@u_guest_custom)
         should_not_email(@u_disabled)
       end
+
+      context 'users not having access to the new location' do
+        it 'does not send email' do
+          old_user = create(:user)
+          ProjectAuthorization.create!(project: project, user: old_user, access_level: Gitlab::Access::GUEST)
+
+          build_group(project)
+          reset_delivered_emails!
+
+          notification.project_was_moved(project, "gitlab/gitlab")
+
+          should_email(@g_watcher)
+          should_email(@g_global_watcher)
+          should_email(project.creator)
+          should_not_email(old_user)
+        end
+      end
     end
 
     context 'user with notifications disabled' do
@@ -2232,8 +2249,8 @@ describe NotificationService, :mailer do
 
   # Users in the project's group but not part of project's team
   # with different notification settings
-  def build_group(project)
-    group = create_nested_group
+  def build_group(project, visibility: :public)
+    group = create_nested_group(visibility)
     project.update(namespace_id: group.id)
 
     # Group member: global=disabled, group=watch
@@ -2249,10 +2266,10 @@ describe NotificationService, :mailer do
 
   # Creates a nested group only if supported
   # to avoid errors on MySQL
-  def create_nested_group
+  def create_nested_group(visibility)
     if Group.supports_nested_objects?
-      parent_group = create(:group, :public)
-      child_group = create(:group, :public, parent: parent_group)
+      parent_group = create(:group, visibility)
+      child_group = create(:group, visibility, parent: parent_group)
 
       # Parent group member: global=disabled, parent_group=watch, child_group=global
       @pg_watcher ||= create_user_with_notification(:watch, 'parent_group_watcher', parent_group)
@@ -2272,7 +2289,7 @@ describe NotificationService, :mailer do
 
       child_group
     else
-      create(:group, :public)
+      create(:group, visibility)
     end
   end