Fix Members Activate and Await Service

It's possible for membership records to be invalid when the inherited
`access_level` is higher. When using the state machine to transition
between membership states, it would validate the records and therefore
fail to change the state.

To prevent this bug from happening we skip the validation by using
`update_all`. To still guarantee valid data, we moved the validation and
restrictions to the `Members::ActivateService` and
`Members::AwaitService`.

* We now call `UserProjectAccessChangedService` when setting a member to
  be `awaiting`, which runs otherwise as an `after_commit` hook.
* We check the free user cap limits when activating a user.
* We prevent the last owner to be set to awaiting.
* We don't allow a user to set themselves to be `awaiting` anymore.

ee/app/models/ee/member.rb

@@ -161,14 +161,7 @@ def last_owner?
     end
     def has_capacity_left?
-      return true unless source.root_ancestor.apply_user_cap?
-      return true if any_existing_active_membership?
-
-      !source.root_ancestor.user_limit_reached?
-    end
-
-    def any_existing_active_membership?
-      user && ::Member.in_hierarchy(source).with_user(user).with_state(:active).any?
+      source.root_ancestor.capacity_left_for_user?(user)
     end
   end
 end

ee/app/models/ee/namespace.rb

@@ -468,6 +468,13 @@ def user_limit_reached?(use_cache: false)
       free_user_cap.reached_limit?
     end
+    def capacity_left_for_user?(user)
+      return true unless apply_user_cap?
+      return true if ::Member.in_hierarchy(root_ancestor).with_user(user).with_state(:active).exists?
+
+      !user_limit_reached?
+    end
+
     def free_plan_user_ids
       strong_memoize(:free_plan_user_ids) do
         billed_users.pluck(:id)

ee/app/services/members/activate_service.rb

@@ -26,14 +26,10 @@ def execute
       return error(_('No group provided')) unless group
       return error(_('You do not have permission to approve a member'), :forbidden) unless allowed?
       return error(_('You can only approve an indivdual user, member, or all members')) unless valid_params?
-      if activate_memberships
-        log_event
-
-        success
-      else
-        error(_('No memberships found'), :bad_request)
-      end
+      return error(_('You cannot approve all pending members on a free plan')) if activate_all && group.free_plan?
+      return error(_('There is no seat left to activate the member')) unless has_capacity_left?
+      activate_memberships
     end
     private
@@ -45,16 +41,24 @@ def valid_params?
     end
     def activate_memberships
-      memberships_found = false
       memberships = activate_all ? awaiting_memberships : scoped_memberships
+      affected_user_ids = Set.new
+
       memberships.find_each do |member|
-        memberships_found = true
-        member.activate
+        member.update_columns(state: ::Member::STATE_ACTIVE)
+        affected_user_ids.add(member.user_id)
       end
-      memberships_found
+      if !affected_user_ids.empty?
+        UserProjectAccessChangedService.new(affected_user_ids.to_a).execute(blocking: false)
+
+        log_event
+        success
+      else
+        error(_('No memberships found'), :bad_request)
+      end
     end
     # rubocop: disable CodeReuse/ActiveRecord
@@ -77,6 +81,12 @@ def allowed?
       can?(current_user, :admin_group_member, group)
     end
