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>

19 files changed, 277 insertions, 217 deletions

diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index c6b1e443de6..a6f1509b451 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/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,14 +37,18 @@ module MembershipActions
   end
 
   def approve_access_request
-    Members::ApproveAccessRequestService.new(membershipable, current_user, params).execute
+    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?
diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb
index 2c371e76313..1efd07835e2 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/app/controllers/groups/group_members_controller.rb
@@ -28,12 +28,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)
-
-    @group_member.update_attributes(member_params)
   end
 
   def resend_invite
diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb
index d7372beb9d3..06388055d52 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/app/controllers/projects/project_members_controller.rb
@@ -27,12 +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)
-
-    @project_member.update_attributes(member_params)
   end
 
   def resend_invite
diff --git a/app/models/member.rb b/app/models/member.rb
index 2d17795e62d..ba040bbeff0 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -145,9 +145,8 @@ class Member < ActiveRecord::Base
         ::Members::ApproveAccessRequestService.new(
           source,
           current_user,
-          id: member.id,
           access_level: access_level
-        ).execute
+        ).execute(member)
       else
         member.save
       end
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
diff --git a/app/workers/remove_expired_members_worker.rb b/app/workers/remove_expired_members_worker.rb
index d80b3b15840..c3974c5ea79 100644
--- a/app/workers/remove_expired_members_worker.rb
+++ b/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
diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb
index 60ae5e6b9a2..23ea05e1105 100644
--- a/lib/api/access_requests.rb
+++ b/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
diff --git a/lib/api/members.rb b/lib/api/members.rb
index bc1de37284a..6367cde04bb 100644
--- a/lib/api/members.rb
+++ b/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
diff --git a/lib/api/v3/members.rb b/lib/api/v3/members.rb
index d7bde8ceb89..dddc7d15d42 100644
--- a/lib/api/v3/members.rb
+++ b/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
diff --git a/spec/services/members/approve_access_request_service_spec.rb b/spec/services/members/approve_access_request_service_spec.rb
index b3018169a1c..90c81c413a4 100644
--- a/spec/services/members/approve_access_request_service_spec.rb
+++ b/spec/services/members/approve_access_request_service_spec.rb
@@ -1,70 +1,57 @@
 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
diff --git a/spec/services/members/authorized_destroy_service_spec.rb b/spec/services/members/authorized_destroy_service_spec.rb
index 9cf6f64a078..db3435398bd 100644
--- a/spec/services/members/authorized_destroy_service_spec.rb
+++ b/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
diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb
index 91152df3ad9..7464d73fd26 100644
--- a/spec/services/members/destroy_service_spec.rb
+++ b/spec/services/members/destroy_service_spec.rb
@@ -1,80 +1,55 @@
 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
diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb
new file mode 100644
index 00000000000..17c5f643f23
--- /dev/null
+++ b/spec/services/members/update_service_spec.rb
@@ -0,0 +1,59 @@
+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