Merge branch '336623-members-api-should-go-through-create-service' into 'master'

Send Members API through create service

See merge request gitlab-org/gitlab!69295

app/models/group.rb

@@ -656,6 +656,10 @@ class Group < Namespace
     members.owners.connected_to_user.order_recent_sign_in.limit(Member::ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT)
   end
 
+  def membership_locked?
+    false # to support project and group calling this as 'source'
+  end
+
   def supports_events?
     false
   end

app/models/project.rb

@@ -1677,6 +1677,10 @@ class Project < ApplicationRecord
     end
   end
 
+  def membership_locked?
+    false
+  end
+
   def bots
     users.project_bot
   end

app/services/members/create_service.rb

@@ -4,9 +4,12 @@ module Members
   class CreateService < Members::BaseService
     BlankInvitesError = Class.new(StandardError)
     TooManyInvitesError = Class.new(StandardError)
+    MembershipLockedError = Class.new(StandardError)
 
     DEFAULT_INVITE_LIMIT = 100
 
+    attr_reader :membership_locked
+
     def initialize(*args)
       super
 
@@ -17,18 +20,22 @@ module Members
 
     def execute
       validate_invite_source!
-      validate_invites!
+      validate_invitable!
 
       add_members
       enqueue_onboarding_progress_action
       result
-    rescue BlankInvitesError, TooManyInvitesError => e
+    rescue BlankInvitesError, TooManyInvitesError, MembershipLockedError => e
       error(e.message)
     end
 
+    def single_member
+      members.last
+    end
+
     private
 
-    attr_reader :source, :errors, :invites, :member_created_namespace_id
+    attr_reader :source, :errors, :invites, :member_created_namespace_id, :members
 
     def invites_from_params
       params[:user_ids]
@@ -38,7 +45,7 @@ module Members
       raise ArgumentError, s_('AddMember|No invite source provided.') unless invite_source.present?
     end
 
-    def validate_invites!
+    def validate_invitable!
       raise BlankInvitesError, blank_invites_message if invites.blank?
 
       return unless user_limit && invites.size > user_limit
@@ -52,7 +59,7 @@ module Members
     end
 
     def add_members
