Merge branch 'move-member-creator-to-own-class' into 'master'

Move member creation process out of model [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!62043

app/models/group.rb

@@ -296,7 +296,7 @@ class Group < Namespace
   end
 
   def add_users(users, access_level, current_user: nil, expires_at: nil)
-    GroupMember.add_users(
+    Members::Groups::CreatorService.add_users( # rubocop:todo CodeReuse/ServiceClass
       self,
       users,
       access_level,
@@ -306,14 +306,13 @@ class Group < Namespace
   end
 
   def add_user(user, access_level, current_user: nil, expires_at: nil, ldap: false)
-    GroupMember.add_user(
-      self,
-      user,
-      access_level,
-      current_user: current_user,
-      expires_at: expires_at,
-      ldap: ldap
-    )
+    Members::Groups::CreatorService.new(self, # rubocop:todo CodeReuse/ServiceClass
+                                        user,
+                                        access_level,
+                                        current_user: current_user,
+                                        expires_at: expires_at,
+                                        ldap: ldap)
+                                   .execute
   end
 
   def add_guest(user, current_user = nil)

app/models/member.rb

@@ -232,140 +232,9 @@ class Member < ApplicationRecord
       find_by(invite_token: invite_token)
     end
 
-    def add_user(source, user, access_level, existing_members: nil, current_user: nil, expires_at: nil, ldap: false)
-      # rubocop: disable CodeReuse/ServiceClass
-      # `user` can be either a User object, User ID or an email to be invited
-      member = retrieve_member(source, user, existing_members)
-      access_level = retrieve_access_level(access_level)
-
-      return member unless can_update_member?(current_user, member)
-
-      set_member_attributes(
-        member,
-        access_level,
-        current_user: current_user,
-        expires_at: expires_at,
-        ldap: ldap
-      )
-
-      if member.request?
-        ::Members::ApproveAccessRequestService.new(
-          current_user,
-          access_level: access_level
-        ).execute(
-          member,
-          skip_authorization: ldap,
-          skip_log_audit_event: ldap
-        )
-      else
-        member.save
-      end
-
-      member
-      # rubocop: enable CodeReuse/ServiceClass
-    end
-
-    # Populates the attributes of a member.
-    #
-    # This logic resides in a separate method so that EE can extend this logic,
-    # without having to patch the `add_user` method directly.
-    def set_member_attributes(member, access_level, current_user: nil, expires_at: nil, ldap: false)
-      member.attributes = {
-        created_by: member.created_by || current_user,
-        access_level: access_level,
-        expires_at: expires_at
-      }
-    end
-
-    def add_users(source, users, access_level, current_user: nil, expires_at: nil)
-      return [] unless users.present?
-
-      emails, users, existing_members = parse_users_list(source, users)
-
-      self.transaction do
-        (emails + users).map! do |user|
-          add_user(
-            source,
-            user,
-            access_level,
-            existing_members: existing_members,
-            current_user: current_user,
-            expires_at: expires_at
-          )
-        end
-      end
-    end
-
-    def access_levels
-      Gitlab::Access.sym_options
-    end
-
     def valid_email?(email)
       Devise.email_regexp.match?(email)
     end
-
-    private
-
-    def parse_users_list(source, list)
-      emails = []
-      user_ids = []
-      users = []
-      existing_members = {}
-
-      list.each do |item|
-        case item
-        when User
-          users << item
-        when Integer
-          user_ids << item
-        when /\A\d+\Z/
-          user_ids << item.to_i
-        when Devise.email_regexp
-          emails << item
-        end
-      end
-
-      if user_ids.present?
-        users.concat(User.where(id: user_ids))
-        # the below will automatically discard invalid user_ids
-        existing_members = source.members_and_requesters.where(user_id: user_ids).index_by(&:user_id)
-      end
-
-      [emails, users, existing_members]
-    end
-
-    # This method is used to find users that have been entered into the "Add members" field.
-    # These can be the User objects directly, their IDs, their emails, or new emails to be invited.
-    def retrieve_user(user)
-      return user if user.is_a?(User)
-
-      return User.find_by(id: user) if user.is_a?(Integer)
-
-      User.find_by_any_email(user) || user
-    end
-
-    def retrieve_member(source, user, existing_members)
-      user = retrieve_user(user)
-
-      if user.is_a?(User)
-        if existing_members
-          existing_members[user.id] || source.members.build(user_id: user.id)
-        else
-          source.members_and_requesters.find_or_initialize_by(user_id: user.id)
-        end
-      else
-        source.members.build(invite_email: user)
-      end
-    end
-
-    def retrieve_access_level(access_level)
-      access_levels.fetch(access_level) { access_level.to_i }
-    end
-
-    def can_update_member?(current_user, member)
-      # There is no current user for bulk actions, in which case anything is allowed
-      !current_user || current_user.can?(:"update_#{member.type.underscore}", member)
-    end
   end
 
   def real_source_type

app/models/members/group_member.rb

@@ -32,10 +32,6 @@ class GroupMember < Member
     Gitlab::Access.options_with_owner
   end
 
-  def self.access_levels
-    Gitlab::Access.sym_options_with_owner
-  end
-
   def self.pluck_user_ids
     pluck(:user_id)
   end

app/models/members/project_member.rb

@@ -48,7 +48,7 @@ class ProjectMember < Member
         project_ids.each do |project_id|
           project = Project.find(project_id)
 
-          add_users(
+          Members::Projects::CreatorService.add_users( # rubocop:todo CodeReuse/ServiceClass
             project,
             users,
             access_level,
@@ -80,12 +80,6 @@ class ProjectMember < Member
     def access_level_roles
       Gitlab::Access.options
     end
-
-    private
-
-    def can_update_member?(current_user, member)
-      super || (member.owner? && member.new_record?)
-    end
   end
 
   def project

app/models/project_team.rb

@@ -42,7 +42,7 @@ class ProjectTeam
   end
 
   def add_users(users, access_level, current_user: nil, expires_at: nil)
-    ProjectMember.add_users(
+    Members::Projects::CreatorService.add_users( # rubocop:todo CodeReuse/ServiceClass
       project,
       users,
       access_level,
@@ -52,13 +52,12 @@ class ProjectTeam
   end
 
   def add_user(user, access_level, current_user: nil, expires_at: nil)
-    ProjectMember.add_user(
-      project,
-      user,
-      access_level,
-      current_user: current_user,
-      expires_at: expires_at
-    )
+    Members::Projects::CreatorService.new(project, # rubocop:todo CodeReuse/ServiceClass
+                                          user,
+                                          access_level,
+                                          current_user: current_user,
+                                          expires_at: expires_at)
+                                     .execute
   end
 
   # Remove all users from project team

app/services/members/creator_service.rb

+# frozen_string_literal: true
+
+module Members
+  # This class serves as more of an app-wide way we add/create members
+  # All roads to add members should take this path.
+  class CreatorService
+    class << self
+      def parsed_access_level(access_level)
+        access_levels.fetch(access_level) { access_level.to_i }
+      end
+
+      def access_levels
+        raise NotImplementedError
+      end
+
+      def add_users(source, users, access_level, current_user: nil, expires_at: nil)
+        return [] unless users.present?
+
+        emails, users, existing_members = parse_users_list(source, users)
+
+        Member.transaction do
+          (emails + users).map! do |user|
+            new(source,
+                user,
+                access_level,
+                existing_members: existing_members,
+                current_user: current_user,
+                expires_at: expires_at)
+              .execute
+          end
+        end
+      end
+
+      private
+
+      def parse_users_list(source, list)
+        emails = []
+        user_ids = []
+        users = []
+        existing_members = {}
+
+        list.each do |item|
+          case item
+          when User
+            users << item
+          when Integer
+            user_ids << item
+          when /\A\d+\Z/
+            user_ids << item.to_i
+          when Devise.email_regexp
+            emails << item
+          end
+        end
+
+        if user_ids.present?
+          users.concat(User.id_in(user_ids))
+          # the below will automatically discard invalid user_ids
+          existing_members = source.members_and_requesters.where(user_id: user_ids).index_by(&:user_id) # rubocop:todo CodeReuse/ActiveRecord
+        end
+
+        [emails, users, existing_members]
+      end
+    end
+
+    def initialize(source, user, access_level, **args)
+      @source = source
+      @user = user
+      @access_level = self.class.parsed_access_level(access_level)
+      @args = args
+    end
+
+    def execute
+      find_or_build_member
+      update_member
+
+      member
+    end
+
+    private
+
+    attr_reader :source, :user, :access_level, :member, :args
+
+    def update_member
+      return unless can_update_member?
+
+      member.attributes = member_attributes
+
+      if member.request?
+        approve_request
+      else
+        member.save
+      end
+    end
+
+    def can_update_member?
+      # There is no current user for bulk actions, in which case anything is allowed
+      !current_user # inheriting classes will add more logic
+    end
+
+    # Populates the attributes of a member.
+    #
+    # This logic resides in a separate method so that EE can extend this logic,
+    # without having to patch the `add_user` method directly.
+    def member_attributes
+      {
+        created_by: member.created_by || current_user,
+        access_level: access_level,
+        expires_at: args[:expires_at]
+      }
+    end
+
+    def approve_request
+      ::Members::ApproveAccessRequestService.new(current_user,
+                                                 access_level: access_level)
+                                            .execute(
+                                              member,
+                                              skip_authorization: ldap,
+                                              skip_log_audit_event: ldap
+                                            )
+    end
+
+    def current_user
+      args[:current_user]
+    end
+
+    def find_or_build_member
+      @user = parse_user_param
+
+      @member = if user.is_a?(User)
+                  find_or_initialize_member_by_user
+                else
+                  source.members.build(invite_email: user)
+                end
+    end
+
+    # This method is used to find users that have been entered into the "Add members" field.
+    # These can be the User objects directly, their IDs, their emails, or new emails to be invited.
+    def parse_user_param
+      case user
+      when User
+        user
+      when Integer
+        # might not return anything - this needs enhancement
+        User.find_by(id: user) # rubocop:todo CodeReuse/ActiveRecord
+      else
+        # must be an email or at least we'll consider it one
+        User.find_by_any_email(user) || user
+      end
+    end
+
+    def find_or_initialize_member_by_user
+      if existing_members
+        # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/334062
+        # i'm not so sure this is needed as the parse_users_list looks at members_and_requesters...
+        # so it is like we could just do a find or initialize by here and be fine
+        existing_members[user.id] || source.members.build(user_id: user.id)
+      else
+        source.members_and_requesters.find_or_initialize_by(user_id: user.id) # rubocop:todo CodeReuse/ActiveRecord
+      end
+    end
+
+    def existing_members
+      args[:existing_members]
+    end
+
+    def ldap
+      args[:ldap] || false
+    end
+  end
+end
+
+Members::CreatorService.prepend_mod_with('Members::CreatorService')

app/services/members/groups/creator_service.rb

+# frozen_string_literal: true
+
+module Members
+  module Groups
+    class CreatorService < Members::CreatorService
+      def self.access_levels
+        Gitlab::Access.sym_options_with_owner
+      end
+
+      private
+
+      def can_update_member?
+        super || current_user.can?(:update_group_member, member)
+      end
+    end
+  end
+end

app/services/members/invite_service.rb

@@ -21,7 +21,7 @@ module Members
     def validate_invites!
       super
 
-      # we need the below due to add_users hitting Member#parse_users_list and ignoring invalid emails
+      # we need the below due to add_users hitting Members::CreatorService.parse_users_list and ignoring invalid emails
       # ideally we wouldn't need this, but we can't really change the add_users method
       valid, invalid = invites.partition { |email| Member.valid_email?(email) }
       @invites = valid

app/services/members/projects/creator_service.rb

+# frozen_string_literal: true
+
+module Members
+  module Projects
+    class CreatorService < Members::CreatorService
+      def self.access_levels
+        Gitlab::Access.sym_options
+      end
+
+      private
+
+      def can_update_member?
+        super || current_user.can?(:update_project_member, member) || adding_a_new_owner?
+      end
+
+      def adding_a_new_owner?
+        # this condition is reached during testing setup a lot due to use of `.add_user`
+        member.owner? && member.new_record?
+      end
+    end
+  end
+end

ee/app/models/ee/member.rb

@@ -11,19 +11,6 @@ module EE
       end
     end
 
-    class_methods do
-      extend ::Gitlab::Utils::Override
-
-      override :set_member_attributes
-      def set_member_attributes(member, access_level, current_user: nil, expires_at: nil, ldap: false)
-        super
-
-        member.attributes = {
-          ldap: ldap
-        }
-      end
-    end
-
     override :notification_service
     def notification_service
       if ldap

ee/app/services/ee/members/creator_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Members
+    module CreatorService
+      extend ::Gitlab::Utils::Override
+
+      private
+
+      override :member_attributes
+      def member_attributes
+        super.merge(ldap: ldap)
+      end
+    end
+  end
+end

ee/spec/support/shared_examples/models/member_shared_examples.rb

@@ -14,13 +14,13 @@ RSpec.shared_examples 'member validations' do
         end
 
         it 'allows adding the group member' do
-          member = described_class.add_user(entity, user, Member::DEVELOPER)
+          member = entity.add_user(user, Member::DEVELOPER)
 
           expect(member).to be_valid
         end
 
         it 'does not add the group member' do
-          member = described_class.add_user(entity, create(:user), Member::DEVELOPER)
+          member = entity.add_user(create(:user), Member::DEVELOPER)
 
           expect(member).not_to be_valid
           expect(member.errors.messages[:user]).to eq(['is not linked to a SAML account'])
@@ -34,7 +34,7 @@ RSpec.shared_examples 'member validations' do
           end
 
           it 'does not allow adding a group member with SSO enforced on subgroup' do
-            member = described_class.add_user(entity, create(:user), ProjectMember::DEVELOPER)
+            member = entity.add_user(create(:user), ProjectMember::DEVELOPER)
 
             expect(member).not_to be_valid
             expect(member.errors.messages[:user]).to eq(['is not linked to a SAML account'])
@@ -44,7 +44,7 @@ RSpec.shared_examples 'member validations' do
 
       context 'enforced SSO disabled' do
         it 'allows adding the group member' do
-          member = described_class.add_user(entity, user, Member::DEVELOPER)
+          member = entity.add_user(user, Member::DEVELOPER)
 
           expect(member).to be_valid
         end

spec/models/members/group_member_spec.rb

@@ -47,27 +47,6 @@ RSpec.describe GroupMember do
     end
   end
 
-  describe '.access_levels' do
-    it 'returns Gitlab::Access.options_with_owner' do
-      expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner)
-    end
-  end
-
-  describe '.add_users' do
-    it 'adds the given users to the given group' do
-      group = create(:group)
-      users = create_list(:user, 2)
-
-      described_class.add_users(
-        group,
-        [users.first.id, users.second],
-        described_class::MAINTAINER
-      )
-
-      expect(group.users).to include(users.first, users.second)
-    end
-  end
-
   it_behaves_like 'members notifications', :group
 
   describe '#namespace_id' do

spec/models/members/project_member_spec.rb

@@ -23,19 +23,6 @@ RSpec.describe ProjectMember do
     end
   end
 
-  describe '.add_user' do
-    it 'adds the user as a member' do
-      user = create(:user)
-      project = create(:project)
-
-      expect(project.users).not_to include(user)
-
-      described_class.add_user(project, user, :maintainer, current_user: project.owner)
-
-      expect(project.users.reload).to include(user)
-    end
-  end
-
   describe '#real_source_type' do
     subject { create(:project_member).real_source_type }
 

spec/services/members/groups/creator_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::Groups::CreatorService do
+  it_behaves_like 'member creation' do
+    let_it_be(:source, reload: true) { create(:group, :public) }
+    let_it_be(:member_type) { GroupMember }
+  end
+
+  describe '.access_levels' do
+    it 'returns Gitlab::Access.options_with_owner' do
+      expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner)
+    end
+  end
+end

spec/services/members/projects/creator_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::Projects::CreatorService do
+  it_behaves_like 'member creation' do
+    let_it_be(:source, reload: true) { create(:project, :public) }
+    let_it_be(:member_type) { ProjectMember }
+  end
+
+  describe '.access_levels' do
+    it 'returns Gitlab::Access.sym_options' do
+      expect(described_class.access_levels).to eq(Gitlab::Access.sym_options)
+    end
+  end
+end

spec/support/shared_examples/models/member_shared_examples.rb

@@ -75,3 +75,259 @@ RSpec.shared_examples '#valid_level_roles' do |entity_name|
     expect(presenter.valid_level_roles).to eq(expected_roles)
   end
 end
+
+RSpec.shared_examples_for "member creation" do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:admin) { create(:admin) }
+
+  describe '#execute' do
+    it 'returns a Member object', :aggregate_failures do
+      member = described_class.new(source, user, :maintainer).execute
+
+      expect(member).to be_a member_type
+      expect(member).to be_persisted
+    end
+
+    context 'when admin mode is enabled', :enable_admin_mode do
+      it 'sets members.created_by to the given admin current_user' do
+        member = described_class.new(source, user, :maintainer, current_user: admin).execute
+
+        expect(member.created_by).to eq(admin)
+      end
+    end
+
+    context 'when admin mode is disabled' do
+      it 'rejects setting members.created_by to the given admin current_user' do
+        member = described_class.new(source, user, :maintainer, current_user: admin).execute
+
+        expect(member.created_by).to be_nil
+      end
+    end
+
+    it 'sets members.expires_at to the given expires_at' do
+      member = described_class.new(source, user, :maintainer, expires_at: Date.new(2016, 9, 22)).execute
+
+      expect(member.expires_at).to eq(Date.new(2016, 9, 22))
+    end
+
+    described_class.access_levels.each do |sym_key, int_access_level|
+      it "accepts the :#{sym_key} symbol as access level", :aggregate_failures do
+        expect(source.users).not_to include(user)
+
+        member = described_class.new(source, user.id, sym_key).execute
+
+        expect(member.access_level).to eq(int_access_level)
+        expect(source.users.reload).to include(user)
+      end
+
+      it "accepts the #{int_access_level} integer as access level", :aggregate_failures do
+        expect(source.users).not_to include(user)
+
+        member = described_class.new(source, user.id, int_access_level).execute
+
+        expect(member.access_level).to eq(int_access_level)
+        expect(source.users.reload).to include(user)
+      end
+    end
+
+    context 'with no current_user' do
+      context 'when called with a known user id' do
+        it 'adds the user as a member' do
+          expect(source.users).not_to include(user)
+
+          described_class.new(source, user.id, :maintainer).execute
+
+          expect(source.users.reload).to include(user)
+        end
+      end
+
+      context 'when called with an unknown user id' do
+        it 'adds the user as a member' do
+          expect(source.users).not_to include(user)
+
+          described_class.new(source, non_existing_record_id, :maintainer).execute
+
+          expect(source.users.reload).not_to include(user)
+        end
+      end
+
+      context 'when called with a user object' do
+        it 'adds the user as a member' do
+          expect(source.users).not_to include(user)
+
+          described_class.new(source, user, :maintainer).execute
+
+          expect(source.users.reload).to include(user)
+        end
+      end
+
+      context 'when called with a requester user object' do
+        before do
+          source.request_access(user)
+        end
+
+        it 'adds the requester as a member', :aggregate_failures do
+          expect(source.users).not_to include(user)
+          expect(source.requesters.exists?(user_id: user)).to be_truthy
+
+          expect do
+            described_class.new(source, user, :maintainer).execute
+          end.to raise_error(Gitlab::Access::AccessDeniedError)
+
+          expect(source.users.reload).not_to include(user)
+          expect(source.requesters.reload.exists?(user_id: user)).to be_truthy
+        end
+      end
+
+      context 'when called with a known user email' do
+        it 'adds the user as a member' do
+          expect(source.users).not_to include(user)
+
+          described_class.new(source, user.email, :maintainer).execute
+
+          expect(source.users.reload).to include(user)
+        end
+      end
+
+      context 'when called with an unknown user email' do
+        it 'creates an invited member' do
+          expect(source.users).not_to include(user)
+
+          described_class.new(source, 'user@example.com', :maintainer).execute
+
+          expect(source.members.invite.pluck(:invite_email)).to include('user@example.com')
+        end
+      end
+
+      context 'when called with an unknown user email starting with a number' do
+        it 'creates an invited member', :aggregate_failures do
+          email_starting_with_number = "#{user.id}_email@example.com"
+
+          described_class.new(source, email_starting_with_number, :maintainer).execute
+
+          expect(source.members.invite.pluck(:invite_email)).to include(email_starting_with_number)
+          expect(source.users.reload).not_to include(user)
+        end
+      end
+    end
+
+    context 'when current_user can update member', :enable_admin_mode do
+      it 'creates the member' do
+        expect(source.users).not_to include(user)
+
+        described_class.new(source, user, :maintainer, current_user: admin).execute
+
+        expect(source.users.reload).to include(user)
+      end
+
+      context 'when called with a requester user object' do
+        before do
+          source.request_access(user)
+        end
+
+        it 'adds the requester as a member', :aggregate_failures do
+          expect(source.users).not_to include(user)
+          expect(source.requesters.exists?(user_id: user)).to be_truthy
+
+          described_class.new(source, user, :maintainer, current_user: admin).execute
+
+          expect(source.users.reload).to include(user)
+          expect(source.requesters.reload.exists?(user_id: user)).to be_falsy
+        end
+      end
+    end
+
+    context 'when current_user cannot update member' do
+      it 'does not create the member', :aggregate_failures do
+        expect(source.users).not_to include(user)
+
+        member = described_class.new(source, user, :maintainer, current_user: user).execute
+
+        expect(source.users.reload).not_to include(user)
+        expect(member).not_to be_persisted
+      end
+
+      context 'when called with a requester user object' do
+        before do
+          source.request_access(user)
+        end
+
+        it 'does not destroy the requester', :aggregate_failures do
+          expect(source.users).not_to include(user)
+          expect(source.requesters.exists?(user_id: user)).to be_truthy
+
+          described_class.new(source, user, :maintainer, current_user: user).execute
+
+          expect(source.users.reload).not_to include(user)
+          expect(source.requesters.exists?(user_id: user)).to be_truthy
+        end
+      end
+    end
+
+    context 'when member already exists' do
+      before do
+        source.add_user(user, :developer)
+      end
+
+      context 'with no current_user' do
+        it 'updates the member' do
+          expect(source.users).to include(user)
+
+          described_class.new(source, user, :maintainer).execute
+
+          expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::MAINTAINER)
+        end
+      end
+
+      context 'when current_user can update member', :enable_admin_mode do
+        it 'updates the member' do
+          expect(source.users).to include(user)
+
+          described_class.new(source, user, :maintainer, current_user: admin).execute
+
+          expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::MAINTAINER)
+        end
+      end
+
+      context 'when current_user cannot update member' do
+        it 'does not update the member' do
+          expect(source.users).to include(user)
+
+          described_class.new(source, user, :maintainer, current_user: user).execute
+
+          expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::DEVELOPER)
+        end
+      end
+    end
+  end
+
+  describe '.add_users' do
+    let_it_be(:user1) { create(:user) }
+    let_it_be(:user2) { create(:user) }
+
+    it 'returns a Member objects' do
+      members = described_class.add_users(source, [user1, user2], :maintainer)
+
+      expect(members).to be_a Array
+      expect(members.size).to eq(2)
+      expect(members.first).to be_a member_type
+      expect(members.first).to be_persisted
+    end
+
+    it 'returns an empty array' do
+      members = described_class.add_users(source, [], :maintainer)
+
+      expect(members).to be_a Array
+      expect(members).to be_empty
+    end
+
+    it 'supports different formats' do
+      list = ['joe@local.test', admin, user1.id, user2.id.to_s]
+
+      members = described_class.add_users(source, list, :maintainer)
+
+      expect(members.size).to eq(4)
+      expect(members.first).to be_invite
+    end
+  end
+end