From 521f95f1b9815572e4ae32bf1d5ad93ad06fc5f5 Mon Sep 17 00:00:00 2001
From: Hinam Mehra <hmehra@gitlab.com>
Date: Fri, 30 Aug 2024 07:18:16 +0000
Subject: [PATCH] Update billed_shared_group_members to include group links
- When calculating billed members for a group-link,
include all non-guest users and elevated guests
from the invited group.
---
app/models/group_group_link.rb | 3 +-
app/models/project_group_link.rb | 2 +-
ee/app/models/ee/group.rb | 47 +++++-
ee/app/models/ee/group_group_link.rb | 2 +
ee/lib/ee/api/members.rb | 5 +-
ee/spec/models/ee/group_spec.rb | 204 +++++++++++++++++++++++++++
6 files changed, 256 insertions(+), 7 deletions(-)
diff --git a/app/models/group_group_link.rb b/app/models/group_group_link.rb
index 8ae74fa60e497..470f62e2e797a 100644
--- a/app/models/group_group_link.rb
+++ b/app/models/group_group_link.rb
@@ -12,7 +12,8 @@ class GroupGroupLink < ApplicationRecord
validates :shared_with_group, presence: true
validates :group_access, inclusion: { in: Gitlab::Access.all_values }, presence: true
- scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) }
+ scope :guests, -> { where(group_access: Gitlab::Access::GUEST) }
+ scope :non_guests, -> { where('group_group_links.group_access > ?', Gitlab::Access::GUEST) }
scope :for_shared_groups, ->(group_ids) { where(shared_group_id: group_ids) }
scope :with_owner_or_maintainer_access, -> do
diff --git a/app/models/project_group_link.rb b/app/models/project_group_link.rb
index 95e83d96c1037..ed6bd21ec7c28 100644
--- a/app/models/project_group_link.rb
+++ b/app/models/project_group_link.rb
@@ -14,7 +14,7 @@ class ProjectGroupLink < ApplicationRecord
validates :group_access, inclusion: { in: Gitlab::Access.all_values }, presence: true
validate :different_group
- scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) }
+ scope :non_guests, -> { where('project_group_links.group_access > ?', Gitlab::Access::GUEST) }
scope :in_group, ->(group_ids) { where(group_id: group_ids) }
scope :for_projects, ->(project_ids) { where(project_id: project_ids) }
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index d7ede467da1f4..fe3af8905d59a 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -143,6 +143,12 @@ module Group
.merge(guests_scope)
end
+ scope :invited_groups_with_guest_member_role, ->(group) do
+ joins(:shared_group_links)
+ .where(group_group_links: { shared_group_id: group.self_and_descendants })
+ .merge(::GroupGroupLink.guests.with_custom_role)
+ end
+
scope :invited_groups_in_projects_for_hierarchy, ->(group, exclude_guests = false) do
guests_scope = exclude_guests ? ::ProjectGroupLink.non_guests : ::ProjectGroupLink.all
@@ -921,7 +927,28 @@ def billed_shared_group_users(exclude_guests: false)
def billed_shared_group_members(exclude_guests: false)
groups = self.class.invited_groups_in_groups_for_hierarchy(self, exclude_guests)
- invited_or_shared_group_members(groups, exclude_guests: exclude_guests).not_banned_in(root_ancestor)
+
+ # gets all billable members from group-invites with access_level > GUEST
+ members = invited_or_shared_group_members(groups, exclude_guests: exclude_guests)
+
+ # gets all billable members from group-invites with access_level = GUEST + custom_role
+ members_with_custom_role = billed_shared_guest_group_members_with_custom_role(exclude_guests: exclude_guests)
+
+ # merge both and return
+ # see acceptance criteria - https://gitlab.com/gitlab-org/gitlab/-/issues/443369#note_2045035173
+ ::GroupMember.from_union([members, members_with_custom_role]).not_banned_in(root_ancestor)
+ end
+
+ def billed_shared_guest_group_members_with_custom_role(exclude_guests: false)
+ return ::GroupMember.none unless exclude_guests
+ return ::GroupMember.none unless ::Feature.enabled?(:assign_custom_roles_to_group_links_saas, self)
+
+ # invited groups that have access_level = GUEST + custom_role
+ groups = self.class.invited_groups_with_guest_member_role(self)
+
+ # get all members from those invited groups that have
+ # access_level = GUEST + custom_role (that has occupies_seat = TRUE)
+ members_with_elevating_guest_member_role(groups)
end
# Members belonging to Groups invited to collaborate with Projects
@@ -1107,11 +1134,23 @@ def billed_user_ids_including_guests
end
def invited_or_shared_group_members(groups, exclude_guests: false)
- guests_scope = exclude_guests ? ::GroupMember.non_guests : ::GroupMember.all
+ non_guests_scope = if ::Feature.enabled?(:assign_custom_roles_to_group_links_saas, self)
+ ::GroupMember.with_elevated_guests
+ else
+ ::GroupMember.non_guests
+ end
+
+ guests_scope = exclude_guests ? non_guests_scope : ::GroupMember.all
+
+ ::GroupMember.active_without_invites_and_requests
+ .with_source_id(groups.self_and_ancestors)
+ .merge(guests_scope)
+ end
+ def members_with_elevating_guest_member_role(groups)
::GroupMember.active_without_invites_and_requests
- .with_source_id(groups.self_and_ancestors)
- .merge(guests_scope)
+ .with_source_id(groups.self_and_ancestors)
+ .merge(::GroupMember.elevated_guests)
end
def users_without_bots(members, merge_condition: ::User.all)
diff --git a/ee/app/models/ee/group_group_link.rb b/ee/app/models/ee/group_group_link.rb
index 6cd23f04bddd0..c9b20ee247f9c 100644
--- a/ee/app/models/ee/group_group_link.rb
+++ b/ee/app/models/ee/group_group_link.rb
@@ -15,6 +15,8 @@ module GroupGroupLink
scope :in_shared_group, ->(shared_groups) { where(shared_group: shared_groups) }
scope :not_in_shared_with_group, ->(shared_with_groups) { where.not(shared_with_group: shared_with_groups) }
+ scope :with_custom_role, -> { where.not(member_role_id: nil) }
+
validate :group_with_allowed_email_domains
end
diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb
index 47752879f70b0..d19c71139f590 100644
--- a/ee/lib/ee/api/members.rb
+++ b/ee/lib/ee/api/members.rb
@@ -212,7 +212,10 @@ module Members
invited_group_to_project_memberships = group.billed_invited_group_to_project_members(exclude_guests: exclude_guests).with_user(user.id)
invited_group_to_group_memberships = group.billed_shared_group_members(exclude_guests: exclude_guests).with_user(user.id)
- invited_memberships = invited_group_to_group_memberships.or(invited_group_to_project_memberships)
+ invited_memberships = ::GroupMember.from_union([
+ invited_group_to_group_memberships,
+ invited_group_to_project_memberships
+ ])
present paginate(invited_memberships), with: ::EE::API::Entities::BillableMembership
end
diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb
index dc59db7f75000..252e69593c509 100644
--- a/ee/spec/models/ee/group_spec.rb
+++ b/ee/spec/models/ee/group_spec.rb
@@ -1703,6 +1703,210 @@
expect(group.billed_shared_group_members.map(&:user)).to exclude(banned_user)
end
end
+
+ context 'with member roles' do
+ let_it_be(:group) { create(:group) }
+ let_it_be(:invited_group) { create(:group) }
+
+ let_it_be(:guest_elevated) { create(:member_role, :guest, :read_vulnerability) }
+ let_it_be(:guest_basic) { create(:member_role, :guest, :read_code) }
+ let_it_be(:developer_lead) { create(:member_role, :developer, :admin_vulnerability) }
+
+ let_it_be(:member_a) { create(:group_member, :guest, source: invited_group) }
+ let_it_be(:member_b) { create(:group_member, :guest, source: invited_group, member_role: guest_elevated) }
+ let_it_be(:member_c) { create(:group_member, :guest, source: invited_group, member_role: guest_basic) }
+ let_it_be(:member_d) { create(:group_member, :developer, source: invited_group) }
+ let_it_be(:member_e) { create(:group_member, :developer, source: invited_group, member_role: developer_lead) }
+
+ let_it_be(:invited_access_level) { :guest }
+ let_it_be(:invited_member_role) { nil }
+
+ before do
+ create(
+ :group_group_link,
+ invited_access_level,
+ shared_with_group: invited_group,
+ shared_group: group,
+ member_role: invited_member_role
+ )
+ end
+
+ shared_examples 'returns all members of the invited group' do
+ it do
+ expect(group.billed_shared_group_members)
+ .to match_array([member_a, member_b, member_c, member_d, member_e])
+ end
+ end
+
+ shared_examples 'returns empty array' do
+ it do
+ expect(group.billed_shared_group_members(exclude_guests: true)).to match_array([])
+ end
+ end
+
+ shared_examples 'returns elevated guests from invited group' do
+ it do
+ expect(group.billed_shared_group_members(exclude_guests: true)).to match_array([member_b])
+ end
+ end
+
+ shared_examples 'returns non-guests and elevated guests from invited group' do
+ it do
+ expect(group.billed_shared_group_members(exclude_guests: true)).to match_array([member_b, member_d, member_e])
+ end
+ end
+
+ shared_examples 'returns non-guests from invited group' do
+ it do
+ expect(group.billed_shared_group_members(exclude_guests: true)).to match_array([member_d, member_e])
+ end
+ end
+
+ context 'when group link is assigned a guest role' do
+ let(:invited_access_level) { :guest }
+ let(:invited_member_role) { nil }
+
+ context 'when feature-flag `assign_custom_roles_to_group_links_saas` is enabled' do
+ context 'with guests' do
+ it_behaves_like 'returns all members of the invited group'
+ end
+
+ context 'without guests' do
+ it_behaves_like 'returns empty array'
+ end
+ end
+
+ context 'when feature-flag `assign_custom_roles_to_group_links_saas` is disabled' do
+ before do
+ stub_feature_flags(assign_custom_roles_to_group_links_saas: false)
+ end
+
+ context 'with guests' do
+ it_behaves_like 'returns all members of the invited group'
+ end
+
+ context 'without guests' do
+ it_behaves_like 'returns empty array'
+ end
+ end
+ end
+
+ context 'when group link is assigned a guest member role that does not occupy a seat' do
+ let(:invited_access_level) { :guest }
+ let(:invited_member_role) { guest_basic }
+
+ context 'when feature-flag `assign_custom_roles_to_group_links_saas` is enabled' do
+ context 'with guests' do
+ it_behaves_like 'returns all members of the invited group'
+ end
+
+ context 'without guests' do
+ it_behaves_like 'returns elevated guests from invited group'
+ end
+ end
+
+ context 'when feature-flag `assign_custom_roles_to_group_links_saas` is disabled' do
+ before do
+ stub_feature_flags(assign_custom_roles_to_group_links_saas: false)
+ end
+
+ context 'with guests' do
+ it_behaves_like 'returns all members of the invited group'
+ end
+
+ context 'without guests' do
+ it_behaves_like 'returns empty array'
+ end
+ end
+ end
+
+ context 'when group link is assigned a guest member role that occupies a seat' do
+ let(:invited_access_level) { :guest }
+ let(:invited_member_role) { guest_elevated }
+
+ context 'when feature-flag `assign_custom_roles_to_group_links_saas` is enabled' do
+ context 'with guests' do
+ it_behaves_like 'returns all members of the invited group'
+ end
+
+ context 'without guests' do
+ it_behaves_like 'returns elevated guests from invited group'
+ end
+ end
+
+ context 'when feature-flag `assign_custom_roles_to_group_links_saas` is disabled' do
+ before do
+ stub_feature_flags(assign_custom_roles_to_group_links_saas: false)
+ end
+
+ context 'with guests' do
+ it_behaves_like 'returns all members of the invited group'
+ end
+
+ context 'without guests' do
+ it_behaves_like 'returns empty array'
+ end
+ end
+ end
+
+ context 'when group link is assigned a non-guest role' do
+ let(:invited_access_level) { :developer }
+ let(:invited_member_role) { nil }
+
+ context 'when feature-flag `assign_custom_roles_to_group_links_saas` is enabled' do
+ context 'with guests' do
+ it_behaves_like 'returns all members of the invited group'
+ end
+
+ context 'without guests' do
+ it_behaves_like 'returns non-guests and elevated guests from invited group'
+ end
+ end
+
+ context 'when feature-flag `assign_custom_roles_to_group_links_saas` is disabled' do
+ before do
+ stub_feature_flags(assign_custom_roles_to_group_links_saas: false)
+ end
+
+ context 'with guests' do
+ it_behaves_like 'returns all members of the invited group'
+ end
+
+ context 'without guests' do
+ it_behaves_like 'returns non-guests from invited group'
+ end
+ end
+ end
+
+ context 'when group link is assigned a non-guest custom role' do
+ let(:invited_access_level) { :developer }
+ let(:invited_member_role) { developer_lead }
+
+ context 'when feature-flag `assign_custom_roles_to_group_links_saas` is enabled' do
+ context 'with guests' do
+ it_behaves_like 'returns all members of the invited group'
+ end
+
+ context 'without guests' do
+ it_behaves_like 'returns non-guests and elevated guests from invited group'
+ end
+ end
+
+ context 'when feature-flag `assign_custom_roles_to_group_links_saas` is disabled' do
+ before do
+ stub_feature_flags(assign_custom_roles_to_group_links_saas: false)
+ end
+
+ context 'with guests' do
+ it_behaves_like 'returns all members of the invited group'
+ end
+
+ context 'without guests' do
+ it_behaves_like 'returns non-guests from invited group'
+ end
+ end
+ end
+ end
end
describe '#billed_invited_group_to_project_users' do
--
GitLab