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>

app/controllers/concerns/membership_actions.rb

@@ -15,8 +15,9 @@ module MembershipActions
   end
 
   def destroy
+    member = membershipable.members_and_requesters.find(params[:id])
     Members::DestroyService.new(membershipable, current_user, params)
-      .execute(:all)
+      .execute(member)
 
     respond_to do |format|
       format.html do
@@ -36,16 +37,18 @@ module MembershipActions
   end
 
   def approve_access_request
-    member = Members::ApproveAccessRequestService.new(membershipable, current_user, params).execute
-
-    log_audit_event(member, action: :create)
+    access_requester = membershipable.requesters.find(params[:id])
+    Members::ApproveAccessRequestService
+      .new(membershipable, current_user, params)
+      .execute(access_requester)
 
     redirect_to members_page_url
   end
 
   def leave
-    member = Members::DestroyService.new(membershipable, current_user, user_id: current_user.id)
-      .execute(:all)
+    member = membershipable.members_and_requesters.find_by!(user_id: current_user.id)
+    Members::DestroyService.new(membershipable, current_user)
+      .execute(member)
 
     notice =
       if member.request?
@@ -70,11 +73,6 @@ module MembershipActions
     raise NotImplementedError
   end
 
-  def log_audit_event(member, options = {})
-    AuditEventService.new(current_user, membershipable, options)
-      .for_member(member).security_event
-  end
-
   def members_page_url
     if membershipable.is_a?(Project)
       project_project_members_path(membershipable)

app/controllers/groups/group_members_controller.rb

@@ -35,16 +35,11 @@ class Groups::GroupMembersController < Groups::ApplicationController
   end
 
   def update
-    @group_member = @group.members_and_requesters.find(params[:id])
+    member = @group.members_and_requesters.find(params[:id])
+    @group_member = Members::UpdateService
+      .new(@group, current_user, member_params)
+      .execute(member)
       .present(current_user: current_user)
-
-    return render_403 unless can?(current_user, :update_group_member, @group_member)
-
-    old_access_level = @group_member.human_access
-
-    if @group_member.update_attributes(member_params)
-      log_audit_event(@group_member, action: :update, old_access_level: old_access_level)
-    end
   end
 
   def resend_invite

app/controllers/projects/project_members_controller.rb

@@ -27,16 +27,11 @@ class Projects::ProjectMembersController < Projects::ApplicationController
   end
 
   def update
-    @project_member = @project.members_and_requesters.find(params[:id])
+    member = @project.members_and_requesters.find(params[:id])
+    @project_member = Members::UpdateService
+      .new(@project, current_user, member_params)
+      .execute(member)
       .present(current_user: current_user)
-
-    return render_403 unless can?(current_user, :update_project_member, @project_member)
-
-    old_access_level = @project_member.human_access
-
-    if @project_member.update_attributes(member_params)
-      log_audit_event(@project_member, action: :update, old_access_level: old_access_level)
-    end
   end
 
   def resend_invite

app/models/member.rb

@@ -148,9 +148,8 @@ class Member < ActiveRecord::Base
         ::Members::ApproveAccessRequestService.new(
           source,
           current_user,
-          id: member.id,
           access_level: access_level
-        ).execute(force: ldap)
+        ).execute(member, ldap: ldap)
       else
         member.save
       end

app/services/members/approve_access_request_service.rb

 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
+    prepend EE::Members::ApproveAccessRequestService
 
     # 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

app/services/members/authorized_destroy_service.rb

 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

app/services/members/base_service.rb

+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

app/services/members/create_service.rb

 module Members
-  class CreateService < BaseService
-    DEFAULT_LIMIT = 100
+  class CreateService < Members::BaseService
+    prepend EE::Members::CreateService
 
-    def initialize(source, current_user, params = {})
-      @source = source
-      @current_user = current_user
-      @params = params
-      @error = nil
-    end
+    DEFAULT_LIMIT = 100
 
     def execute
       return error('No users specified.') if params[:user_ids].blank?
@@ -17,17 +12,14 @@ module Members
       return error("Too many users specified (limit is #{user_limit})") if
         user_limit && user_ids.size > user_limit
 
