Improve Member services

Signed-off-by: Rémy Coutable <remy@rymai.me>

7 files changed, 60 insertions, 85 deletions

diff --git a/app/services/members/approve_access_request_service.rb b/app/services/members/approve_access_request_service.rb
index 19431ac76dc..6be08b590bc 100644
--- a/app/services/members/approve_access_request_service.rb
+++ b/app/services/members/approve_access_request_service.rb
@@ -1,25 +1,20 @@
 module Members
   class ApproveAccessRequestService < Members::BaseService
-    # opts - A hash of options
-    #   :ldap - The call is from a LDAP sync: current_user can be nil in that case
-    def execute(access_requester, opts = {})
-      raise Gitlab::Access::AccessDeniedError unless can_update_access_requester?(access_requester, opts[:ldap])
+    def execute(access_requester, skip_authorization: false, skip_log_audit_event: false)
+      raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_update_access_requester?(access_requester)
 
       access_requester.access_level = params[:access_level] if params[:access_level]
       access_requester.accept_request
 
-      after_execute(member: access_requester, **opts)
+      after_execute(member: access_requester, skip_log_audit_event: skip_log_audit_event)
 
       access_requester
     end
 
     private
 
-    def can_update_access_requester?(access_requester, ldap)
-      access_requester && (
-        ldap ||
-        can?(current_user, update_member_permission(access_requester), access_requester)
-      )
+    def can_update_access_requester?(access_requester)
+      can?(current_user, update_member_permission(access_requester), access_requester)
     end
   end
 end
diff --git a/app/services/members/authorized_destroy_service.rb b/app/services/members/authorized_destroy_service.rb
deleted file mode 100644
index 793a951d63b..00000000000
--- a/app/services/members/authorized_destroy_service.rb
+++ /dev/null
@@ -1,59 +0,0 @@
-module Members
-  class AuthorizedDestroyService < BaseService
-    def initialize(current_user = nil)
-      @current_user = current_user
-    end
-
-    def execute(member)
-      return false if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
-
-      Member.transaction do
-        unassign_issues_and_merge_requests(member) unless member.invite?
-        member.notification_setting&.destroy
-
-        member.destroy
-      end
-
-      if member.request? && member.user != current_user
-        notification_service.decline_access_request(member)
-      end
-
-      member
-    end
-
-    private
-
-    def unassign_issues_and_merge_requests(member)
-      if member.is_a?(GroupMember)
-        issues = Issue.unscoped.select(1)
-                 .joins(:project)
-                 .where('issues.id = issue_assignees.issue_id AND projects.namespace_id = ?', member.source_id)
-
-        # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
-        IssueAssignee.unscoped
-          .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
-          .delete_all
-
-        MergeRequestsFinder.new(current_user, group_id: member.source_id, assignee_id: member.user_id)
-          .execute
-          .update_all(assignee_id: nil)
-      else
-        project = member.source
-
-        # SELECT 1 FROM issues WHERE issues.id = issue_assignees.issue_id AND issues.project_id = X
-        issues = Issue.unscoped.select(1)
-                 .where('issues.id = issue_assignees.issue_id')
-                 .where(project_id: project.id)
-
-        # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
-        IssueAssignee.unscoped
-          .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
-          .delete_all
-
-        project.merge_requests.opened.assigned_to(member.user).update_all(assignee_id: nil)
-      end
-
-      member.user.invalidate_cache_counts
-    end
-  end
-end
diff --git a/app/services/members/base_service.rb b/app/services/members/base_service.rb
index 5c52468aaca..74556fb20cf 100644
--- a/app/services/members/base_service.rb
+++ b/app/services/members/base_service.rb
@@ -1,17 +1,13 @@
 module Members
   class BaseService < ::BaseService
-    attr_accessor :source
-
-    # source - The source object that respond to `#members` (e.g. project or group)
     # current_user - The user that performs the action
     # params - A hash of parameters
-    def initialize(source, current_user, params = {})
-      @source = source
+    def initialize(current_user = nil, params = {})
       @current_user = current_user
       @params = params
     end
 
-    def after_execute(**args)
+    def after_execute(args)
       # overriden in EE::Members modules
     end
 
diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb
index 61802ba09ce..bc6a9405aac 100644
--- a/app/services/members/create_service.rb
+++ b/app/services/members/create_service.rb
@@ -2,7 +2,7 @@ module Members
   class CreateService < Members::BaseService
     DEFAULT_LIMIT = 100
 
-    def execute
+    def execute(source)
       return error('No users specified.') if params[:user_ids].blank?
 
       user_ids = params[:user_ids].split(',').uniq
@@ -17,7 +17,7 @@ module Members
         current_user: current_user
       )
 
-      members.compact.each { |member| after_execute(member: member) }
+      members.each { |member| after_execute(member: member) }
 
       success
     end
diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb
index 477ab127b2d..b141bfd5fbc 100644
--- a/app/services/members/destroy_service.rb
+++ b/app/services/members/destroy_service.rb
@@ -1,9 +1,20 @@
 module Members
   class DestroyService < Members::BaseService
-    def execute(member)
-      raise Gitlab::Access::AccessDeniedError unless can_destroy_member?(member)
+    def execute(member, skip_authorization: false)
+      raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_destroy_member?(member)
 
-      AuthorizedDestroyService.new(current_user).execute(member)
+      return member if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
+
+      Member.transaction do
+        unassign_issues_and_merge_requests(member) unless member.invite?
+        member.notification_setting&.destroy
+
+        member.destroy
+      end
+
+      if member.request? && member.user != current_user
+        notification_service.decline_access_request(member)
+      end
 
       after_execute(member: member)
 
@@ -13,7 +24,7 @@ module Members
     private
 
     def can_destroy_member?(member)
-      member && can?(current_user, destroy_member_permission(member), member)
+      can?(current_user, destroy_member_permission(member), member)
     end
 
     def destroy_member_permission(member)
@@ -26,5 +37,38 @@ module Members
         raise "Unknown member type: #{member}!"
       end
     end
+
+    def unassign_issues_and_merge_requests(member)
+      if member.is_a?(GroupMember)
+        issues = Issue.unscoped.select(1)
+                 .joins(:project)
+                 .where('issues.id = issue_assignees.issue_id AND projects.namespace_id = ?', member.source_id)
+
+        # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
+        IssueAssignee.unscoped
+          .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
+          .delete_all
+
+        MergeRequestsFinder.new(current_user, group_id: member.source_id, assignee_id: member.user_id)
+          .execute
+          .update_all(assignee_id: nil)
+      else
+        project = member.source
+
+        # SELECT 1 FROM issues WHERE issues.id = issue_assignees.issue_id AND issues.project_id = X
+        issues = Issue.unscoped.select(1)
+                 .where('issues.id = issue_assignees.issue_id')
+                 .where(project_id: project.id)
+
+        # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
+        IssueAssignee.unscoped
+          .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
+          .delete_all
+
+        project.merge_requests.opened.assigned_to(member.user).update_all(assignee_id: nil)
+      end
+
+      member.user.invalidate_cache_counts
+    end
   end
 end
diff --git a/app/services/members/request_access_service.rb b/app/services/members/request_access_service.rb
index e30722efc13..24293b30005 100644
--- a/app/services/members/request_access_service.rb
+++ b/app/services/members/request_access_service.rb
@@ -1,6 +1,6 @@
 module Members
   class RequestAccessService < Members::BaseService
-    def execute
+    def execute(source)
       raise Gitlab::Access::AccessDeniedError unless can_request_access?(source)
 
       source.members.create(
@@ -12,7 +12,7 @@ module Members
     private
 
     def can_request_access?(source)
-      source && can?(current_user, :request_access, source)
+      can?(current_user, :request_access, source)
     end
   end
 end
diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb
index ebea9f96069..48b3d59f7bd 100644
--- a/app/services/members/update_service.rb
+++ b/app/services/members/update_service.rb
@@ -2,8 +2,7 @@ module Members
   class UpdateService < Members::BaseService
     # returns the updated member
     def execute(member, permission: :update)
-      permission_target = permission == :override ? source : member
-      raise Gitlab::Access::AccessDeniedError unless can?(current_user, action_member_permission(permission, member), permission_target)
+      raise Gitlab::Access::AccessDeniedError unless can?(current_user, action_member_permission(permission, member), member)
 
       old_access_level = member.human_access