+    def has_capacity_left?
+      return true if activate_all && !group.free_plan?
+
+      group.root_ancestor.capacity_left_for_user?(user || member.user)
+    end
+
     def log_event
       log_params = {
         group: group.id,

ee/app/services/members/await_service.rb

@@ -15,6 +15,8 @@ def execute
       return error(_('No group provided')) unless group
       return error(_('No user provided')) unless user
       return error(_('You do not have permission to set a member awaiting')) unless allowed?
+      return error(_('The last owner cannot be set to awaiting')) if group.last_owner?(user)
+      return error(_('You cannot set yourself to awaiting')) if current_user == user
       set_memberships_to_awaiting
     end
@@ -24,14 +26,14 @@ def execute
     attr_reader :group, :current_user, :user
     def set_memberships_to_awaiting
-      memberships_found = false
-      memberships.find_each do |member|
-        memberships_found = true
-        member.wait
-      end
-      if memberships_found
+      # rubocop: disable CodeReuse/ActiveRecord
+      affected_memberships = Member.where(id: memberships)
+        .update_all(state: ::Member::STATE_AWAITING)
+      # rubocop: enable CodeReuse/ActiveRecord
+      if affected_memberships > 0
+        UserProjectAccessChangedService.new(user.id).execute(blocking: false)
         log_audit_event
         ServiceResponse.success
       else

ee/spec/models/ee/namespace_spec.rb

@@ -1826,6 +1826,39 @@
     end
   end
+  describe '#capacity_left_for_user?' do
+    let(:namespace) { create(:group) }
+    let(:user) { create(:user) }
+
+    where(:apply_user_cap, :user_limit_reached, :existing_membership, :result) do
+      false           | false              | false               | true
+      false           | false              | true                | true
+      false           | true               | true                | true
+      true            | false              | false               | true
+      true            | false              | true                | true
+      true            | true               | true                | true
+      true            | true               | false               | false
+    end
+
+    subject { namespace.capacity_left_for_user?(user) }
+
+    with_them do
+      before do
+        create(:group_member, group: namespace, user: user) if existing_membership
+
+        free_user_cap = instance_double(
+          Namespaces::FreeUserCap::Standard,
+          enforce_cap?: apply_user_cap,
+          reached_limit?: user_limit_reached
+        )
+
+        allow(namespace).to receive(:free_user_cap).and_return(free_user_cap)
+      end
+
+      it { is_expected.to eq(result) }
+    end
+  end
+
   describe '#has_free_or_no_subscription?', :saas do
     it 'returns true with a free plan' do
       namespace = create(:group_with_plan, plan: :free_plan)

ee/spec/services/members/activate_service_spec.rb

@@ -3,6 +3,140 @@
 require 'spec_helper'
 RSpec.describe Members::ActivateService do
+  shared_examples 'handles free user cap' do
+    context 'check if free user cap has been reached', :saas do
+      let_it_be_with_reload(:root_group) { create(:group_with_plan, plan: :free_plan) }
+      let_it_be_with_reload(:sub_group) { create(:group, parent: root_group) }
+      let_it_be_with_reload(:project) { create(:project, namespace: root_group) }
+
+      before do
+        allow(group).to receive(:user_cap_available?).and_return(false)
+        stub_ee_application_setting(should_check_namespace_plan: true)
+      end
+
+      context 'when the :free_user_cap feature flag is disabled' do
+        before do
+          stub_feature_flags(free_user_cap: false)
+        end
+
+        it_behaves_like 'successful member activation' do
+          let(:member) { create(:group_member, :awaiting, group: group, user: user) }
+        end
+      end
+
+      context 'when the :free_user_cap feature flag is enabled' do
+        before do
+          stub_feature_flags(free_user_cap: true)
+        end
+
+        context 'when the free user cap has not been reached' do
+          it_behaves_like 'successful member activation' do
+            let(:member) { create(:group_member, :awaiting, group: root_group, user: user) }
+          end
+
+          it_behaves_like 'successful member activation' do
+            let(:member) { create(:group_member, :awaiting, group: sub_group, user: user) }
+          end
+
+          it_behaves_like 'successful member activation' do
+            let(:member) { create(:project_member, :awaiting, project: project, user: user) }
+          end
+        end
+
+        context 'when the free user cap has been reached' do
+          before do
+            stub_const('::Namespaces::FreeUserCap::FREE_USER_LIMIT', 1)
+          end
+
+          context 'when group member' do
+            let(:member) { create(:group_member, :awaiting, group: root_group, user: user) }
+
+            it 'keeps the member awaiting' do
+              result = execute
+
+              expect(result[:status]).to eq :error
+              expect(result[:message]).to eq 'There is no seat left to activate the member'
+              expect(member.reload).to be_awaiting
+            end
+          end
+
+          context 'when sub-group member' do
+            let(:member) { create(:group_member, :awaiting, group: sub_group, user: user) }
+
+            it 'keeps the member awaiting' do
+              result = execute
+
+              expect(result[:status]).to eq :error
+              expect(result[:message]).to eq 'There is no seat left to activate the member'
+              expect(member.reload).to be_awaiting
+            end
+          end
+
+          context 'when project member' do
+            let(:member) { create(:project_member, :awaiting, project: project, user: user) }
+
+            it 'keeps the member awaiting' do
+              result = execute
+
+              expect(result[:status]).to eq :error
+              expect(result[:message]).to eq 'There is no seat left to activate the member'
+              expect(member.reload).to be_awaiting
+            end
+          end
+
+          context 'when there is already an active membership' do
+            before do
+              stub_const('::Namespaces::FreeUserCap::FREE_USER_LIMIT', 2)
+            end
+
+            context 'when active group membership' do
+              let(:member) { create(:group_member, :awaiting, group: sub_group, user: user) }
+
+              before do
+                create(:group_member, :active, group: group, user: user)
+              end
+
+              it 'sets the group member to active' do
+                execute
+
+                expect(member.reload).to be_active
+              end
+            end
+
+            context 'when active project membership' do
+              let(:member) { create(:group_member, :awaiting, group: group, user: user) }
+
+              before do
+                create(:project_member, :active, project: project, user: user)
+              end
+
+              it 'sets the group member to active' do
+                execute
+
+                expect(member.reload).to be_active
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+
+  # There is a bug where member records are not valid when the membership to the sub-group
+  # has a lower access level than the membership to the parent group.
+  # https://gitlab.com/gitlab-org/gitlab/-/issues/362091
+  shared_examples 'when user has multiple memberships with invalid access levels' do
+    let_it_be(:member) { create(:group_member, :awaiting, :developer, group: sub_group, user: user) }
+    let_it_be(:parent_membership) { create(:group_member, :awaiting, :maintainer, group: root_group, user: user) }
+
+    it 'activates all memberships' do
+      execute
+
+      expect(member.reload).to be_active
+      expect(parent_membership.reload).to be_active
+    end
+  end
+
   describe '#execute' do
     let_it_be(:current_user) { create(:user) }
     let_it_be(:user) { create(:user) }
@@ -46,6 +180,14 @@
         expect(member.reload.active?).to be true
       end
+      it 'calls UserProjectAccessChangedService' do
+        expect_next_instance_of(UserProjectAccessChangedService, [user.id]) do |service|
+          expect(service).to receive(:execute).with(blocking: false)
+        end
+
+        execute
+      end
+
       it 'logs the approval in application logs' do
         expected_params = {
           message: "Group member access approved",
@@ -149,6 +291,20 @@
             end
           end
         end
+
+        context 'when member is not member of the group' do
+          let_it_be(:member) { create(:group_member, :awaiting, group: create(:group), user: user) }
+
+          it 'returns an error' do
+            result = execute
+
+            expect(result[:status]).to eq :error
+            expect(result[:message]).to eq 'No memberships found'
+          end
+        end
+
+        it_behaves_like 'handles free user cap'
+        it_behaves_like 'when user has multiple memberships with invalid access levels'
       end
       context 'when activating a user' do
@@ -195,6 +351,20 @@
             expect(sub_member.reload.active?).to be true
           end
         end
+
+        context 'when user is not member of the group' do
+          let_it_be(:member) { create(:group_member, :awaiting, group: create(:group), user: user) }
+
+          it 'returns an error' do
+            result = execute
+
+            expect(result[:status]).to eq :error
+            expect(result[:message]).to eq 'No memberships found'
+          end
+        end
+
+        # it_behaves_like 'handles free user cap'
+        it_behaves_like 'when user has multiple memberships with invalid access levels'
       end
       context 'when activating all awaiting members' do
@@ -240,6 +410,39 @@
           execute
         end
+
+        it 'calls UserProjectAccessChangedService' do
+          double = instance_double(UserProjectAccessChangedService, :execute)
+          user_ids = [group_members, sub_group_members, project_members].flatten.map { |m| m.user_id }
+
+          expect(UserProjectAccessChangedService).to receive(:new).with(match_array(user_ids)).and_return(double)
+          expect(double).to receive(:execute).with(blocking: false)
+
+          execute
+        end
+
+        context 'when on saas', :saas do
+          context 'when group is a group with paid plan' do
+            let_it_be_with_reload(:root_group) { create(:group_with_plan, plan: :premium_plan) }
+
+            it 'is successful' do
+              result = execute
+
+              expect(result[:status]).to eq :success
+            end
+          end
+
+          context 'when group is a group with a free plan' do
+            let_it_be_with_reload(:root_group) { create(:group_with_plan, plan: :free_plan) }
+
+            it 'returns an error' do
+              result = execute
+
+              expect(result[:status]).to eq :error
+              expect(result[:message]).to eq 'You cannot approve all pending members on a free plan'
+            end
+          end
+        end
       end
     end
   end

ee/spec/services/members/await_service_spec.rb

@@ -39,6 +39,14 @@
         expect(member.reload).to be_awaiting
       end
+      it 'calls UserProjectAccessChangedService' do
+        expect_next_instance_of(UserProjectAccessChangedService, user.id) do |service|
+          expect(service).to receive(:execute).with(blocking: false)
+        end
+
+        execute
+      end
+
       it 'tracks an audit event' do
         execute
@@ -71,50 +79,96 @@
         group.add_owner(current_user)
       end
-      context 'when member of the root group' do
-        it_behaves_like 'succesfully sets member to be awaiting' do
-          let(:member) { create(:group_member, :active, group: root_group, user: user) }
+      context 'when not the last owner' do
+        let_it_be(:owner) { create(:group_member, :owner, group: root_group)}
+
+        context 'when member of the root group' do
+          it_behaves_like 'succesfully sets member to be awaiting' do
+            let(:member) { create(:group_member, :active, group: root_group, user: user) }
+          end
         end
-      end
-      context 'when member of a sub-group' do
-        it_behaves_like 'succesfully sets member to be awaiting' do
-          let(:member) { create(:group_member, :active, group: sub_group, user: user) }
+        context 'when member of a sub-group' do
+          it_behaves_like 'succesfully sets member to be awaiting' do
+            let(:member) { create(:group_member, :active, group: sub_group, user: user) }
+          end
         end
-      end
-      context 'when member is an awaiting member of a project' do
-        it_behaves_like 'succesfully sets member to be awaiting' do
-          let(:member) { create(:project_member, :active, project: project, user: user) }
+        context 'when member is an awaiting member of a project' do
+          it_behaves_like 'succesfully sets member to be awaiting' do
+            let(:member) { create(:project_member, :active, project: project, user: user) }
+          end
         end
-      end
-      context 'when there are multiple member records in the hierarchy' do
-        let_it_be(:root_member) { create(:group_member, :active, :developer, group: root_group, user: user) }
-        let_it_be(:sub_member) { create(:group_member, :active, :maintainer, group: sub_group, user: user) }
-        let_it_be(:project_member) { create(:project_member, :active, :maintainer, project: project, user: user) }
-        it 'sets them all to awaiting', :aggregate_failures do
-          expect(execute).to be_success
-          expect(root_member.reload).to be_awaiting
-          expect(sub_member.reload).to be_awaiting
-          expect(project_member.reload).to be_awaiting
+        context 'when there are multiple member records in the hierarchy' do
+          let_it_be(:root_member) { create(:group_member, :active, :developer, group: root_group, user: user) }
+          let_it_be(:sub_member) { create(:group_member, :active, :maintainer, group: sub_group, user: user) }
+          let_it_be(:project_member) { create(:project_member, :active, :maintainer, project: project, user: user) }
+          it 'sets them all to awaiting', :aggregate_failures do
+            expect(execute).to be_success
+            expect(root_member.reload).to be_awaiting
+            expect(sub_member.reload).to be_awaiting
+            expect(project_member.reload).to be_awaiting
+          end
         end
-      end
-      context 'when there are no active memberships' do
-        let_it_be(:root_member) { create(:group_member, :awaiting, :developer, group: root_group, user: user) }
-        it_behaves_like 'returns an error', 'No memberships found'
-      end
-      it 'does not affect other memberships' do
-        other_member = create(:group_member, :awaiting, group: root_group, user: create(:user))
-        execute
-        expect(other_member.reload).to be_awaiting
+        context 'when there are no active memberships' do
+          let_it_be(:root_member) { create(:group_member, :awaiting, :developer, group: root_group, user: user) }
+          it_behaves_like 'returns an error', 'No memberships found'
+        end
+        it 'does not affect other memberships' do
+          other_member = create(:group_member, :awaiting, group: root_group, user: create(:user))
+          execute
+
+          expect(other_member.reload).to be_awaiting
+        end
+
+        context 'when current_user is the same user' do
+          let_it_be(:current_user) { user }
+
+          before do
+            group.add_owner(user)
+          end
+
+          it_behaves_like 'returns an error', 'You cannot set yourself to awaiting'
+        end
+
+        context 'when user is not member of the group' do
+          let_it_be(:member) { create(:group_member, :active, group: create(:group), user: user) }
+
+          it 'returns an error' do
+            result = execute
+
+            expect(result[:status]).to eq :error
+            expect(result[:message]).to eq 'No memberships found'
+          end
+        end
+
+        # There is a bug where member records are not valid when the membership to the sub-group
+        # has a lower access level than the membership to the parent group.
+        # https://gitlab.com/gitlab-org/gitlab/-/issues/362091
+        context 'when user has multiple memberships with invalid access levels' do
+          let_it_be(:sub_membership) { create(:group_member, :active, :developer, group: sub_group, user: user) }
+          let_it_be(:parent_membership) { create(:group_member, :active, :maintainer, group: root_group, user: user) }
+
+          it 'sets all memberships to be awaiting' do
+            execute
+
+            expect(sub_membership.reload).to be_awaiting
+            expect(parent_membership.reload).to be_awaiting
+          end
+        end
+      end
+
+      context 'when user is the last owner' do
+        let_it_be(:user) { current_user }
+        it_behaves_like 'returns an error', 'The last owner cannot be set to awaiting'
       end
     end
   end

locale/gitlab.pot

@@ -37973,6 +37973,9 @@ msgstr ""
 msgid "The issue was successfully promoted to an epic. Redirecting to epic..."
 msgstr ""
+msgid "The last owner cannot be set to awaiting"
+msgstr ""
+
 msgid "The latest artifacts created by jobs in the most recent successful pipeline will be stored."
 msgstr ""
@@ -38345,6 +38348,9 @@ msgstr ""
 msgid "There is no data available. Please change your selection."
 msgstr ""
+msgid "There is no seat left to activate the member"
+msgstr ""
+
 msgid "There is no table data available."
 msgstr ""
@@ -43447,6 +43453,9 @@ msgstr ""
 msgid "You cannot access the raw file. Please wait a minute."
 msgstr ""
+msgid "You cannot approve all pending members on a free plan"
+msgstr ""
+
 msgid "You cannot approve your own deployment."
 msgstr ""
@@ -43471,6 +43480,9 @@ msgstr ""
 msgid "You cannot rename an environment after it's created."
 msgstr ""
+msgid "You cannot set yourself to awaiting"
+msgstr ""
+
 msgid "You cannot write to a read-only secondary GitLab Geo instance. Please use %{link_to_primary_node} instead."
 msgstr ""