-      members = @source.add_users(
+      members = source.add_users(
         user_ids,
         params[:access_level],
         expires_at: params[:expires_at],
         current_user: current_user
       )
 
-      members.compact.each do |member|
-        AuditEventService.new(@current_user, @source, action: :create)
-          .for_member(member).security_event
-      end
+      members.compact.each { |member| after_execute(member: member) }
 
       success
     end

app/services/members/destroy_service.rb

 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)
-
-      member = find_member!(scope)
+  class DestroyService < Members::BaseService
+    prepend EE::Members::DestroyService
 
+    def execute(member)
       raise Gitlab::Access::AccessDeniedError unless can_destroy_member?(member)
 
-      AuthorizedDestroyService.new(member, current_user).execute
+      AuthorizedDestroyService.new(current_user).execute(member)
 
-      AuditEventService.new(@current_user, @source, action: :destroy)
-        .for_member(member).security_event
+      after_execute(member: member)
 
       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
@@ -50,6 +24,8 @@ module Members
         :destroy_group_member
       when ProjectMember
         :destroy_project_member
+      else
+        raise "Unknown member type: #{member}!"
       end
     end
   end

app/services/members/request_access_service.rb

 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)
 

app/services/members/update_service.rb

+module Members
+  class UpdateService < Members::BaseService
+    prepend EE::Members::UpdateService
+
+    # 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

app/workers/remove_expired_members_worker.rb

@@ -5,7 +5,7 @@ class RemoveExpiredMembersWorker
   def perform
     Member.expired.find_each do |member|
       begin
-        Members::AuthorizedDestroyService.new(member).execute
+        Members::AuthorizedDestroyService.new.execute(member)
       rescue => ex
         logger.error("Expired Member ID=#{member.id} cannot be removed - #{ex}")
       end

ee/app/controllers/ee/groups/group_members_controller.rb

@@ -5,13 +5,11 @@ module EE
 
       # rubocop:disable Gitlab/ModuleWithInstanceVariables
       def override
-        @group_member = @group.group_members.find(params[:id])
-
-        return render_403 unless can?(current_user, :override_group_member, @group_member)
-
-        if @group_member.update_attributes(override_params)
-          log_audit_event(@group_member, action: :override)
+        member = @group.members.find_by!(id: params[:id])
+        updated_member = ::Members::UpdateService.new(@group, current_user, override_params)
+          .execute(member, permission: :override)
 
+        if updated_member.valid?
           respond_to do |format|
             format.js { head :ok }
           end

ee/app/services/ee/members/approve_access_request_service.rb

+module EE
+  module Members
+    module ApproveAccessRequestService
+      def after_execute(member:, **opts)
+        super
+
+        # Don't log this event in the case of a mass-approval, e.g. LDAP group-sync
+        log_audit_event(member: member) unless opts[:ldap]
+      end
+
+      private
+
+      def log_audit_event(member:)
+        ::AuditEventService.new(
+          current_user,
+          source,
+          action: :create
+        ).for_member(member).security_event
+      end
+    end
+  end
+end

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

+module EE
+  module Members
+    module CreateService
+      def after_execute(member:)
+        super
+
+        log_audit_event(member: member)
+      end
+
+      private
+
+      def log_audit_event(member:)
+        ::AuditEventService.new(
+          current_user,
+          source,
+          action: :create
+        ).for_member(member).security_event
+      end
+    end
+  end
+end

ee/app/services/ee/members/destroy_service.rb

+module EE
+  module Members
+    module DestroyService
+      def after_execute(member:)
+        super
+
+        log_audit_event(member: member)
+      end
+
+      private
+
+      def log_audit_event(member:)
+        ::AuditEventService.new(
+          current_user,
+          source,
+          action: :destroy
+        ).for_member(member).security_event
+      end
+    end
+  end
+end

ee/app/services/ee/members/update_service.rb

+module EE
+  module Members
+    module UpdateService
+      extend ActiveSupport::Concern
+
+      def after_execute(action:, old_access_level:, member:)
+        super
+
+        log_audit_event(action: action, old_access_level: old_access_level, member: member)
+      end
+
+      private
+
+      def log_audit_event(action:, old_access_level:, member:)
+        ::AuditEventService.new(
+          current_user,
+          source,
+          action: action,
+          old_access_level: old_access_level
+        ).for_member(member).security_event
+      end
+    end
+  end
+end

lib/api/access_requests.rb

