Merge branch 'if-208970-group_auth_update_to_consider_shared_groups' into 'master'

Group authorization refresh to consider shared groups

See merge request gitlab-org/gitlab!31204

app/models/group.rb

@@ -325,15 +325,17 @@ class Group < Namespace
   def members_with_parents
     # Avoids an unnecessary SELECT when the group has no parents
     source_ids =
-      if parent_id
+      if has_parent?
         self_and_ancestors.reorder(nil).select(:id)
       else
         id
       end
 
-    GroupMember
-      .active_without_invites_and_requests
-      .where(source_id: source_ids)
+    group_hierarchy_members = GroupMember.active_without_invites_and_requests
+                                         .where(source_id: source_ids)
+
+    GroupMember.from_union([group_hierarchy_members,
+                            members_from_self_and_ancestor_group_shares])
   end
 
   def members_from_self_and_ancestors_with_effective_access_level
@@ -398,7 +400,7 @@ class Group < Namespace
                                             .first
                                             &.access_level
 
-    max_member_access || max_member_access_for_user_from_shared_groups(user) || GroupMember::NO_ACCESS
+    max_member_access || GroupMember::NO_ACCESS
   end
 
   def mattermost_team_params
@@ -524,27 +526,39 @@ class Group < Namespace
     errors.add(:visibility_level, "#{visibility} is not allowed since there are sub-groups with higher visibility.")
   end
 
-  def max_member_access_for_user_from_shared_groups(user)
+  def members_from_self_and_ancestor_group_shares
     group_group_link_table = GroupGroupLink.arel_table
     group_member_table = GroupMember.arel_table
 
-    group_group_links_query = GroupGroupLink.where(shared_group_id: self_and_ancestors_ids)
+    source_ids =
+      if has_parent?
+        self_and_ancestors.reorder(nil).select(:id)
+      else
+        id
+      end
+
+    group_group_links_query = GroupGroupLink.where(shared_group_id: source_ids)
     cte = Gitlab::SQL::CTE.new(:group_group_links_cte, group_group_links_query)
     cte_alias = cte.table.alias(GroupGroupLink.table_name)
 
-    link = GroupGroupLink
-             .with(cte.to_arel)
-             .select(smallest_value_arel([cte_alias[:group_access], group_member_table[:access_level]],
-                                         'group_access'))
-             .from([group_member_table, cte.alias_to(group_group_link_table)])
-             .where(group_member_table[:user_id].eq(user.id))
-             .where(group_member_table[:requested_at].eq(nil))
-             .where(group_member_table[:source_id].eq(group_group_link_table[:shared_with_group_id]))
-             .where(group_member_table[:source_type].eq('Namespace'))
-             .reorder(Arel::Nodes::Descending.new(group_group_link_table[:group_access]))
-             .first
-
-    link&.group_access
+    # Instead of members.access_level, we need to maximize that access_level at
+    # the respective group_group_links.group_access.
+    member_columns = GroupMember.attribute_names.map do |column_name|
+      if column_name == 'access_level'
+        smallest_value_arel([cte_alias[:group_access], group_member_table[:access_level]],
+                            'access_level')
+      else
+        group_member_table[column_name]
+      end
+    end
+
+    GroupMember
+      .with(cte.to_arel)
+      .select(*member_columns)
+      .from([group_member_table, cte.alias_to(group_group_link_table)])
+      .where(group_member_table[:requested_at].eq(nil))
+      .where(group_member_table[:source_id].eq(group_group_link_table[:shared_with_group_id]))
+      .where(group_member_table[:source_type].eq('Namespace'))
   end
 
   def smallest_value_arel(args, column_alias)

app/services/groups/transfer_service.rb

@@ -88,6 +88,7 @@ module Groups
       end
 
       @group.parent = @new_parent_group
+      @group.clear_memoization(:self_and_ancestors_ids)
       @group.save!
     end
 

changelogs/unreleased/208970-group_auth_update_to_consider_shared_groups.yml

