diff options
| -rw-r--r-- | app/models/member.rb | 2 | ||||
| -rw-r--r-- | app/models/project_team.rb | 12 | ||||
| -rw-r--r-- | app/services/notification_service.rb | 3 | ||||
| -rw-r--r-- | changelogs/unreleased/security-project-move-users.yml | 5 | ||||
| -rw-r--r-- | spec/models/project_team_spec.rb | 15 | ||||
| -rw-r--r-- | spec/services/notification_service_spec.rb | 29 |
6 files changed, 59 insertions, 7 deletions
diff --git a/app/models/member.rb b/app/models/member.rb index b0f049438eb..a0f755f88b5 100644 --- a/app/models/member.rb +++ b/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? diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 33bc6a561f9..aeba2843e5d 100644 --- a/app/models/project_team.rb +++ b/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 diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index e1cf327209b..1a65561dd70 100644 --- a/app/services/notification_service.rb +++ b/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( diff --git a/changelogs/unreleased/security-project-move-users.yml b/changelogs/unreleased/security-project-move-users.yml new file mode 100644 index 00000000000..744df68651f --- /dev/null +++ b/changelogs/unreleased/security-project-move-users.yml @@ -0,0 +1,5 @@ +--- +title: Notify only users who can access the project on project move. +merge_request: +author: +type: security diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index c4af17f4726..3537dead5d1 100644 --- a/spec/models/project_team_spec.rb +++ b/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) diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index d20e712d365..6a5a6989607 100644 --- a/spec/services/notification_service_spec.rb +++ b/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 |