@@ -53,7 +53,10 @@ module API
         put ':id/access_requests/:user_id/approve' do
           source = find_source(source_type, params[:id])
 
-          member = ::Members::ApproveAccessRequestService.new(source, current_user, declared_params).execute
+          access_requester = source.requesters.find_by!(user_id: params[:user_id])
+          member = ::Members::ApproveAccessRequestService
+            .new(source, current_user, declared_params)
+            .execute(access_requester)
 
           status :created
           present member, with: Entities::Member
@@ -70,8 +73,7 @@ module API
           member = source.requesters.find_by!(user_id: params[:user_id])
 
           destroy_conditionally!(member) do
-            ::Members::DestroyService.new(source, current_user, params)
-              .execute(:requesters)
+            ::Members::DestroyService.new(source, current_user).execute(member)
           end
         end
       end

lib/api/members.rb

@@ -81,12 +81,18 @@ module API
           source = find_source(source_type, params.delete(:id))
           authorize_admin_source!(source_type, source)
 
-          member = source.members.find_by!(user_id: params.delete(:user_id))
-
-          if member.update_attributes(declared_params(include_missing: false))
-            present member, with: Entities::Member
+          member = source.members.find_by!(user_id: params[:user_id])
+          updated_member =
+            ::Members::UpdateService.new(
+              source,
+              current_user,
+              declared_params(include_missing: false)
+            ).execute(member)
+
+          if updated_member.valid?
+            present updated_member, with: Entities::Member
           else
-            render_validation_error!(member)
+            render_validation_error!(updated_member)
           end
         end
 
@@ -99,7 +105,7 @@ module API
           member = source.members.find_by!(user_id: params[:user_id])
 
           destroy_conditionally!(member) do
-            ::Members::DestroyService.new(source, current_user, declared_params).execute
+            ::Members::DestroyService.new(source, current_user).execute(member)
           end
         end
       end

lib/api/v3/members.rb

@@ -124,7 +124,7 @@ module API
               status(200  )
               { message: "Access revoked", id: params[:user_id].to_i }
             else
-              ::Members::DestroyService.new(source, current_user, declared_params).execute
+              ::Members::DestroyService.new(source, current_user).execute(member)
 
               present member, with: ::API::Entities::Member
             end

spec/services/members/approve_access_request_service_spec.rb

 require 'spec_helper'
 
 describe Members::ApproveAccessRequestService do
-  let(:user) { create(:user) }
-  let(:access_requester) { create(:user) }
   let(:project) { create(:project, :public, :access_requestable) }
   let(:group) { create(:group, :public, :access_requestable) }
+  let(:current_user) { create(:user) }
+  let(:access_requester_user) { create(:user) }
+  let(:access_requester) { source.requesters.find_by!(user_id: access_requester_user.id) }
+  let(:params) { {} }
   let(:opts) { {} }
 
   shared_examples 'a service raising ActiveRecord::RecordNotFound' do
     it 'raises ActiveRecord::RecordNotFound' do
-      expect { described_class.new(source, user, params).execute(opts) }.to raise_error(ActiveRecord::RecordNotFound)
+      expect { described_class.new(source, current_user, params).execute(access_requester, opts) }.to raise_error(ActiveRecord::RecordNotFound)
     end
   end
 
   shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
     it 'raises Gitlab::Access::AccessDeniedError' do
-      expect { described_class.new(source, user, params).execute(opts) }.to raise_error(Gitlab::Access::AccessDeniedError)
+      expect { described_class.new(source, current_user, params).execute(access_requester, opts) }.to raise_error(Gitlab::Access::AccessDeniedError)
     end
   end
 
   shared_examples 'a service approving an access request' do
     it 'succeeds' do
-      expect { described_class.new(source, user, params).execute(opts) }.to change { source.requesters.count }.by(-1)
+      expect { described_class.new(source, current_user, params).execute(access_requester, opts) }.to change { source.requesters.count }.by(-1)
     end
 
     it 'returns a <Source>Member' do
-      member = described_class.new(source, user, params).execute(opts)
+      member = described_class.new(source, current_user, params).execute(access_requester, opts)
 
       expect(member).to be_a "#{source.class}Member".constantize
       expect(member.requested_at).to be_nil
     end
 
     context 'with a custom access level' do
