Optimize the invited group link access level check

2 files changed, 33 insertions, 19 deletions

diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index 21b3a013673..03ea8b8b851 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -155,10 +155,13 @@ class ProjectTeam
         merge_max!(access, group_access)
       end
 
+      # Each group produces a list of maximum access level per user. We take the
+      # max of the values produced by each group.
       if project.invited_groups.any? && project.allowed_to_share_with_group?
-        # Not optimized
-        invited_levels = user_ids.map { |id| [id, max_invited_level(id)] }.to_h
-        merge_max!(access, invited_levels)
+        project.project_group_links.each do |group_link|
+          invited_access = max_invited_level_for_users(group_link, user_ids)
+          merge_max!(access, invited_access)
+        end
       end
     end
 
@@ -171,20 +174,21 @@ class ProjectTeam
 
   private
 
-  def max_invited_level(user_id)
-    project.project_group_links.map do |group_link|
-      invited_group = group_link.group
-      access = invited_group.group_members.find_by(user_id: user_id).try(:access_field)
-      access = Gitlab::Access::NO_ACCESS unless access.present?
-
-      # If group member has higher access level we should restrict it
-      # to max allowed access level
-      if access && access > group_link.group_access
-        access = group_link.group_access
-      end
-
-      access
-    end.compact.max
+  # For a given group, return the maximum access level for the user. This is the min of
+  # the invited access level of the group and the access level of the user within the group.
+  # For example, if the group has been given DEVELOPER access but the member has MASTER access,
+  # the user should receive only DEVELOPER access.
+  def max_invited_level_for_users(group_link, user_ids)
+    invited_group = group_link.group
+    capped_access_level = group_link.group_access
+    access = invited_group.group_members.access_for_user_ids(user_ids)
+
+    # If the user is not in the list, assume he/she does not have access
+    missing_users = user_ids - access.keys
+    missing_users.each { |id| access[id] = Gitlab::Access::NO_ACCESS }
+
+    # Cap the maximum access by the invited level access
+    access.each { |key, value| access[key] = [value, capped_access_level].min }
   end
 
   def fetch_members(level = nil)
diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb
index 115fffd82d9..f9b3e65745e 100644
--- a/spec/models/project_team_spec.rb
+++ b/spec/models/project_team_spec.rb
@@ -214,20 +214,30 @@ describe ProjectTeam, models: true do
 
       group = create(:group)
       group_developer = create(:user)
+      second_developer = create(:user)
       project.project_group_links.create(
         group: group,
         group_access: Gitlab::Access::DEVELOPER)
 
       group.add_master(promoted_guest)
       group.add_developer(group_developer)
-      users = [master, reporter, promoted_guest, guest, group_developer].map(&:id)
+      group.add_developer(second_developer)
+
+      second_group = create(:group)
+      project.project_group_links.create(
+          group: second_group,
+          group_access: Gitlab::Access::MASTER)
+      second_group.add_master(second_developer)
+
+      users = [master, reporter, promoted_guest, guest, group_developer, second_developer].map(&:id)
 
       expected = {
           master.id => Gitlab::Access::MASTER,
           reporter.id => Gitlab::Access::REPORTER,
           promoted_guest.id => Gitlab::Access::DEVELOPER,
           guest.id => Gitlab::Access::GUEST,
-          group_developer.id => Gitlab::Access::DEVELOPER
+          group_developer.id => Gitlab::Access::DEVELOPER,
+          second_developer.id => Gitlab::Access::MASTER
       }
 
       expect(project.team.max_member_access_for_user_ids(users)).to eq(expected)