+---
+title: Group authorization refresh to consider shared groups
+merge_request: 31204
+author:
+type: fixed

spec/models/group_spec.rb

@@ -662,6 +662,19 @@ describe Group do
       expect(group.members_with_parents).to include(developer)
       expect(group.members_with_parents).to include(maintainer)
     end
+
+    context 'group sharing' do
+      let!(:shared_group) { create(:group) }
+
+      before do
+        create(:group_group_link, shared_group: shared_group, shared_with_group: group)
+      end
+
+      it 'returns shared with group members' do
+        expect(shared_group.members_with_parents).to(
+          include(developer))
+      end
+    end
   end
 
   describe '#members_from_self_and_ancestors_with_effective_access_level' do
@@ -800,6 +813,22 @@ describe Group do
       expect(group.user_ids_for_project_authorizations)
         .to include(maintainer.id, developer.id)
     end
+
+    context 'group sharing' do
+      let_it_be(:group) { create(:group) }
+      let_it_be(:group_user) { create(:user) }
+      let_it_be(:shared_group) { create(:group) }
+
+      before do
+        group.add_developer(group_user)
+        create(:group_group_link, shared_group: shared_group, shared_with_group: group)
+      end
+
+      it 'returns the user IDs for shared with group members' do
+        expect(shared_group.user_ids_for_project_authorizations).to(
+          include(group_user.id))
+      end
+    end
   end
 
   describe '#update_two_factor_requirement' do

spec/services/authorized_project_update/project_create_service_spec.rb

@@ -56,21 +56,47 @@ describe AuthorizedProjectUpdate::ProjectCreateService do
     end
 
     context 'membership overrides' do
-      before do
-        create(:group_member, access_level: Gitlab::Access::REPORTER, group: group_parent, user: group_user)
-        create(:group_member, access_level: Gitlab::Access::DEVELOPER, group: group, user: group_user)
-        ProjectAuthorization.delete_all
+      context 'group hierarchy' do
+        before do
+          create(:group_member, access_level: Gitlab::Access::REPORTER, group: group_parent, user: group_user)
+          create(:group_member, access_level: Gitlab::Access::DEVELOPER, group: group, user: group_user)
+          ProjectAuthorization.delete_all
+        end
+
+        it 'creates project authorization' do
+          expect { service.execute }.to(
+            change { ProjectAuthorization.count }.from(0).to(1))
+
+          project_authorization = ProjectAuthorization.where(
+            project_id: group_project.id,
+            user_id: group_user.id,
+            access_level: Gitlab::Access::DEVELOPER)
+          expect(project_authorization).to exist
+        end
       end
 
-      it 'creates project authorization' do
-        expect { service.execute }.to(
-          change { ProjectAuthorization.count }.from(0).to(1))
+      context 'group sharing' do
+        let!(:shared_with_group) { create(:group) }
 
-        project_authorization = ProjectAuthorization.where(
-          project_id: group_project.id,
-          user_id: group_user.id,
-          access_level: Gitlab::Access::DEVELOPER)
-        expect(project_authorization).to exist
+        before do
+          create(:group_member, access_level: Gitlab::Access::REPORTER, group: group, user: group_user)
+          create(:group_member, access_level: Gitlab::Access::MAINTAINER, group: shared_with_group, user: group_user)
+
+          create(:group_group_link, shared_group: group, shared_with_group: shared_with_group, group_access: Gitlab::Access::DEVELOPER)
+
+          ProjectAuthorization.delete_all
+        end
+
+        it 'creates project authorization' do
+          expect { service.execute }.to(
+            change { ProjectAuthorization.count }.from(0).to(1))
+
+          project_authorization = ProjectAuthorization.where(
+            project_id: group_project.id,
+            user_id: group_user.id,
+            access_level: Gitlab::Access::DEVELOPER)
+          expect(project_authorization).to exist
+        end
       end
     end
 

spec/services/projects/create_service_spec.rb