-      let(:params2) { params.merge(user_id: access_requester.id, access_level: Gitlab::Access::MASTER) }
-
       it 'returns a ProjectMember with the custom access level' do
-        member = described_class.new(source, user, params2).execute(opts)
+        member = described_class.new(source, current_user, params.merge(access_level: Gitlab::Access::MASTER)).execute(access_requester, opts)
 
-        expect(member.access_level).to eq Gitlab::Access::MASTER
+        expect(member.access_level).to eq(Gitlab::Access::MASTER)
       end
     end
   end
 
-  context 'when no access requester are found' do
-    let(:params) { { user_id: 42 } }
-
-    it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
-      let(:source) { project }
-    end
-
-    it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
-      let(:source) { group }
-    end
-  end
-
   context 'when an access requester is found' do
     before do
-      project.request_access(access_requester)
-      group.request_access(access_requester)
+      project.request_access(access_requester_user)
+      group.request_access(access_requester_user)
     end
-    let(:params) { { user_id: access_requester.id } }
 
     context 'when current user is nil' do
       let(:user) { nil }
 
-      context 'and :force option is not given' do
+      context 'and :ldap option is not given' do
         it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
           let(:source) { project }
         end
@@ -74,8 +61,8 @@ describe Members::ApproveAccessRequestService do
         end
       end
 
-      context 'and :force option is false' do
-        let(:opts) { { force: false } }
+      context 'and :ldap option is false' do
+        let(:opts) { { ldap: false } }
 
         it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
           let(:source) { project }
@@ -86,8 +73,8 @@ describe Members::ApproveAccessRequestService do
         end
       end
 
-      context 'and :force option is true' do
-        let(:opts) { { force: true } }
+      context 'and :ldap option is true' do
+        let(:opts) { { ldap: true } }
 
         it_behaves_like 'a service approving an access request' do
           let(:source) { project }
@@ -98,8 +85,8 @@ describe Members::ApproveAccessRequestService do
         end
       end
 
-      context 'and :force param is true' do
-        let(:params) { { user_id: access_requester.id, force: true } }
+      context 'and :ldap param is true' do
+        let(:params) { { ldap: true } }
 
         it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
           let(:source) { project }
@@ -123,8 +110,8 @@ describe Members::ApproveAccessRequestService do
 
     context 'when current user can approve access request to the project' do
       before do
-        project.add_master(user)
-        group.add_owner(user)
+        project.add_master(current_user)
+        group.add_owner(current_user)
       end
 
       it_behaves_like 'a service approving an access request' do
@@ -134,14 +121,6 @@ describe Members::ApproveAccessRequestService do
       it_behaves_like 'a service approving an access request' do
         let(:source) { group }
       end
-
-      context 'when given a :id' do
-        let(:params) { { id: project.requesters.find_by!(user_id: access_requester.id).id } }
-
-        it_behaves_like 'a service approving an access request' do
-          let(:source) { project }
-        end
-      end
     end
   end
 end

spec/services/members/authorized_destroy_service_spec.rb

@@ -17,7 +17,7 @@ describe Members::AuthorizedDestroyService do
 
       member = create :project_member, :invited, project: project
 
-      expect { described_class.new(member, member_user).execute }
+      expect { described_class.new(member_user).execute(member) }
         .to change { Member.count }.from(3).to(2)
     end
 
@@ -26,7 +26,7 @@ describe Members::AuthorizedDestroyService do
 
       member = create :project_member, :invited, project: project
 
-      expect { described_class.new(member, member_user).execute }
+      expect { described_class.new(member_user).execute(member) }
         .not_to change { NotificationSetting.count }
     end
 
@@ -35,7 +35,7 @@ describe Members::AuthorizedDestroyService do
 
       member = create :group_member, :invited, group: group
 
-      expect { described_class.new(member, member_user).execute }
+      expect { described_class.new(member_user).execute(member) }
         .to change { Member.count }.from(2).to(1)
     end
 
@@ -44,7 +44,7 @@ describe Members::AuthorizedDestroyService do
 
       member = create :group_member, :invited, group: group
 
-      expect { described_class.new(member, member_user).execute }
+      expect { described_class.new(member_user).execute(member) }
         .not_to change { NotificationSetting.count }
     end
   end
@@ -53,7 +53,7 @@ describe Members::AuthorizedDestroyService do
     it "doesn't destroy member notification_settings" do
       member = create(:project_member, user: member_user, requested_at: Time.now)
 
-      expect { described_class.new(member, member_user).execute }
+      expect { described_class.new(member_user).execute(member) }
         .not_to change { NotificationSetting.count }
     end
   end
@@ -71,7 +71,7 @@ describe Members::AuthorizedDestroyService do
       merge_request = create :merge_request, target_project: group_project, source_project: group_project, assignee: member_user
       create :merge_request, target_project: project, source_project: project, assignee: member_user
 
-      expect { described_class.new(member, member_user).execute }
+      expect { described_class.new(member_user).execute(member) }
         .to change { number_of_assigned_issuables(member_user) }.from(4).to(2)
 
       expect(issue.reload.assignee_ids).to be_empty
@@ -82,7 +82,7 @@ describe Members::AuthorizedDestroyService do
       group.add_developer(member_user)
       member = group.members.find_by(user_id: member_user.id)
 
-      expect { described_class.new(member, member_user).execute }
+      expect { described_class.new(member_user).execute(member) }
         .to change { member_user.notification_settings.count }.by(-1)
     end
   end
@@ -98,12 +98,12 @@ describe Members::AuthorizedDestroyService do
       create :issue, project: project, assignees: [member_user]
       create :merge_request, target_project: project, source_project: project, assignee: member_user
 
-      expect { described_class.new(member, member_user).execute }
+      expect { described_class.new(member_user).execute(member) }
         .to change { number_of_assigned_issuables(member_user) }.from(2).to(0)
     end
 
     it 'destroys member notification_settings' do
-      expect { described_class.new(member, member_user).execute }
+      expect { described_class.new(member_user).execute(member) }
         .to change { member_user.notification_settings.count }.by(-1)
     end
   end

spec/services/members/destroy_service_spec.rb

 require 'spec_helper'
 
 describe Members::DestroyService do
-  let(:user) { create(:user) }
+  let(:current_user) { create(:user) }
   let(:member_user) { create(:user) }
   let(:project) { create(:project, :public) }
   let(:group) { create(:group, :public) }
 
   shared_examples 'a service raising ActiveRecord::RecordNotFound' do
     it 'raises ActiveRecord::RecordNotFound' do
-      expect { described_class.new(source, user, params).execute }.to raise_error(ActiveRecord::RecordNotFound)
+      expect { described_class.new(source, current_user).execute(member) }.to raise_error(ActiveRecord::RecordNotFound)
     end
   end
 
   shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
     it 'raises Gitlab::Access::AccessDeniedError' do
-      expect { described_class.new(source, user, params).execute }.to raise_error(Gitlab::Access::AccessDeniedError)
+      expect { described_class.new(source, current_user).execute(member) }.to raise_error(Gitlab::Access::AccessDeniedError)
     end
   end
 
   shared_examples 'a service destroying a member' do
     it 'destroys the member' do
-      expect { described_class.new(source, user, params).execute }.to change { source.members.count }.by(-1)
+      expect { described_class.new(source, current_user).execute(member) }.to change { source.members.count }.by(-1)
     end
+  end
 
-    context 'when the given member is an access requester' do
-      before do
-        source.members.find_by(user_id: member_user).destroy
-        source.update_attributes(request_access_enabled: true)
-        source.request_access(member_user)
-      end
-      let(:access_requester) { source.requesters.find_by(user_id: member_user) }
-
-      it_behaves_like 'a service raising ActiveRecord::RecordNotFound'
-
-      %i[requesters all].each do |scope|
-        context "and #{scope} scope is passed" do
-          it 'destroys the access requester' do
-            expect { described_class.new(source, user, params).execute(scope) }.to change { source.requesters.count }.by(-1)
-          end
-
-          it 'calls Member#after_decline_request' do
-            expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(access_requester)
-
-            described_class.new(source, user, params).execute(scope)
-          end
-
-          context 'when current user is the member' do
-            it 'does not call Member#after_decline_request' do
-              expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(access_requester)
-
-              described_class.new(source, member_user, params).execute(scope)
-            end
-          end
-        end
-      end
+  shared_examples 'a service destroying an access requester' do
+    it 'destroys the access requester' do
+      expect { described_class.new(source, current_user).execute(access_requester) }.to change { source.requesters.count }.by(-1)
     end
-  end
 
-  context 'when no member are found' do
-    let(:params) { { user_id: 42 } }
+    it 'calls Member#after_decline_request' do
+      expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(access_requester)
 
-    it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
-      let(:source) { project }
+      described_class.new(source, current_user).execute(access_requester)
     end
 
-    it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
-      let(:source) { group }
+    context 'when current user is the member' do
+      it 'does not call Member#after_decline_request' do
+        expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(access_requester)
+
+        described_class.new(source, member_user).execute(access_requester)
+      end
     end
   end
 
-  context 'when a member is found' do
+  context 'with a member' do
     before do
       project.add_developer(member_user)
       group.add_developer(member_user)
     end
-    let(:params) { { user_id: member_user.id } }
+    let(:member) { source.members.find_by(user_id: member_user.id) }
 
     context 'when current user cannot destroy the given member' do
       it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
@@ -88,8 +63,8 @@ describe Members::DestroyService do
 
     context 'when current user can destroy the given member' do
       before do
-        project.add_master(user)
-        group.add_owner(user)
+        project.add_master(current_user)
+        group.add_owner(current_user)
       end
 
       it_behaves_like 'a service destroying a member' do
@@ -99,14 +74,42 @@ describe Members::DestroyService do
       it_behaves_like 'a service destroying a member' do
         let(:source) { group }
       end
+    end
+  end
 
-      context 'when given a :id' do
-        let(:params) { { id: project.members.find_by!(user_id: user.id).id } }
+  context 'with an access requester' do
+    before do
+      project.update_attributes(request_access_enabled: true)
+      group.update_attributes(request_access_enabled: true)
+      project.request_access(member_user)
+      group.request_access(member_user)
+    end
+    let(:access_requester) { source.requesters.find_by(user_id: member_user.id) }
 
-        it 'destroys the member' do
-          expect { described_class.new(project, user, params).execute }
-            .to change { project.members.count }.by(-1)
-        end
+    context 'when current user cannot destroy the given access requester' do
+      it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+        let(:source) { project }
+        let(:member) { access_requester }
+      end
+
+      it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+        let(:source) { group }
+        let(:member) { access_requester }
+      end
+    end
+
+    context 'when current user can destroy the given access requester' do
+      before do
+        project.add_master(current_user)
+        group.add_owner(current_user)
+      end
+
+      it_behaves_like 'a service destroying an access requester' do
+        let(:source) { project }
+      end
+
+      it_behaves_like 'a service destroying an access requester' do
+        let(:source) { group }
       end
     end
   end

spec/services/members/update_service_spec.rb

+require 'spec_helper'
+
+describe Members::UpdateService do
+  let(:project) { create(:project, :public) }
+  let(:group) { create(:group, :public) }
+  let(:current_user) { create(:user) }
+  let(:member_user) { create(:user) }
+  let(:permission) { :update }
+  let(:member) { source.members_and_requesters.find_by!(user_id: member_user.id) }
+  let(:params) do
+    { access_level: Gitlab::Access::MASTER }
+  end
+
+  shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
+    it 'raises Gitlab::Access::AccessDeniedError' do
+      expect { described_class.new(source, current_user, params).execute(member, permission: permission) }
+        .to raise_error(Gitlab::Access::AccessDeniedError)
+    end
+  end
+
+  shared_examples 'a service updating a member' do
+    it 'updates the member' do
+      updated_member = described_class.new(source, current_user, params).execute(member, permission: permission)
+
+      expect(updated_member).to be_valid
+      expect(updated_member.access_level).to eq(Gitlab::Access::MASTER)
+    end
+  end
+
+  before do
+    project.add_developer(member_user)
+    group.add_developer(member_user)
+  end
+
+  context 'when current user cannot update the given member' do
+    it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+      let(:source) { project }
+    end
+
+    it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+      let(:source) { group }
+    end
+  end
+
+  context 'when current user can update the given member' do
+    before do
+      project.add_master(current_user)
+      group.add_owner(current_user)
+    end
+
+    it_behaves_like 'a service updating a member' do
+      let(:source) { project }
+    end
+
+    it_behaves_like 'a service updating a member' do
+      let(:source) { group }
+    end
+  end
+end