Commit 3158f57d authored by Rémy Coutable's avatar Rémy Coutable

Improve Members::DestroyService

Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 958815a0
...@@ -15,18 +15,16 @@ module MembershipActions ...@@ -15,18 +15,16 @@ module MembershipActions
end end
def leave def leave
@member = membershipable.members.find_by(user_id: current_user) || Members::DestroyService.new(membershipable, current_user, user_id: current_user.id).execute(:all)
membershipable.requesters.find_by(user_id: current_user)
Members::DestroyService.new(@member, current_user).execute
source_type = @member.real_source_type.humanize(capitalize: false) source_type = membershipable.class.to_s.humanize(capitalize: false)
notice = notice =
if @member.request? if @member.request?
"Your access request to the #{source_type} has been withdrawn." "Your access request to the #{source_type} has been withdrawn."
else else
"You left the \"#{@member.source.human_name}\" #{source_type}." "You left the \"#{membershipable.human_name}\" #{source_type}."
end end
redirect_path = @member.request? ? @member.source : [:dashboard, @member.real_source_type.tableize] redirect_path = @member.request? ? @member.source : [:dashboard, membershipable.class.to_s.tableize]
redirect_to redirect_path, notice: notice redirect_to redirect_path, notice: notice
end end
......
...@@ -40,10 +40,7 @@ class Groups::GroupMembersController < Groups::ApplicationController ...@@ -40,10 +40,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
end end
def destroy def destroy
@group_member = @group.members.find_by(id: params[:id]) || Members::DestroyService.new(@group, current_user, user_id: params[:id]).execute(:all)
@group.requesters.find_by(id: params[:id])
Members::DestroyService.new(@group_member, current_user).execute
respond_to do |format| respond_to do |format|
format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' } format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' }
......
...@@ -55,10 +55,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController ...@@ -55,10 +55,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
end end
def destroy def destroy
@project_member = @project.members.find_by(id: params[:id]) || Members::DestroyService.new(@project, current_user, user_id: params[:id]).execute(:all)
@project.requesters.find_by(id: params[:id])
Members::DestroyService.new(@project_member, current_user).execute
respond_to do |format| respond_to do |format|
format.html do format.html do
......
module Members module Members
class DestroyService < BaseService class DestroyService < BaseService
attr_accessor :member, :current_user include MembersHelper
def initialize(member, current_user) attr_accessor :source
@member = member
ALLOWED_SCOPES = %i[members requesters all]
def initialize(source, current_user, params = {})
@source = source
@current_user = current_user @current_user = current_user
@params = params
end end
def execute def execute(scope = :members)
unless member && can?(current_user, "destroy_#{member.type.underscore}".to_sym, member) raise "scope :#{scope} is not allowed!" unless ALLOWED_SCOPES.include?(scope)
raise Gitlab::Access::AccessDeniedError
end member = find_member(scope)
raise Gitlab::Access::AccessDeniedError if cannot_destroy_member?(member)
AuthorizedDestroyService.new(member, current_user).execute AuthorizedDestroyService.new(member, current_user).execute
end end
private
def find_member(scope)
case scope
when :all
source.members.find_by(user_id: params[:user_id]) ||
source.requesters.find_by!(user_id: params[:user_id])
else
source.public_send(scope).find_by!(user_id: params[:user_id])
end
end
def cannot_destroy_member?(member)
!member || !can?(current_user, action_member_permission(:destroy, member), member)
end
end end
end end
...@@ -75,9 +75,7 @@ module API ...@@ -75,9 +75,7 @@ module API
required_attributes! [:user_id] required_attributes! [:user_id]
source = find_source(source_type, params[:id]) source = find_source(source_type, params[:id])
access_requester = source.requesters.find_by!(user_id: params[:user_id]) ::Members::DestroyService.new(source, current_user, declared(params)).execute(:requesters)
::Members::DestroyService.new(access_requester, current_user).execute
end end
end end
end end
......
...@@ -65,6 +65,14 @@ module API ...@@ -65,6 +65,14 @@ module API
# for both project and group members in 9.0! # for both project and group members in 9.0!
conflict!('Member already exists') if source_type == 'group' && member conflict!('Member already exists') if source_type == 'group' && member
access_requester = source.requesters.find_by(user_id: params[:user_id])
if access_requester
# We delete a potential access requester before creating the new member.
# We pass current_user = access_requester so that the requester doesn't
# receive a "access denied" email.
::Members::DestroyService.new(source, access_requester.user, params).execute(:requesters)
end
unless member unless member
member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at]) member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
end end
...@@ -134,7 +142,7 @@ module API ...@@ -134,7 +142,7 @@ module API
if member.nil? if member.nil?
{ message: "Access revoked", id: params[:user_id].to_i } { message: "Access revoked", id: params[:user_id].to_i }
else else
::Members::DestroyService.new(member, current_user).execute ::Members::DestroyService.new(source, current_user, params).execute
present member.user, with: Entities::Member, member: member present member.user, with: Entities::Member, member: member
end end
......
...@@ -195,7 +195,7 @@ describe API::AccessRequests, api: true do ...@@ -195,7 +195,7 @@ describe API::AccessRequests, api: true do
end end
context 'when authenticated as the access requester' do context 'when authenticated as the access requester' do
it 'returns 200' do it 'deletes the access requester' do
expect do expect do
delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", access_requester) delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", access_requester)
...@@ -205,7 +205,7 @@ describe API::AccessRequests, api: true do ...@@ -205,7 +205,7 @@ describe API::AccessRequests, api: true do
end end
context 'when authenticated as a master/owner' do context 'when authenticated as a master/owner' do
it 'returns 200' do it 'deletes the access requester' do
expect do expect do
delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", master) delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{access_requester.id}", master)
...@@ -213,6 +213,16 @@ describe API::AccessRequests, api: true do ...@@ -213,6 +213,16 @@ describe API::AccessRequests, api: true do
end.to change { source.requesters.count }.by(-1) end.to change { source.requesters.count }.by(-1)
end end
context 'user_id matches a member' do
it 'returns 404' do
expect do
delete api("/#{source_type.pluralize}/#{source.id}/access_requests/#{developer.id}", master)
expect(response).to have_http_status(404)
end.not_to change { source.requesters.count }
end
end
context 'user_id does not match an existing access requester' do context 'user_id does not match an existing access requester' do
it 'returns 404' do it 'returns 404' do
expect do expect do
......
...@@ -2,70 +2,102 @@ require 'spec_helper' ...@@ -2,70 +2,102 @@ require 'spec_helper'
describe Members::DestroyService, services: true do describe Members::DestroyService, services: true do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project) } let(:member_user) { create(:user) }
let!(:member) { create(:project_member, source: project) } let(:project) { create(:project, :public) }
let(:group) { create(:group, :public) }
context 'when member is nil' do shared_examples 'a service raising ActiveRecord::RecordNotFound' do
before do it 'raises ActiveRecord::RecordNotFound' do
project.team << [user, :developer] expect { described_class.new(source, user, params).execute }.to raise_error(ActiveRecord::RecordNotFound)
end end
end
it 'does not destroy the member' do shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
expect { destroy_member(nil, user) }.to raise_error(Gitlab::Access::AccessDeniedError) it 'raises Gitlab::Access::AccessDeniedError' do
expect { described_class.new(source, user, params).execute }.to raise_error(Gitlab::Access::AccessDeniedError)
end end
end end
context 'when current user cannot destroy the given member' do shared_examples 'a service destroying a member' do
before do it 'destroys the member' do
project.team << [user, :developer] expect { described_class.new(source, user, params).execute }.to change { source.members.count }.by(-1)
end end
it 'does not destroy the member' do context 'when the given member is an access requester' do
expect { destroy_member(member, user) }.to raise_error(Gitlab::Access::AccessDeniedError) before do
source.members.find_by(user_id: member_user).destroy
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
end end
end end
context 'when current user can destroy the given member' do context 'when no member are found' do
before do let(:params) { { user_id: 42 } }
project.team << [user, :master]
it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
let(:source) { project }
end end
it 'destroys the member' do it_behaves_like 'a service raising ActiveRecord::RecordNotFound' do
destroy_member(member, user) let(:source) { group }
end
end
expect(member).to be_destroyed context 'when a member is found' do
before do
project.team << [member_user, :developer]
group.add_developer(member_user)
end end
let(:params) { { user_id: member_user.id } }
context 'when the given member is a requester' do context 'when current user cannot destroy the given member' do
before do it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
member.update_column(:requested_at, Time.now) let(:source) { project }
end end
it 'calls Member#after_decline_request' do it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member) let(:source) { group }
destroy_member(member, user)
end end
end
context 'when current user is the member' do context 'when current user can destroy the given member' do
it 'does not call Member#after_decline_request' do before do
expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) project.team << [user, :master]
group.add_owner(user)
destroy_member(member, member.user)
end
end end
context 'when current user is the member and ' do it_behaves_like 'a service destroying a member' do
it 'does not call Member#after_decline_request' do let(:source) { project }
expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) end
destroy_member(member, member.user) it_behaves_like 'a service destroying a member' do
end let(:source) { group }
end end
end end
end end
def destroy_member(member, user)
Members::DestroyService.new(member, user).execute
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment