Remove explicit audit event log in MembershipActions

Move it to Members::ApproveAccessRequestService.

Also, note that there was a double audit event log for access request
destruction.

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

7 files changed, 96 insertions, 87 deletions

diff --git a/app/services/members/approve_access_request_service.rb b/app/services/members/approve_access_request_service.rb
index 2a2bb0cae5b..19431ac76dc 100644
--- a/app/services/members/approve_access_request_service.rb
+++ b/app/services/members/approve_access_request_service.rb
@@ -1,51 +1,25 @@
 module Members
-  class ApproveAccessRequestService < BaseService
-    include MembersHelper
-
-    attr_accessor :source
-
-    # source - The source object that respond to `#requesters` (i.g. project or group)
-    # current_user - The user that performs the access request approval
-    # params - A hash of parameters
-    #   :user_id - User ID used to retrieve the access requester
-    #   :id - Member ID used to retrieve the access requester
-    #   :access_level - Optional access level set when the request is accepted
-    def initialize(source, current_user, params = {})
-      @source = source
-      @current_user = current_user
-      @params = params.slice(:user_id, :id, :access_level)
-    end
-
+  class ApproveAccessRequestService < Members::BaseService
     # opts - A hash of options
-    #   :force - Bypass permission check: current_user can be nil in that case
-    def execute(opts = {})
-      condition = params[:user_id] ? { user_id: params[:user_id] } : { id: params[:id] }
-      access_requester = source.requesters.find_by!(condition)
-
-      raise Gitlab::Access::AccessDeniedError unless can_update_access_requester?(access_requester, opts)
+    #   :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])
 
       access_requester.access_level = params[:access_level] if params[:access_level]
       access_requester.accept_request
 
+      after_execute(member: access_requester, **opts)
+
       access_requester
     end
 
     private
 
-    def can_update_access_requester?(access_requester, opts = {})
+    def can_update_access_requester?(access_requester, ldap)
       access_requester && (
-        opts[:force] ||
+        ldap ||
         can?(current_user, update_member_permission(access_requester), access_requester)
       )
     end
-
-    def update_member_permission(member)
-      case member
-      when GroupMember
-        :update_group_member
-      when ProjectMember
-        :update_project_member
-      end
-    end
   end
 end
diff --git a/app/services/members/authorized_destroy_service.rb b/app/services/members/authorized_destroy_service.rb
index 2e89f00dad8..793a951d63b 100644
--- a/app/services/members/authorized_destroy_service.rb
+++ b/app/services/members/authorized_destroy_service.rb
@@ -1,12 +1,10 @@
 module Members
   class AuthorizedDestroyService < BaseService
-    attr_accessor :member, :user
-
-    def initialize(member, user = nil)
-      @member, @user = member, user
+    def initialize(current_user = nil)
+      @current_user = current_user
     end
 
-    def execute
+    def execute(member)
       return false if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
 
       Member.transaction do
@@ -16,7 +14,7 @@ module Members
         member.destroy
       end
 
-      if member.request? && member.user != user
+      if member.request? && member.user != current_user
         notification_service.decline_access_request(member)
       end
 
@@ -36,7 +34,7 @@ module Members
           .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
           .delete_all
 
-        MergeRequestsFinder.new(user, group_id: member.source_id, assignee_id: member.user_id)
+        MergeRequestsFinder.new(current_user, group_id: member.source_id, assignee_id: member.user_id)
           .execute
           .update_all(assignee_id: nil)
       else
diff --git a/app/services/members/base_service.rb b/app/services/members/base_service.rb
new file mode 100644
index 00000000000..5c52468aaca
--- /dev/null
+++ b/app/services/members/base_service.rb
@@ -0,0 +1,53 @@
+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
+      @current_user = current_user
+      @params = params
+    end
+
+    def after_execute(**args)
+      # overriden in EE::Members modules
+    end
+
+    private
+
+    def update_member_permission(member)
+      case member
+      when GroupMember
+        :update_group_member
+      when ProjectMember
+        :update_project_member
+      else
+        raise "Unknown member type: #{member}!"
+      end
+    end
+
+    def override_member_permission(member)
+      case member
+      when GroupMember
+        :override_group_member
+      when ProjectMember
+        :override_project_member
+      else
+        raise "Unknown member type: #{member}!"
+      end
+    end
+
+    def action_member_permission(action, member)
+      case action
+      when :update
+        update_member_permission(member)
+      when :override
+        override_member_permission(member)
+      else
+        raise "Unknown action '#{action}' on #{member}!"
+      end
+    end
+  end
+end
diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb
index 26906ae7167..61802ba09ce 100644
--- a/app/services/members/create_service.rb
+++ b/app/services/members/create_service.rb
@@ -1,14 +1,7 @@
 module Members
-  class CreateService < BaseService
+  class CreateService < Members::BaseService
     DEFAULT_LIMIT = 100
 
-    def initialize(source, current_user, params = {})
-      @source = source
-      @current_user = current_user
-      @params = params
-      @error = nil
-    end
-
     def execute
       return error('No users specified.') if params[:user_ids].blank?
 
@@ -17,13 +10,15 @@ module Members
       return error("Too many users specified (limit is #{user_limit})") if
         user_limit && user_ids.size > user_limit
 
-      @source.add_users(
+      members = source.add_users(
         user_ids,
         params[:access_level],
         expires_at: params[:expires_at],
         current_user: current_user
       )
 
+      members.compact.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 05b93ac8fdb..477ab127b2d 100644
--- a/app/services/members/destroy_service.rb
+++ b/app/services/members/destroy_service.rb
@@ -1,40 +1,17 @@
 module Members
-  class DestroyService < BaseService
-    include MembersHelper
-
-    attr_accessor :source
-
-    ALLOWED_SCOPES = %i[members requesters all].freeze
-
-    def initialize(source, current_user, params = {})
-      @source = source
-      @current_user = current_user
-      @params = params
-    end
-
-    def execute(scope = :members)
-      raise "scope :#{scope} is not allowed!" unless ALLOWED_SCOPES.include?(scope)
+  class DestroyService < Members::BaseService
+    def execute(member)
+      raise Gitlab::Access::AccessDeniedError unless can_destroy_member?(member)
 
-      member = find_member!(scope)
+      AuthorizedDestroyService.new(current_user).execute(member)
 
-      raise Gitlab::Access::AccessDeniedError unless can_destroy_member?(member)
+      after_execute(member: member)
 
-      AuthorizedDestroyService.new(member, current_user).execute
+      member
     end
 
     private
 
-    def find_member!(scope)
-      condition = params[:user_id] ? { user_id: params[:user_id] } : { id: params[:id] }
-      case scope
-      when :all
-        source.members.find_by(condition) ||
-          source.requesters.find_by!(condition)
-      else
-        source.public_send(scope).find_by!(condition) # rubocop:disable GitlabSecurity/PublicSend
-      end
-    end
-
     def can_destroy_member?(member)
       member && can?(current_user, destroy_member_permission(member), member)
     end
@@ -45,6 +22,8 @@ module Members
         :destroy_group_member
       when ProjectMember
         :destroy_project_member
+      else
+        raise "Unknown member type: #{member}!"
       end
     end
   end
diff --git a/app/services/members/request_access_service.rb b/app/services/members/request_access_service.rb
index 2614153d900..e30722efc13 100644
--- a/app/services/members/request_access_service.rb
+++ b/app/services/members/request_access_service.rb
@@ -1,12 +1,5 @@
 module Members
-  class RequestAccessService < BaseService
-    attr_accessor :source
-
-    def initialize(source, current_user)
-      @source = source
-      @current_user = current_user
-    end
-
+  class RequestAccessService < Members::BaseService
     def execute
       raise Gitlab::Access::AccessDeniedError unless can_request_access?(source)
 
diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb
new file mode 100644
index 00000000000..ebea9f96069
--- /dev/null
+++ b/app/services/members/update_service.rb
@@ -0,0 +1,17 @@
+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)
+
+      old_access_level = member.human_access
+
+      if member.update_attributes(params)
+        after_execute(action: permission, old_access_level: old_access_level, member: member)
+      end
+
+      member
+    end
+  end
+end