...@@ -7,6 +7,7 @@ class Member < ActiveRecord::Base ...@@ -7,6 +7,7 @@ class Member < ActiveRecord::Base
include Expirable include Expirable
include Gitlab::Access include Gitlab::Access
include Presentable include Presentable
include FromUnion
attr_accessor :raw_invite_token attr_accessor :raw_invite_token
...@@ -83,6 +84,14 @@ class Member < ActiveRecord::Base ...@@ -83,6 +84,14 @@ 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_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 :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) do
if project.group
from_union([GroupMember.where(source_id: project.group.self_and_ancestors), project.project_members])
else
project.project_members
end
end
before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? } before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? }
after_create :send_invite, if: :invite?, unless: :importing? after_create :send_invite, if: :invite?, unless: :importing?
... ...
......
...@@ -74,6 +74,14 @@ class ProjectTeam ...@@ -74,6 +74,14 @@ class ProjectTeam
end end
alias_method :users, :members 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 def guests
@guests ||= fetch_members(Gitlab::Access::GUEST) @guests ||= fetch_members(Gitlab::Access::GUEST)
end end
...@@ -191,4 +199,8 @@ class ProjectTeam ...@@ -191,4 +199,8 @@ class ProjectTeam
def group def group
project.group project.group
end end
def member_user_ids
Member.on_project_and_ancestors(project).select(:user_id)
end
end end
...@@ -373,7 +373,8 @@ class NotificationService ...@@ -373,7 +373,8 @@ class NotificationService
end end
def project_was_moved(project, old_path_with_namespace) 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| recipients.each do |recipient|
mailer.project_was_moved_email( mailer.project_was_moved_email(
... ...
......
---
title: Notify only users who can access the project on project move.
merge_request:
author:
type: security
...@@ -178,6 +178,21 @@ describe ProjectTeam do ...@@ -178,6 +178,21 @@ describe ProjectTeam do
end end
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 describe "#human_max_access" do
it 'returns Maintainer role' do it 'returns Maintainer role' do
user = create(:user) user = create(:user)
... ...
......
...@@ -1646,6 +1646,23 @@ describe NotificationService, :mailer do ...@@ -1646,6 +1646,23 @@ describe NotificationService, :mailer do
should_not_email(@u_guest_custom) should_not_email(@u_guest_custom)
should_not_email(@u_disabled) should_not_email(@u_disabled)
end 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 end
context 'user with notifications disabled' do context 'user with notifications disabled' do
...@@ -2178,8 +2195,8 @@ describe NotificationService, :mailer do ...@@ -2178,8 +2195,8 @@ describe NotificationService, :mailer do
# Users in the project's group but not part of project's team # Users in the project's group but not part of project's team
# with different notification settings # with different notification settings
def build_group(project) def build_group(project, visibility: :public)
group = create_nested_group group = create_nested_group(visibility)
project.update(namespace_id: group.id) project.update(namespace_id: group.id)
# Group member: global=disabled, group=watch # Group member: global=disabled, group=watch
...@@ -2195,10 +2212,10 @@ describe NotificationService, :mailer do ...@@ -2195,10 +2212,10 @@ describe NotificationService, :mailer do
# Creates a nested group only if supported # Creates a nested group only if supported
# to avoid errors on MySQL # to avoid errors on MySQL
def create_nested_group def create_nested_group(visibility)
if Group.supports_nested_groups? if Group.supports_nested_groups?
parent_group = create(:group, :public) parent_group = create(:group, visibility)
child_group = create(:group, :public, parent: parent_group) child_group = create(:group, visibility, parent: parent_group)
# Parent group member: global=disabled, parent_group=watch, child_group=global # Parent group member: global=disabled, parent_group=watch, child_group=global
@pg_watcher ||= create_user_with_notification(:watch, 'parent_group_watcher', parent_group) @pg_watcher ||= create_user_with_notification(:watch, 'parent_group_watcher', parent_group)
...@@ -2218,7 +2235,7 @@ describe NotificationService, :mailer do ...@@ -2218,7 +2235,7 @@ describe NotificationService, :mailer do
child_group child_group
else else
create(:group, :public) create(:group, visibility)
end end
end end
... ...
......