Commit 022f60e8 authored by Jan Provaznik's avatar Jan Provaznik

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.
parent ae216618
...@@ -84,6 +84,8 @@ class Member < ActiveRecord::Base ...@@ -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_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) { where(source: [project] + project.ancestors) }
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
...@@ -2232,8 +2249,8 @@ describe NotificationService, :mailer do ...@@ -2232,8 +2249,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
...@@ -2249,10 +2266,10 @@ describe NotificationService, :mailer do ...@@ -2249,10 +2266,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_objects? if Group.supports_nested_objects?
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)
...@@ -2272,7 +2289,7 @@ describe NotificationService, :mailer do ...@@ -2272,7 +2289,7 @@ describe NotificationService, :mailer do
child_group child_group
else else
create(:group, :public) create(:group, visibility)
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment