Merge branch 'pending-group-member-access-part2' into 'master'

Exclude Pending Memberships from Billable Members Count

See merge request gitlab-org/gitlab!79377

app/models/member.rb

@@ -108,6 +108,8 @@ class Member < ApplicationRecord
       .reorder(nil)
   end
 
+  scope :active_state, -> { where(state: STATE_ACTIVE) }
+
   scope :connected_to_user, -> { where.not(user_id: nil) }
 
   # This scope is exclusively used to get the members
@@ -128,7 +130,8 @@ class Member < ApplicationRecord
   end
 
   scope :without_invites_and_requests, -> do
-    non_request
+    active_state
+      .non_request
       .non_invite
       .non_minimal_access
   end

ee/spec/features/pending_group_memberships_spec.rb

@@ -12,6 +12,28 @@ RSpec.describe 'Pending group memberships', :js do
   context 'with a public group' do
     let_it_be(:group) { create(:group, :public) }
 
+    it 'a pending member sees a public group as if not a member' do
+      create(:group_member, :awaiting, :developer, source: group, user: developer)
+
+      visit group_path(group)
+
+      expect(page).to have_content "Group ID: #{group.id}"
+      expect(page).not_to have_content "New project"
+      expect(page).not_to have_content "Recent activity"
+    end
+
+    it 'a pending member sees a public group with a project as if not a member' do
+      project = create(:project, :public, namespace: group)
+      create(:group_member, :awaiting, :developer, source: group, user: developer)
+
+      visit group_path(group)
+
+      expect(page).to have_content "Group ID: #{group.id}"
+      expect(page).to have_content project.name
+      expect(page).not_to have_content "New project"
+      expect(page).not_to have_content "Recent activity"
+    end
+
     it 'a pending group member gets a 404 for a private project in the group' do
       project = create(:project, :private, namespace: group)
       create(:group_member, :awaiting, :developer, source: group, user: developer)
@@ -22,10 +44,38 @@ RSpec.describe 'Pending group memberships', :js do
     end
   end
 
+  context 'with a private group' do
+    let_it_be(:group) { create(:group, :private) }
+
+    it 'a pending member gets a 404 for a private group' do
+      create(:group_member, :awaiting, :developer, source: group, user: developer)
+
+      visit group_path(group)
+
+      expect(page).to have_content 'Page Not Found'
+    end
+  end
+
   context 'with a subgroup' do
     let_it_be(:group) { create(:group, :private) }
     let_it_be(:subgroup) { create(:group, :private, parent: group) }
 
+    it 'a pending member of the root group sees the root group as if not a member' do
+      create(:group_member, :awaiting, :developer, source: group, user: developer)
+
+      visit group_path(group)
+
+      expect(page).to have_content 'Page Not Found'
+    end
+
+    it 'a pending member of the root group sees a subgroup as if not a member' do
+      create(:group_member, :awaiting, :developer, source: group, user: developer)
+
+      visit group_path(subgroup)
+
+      expect(page).to have_content 'Page Not Found'
+    end
+
     it 'a pending member of the root group sees a subgroup project as if not a member' do
       project = create(:project, :private, namespace: subgroup)
       create(:group_member, :awaiting, :developer, source: group, user: developer)

ee/spec/models/ee/namespace_spec.rb

@@ -963,6 +963,7 @@ RSpec.describe Namespace do
           group.add_developer(developer)
           group.add_developer(create(:user, :blocked))
           group.add_guest(guest)
+          create(:group_member, :awaiting, :developer, source: group)
         end
 
         subject(:billed_user_ids) { group.billed_user_ids }
@@ -991,6 +992,7 @@ RSpec.describe Namespace do
               project.add_guest(create(:user))
               project.add_developer(developer)
               project.add_developer(create(:user, :blocked))
+              create(:project_member, :awaiting, :developer, source: project)
             end
 
             it 'includes invited active users except guests to the group', :aggregate_failures do
@@ -1024,6 +1026,7 @@ RSpec.describe Namespace do
                 invited_group.add_guest(create(:user))
                 invited_group.add_developer(create(:user, :blocked))
                 invited_group.add_developer(developer)
+                create(:group_member, :awaiting, :developer, source: invited_group)
               end
 
               context 'when group is invited as non guest' do
@@ -1061,6 +1064,7 @@ RSpec.describe Namespace do
               shared_group.add_developer(shared_group_developer)
               shared_group.add_guest(create(:user))
               shared_group.add_developer(create(:user, :blocked))
+              create(:group_member, :awaiting, :developer, source: shared_group)
 
               create(:group_group_link, { shared_with_group: shared_group,
                                           shared_group: group })
@@ -1080,6 +1084,7 @@ RSpec.describe Namespace do
                 another_shared_group.add_developer(another_shared_group_developer)
                 another_shared_group.add_guest(create(:user))
                 another_shared_group.add_developer(create(:user, :blocked))
+                create(:group_member, :awaiting, :developer, source: another_shared_group)
               end
 
               context 'when subgroup invites another group as non guest' do
@@ -1150,6 +1155,7 @@ RSpec.describe Namespace do
                 project.add_guest(project_guest)
                 project.add_developer(create(:user, :blocked))
                 project.add_developer(developer)
+                create(:project_member, :awaiting, :developer, source: project)
               end
 
               it 'includes invited active users to the group', :aggregate_failures do
@@ -1181,6 +1187,7 @@ RSpec.describe Namespace do
                   invited_group.add_developer(developer)
                   invited_group.add_guest(invited_group_guest)
                   invited_group.add_developer(create(:user, :blocked))
+                  create(:group_member, :awaiting, :developer, source: invited_group)
                   create(:project_group_link, project: project, group: invited_group)
                 end
 
@@ -1213,6 +1220,7 @@ RSpec.describe Namespace do
                 shared_group.add_developer(shared_group_developer)
                 shared_group.add_guest(shared_group_guest)
                 shared_group.add_developer(create(:user, :blocked))
+                create(:group_member, :awaiting, :developer, source: shared_group)
 
                 create(:group_group_link, { shared_with_group: shared_group,
                                             shared_group: group })
@@ -1249,6 +1257,7 @@ RSpec.describe Namespace do
         group.add_developer(developer)
         group.add_developer(create(:user, :blocked))
         group.add_guest(create(:user))
+        create(:group_member, :awaiting, :developer, source: group)
       end
 
       context 'with an ultimate plan' do
@@ -1256,7 +1265,7 @@ RSpec.describe Namespace do
           create(:gitlab_subscription, namespace: group, hosted_plan: ultimate_plan)
         end
 
-        it 'counts only active users with an access level higher than guest' do
+        it 'counts only active users with an active membership with an access level higher than guest' do
           expect(group.billable_members_count).to eq(1)
         end
 
@@ -1268,9 +1277,10 @@ RSpec.describe Namespace do
             project.add_guest(create(:user))
             project.add_developer(developer)
             project.add_developer(create(:user, :blocked))
+            create(:project_member, :awaiting, :developer, source: project)
           end
 
-          it 'includes invited active users except guests' do
+          it 'includes invited active users except guests and awaiting members' do
             expect(group.billable_members_count).to eq(2)
           end
 
@@ -1291,6 +1301,7 @@ RSpec.describe Namespace do
               invited_group.add_guest(create(:user))
               invited_group.add_developer(create(:user, :blocked))
               invited_group.add_developer(developer)
+              create(:group_member, :awaiting, :developer, source: invited_group)
               create(:project_group_link, project: project, group: invited_group)
             end
 
@@ -1307,6 +1318,7 @@ RSpec.describe Namespace do
             other_group.add_developer(create(:user))
             other_group.add_guest(create(:user))
             other_group.add_developer(create(:user, :blocked))
+            create(:group_member, :awaiting, :developer, source: other_group)
 
             create(:group_group_link, { shared_with_group: other_group,
                                         shared_group: group })
@@ -1334,6 +1346,7 @@ RSpec.describe Namespace do
               project.add_guest(create(:user))
               project.add_developer(create(:user, :blocked))
               project.add_developer(developer)
+              create(:project_member, :awaiting, :developer, source: project)
             end
 
             it 'includes invited active users to the group' do
@@ -1357,6 +1370,7 @@ RSpec.describe Namespace do
                 invited_group.add_developer(developer)
                 invited_group.add_guest(create(:user))
                 invited_group.add_developer(create(:user, :blocked))
+                create(:group_member, :awaiting, :developer, source: invited_group)
                 create(:project_group_link, project: project, group: invited_group)
               end
 
@@ -1374,6 +1388,7 @@ RSpec.describe Namespace do
               other_group.add_developer(create(:user))
               other_group.add_guest(create(:user))
               other_group.add_developer(create(:user, :blocked))
+              create(:group_member, :awaiting, :developer, source: other_group)
 
               create(:group_group_link, { shared_with_group: other_group,
                                           shared_group: group })

ee/spec/policies/group_policy_spec.rb

@@ -1811,4 +1811,63 @@ RSpec.describe GroupPolicy do
       it { is_expected.to(be_disallowed(:admin_external_audit_events)) }
     end
   end
+
+  describe 'pending memberships' do
+    let_it_be(:user) { create(:user) }
+
+    context 'with a private group' do
+      let_it_be(:private_group) { create(:group, :private) }
+
+      subject { described_class.new(user, private_group) }
+
+      context 'developer' do
+        before do
+          create(:group_member, :awaiting, :developer, source: private_group, user: user)
+        end
+
+        it 'has permission identical to a private group in which the user is not a member' do
+          expect_private_group_permissions_as_if_non_member
+        end
+
+        context 'with a project in the group' do
+          let_it_be(:project) { create(:project, :private, namespace: private_group) }
+
+          it 'has permission identical to a private group in which the user is not a member' do
+            expect_private_group_permissions_as_if_non_member
+          end
+        end
+      end
+    end
+
+    context 'with a public group' do
+      let_it_be(:public_group) { create(:group, :public, :crm_enabled) }
+
+      subject { described_class.new(user, public_group) }
+
+      context 'developer' do
+        before do
+          create(:group_member, :awaiting, :developer, source: public_group, user: user)
+        end
+
+        it 'has permission identical to a public group in which the user is not a member' do
+          expect_allowed(*public_permissions)
+          expect_disallowed(:upload_file)
+          expect_disallowed(*reporter_permissions)
+          expect_disallowed(*developer_permissions)
+          expect_disallowed(*maintainer_permissions)
+          expect_disallowed(*owner_permissions)
+          expect_disallowed(:read_namespace)
+        end
+      end
+    end
+
+    def expect_private_group_permissions_as_if_non_member
+      expect_disallowed(*public_permissions)
+      expect_disallowed(*guest_permissions)
+      expect_disallowed(*reporter_permissions)
+      expect_disallowed(*developer_permissions)
+      expect_disallowed(*maintainer_permissions)
+      expect_disallowed(*owner_permissions)
+    end
+  end
 end

spec/models/member_spec.rb

@@ -173,6 +173,8 @@ RSpec.describe Member do
     let_it_be(:group) { create(:group) }
     let_it_be(:blocked_pending_approval_user) { create(:user, :blocked_pending_approval ) }
     let_it_be(:blocked_pending_approval_project_member) { create(:project_member, :invited, :developer, project: project, invite_email: blocked_pending_approval_user.email) }
+    let_it_be(:awaiting_group_member) { create(:group_member, :awaiting, group: group) }
+    let_it_be(:awaiting_project_member) { create(:project_member, :awaiting, project: project) }
 
     before_all do
       @owner_user = create(:user).tap { |u| group.add_owner(u) }
@@ -471,6 +473,8 @@ RSpec.describe Member do
       it { is_expected.to include @blocked_maintainer }
       it { is_expected.to include @blocked_developer }
       it { is_expected.not_to include @member_with_minimal_access }
+      it { is_expected.not_to include awaiting_group_member }
+      it { is_expected.not_to include awaiting_project_member }
     end
 
     describe '.connected_to_user' do
@@ -561,6 +565,21 @@ RSpec.describe Member do
         end
       end
     end
+
+    describe '.active_state' do
+      let_it_be(:active_group_member) { create(:group_member, group: group) }
+      let_it_be(:active_project_member) { create(:project_member, project: project) }
+
+      it 'includes members with an active state' do
+        expect(group.members.active_state).to include active_group_member
+        expect(project.members.active_state).to include active_project_member
+      end
+
+      it 'does not include members with an awaiting state' do
+        expect(group.members.active_state).not_to include awaiting_group_member
+        expect(project.members.active_state).not_to include awaiting_project_member
+      end
+    end
   end
 
   describe 'Delegate methods' do