@@ -88,6 +88,116 @@ describe Projects::CreateService, '#execute' do
     end
   end
 
+  context 'group sharing', :sidekiq_inline do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:shared_group) { create(:group) }
+    let_it_be(:shared_group_user) { create(:user) }
+    let(:opts) do
+      {
+        name: 'GitLab',
+        namespace_id: shared_group.id
+      }
+    end
+
+    before do
+      create(:group_group_link, shared_group: shared_group, shared_with_group: group)
+
+      shared_group.add_maintainer(shared_group_user)
+      group.add_developer(user)
+    end
+
+    it 'updates authorization' do
+      shared_group_project = create_project(shared_group_user, opts)
+
+      expect(
+        Ability.allowed?(shared_group_user, :read_project, shared_group_project)
+      ).to be_truthy
+      expect(
+        Ability.allowed?(user, :read_project, shared_group_project)
+      ).to be_truthy
+    end
+  end
+
+  context 'membership overrides', :sidekiq_inline do
+    let_it_be(:group) { create(:group, :private) }
+    let_it_be(:subgroup_for_projects) { create(:group, :private, parent: group) }
+    let_it_be(:subgroup_for_access) { create(:group, :private, parent: group) }
+    let_it_be(:group_maintainer) { create(:user) }
+    let(:group_access_level) { Gitlab::Access::REPORTER }
+    let(:subgroup_access_level) { Gitlab::Access::DEVELOPER }
+    let(:share_max_access_level) { Gitlab::Access::MAINTAINER }
+    let(:opts) do
+      {
+        name: 'GitLab',
+        namespace_id: subgroup_for_projects.id
+      }
+    end
+
+    before do
+      group.add_maintainer(group_maintainer)
+
+      create(:group_group_link, shared_group: subgroup_for_projects,
+                                shared_with_group: subgroup_for_access,
+                                group_access: share_max_access_level)
+    end
+
+    context 'membership is higher from group hierarchy' do
+      let(:group_access_level) { Gitlab::Access::MAINTAINER }
+
+      it 'updates authorization' do
+        create(:group_member, access_level: subgroup_access_level, group: subgroup_for_access, user: user)
+        create(:group_member, access_level: group_access_level, group: group, user: user)
+
+        subgroup_project = create_project(group_maintainer, opts)
+
+        project_authorization = ProjectAuthorization.where(
+          project_id: subgroup_project.id,
+          user_id: user.id,
+          access_level: group_access_level)
+
+        expect(project_authorization).to exist
+      end
+    end
+
+    context 'membership is higher from group share' do
+      let(:subgroup_access_level) { Gitlab::Access::MAINTAINER }
+
+      context 'share max access level is not limiting' do
+        it 'updates authorization' do
+          create(:group_member, access_level: group_access_level, group: group, user: user)
+          create(:group_member, access_level: subgroup_access_level, group: subgroup_for_access, user: user)
+
+          subgroup_project = create_project(group_maintainer, opts)
+
+          project_authorization = ProjectAuthorization.where(
+            project_id: subgroup_project.id,
+            user_id: user.id,
+            access_level: subgroup_access_level)
+
+          expect(project_authorization).to exist
+        end
+      end
+
+      context 'share max access level is limiting' do
+        let(:share_max_access_level) { Gitlab::Access::DEVELOPER }
+
+        it 'updates authorization' do
+          create(:group_member, access_level: group_access_level, group: group, user: user)
+          create(:group_member, access_level: subgroup_access_level, group: subgroup_for_access, user: user)
+
+          subgroup_project = create_project(group_maintainer, opts)
+
+          project_authorization = ProjectAuthorization.where(
+            project_id: subgroup_project.id,
+            user_id: user.id,
+            access_level: share_max_access_level)
+
+          expect(project_authorization).to exist
+        end
+      end
+    end
+  end
+
   context 'error handling' do
     it 'handles invalid options' do
       opts[:default_branch] = 'master'