-      members = source.add_users(
+      @members = source.add_users(
         invites,
         params[:access_level],
         expires_at: params[:expires_at],

app/services/members/invite_service.rb

@@ -18,7 +18,7 @@ module Members
       params[:email]
     end
 
-    def validate_invites!
+    def validate_invitable!
       super
 
       # we need the below due to add_users hitting Members::CreatorService.parse_users_list and ignoring invalid emails

ee/app/models/ee/project.rb

@@ -470,6 +470,13 @@ module EE
       super && !(group && ::Gitlab::CurrentSettings.lock_memberships_to_ldap?)
     end
 
+    override :membership_locked?
+    def membership_locked?
+      return false unless group
+
+      group.membership_lock?
+    end
+
     # TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329
     def approvals_before_merge
       return 0 unless feature_available?(:merge_request_approvers)

ee/app/services/ee/members/create_service.rb

@@ -5,9 +5,10 @@ module EE
     module CreateService
       private
 
-      def validate_invites!
+      def validate_invitable!
         super
 
+        check_membership_lock!
         check_quota!
       end
 
@@ -21,6 +22,13 @@ module EE
               )
       end
 
+      def check_membership_lock!
+        return unless source.membership_locked?
+
+        @membership_locked = true # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        raise ::Members::CreateService::MembershipLockedError
+      end
+
       def invite_quota_exceeded?
         return unless source.actual_limits.daily_invites
 

ee/lib/ee/api/helpers/members_helpers.rb

@@ -51,17 +51,6 @@ module EE
           can?(current_user, :read_group_saml_identity, members_source)
         end
 
-        override :create_member
-        def create_member(current_user, user, source, params)
-          member = super
-
-          return false unless member
-
-          log_audit_event(member) if member.persisted? && member.valid?
-
-          member
-        end
-
         def find_member(params)
           source = find_source(:group, params.delete(:id))
           authorize! :override_group_member, source
@@ -76,14 +65,6 @@ module EE
             render_validation_error!(updated_member)
           end
         end
-
-        def log_audit_event(member)
-          ::AuditEventService.new(
-            current_user,
-            member.source,
-            action: :create
-          ).for_member(member).security_event
-        end
       end
     end
   end

ee/spec/lib/ee/api/helpers/members_helpers_spec.rb

@@ -10,32 +10,6 @@ RSpec.describe EE::API::Helpers::MembersHelpers do
     allow(members_helpers).to receive(:current_user).and_return(create(:user))
   end
 
-  shared_examples 'creates security_event' do |source_type|
-    context "with :source_type == #{source_type.pluralize}" do
-      it 'creates security_event' do
-        security_event = members_helpers.log_audit_event(member)
-
-        expect(security_event.entity_id).to eq(source.id)
-        expect(security_event.entity_type).to eq(source_type.capitalize)
-        expect(security_event.details.fetch(:target_id)).to eq(member.id)
-      end
-    end
-  end
-
-  describe '#log_audit_event' do
-    subject { members_helpers }
-
-    it_behaves_like 'creates security_event', 'group' do
-      let(:source) { create(:group) }
-      let(:member) { create(:group_member, :owner, group: source, user: create(:user)) }
-    end
-
-    it_behaves_like 'creates security_event', 'project' do
-      let(:source) { create(:project) }
-      let(:member) { create(:project_member, project: source, user: create(:user)) }
-    end
-  end
-
   describe '.member_sort_options' do
     it 'lists all keys available in group member view' do
       sort_options = %w[

ee/spec/models/project_spec.rb

@@ -928,6 +928,35 @@ RSpec.describe Project do
     end
   end
 
+  describe '#membership_locked?' do
+    let(:project) { build_stubbed(:project, group: group) }
+    let(:group) { nil }
+
+    context 'when project has no group' do
+      let(:project) { Project.new }
+
+      it 'is false' do
+        expect(project).not_to be_membership_locked
+      end
+    end
+
+    context 'with group_membership_lock enabled' do
+      let(:group) { build_stubbed(:group, membership_lock: true) }
+
+      it 'is true' do
+        expect(project).to be_membership_locked
+      end
+    end
+
+    context 'with group_membership_lock disabled' do
+      let(:group) { build_stubbed(:group, membership_lock: false) }
+
+      it 'is false' do
+        expect(project).not_to be_membership_locked
+      end
+    end
+  end
+
   describe '#feature_available?' do
     let(:namespace) { build(:namespace) }
     let(:plan_license) { nil }

lib/api/helpers/members_helpers.rb

@@ -50,24 +50,48 @@ module API
         GroupMembersFinder.new(group).execute
       end
 
-      def create_member(current_user, user, source, params)
-        source.add_user(user, params[:access_level], current_user: current_user, expires_at: params[:expires_at])
+      def present_members(members)
+        present members, with: Entities::Member, current_user: current_user, show_seat_info: params[:show_seat_info]
       end
 
-      def track_areas_of_focus(member, areas_of_focus)
-        return unless areas_of_focus
+      def present_member_invitations(invitations)
+        present invitations, with: Entities::Invitation, current_user: current_user
+      end
+
+      def add_single_member_by_user_id(create_service_params)
+        source = create_service_params[:source]
+        user_id = create_service_params[:user_ids]
+        user = User.find_by(id: user_id) # rubocop: disable CodeReuse/ActiveRecord
+
+        if user
+          conflict!('Member already exists') if member_already_exists?(source, user_id)
+
+          instance = ::Members::CreateService.new(current_user, create_service_params)
+          instance.execute
+
+          not_allowed! if instance.membership_locked # This currently can only be reached in EE if group membership is locked
 
-        areas_of_focus.each do |area_of_focus|
-          Gitlab::Tracking.event(::Members::CreateService.name, 'area_of_focus', label: area_of_focus, property: member.id.to_s)
+          member = instance.single_member
+          render_validation_error!(member) if member.invalid?
+
+          present_members(member)
+        else
+          not_found!('User')
         end
       end
 
-      def present_members(members)
-        present members, with: Entities::Member, current_user: current_user, show_seat_info: params[:show_seat_info]
+      def add_multiple_members?(user_id)
+        user_id.include?(',')
       end
 
-      def present_member_invitations(invitations)
-        present invitations, with: Entities::Invitation, current_user: current_user
+      def add_single_member?(user_id)
+        user_id.present?
+      end
+
+      private
+
+      def member_already_exists?(source, user_id)
+        source.members.exists?(user_id: user_id) # rubocop: disable CodeReuse/ActiveRecord
       end
     end
   end

lib/api/members.rb

@@ -96,42 +96,22 @@ module API
           optional :invite_source, type: String, desc: 'Source that triggered the member creation process', default: 'members-api'
           optional :areas_of_focus, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Areas the inviter wants the member to focus upon'
         end
-        # rubocop: disable CodeReuse/ActiveRecord
+
         post ":id/members" do
           ::Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/333434')
 
           source = find_source(source_type, params[:id])
           authorize_admin_source!(source_type, source)
 
-          if params[:user_id].to_s.include?(',')
-            create_service_params = params.except(:user_id).merge({ user_ids: params[:user_id], source: source })
+          user_id = params[:user_id].to_s
+          create_service_params = params.except(:user_id).merge({ user_ids: user_id, source: source })
 
+          if add_multiple_members?(user_id)
             ::Members::CreateService.new(current_user, create_service_params).execute
-          elsif params[:user_id].present?
-            member = source.members.find_by(user_id: params[:user_id])
-            conflict!('Member already exists') if member
-
-            user = User.find_by_id(params[:user_id])
-            not_found!('User') unless user
-
-            member = create_member(current_user, user, source, params)
-
-            if !member
-              not_allowed! # This currently can only be reached in EE
-            elsif member.valid? && member.persisted?
-              present_members(member)
-              Gitlab::Tracking.event(::Members::CreateService.name,
-                                     'create_member',
-                                     label: params[:invite_source],
-                                     property: 'existing_user',
-                                     user: current_user)
-              track_areas_of_focus(member, params[:areas_of_focus])
-            else
-              render_validation_error!(member)
-            end
+          elsif add_single_member?(user_id)
+            add_single_member_by_user_id(create_service_params)
           end
         end
-        # rubocop: enable CodeReuse/ActiveRecord
 
         desc 'Updates a member of a group or project.' do
           success Entities::Member

spec/models/group_spec.rb

@@ -2486,6 +2486,12 @@ RSpec.describe Group do
     end
   end
 
+  describe '#membership_locked?' do
+    it 'returns false' do
+      expect(build(:group)).not_to be_membership_locked
+    end
+  end
+
   describe '#default_owner' do
     let(:group) { build(:group) }
 

spec/models/project_spec.rb

@@ -604,6 +604,12 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
+  describe '#membership_locked?' do
+    it 'returns false' do
+      expect(build(:project)).not_to be_membership_locked
+    end
+  end
+
   describe '#autoclose_referenced_issues' do
     context 'when DB entry is nil' do
       let(:project) { build(:project, autoclose_referenced_issues: nil) }

spec/requests/api/members_spec.rb

@@ -311,36 +311,6 @@ RSpec.describe API::Members do
             expect(json_response['status']).to eq('error')
             expect(json_response['message']).to eq(error_message)
           end
-
-          context 'with invite_source considerations', :snowplow do
-            let(:params) { { user_id: user_ids, access_level: Member::DEVELOPER } }
-
-            it 'tracks the invite source as api' do
-              post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
-                   params: params
-
-              expect_snowplow_event(
-                category: 'Members::CreateService',
-                action: 'create_member',
-                label: 'members-api',
-                property: 'existing_user',
-                user: maintainer
-              )
-            end
-
-            it 'tracks the invite source from params' do
-              post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
-                   params: params.merge(invite_source: '_invite_source_')
-
-              expect_snowplow_event(
-                category: 'Members::CreateService',
-                action: 'create_member',
-                label: '_invite_source_',
-                property: 'existing_user',
-                user: maintainer
-              )
-            end
-          end
         end
       end
 
@@ -410,48 +380,28 @@ RSpec.describe API::Members do
       end
 
       context 'with areas_of_focus considerations', :snowplow do
-        context 'when there is 1 user to add' do
-          let(:user_id) { stranger.id }
+        let(:user_id) { stranger.id }
 
-          context 'when areas_of_focus is present in params' do
-            it 'tracks the areas_of_focus' do
-              post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
-                   params: { user_id: user_id, access_level: Member::DEVELOPER, areas_of_focus: 'Other' }
-
-              expect_snowplow_event(
-                category: 'Members::CreateService',
-                action: 'area_of_focus',
-                label: 'Other',
-                property: source.members.last.id.to_s
-              )
-            end
-          end
-
-          context 'when areas_of_focus is not present in params' do
-            it 'does not track the areas_of_focus' do
-              post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
-                   params: { user_id: user_id, access_level: Member::DEVELOPER }
+        context 'when areas_of_focus is present in params' do
+          it 'tracks the areas_of_focus' do
+            post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                 params: { user_id: user_id, access_level: Member::DEVELOPER, areas_of_focus: 'Other' }
 
-              expect_no_snowplow_event(category: 'Members::CreateService', action: 'area_of_focus')
-            end
+            expect_snowplow_event(
+              category: 'Members::CreateService',
+              action: 'area_of_focus',
+              label: 'Other',
+              property: source.members.last.id.to_s
+            )
           end
         end
 
-        context 'when there are multiple users to add' do
-          let(:user_id) { [developer.id, stranger.id].join(',') }
+        context 'when areas_of_focus is not present in params' do
+          it 'does not track the areas_of_focus' do
+            post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                 params: { user_id: user_id, access_level: Member::DEVELOPER }
 
-          context 'when areas_of_focus is present in params' do
-            it 'tracks the areas_of_focus' do
-              post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
-                   params: { user_id: user_id, access_level: Member::DEVELOPER, areas_of_focus: 'Other' }
-
-              expect_snowplow_event(
-                category: 'Members::CreateService',
-                action: 'area_of_focus',
-                label: 'Other',
-                property: source.members.last.id.to_s
-              )
-            end
+            expect_no_snowplow_event(category: 'Members::CreateService', action: 'area_of_focus')
           end
         end
       end